Repository navigation
Conversation
d7f620f to
8ad5c0b
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
me2seeks
left a comment
There was a problem hiding this comment.
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
- [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-
anyerrors). A UI-geometry change with no manual window verification and no CI is a merge-gate gap, not a code defect. - [P3] When
ResizeObserveris unavailable, no measurement ever lands, sosessionWorkbarMaxWidthpins 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.
7eb5aab to
66dfa04
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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.
3f50541 to
4321ec3
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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.
|
Hi! Heads-up from #5763, which just merged — a few things changed underneath this PR, so the rebase will need some adjustment:
On the bright side, the measured ceiling approach fits the new model nicely and is more accurate than our |
4321ec3 to
361fd52
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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.
Screen.Recording.2026-09-30.at.09.50.46.mov |
361fd52 to
7bf6fb9
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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.
24393b4 to
7c7ab2a
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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
7c7ab2a to
4e44d77
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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
_minSizePxis 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.
Summary
main.Fixes #5578
Verification
mainat0235a5df899fbce9f5ead7a021231b4243c7b9c9; current head is4e44d77f5f1a9d110cb3bf679324b856d62c7271.npm ci, full workspace test build, renderer production build, workspace typecheck, lint, and format check passed on the rebased head.git diff --checkpassed.AI use
Select exactly one:
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
Does this PR entail a change in behavior?