Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new --new flag that allows users to create a new Mermaid diagram file and immediately launch the interactive editor. The implementation includes interactive graph type selection, automatic file naming with collision avoidance, and directory creation.
Key changes:
- Added
--newflag to create new diagrams with interactive editor launch - Implemented graph type selection using the
dialoguercrate for interactive prompts - Added file path collision detection with automatic renaming (e.g.,
diagram1.mmd,diagram2.mmd)
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/cli.rs | Added --new flag, GraphType enum, select_graph_type() and ensure_unique_path() functions, and run_new() implementation |
| Cargo.toml | Added dialoguer dependency and release profile optimizations |
| Cargo.lock | Updated dependencies to include dialoguer and related transitive dependencies, plus general dependency version updates |
Comments suppressed due to low confidence (1)
src/cli.rs:218
- The
run_render_or_edit_syncfunction doesn't check for thecli.newflag, which means it will silently ignore--newand fall through torun_render. This should bail with an error message similar to the--editcheck to inform users that--newrequires the 'server' feature.
pub fn run_render_or_edit_sync(cli: RenderArgs) -> Result<()> {
if cli.edit {
bail!("--edit requires the 'server' feature to be enabled");
} else {
run_render(cli)?;
}
Ok(())
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if output.is_some() { | ||
| bail!("--new does not support specifying an output file"); | ||
| } | ||
|
|
||
| if output_format.is_some() { | ||
| bail!("--new does not support selecting an output format"); | ||
| } | ||
|
|
||
| if png { | ||
| bail!("--new does not support the --png flag"); | ||
| } | ||
|
|
There was a problem hiding this comment.
These runtime checks are redundant because lines 67 of the RenderArgs struct already define conflicts_with_all = [\"output\", \"output_format\", \"png\", \"edit\"] for the new flag. Clap will prevent these combinations at parse time, so these runtime checks will never be reached and can be removed.
| if output.is_some() { | |
| bail!("--new does not support specifying an output file"); | |
| } | |
| if output_format.is_some() { | |
| bail!("--new does not support selecting an output format"); | |
| } | |
| if png { | |
| bail!("--new does not support the --png flag"); | |
| } |
| Some(ext) => format!("{stem}{counter}.{ext}"), | ||
| None => format!("{stem}{counter}"), |
There was a problem hiding this comment.
[nitpick] Missing separator between stem and counter in the generated filename. When a file diagram.mmd exists, this creates diagram1.mmd instead of diagram-1.mmd or diagram_1.mmd, which may be less readable. Consider adding a separator like - or _ for better file naming conventions.
| Some(ext) => format!("{stem}{counter}.{ext}"), | |
| None => format!("{stem}{counter}"), | |
| Some(ext) => format!("{stem}-{counter}.{ext}"), | |
| None => format!("{stem}-{counter}"), |
| Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { | ||
| bail!( | ||
| "diagram '{}' already exists; refusing to overwrite", | ||
| target_path.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
This error case at lines 315-320 is unreachable because ensure_unique_path() at line 307 already guarantees that target_path doesn't exist. The AlreadyExists error can only occur in race condition scenarios where the file is created between the ensure_unique_path() check and the open() call.
| Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { | |
| bail!( | |
| "diagram '{}' already exists; refusing to overwrite", | |
| target_path.display() | |
| ); | |
| } |
|
|
||
| fn initial_contents(self) -> &'static str { | ||
| match self { | ||
| GraphType::Flowchart => "graph TD\nHello World!", |
There was a problem hiding this comment.
[nitpick] The initial diagram content uses a raw string with \\n which may not render correctly on Windows systems where line endings are \\r\\n. Consider using a multi-line string literal or platform-specific line endings.
|
these seem like nitpicks, added redundancy isnt too big of a deal |
No description provided.