fix(desktop): give the titlebar one control height and one name authority - #5761
Conversation
ba82ee0 to
27691b8
Compare
jackwener
left a comment
There was a problem hiding this comment.
[kabi-opus-dev] Review of removal completeness and the architecture gate. Automated review (Claude). The parent-session entry point and focus rings are Haoqing Sol6_2's part; I make no claim on them. Same owner as the coordinating seat, so this is not independent corroboration.
Bound to 27691b884c6390768a6772f24bd6559e821a9187; head re-checked before publishing. Merge-base fdaa85806.
Conclusion: no P0–P3. The removal is complete, and the app-shell.tsx ledger change is real.
① Removal completeness
Repo-wide git grep at head, with base counts as the positive control:
| symbol | base | head |
|---|---|---|
branchBanner / onBranchBannerClick |
12 / 4 | 0 / 0 |
BranchBanner / BranchBannerSessionInput / SessionContextBranch |
24 / 3 / 2 | 1† / 0 / 0 |
deriveBranchBanner / branch-banner |
8 / 4 | 1† / 2† |
fromAbortedTurn / onBranchNavigate |
9 / 4 | 0 / 0 |
copy branchBeforeInterrupt / sessionLineageAriaLabel (3 locales) |
6 / 5 | 0 / 0 |
CSS maka-session-context__lineage / __breadcrumb* / interrupt-origin |
2 / 7 / 1 | 0 / 0 / 0 |
† All remaining hits are in docs/archive/desktop-smoke-plan-legacy.md, a historical record that is correct to keep.
fromAbortedTurnremoval loses no reachable behaviour. At base, the only production caller,use-session-navigation-reads.ts:47, calledderiveBranchBannerwith two arguments, and so did the story. The flag was never set, so the "从中断前分支" token could not render in the app.- Knip (
npx knip --workspace apps/desktopand--workspace packages/ui, the same invocations CI uses): both exit 0 with no findings. Positive control: an unused export planted insession-context-layer.tsxwas reported (exit 1), then reverted. - Types:
@maka/uitypecheck exit 0. For desktop I rantsconfig.main,.rendererand.storybookindividually, and all three exit 0 with 0 errors. The npmtypecheckscript chains them with&&, and my environment has 7 pre-existing errors in the untouchedsrc/preload/runtime-host-session-catalog.ts, so running the chain would have hidden the other three. So no caller still passes the removedChatView/SessionContextLayerprops (branchBanner,onBranchBannerClick,branch,onBranchNavigate,sessionName), and the stories compile. openSessionInChatstays live: it still has other uses inapp-shell.tsx, for example the titlebar parentonOpenat :1228.- Biome lint on the 12 changed TS/TSX files is clean.
② Architecture gate, run the way CI runs it
check-renderer-architecture.mjs --base fdaa85806… --strict-base: passed (fixtures 112/112). Also against current main2a568ef97(3 commits ahead of the merge-base): passed.- The ledger number is real, not just accepted by the gate. I regenerated the ledger at head with
--write, and it is byte-identical to the committedrenderer-architecture.json. I then restored the file, and the tree is clean. The −12 onapp-shell.tsx(nonTriviaTokens12385 → 12373) matches the three removed lines exactly:branchBanner,(2 tokens),branchBanner={branchBanner}(5 tokens) andonBranchBannerClick={openSessionInChat}(5 tokens). git merge-tree --write-tree HEAD origin/mainexits 0.
Tests run
session-context-layer-goal + titlebar-session-identity (6/6), session-navigation-controller (5/5).
Not verified
I did not check the titlebar size change, the removed titlebar focus-ring and rename-input CSS, the parent-entry behaviour, or any rendering or visual effect. Those are Sol6_2's part. I did not run the app, E2E, Storybook smoke or the full suite.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the 17-file change at 27691b884c6390768a6772f24bd6559e821a9187. I found one P2 usability regression, described inline. Ordinary branch navigation still reaches its parent through parentSessionId and the titlebar arrow, but the parent title is no longer visible when the rail is collapsed.
I checked the renderer parent selector and titlebar wiring, the context-layer deletion, titlebar geometry and keyboard focus in Chromium Storybook at 1280, 720, and 420 px, and the untitled-parent copy for en, zh-CN, and zh-TW. The 28 px titlebar buttons start at y=6; Astryx's 2 px outline with a 3 px offset starts at y=1 and was not clipped in those browser checks. The localized parent labels resolve to New task, 新建任务, and 建立任務. npm ci, build:test, 11 focused tests, Desktop story typecheck, Storybook build, git diff --check, and a merge-tree against current main passed; the current-head hosted test check is green.
A linked subagent child uses subagentParent.parentSessionId, whereas the production titlebar selector reads only parentSessionId (use-session-navigation-reads.ts:47-56), so it still has no titlebar parent arrow. The removed breadcrumb used that same ordinary field, so I did not attribute this pre-existing gap to this PR; the subagent story injects a parent directly and does not verify the production selector. I did not test packaged Electron, native window controls, or screen readers.
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.
…rity The titlebar mixed two control heights: the sidebar search and collapse buttons were Astryx md (32px, flush with the 32px strip) while the session identity and the Workbar restore were sm (28px). All titlebar controls now use sm, so they share one centre line and one box. At 28px the default Astryx focus outline (2px + 3px offset) clears the top of the window, so the product's inset box-shadow focus ring for titlebar buttons is gone. The rename field also stops stripping the Astryx TextInput and drawing its own underline shadow; only the content-sized width stays. SessionContextLayer still repeated the session name (and the parent) in a branch breadcrumb, a second presentation of what TitlebarSessionIdentity already shows with its parent-return button. The breadcrumb, its BranchBanner derivation and the fromAbortedTurn token that production never supplied are removed; the parent name in the titlebar now goes through presentSessionName like the session name does. Generated-by: Devin
…e titlebar A linked subagent child records its parent only in subagentParent; the Host never sets parentSessionId on it. Since apache#5532 the titlebar read its parent from a selector that looked at parentSessionId alone, so a subagent child opened from the stream had no return arrow. Before that, the titlebar took the parent from the rail's revision-aware linked tree, which is still the projection the rail uses to highlight the ancestor row. The selector now asks that projection first and falls back to the ordinary parentSessionId, which branches and side conversations still use for their return arrow. The test fixture's child drops the parentSessionId it never has in production, which is what hid the gap. Generated-by: Devin
27691b8 to
7b88874
Compare
|
The linked-subagent parent gap from the review summary is fixed in 7b88874. Since #5532 the titlebar parent came from a selector that read only |
jackwener
left a comment
There was a problem hiding this comment.
Approving at 7b8887475ad6cad52f25858e29f61c1e3f34450a. No open P0–P2.
- The earlier P2 (parent name only in the back-arrow tooltip once the context-band breadcrumb is gone) is accepted as design by the author and maintainer, per the #4985 titlebar decision recorded above; not re-litigated here.
7b8887475: the titlebar parent now comes from the rail's revision-aware linked tree first, thenparentSessionId. Probed on the current build: a linked subagent child returns to the family representative (root-v2), branch / side conversation still return to the physical parent, and a parentless revision gets no arrow. Related tests 18/18; the fixture no longer carries the production-impossibleparentSessionId, so the reads test fails on the old selector.- Earlier round: removal of the breadcrumb/BranchBanner/
fromAbortedTurnsurface is clean repo-wide;--strict-basearchitecture gate passes and the ledger regenerates byte-identical; focus ring unclipped at 1280/720/420; untitled parent label correct in en / zh-CN / zh-TW. - CI
testgreen; merges cleanly with current main.
Not run: packaged Electron, native window, screen reader.
Reviews: #5761 (review) , #5761 (review)
Summary
Closes the Titlebar and Session rail items of #4679. On
mainI checked each item and changed only the ones that were still open.Titlebar geometry. The strip is 32px (
--h-titlebar36px minus the 4px resize edge), which already sits on the 4px grid. Its controls did not share a height, though: the sidebar search and collapse buttons were Astryxmd(32px, the full strip height) while the session identity buttons and the Workbar restore button weresm(28px). All titlebar controls now usesm, so they have one box size and one centre line (y 6–34 in every story I measured).No product shadow. Titlebar controls paint no shadow at rest. The two product-drawn shadows were an inset
box-shadowfocus ring that replaced the Astryx outline, and the rename field's0 1px 0underline. Both are gone:TextInputchrome (border, background, font, padding). It now renders as the standardsmfield with Astryx's own focus treatment. I keptfield-sizing: contentso the field still hugs the name.One title authority. Two components rendered the session name:
TitlebarSessionIdentityand a branch breadcrumb inSessionContextLayer. The breadcrumb showed "parent › current name" whenever there was no Goal, and repeated both the name and the parent-return link that the titlebar identity already provides. It is removed, along with the code that only existed to feed it:BranchBannerderivation, which recomputed the same parent the titlebar already reads throughactiveParentSessionChatView/SessionContextLayerprops and copyfromAbortedTurntoken, which production never suppliedThis follows the direction in
docs/frontend-architecture-astryx-review-2026-08-09.md: the titlebar owns the name, and the context band owns runtime chips only. The titlebar also rendered the parent name raw, so an untitled parent read "New Chat" in every locale. It now goes throughpresentSessionName, like the session name.Subagent children get their titlebar parent back (raised in review). A linked subagent child records its parent only in
subagentParent; the Host never setsparentSessionIdon it (session-manager.ts). Since #5532 the titlebar took its parent from a selector that readparentSessionIdalone, so a subagent child opened from the stream had no return arrow. Before #5532 it came fromderiveSessionRail(...).activeParentSession, the revision-aware linked tree the rail still uses to highlight the ancestor row. The selector now asks that projection first and falls back toparentSessionIdfor branches and side conversations. The test fixture's child had aparentSessionIdit never has in production, which is what hid the gap; it now matches production, and the reads test fails on the old selector (actual: undefined).Right-side actions. These were already Astryx
IconButton+Tooltip. I did not wrap them in AstryxToolbar:Toolbaris aSectionwithspacing-2block padding, which makes it a 44px bar. That cannot sit inside the 32px drag strip without overriding Astryx internals, and its no-drag rect would reach down over the top of the content plate.Already satisfied on
main, not changed:titlebar-dim-color.ts/titlebar-modal-sync.ts(DESIGN.md §8):theme.tsalready samplesgetComputedStyle(html).backgroundColorand the open dialog's computed::backdrop. Neither reads a token string, andtitlebar-dim-color.tsis pure colour math.Badgeinpackages/ui/src/session-history-list.tsx: none of them is decorative, so they stay.metais the Runtime Host a remote-workspace session runs on (e.g. "Remote Mac").executorIdnames an external ACP executor (e.g.antigravity) and is also exposed through the row's hover-card description. Neither is a status, soStatusDotwould misstate them. The warningBadgerendered throughsessionBadge(SessionTurnRequestBadge) is a real count of pending guest turn requests.Refs #4679
Verification
Titlebar geometry, measured by attaching to the Storybook preview over CDP (1280×900, before on
main, after on this branch):maindefault-layoutworkbar-titlebar-restorefocus({focusVisible:true}))outline: none, inset box-shadowoutline: 2px solid, offset 3px, ring top y=1TextInput, 1px border, no product shadowConsole errors were the same on both builds (two static-server 404s).
Checks:
@maka/ui: all 681 tests pass.session-navigation-controller,session-navigation-boundary,titlebar-modal-dim,ink-ladder-contract,quote-companion-retry,new-task-staged-content,traditional-chinese-peer-mesh-copyandsession-inspector-compositionpass.npm --workspace @maka/desktop run typecheck(includes stories),npm run format,npm run lint,npx knipforapps/desktopandpackages/ui,check:renderer-architecture --base <merge-base>(ledger token count lowered), andastryx:surface-inventory:writeall pass.Storybook play functions on the built catalog in light and dark:
default-layout,update-downloaded-collapsed,titlebar-project-feedback-narrow,titlebar-parent-return,titlebar-identity-truncated,session-context-layer,titlebar-with-wide-workbar,workbar-titlebar-restore,narrow-workbar-clears-titlebar-reserve(at 720px, its intended viewport) andsubagent-sessions--child-sessionall pass. My local runner sometimes missedstoryFinishedon both builds; each story was re-run until it reported a result. The full smoke and E2E suites are left to CI.New checks:
default-layoutplay now asserts that all titlebar controls have one height. Onmainit would see {32, 28} and fail.ComposedShellstory now passes the titlebar parent the way production does. Before, lineage stories showed no parent at all.Not covered:
ChatViewno longer accepts the prop.mainand unchanged here: renaming a very long title grows the field past the window's left edge, because the Astryx field wrappers keepmin-width: auto.AI use
Tool(s) and scope: Devin (Claude) audited the #4679 items against
main, made the changes and tests, ran the CDP measurements and screenshots, and drafted this description.Checklist
Does this PR entail a change in behavior?