Skip to content

refactor(desktop): extract Workbar as a feature slice - #3444

Open
chihumyum wants to merge 6 commits into
apache:mainfrom
chihumyum:refactor/workbar-controller
Open

refactor(desktop): extract Workbar as a feature slice#3444
chihumyum wants to merge 6 commits into
apache:mainfrom
chihumyum:refactor/workbar-controller

Conversation

@chihumyum

Copy link
Copy Markdown
Contributor

Summary

  • Extract Workbar layout, tools, and dynamic-resource lifecycles into a feature-owned vertical slice.
  • Keep Electron bridge access in a Desktop adapter; the Workbar feature no longer imports AppShell, preload, or main, and does not access window.maka.
  • Reduce AppShell to Workbar host inputs plus the host / commands / selectors surface while preserving storage, IPC, layout, mounting, and cleanup behavior.

Refs #3439

Verification

  • npm run lint — passed (2,513 files)
  • npm run format:check — passed (1,565 files)
  • npm run build — passed
  • npm run typecheck — passed
  • npm --workspace @maka/desktop test — passed (1,061 tests, 124 suites)
  • npx knip --workspace apps/desktop — passed
  • npx knip --workspace packages/ui — passed
  • npm run astryx:surface-inventory — passed (199 files, 1 exclusion)
  • npm --workspace @maka/desktop run build-storybook — passed
  • npm --workspace @maka/desktop run smoke:storybook — passed (162 stories)
  • npm --workspace @maka/desktop run e2e — passed (41 passed, 1 skipped)

Review focus

  • v1/v2 to v3 Workbar storage hydration and transient-field filtering
  • Terminal ownership, stale-start fencing, close/session-switch cleanup, and RAF disposal
  • Side Chat collapse/close/source-switch behavior and late fork/send disposal fencing
  • Desktop service mapping and subscription disposal, especially Inspector pagination and Session usage authority
  • CSS relocation/import order and right/bottom/narrow Workbar visual parity

AI use

Select exactly one:

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

Tool(s) and scope: Codex — architecture analysis, implementation, tests, latest-main replay, lifecycle race review, and pull request preparation. The human contributor reviewed the contribution and accepts responsibility for it.

All substantive commits include Generated-by: Codex; retain the trailer in the final squash commit.

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
chihumyum marked this pull request as ready for review August 21, 2026 17:25
@chihumyum
chihumyum force-pushed the refactor/workbar-controller branch from e42f8c2 to 13d9b75 Compare August 22, 2026 07:00

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

Thanks for extracting Workbar into a coherent feature slice. The production boundary looks clean: topology and persistence have one owner, Desktop IO stays behind the existing adapter seam, and the Terminal and side-chat lifecycle paths retain their stale-result and disposal fencing.

I found no blocking production issue. I left one non-blocking P2 inline because the narrow side-chat regression test currently does not resize the overlay it intends to exercise.

AI-assisted review disclosure: Codex coordinated independent architecture, lifecycle, and UI/test review passes against exact head 13d9b75cb3d0ed4d2bd44e4e4f3c16d837f70337; I reviewed the consolidated evidence and approve this exact head.

Comment thread apps/desktop/src/renderer/styles/workbar/shell.css
@chihumyum
chihumyum requested a review from Astro-Han August 22, 2026 08:06

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

Thanks for simplifying the Workbar into a real feature slice. The latest layout regression is fixed, the current-head test is green, and two independent passes found the extraction behavior-preserving across the major state, Side Chat, and Terminal boundaries. One recoverable Terminal cleanup edge remains in the extracted owner, noted inline.

AI-assisted review disclosure: OpenAI Codex coordinated two independent exact-head reviews. I verified the retained failure path, resolved prior thread, current-head CI, mergeability, and review state, and I made the final review decision.

if (stoppedTerminalKeysRef.current.has(key)) return;
stoppedTerminalKeysRef.current.add(key);
ownedTerminalResourcesRef.current.delete(key);
void terminal.stop({ sessionId, ref }).catch(() => undefined);

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.

[P2] This marks the resource stopped and removes ownership before terminal.stop() succeeds, then swallows rejection. On a reasonable Host disconnect/IPC failure, the remote shell can keep running while the tab and only cleanup ownership are gone; later cleanup will not retry because the key is already in stoppedTerminalKeysRef. Could we keep a pending-stop ownership entry until success (or hand it to a main-process retry authority), and add a stop-rejection test that proves cleanup remains retryable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9211905. Terminal cleanup now keeps ownership while stop is in flight, records completion only after success, and clears the in-flight marker on rejection so a later Session cleanup can retry. Added a controller regression test covering first-stop rejection followed by a successful retry. I also rebased onto upstream/main f83b469 and resolved the CSS move conflict while preserving the upstream chat-radius behavior. Local verification passed: full build, full typecheck, 36 directed tests, and 6/6 Electron Workbar E2E. @Astro-Han, could you please take another look?

@chihumyum
chihumyum force-pushed the refactor/workbar-controller branch 3 times, most recently from ecbe4d5 to 08aa100 Compare August 22, 2026 11:33

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

Thanks for taking this on — 83 files and +5813/-2957 of pure relocation is a lot of careful work, and the slice boundary you landed on reads well.

Reviewed at exact head 08aa1007b842e207ebf8c647d5382944b70f3c99 against base f2722fd143a575bb18e54ba30985fca16e2a7e2c. No blocking findings.

For a refactor PR the only claim that matters is "this is just a move", so that is what we checked:

  • Old paths are actually gone. session-workbar-layout.ts is deleted, use-shell-layout.ts drops 224 lines, app-shell.tsx drops 596, app-shell-effects.ts drops 60. No second Workbar authority survives alongside the new slice.
  • CSS relocation is cascade-equivalent. 1416 lines leave chat-detail.css / quote-side-panel.css and 1418 arrive in styles/workbar/*.css. The six new workbar/* imports now sit before chat-detail.css rather than inside it, which would matter if the 26 lines left in chat-detail.css shared selectors with the moved rules — they do not (what remains is .maka-turn[data-search-highlight] plus comments). side-chat.css is imported immediately after quote-side-panel.css, preserving its position relative to everything downstream.
  • @xterm/xterm/css/xterm.css moved from two component-level TS imports to a single top-level @import. It is unlayered in both forms, so its precedence over the layer(components) rules that style .xterm is unchanged. The only difference is that it is now always loaded instead of loaded on first terminal mount, which is not a behaviour change worth flagging in a desktop app.
  • Failure paths were preserved, not simplified away. stopTerminal marks a terminal stopped and releases ownership only inside .then(); a rejected stop keeps ownership so a later cleanup can retry. Terminal registration precedes tab commit, stale generations stop, and the Side Chat path keeps its dispose fence after each await.

Not merge-ready yet, for reasons outside the code: the branch is CONFLICTING against main, and there are no checks on this head at all — statusCheckRollup is empty. The existing APPROVED decision belongs to an older head; this repository does not dismiss stale reviews, so that badge is not evidence anyone has read the current code. Please rebase and let CI run; given the size of this diff, a conflict resolution is exactly where an accidental behaviour change would slip in, so we will re-check the moved logic on the new head rather than carry this result forward.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

@chihumyum
chihumyum force-pushed the refactor/workbar-controller branch from 08aa100 to 0c77971 Compare August 22, 2026 13:37
@chihumyum
chihumyum force-pushed the refactor/workbar-controller branch from 0c77971 to 1ee4ab5 Compare August 22, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants