diff --git a/plans/reports/code-reviewer-260518-1029-restart-button-impl.md b/plans/reports/code-reviewer-260518-1029-restart-button-impl.md new file mode 100644 index 0000000..a0c0806 --- /dev/null +++ b/plans/reports/code-reviewer-260518-1029-restart-button-impl.md @@ -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 `""` 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.