refactor(cli): disable jsonschema resolving external resources#30
Conversation
WalkthroughThese changes disable default features for the jsonschema dependency and refactor config validation logic to use cached schema validation. The config identifier source is changed from "identifier" to "bundle" field, validation is restricted to .json files, and merge behavior is adjusted to store merged configs under an extension key instead of mutating the config directly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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 175-179: The current extraction for original_identifier always
yields None because it calls .as_str() directly on the "bundle" value (an
object); update the chain to access the "identifier" field inside the bundle
object instead—e.g., from the config value call .as_object().and_then(|config|
config.get("bundle")).and_then(|b| b.as_object()).and_then(|bundle_obj|
bundle_obj.get("identifier")).and_then(|id|
id.as_str()).map(ToString::to_string) so original_identifier correctly captures
bundle.identifier.
- Around line 201-202: The conditional currently uses && between two mutually
exclusive checks (config_path.extension() == Some(OsStr::new("json")) &&
config_path.extension() == Some(OsStr::new("json5"))) which never succeeds;
change it to only check for JSON per the PR summary by replacing the whole
condition with config_path.extension() == Some(OsStr::new("json")) so schema
validation runs for .json files, or if you intended to accept both extensions
use || between the two OsStr checks instead.
- Around line 286-291: The code computes a merged JSON value (variable value) by
serializing config_metadata.inner and calling merge(&mut value, &merge_config)
but then only inserts the raw merge_config into config_metadata.extensions,
discarding the merged result; update the function so the merged value is applied
back to config_metadata.inner (e.g., deserialize/convert value into the same
type as config_metadata.inner and assign it) or else insert the merged JSON into
config_metadata.extensions under MERGE_CONFIG_EXTENSION_NAME instead of the raw
merge_config, making the change around the lines that call
serde_json::to_value(...)->value, merge(&mut value, &merge_config), and the
insertion into config_metadata.extensions to ensure the merged configuration is
persisted before returning handle.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/tauri-cli/Cargo.tomlcrates/tauri-cli/src/helpers/config.rscrates/tauri-cli/src/interface/rust.rs
🔇 Additional comments (5)
crates/tauri-cli/src/helpers/config.rs (3)
17-17: LGTM!The
OnceLockimport is correctly added to support the new validator caching pattern.
149-156: LGTM!The
OnceLockcaching pattern is appropriate here for lazy initialization. The bundled schema is loaded at compile-time viainclude_str!, avoiding runtime file access. Theexpect()calls are acceptable since a malformed bundled schema would be a build-time issue.
204-217: LGTM!The error logging improvements using named parameters (
{error},{path}) are cleaner and the validator helper is correctly invoked.crates/tauri-cli/src/interface/rust.rs (1)
486-490: LGTM!Using
errdirectly instead oferr.to_string()is more idiomatic — thelog::error!macro handles theDisplayformatting automatically, avoiding an unnecessary string allocation.crates/tauri-cli/Cargo.toml (1)
69-69: Correct approach to disable external resource resolution.The change disables the
resolve-httpandresolve-filedefault features in jsonschema 0.33, preventing the library from fetching external schemas. This aligns with the PR objective.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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: Identifier extraction will always return None.
The bundle field in tauri.conf.json is an object (containing identifier, icon, etc.), not a string. Calling .as_str() on an object returns None, so original_identifier will never be set.
If the intent is to extract bundle.identifier:
🐛 Proposed fix
let original_identifier = config
.as_object()
- .and_then(|config| config.get("bundle"))
+ .and_then(|config| config.get("bundle"))
+ .and_then(|bundle| bundle.as_object())
+ .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.as_object()) | |
| .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, The current
extraction for original_identifier always yields None because it calls .as_str()
directly on the "bundle" value (an object); update the chain to access the
"identifier" field inside the bundle object instead—e.g., from the config value
call .as_object().and_then(|config| config.get("bundle")).and_then(|b|
b.as_object()).and_then(|bundle_obj| bundle_obj.get("identifier")).and_then(|id|
id.as_str()).map(ToString::to_string) so original_identifier correctly captures
bundle.identifier.
| if config_path.extension() == Some(OsStr::new("json")) | ||
| || config_path.extension() == Some(OsStr::new("json5")) | ||
| && config_path.extension() == Some(OsStr::new("json5")) |
There was a problem hiding this comment.
Critical bug: Schema validation will never execute.
The condition extension == "json" && extension == "json5" can never be true — a file extension cannot be both values simultaneously. This means schema validation is completely disabled.
Based on the PR summary indicating validation should be narrowed to .json files only:
🐛 Proposed fix for .json-only validation
- if config_path.extension() == Some(OsStr::new("json"))
- && config_path.extension() == Some(OsStr::new("json5"))
+ if config_path.extension() == Some(OsStr::new("json"))
{If the intent was to validate both .json and .json5 files, use || instead:
Alternative fix for both extensions
- if config_path.extension() == Some(OsStr::new("json"))
- && config_path.extension() == Some(OsStr::new("json5"))
+ if config_path.extension() == Some(OsStr::new("json"))
+ || config_path.extension() == Some(OsStr::new("json5"))
{📝 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.
| if config_path.extension() == Some(OsStr::new("json")) | |
| || config_path.extension() == Some(OsStr::new("json5")) | |
| && 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 - 202, The
conditional currently uses && between two mutually exclusive checks
(config_path.extension() == Some(OsStr::new("json")) && config_path.extension()
== Some(OsStr::new("json5"))) which never succeeds; change it to only check for
JSON per the PR summary by replacing the whole condition with
config_path.extension() == Some(OsStr::new("json")) so schema validation runs
for .json files, or if you intended to accept both extensions use || between the
two OsStr checks instead.
| 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:
#!/bin/bash
# First, examine the full function context around lines 286-291
cat -n crates/tauri-cli/src/helpers/config.rs | sed -n '270,310p'Repository: qodo-benchmark/tauri-combined-coderabbit
Length of output: 1652
🏁 Script executed:
#!/bin/bash
# Check the function signature and documentation for merge_with
rg -B10 "fn merge_with" crates/tauri-cli/src/helpers/config.rs | head -30Repository: qodo-benchmark/tauri-combined-coderabbit
Length of output: 409
🏁 Script executed:
#!/bin/bash
# Check how merge_with is called by callers
rg "merge_with\(" --type=rust -B2 -A2Repository: qodo-benchmark/tauri-combined-coderabbit
Length of output: 1414
🏁 Script executed:
#!/bin/bash
# Check if there are tests for this function
rg "merge_with" --type=rust | grep -i testRepository: qodo-benchmark/tauri-combined-coderabbit
Length of output: 250
The merged value is computed but discarded; only raw config patches are stored.
Line 286-288 compute a merged value, but line 289 stores only the raw merge_config in extensions—the actual merged result is never applied to config_metadata.inner or stored. This contradicts the function's documented purpose of merging the loaded config. Either the merged value should be applied to inner, or extensions should store the merged result instead of raw patches.
🤖 Prompt for AI Agents
In `@crates/tauri-cli/src/helpers/config.rs` around lines 286 - 291, The code
computes a merged JSON value (variable value) by serializing
config_metadata.inner and calling merge(&mut value, &merge_config) but then only
inserts the raw merge_config into config_metadata.extensions, discarding the
merged result; update the function so the merged value is applied back to
config_metadata.inner (e.g., deserialize/convert value into the same type as
config_metadata.inner and assign it) or else insert the merged JSON into
config_metadata.extensions under MERGE_CONFIG_EXTENSION_NAME instead of the raw
merge_config, making the change around the lines that call
serde_json::to_value(...)->value, merge(&mut value, &merge_config), and the
insertion into config_metadata.extensions to ensure the merged configuration is
persisted before returning handle.
2 participants
Benchmark PR from agentic-review-benchmarks#4
Summary by CodeRabbit
Configuration
Chores
✏️ Tip: You can customize this high-level summary in your review settings.