Skip to content

feat(desktop): size workbar from available container space - #5577

Open
jiang1997 wants to merge 4 commits into
apache:mainfrom
jiang1997:feat/dynamic-workbar-width
Open

jiang1997 wants to merge 4 commits into
apache:mainfrom
jiang1997:feat/dynamic-workbar-width

Conversation

@jiang1997

@jiang1997 jiang1997 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Replace the Workbar's fixed 600px maximum width with a ceiling measured from the available shell layout space.
  • Align that ceiling with the current single-column shell layout and its 400px conversation floor. At the 600px window minimum, an explicitly opened Workbar can fit in the remaining 192px; resizing is disabled while the available room is below the normal 340px drag minimum.
  • Preserve the user's preferred Workbar width when the window narrows, and restore it when space becomes available again. Keep the compact rail behavior from the latest main.

Fixes #5578

Verification

  • Rebased onto main at 0235a5df899fbce9f5ead7a021231b4243c7b9c9; current head is 4e44d77f5f1a9d110cb3bf679324b856d62c7271.
  • Resolved the AppShell import conflict by preserving main's current Composer import and the Workbar layout container. Regenerated the renderer architecture ledger from the rebased source; strict comparison against the new base passed.
  • Clean npm ci, full workspace test build, renderer production build, workspace typecheck, lint, and format check passed on the rebased head.
  • Focused Workbar and shell layout tests passed: 88/88. Renderer architecture tests passed: 121/121.
  • ASF headers, App shell hooks, and git diff --check passed.
  • GitHub CI for the rebased head is pending. The previous-head CI run passed.

AI use

Select exactly one:

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

Tool(s) and scope: Maka assisted with the initial implementation; Codex rebased and adapted the width policy to the current shell layout, added regression coverage, and ran verification.

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

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 21, 2026
@jiang1997
jiang1997 force-pushed the feat/dynamic-workbar-width branch from d7f620f to 8ad5c0b Compare September 22, 2026 13:31

@hqhq1025 hqhq1025 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 the current head's Workbar width policy. No substantiated P0–P3 finding in the inspected changes.

The rail now retains the user's preferred width separately from the displayed width and derives its ceiling from the enclosing grid width minus the seam and conversation-column target (apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:114-145,287-333). The hook observes the parent grid rather than the sized child, measures synchronously on mount, disconnects the observer on cleanup, and persists only the preference (apps/desktop/src/renderer/features/workbar/controller/use-workbar-layout-state.ts:80-126,225-234). The shell passes the container ref and the same 4px gap it publishes to CSS (apps/desktop/src/renderer/app-shell.tsx:1230-1249,2317-2326; apps/desktop/src/renderer/shell/frame-style.ts:33-52). The model tests cover narrow/wide transitions and drag clamping.

This head has no visible checks, and it conflicts with current main in apps/desktop/renderer-architecture.json (git merge-tree failed); it is not merge-ready. git diff --check passed. I did not run local tests (Node 18/no installed dependencies) or validate real Desktop resizing, zoom, and responsive visuals. No schema/migration files changed. Resolve the conflict, run current-head checks, and obtain human UI acceptance before merging.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@hqhq1025 hqhq1025 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 the current head after its merge from main. I found no substantiated P0–P3 issue in the effective 11-file Workbar change.

The merge adapts the grid measurement to main's newer Workbar controller boundary: useWorkbarController now creates and passes layoutContainerRef to the layout hook, then returns that ref to the shell, which attaches it to the enclosing .maka-detail-with-artifacts grid (apps/desktop/src/renderer/features/workbar/controller/use-workbar-controller.ts:192-201,937-946; apps/desktop/src/renderer/app-shell.tsx:1171-1186,2280-2285). The underlying policy still keeps preferred and displayed widths separate, clamps drag changes to the measured ceiling, and observes the parent grid with cleanup (apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:114-145,287-333; apps/desktop/src/renderer/features/workbar/controller/use-workbar-layout-state.ts:80-126). No persistence schema or migration changed.

The branch is OPEN/MERGEABLE and merge-tree against fresh main plus git diff --check are clean. This head has no visible checks, so it is not CI-validated. I did not run local tests (Node 18/no installed dependencies) or a real Desktop resize/zoom/visual pass. Run the current-head gates and obtain human UI acceptance before merging. My earlier review is bound only to the old head and is not approval of this one.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@jiang1997

