diff --git a/docs/plans/2026-07-09-001-fix-menu-stale-closure-window-surfacing-plan.md b/docs/plans/2026-07-09-001-fix-menu-stale-closure-window-surfacing-plan.md new file mode 100644 index 0000000..add1bc4 --- /dev/null +++ b/docs/plans/2026-07-09-001-fix-menu-stale-closure-window-surfacing-plan.md @@ -0,0 +1,282 @@ +--- +title: 'fix: menu stale-closure, window-close capability, and deep-link window surfacing' +date: 2026-07-09 +type: fix +status: planned +plan_depth: lightweight +--- + +# fix: Menu stale-closure, window-close capability, and deep-link window surfacing + +Group 2 of a multi-group port from a downstream fork. Three small, independent +defects ported as **behavior specs** (not patches) from `docs/to-improve/`: + +- `docs/to-improve/04-window-close-capability.md` +- `docs/to-improve/05-deep-link-surface-window.md` +- `docs/to-improve/06-menu-handler-stale-closure.md` + +Each is applied to match this repo's existing conventions. The specs' own code is +reference only — the ground truth below reflects the current state of this repo, +verified before planning. + +--- + +## Problem Frame + +Three latent defects, all reachable in the shipped desktop app: + +1. **Window won't close from the unsaved-changes dialog (spec 04).** The + `onCloseRequested` guard in `src/App.tsx` correctly calls `win.destroy()` on + both the guard-resolved ("Don't Save") path and the catch path — but the + Tauri capability set does not grant `core:window:allow-destroy`. A + frontend-initiated `destroy()` is blocked by the ACL, so choosing "Don't + Save" (or hitting the catch) silently fails to close the window. This is a + real present bug in the built app, not a theoretical one. + +2. **Deep link opens the doc but leaves the window hidden/behind (spec 05).** + The `on_open_url` deep-link handler in `src-tauri/src/lib.rs` emits + `deep-link-open` so the frontend loads the file, but never surfaces the main + window. If Quill is running hidden, minimized, or in the background, a + `quill://open?file=…` link loads the document invisibly — the user sees + nothing happen. + +3. **Cmd+S and other native-menu shortcuts fire stale handlers (spec 06).** The + menu-event effect in `src/App.tsx` registers Tauri listeners exactly once on + mount, reading handlers through a ref that is refreshed every render. But the + wiring aliases `const h = menuHandlersRef.current` at registration time and + dereferences `h.` inside each callback — snapshotting the **first render's** + handlers. Symptom: Cmd+S always routes to Save-As because `handleSave` closes + over the initial `openFilePath = null`. Every menu action except Open Recent + (which already reads the ref at fire time) and Clear Recent (which calls + module functions directly) is affected. The effect's own trailing comment + claims "handlers are read live through menuHandlersRef" — which is exactly + what the buggy alias breaks. + +## Scope Boundaries + +**In scope:** the three fixes above, plus one unit test for the menu +stale-closure (the only automated-testable piece). + +**Out of scope / explicitly excluded:** + +- Any other change to `src/App.tsx` beyond the menu-wiring fix. The + `onCloseRequested` guard already calls `win.destroy()` correctly on both paths + — do **not** re-touch it. +- The asset-protocol `$HOME` scope in `tauri.conf.json` — leave untouched. +- Broad window capabilities. Grant **only** `core:window:allow-destroy`, not a + blanket window permission set. +- The cold-start pending-deep-link path (`take_pending_deep_link` / + `PendingDeepLink`). Surfacing belongs on the warm `on_open_url` emit path where + a running window exists; cold start already launches and surfaces the app + through normal startup. +- Internal Amazon/Brazil fork adaptations (build wiring, dependency mirror, CSP + scrubbing, font vendoring, plugin repointing, update-checker removal) — never + ported. + +## Requirements + +- **R1** — Choosing "Don't Save" (or the catch path) in the unsaved-changes + dialog on window close actually closes the window. (spec 04) +- **R2** — A `quill://open?file=…` deep link received while the app is running + brings the main window to the foreground (shown, unminimized, focused) in + addition to loading the document. (spec 05) +- **R3** — Native-menu shortcuts (Cmd+S in particular) invoke the current + render's handlers, so Save saves to the open file rather than always prompting + Save-As. (spec 06) +- **R4** — The menu-handler-ref contract is protected by a unit test that fails + if wiring reverts to a registration-time snapshot. + +## Key Technical Decisions + +- **Grant exactly `core:window:allow-destroy` (spec 04).** Verified against + `src-tauri/gen/schemas/acl-manifests.json`: `core:default` bundles + `core:window:default`, which is read-only window commands and does **not** + include `allow-destroy`. `allow-destroy` is a valid grantable permission. + Adding just that one permission is the minimal grant that unblocks the + existing `win.destroy()` calls — no App.tsx change needed. Frontend-initiated + window operations are the only ones gated by capabilities; this is why the + Rust-side surfacing in spec 05 needs no capability change. + +- **Surface the window in Rust, best-effort (spec 05).** After the existing + `handle.emit("deep-link-open", path)`, resolve the main window via + `handle.get_webview_window("main")` (the `Manager` trait is already imported at + `src-tauri/src/lib.rs:10`; window label `"main"` matches the capability scope) + and call `show()`, `unminimize()`, `set_focus()`, ignoring each `Result` with + `let _ =` — the same best-effort style the surrounding closure already uses for + `emit`. Rust-initiated window ops are not capability-gated, so no + `default.json` change accompanies this. Additive only: a few lines inside the + existing `if let Some(path) = …` branch, after the emit. + +- **Read the ref at fire time, per callback (spec 06).** Delete the + `const h = menuHandlersRef.current` alias and make each wired callback + dereference `menuHandlersRef.current` when it runs, mirroring the Open-Recent + block that already does this correctly (`const cur = menuHandlersRef.current` + inside the callback). This keeps the register-once / read-live design the + effect's comment already documents — the fix makes the code match its stated + contract. `menu-clear-recent` already calls module functions directly (not via + `h`); leave it as-is. + +- **Unit-test the ref contract, not the whole effect (spec 06).** The failing + behavior is purely "does the wired callback read the latest ref value or a + snapshot?" A focused unit test that simulates a ref reassigned across renders + and asserts the wired callback invokes the latest handler pins the contract + without standing up Tauri's `listen`. Deep-link surfacing and the capability + grant are verified manually in a built `.app` (out of automated-test scope). + +## Implementation Units + +### U1. Grant `core:window:allow-destroy` capability + +**Goal:** Unblock the frontend-initiated `win.destroy()` so the unsaved-changes +dialog can actually close the window. (R1) + +**Requirements:** R1 + +**Dependencies:** none + +**Files:** + +- `src-tauri/capabilities/default.json` (modify) + +**Approach:** Add `"core:window:allow-destroy"` to the `permissions` array. +Grant only that one permission — do not add blanket window perms. The App.tsx +`onCloseRequested` guard already calls `win.destroy()` on both the +guard-resolved and catch paths; this is the only missing piece. + +**Patterns to follow:** the existing flat string-array entries in +`permissions` (e.g. `"dialog:allow-open"`). + +**Test scenarios:** `Test expectation: none — capability-manifest JSON change; +verified manually in a built .app by choosing "Don't Save" on window close and +confirming the window closes. No automated harness exercises the Tauri ACL.` + +**Verification:** `cargo test` and `cargo clippy -- -D warnings` still pass (the +capability file is validated at build time); the JSON remains well-formed and +`prettier`-clean. + +### U2. Surface the main window on warm deep-link open + +**Goal:** Bring the main window to the foreground when a `quill://open` link +arrives while the app is running. (R2) + +**Requirements:** R2 + +**Dependencies:** none + +**Files:** + +- `src-tauri/src/lib.rs` (modify — the `on_open_url` closure, ~`:1527`) + +**Approach:** Immediately after the existing +`let _ = handle.emit("deep-link-open", path);` (currently `src-tauri/src/lib.rs:1535`), +add a best-effort surfacing block: resolve `handle.get_webview_window("main")` +and, when present, call `.show()`, `.unminimize()`, `.set_focus()`, discarding +each `Result` with `let _ =`. `Manager` (providing `get_webview_window`) is +already imported at `src-tauri/src/lib.rs:10`; `handle` is already cloned before +the closure. Keep it inside the existing `if let Some(path) = parse_quill_open(…)` +branch. Do **not** touch the cold-start `PendingDeepLink` buffering — that path +surfaces via normal startup. + +**Patterns to follow:** the surrounding closure's own best-effort style — +`let _ = handle.emit(...)` and `let _ = app.emit(id, ())` elsewhere in `setup`. + +**Test scenarios:** `Test expectation: none — Tauri window surfacing has no +headless-testable seam here; verified manually in a built .app by minimizing the +window, opening a quill://open?file=… link, and confirming the window comes +forward with the document loaded.` + +**Verification:** `cargo test`, `cargo clippy -- -D warnings`, and +`cargo fmt --check` pass; the added lines compile with no unused-import or +unused-result warnings. + +### U3. Fix menu-handler stale closure + add ref-contract unit test + +**Goal:** Make native-menu callbacks invoke the current render's handlers, and +protect that with a unit test. (R3, R4) + +**Requirements:** R3, R4 + +**Dependencies:** none + +**Files:** + +- `src/App.tsx` (modify — the menu-event `useEffect`, ~`:626`–`:643`) +- `src/test/utils/menuHandlerRef.test.ts` (create) + +**Approach:** In the menu-event effect, remove the +`const h = menuHandlersRef.current` alias and rewrite each `wire(...)` callback +to read `menuHandlersRef.current` at fire time — e.g. +`await wire('menu-save', () => void menuHandlersRef.current.handleSave())` — for +all of: `menu-new`, `menu-open`, `menu-save`, `menu-save-as`, +`menu-export-pdf`, `menu-quit`, `menu-copy-diagnostics`, `menu-reveal-logs`. +Preserve the existing `void`/no-`void` shape per handler (the async ones keep +`void`). Leave `menu-clear-recent` (calls module functions directly) and the +Open-Recent block (already reads `menuHandlersRef.current` at fire time) +untouched. This makes the code match the effect's existing trailing comment, +"handlers are read live through menuHandlersRef." + +For the test: model the ref-read pattern in isolation. Build a small harness that +mirrors the wiring — a mutable ref object whose `.current` is reassigned to +simulate a later render, and a `wire(event, fn)` that stores `fn`. Fire the +stored callback **after** reassigning the ref, and assert it invokes the latest +handler, not the one present at registration. A registration-time snapshot +(`const h = ref.current`) must make the test fail; a fire-time read +(`ref.current.handler()`) must make it pass. + +**Patterns to follow:** the Open-Recent handler in the same effect +(`src/App.tsx:650`, `const cur = menuHandlersRef.current` inside the callback) is +the correct reference model. For the test file, mirror the structure and imports +of existing specs under `src/test/utils/` (Vitest `describe`/`it`/`expect`). + +**Test scenarios** (`src/test/utils/menuHandlerRef.test.ts`): + +- **Covers R4.** Fire-time read invokes the latest handler: given a ref whose + `.current.handleSave` is `fnA` at wiring time, wire a callback that reads + `ref.current.handleSave()`, then reassign `ref.current` so `handleSave` is + `fnB`, fire the callback, and assert `fnB` was called and `fnA` was not. +- **Guard against snapshot regression:** demonstrate that the fire-time pattern + survives multiple reassignments — reassign the ref twice and confirm the + callback always calls the newest handler. (This is the assertion a + registration-time alias would fail.) +- **Multiple events, independent handlers:** wire two events reading different + keys off the ref (`handleSave`, `handleNew`); reassign both; fire each; assert + each invokes its own latest handler. Confirms the pattern is per-callback, not + shared state. + +**Verification:** `npm run typecheck`, `npm run lint`, `npm run format:check`, +and `npm test` all pass; the new test fails if the wiring is reverted to a +`const h = menuHandlersRef.current` snapshot (sanity-check by temporarily +reintroducing the alias locally, confirming red, then reverting). + +--- + +## Risks & Dependencies + +- **Low blast radius.** All three changes are additive or localized. Spec 04 is a + one-line capability addition; spec 05 is a few additive Rust lines on an + existing branch; spec 06 removes an alias and inlines a ref read that the + Open-Recent path already proves works. +- **The two runtime-only fixes (U1, U2) have no automated coverage** — this is + inherent to Tauri capability ACLs and native window surfacing. Both are + verified manually in a built `.app`. The plan does not pretend otherwise; the + menu fix (U3), which _is_ testable, carries the unit test. +- **No cross-unit dependencies.** U1, U2, U3 are independent and can land in any + order; they ship together as one PR for the Group 2 theme. + +## Verification (whole change) + +Match the CI bar before opening the PR: + +- Frontend: `npm run typecheck`, `npm run lint`, `npm run format:check`, + `npm test`. +- Rust: `cargo test`, `cargo clippy -- -D warnings`, `cargo fmt --check` (run in + `src-tauri/`). + +Manual (built `.app`, out of automated scope, for the record): + +- Spec 04: open a dirty doc, close the window, choose "Don't Save" → window + closes. +- Spec 05: minimize/background the app, trigger a `quill://open?file=…` deep + link → window comes forward with the doc loaded. +- Spec 06: open a saved file, edit, press Cmd+S → saves in place (no Save-As + dialog). diff --git a/src-tauri/capabilities/default.json b/src-tauri/capabilities/default.json index 651579c..1b1567d 100644 --- a/src-tauri/capabilities/default.json +++ b/src-tauri/capabilities/default.json @@ -5,6 +5,7 @@ "windows": ["main"], "permissions": [ "core:default", + "core:window:allow-destroy", "opener:default", "dialog:allow-open", "dialog:allow-save", diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 8c590df..31b2deb 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1533,6 +1533,13 @@ pub fn run() { *lock_recover(&pending.0) = Some(path.clone()); } let _ = handle.emit("deep-link-open", path); + // Surface the window in case the app is running hidden, + // minimized, or in the background — best-effort. + if let Some(win) = handle.get_webview_window("main") { + let _ = win.show(); + let _ = win.unminimize(); + let _ = win.set_focus(); + } } } }); diff --git a/src/App.tsx b/src/App.tsx index 5594174..9f80ab9 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -631,16 +631,18 @@ export default function App() { const wire = async (event: string, fn: () => void) => { unlisteners.push(await listen(event, () => fn())); }; - const h = menuHandlersRef.current; - await wire('menu-new', () => h.handleNew()); - await wire('menu-open', () => h.handleOpen()); - await wire('menu-save', () => void h.handleSave()); - await wire('menu-save-as', () => void h.handleSaveAs()); - await wire('menu-export-pdf', () => h.handleExportPdf()); - await wire('menu-quit', () => h.handleQuit()); + await wire('menu-new', () => menuHandlersRef.current.handleNew()); + await wire('menu-open', () => menuHandlersRef.current.handleOpen()); + await wire('menu-save', () => void menuHandlersRef.current.handleSave()); + await wire('menu-save-as', () => void menuHandlersRef.current.handleSaveAs()); + await wire('menu-export-pdf', () => menuHandlersRef.current.handleExportPdf()); + await wire('menu-quit', () => menuHandlersRef.current.handleQuit()); await wire('menu-clear-recent', () => void syncRecentMenu(clearRecentFiles())); - await wire('menu-copy-diagnostics', () => void h.handleCopyDiagnostics()); - await wire('menu-reveal-logs', () => void h.handleRevealLogs()); + await wire( + 'menu-copy-diagnostics', + () => void menuHandlersRef.current.handleCopyDiagnostics(), + ); + await wire('menu-reveal-logs', () => void menuHandlersRef.current.handleRevealLogs()); // Open Recent replaces the document, so it runs through the same // unsaved-changes guard as File → Open and deep links. unlisteners.push( diff --git a/src/test/utils/menuHandlerRef.test.ts b/src/test/utils/menuHandlerRef.test.ts new file mode 100644 index 0000000..1caeda4 --- /dev/null +++ b/src/test/utils/menuHandlerRef.test.ts @@ -0,0 +1,96 @@ +import { describe, it, expect, vi } from 'vitest'; + +/** + * Regression guard for the native-menu handler wiring in App.tsx. + * + * Menu listeners are registered exactly once on mount, but the handlers they + * invoke change every render (they close over the latest openFilePath, dirty + * state, etc.). App keeps the current handlers in `menuHandlersRef`, reassigned + * on every render, and each wired callback must dereference + * `menuHandlersRef.current` AT FIRE TIME. + * + * The bug this protects against: aliasing `const h = menuHandlersRef.current` + * at registration time and calling `h.handleSave()` inside the callback, which + * snapshots the FIRST render's handlers. Symptom in the app: Cmd+S always + * routes to Save-As because `handleSave` closes over the initial + * `openFilePath = null`. + * + * These tests model the exact wiring shape from App.tsx (a ref reassigned + * across renders + a `wire(event, fn)` that stores the callback) so a revert to + * a registration-time snapshot fails here. + */ + +// Mirror of App.tsx's menuHandlersRef shape (only the fields the test exercises). +type Handlers = { + handleSave: () => void; + handleNew: () => void; +}; + +// Mirror of App.tsx's `wire`: registers a callback once, keyed by event name. +function makeWiring() { + const registry = new Map void>(); + const wire = (event: string, fn: () => void) => { + registry.set(event, fn); + }; + const fire = (event: string) => registry.get(event)?.(); + return { wire, fire }; +} + +describe('menu handler ref wiring', () => { + it('fires the LATEST handler after the ref is reassigned across renders', () => { + const first = vi.fn(); + const ref: { current: Handlers } = { + current: { handleSave: first, handleNew: vi.fn() }, + }; + const { wire, fire } = makeWiring(); + + // Correct pattern: read menuHandlersRef.current at fire time. + wire('menu-save', () => ref.current.handleSave()); + + // A later render swaps in fresh handlers. + const second = vi.fn(); + ref.current = { handleSave: second, handleNew: vi.fn() }; + + fire('menu-save'); + + // Must call the newest handler, never the registration-time one. + expect(second).toHaveBeenCalledTimes(1); + expect(first).not.toHaveBeenCalled(); + }); + + it('always tracks the newest handler across multiple reassignments', () => { + const ref: { current: Handlers } = { + current: { handleSave: vi.fn(), handleNew: vi.fn() }, + }; + const { wire, fire } = makeWiring(); + wire('menu-save', () => ref.current.handleSave()); + + // Reassign twice — a registration-time alias would be stuck on render #1. + ref.current = { handleSave: vi.fn(), handleNew: vi.fn() }; + const latest = vi.fn(); + ref.current = { handleSave: latest, handleNew: vi.fn() }; + + fire('menu-save'); + + expect(latest).toHaveBeenCalledTimes(1); + }); + + it('routes each event to its own latest handler independently', () => { + const ref: { current: Handlers } = { + current: { handleSave: vi.fn(), handleNew: vi.fn() }, + }; + const { wire, fire } = makeWiring(); + wire('menu-save', () => ref.current.handleSave()); + wire('menu-new', () => ref.current.handleNew()); + + const newSave = vi.fn(); + const newNew = vi.fn(); + ref.current = { handleSave: newSave, handleNew: newNew }; + + fire('menu-save'); + fire('menu-new'); + + expect(newSave).toHaveBeenCalledTimes(1); + expect(newNew).toHaveBeenCalledTimes(1); + }); +});