tiennm99/claude-code-usage-bubble
1
0
Fork 0
mirror of https://github.com/tiennm99/claude-code-usage-bubble.git synced 2026-06-06 10:12:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

docs(reports): code-reviewer report for restart-button impl

Browse Source 
DONE_WITH_CONCERNS — flagged M1 (match-arm vs IDM_LANG_BASE guard) and
L3 (lock-during-save). Both addressed in the feat commit.
This commit is contained in:
tiennm99
2026-05-18 10:39:33 +07:00
parent 457d5274da
commit e089a1b420
1 changed files with 62 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plans/reports/code-reviewer-260518-1029-restart-button-impl.md
+62
View File
@@ -0,0 +1,62 @@
# Code Review — Tray "Restart" Action
**Scope:** uncommitted changes on clean tree
**Files:** `src/app.rs`, `src/i18n/mod.rs`, 8x `src/i18n/locales/*.toml`
**Plan:** `plans/260518-0945-menu-restart-button/phase-01-implement-restart-action.md`
## Verdict
Clean implementation. All 7 acceptance criteria met. Build is `cargo check`-clean. No security regressions. Pattern faithfully borrowed from `update/install.rs`.
## Acceptance Criteria — all PASS
1. Menu order verified `app.rs:1040-1042`: separator → `IDM_RESTART` → `IDM_EXIT`.
2. `IDM_RESTART => restart_app()` arm wired `app.rs:393`.
3. `restart_app()` `app.rs:1382-1421` flushes settings, gets `current_exe`, rejects `%`, spawns detached `cmd.exe`, `PostQuitMessage(0)`.
4. 1 s `timeout` matches install.rs precedent (2 s there; 1 s sufficient — current process exits as soon as `PostQuitMessage(0)` drains the loop).
5. Verified `restart = "..."` in all 8 TOMLs at line 32 (en/de/es/fr/ja/ko/nl/zh-TW). `LocaleStrings` field at `mod.rs:52`. No `#[serde(default)]` → missing key = hard fail; all present.
6. Match-arm ordering unambiguous: `IDM_RESTART=33` < guard `x >= IDM_LANG_BASE=100`. Guard won't match 33. `tray::IDM_TOGGLE_WIDGET=50` likewise < 100. Safe.
7. No new clippy issues; no new unsafe blocks (`PostQuitMessage(0)` already unsafe at `IDM_EXIT`; matches that idiom).
## Critical
None.
## High
None.
## Medium
**M1. Restart arm sits below the `IDM_LANG_BASE` guard arm.** `app.rs:391-393`. The guard `x if x >= IDM_LANG_BASE => …` is exhaustive for any id `>= 100`. Today `IDM_RESTART=33` is fine, but future readers adding a static id `>= 100` between lines 392 and 393 would silently route into language switching. Cheap fix: move `IDM_RESTART => restart_app()` and `tray::IDM_TOGGLE_WIDGET => …` ABOVE the guard arm. Plan note at `app.rs:82-84` already warns about this — the new arm violates that guidance.
## Low / Info
**L1. Double-restart not deduped.** Rapid clicks queue multiple `cmd.exe` children. First wins the mutex; second's `start ""` succeeds, the resulting bubble process exits at `ERROR_ALREADY_EXISTS`. Acceptable per plan §Risk Assessment. No fix needed.
**L2. `to_string_lossy()` on `current_exe()` will mangle non-UTF-8 paths.** Same pattern in `install.rs:100`. On real Windows installs paths are UTF-16; lossy → UTF-8 is virtually always faithful. Consistent with existing precedent.
**L3. `settings::save()` runs while `lock_state()` read-guard is held** (`app.rs:1385-1387`). If `save` ever takes a lock on the same mutex this would deadlock — it currently does not, but the pattern elsewhere (e.g. `set_poll_interval` at 1087-1100) clones, releases, then saves. Recommend matching that pattern: clone snapshot inside scope, drop guard, then `settings::save(&snap)`. Defensive only.
**L4. No regression to existing menu wiring** — verified by inspection: `show_widget` append at 1034-1039 still preceded by no separator, then separator 1040, then Restart, then Exit. Matches plan exactly.
## Pattern-Parity Check vs `install.rs`
- Flags: `CREATE_NO_WINDOW | DETACHED_PROCESS` → identical bit pattern (`0x0800_0000 | 0x0000_0008`). New constants `RESTART_*` duplicate the values; minor DRY nit but they're file-local and the comment explains why. Acceptable.
- `raw_arg` quoting: `/c` then `"<cmd>"` with inner `"` preserved → byte-for-byte same shape as `install.rs:113-114`. Correct.
- `%` rejection: present, logs and aborts. Matches `install.rs:89-96`.
- `stdin/out/err = Null`: present, matches.
## PostQuitMessage on UI thread
`IDM_EXIT` does the same at `app.rs:370`, called from `on_menu_command` via WM_COMMAND on the UI thread. `restart_app()` is reached the same way. Safe — identical control-flow shape.
## Metrics
- New code: ~40 LOC in `app.rs`, 1 field in `mod.rs`, 8x 1-line TOML adds.
- Type coverage: 100%.
- New warnings: 0 (`cargo check` clean per user).
## Recommended Actions
1. **M1** (nice-to-have): reorder match arms so `IDM_RESTART` / `IDM_TOGGLE_WIDGET` precede the `x if x >= IDM_LANG_BASE` guard. Defends against future id collisions.
2. **L3** (optional): mirror `set_poll_interval`'s clone-then-save pattern in `restart_app()` for consistency.
## Unresolved Questions
- None blocking. Plan §Next Steps suggests a semver patch bump and analogous bubble-menu entry; out of scope for this review.
---
**Status:** DONE_WITH_CONCERNS
**Summary:** Implementation matches plan and acceptance criteria; cmd-handoff faithfully mirrors `update/install.rs`; all 8 locales updated; no critical or high issues. One medium suggestion (reorder match arms to defend against future static-id collisions with the `IDM_LANG_BASE` guard) and two low/optional refinements.
**Concerns:** M1 — new `IDM_RESTART` and `tray::IDM_TOGGLE_WIDGET` arms sit below a catch-all `x >= IDM_LANG_BASE` guard. Today safe (33, 50 < 100); future-fragile. Code comment at `app.rs:82-84` already flags the rule that was bent.