fix(desktop): rebuild the workbar shell on TabList and one open/close control - #4789
Conversation
e4f7996 to
6885bdb
Compare
68881d6 to
69ccc9c
Compare
… control The right workbar drew its own tab strip: a hand-written `role="tablist"` div of ghost `Button`s with a close `IconButton` beside each, dnd-kit drag reorder, a context menu of move/close verbs, and a preview/pin state no renderer code ever set. It read as a browser tab bar without being one, and the strip's container had no `min-width: 0`, so at the panel's floor the tabs spilled past the edge and pushed [+] and the collapse toggle off screen instead of scrolling. The strip is now Astryx's `TabList`. That settles what the hand-written one could not decide: - `Tab` renders `endContent` inside its own `<button>`, so a per-tab close would nest a button in a button, and `TabList` warns when a `role=tablist` strip's direct children are not tabs. Opening and closing therefore share the `[+]` menu, which lists every registered tool and marks the open ones with a checkmark — the row-end mark `Selector` documents as the house convention, not a boxed checkbox, and with no shortcut column, since the launcher below already lists every face with its own. - Nothing reorders tabs any more, so the strip's order is the order the faces were opened in, and dnd-kit leaves the dependency tree with it. - The strip lives in a `min-width: 0` flex item and takes its height from the titlebar clearance it stands in, never the other way round. - The rail under the tabs belongs to the bar, not to the strip. `TabList hasDivider` draws it on the strip's own box, which ends where the tabs do, so it ran out from under `[+]` and the collapse toggle and read as a line beneath half a row. `Tab` names a second idiom for exactly this — a toolbar with a bottom divider, handing the tabs `--_tab-indicator-bottom` so the selected indicator drops through the bar's gap and lands on the rail — and that is what the bar does now. Alongside the shell: - The Task face is retired; its ledger is folded into Work Board separately. The renderer's read path goes with it — the hook, `SessionTodoPanel`, its stylesheet, its shared copy, and `WorkbarTodoService`, which had no caller left once the hook went. `todo:read` and `querySessionTodo` stay: that is the boundary Work Board will read through, and it carries its own projection tests. - Review carried `git-branch`, which read as "switch branch". The face lists changed files and their diffs, so it carries `file-diff`. - `WORKBAR_TOOL_DEFINITIONS` is now the only place a face's icon, shortcut and order are written. The strip, the `[+]` menu and the launcher all read it instead of keeping three hand-maintained lists in the surface. - The panel's floor rises from 320 to 340, the width `astryx docs layout` budgets for a detail panel and the point where five tabs stop fitting (386px needed against 260px available). The `turn-narrative` e2e fixture opened the workbar on the Task face and seeded a to-do ledger for it. The scenario is about focus order through the transcript, so it opens 变更 instead and no longer seeds what nothing renders. The workbar story frame declares `--maka-plate-titlebar-clearance`, which AppShell puts on the frame holding the plates. Without it the bar's grid row collapsed and the strip floated mid-panel, so the story could not show the shell it exists to pin. Refs #2188 Generated-by: Claude Code
69ccc9c to
56d2c71
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 56d2c717. The [P1] is closed — the accessibility journey now matches the shell it tests, and the E2E actually ran this time. 17 checks pass, one skipping.
Why this round's green means something
The previous head reported passing checks too, but the test job exited at the stale astryx inventory step, so Knip and the Electron E2E never ran — everything after that point was unexecuted rather than passing. I read that log and called the failure mechanical, which was wrong: a job that stops early hides whatever would have failed next, and both did. This head runs them.
What was fixed
Both failures I reported are gone, verified by running that spec directly on this head:
:151no longer looks for the任务待办region or its todo listitems — the fixture stopped seeding SessionTodo when the Task face was retired, so the assertion was outliving its subject;:306no longer waits on the old accessible name打开工作栏标签, which has no match in production since the open/close control was unified.
The part that mattered most is the one that a rename alone would have missed. The registry walk that follows those assertions would still have tried to click the retired 待办 entry, so the journey needed to be updated to the current registry rather than to the current string. The comment now states that directly — registry order, minus the Task face this shell retired — which makes the next reader's job easier than a silent list change would have.
The earlier Knip failure is also resolved: the unused singular closeSessionWorkbarTab export is gone, and the plural form that remains has a caller.
The rest, confirmed earlier and unchanged
The deletion surface is clean — no dangling production consumers for SessionTodoPanel, useSessionTodo, WorkbarTodoService, the desktop adapter, or the Task registry entries, and dnd-kit has no remaining importer.
SessionTodo's durable data is untouched. workflow_session_todo_documents is neither migrated nor deleted; the SQLite store, revision/copy/purge, todo_read / todo_write, the Host query and the preload IPC all remain. Old todos stay on disk and agents can still read and write them — what was retired is the Desktop renderer face. That is a product decision rather than a defect, and worth being deliberate about, but nothing is lost.
The simplification is real: dnd-kit reordering, per-tab close, pin-preview and context-menu close-others are gone, and the reducer dropped reorder / move / pin / preview. Complexity was removed rather than relocated into TabList, which is presentation.
One non-blocking note carried over: the title and README describe one open/close control, while production still has several writers — the [+] menu, the empty-strip launcher, keyboard shortcuts, commands.openTool, the Files Esc path, and the strip/titlebar column toggle. The Files Esc path is the one worth a second look, since it collapses the whole column rather than closing the files face — a different action from the other five. onOpenLauncher is also still plumbed through the host without ever being called.
This is a UI refactor, so the merge decision remains a human's.
简体中文
在 56d2c717 上批准。那条 [P1] 已关闭——无障碍 journey 现在与它所测试的 shell 一致,而且这一轮 E2E 确实跑了。17 项检查通过,一项跳过。
为什么这一轮的绿是有意义的
上一个 head 同样报告过检查通过,但 test job 在 astryx inventory 过期那一步就退出了,于是 Knip 与 Electron E2E 根本没有运行——那之后的一切是「未执行」而不是「通过」。我读了那份日志,把这次失败判成机械性的,这是错的:一个提前停止的 job 会藏起它接下来本会失败的一切,而那两项都失败了。 这个 head 把它们跑起来了。
修了什么
我报告的两处失败都消失了,在这个 head 上直接运行该 spec 验证过:
:151不再去找任务待办region 及其 todo 列表项——Task face 退役时 fixture 就不再 seed SessionTodo,那条断言比它的对象活得更久了;:306不再等待旧的可访问名打开工作栏标签,自 open/close 控制统一之后,它在生产中已无匹配。
最要紧的是「只改名字会漏掉」的那一部分。 那两条断言之后的 registry 遍历,仍然会去点击已经退役的 待办 条目,所以这条 journey 需要更新到当前的 registry,而不是更新到当前的字符串。注释现在把这一点直说了——registry 顺序,减去这个 shell 已退役的 Task face——这比一次静默的列表改动对下一个读者友好得多。
先前的 Knip 失败也已解决:未被使用的单数 closeSessionWorkbarTab 导出已移除,保留下来的复数形式有调用方。
其余部分,先前已确认且未变
删除面是干净的——SessionTodoPanel、useSessionTodo、WorkbarTodoService、desktop adapter 与 Task registry 条目都没有悬空的生产消费者,dnd-kit 也没有剩余的 importer。
SessionTodo 的持久数据一点没动。 workflow_session_todo_documents 既不迁移也不删除;SQLite store、revision/copy/purge、todo_read / todo_write、Host 查询与 preload IPC 全部保留。旧的 todo 仍在磁盘上,agent 仍然可读可写——被退役的是 Desktop 的渲染面。 这是一个产品决定而非缺陷,值得有意识地做出,但没有任何东西丢失。
简化是真实的:dnd-kit 重排、per-tab 关闭、pin-preview 与右键菜单的 close-others 都没了,reducer 也删掉了 reorder / move / pin / preview。复杂度是被移除,而不是被挪进 TabList——后者是表现层。
一条不阻塞的遗留说明:标题与 README 描述的是一个 open/close 控制,而生产中仍有若干写者——[+] 菜单、空 strip 的 launcher、键盘快捷键、commands.openTool、Files Esc 路径,以及 strip/标题栏的列开关。其中 Files Esc 那条值得再看一眼,因为它折叠的是整列,而不是关闭 files face——与其余五条不是同一类动作。onOpenLauncher 也仍然从 host 接通,却从未被调用。
这是一次 UI 重构,合并与否仍由人决定。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
The right workbar drew its own tab strip: a hand-written
role="tablist"div of ghostButtons with a closeIconButtonbeside each, dnd-kit drag reorder, a context menu of move/close verbs, and a preview/pin state no renderer code ever set. It read as a browser tab bar without being one, and its container had nomin-width: 0, so at the panel's floor the tabs spilled past the edge and pushed[+]and the collapse toggle off screen instead of scrolling. This replaces it with@astryxdesign/core/TabListand deletes what the re-implementation carried.Three consequences are not obvious, so they are the reason the shape changed rather than just the markup:
TabrendersendContentinside its own<button>, so a per-tab close would nest a button in a button, andTabListwarns when arole="tablist"strip's direct children are not tabs. Opening and closing therefore share the[+]menu, which lists every registered tool and marks the open ones with a checkmark — the row-end markSelectordocuments as "the house convention shared with Typeahead and CommandPalette", not a boxed checkbox. It carries no shortcut column, because the launcher below already lists every face with its own and that is where a shortcut is learned.TabList hasDividerdraws it on the strip's own box, which ends where the tabs do, so it ran out from under[+]and the collapse toggle and read as a line beneath half a row.Tabdocuments a second idiom for exactly this — a toolbar with a bottom divider, handing the tabs--_tab-indicator-bottomso the selected indicator drops through the bar's gap and lands on the rail — and that is what the bar does now.Replacing the strip also closes two of the three remaining defects #2188 measured on the old one, which the re-implementation caused and
TabListdoes not:.maka-workbar-tab:hoverand[data-active]shared one rule, so the selected tab's marker was--state-hover-bg, with a second marker (a 2px underline) on top of it. Selection is now weight 600 plusTabList's own rail, and hover is a background — one meaning, one marker, and hovering an unselected tab can no longer make it look selected.[+]28, collapse 32.WorkbarTogglewas hard-wired tomd, which is the titlebar rail's size, correct where it stands beside the sidebar and search actions. It takes asizenow, and both placements of that one control passsm: the workbar's own bar, and the titlebar affordance that stands in for it while the panel is collapsed. They are the same control in two bands, sosession-workbar.spec.tsholds them to the same x and y within 1px, and a size that differed by placement would break that by 2px.The third defect #2188 lists — the bar's asymmetric padding — is not fixed here. Narrowing the right pad from the window titlebar's
--space-6to the bar's own--space-2slides the collapse toggle 16px on every collapse, which the alignment contract above forbids. Squaring the two needs the titlebar strip's gutter to move as well, and that is a shell-wide change, not this one.Four smaller changes ride along because leaving any of them would need a follow-up PR to explain:
SessionTodoPanel, its stylesheet, its shared copy, andWorkbarTodoService, which had no caller left once the hook went. Thetodo:readIPC channel, its preload method andquerySessionTododeliberately stay: that is the boundary Work Board will read through, and it carries its own projection tests.git-branch, which reads as "switch branch". The face lists changed files and their diffs, so it carriesfile-diff.WORKBAR_TOOL_DEFINITIONSbecomes the only place a face's icon, shortcut and order are written. The strip, the[+]menu and the launcher all read it; the surface previously kept two more hand-maintained lists beside it.accessibility-coverage.spec.ts, the collapse contract insession-workbar.spec.ts, and the settings overlay insettings.spec.tsall asserted the retired Task face, the per-tab close, or[+]'s old "swap the panel for the launcher" behaviour. test(desktop): reduce P1 Electron coverage at owning boundaries #4803 proposes deletingaccessibility-coverage.spec.tsoutright as part of tracking(desktop): move every test out of Electron E2E that does not need it #4761; that call belongs to that PR, and leaving a broken assertion here for it to inherit would make this PR depend on it landing.Refs #2188
Verification
npm run typecheck,npm run lintandnpm testpass. CI's Electron E2E run is green: 77 passed, 1 skipped, 0 failed.workbar-model.test.tsgains one case for the migration this creates: an install that had the Task face open still carriesworkbar:tasksin v3 storage, and the retired kind must be dropped rather than restored as a tab with no panel behind it. It fails on the parent commit, wheretasksis still a valid kind and the tab is restored.Measured in the built Storybook against
product-session-workbar, since every defect here is a layout one. Each number replaces one #2188 measured on the old strip.The bar's height no longer depends on what is in it — this is the jump the strip used to cause:
[+]Control heights in one row: tab 28 /
[+]28 / collapse 28 (was 28 / 28 / 32).The rail, both themes: it spans x 800–1280, the panel's full width, with
[+]at 1192 and the collapse toggle at 1228 standing over it — the old one stopped where the tabs ended. The selected tab's indicator spans y 30–32 and the rail y 31–32, so the indicator's bottom edge lands on the rail rather than 1px past it. TheTabListis 28px tall withborder-bottom: 0px, so it matches the two buttons beside it instead of overflowing the 32px bar by a pixel.Selected against hovered, both themes, on the new
SeveralFacesstory: the selected tab is weight 600 with no fill; the two unselected are weight 400, and hovering one draws a background it does not share with the selected tab. The old rule gave both the same--state-hover-bg.Overflow at the floor, on
SeveralFacesAtColumnFloor: at the 320px floor the three tabs need 284px and the strip has 220px, soTabListscrolls inside itself and[+]stays on screen. This is the 264px-hole defect's real cause — the strip's container had nomin-width: 0, so it spilled instead. At the 480 default the same three tabs fit and nothing scrolls.The DOM carries no per-tab close buttons, and the stories log no console errors.
SeveralFacesandSeveralFacesAtColumnFloorare new: the story group had no multi-tab scenario at all, so neither the selected/hover distinction nor the overflow behaviour had anywhere to be seen. They open 变更 + 浏览器 + 生成文件 rather than Work Board, because this group's bridge stubs noworkBoardservice and that panel subscribes to it on mount.Screenshots
Each image is one page, BEFORE (
main) on the left and AFTER on the right, shot from two Storybook builds of the same stories at 1280×760, 2× DPR, cropped to the workbar column. To make the left half honest rather than a harness artifact, the parent commit's story wrapper was given the two things the AFTER stories add and nothing else: the--maka-plate-titlebar-clearancethe real AppShell declares (without it the bar floats mid-panel in both builds), and thealsoOpenhelper, since the group had no multi-tab story to compare against.The strip, three faces open — pills with a per-tab
×becomeTabListwith its rail, and the rail runs the whole bar rather than stopping where the tabs do. The bar is 32px in both, so nothing below it moves.The same strip at the panel's 320px floor — before, the third tab is clipped mid-glyph with nothing to say it continues; after, the strip scrolls inside itself and shows the chevron.
What
[+]puts on screen. Before, it swapped the whole panel for the launcher list, so the face you were reading disappeared to open another; after, it opens a menu over the panel, with a row-end checkmark on the faces already open. (The left half is the launcher itself, becauseonOpenLauncheris a no-op in the story harness and cannot be clicked.)The launcher — 待办 is gone and 变更 carries
file-diffinstead ofgit-branch.The remaining faces, unchanged below the bar (变更 / 生成文件 / 追踪 / 侧边对话)
Review focus
[+]is now the only way to close a face. If that reads as a regression against per-tab close, the constraint above is the reason, and the alternative would be droppingrole="tablist"— which is what this issue set out to fix.The one defect #2188 lists that this PR leaves open is the bar's asymmetric right pad, for the reason given under Summary. Fixing it means moving the window titlebar strip's gutter too, so that the one collapse toggle keeps its position across both bands.
AI use
Tool(s) and scope: Claude Code — drafted the implementation and the Storybook measurements under my direction and review; the design was settled by comparing rendered alternatives, not generated.
Checklist
Does this PR entail a change in behavior?