Less statics - #5
Conversation
📝 WalkthroughWalkthroughThis refactor reduces internal static constructs by introducing a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tauri-cli/src/mobile/android/android_studio_script.rs (1)
88-97:⚠️ Potential issue | 🔴 CriticalRemove the redundant
merge_config_withcall or clarify the necessity of both functions.The code applies the same config overrides twice:
reload_config(lines 54–62) reloads the config from disk and applies the merge configs during the load process viaload_config(merge_configs, true, ...), and thenmerge_config_with(lines 88–97) applies the identicalcli_options.configvalues to the already-reloaded configuration again.
reload_configreplaces the entire config object with a fresh load that includes the merges, whilemerge_config_withperforms an in-place merge on the existing config. Unless there is a specific reason to apply these overrides a second time after reloading, the second call should be removed.
🤖 Fix all issues with AI agents
In @.changes/reduce-internal-statics.md:
- Line 6: Update the changelog sentence "Refactored internal use of static on
config and directory resolvings, no user facing changes, please report any
regressions if you encounter any" to correct grammar and punctuation: hyphenate
"user-facing", break the run-on into two sentences or use proper punctuation,
fix "resolvings" to "resolving", and ensure the line ends with a period; e.g.
split into "Refactored internal use of 'static' on config and directory
resolving. No user-facing changes; please report any regressions if you
encounter any."
In `@crates/tauri-cli/src/remove.rs`:
- Line 49: The call to manager.remove currently passes dirs.tauri instead of the
frontend directory and loses ownership of frontend_dir via the earlier .map(),
causing npm removal to run in the wrong working directory; fix by preserving
frontend_dir (avoid consuming it with .map()) and construct PackageManager with
PackageManager::from_project(&frontend_dir) then call
manager.remove(&[npm_name], &frontend_dir) inside an if let Some(frontend_dir) =
frontend_dir block so the correct frontend_dir is passed to manager.remove
(reference symbols: frontend_dir, PackageManager::from_project, manager.remove,
npm_name).
🧹 Nitpick comments (3)
crates/tauri-cli/src/helpers/app_paths.rs (1)
130-144: Consider the safety ofparent().unwrap()fallback.On line 141,
tauri.parent().unwrap()is used as a fallback whenresolve_frontend_dir()returnsNone. While the tauri directory should always have a parent (since it must be a valid project directory containing config files), this could theoretically panic if the tauri path is the filesystem root.Given that the preceding logic in
resolve_tauri_dir()ensures the path contains configuration files and is not a root path, this should be safe in practice. However, you might consider adding a comment explaining this assumption or usingunwrap_or_elsewith a descriptive panic message for consistency with the error handling pattern above.crates/tauri-cli/src/mobile/mod.rs (1)
476-488: Redundantto_path_buf()call.
tauri_diris converted toPathBufon line 476, thento_path_buf()is called again on line 477. The second conversion is unnecessary.♻️ Proposed fix
let app_settings = interface.app_settings(); let tauri_dir = tauri_dir.to_path_buf(); - App::from_raw(tauri_dir.to_path_buf(), raw) + App::from_raw(tauri_dir.clone(), raw) .unwrap() .with_target_dir_resolver(move |target, profile| {crates/tauri-cli/src/mobile/ios/build.rs (1)
329-329: Unnecessary double reference.
dirsis already&Dirs, so&dirscreates&&Dirs. While this works due to auto-deref, it's inconsistent with other call sites.♻️ Proposed fix
&mut config, &mut env, noise_level, - &dirs, + dirs, )?;
| "tauri-cli": patch:changes | ||
| --- | ||
|
|
||
| Refactored internal use of static on config and directory resolvings, no user facing changes, please report any regressions if you encounter any |
There was a problem hiding this comment.
Fix grammar and punctuation in the changelog description.
The description has the following issues:
- "user facing" should be hyphenated as "user-facing"
- Run-on sentence structure needs proper punctuation
- Missing period at the end
📝 Proposed fix
-Refactored internal use of static on config and directory resolvings, no user facing changes, please report any regressions if you encounter any
+Refactored internal use of static on config and directory resolvings. No user-facing changes; please report any regressions if you encounter any.📝 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.
| Refactored internal use of static on config and directory resolvings, no user facing changes, please report any regressions if you encounter any | |
| Refactored internal use of static on config and directory resolvings. No user-facing changes; please report any regressions if you encounter any. |
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: Use a hyphen to join words.
Context: ...config and directory resolvings, no user facing changes, please report any regres...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In @.changes/reduce-internal-statics.md at line 6, Update the changelog sentence
"Refactored internal use of static on config and directory resolvings, no user
facing changes, please report any regressions if you encounter any" to correct
grammar and punctuation: hyphenate "user-facing", break the run-on into two
sentences or use proper punctuation, fix "resolvings" to "resolving", and ensure
the line ends with a period; e.g. split into "Refactored internal use of
'static' on config and directory resolving. No user-facing changes; please
report any regressions if you encounter any."
| if let Some(manager) = frontend_dir.map(PackageManager::from_project) { | ||
| let npm_name = format!("@tauri-apps/plugin-{plugin}"); | ||
| manager.remove(&[npm_name], tauri_dir)?; | ||
| manager.remove(&[npm_name], dirs.tauri)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the PackageManager remove method signature and implementation
rg -n "fn remove" crates/tauri-cli/src/helpers/npm.rs -A 15Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 558
🏁 Script executed:
# Also search for the full file to understand context
fd "npm.rs" crates/tauri-cli/srcRepository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 130
🏁 Script executed:
# Look at the remove.rs file to see the full context around line 49
cat -n crates/tauri-cli/src/remove.rs | head -60Repository: agentic-review-benchmarks/tauri-combined-coderabbit
Length of output: 2138
Fix incorrect working directory for npm package removal.
The manager.remove() call at line 49 passes dirs.tauri as the second argument, but the method signature explicitly expects frontend_dir. NPM operations must run in the frontend directory where package.json exists, not the Rust source directory.
The frontend_dir is consumed by the .map() on line 47, so it needs to be preserved. Refactor as follows:
if let Some(frontend_dir) = frontend_dir {
let manager = PackageManager::from_project(&frontend_dir);
let npm_name = format!("@tauri-apps/plugin-{plugin}");
manager.remove(&[npm_name], &frontend_dir)?;
}
🤖 Prompt for AI Agents
In `@crates/tauri-cli/src/remove.rs` at line 49, The call to manager.remove
currently passes dirs.tauri instead of the frontend directory and loses
ownership of frontend_dir via the earlier .map(), causing npm removal to run in
the wrong working directory; fix by preserving frontend_dir (avoid consuming it
with .map()) and construct PackageManager with
PackageManager::from_project(&frontend_dir) then call
manager.remove(&[npm_name], &frontend_dir) inside an if let Some(frontend_dir) =
frontend_dir block so the correct frontend_dir is passed to manager.remove
(reference symbols: frontend_dir, PackageManager::from_project, manager.remove,
npm_name).
1 participant
Benchmark PR from agentic-review-benchmarks#5
Summary by CodeRabbit