Skip to content

refactor(desktop): move the overlay surfaces below AppShell - #4997

Open
chihumyum wants to merge 1 commit into
apache:mainfrom
chihumyum:refactor/overlays-root
Open

refactor(desktop): move the overlay surfaces below AppShell#4997
chihumyum wants to merge 1 commit into
apache:mainfrom
chihumyum:refactor/overlays-root

Conversation

@chihumyum

Copy link
Copy Markdown
Contributor

Summary

Move the shell's overlay surfaces below AppShell: the keyboard help, the Command Palette, the Search modal, and the Settings modal now have one owner, features/overlays, with OverlaysRoot registered in controllerOwners as the only caller of useOverlaysController.

  • OverlaysRoot owns the four open flags, the Settings request with its three sub-surfaces (provider catalog, connection detail, provider create), the Search scroll target, and the global shortcuts (mod+k, mod+/, mod+?, bare ?). It hands the shell frame the overlays through a render prop the way TaskEntryRoot hands over taskEntry, so AppShell and AppShellContent call none of the four hooks: the gate inventory goes from 40 hooks / 71 call sites to 36 / 67.
  • The Settings surface is data. Every opener is an intent, openSettingsSurface applies it, and the section an intent lands on is what the adapter persists; the transitions are unit-tested without React. openProjectSettings still replaces the whole request, and closing still keeps the connection slug and create type for the next open, as before.
  • The keyboard help, the palette rows, and the Search modal render from the slice and read the controller directly. The legacy overlay layer keeps only what cannot leave the legacy zone, the lazy Settings modal and the palette's command list, and reads what to show through OverlaysConsumer; its props drop from 34 to 17, and the 15 pass-through values the shell used to thread into it are gone.
  • One Desktop adapter carries the thread search, the remembered Settings section, and the focus settle before Settings opens. The feature touches no bridge path and no browser global; use-shell-search.ts was the last renderer-root reader of window.maka.search.
  • Five files leave the renderer root (keyboard-help.tsx, command-palette.tsx, command-palette-types.ts, use-shell-search.ts, use-settings-modal.ts, 251 → 246 legacy files). The ledger's commands-and-overlays ownership entry now names features/overlays as the home of the two files that stay.
  • Kept as they were: the blur before a closed-to-open Settings transition (macOS menu commands), the palette's per-open command freeze (refactor(ui): restore @maka/ui host-agnosticism and relocate render-layer domain logic #1045), the Escape owner for the lazy Settings chunk, and the one-identity searchThread the Search modal's debounce depends on.
  • Not here: useSessionCollaborationDialog, the fifth modal in hasModalOpen, which needs openSettingsSection from this slice and follows separately; and the palette's command list, which stays a shell concern because its rows are shell actions.

Refs #4582

Verification

  • Node 24: desktop test:dist 2415/2415, @maka/ui and @maka/desktop typecheck (stories included), lint, format:check
  • check:renderer-architecture --base upstream/main with the new OverlaysRoot owner registration passing the guard; check:app-shell-hooks at 36 / 67; Astryx inventory; Knip (apps/desktop); ASF headers; git diff --check
  • New suites: overlays-model (9: every Settings intent, close, profile no-op, scroll-target consumption), overlays-provider-scope (5: closed initial projection, stable commands across opens, persisted section and single blur per closed-to-open, request replacement on a project open, scroll target handled once, service routing, the missing-root throw), overlays-boundary (8: services hook only in the controller, no bridge or shell dependency in the slice, the entry surface pinned to the shell / overlay layer / composition, adapter owned by composition, controller and fakes out of the entry, window.maka.search absent from every production module, one OverlaysRoot mount and overlay UI only in the overlay layer, no overlay hook left in the shell), overlays-services-adapter (2)
  • The Product/Command Search story mounts the palette through OverlaysRoot and opens it through the owner's command
  • The Electron E2E suite runs in CI; sidebar-project-reload asserts the Search modal, and the E2E fixture now opens it through the overlays command instead of the removed setter

Review focus

The hand-off and the nesting. OverlaysRoot sits inside TaskEntryRoot's frame and around AppShellContent, so an overlay change re-renders the shell frame exactly as the shell's own useState did before, and nothing above it. The only remaining injection point is commandOptions.

AI use

Select exactly one:

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

Tool(s) and scope: Claude Code designed the slice, wrote the implementation and tests, 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

Give the keyboard help, the Command Palette, the Search modal and the
Settings modal one owner outside the shell. `features/overlays` holds
their open state, the Settings request with its sub-surfaces, the Search
scroll target and the global shortcuts; `OverlaysRoot` is the registered
owner of `useOverlaysController` and hands the shell frame the overlays
through a render prop, so `AppShellContent` calls none of the four hooks.

The legacy overlay layer reads what to show through `OverlaysConsumer`
and keeps only the lazy Settings modal and the palette's command list,
which are shell concerns: its props drop from 34 to 17. The thread
search, the remembered Settings section and the focus settle before
Settings opens reach the feature through one Desktop adapter.

`AppShellContent` loses four hooks (40 to 36 in the gate inventory) and
five legacy files leave the renderer root.
@chihumyum
chihumyum force-pushed the refactor/overlays-root branch from ceaf524 to 6c7b6d5 Compare September 8, 2026 15:03
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 87797378c after #4986, #5001, #5003, #4985 and #5008 landed. app-shell.tsx merged on its own; the hand-resolved conflict was the hook-gate inventory (main took useEffect from 10 to 8, this branch removes useKeyboardHelp), and the ledger and Astryx inventory were regenerated with the OverlaysRoot owner registration and the commands-and-overlays ownership home replayed. Exact head: 6c7b6d549. Local verification on Node 24 is fully green (desktop test:dist 2531/2531, typecheck with stories, lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks now at 36 hooks / 65 call sites, Astryx inventory, Knip, ASF headers, git diff --check).

Posted by Claude Code on behalf of the PR author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant