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>
Contributor
📝 WalkthroughWalkthroughRewires CLI build/run plumbing: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (build/run)
participant Common as common.rs
participant FS as Filesystem
participant Cargo as Cargo
CLI->>Common: start build/run with BuildRunArgs
Common->>FS: canonicalize workspace & config paths
Common->>FS: load config (get_config)
Common->>Common: derive dependencies & generate_server_structure
Common->>Cargo: run cargo via cargo_command (build/run, otel/release)
Cargo-->>Common: exit status
Common-->>CLI: return success or bail with error
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
crates/cli/src/common.rs (2)
97-108: Silent metadata mismatch: modules in config but not in the workspace are left with empty metadata.
get_configonly merges metadata for modules that exist in both the config and the workspace members. If a module is listed in the config but not found in the workspace (e.g., typo in the config, or the crate hasn't been added yet), there's no warning. This could lead to confusing downstream errors.Consider logging a warning for config modules that don't have a matching workspace member.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 97 - 108, get_config currently skips modules present in the config but missing from workspace members, leaving their metadata empty without any notice. Modify get_config (which calls get_config_from_path and get_module_name_from_crate and iterates over config.modules) so when members.remove(module.0.as_str()) returns None you emit a warning (e.g., via log::warn or the project's logger) that the config module (use module.0) has no matching workspace member and therefore no metadata merged; keep existing behavior otherwise. Ensure the warning message includes the module name and concise context to aid debugging.
122-167: Avoid cloning thedependenciesmap; take&mutinstead.
insert_required_depstakes the map by value, forcing a.clone()at the call site (line 177). Accepting&mut HashMapavoids the allocation.♻️ Proposed refactor
-fn insert_required_deps( - mut dependencies: HashMap<String, ConfigModuleMetadata>, -) -> HashMap<String, ConfigModuleMetadata> { +fn insert_required_deps( + dependencies: &mut HashMap<String, ConfigModuleMetadata>, +) { dependencies.insert( "modkit".to_owned(), // ... ); // ... other inserts ... - dependencies }Then update the call site:
+ let mut deps = dependencies.clone(); + insert_required_deps(&mut deps); let cargo_toml = toml::to_string(&CargoToml { - dependencies: insert_required_deps(dependencies.clone()), + dependencies: deps, features, ..Default::default() })Actually, since
generate_server_structuretakesdependenciesby shared reference (&HashMap), the clone is still needed to avoid mutating the caller's data. So the clone cannot be eliminated without a broader signature change. The current approach is acceptable.Also applies to: 176-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 122 - 167, insert_required_deps currently takes ownership of the HashMap causing callers to clone; change its signature to fn insert_required_deps(dependencies: &mut HashMap<String, ConfigModuleMetadata>) and update its body to mutate the passed-in map (remove the final return), then update the caller(s) that currently call insert_required_deps(dependencies.clone()) to pass a mutable reference (&mut dependencies) instead so the extra clone/allocation is eliminated; ensure you update any imports/usages referencing insert_required_deps accordingly.
🤖 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/common.rs`:
- Around line 200-202: The error message passed to path.parent().context(...)
contains a typo ("unreacheable"); update the string to "this should be
unreachable, the parent for the file structure always exists" by editing the
context call used with path.parent() so the message reads "unreachable" instead
of "unreacheable".
---
Nitpick comments:
In `@crates/cli/src/common.rs`:
- Around line 97-108: get_config currently skips modules present in the config
but missing from workspace members, leaving their metadata empty without any
notice. Modify get_config (which calls get_config_from_path and
get_module_name_from_crate and iterates over config.modules) so when
members.remove(module.0.as_str()) returns None you emit a warning (e.g., via
log::warn or the project's logger) that the config module (use module.0) has no
matching workspace member and therefore no metadata merged; keep existing
behavior otherwise. Ensure the warning message includes the module name and
concise context to aid debugging.
- Around line 122-167: insert_required_deps currently takes ownership of the
HashMap causing callers to clone; change its signature to fn
insert_required_deps(dependencies: &mut HashMap<String, ConfigModuleMetadata>)
and update its body to mutate the passed-in map (remove the final return), then
update the caller(s) that currently call
insert_required_deps(dependencies.clone()) to pass a mutable reference (&mut
dependencies) instead so the extra clone/allocation is eliminated; ensure you
update any imports/usages referencing insert_required_deps accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/run/mod.rscrates/cli/src/run/run_loop.rscrates/cli/src/run/templates.rs
💤 Files with no reviewable changes (1)
- crates/cli/src/run/templates.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/cli/src/common.rs (2)
127-128:insert_required_depssignature forces an avoidable clone at the call site.The function takes ownership, inserts entries, and returns the map — but line 182 must call
dependencies.clone()just to satisfy this. Changing to&mut HashMap<...>eliminates the allocation.♻️ Proposed refactor
fn insert_required_deps( - mut dependencies: HashMap<String, ConfigModuleMetadata>, -) -> HashMap<String, ConfigModuleMetadata> { + dependencies: &mut HashMap<String, ConfigModuleMetadata>, +) { dependencies.insert( /* ... */ ); // ... other inserts ... - dependencies }And at the call site (line 181–186):
+ let mut cargo_toml_deps = dependencies.clone(); + insert_required_deps(&mut cargo_toml_deps); let cargo_toml = toml::to_string(&CargoToml { - dependencies: insert_required_deps(dependencies.clone()), + dependencies: cargo_toml_deps, features, ..Default::default() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 127 - 128, The function insert_required_deps currently takes ownership of a HashMap<String, ConfigModuleMetadata>, forcing callers to clone the map; change its signature to take a mutable reference (&mut HashMap<String, ConfigModuleMetadata>) and mutate it in place, updating its internals to use the borrowed map instead of returning a new one, and update all call sites (remove dependencies.clone() and pass &mut dependencies) so no extra allocation is needed; ensure types reference ConfigModuleMetadata consistently and adjust any return values or uses accordingly.
202-203:PathBuf::from(path)is redundant —Path::joinalready returns aPathBuf.♻️ Proposed simplification
- let path = PathBuf::from(path).join(BASE_PATH).join(relative_path); + let path = path.join(BASE_PATH).join(relative_path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 202 - 203, In create_file_structure, remove the redundant PathBuf::from(path) call and use the &Path parameter's join method directly so the intermediate PathBuf is produced via path.join(BASE_PATH).join(relative_path); update the let binding (still named path) to use path.join(...) to produce a PathBuf and leave BASE_PATH and relative_path usage unchanged.
🤖 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/common.rs`:
- Around line 36-81: The generated CARGO_SERVER_MAIN uses serde_json only inside
#[cfg(feature = "otel")] blocks but insert_required_deps currently always
injects serde_json into Cargo.toml, causing unused dependency errors; update
insert_required_deps to either (A) add serde_json to the generated Cargo.toml
only when the otel flag is enabled, or (B) add serde_json as an optional
dependency and include it under the otel feature in the features table so
serde_json is only enabled when feature "otel" is active (reference function
insert_required_deps and constant CARGO_SERVER_MAIN and ensure Cargo.toml
feature wiring reflects this).
- Around line 225-228: The generated `use` lines embed HashMap keys directly
(the `dependencies.keys().map(|name| format!("use {name} as _;\n"))`
expression), which fails for hyphenated crate names; sanitize the identifier by
replacing hyphens with underscores before formatting (e.g. compute let ident =
name.replace('-', "_") inside the map and use format!("use {ident} as _;\n")),
so generated Rust identifiers are valid.
---
Nitpick comments:
In `@crates/cli/src/common.rs`:
- Around line 127-128: The function insert_required_deps currently takes
ownership of a HashMap<String, ConfigModuleMetadata>, forcing callers to clone
the map; change its signature to take a mutable reference (&mut HashMap<String,
ConfigModuleMetadata>) and mutate it in place, updating its internals to use the
borrowed map instead of returning a new one, and update all call sites (remove
dependencies.clone() and pass &mut dependencies) so no extra allocation is
needed; ensure types reference ConfigModuleMetadata consistently and adjust any
return values or uses accordingly.
- Around line 202-203: In create_file_structure, remove the redundant
PathBuf::from(path) call and use the &Path parameter's join method directly so
the intermediate PathBuf is produced via
path.join(BASE_PATH).join(relative_path); update the let binding (still named
path) to use path.join(...) to produce a PathBuf and leave BASE_PATH and
relative_path usage unchanged.
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/cli/src/common.rs (1)
213-215: Hyphenated crate names produce invalid Rust identifiers in generatedusestatements.
nameis taken directly from theHashMapkey. A module like"my-service"would emituse my-service as _;, which is a syntax error. This was raised in a previous review and remains unresolved.🐛 Proposed fix
- .map(|name| format!("use {name} as _;\n")) + .map(|name| format!("use {} as _;\n", name.replace('-', "_")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 213 - 215, The generated use statements use raw crate keys which may contain hyphens (e.g., "my-service") and produce invalid Rust identifiers; update the mapping in the dependencies.keys().map(...) closure to convert crate names to valid Rust identifiers (at minimum replace '-' with '_', e.g. let ident = name.replace('-', "_")) and use that ident in the format!("use {ident} as _;\n") so hyphenated crate names emit valid use statements; ensure the change is applied where the dependencies mapping occurs in common.rs.
🧹 Nitpick comments (1)
crates/cli/src/common.rs (1)
175-177: Consider caching the parsed Liquid template withOnceLock.
CARGO_SERVER_MAINis a compile-time constant, butgenerate_server_structurere-parses it on every invocation. Astd::sync::OnceLockoronce_cell::sync::Lazywould parse the template once.♻️ Suggested approach
+use std::sync::OnceLock; + +fn server_main_template() -> &'static liquid::Template { + static TEMPLATE: OnceLock<liquid::Template> = OnceLock::new(); + TEMPLATE.get_or_init(|| { + liquid::ParserBuilder::with_stdlib() + .build() + .expect("liquid stdlib build failed") + .parse(CARGO_SERVER_MAIN) + .expect("CARGO_SERVER_MAIN template is invalid") + }) +} pub fn generate_server_structure(...) -> anyhow::Result<()> { ... - let main_template = liquid::ParserBuilder::with_stdlib() - .build()? - .parse(CARGO_SERVER_MAIN)?; + let main_template = server_main_template(); ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/common.rs` around lines 175 - 177, The code reparses the compile-time constant CARGO_SERVER_MAIN on every call to generate_server_structure; change this by creating a static std::sync::OnceLock (or once_cell::sync::Lazy) to hold the parsed liquid::Template and initialize it once, then replace the local main_template creation in generate_server_structure with a lookup from that static (e.g., TEMPLATE_ONCE.get_or_init(|| ParserBuilder::with_stdlib().build()?.parse(CARGO_SERVER_MAIN))). Ensure error handling matches the current return type so failures to parse propagate correctly.
🤖 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/common.rs`:
- Around line 213-215: The generated use statements use raw crate keys which may
contain hyphens (e.g., "my-service") and produce invalid Rust identifiers;
update the mapping in the dependencies.keys().map(...) closure to convert crate
names to valid Rust identifiers (at minimum replace '-' with '_', e.g. let ident
= name.replace('-', "_")) and use that ident in the format!("use {ident} as
_;\n") so hyphenated crate names emit valid use statements; ensure the change is
applied where the dependencies mapping occurs in common.rs.
---
Nitpick comments:
In `@crates/cli/src/common.rs`:
- Around line 175-177: The code reparses the compile-time constant
CARGO_SERVER_MAIN on every call to generate_server_structure; change this by
creating a static std::sync::OnceLock (or once_cell::sync::Lazy) to hold the
parsed liquid::Template and initialize it once, then replace the local
main_template creation in generate_server_structure with a lookup from that
static (e.g., TEMPLATE_ONCE.get_or_init(||
ParserBuilder::with_stdlib().build()?.parse(CARGO_SERVER_MAIN))). Ensure error
handling matches the current return type so failures to parse propagate
correctly.
This was referenced Mar 4, 2026
This was referenced Apr 7, 2026
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