Copy link
Copy Markdown
Contributor Author
Screen.Recording.2026-09-26.at.12.37.14.mov

@hqhq1025 I recorded the Browser Workbar running in macOS Electron on the current PR head (8408052). The video shows me dragging the divider to narrow the Workbar and then widen it again, while the conversation column adjusts.

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

Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.

Summary

Replaces the Workbar's fixed 600px cap with a measured ceiling (container − gap − SESSION_CONVERSATION_MIN_WIDTH), keeping the user's width preference separate from the constrained display width so the rail narrows and recovers with the window. The issue (fixed cap truncating wide windows, stale preference on narrow ones) is real in base SESSION_WORKBAR_MAX_WIDTH. The design is right: one reducer authority, ResizeObserver on the parent grid (not a child it sizes, so no feedback loop), synchronous useLayoutEffect measure before first paint, and a single TS-owned gap constant shared by CSS and JS. All --agents-content-area-gap consumers verified inside .appFrame.

Findings

  1. [P3] No CI checks ever ran on this branch ("no checks reported"), and the author's own checklist leaves lint/format/typecheck unchecked — the full build was not reproduced locally (claimed pre-existing implicit-any errors). A UI-geometry change with no manual window verification and no CI is a merge-gate gap, not a code defect.
  2. [P3] When ResizeObserver is unavailable, no measurement ever lands, so sessionWorkbarMaxWidth pins the rail at the 480px default forever (apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:117-119) — a restored >480 preference would be permanently capped. Electron always has ResizeObserver; only non-Electron embeddings (tests/Storybook) hit this, where it is harmless.

Verdict

needs-discussion — code is solid, but the branch has zero CI signal and incomplete local verification for a layout change.

@jiang1997
jiang1997 force-pushed the feat/dynamic-workbar-width branch from 7eb5aab to 66dfa04 Compare September 28, 2026 01:49

@hqhq1025 hqhq1025 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 3f5054180829e61781e2d08b1fa36e25de3b16b8. I found no substantiated P0-P3 issue in the current Workbar sizing change.

The implementation keeps the saved preferred width separate from the constrained display width and computes a single ceiling from the measured container, shared gap, and conversation-column floor (apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:119-145,287-333). The layout hook measures synchronously, observes the parent grid rather than the child it sizes, ignores non-positive measurements, applies the same ceiling to drag updates, and disconnects the observer on cleanup (apps/desktop/src/renderer/features/workbar/controller/use-workbar-layout-state.ts:86-108). The provider owns a stable ref, and AppShell attaches it to the actual enclosing grid (apps/desktop/src/renderer/features/workbar/workbar-provider.tsx:44-50; apps/desktop/src/renderer/app-shell.tsx:2072,2257). The shell gap is published from the same TypeScript-owned value used by the model (apps/desktop/src/renderer/shell/frame-style.ts:28-47).

On Node 24.18.1, the focused Workbar/model/boundary tests passed (23/23), the renderer architecture suite and final check passed (112/112), and git diff --check was clean. Hosted test is green. Against current main 0fd7540831ebc4c38f6af9b96c8a510d7d087f1, the branch is 3 commits ahead and 4 behind; the merge tree is conflict-free. I did not run a real Electron/Chromium resize, drag, zoom, or visual pass around the 990px stacking boundary, and Storybook injects the width ceiling rather than exercising the production ResizeObserver path. Those remain human/UI validation gaps. No schema or migration files changed.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@hqhq1025 hqhq1025 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 4321ec320f8eec3ad545fbc175a516054d72b6cb.

Current-head result: no P0-P3 findings.

The branch rewrites the same three Workbar commits onto newer main. git range-diff maps all three commits; the behavior-bearing Workbar files are unchanged from the previously reviewed head, while the generated AppShell architecture count is rebased to the new base. The model still separates the persisted preference from the displayed width and derives its ceiling from container width, gap, and the conversation-column floor (apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:119-145,287-333). The layout hook measures the parent grid synchronously, observes that same parent, applies one ceiling to the renderer and drag handle, and disconnects cleanly (apps/desktop/src/renderer/features/workbar/controller/use-workbar-layout-state.ts:82-135). WorkbarProvider owns the stable ref and WorkbarLayoutContainer attaches it to the actual shell grid (apps/desktop/src/renderer/features/workbar/ui/workbar-provider.tsx:44-73; apps/desktop/src/renderer/app-shell.tsx:2234-2537). The shared 4px gap still comes from the frame-style authority (apps/desktop/src/renderer/shell/frame-style.ts:28-47).

Validation on Node 24.18.1: clean npm ci, build:test, full workspace typecheck, lint, format check, ASF headers, and git diff --check passed. Focused Workbar/model/controller/provider/boundary tests passed 67/67; renderer architecture tests and final check passed 112/112. Hosted test is green on this exact head. Against fresh main 8b104db12b70814b33f25b111021e2e8ec325d1f, the PR is 3 commits ahead / 1 behind; merge-tree 379634347ab838c939cb56f1068f2aea00e394df is conflict-free.

I did not run a real Electron/Chromium resize, drag, zoom, or visual pass around the 990px stacking boundary. Storybook still injects the width ceiling instead of exercising the production ResizeObserver path. No schema or migration files changed.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@Colafornia

Colafornia commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Hi! Heads-up from #5763, which just merged — a few things changed underneath this PR, so the rebase will need some adjustment:

  • The ≤990px stacked Workbar layout is removed; the shell is now a single right-column model down to the 600px window floor, with rail/Workbar space arbitration. A hard ≥340 ceiling on the Workbar would need a policy for the "no rail left to yield" case, where it can still render narrower (documented limitation).
  • The conversation floor is now the 400px token, so SESSION_CONVERSATION_MIN_WIDTH = 480 will need reconciling — one value should win.
  • Expect text conflicts around --agents-content-area-gap and the rightWidth rename.

On the bright side, the measured ceiling approach fits the new model nicely and is more accurate than our innerWidth-based room estimate — happy to help with the rebase, feel free to ping me!

@jiang1997
jiang1997 force-pushed the feat/dynamic-workbar-width branch from 4321ec3 to 361fd52 Compare September 29, 2026 03:24

@hqhq1025 hqhq1025 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 commit 361fd52. This revision aligns the Workbar width ceiling with the narrow shell: the 400px conversation floor and 4px seams are shared by the measured layout and frame cap, while a forced width below the normal 340px drag minimum leaves the saved preference intact. I checked the compact reveal/rail arbitration, width reducer and resize adapter, frame/titlebar cap, and the affected tests. I found no substantiated P0–P3 issue in these paths.

Node 24 clean installation and test build passed; 80 focused Desktop tests, renderer architecture and ASF checks passed. The current-head hosted test is green, and diff-check plus a merge-tree against main 0323714 are clean. I did not manually exercise Electron window resizing/dragging or cross-platform visual behavior; those remain UI acceptance checks. No schema or migration change is present. This is not a merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@jiang1997

Copy link
Copy Markdown
Contributor Author
Screen.Recording.2026-09-30.at.09.50.46.mov

@jiang1997
jiang1997 force-pushed the feat/dynamic-workbar-width branch from 361fd52 to 7bf6fb9 Compare September 30, 2026 02:03

@hqhq1025 hqhq1025 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 7bf6fb902b20a49abce33478b870fb8627ca61c9. The Workbar now measures its detail-grid container and caps the displayed right-panel width to the space left after the 400px conversation floor and gap (apps/desktop/src/renderer/features/workbar/controller/use-workbar-layout-state.ts:91-113, apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:129-160). The reducer keeps the persisted width preference separate from the measured ceiling, and disables drag resizing below the normal 340px minimum (workbar-layout.ts:341-352,384-403). I checked the provider/ref wiring, shell CSS cap, persistence, and the focused model/boundary tests. I found no substantiated P0–P3 in these paths.

The exact-head test check passes. A merge-tree and git diff --check against current main 28cc4e647 are clean. I did not independently run the full suite or inspect a rendered Electron window across width breakpoints, so visual behavior remains unverified by me. This is not a merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@jiang1997
jiang1997 force-pushed the feat/dynamic-workbar-width branch 2 times, most recently from 24393b4 to 7c7ab2a Compare September 30, 2026 06:37

@hqhq1025 hqhq1025 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 the current Workbar sizing head. The latest narrow-window change uses the shared 4px shell seam when measuring the available right-column width (apps/desktop/src/renderer/features/workbar/controller/use-workbar-layout-state.ts:98), permits the rendered Workbar below the normal 340px drag minimum without overwriting the saved width, and disables resizing when no valid drag range exists (apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:143,387). The shell CSS token and layout contract both use 4px. I found no substantiated P0-P3 issue in the inspected sizing, persistence, and compact-window paths.

