fix(desktop): give a header-hidden zone a way back - #84458
Conversation
fix(desktop): give a header-hidden zone a way back
|
Enough1122
left a comment
There was a problem hiding this comment.
One real edge-case regression plus an accessibility gap on the new reveal strip (see inline).
| // the flag was added: double-click reveals the header, right-click opens the | ||
| // zone menu. A flex CHILD, not an overlay — 6px the content is laid out | ||
| // below, rather than 6px of the content's own top row swallowed. | ||
| const revealEdge = showRevealEdge({ headerHidden: node.headerHidden, isEmpty, minimized: node.minimized }) |
There was a problem hiding this comment.
The strip keys only on the EXPLICIT node.headerHidden === true, but the header itself is also suppressed by the contextual paneChrome(active).headerVeto (line 255) — which applies to the workspace pane when it shows a full page (skills/messaging/artifacts). Sequence: hide the workspace header via the zone menu → navigate to a full-page route (headerVeto active) → the strip still shows → double-click it → setTreeGroupHeaderHidden(node.id, false) clears the explicit flag → revealEdge becomes false (strip disappears) while headerHidden is STILL true via the veto → the zone loses both the header and the only affordance to restore it (the strip's right-click zone menu), and back on chat a lone uncloseable workspace pane stays header-hidden by the contextual default. Before this PR the zone was stuck with no affordance either, but the strip now actively removes the one path back after a double-click. Suggest gating the strip on the EFFECTIVE header-hidden state minus the veto, or clearing the veto (leaving the page) before revealing; also, the button has no onClick — only onKeyDown — so assistive-tech activation that dispatches a click gets no response.
Hiding a zone's header takes its tab strip with it, and the strip is the only host of the zone menu -- so an explicitly hidden zone has no tab, no close button and no menu. A zone of closeable tiles (a preview, the Browser) is then stranded on screen: only Cmd/Ctrl+W (hover it first) or a layout reset gets rid of it, and neither is discoverable from a zone showing no chrome at all. model.ts has documented the way out since the flag was added -- "double- click the zone's top edge to bring it back" -- but the edge was never implemented. Implement it: double-click (or Enter/Space) reveals the header, right-click opens the zone menu, so Close is reachable without the strip. The strip is a flex child rather than an overlay, so 6px of content is laid out below it instead of the content's own top row being swallowed -- a preview's SOURCE/DIFF/Edit row sits at the very top of its pane. It carries a faint persistent tint so it can be found without hovering. Only an EXPLICIT hide qualifies. A contextual hide (lone side chrome) is not a state the user chose and lifts on its own. A full-page view's headerVeto is excluded too: it suppresses the header independently, so revealing there would be a dead gesture -- clearing the flag cannot bring a vetoed header back, and it would spend the strip (and the zone menu it hosts) on that page for nothing. The veto lifts by itself and the flag is still set underneath, so the edge returns then. One handler reads `event.detail`: 0 for a keyboard or assistive-tech activation, 2 for the second click of a double-click, so a stray single click aimed at the content row below is ignored. Listening on dblclick + keydown instead would miss the synthetic click AT dispatches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
60a1056 to
2816893
Compare
|
Thanks — the veto interaction is a real bug and I've fixed it. Point by point. 1. One correction to the scenario, though: the zone does not stay header-hidden back on chat. 2. No 3. Discoverability — partly taken. The strip now carries a faint persistent tint ( 4. Target size — pushing back, with reasoning. 24px is the right default, but the only ways to get there are eating 24px of content in every header-hidden zone or overlaying the content — and overlaying is what the flex-child layout deliberately avoids, because a preview's 5. Render test — added. Verified on Windows 11: |
What does this PR do?
Hiding a zone's header is one double-tap on its tab strip (
tree-group.tsx,hideHeaderDoubleTap). Getting it back is the problem: the hide takes the strip with it, and the strip is the only host of the zone menu — so the hidden zone has no tab, no ✕, and no "Show header". A zone of closeable tiles (a preview, the Browser) is then stranded on screen.I hit this with two file previews open. Both zones had been double-tap-hidden, and there was no affordance left on either one: no tab, no ✕, and right-clicking the body reaches the pane's content, not the zone. The only escapes were ⌘W (works —
closeFocusedSessionTabkeys onisMainStripPane) and ⌃Tab (needs ≥2 tabs in the zone, so it does nothing for a lone preview). Neither is discoverable from a zone that shows no chrome at all.model.tshas documented the way out since the flag was added:The top edge was never implemented. This PR implements it.
Relationship to #81638 (open, same trap): that PR restores the workspace zone's bar from the sidebar session-row menu —
isWorkspaceTabBarHidden()is workspace-specific, and the sidebar row is the surface it hangs off. A preview or Browser zone has no sidebar row, so it stays stranded. This PR is zone-agnostic: every zone carries its own way back, on itself. They're complementary — #81638 gives the chat case a discoverable menu item, this gives every other zone any affordance at all. If maintainers would rather have one mechanism, I'm happy to adapt.Also adjacent: #79520 (session-tile self-heal) and #75848 (which established that an explicit hide persists — deliberately kept intact here).
Related Issue
No existing issue — found while using the desktop app. Happy to file one if preferred.
Type of Change
Changes Made
apps/desktop/src/components/pane-shell/tree/renderer/lone-header.ts— addshowRevealEdge()next toforceLoneHeaderForPanes(); both answer "when does a zone show chrome it would otherwise go without", so they belong together and stay unit-testable. Gated on an explicit hide (headerHidden === true): a contextual hide (lone side chrome, aheaderVetopage) is not a state the user chose and lifts on its own, so it keeps its clean edge.apps/desktop/src/components/pane-shell/tree/renderer/tree-group.tsx— render a 6px reveal strip at the top of an explicitly-hidden zone. Double-click reveals (mirroring the gesture that hid it); Enter/Space reveal from the keyboard; right-click opens the zone menu, so Close/Show header are reachable without the strip.apps/desktop/src/components/pane-shell/tree/renderer/lone-header.test.ts— 4 tests forshowRevealEdge(explicit hide, contextual hide, minimized, empty).Two things I deliberately did not do:
ZoneMenuaround the body. Wrapping the body would put the menu everywhere the comment intree-group.tsxalready claims it lives, but toggling that wrapper adds/removes aContextMenuTriggerabove the pane layers and remounts the whole zone — a chat transcript would lose its scroll position on every header toggle.No i18n additions:
t.zones.showHeaderalready exists in every locale.How to Test
Checklist
Code
I've read the Contributing Guide
My commit messages follow Conventional Commits (
fix(scope):,feat(scope):, etc.)I searched for existing PRs to make sure this isn't a duplicate — see the fix(desktop): restore hidden tab bar from sidebar row menu #81638 comparison above
My PR contains only changes related to this fix/feature (no unrelated commits)
I've run the desktop workspace's CI scripts.
check:lint: 0 errors.check:test:ui: thepane-shellscope this change lives in is fully green (18 files / 104 tests). This change is renderer-only and touches no Python, sopytest tests/is unaffected.For transparency: on my Windows box
check:test:uiandcheck:test:desktop:platformsalso report failures in unrelated files —use-prompt-actions/utilsandtool/fallback-model(number formatting under a non-English system locale), and thessh-*/update-relaunch/windows-hermes-pathelectron tests (bash -nand POSIX venv layout). I verified these are pre-existing: with my change stashed, the same tests fail identically. They should pass on CI'subuntu-latest. Flagging in case they're news to anyone — happy to open a separate issue.I've added tests for my changes
I've tested on my platform: Windows 11 (26100), built with
npm run build+electron-builder --dirand exercised in the packaged appDocumentation & Housekeeping
docs/, docstrings) — themodel.tscomment already described this behavior; the code now matches itScreenshots / Logs
Before: two preview zones (
INDEX.HTML,.ENV) with no tab strip and no way to close them.After: the reveal strip restores the header, tabs and ✕ on both.