refactor(desktop): take the overlays review follow-ups

[chihumyum]

Summary

The four non-blocking P3s from the #4997 approval, plus the destination note it asked for.

• app-shell-command-actions.ts: the comment on onOpenShortcuts no longer cites the deleted useKeyboardHelp; it now says the shell hands in the overlays owner's openHelp command and why that beats a synthetic KeyboardEvent.
• features/overlays/model/settings-surface.ts is now model/settings-modal-state.ts, and its symbols follow (SettingsModalState, SettingsModalIntent, openSettingsModal, closeSettingsModal, CLOSED_SETTINGS_MODAL), so nothing in the slice shares a name with the SettingsSurface component in settings/settings-surface.tsx. The only importers are the controller, the projection type, the testing entry, and the model test.
• overlays-boundary.test.ts drops the case that asserted no overlay hook is left in app-shell.tsx; check-app-shell-hooks.mjs already rejects exactly that.
• The slice README records why the ledger's commands-and-overlays destination moved from application/overlays to features/overlays: the owner guard registers owners only under features/<slice>/index.ts, and neither the application nor the feature zone may import legacy code, so the two legacy files stay on the entry's list until the Settings modal and the command builders can follow.

Kept as is: OverlayFocusService. The review called inlining the single blur() legal either way; the port keeps the controller free of DOM access and lets overlays-provider-scope.test.ts assert the blur-once-per-closed-to-open rule without a document.

No runtime behavior changes. Refs #4582

Verification

• Node 24: desktop test:dist 2675/2675, typecheck (stories included), lint, format:check
• check:renderer-architecture --base upstream/main, check:app-shell-hooks, Astryx inventory (regenerated for the renamed file), Knip (apps/desktop), ASF headers, git diff --check

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code applied the rename, the comment and README edits, and ran the verification; the human contributor reviewed the work and chose to submit it.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[Astro-Han]

Reviewed against gh pr diff plus the PR head (414ae07e6). All four P3s from the #4997 approval and the destination note are addressed, and I could not find a behavior change. Notes and two nits below.

Verified

The rename is mechanical. Normalizing the identifier renames, model/settings-surface.ts (main) and model/settings-modal-state.ts (PR head) are byte-identical, so the state machine — the request merge/replace rules, the sub-surface resets, closeSettingsModal's early return — is untouched. The use-overlays-controller.ts, overlays-projection.ts, and testing.ts hunks are identifier-only. No dangling references at the PR head: SettingsSurface* / openSettingsSurface / settings-surface.js now resolve only to the unrelated settings/settings-surface.tsx component, so the name collision with the 1264-line component is genuinely gone.

renderer-architecture.json regeneration is exact. I re-parsed both versions of app-shell-command-actions.ts with the checker's own parser config (explicitResourceManagement, importAttributes, jsx, typescript, tokens: true): 2299 → 2298 tokens, matching the committed nonTriviaTokens and nothing else in that entry. The renamed model file needs no ledger update — legacyAppShell.files only inventories shell/legacy paths, and features/overlays is not in legacyRendererFiles, legacyFeatureImports, or the Astryx surface inventory.

The reworded comment is accurate. openHelp in the shell really is overlays.commands.openHelp (app-shell.tsx:360-371 destructures the projection's commands and openHelp is passed down into the command-list options), and the foot-gun it names is real: the controller registers a bare ? hotkey (use-overlays-controller.ts, { keys: '?', onPress: … }), so a synthetic KeyboardEvent for ? would have gone through the global listener.

Dropping the boundary test loses no coverage. scripts/check-app-shell-hooks.mjs fails closed on any use* call in AppShell/AppShellContent (unknown names → added, above budget → grown, below → stale), and app-shell.tsx defines only those two components, so the deleted assertion — six specific overlay hook names anywhere in the file — is subsumed by a strictly name-agnostic gate. Nothing in the repo referenced the deleted test by name.

The README's Destination section checks out. commands-and-overlays did move application/overlays → features/overlays (in #4997, f875dc8f3), and the two reasons given are the real ones: check-renderer-architecture.mjs forbids legacy targets from both the application and feature source zones, and it requires production consumers to reach an owner through the slice's public entry. The two files left on the legacy list (app-shell-overlays.tsx, app-shell-command-actions.ts) are exactly the ledger's legacyPaths.

Keeping OverlayFocusService is defensible and matches "either way is fine": ports.ts:39-45 already records why the port exists (macOS menu commands open Settings without moving DOM focus), and overlays-provider-scope.test.ts:117-145 asserts the blur-once-per-closed→open rule through an injected fake — which inlining into the controller would put back behind a document.

For a pure rename + comment + doc change there is nothing meaningful to test; typecheck plus the existing model tests cover the symbol moves.

Nits

1. features/overlays/README.md:56 — the new "Model" line is 84 columns, while the rest of the file wraps near 80 (only the pre-existing Ownership bullet at line 33 is longer). Biome doesn't format markdown, so format:check won't flag it; rewrapping keeps the file's rhythm.
2. features/overlays/README.md (Destination) — "the controllerOwners guard registers owners only under features/<slice>/index.ts" is a hair off: the ledger's owner field is features/overlays/ui/overlays-root.tsx. What the guard ties to index.ts is that production consumers must import the owner through the public entry, and that index.ts must not expose the controller itself. Worth saying precisely, since this paragraph is the record of why the destination is features/overlays.

Not an issue, just noting: main is 30 commits ahead of the merge-base and touched app-shell.tsx and renderer-architecture.json; a git merge-tree dry run merges cleanly, so no rebase friction expected.

[Astro-Han]

Clean follow-up — every claim verified against the code.

• Rename is complete and the disambiguation is real: all five consumers updated, remaining SettingsSurface references belong to the component that deliberately keeps its name, and features/overlays/index.ts never re-exported the model so nothing leaks through the barrel.
• No behavior change: the renamed model is byte-identical modulo identifiers; setSettings(closeSettingsModal) keeps the same updater shape.
• The dropped boundary test is genuinely redundant — check-app-shell-hooks.mjs counts every hook in both render bodies against an exact ALLOWED inventory that fails on growth AND shrinkage, so it covers the same scope with a broader net, and it runs in CI.
• The README destination note matches the actual guard rules (controller ownership, index-barrel enforcement, legacy import budgets) as wired in check-renderer-architecture.mjs.
• OverlayFocusService staying is the right call — the port keeps the controller DOM-free and is what makes the blur-once-per-closed-to-open rule testable without a document.