Merged
Conversation
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Rust workspace with two member crates ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant RunArgs
participant RunLoop
participant ConfigLoader as Config
participant Watcher
participant Templates
participant Cargo
User->>CLI: run --path . [--watch]
CLI->>RunArgs: parse args
RunArgs->>RunLoop: new(canonical_path, config_path)
activate RunLoop
RunLoop->>ConfigLoader: load config & deps
ConfigLoader-->>RunLoop: Config + deps
alt watch mode
RunLoop->>Watcher: watch config & dep paths
Watcher-->>RunLoop: change event
loop on change
RunLoop->>Templates: render Cargo.toml & main.rs
Templates-->>RunLoop: rendered files
RunLoop->>Cargo: spawn/manage cargo run
Cargo-->>User: build/run output
end
else one-shot
RunLoop->>Templates: render files
RunLoop->>Cargo: cargo run once
Cargo-->>User: build/run output
end
deactivate RunLoop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
crates/cli/src/lint/mod.rs (1)
3-11: Consider includingCommonArgsfor consistency.Unlike
BuildArgsandConfigArgs,LintArgsdoesn't flattenCommonArgs. If the lint command might need access to the configuration file (e.g., for lint rules or settings), consider adding:+use crate::common::CommonArgs; + #[derive(Args)] pub struct LintArgs { #[arg(long)] clippy: bool, #[arg(long)] dylint: bool, #[arg(long)] pattern: Option<String>, + #[command(flatten)] + common_args: CommonArgs, }If lint is intentionally standalone without configuration, this can be ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lint/mod.rs` around lines 3 - 11, LintArgs currently omits CommonArgs unlike BuildArgs/ConfigArgs; to add consistent CLI config handling, add a field like `common: CommonArgs` to the `LintArgs` struct and annotate it to flatten the sub-options (e.g., using the clap flatten attribute such as `#[clap(flatten)]` or the project's existing flatten attribute pattern), then update any places that construct or pattern-match on `LintArgs` to account for the new `common` field so the lint command can access configuration options.crates/module-parser/src/metadata.rs (2)
6-6: Prefer&Pathover&PathBufin function signatures.Using
&PathBufis redundant since&Pathis more general and accepts both&Pathand&PathBufvia deref coercion.♻️ Suggested change
-pub fn get_module_name_from_crate(path: &PathBuf) -> anyhow::Result<HashMap<String, ConfigModule>> { +pub fn get_module_name_from_crate(path: &Path) -> anyhow::Result<HashMap<String, ConfigModule>> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/metadata.rs` at line 6, Change the function signature of get_module_name_from_crate to accept a &Path instead of &PathBuf: update the signature in metadata.rs and any direct callers to pass their Path/PathBuf (relying on deref coercion), and adjust any documentation/comments referring to the parameter type; ensure imports remain the same and run cargo build/tests to catch any remaining mismatched references to get_module_name_from_crate.
20-22: Silently swallowing errors may hide configuration issues.Printing to stderr and continuing means callers have no indication that some modules failed to parse. Consider returning partial results with a list of errors, or at least logging with a structured logger.
♻️ Alternative: collect warnings
+ let mut warnings = Vec::new(); for pkg in res.packages { for t in &pkg.targets { if t.is_lib() { match super::module_rs::retrieve_module_rs(&pkg, t.clone()) { Ok(module) => { members.insert(module.0, module.1); } Err(e) => { - eprintln!("{e}"); + warnings.push(format!("{}: {e}", pkg.name)); } }; } } } + for w in &warnings { + tracing::warn!("{w}"); + } Ok(members)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/metadata.rs` around lines 20 - 22, The Err arm that currently does eprintln!("{e}") should not silently swallow parse failures; replace the eprintln call in that Err(e) branch with logic that (1) records the error into a collection (e.g., push e or (module_id, e) into a Vec<ParseError>) and (2) emits a structured log (e.g., tracing::error!(error = %e, "module parse failed")). Then update the surrounding function's signature (the function that contains this match/Err branch) to return either a Result with partial successes and an errors Vec or a tuple/struct like (Vec<ModuleMetadata>, Vec<ParseError>) so callers can see which modules failed; ensure any callers are updated to handle the new return type and propagate or surface the collected errors instead of ignoring them.crates/cli/src/run/mod.rs (1)
29-38:canonicalize()fails if path doesn't exist.Both
configandpathare canonicalized, which requires the paths to exist. If a user runs the command before creating the config file or target directory, the error message "can't canonicalize path" is unhelpful.Consider checking existence first and providing a clearer error, or deferring canonicalization until after validation.
♻️ Suggested improvement
let config_path = self .common_args .config .canonicalize() - .context("can't canonicalize path")?; + .with_context(|| format!("config file not found: {}", self.common_args.config.display()))?; let path = self .path .canonicalize() - .context("can't canonicalize path")?; + .with_context(|| format!("module path not found: {}", self.path.display()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/mod.rs` around lines 29 - 38, The current use of canonicalize() on self.common_args.config and self.path (producing config_path and path) will error if those paths don't exist; change the logic in run::mod.rs to first check existence (e.g., using exists or metadata) for self.common_args.config and self.path and return a clearer error message if missing, or defer calling canonicalize() until after validation/creation of the file/directory; update the error contexts from generic "can't canonicalize path" to specific messages referencing config_path / self.common_args.config and path / self.path so users see which resource is missing.crates/cli/src/setup/module.rs (1)
9-13: Minor inconsistency:todo!()vsunimplemented!().Other placeholder implementations in this PR use
unimplemented!("Not implemented yet")(e.g.,tools.rs,config.rs,build/mod.rs), while this usestodo!("gimme time"). Consider aligning for consistency.♻️ Suggested change
pub fn run(&self) -> anyhow::Result<()> { - todo!("gimme time") + unimplemented!("Not implemented yet") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/module.rs` around lines 9 - 13, The ModuleArgs::run implementation uses todo!("gimme time") which is inconsistent with other placeholders; replace the todo! call in the ModuleArgs::run function with the project-standard unimplemented!("Not implemented yet") so the placeholder matches other modules (see ModuleArgs::run, tools.rs, config.rs, build::mod).crates/module-parser/src/lib.rs (1)
5-6: Consider explicit re-exports instead of glob exports.Glob re-exports (
pub use config::*) can make it harder to track the public API surface and may accidentally expose internal items if the submodules grow. Explicit exports provide better control.This is a minor suggestion for a library crate where API clarity matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/lib.rs` around lines 5 - 6, Replace the glob re-exports `pub use config::*` and `pub use metadata::*` with explicit named re-exports to make the crate's public API clear: inspect the `config` and `metadata` modules and export only the intended public types/functions (e.g., `pub use config::{TypeA, parse_config, ConfigError}` and `pub use metadata::{Metadata, MetadataBuilder}`) by listing their identifiers in `lib.rs` instead of `*`; ensure any items you want hidden remain non-`pub` in the submodules and update the named lists when you intentionally add new public items.crates/cli/src/setup/init.rs (1)
31-42: Edge case: path ending with/or root paths may fail unexpectedly.When
self.pathis/or ends with a trailing separator,file_name()returnsNone, causing a vague "path is strange" error. Consider canonicalizing the path first or providing a clearer error message.♻️ Suggested improvement
+ let path = self.path.canonicalize().unwrap_or_else(|_| self.path.clone()); let name = self - .path + path .file_name() - .context("path is strange")? + .context("cannot derive project name: path has no final component")? .to_str() - .context("name is strange")?; + .context("cannot derive project name: path contains invalid UTF-8")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/setup/init.rs` around lines 31 - 42, The code assumes self.path.file_name() is always Some, which fails for root or trailing-separator paths; after creating the directory, canonicalize the path and use the canonicalized Path (e.g., let canon = self.path.canonicalize().context("failed to canonicalize path")?) before calling canon.file_name(), and replace the vague "path is strange" message with a clearer one like "cannot determine directory name from path (root or trailing separator?)"; update uses of file_name()/to_str() to operate on the canonicalized Path and propagate a more descriptive error.crates/module-parser/src/module_rs.rs (1)
62-102: Validate non‑string module args to avoid silent misconfigurations.
Right now non-stringname/depsentries are silently ignored, which can mask config errors.♻️ Suggested validation
if meta.path.is_ident("name") { let value = meta.value()?; let lit: Lit = value.parse()?; - if let Lit::Str(lit_str) = lit { - name = Some(lit_str.value()); - } + let Lit::Str(lit_str) = lit else { + return Err(meta.error("name must be a string literal")); + }; + name = Some(lit_str.value()); } else if meta.path.is_ident("deps") { @@ let lit: Lit = content.parse()?; - if let Lit::Str(lit_str) = lit { - deps.push(lit_str.value()); - } + let Lit::Str(lit_str) = lit else { + return Err(meta.error("deps entries must be string literals")); + }; + deps.push(lit_str.value());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/module_rs.rs` around lines 62 - 102, The parse_module_args function currently ignores non-string values for "name" and "deps" (and silently accepts bad tokens in "capabilities"); update parse_module_args to validate and error on invalid types instead of skipping: when parsing meta.path "name" or each entry in "deps" parse the value to a Lit and if it is not Lit::Str return an anyhow::Error (use anyhow::bail or context) that includes which key ("name" or "deps") and the offending token; likewise, for "capabilities" ensure each entry parses to a syn::Ident and bail with a clear message if not; return these errors so ModuleInfo is only constructed when values are valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 23: Replace the wildcard dependency for liquid with a pinned semver
version: locate the liquid entry in Cargo.toml (the line currently reading
liquid = { version = "*" }) and change it to a specific stable release such as
"0.26.11" or a bounded semver like "^0.26.11" (or ">=0.26.11, <0.27.0") to
ensure reproducible builds and avoid accidental breaking upgrades.
In `@crates/cli/src/run/mod.rs`:
- Around line 17-19: The doc comment above the command-line flag `otel` is
incorrect (it reads "Watch for changes"); update the doc comment for the `otel`
field (in the struct containing `#[arg(long)] otel: bool`) to a correct
description such as enabling OpenTelemetry tracing (or similar accurate text),
and ensure the `watch` flag's comment remains "Watch for changes" if present so
the two flags are not confused.
In `@crates/cli/src/run/run_loop.rs`:
- Around line 33-115: The watcher loop currently uses while let Ok(Ok(event)) =
fs_rx.recv(), which exits on Ok(Err(_)) and shuts down watch mode on any watcher
error; change the loop in run (in run_loop.rs) to explicitly match fs_rx.recv()
in a loop and handle three cases: Ok(Ok(event)) -> existing event processing
(use is_config_change, reload config, reconcile watched_paths, send
RunSignal::Rerun, etc.), Ok(Err(err)) -> log the watcher error (eprintln! or
processLogger) and continue without shutting down the runner, Err(_) -> break
the loop (channel closed) and proceed to send RunSignal::Stop and join
runner_handle; reference fs_rx, signal_tx, RunSignal, watched_paths,
current_deps and the config reloading/generation logic when moving code into the
Ok(Ok(event)) branch.
In `@crates/cli/src/run/templates.rs`:
- Around line 61-64: The generated use statements break on crate names with
hyphens; update the mapping over dependencies.keys() (the closure that builds
the use lines) to sanitize crate names by replacing '-' with '_' before
formatting (e.g., compute a sanitized_name from name via replace('-', '_') and
then format!("use {sanitized_name} as _;\n")). This ensures valid Rust
identifiers are emitted for crates like "my-module".
- Around line 66-69: The template currently uses
config_path.display().to_string() which on Windows yields backslashes that
become problematic in generated Rust string literals; update the liquid::object!
entry for "config_path" to produce a safe string by normalizing or escaping
Windows separators — for example replace config_path.display().to_string() with
something like config_path.to_string_lossy().replace('\\','/') (or alternatively
.to_string_lossy().replace('\\','\\\\') to escape backslashes) so the generated
code compiles on Windows.
In `@crates/cli/src/setup/mod.rs`:
- Around line 20-35: The match in SetupCommand::run currently delegates to
tools::ToolsArgs::run and config::ConfigArgs::run which call unimplemented!()
and will panic; update SetupCommand::run to detect the Tools and Config variants
and return a user-facing error (e.g., Err(anyhow::anyhow!("'setup tools' is not
implemented yet") or anyhow::bail!("...")) instead of invoking args.run()),
while leaving Init and Module to call their run() methods; reference
SetupCommand::run, tools::ToolsArgs::run, and config::ConfigArgs::run when
making the change.
In `@crates/module-parser/src/config.rs`:
- Around line 10-21: In Config::create_dependencies detect when two modules map
to the same dependency key (the package string after package.replace("-", "_"))
before inserting into the dependencies HashMap: compute the key (e.g., let key =
package.replace("-", "_")), check dependencies.contains_key(&key) and if true
bail! with a clear error that names the conflicting module and key (or otherwise
return an Err) to avoid silent overwrites; otherwise insert the module.metadata
as currently done.
In `@crates/module-parser/src/module_rs.rs`:
- Around line 8-38: In retrieve_module_rs, metadata.path is derived from
src.parent(), which breaks for crates whose lib is not under src; change it to
use the package manifest location instead: set ConfigModuleMetadata.path to
package.manifest_path.parent().map(|p| p.display().to_string()) (preserving the
Option), so the module path reflects the crate root from the Cargo.toml
location; update the assignment inside the ConfigModule creation in
retrieve_module_rs accordingly.
---
Nitpick comments:
In `@crates/cli/src/lint/mod.rs`:
- Around line 3-11: LintArgs currently omits CommonArgs unlike
BuildArgs/ConfigArgs; to add consistent CLI config handling, add a field like
`common: CommonArgs` to the `LintArgs` struct and annotate it to flatten the
sub-options (e.g., using the clap flatten attribute such as `#[clap(flatten)]`
or the project's existing flatten attribute pattern), then update any places
that construct or pattern-match on `LintArgs` to account for the new `common`
field so the lint command can access configuration options.
In `@crates/cli/src/run/mod.rs`:
- Around line 29-38: The current use of canonicalize() on
self.common_args.config and self.path (producing config_path and path) will
error if those paths don't exist; change the logic in run::mod.rs to first check
existence (e.g., using exists or metadata) for self.common_args.config and
self.path and return a clearer error message if missing, or defer calling
canonicalize() until after validation/creation of the file/directory; update the
error contexts from generic "can't canonicalize path" to specific messages
referencing config_path / self.common_args.config and path / self.path so users
see which resource is missing.
In `@crates/cli/src/setup/init.rs`:
- Around line 31-42: The code assumes self.path.file_name() is always Some,
which fails for root or trailing-separator paths; after creating the directory,
canonicalize the path and use the canonicalized Path (e.g., let canon =
self.path.canonicalize().context("failed to canonicalize path")?) before calling
canon.file_name(), and replace the vague "path is strange" message with a
clearer one like "cannot determine directory name from path (root or trailing
separator?)"; update uses of file_name()/to_str() to operate on the
canonicalized Path and propagate a more descriptive error.
In `@crates/cli/src/setup/module.rs`:
- Around line 9-13: The ModuleArgs::run implementation uses todo!("gimme time")
which is inconsistent with other placeholders; replace the todo! call in the
ModuleArgs::run function with the project-standard unimplemented!("Not
implemented yet") so the placeholder matches other modules (see ModuleArgs::run,
tools.rs, config.rs, build::mod).
In `@crates/module-parser/src/lib.rs`:
- Around line 5-6: Replace the glob re-exports `pub use config::*` and `pub use
metadata::*` with explicit named re-exports to make the crate's public API
clear: inspect the `config` and `metadata` modules and export only the intended
public types/functions (e.g., `pub use config::{TypeA, parse_config,
ConfigError}` and `pub use metadata::{Metadata, MetadataBuilder}`) by listing
their identifiers in `lib.rs` instead of `*`; ensure any items you want hidden
remain non-`pub` in the submodules and update the named lists when you
intentionally add new public items.
In `@crates/module-parser/src/metadata.rs`:
- Line 6: Change the function signature of get_module_name_from_crate to accept
a &Path instead of &PathBuf: update the signature in metadata.rs and any direct
callers to pass their Path/PathBuf (relying on deref coercion), and adjust any
documentation/comments referring to the parameter type; ensure imports remain
the same and run cargo build/tests to catch any remaining mismatched references
to get_module_name_from_crate.
- Around line 20-22: The Err arm that currently does eprintln!("{e}") should not
silently swallow parse failures; replace the eprintln call in that Err(e) branch
with logic that (1) records the error into a collection (e.g., push e or
(module_id, e) into a Vec<ParseError>) and (2) emits a structured log (e.g.,
tracing::error!(error = %e, "module parse failed")). Then update the surrounding
function's signature (the function that contains this match/Err branch) to
return either a Result with partial successes and an errors Vec or a
tuple/struct like (Vec<ModuleMetadata>, Vec<ParseError>) so callers can see
which modules failed; ensure any callers are updated to handle the new return
type and propagate or surface the collected errors instead of ignoring them.
In `@crates/module-parser/src/module_rs.rs`:
- Around line 62-102: The parse_module_args function currently ignores
non-string values for "name" and "deps" (and silently accepts bad tokens in
"capabilities"); update parse_module_args to validate and error on invalid types
instead of skipping: when parsing meta.path "name" or each entry in "deps" parse
the value to a Lit and if it is not Lit::Str return an anyhow::Error (use
anyhow::bail or context) that includes which key ("name" or "deps") and the
offending token; likewise, for "capabilities" ensure each entry parses to a
syn::Ident and bail with a clear message if not; return these errors so
ModuleInfo is only constructed when values are valid.
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cli/src/run/run_loop.rs (1)
313-324: Consider avoiding.unwrap()onpath.parent().While the current usage with relative paths like
"src/main.rs"ensuresparent()always returnsSome, usingunwrap()is fragile if future paths are added incorrectly.♻️ Proposed improvement
fn create_file_structure(path: &Path, relative_path: &str, contents: &str) -> anyhow::Result<()> { let path = PathBuf::from(path).join(BASE_PATH).join(relative_path); - fs::create_dir_all(path.parent().unwrap()).context("can't create directory")?; + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).context("can't create directory")?; + } let mut file = fs::OpenOptions::new()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/run_loop.rs` around lines 313 - 324, In create_file_structure, avoid calling unwrap() on path.parent() — instead get the parent directory into a variable (e.g., parent_dir) and convert the Option to a Result with a clear error using ok_or_else or anyhow::bail so you return a meaningful error when no parent exists; then call fs::create_dir_all(parent_dir) and proceed to open/write the file. This removes the panic risk and keeps error handling consistent with the existing .context(...) usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/cli/src/run/run_loop.rs`:
- Around line 33-123: The watcher error handling in run() is already fixed—file
watcher errors are logged and the loop continues (see run(), fs_rx handling, and
cargo_run_loop usage), so no code changes are necessary; leave the event loop,
the eprintln! in the fs_rx recv error branch, and the signal_tx send behavior
as-is.
In `@crates/cli/src/run/templates.rs`:
- Around line 57-72: prepare_cargo_server_main already normalizes Windows
backslashes by replacing '\' with '/' and the crate-name hyphen-to-underscore
normalization is handled upstream in config.rs via package.replace("-", "_"), so
no functional change is needed; add a short inline comment inside
prepare_cargo_server_main referencing the upstream normalization (config.rs and
package.replace("-", "_")) to prevent future confusion and ensure reviewers know
hyphen handling is centralized.
In `@crates/module-parser/src/config.rs`:
- Around line 10-26: Duplicate detection for package keys is already implemented
in Config::create_dependencies: it checks module.metadata.package
(module.metadata.package.clone()), normalizes with package.replace("-", "_"),
and bails if dependencies.contains_key(&package) before inserting into
dependencies; no code changes required—keep the current logic in
create_dependencies as-is and proceed with the approval.
In `@crates/module-parser/src/module_rs.rs`:
- Around line 8-41: The crate root path should be derived from the package
manifest parent rather than the source file parent; in retrieve_module_rs
replace any use of src.parent() for the crate root with
package.manifest_path.parent().map(|p| p.display().to_string()) (as assigned to
crate_root) so ConfigModuleMetadata.path gets the manifest-derived path; locate
this logic in the retrieve_module_rs function and ensure
ConfigModuleMetadata.path uses the crate_root variable.
---
Nitpick comments:
In `@crates/cli/src/run/run_loop.rs`:
- Around line 313-324: In create_file_structure, avoid calling unwrap() on
path.parent() — instead get the parent directory into a variable (e.g.,
parent_dir) and convert the Option to a Result with a clear error using
ok_or_else or anyhow::bail so you return a meaningful error when no parent
exists; then call fs::create_dir_all(parent_dir) and proceed to open/write the
file. This removes the panic risk and keeps error handling consistent with the
existing .context(...) usage.
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/run/run_loop.rs`:
- Around line 84-106: The code updates current_deps and watched_paths even when
generate_server_structure fails, preventing a future identical config from
retriggering generation; change the control flow so that after computing
new_deps (from get_config and create_dependencies) you attempt
generate_server_structure(&self.path, &self.config_path, &new_deps) and only on
its Ok path proceed to compute new_watched = collect_dep_paths(&new_deps,
&self.path), unwatch/watch accordingly, and assign watched_paths = new_watched
and current_deps = new_deps; on Err from generate_server_structure return/skip
updating state so the next config change can still trigger regeneration. Keep
references to generate_server_structure, collect_dep_paths, get_config,
create_dependencies, current_deps, watched_paths, and watcher.watch/unwatch.
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cli/src/run/run_loop.rs (1)
167-189: Coalesce bursty rerun signals to reduce restart thrash.
Multiple file events can queue manyReruns; draining extras before restarting avoids rapid kill/spawn loops.♻️ Suggested refactor
- Ok(RunSignal::Rerun) => { - let _ = child.kill(); - let _ = child.wait(); - break true; - } + Ok(RunSignal::Rerun) => { + // Drain extra reruns; honor a queued Stop. + let mut stop = false; + loop { + match signal_rx.try_recv() { + Ok(RunSignal::Rerun) => continue, + Ok(RunSignal::Stop) + | Err(mpsc::TryRecvError::Disconnected) => { + stop = true; + break; + } + Err(mpsc::TryRecvError::Empty) => break, + } + } + let _ = child.kill(); + let _ = child.wait(); + if stop { + return; + } + break true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/run_loop.rs` around lines 167 - 189, When handling RunSignal::Rerun in the loop that sets rerun, coalesce bursty Rerun events by draining any additional Rerun messages from signal_rx before killing/restarting the child: inside the branch matching Ok(RunSignal::Rerun) (the block that currently calls child.kill() and child.wait()), loop on signal_rx.try_recv() and consume all Ok(RunSignal::Rerun) results (ignore Empty/Disconnected/Stop per existing semantics), then perform the child.kill() and child.wait() once and break true; reference symbols: signal_rx.try_recv(), RunSignal::Rerun, child.kill(), child.wait(), and the rerun loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/run/run_loop.rs`:
- Around line 64-67: The current watch call watches the file path
(watcher.watch(&self.config_path, ...)) which misses atomic-rename saves; change
it to watch the config file's parent directory instead by computing the parent
directory of self.config_path (falling back to self.config_path if no parent)
and pass that directory to watcher.watch with RecursiveMode::NonRecursive,
keeping the existing .context("failed to watch config file")? error handling and
message.
---
Nitpick comments:
In `@crates/cli/src/run/run_loop.rs`:
- Around line 167-189: When handling RunSignal::Rerun in the loop that sets
rerun, coalesce bursty Rerun events by draining any additional Rerun messages
from signal_rx before killing/restarting the child: inside the branch matching
Ok(RunSignal::Rerun) (the block that currently calls child.kill() and
child.wait()), loop on signal_rx.try_recv() and consume all Ok(RunSignal::Rerun)
results (ignore Empty/Disconnected/Stop per existing semantics), then perform
the child.kill() and child.wait() once and break true; reference symbols:
signal_rx.try_recv(), RunSignal::Rerun, child.kill(), child.wait(), and the
rerun loop.
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cli/src/run/run_loop.rs (3)
277-322: Required dependencies silently overwrite user-specified versions.
HashMap::insertreplaces any existing entry. If a user's config specifiestokiooranyhowwith different features or versions, those are silently overwritten. This could cause unexpected behavior if the user relies on specific configurations.Consider using
entry().or_insert()to preserve user-specified deps, or document that these dependencies are managed by the framework and must not be overridden.💡 Alternative using entry API
- dependencies.insert( - "tokio".to_owned(), - ConfigModuleMetadata { - package: Some("tokio".to_owned()), - features: vec!["full".to_owned()], - version: Some("1".to_owned()), - ..Default::default() - }, - ); + dependencies.entry("tokio".to_owned()).or_insert(ConfigModuleMetadata { + package: Some("tokio".to_owned()), + features: vec!["full".to_owned()], + version: Some("1".to_owned()), + ..Default::default() + });Apply similarly to other required dependencies, or if overwriting is intentional, add a comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/run_loop.rs` around lines 277 - 322, The code currently uses HashMap::insert to add framework-required deps which silently overwrite user-specified entries; update the logic that populates the dependencies map (e.g., where required deps like "tokio" and "anyhow" are added—look for variables such as required_deps, required_dependencies, or deps_map in run_loop.rs or the run_loop function) to use the entry API (entry().or_insert(...) or entry().or_insert_with(...)) so existing user-provided versions/features are preserved, or explicitly check contains_key and only insert when absent; apply the same change to all places adding framework-managed deps or add a clear comment if overwriting is intentional.
111-125: Clarify the restart-on-watcher-error strategy.On watcher
unwatch/watchfailures, the code sendsStopto the runner but returnsOk(RunSignal::Rerun). This appears intentional to trigger a full restart at the outer level, but the mixed signals are confusing without explanation.Consider adding a brief comment explaining why a full restart is preferred over continuing with a degraded watch set.
💡 Suggested clarification
for old in watched_paths.difference(&new_watched) { if let Err(err) = watcher.unwatch(old) { eprintln!("failed to unwatch {old:?}: {err}"); + // Watcher state is inconsistent; trigger full restart _ = signal_tx.send(RunSignal::Stop); runner_handle.join().expect("runner thread panicked"); return Ok(RunSignal::Rerun); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/run_loop.rs` around lines 111 - 125, The watcher error handling is sending a Stop to the runner but also returning Ok(RunSignal::Rerun), which is intentional but unclear; add a brief comment in run_loop.rs at the block that sends the Stop (where runner_tx.send(Stop) or similar is called and RunSignal::Rerun is returned) explaining that on watch/unwatch failures we prefer a full restart (Stop the current runner and ask the outer loop to recreate watchers and the runner) rather than continuing with a degraded watcher set to ensure consistent state and avoid concurrent runners or partial watches.
252-268: Prefer&Pathover&PathBuffor function parameters.Using
&PathBufinstead of&Pathis a Rust anti-pattern.&Pathaccepts both&Pathand&PathBuf(viaDeref), providing greater flexibility for callers.♻️ Suggested refactor
-fn get_config(path: &PathBuf, config_path: &PathBuf) -> anyhow::Result<Config> { +fn get_config(path: &Path, config_path: &Path) -> anyhow::Result<Config> { let mut config = get_config_from_path(config_path)?; let mut members = get_module_name_from_crate(path)?; // ... } -fn get_config_from_path(path: &PathBuf) -> anyhow::Result<Config> { +fn get_config_from_path(path: &Path) -> anyhow::Result<Config> { let config = fs::File::open(path).context("config not available")?; serde_saphyr::from_reader(config).context("config not valid") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/run/run_loop.rs` around lines 252 - 268, Several functions in run_loop.rs use &PathBuf for parameters, which is an anti-pattern; change their parameter types to &Path (from std::path::Path) so callers can pass either &Path or &PathBuf. For each function signature that currently takes &PathBuf (look for occurrences in run_loop.rs), replace &PathBuf with &Path, add use std::path::Path if missing, and update call sites to pass the same reference (no cloning needed); ensure any pattern matches or methods that require PathBuf are adjusted (e.g., convert to owned PathBuf only where ownership is required by calling .to_path_buf()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/cli/src/run/run_loop.rs`:
- Around line 277-322: The code currently uses HashMap::insert to add
framework-required deps which silently overwrite user-specified entries; update
the logic that populates the dependencies map (e.g., where required deps like
"tokio" and "anyhow" are added—look for variables such as required_deps,
required_dependencies, or deps_map in run_loop.rs or the run_loop function) to
use the entry API (entry().or_insert(...) or entry().or_insert_with(...)) so
existing user-provided versions/features are preserved, or explicitly check
contains_key and only insert when absent; apply the same change to all places
adding framework-managed deps or add a clear comment if overwriting is
intentional.
- Around line 111-125: The watcher error handling is sending a Stop to the
runner but also returning Ok(RunSignal::Rerun), which is intentional but
unclear; add a brief comment in run_loop.rs at the block that sends the Stop
(where runner_tx.send(Stop) or similar is called and RunSignal::Rerun is
returned) explaining that on watch/unwatch failures we prefer a full restart
(Stop the current runner and ask the outer loop to recreate watchers and the
runner) rather than continuing with a degraded watcher set to ensure consistent
state and avoid concurrent runners or partial watches.
- Around line 252-268: Several functions in run_loop.rs use &PathBuf for
parameters, which is an anti-pattern; change their parameter types to &Path
(from std::path::Path) so callers can pass either &Path or &PathBuf. For each
function signature that currently takes &PathBuf (look for occurrences in
run_loop.rs), replace &PathBuf with &Path, add use std::path::Path if missing,
and update call sites to pass the same reference (no cloning needed); ensure any
pattern matches or methods that require PathBuf are adjusted (e.g., convert to
owned PathBuf only where ownership is required by calling .to_path_buf()).
This was referenced Feb 24, 2026
This was referenced Mar 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores