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

@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.

@chihumyum
chihumyum force-pushed the refactor/overlays-root branch from 6c7b6d5 to 4b2b3d1 Compare September 9, 2026 15:03
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main fb8b15d08 (24 commits since the previous base). Hand-resolved: the AppShell mount, where #4979 turned AppShellContent's props into one spread and this branch adds overlays to that spread inside OverlaysRoot's render prop; the composition file, where the new WorkHubServicesProvider now wraps OverlaysServicesProvider; and the hook-gate inventory (useEffect 7). One semantic merge worth a look: #4979's openWorkHub closed Settings through the removed setSettingsOpen(false), so it now calls overlays.commands.closeSettings() (the raw surface close, not the shell's closeSettings with its onboarding re-read, matching what the setter did). Ledger and Astryx inventory regenerated with the owner registration and ownership home replayed. Exact head: 4b2b3d124; local verification on Node 24 fully green (desktop test:dist 2460/2460, typecheck with stories, lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks at 36 hooks / 62 call sites, Astryx inventory, Knip, ASF headers, git diff --check).

Posted by Claude Code on behalf of the PR author.

@chihumyum

chihumyum commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han this one is ready for your look when you have time: the Commands / overlays slice claimed on #4582, sitting on main 84255571b at 02f554237. The two things I would want checked first are the hand-off shape (OverlaysRoot inside TaskEntryRoot's frame, overlays reaching AppShellContent through the render prop, no overlay hook left in the shell) and the split forced by the checker: the state, shortcuts, help, palette rows and Search modal live in features/overlays, while the lazy Settings modal and the palette's command list stay in the legacy overlay layer because neither the feature nor the application zone may import settings/. AppShellContent goes from 40 to 36 hooks in the gate inventory; the Settings surface is a pure model with its own tests. app-shell.tsx moves almost daily on main and each rebase re-touches this diff, so an early pass would save both of us a few rounds.

Posted by Claude Code on behalf of the PR author.

@chihumyum
chihumyum force-pushed the refactor/overlays-root branch 2 times, most recently from 372e1f1 to 96340b4 Compare September 9, 2026 15:52
@chihumyum

Copy link
Copy Markdown
Contributor Author

For the record: the one red run on this PR (at 372e1f163) failed workhub-layout.spec.ts on the anchors-rail drag (scrollLeft stayed 0 after the 300px drag). The same branch content passed that test at the previous head, the only main commit in between (#5057) touches the transcript scroll in packages/ui, the rail's pointer-capture drag is untouched here, and the run at 96340b4d4 on main 8c02f3d32 is green again with no code change, so I am treating it as a one-off of the window-bounds restore that precedes the drag rather than something this PR changed.

Posted by Claude Code on behalf of the PR author.

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 96340b4 to 02f5542 Compare September 10, 2026 13:54
@chihumyum

Copy link
Copy Markdown
Contributor Author

The Desktop E2E step is red again at 02f554237, this time on workhub-layout.spec.ts:295 (the second queued follow-up after the WorkHub renderer recovery: Enter left the text in the composer, so the queue stayed at one entry). Evidence that this is the WorkHub spec on the Linux runner and not this branch:

  • The branch content has not changed across the last six heads (rebases only). Their CI history for the Desktop E2E step is green, green, green, red, green, red, and the two reds fail two different assertions in the same spec (the anchors-rail drag at :75, then the queued Enter at :353).
  • Locally on 02f554237 (macOS, built with build:with-deps), workhub-layout.spec.ts passes 6 of 6 across three repeats of both tests.
  • The failure screenshots show no overlay in either window: the drag screenshot has the WorkHub rail unscrolled with the draft intact, the queue screenshot has one queued entry and the second text still in the editor, and the main window shows the native WorkHub view over the content area, which is the expected docked state.
  • Nothing in this PR touches the WorkHub renderer's composer, the rail's pointer-capture drag, or Enter handling; the only shortcuts it registers are mod+k, mod+/, mod+? and bare ?, and none of the typed text contains those.

I am not going to keep force-pushing to re-roll the E2E; the next rebase (which main's pace makes likely within the day) will re-run it. If a maintainer can re-run the failed job in the meantime, that would tell us the same thing faster.

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