Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors the CLI plugin to expose a concrete Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (Manager)
participant PluginCli as PluginCli
participant File as FileSystem
participant Tray as Tray/Menu
participant Ext as tauri_plugin_cli (CliExt)
App->>PluginCli: plugin_cli() %% accessor
activate PluginCli
App->>PluginCli: check_cli_status()
PluginCli->>File: resolve symlink & target
File-->>PluginCli: exists / target path
PluginCli-->>App: CliStatus { is_installed, symlink_path, target_path }
alt User selects Install via Tray
Tray->>App: Menu event (AppCliInstall)
App->>PluginCli: install_cli_to_path()
PluginCli->>PluginCli: get_cli_executable_path()
PluginCli->>Ext: app.cli().matches() / plugin data
Ext-->>PluginCli: CLI matches/info
PluginCli->>File: create parent dir + create symlink or copy
File-->>PluginCli: success
PluginCli-->>App: Ok
App->>Tray: refresh app menu (app_cli_menu)
else User selects Uninstall via Tray
Tray->>App: Menu event (AppCliUninstall)
App->>PluginCli: uninstall_cli_from_path()
PluginCli->>File: remove symlink/cleanup
File-->>PluginCli: success
PluginCli-->>App: Ok
App->>Tray: refresh app menu (app_cli_menu)
end
deactivate PluginCli
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/tray/src/ext.rs (1)
63-88: Add submenu item count validation before remove operations (lines 76–77)The code performs two consecutive
remove_at(0)?calls on the submenu without validating that the submenu has at least 2 items. If the submenu has fewer items than expected, the second removal will fail at runtime. Add a guard on the submenu's item count before these operations, or use a more defensive approach to replace specific entries.if let MenuItemKind::Submenu(submenu) = &items[0] { let submenu_items = submenu.items()?; if submenu_items.len() >= 2 { submenu.remove_at(0)?; submenu.remove_at(0)?; submenu.prepend(&cli_item)?; submenu.prepend(&info_item)?; } }
🧹 Nitpick comments (3)
plugins/tray/src/ext.rs (2)
115-184: CLI install/uninstall actions silently ignore failuresFor
AppCliInstall/AppCliUninstall, errors fromapp.plugin_cli().{install,uninstall}_cli_from_path()and fromapp.create_app_menu()are discarded (if let Ok(_) = ... { let _ = ...; }). This keeps the flow simple but makes it hard to diagnose permission/path issues (e.g., Windows symlink privileges, non‑symlink path errors). You might want to at least log these errors (or surface a dialog) so users and logs can see why install/uninstall did nothing.
235-261: CLI menu label toggling based onCliStatus
app_cli_menucorrectly checksplugin_cli().check_cli_status().map(|status| status.is_installed).unwrap_or(false)and switches between “Install CLI” and “Uninstall CLI” menu IDs/labels. On unsupported platforms wherecheck_cli_statusmight error, this gracefully falls back to showing “Install CLI”. If you ever want to hide the CLI option entirely when the platform is unsupported, you could treat specific error variants (e.g.,UnsupportedPlatform) differently here.plugins/cli2/src/ext.rs (1)
53-112: Install/uninstall logic and safety checks
install_cli_to_pathanduninstall_cli_from_pathare generally well‑behaved:
- Parent directories are created before symlink creation.
- Existing files are removed before creating a new symlink.
- Uninstall uses
symlink_metadataand only removes the path when it’s actually a symlink, returningNonSymlinkCliPathotherwise.This minimizes the risk of deleting an arbitrary non‑CLI file. The only behavior you might want to consider later is distinguishing
UnsupportedPlatformat the call sites (e.g., to hide or disable CLI install UI) instead of letting it bubble up silently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
plugins/cli2/src/commands.rs(1 hunks)plugins/cli2/src/ext.rs(6 hunks)plugins/cli2/src/lib.rs(1 hunks)plugins/tray/Cargo.toml(1 hunks)plugins/tray/src/ext.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/cli2/src/commands.rs (1)
plugins/cli2/src/ext.rs (1)
check_cli_status(114-132)
plugins/cli2/src/ext.rs (1)
plugins/cli2/src/commands.rs (1)
check_cli_status(23-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: fmt
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (7)
plugins/tray/Cargo.toml (1)
22-22: Newtauri-plugin-cli2workspace dependencyAdding
tauri-plugin-cli2 = { workspace = true }matches the existing dependency style. Just ensure the crate is correctly defined as a workspace member (name and features) so this resolves as expected in all builds.plugins/cli2/src/lib.rs (1)
9-10: Publicly re-exportingCliExtfromtauri-plugin-cliRe‑exporting
CliExthere is fine and doesn’t conflict with local names; it effectively makesCliExtpart of this crate’s public surface. Please confirm that exposing this trait fromcli2is intentional (i.e., expected to be used by downstream crates) and not just for internal convenience.plugins/cli2/src/commands.rs (1)
1-29: Commands correctly routed throughplugin_cli()Using
app.plugin_cli().{install_cli_to_path, uninstall_cli_from_path, check_cli_status}with theCliPluginExtimport cleanly centralizes CLI behavior behindPluginCliwhile preserving the existingResult<_, String>surface viamap_err(|e| e.to_string()). No functional regressions are apparent here.plugins/tray/src/ext.rs (1)
15-53: Menu enum ↔ ID mapping looks consistentThe new
HyprMenuItem::AppCliInstall/AppCliUninstallvariants and theirMenuIdstring mappings are symmetrical and consistent with the existing naming scheme, so event routing viaHyprMenuItem::from(MenuId)should work as expected.plugins/cli2/src/ext.rs (3)
3-25:PluginCliwrapper andhandle_cli_matchesbehaviorWrapping CLI operations in
PluginCli<R>tied to anAppHandleis a clean way to centralize this logic. One notable semantic change is inhandle_cli_matches: on error it now prints to stderr and callsstd::process::exit(1)instead of propagating the error, while--help/--versionexit with code 0. That’s appropriate for a CLI entrypoint, but it does mean callers cannot recover or log differently; please confirm this “hard exit” behavior is what you want for all usages ofhandle_cli_matches.
26-52: Symlink path selection and executable resolution
get_cli_symlink_pathchoosing$HOME/.local/bin/hyprnote(or%USERPROFILE%\\.local\\bin\\hyprnote.exeon Windows) keeps everything in user‑writable locations, with sensible fallbacks.get_cli_executable_pathusingstd::env::current_exe()is also a straightforward way to locate the running binary for symlinking. Both look reasonable; just be aware that on Windows, creating symlinks may require additional privileges, so you’ll likely seePermissionDeniederrors in the wild.
114-153:check_cli_status,CliPluginExt, andCliStatusAPI shape
check_cli_statusreturning a simpleCliStatuswithis_installed,symlink_path, andtarget_pathmatches the tray’s needs and keeps the API straightforward. The newCliPluginExttrait providingplugin_cli()for anyManager<R>(includingAppHandle) is idiomatic and integrates cleanly with call sites likeapp.plugin_cli().check_cli_status(). Public fields onCliStatusare fine here given it’s a pure data carrier.
1 participant
No description provided.