Skip to content

fix(desktop): rebuild the workbar shell on TabList and one open/close control - #4789

Merged
Astro-Han merged 1 commit into
mainfrom
fix/desktop-workbar-tab-strip-on-tablist
Sep 5, 2026
Merged

fix(desktop): rebuild the workbar shell on TabList and one open/close control#4789
Astro-Han merged 1 commit into
mainfrom
fix/desktop-workbar-tab-strip-on-tablist

Conversation

@Astro-Han

@Astro-HanAstro-Han commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

The right workbar drew its own tab strip: a hand-written role="tablist" div of ghost Buttons 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 its 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. This replaces it with @astryxdesign/core/TabList and deletes what the re-implementation carried.

Three consequences are not obvious, so they are the reason the shape changed rather than just the markup:

  • Tabs have no close control.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 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.
  • Tabs no longer reorder. The strip's order is the order the faces were opened in, and dnd-kit leaves the dependency tree with it.
  • The strip does not set the bar's height, and does not own the rail. The height comes from the titlebar clearance the bar stands in. The rail comes from the bar too: 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 documents 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.

Replacing the strip also closes two of the three remaining defects #2188 measured on the old one, which the re-implementation caused and TabList does not:

  • Selected and hovered read the same..maka-workbar-tab:hover and [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 plus TabList's own rail, and hover is a background — one meaning, one marker, and hovering an unselected tab can no longer make it look selected.
  • Adjacent controls were different heights — tab 28, [+] 28, collapse 32. WorkbarToggle was hard-wired to md, which is the titlebar rail's size, correct where it stands beside the sidebar and search actions. It takes a size now, and both placements of that one control pass sm: 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, so session-workbar.spec.ts holds 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-6 to the bar's own --space-2 slides 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:

  • 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. The todo:read IPC channel, its preload method and querySessionTodo deliberately stay: that is the boundary Work Board will read through, and it carries its own projection tests.
  • Review carried git-branch, which reads as "switch branch". The face lists changed files and their diffs, so it carries file-diff.
  • WORKBAR_TOOL_DEFINITIONS becomes 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.
  • Three E2E journeys are updated to the new shell, not deleted: the workbar walk in accessibility-coverage.spec.ts, the collapse contract in session-workbar.spec.ts, and the settings overlay in settings.spec.ts all asserted the retired Task face, the per-tab close, or [+]'s old "swap the panel for the launcher" behaviour. test(desktop): move P1 journeys off Electron #4803 proposes deleting accessibility-coverage.spec.ts outright 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 lint and npm test pass. CI's Electron E2E run is green: 77 passed, 1 skipped, 0 failed.

workbar-model.test.ts gains one case for the migration this creates: an install that had the Task face open still carries workbar:tasks in 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, where tasks is 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:

bar heightstrip height[+]collapse
no face open3232y=2, h=2828
one face open3232y=2, h=2828
three faces3232y=2, h=2828
three faces at the 320 floor3232y=2, h=2828

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. The TabList is 28px tall with border-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 SeveralFaces story: 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, so TabList scrolls inside itself and [+] stays on screen. This is the 264px-hole defect's real cause — the strip's container had no min-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.

SeveralFaces and SeveralFacesAtColumnFloor are 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 no workBoard service 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-clearance the real AppShell declares (without it the bar floats mid-panel in both builds), and the alsoOpen helper, since the group had no multi-tab story to compare against.

The strip, three faces open — pills with a per-tab × become TabList with 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.

lightdark

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.

lightdark

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, because onOpenLauncher is a no-op in the story harness and cannot be clicked.)

lightdark

The launcher — 待办 is gone and 变更 carries file-diff instead of git-branch.

lightdark
The remaining faces, unchanged below the bar (变更 / 生成文件 / 追踪 / 侧边对话)
lightdark
变更
生成文件
追踪
侧边对话

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 dropping role="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

  • Generative tooling made a substantive contribution

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

  • 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-actionsgithub-actionsBot added the effort/XL Under 2500 readable lines label Sep 4, 2026
@Astro-Han
Astro-Hanforce-pushed the fix/desktop-workbar-tab-strip-on-tablist branch 2 times, most recently from e4f7996 to 6885bdbCompareSeptember 4, 2026 15:08
@Astro-Han
Astro-Hanforce-pushed the fix/desktop-workbar-tab-strip-on-tablist branch 6 times, most recently from 68881d6 to 69ccc9cCompareSeptember 5, 2026 03:04
… 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
@Astro-Han
Astro-Hanforce-pushed the fix/desktop-workbar-tab-strip-on-tablist branch from 69ccc9c to 56d2c71CompareSeptember 5, 2026 03:19

@jackwenerjackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  • :151 no 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;
  • :306 no 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 导出已移除,保留下来的复数形式有调用方。

其余部分,先前已确认且未变

删除面是干净的——SessionTodoPaneluseSessionTodoWorkbarTodoService、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.

@Astro-Han
Astro-Han merged commit cbeb1a9 into mainSep 5, 2026
18 checks passed
@Astro-Han
Astro-Han deleted the fix/desktop-workbar-tab-strip-on-tablist branch September 5, 2026 05:10
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XLUnder 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Astro-Han@jackwener