Skip to content

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

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/overlays-root
Sep 15, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/overlays-root

Conversation

@chihumyum

@chihumyum chihumyum commented Sep 7, 2026 •

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 / 65 call sites to 36 / 61.
  • 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 thread search and cancellation, the remembered Settings section, and the focus settle before Settings opens. The feature touches no bridge path and no browser global. The search services added in fix(desktop): stop canceled history searches and clear stale loading #5256 are folded into this adapter; request IDs and cancellation remain connected through the modal and controller.
  • Four files leave the renderer root (keyboard-help.tsx, command-palette.tsx, command-palette-types.ts, use-settings-modal.ts, 233 → 229 legacy files). The search hook and service seam from features/search move into this slice. 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 current SettingsOverlay 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

Validated on Node 24.19.0 / npm 11.19.0 against main 3f297e9aa:

  • Clean full workspace build; desktop test:dist 2673/2673 and UI test:dist 496/496.
  • Full workspace typecheck (desktop stories included), lint, format, Knip for Desktop and UI, ASF headers, Windows test inventory, Astryx inventory, and git diff --check.
  • Renderer architecture fixtures 112/112 and check:renderer-architecture --base upstream/main; OverlaysRoot remains the registered controller owner. AppShell hook gate: 36 hooks / 61 call sites.
  • Electron E2E: 6/6 across settings.spec.ts, sidebar-project-reload.spec.ts, and workhub-layout.spec.ts.
  • Overlay model, provider, boundary and adapter tests; a new integration test mounts the real OverlaysRoot, SearchModalHost, and Desktop adapter to verify request identity, supersession, dismissal, reopening, and unmount cancellation. Search navigation retains main's transcript-owned target clearing.

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 and implemented the original slice. Codex resolved the conflicts with current main, preserved search cancellation and Settings dismissal, updated the regression coverage, and ran the verification above. The human contributor owns final review and submission.

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.

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

Give keyboard help, the Command Palette, Search and Settings one owner in
features/overlays. OverlaysRoot owns useOverlaysController and hands a
stable command projection to AppShellContent; the legacy overlay layer
keeps the lazy Settings chunk and shell command list.

Preserve the current main search lifecycle: pass request IDs and
cancellation through the modal, controller and Desktop adapter, and let
the transcript reading-position owner clear navigation targets. Retain
SettingsOverlay as the single Escape owner across lazy chunk loading.

Register OverlaysRoot in controllerOwners and regenerate architecture and
Astryx inventories against main. Cover search cancellation through the
real overlay provider, modal and Desktop adapter.

Generated-by: Claude Code
Generated-by: Codex
@chihumyum
chihumyum force-pushed the refactor/overlays-root branch from 02f5542 to 649319e Compare September 15, 2026 09:48

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 649319e37 — reviewed along two angles (architecture/ownership and behavior/test integrity); both came back clean on the mechanics that matter.

The move is sanctioned: commands-and-overlays already listed these legacy paths and the ledger records the retarget to features/overlays; the slice mirrors task-entry (index/ports/services-context/controller/model/ui/testing), useOverlaysController is registered with OverlaysRoot as its single owner, and nothing lands outside approved zones. Ownership stays single — visibility state lives once in the controller, the shell reads the projection, overlay UI reads the context. No parallel path.

Behavior holds on every path traced: the four moved hotkeys, all six Settings openers (intent→section, persist key, blur-once), the #5256 debounce/cancellation contract — now with a real regression test through SearchModal — anyModalOpen/shellObscured, the e2e fixture seam, and the search scroll-target wiring. No surface was dropped; features/search/ leaves no dangling references.

Net prod +939/−707 — the shell sheds ~80 lines and a whole overlay concern; real entropy reduction, not churn.

P3s, none blocking:

  • app-shell-command-actions.ts:150 still cites the deleted useKeyboardHelp — reword to overlays.commands.openHelp or drop it.
  • model/settings-surface.ts shares its name with the 1264-line settings/settings-surface.tsx component; consider renaming or accept it.
  • OverlayFocusService is a port for a single document.activeElement.blur() — legal to inline in the controller; either way is fine.
  • In overlays-boundary.test.ts, the "leaves the shell with no overlay hook" test duplicates what check-app-shell-hooks.mjs already rejects — safe to delete.

One ask: the ledger retargets commands-and-overlays from application/overlays to features/overlays — a change to the declared migration plan, not just execution. Worth one line in the PR body so the destination change is on record.

中文版

两路评审(架构归属、行为/测试)在 649319e37 干净通过。迁移在账本计划内(commands-and-overlays 已列这些 legacy 路径),新 feature slice 结构对齐 task-entry,useOverlaysController 唯一 owner 已注册,无并行路径。行为逐条核实保持:热键、六个 Settings 开启路径、#5256 的 debounce/取消契约(且新增了走 SearchModal 的回归测试)、anyModalOpen、e2e fixture、search scroll-target 接线。净生产 +939/−707,真实减负。P3:过时注释引用已删 hook、model/settings-surface.ts 与现有组件重名、OverlayFocusService port 可内联、boundary 测试里一条与 check-app-shell-hooks 重复可删。另请正文补一句账本 destination 从 application/overlays 改为 features/overlays 的说明。

AI assistance: I used Devin with two delegated review passes (architecture and behavior/test integrity) over the checked-out head; findings were cross-verified by me.

@Astro-Han
Astro-Han merged commit f875dc8 into apache:main Sep 15, 2026
1 check passed
@chihumyum
chihumyum deleted the refactor/overlays-root branch September 17, 2026 17:35
chihumyum added a commit to chihumyum/maka that referenced this pull request Oct 3, 2026
…ined

The "Transitional feature exports outside Conversation" table scheduled
OverlaysConsumer and ManualDiagnosticReportConsumer for M5, with the
legacy command actions. Neither can leave the root within R2.
app-shell-overlays.tsx composes the legacy Settings surface, which it
imports lazily, and the palette command list that apache#4997 deliberately
kept as a shell injection point; feature zones may not import legacy
code. The manual report is the diagnostics owner's command handed to the
shell-built palette options. Both rows now record why they stay.

Refs apache#4582

Generated-by: Claude Opus 5.5
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.

2 participants