refactor(cli): disable jsonschema resolving external resources#4
Conversation
📝 WalkthroughWalkthroughThe changes optimize the jsonschema dependency by disabling default features, introduce a cached JSON schema validator for configuration validation using OnceLock, refactor config merging logic to use metadata extensions, and improve error handling efficiency by reducing unnecessary string allocations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/tauri-cli/src/helpers/config.rs`:
- Around line 201-204: The extension check on config_path is impossible because
it uses && (both "json" and "json5" at once), so schema validation never runs;
update the condition in the block that calls
config_schema_validator().iter_errors(&config) to check for either extension
instead (replace the && with ||) — i.e., use config_path.extension() ==
Some(OsStr::new("json")) || config_path.extension() == Some(OsStr::new("json5"))
(or collapse to a single map/contains check) so .json and .json5 files properly
trigger validation in the config loading logic.
- Around line 175-179: original_identifier is always None because the code calls
.as_str() on the "bundle" value (an object) instead of drilling into that
object; change the extraction to get the "bundle" object first and then read its
identifier field (e.g., replace the chain on config to .and_then(|config|
config.get("bundle")).and_then(|b| b.as_object()).and_then(|b|
b.get("identifier").and_then(|id| id.as_str())).map(ToString::to_string) so
original_identifier actually captures the inner string.
- Around line 286-291: The merged JSON `value` is never written back to the
actual config; after calling merge(&mut value, &merge_config) you must
deserialize the merged `value` and assign it to `config_metadata.inner` so the
in-memory config reflects the merge. Locate the block around
serde_json::to_value(config_metadata.inner.clone()) and, after merge(&mut value,
&merge_config), perform serde_json::from_value(value) (with appropriate error
context) and set config_metadata.inner = deserialized_result before inserting
the MERGE_CONFIG_EXTENSION_NAME extension and returning handle.
| let original_identifier = config | ||
| .as_object() | ||
| .and_then(|config| config.get("identifier")) | ||
| .and_then(|config| config.get("bundle")) | ||
| .and_then(|id| id.as_str()) | ||
| .map(ToString::to_string); |
There was a problem hiding this comment.
Bug: original_identifier extraction will always return None.
The code retrieves "bundle" (which is an object) and then calls .as_str() on it. Since bundle is an object containing the identifier, not a string, this will always return None.
To properly extract the bundle identifier from the config JSON, you need to navigate into the bundle object:
🐛 Proposed fix
let original_identifier = config
.as_object()
- .and_then(|config| config.get("bundle"))
- .and_then(|id| id.as_str())
+ .and_then(|config| config.get("bundle"))
+ .and_then(|bundle| bundle.get("identifier"))
+ .and_then(|id| id.as_str())
.map(ToString::to_string);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let original_identifier = config | |
| .as_object() | |
| .and_then(|config| config.get("identifier")) | |
| .and_then(|config| config.get("bundle")) | |
| .and_then(|id| id.as_str()) | |
| .map(ToString::to_string); | |
| let original_identifier = config | |
| .as_object() | |
| .and_then(|config| config.get("bundle")) | |
| .and_then(|bundle| bundle.get("identifier")) | |
| .and_then(|id| id.as_str()) | |
| .map(ToString::to_string); |
🤖 Prompt for AI Agents
In `@crates/tauri-cli/src/helpers/config.rs` around lines 175 - 179,
original_identifier is always None because the code calls .as_str() on the
"bundle" value (an object) instead of drilling into that object; change the
extraction to get the "bundle" object first and then read its identifier field
(e.g., replace the chain on config to .and_then(|config|
config.get("bundle")).and_then(|b| b.as_object()).and_then(|b|
b.get("identifier").and_then(|id| id.as_str())).map(ToString::to_string) so
original_identifier actually captures the inner string.
| if config_path.extension() == Some(OsStr::new("json")) | ||
| || config_path.extension() == Some(OsStr::new("json5")) | ||
| && config_path.extension() == Some(OsStr::new("json5")) | ||
| { | ||
| let schema: JsonValue = serde_json::from_str(include_str!("../../config.schema.json")) | ||
| .context("failed to parse config schema")?; | ||
| let validator = jsonschema::validator_for(&schema).expect("Invalid schema"); | ||
| let mut errors = validator.iter_errors(&config).peekable(); | ||
| let mut errors = config_schema_validator().iter_errors(&config).peekable(); |
There was a problem hiding this comment.
Critical bug: Schema validation is completely disabled due to impossible condition.
The condition checks if the extension equals both "json" AND "json5" simultaneously, which is logically impossible. This means schema validation will never execute.
If the intent was to validate only .json files (not .json5), it should be:
if config_path.extension() == Some(OsStr::new("json"))If the intent was to validate both formats, the original || was correct:
if config_path.extension() == Some(OsStr::new("json"))
|| config_path.extension() == Some(OsStr::new("json5"))🐛 Proposed fix (assuming intent is to validate .json only)
- if config_path.extension() == Some(OsStr::new("json"))
- && config_path.extension() == Some(OsStr::new("json5"))
+ if config_path.extension() == Some(OsStr::new("json"))
{🤖 Prompt for AI Agents
In `@crates/tauri-cli/src/helpers/config.rs` around lines 201 - 204, The extension
check on config_path is impossible because it uses && (both "json" and "json5"
at once), so schema validation never runs; update the condition in the block
that calls config_schema_validator().iter_errors(&config) to check for either
extension instead (replace the && with ||) — i.e., use config_path.extension()
== Some(OsStr::new("json")) || config_path.extension() ==
Some(OsStr::new("json5")) (or collapse to a single map/contains check) so .json
and .json5 files properly trigger validation in the config loading logic.
| let mut value = | ||
| serde_json::to_value(config_metadata.inner.clone()).context("failed to serialize config")?; | ||
| merge(&mut value, &merge_config); | ||
| config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?; | ||
| config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config); | ||
|
|
||
| Ok(handle) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/tauri-cli/src/helpers/config.rs | sed -n '270,310p'Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1663
🏁 Script executed:
git log -p --all -S 'merge_with' -- 'crates/tauri-cli/src/helpers/config.rs' | head -300Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 77
🏁 Script executed:
rg -B5 -A5 "^fn merge" crates/tauri-cli/src/helpers/config.rsRepository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 413
🏁 Script executed:
rg "merge_with" crates/tauri-cli/src/ -A3 -B1Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1302
🏁 Script executed:
rg "merge_with" crates/tauri-cli/tests/ -A5 -B1 2>/dev/null || echo "No tests found in expected location"Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 130
🏁 Script executed:
rg "use.*merge|fn merge|from_value.*value" crates/tauri-cli/src/helpers/config.rs | head -20Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 304
🏁 Script executed:
rg "MERGE_CONFIG_EXTENSION_NAME" crates/tauri-cli/src/ -B2 -A2Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1205
🏁 Script executed:
cat -n crates/tauri-cli/src/helpers/config.rs | sed -n '190,210p'Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1074
🏁 Script executed:
cat -n crates/tauri-cli/src/helpers/config.rs | sed -n '1,50p'Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1765
🏁 Script executed:
cat -n crates/tauri-cli/src/helpers/config.rs | sed -n '160,200p'Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1786
🏁 Script executed:
cat -n crates/tauri-cli/src/helpers/config.rs | sed -n '270,295p'Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 1127
Merged config value is computed but never applied to the inner config.
The code merges merge_config into value (line 288), but value is never assigned back to config_metadata.inner. This is inconsistent with the load() function (line 197), which applies merge results to the actual config. Only the extension is being stored, leaving the inner config unchanged and the merge operation ineffective.
🐛 Fix: Apply merged result to inner config
let mut value =
serde_json::to_value(config_metadata.inner.clone()).context("failed to serialize config")?;
merge(&mut value, &merge_config);
+ config_metadata.inner = serde_json::from_value(value).context("failed to deserialize merged config")?;
config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config);
Ok(handle)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut value = | |
| serde_json::to_value(config_metadata.inner.clone()).context("failed to serialize config")?; | |
| merge(&mut value, &merge_config); | |
| config_metadata.inner = serde_json::from_value(value).context("failed to parse config")?; | |
| config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config); | |
| Ok(handle) | |
| let mut value = | |
| serde_json::to_value(config_metadata.inner.clone()).context("failed to serialize config")?; | |
| merge(&mut value, &merge_config); | |
| config_metadata.inner = serde_json::from_value(value).context("failed to deserialize merged config")?; | |
| config_metadata.extensions.insert(MERGE_CONFIG_EXTENSION_NAME.into(), merge_config); | |
| Ok(handle) |
🤖 Prompt for AI Agents
In `@crates/tauri-cli/src/helpers/config.rs` around lines 286 - 291, The merged
JSON `value` is never written back to the actual config; after calling
merge(&mut value, &merge_config) you must deserialize the merged `value` and
assign it to `config_metadata.inner` so the in-memory config reflects the merge.
Locate the block around serde_json::to_value(config_metadata.inner.clone()) and,
after merge(&mut value, &merge_config), perform serde_json::from_value(value)
(with appropriate error context) and set config_metadata.inner =
deserialized_result before inserting the MERGE_CONFIG_EXTENSION_NAME extension
and returning handle.
2 participants
Benchmark PR from agentic-review-benchmarks#4
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.