Skip to content

refactor(desktop): move Workbar ownership below AppShell - #4692

Open
theSatvik wants to merge 1 commit into
apache:mainfrom
theSatvik:refactor/workbar-controller-scope
Open

refactor(desktop): move Workbar ownership below AppShell#4692
theSatvik wants to merge 1 commit into
apache:mainfrom
theSatvik:refactor/workbar-controller-scope

Conversation

@theSatvik

Copy link
Copy Markdown

Summary

  • Move production useWorkbarController ownership from AppShell into a feature-owned WorkbarProvider.
  • Let WorkbarHost and the titlebar restore action consume narrow feature contexts instead of receiving the controller model through the shell.
  • Keep shell intents on a stable per-shell bridge and expose only the equality-selected hidden-session projection needed by Session Navigation.
  • Preserve Workbar shortcuts, persistence, and Terminal/Side Chat resource lifecycles.

Refs #4582

Verification

  • fnm exec --using=24.16.0 npm run build:test
  • fnm exec --using=24.16.0 npm --workspace @maka/desktop run build:renderer
  • fnm exec --using=24.16.0 npm run typecheck
  • fnm exec --using=24.16.0 npm --workspace @maka/desktop run test:dist — 2,062 passed
  • Focused Workbar and Session Navigation suites — 25 passed
  • fnm exec --using=24.16.0 npm run check:renderer-architecture -- --base origin/main
  • npm run lint, npm run format:check, Desktop/UI Knip, AppShell hook ratchet, Astryx inventory, and ASF header check
  • Automated adversarial review found no actionable correctness issue.
  • No screenshot: this is a behavior-preserving ownership/render-scope refactor with no visual change.

The aggregate npm run test:dist also exercises unrelated workspaces. The affected Desktop workspace passed completely; the aggregate remained non-zero because the unchanged Runtime Host test WorkHub correction replaces its link without stopping a shared manual Turn fails locally (this branch has no packages/runtime-host diff). Eval's JavaScript suite passed, and its Python suite passed under Python 3.11 (82 tests, 12 skipped); the system python3 is Python 3.9 and cannot parse the suite's X | None annotations.

AI use

Select exactly one:

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

Tool(s) and scope: OpenAI Codex assisted with implementation, tests, documentation, local validation, and automated review. The affected commit carries the required Generated-by: Codex trailer.

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

Own the Workbar controller in a feature provider, expose only stable shell commands plus the hidden-session projection, and let host/titlebar consumers read scoped contexts.
Generated-by: Codex
@github-actionsgithub-actionsBot added the effort/L Under 1000 readable lines label Sep 3, 2026

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

Reviewed exact head 14f0a65516ea0ad5e32a03fd1c62f24adf148d93 against base/current main74a20f60c9a7bb6947e2c428f07ca2e83fad92a1.

I found no actionable P0-P3 issues in the reviewed scope.

The change moves useWorkbarController into the feature-owned provider (apps/desktop/src/renderer/features/workbar/ui/workbar-provider.tsx:70), exposes stable imperative shell commands plus the hidden-session external-store projection (apps/desktop/src/renderer/features/workbar/controller/workbar-shell-bridge.ts:51), and keeps the Session rail subscribed through useSyncExternalStore (apps/desktop/src/renderer/features/session-navigation/controller/use-session-navigation-reads.ts:73). I traced the active-session transition, Terminal cleanup, Side Chat cleanup, bridge publish/disconnect, titlebar/host context reads, AppShell command call sites, and E2E fixture path.

Local validation passed: focused Workbar/Session Navigation tests (25/25), Desktop full test suite under Node 24.18.1 (2062/2062), Desktop typecheck, production Desktop build, renderer architecture checks (71/71), AppShell hook ratchet, lint, format check, ASF headers, and git diff --check. The merge tree is clean and current main equals the PR base.

Hosted CI has not executed: the CI run is terminal action_required with no jobs, and only the label check is green. I also did not run an interactive Electron/macOS smoke test. This is a refactor, so the final design and merge decision remains with a human reviewer.

Automated review notice: This review was produced with AI assistance and does not replace independent human review.

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks—moving ownership out of AppShell while keeping a single Workbar controller is a useful direction.

#4789 has now merged, so please rebase onto current main and adapt the provider/bridge to the new TabList and open/close menu. Keep the retired Tasks face, drag-reordering, and preview paths removed. Then validate the focused Workbar behavior and refresh CI; the earlier checks don't establish compatibility with the new shell.

AI-assisted review with Codex.

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

Labels

effort/LUnder 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@theSatvik@Astro-Han@hqhq1025