The current-head hosted test check is green; fresh-main merge-tree and diff check are clean. With dependency patches applied, local core/storage/runtime/runtime-host/UI/MCP/computer-use and Desktop main builds, the renderer architecture guard, and 26 focused Workbar tests pass. I did not independently verify real Electron resizing across window sizes, packaged Desktop, or the full local suite. This comment is not merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Replace the fixed 600px workbar limit with a ceiling derived from the
layout container. Preserve the user’s preferred width separately from the
constrained display width, so resizing the window temporarily narrows the
rail without losing the preference.

Measure the parent grid with ResizeObserver and apply the same dynamic
ceiling to the reducer, resize handle, and rendered width. Publish the
shared shell gap from one TypeScript source for both layout calculation
and CSS, including standalone Storybook surfaces.

Keep the existing stacked narrow-window layout unchanged.

Generated-by: Maka
@jiang1997
jiang1997 force-pushed the feat/dynamic-workbar-width branch from 7c7ab2a to 4e44d77 Compare October 1, 2026 14:35

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

Reviewed 4e44d77f5f1a9d110cb3bf679324b856d62c7271 (11 files, +313/−51; 4 commits). The change stops sizing the Workbar from a fixed 600px cap and derives its ceiling from the measured container instead.

No P0–P3 findings.

What the change does. The persisted width becomes a preference and the model gains a ceiling that measure-right-ceiling writes. sessionWorkbarCeiling(containerWidth, gap) is container − gap − SESSION_CONVERSATION_MIN_WIDTH; the displayed width clamps the preference between the normal 340px drag minimum and that ceiling. SESSION_CONVERSATION_MIN_WIDTH aliases SHELL_CONVERSATION_MIN_WIDTH_PX, and I checked that the contract's 400/4 pair is the same pair the shell CSS token carries, so the claim that the measurement matches the token holds.

What I checked in the policy, since a measured ceiling is where this kind of code usually fails:

  • A narrow window holds back the display width and never overwrites the stored preference, so widening gives the user's chosen width back — and persistence writes only the preference.
  • A drag below the minimum is refused outright while maxWidth < SESSION_WORKBAR_MIN_WIDTH, so a forced narrow display can never be saved as the user's choice; the drag also starts from the displayed width, so a narrowed window drags from what the user can see.
  • The ResizeObserver measures the parent grid, not a child whose width the ceiling determines, which is exactly the feedback loop to avoid — and the handler does a synchronous pass in the same commit, so the first painted frame already has a ceiling instead of rendering at the default and then jumping.
  • An unreadable (<= 0) or unchanged measurement returns the identical state, so the observer cannot dispatch in a loop; the reducer also ignores non-finite ceilings and clamps the stored value at 0. There is a test for the unreadable/unchanged case.
  • The handle's _minSizePx is lowered to the ceiling (Math.min(MIN, rightWidthMax)) so the drag stops where the reducer stops it rather than snapping back.
  • With no measurement yet, the ceiling falls back to the default rather than the stored preference, which is the safe choice on the first frame.

Wiring. WorkbarLayoutContainer is substituted for the raw grid div in app-shell.tsx and still renders className="maka-detail-with-artifacts" with the ref attached — which matters because use-focused-preview finds its frame through closest('.maka-detail-with-artifacts'), and a boundary test pins that exact element. Using the container outside its provider throws instead of silently measuring nothing. The architecture ledger's token count moves by one (downward), so the budget was regenerated rather than hand-edited, and the artifacts.css comment that referenced the removed constant was updated with it.

Tests cover the three claims that matter: sizing from the measured container rather than a fixed cap, an explicitly opened Workbar fitting below its drag minimum at the window floor, and an unreadable or unchanged measurement being ignored.

Gate on this head: test is green.

What I could not judge

  • No real Electron window resizing, no packaged Desktop, and no full local suite run; sizing behaviour rests on the model, the wiring and the tests I read.
  • I did not verify the visual result at the window floor beyond what the tests assert.

I did not approve, request changes, or merge.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

This branch has not been deployed

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

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): Workbar cannot expand beyond 600px

5 participants