Skip to content

refactor(desktop): derive the tab hover close button from the close verb - #91777

Merged
ethernet8023 merged 1 commit into
mainfrom
ethie/desktop-close
Aug 21, 2026
Merged

refactor(desktop): derive the tab hover close button from the close verb#91777
ethernet8023 merged 1 commit into
mainfrom
ethie/desktop-close

Conversation

@ethernet8023

@ethernet8023 ethernet8023 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

The hover close button on a pane tab was wired separately from the close
gestures that do the same job. PaneTab gated the button on two independent
inputs — the onClose verb, and a showCloseButton prop that TreeGroup fed
from a showCloseButton flag on the pane contribution — while middle-click and
⌘-click read only onClose. A tab could therefore close on a pointer gesture
and show no control for it.

The flag had no user that hideOnly did not already cover. Both setters also
set hideOnly: true, which removes every close gesture:

  • the sessions pane (apps/desktop/src/app/contrib/controller.tsx),
  • the Bots pane (apps/desktop/src/plugins/hermes-bots/plugin.js).

The history explains why. #89551 added showCloseButton as an ✕-suppression
fix. #89572 cherry-picked that commit as its base, then built hideOnly on top
and superseded it. The flag stayed behind as a vestigial rung, not as live
design, so this PR deletes it instead of teaching it to track the gestures.

onClose alone now decides both shapes. A tab that closes shows the button. A
tab without the verb shows nothing. To make a tab uncloseable, give it no close
verb. hideOnly and uncloseable keep their meaning: they gate the verb, and
both shapes follow the verb together.

Related Issue

No issue. This is follow-up cleanup on #89572, which introduced hideOnly and
left the superseded flag in the tree.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/components/ui/pane-tab.tsx: delete the showCloseButton
    prop. The render condition becomes {onClose && !vertical && …}. The
    onClose doc comment states that the chip and the pointer gestures are one
    affordance.
  • apps/desktop/src/components/pane-shell/tree/renderer/track-model.ts: delete
    showCloseButton from PaneChrome. The hideOnly comment now says that it
    removes the close verb, and that the ✕ follows the verb.
  • apps/desktop/src/components/pane-shell/tree/renderer/tree-group.tsx: delete
    the showCloseButton={chrome.showCloseButton !== false} callsite.
  • apps/desktop/src/app/contrib/controller.tsx: drop the flag from the
    sessions pane data. Its hideOnly: true already removes every gesture.
  • apps/desktop/src/plugins/hermes-bots/plugin.js: the same for the Bots pane.
  • apps/desktop/src/components/pane-shell/tree/renderer/tab-close-affordance.test.tsx:
    new contract test (see below). The DialogContent / SheetContent prop of
    the same name is untouched: it is a different prop, it has no close verb to
    derive from, and updates-overlay.tsx changes it while the dialog is open.
  • apps/desktop/src/components/ui/pane-tab.test.tsx: move the "can hide the
    hover ✕" case off the deleted prop. It now asserts that a closeable tab shows
    the button and honors middle-click.

How to Test

Behavior is identical before and after, so the proof is the mutation test:

  1. cd apps/desktop && npx vitest run src/components/pane-shell src/components/ui/pane-tab.test.tsx
    — 154 tests pass.
  2. Break the derivation by hand: change the PaneTab render condition to
    {false && onClose && !vertical && (, which hides the button on tabs that
    still close on middle-click.
  3. npx vitest run src/components/pane-shell/tree/renderer/tab-close-affordance.test.tsx
    — the files and session-tile:abc cases fail. The test detects a button
    that stops tracking the verb.
  4. Revert step 2. All four cases pass again.

In the app: hover the SESSIONS and BOTS tabs. Neither shows an ✕, and neither
closes on middle-click or ⌘-click, the same as before this PR. Hover a files
or session tile tab. Each shows an ✕ and closes on middle-click, the same as
before this PR.

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
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — N/A for the Python suite (no Python file changes). The JS suite is the gate here: the full apps/desktop vitest run gives 6954 pass, 2 fail. The 2 failures are electron/remote-lifecycle.test.ts and electron/update-handoff-marker.test.ts. They fail the same way on a clean tree on this machine (nix sandbox spawn paths), and they do not touch the pane shell.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: NixOS (Linux 7.1.8)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — the
    PaneTab.onClose and PaneChrome.hideOnly doc comments now carry the
    rule. No user-facing doc mentions the flag.
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — renderer-only change with no platform-conditional code. ⌘-click and middle-click already route through lib/middle-click.ts for every host.
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Tests

tab-close-affordance.test.tsx renders the real TreeGroup and asserts one
contract over every tab kind that shares a strip:

expect(hasCloseButton(paneId)).toBe(middleClickCloses(paneId))

It reads closure from the layout tree (the pane is gone), not from a spy, so a
wired-up mock cannot pass it. Cases: hideOnly chrome, a plain side pane, the
uncloseable workspace, and a mirrored session tile.

The test carries no fixture for the deleted prop, because the compiler already
rejects it: passing showCloseButton to PaneTab now fails with TS2322: Property 'showCloseButton' does not exist. A test tombstone would only restate
that, under a name the codebase still uses for the unrelated DialogContent
prop. The remaining cases still catch the regression they exist for: hiding the
button on a closeable tab fails the files and session-tile cases.

Screenshots / Logs

$ npx vitest run src/components/pane-shell src/components/ui/pane-tab.test.tsx
 Test Files  29 passed (29)
      Tests  154 passed (154)

$ npm run typecheck
tsc -p . --noEmit && tsc -p tsconfig.electron.json --noEmit && tsc -p tsconfig.e2e.json --noEmit
(clean)

$ npm run lint
✖ 116 problems (0 errors, 116 warnings)

The single warning on the new file is no-restricted-globals for document.
Every sibling DOM probe in that directory carries the same warning.

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

ran on 6e20855 — refactor(desktop): derive the tab hover close button from th

⚠️ Warnings

OSV vulnerability scan · View job

7 known vulnerabilities found in pinned dependencies.

How to fix:

Review the findings in the Security tab. Update the affected dependencies if a patched version is available.


debug info

CI timings

CI timings · View report · View job

Wall time 4m20s vs 6m42s (-35.3%). 16 job(s) slower, 3 faster,

  • JS & TS checks / apps/desktop / check:test:ui:shard-3of3: -54.0s
  • JS & TS checks / apps/desktop / check:test:ui:shard-1of3: +41.0s
  • JS & TS checks / apps/desktop / check:lint: +22.0s
  • JS & TS checks / apps/desktop / check:test:ui:shard-2of3: +19.0s
  • JS & TS checks / apps/desktop / check:test:desktop:all: +15.0s

PaneTab gated its hover close button on two independent inputs: the
onClose verb, and a showCloseButton prop that TreeGroup fed from a
showCloseButton flag on the pane contribution. The middle-click and
Meta-click gestures read only onClose. A tab could therefore close on a
pointer gesture and advertise no control for it.

The flag had no user that hideOnly did not already cover. Both setters
also set hideOnly: true, which removes every close gesture:

- the sessions pane (app/contrib/controller.tsx),
- the Bots pane (plugins/hermes-bots/plugin.js).

The flag was an opt-out marker with no reachable effect, so this change
deletes it instead of teaching it to track the gestures. onClose alone
now decides both shapes. A tab that closes shows the button. A tab
without the verb shows nothing. To make a tab uncloseable, give it no
close verb.

hideOnly and uncloseable keep their meaning. They gate the verb, and
both shapes follow the verb together.

The DialogContent and SheetContent prop of the same name is a different
prop and stays. It has no close verb to derive from, and one caller
changes it while the dialog is open.

Tests: the new tab-close-affordance test renders the real TreeGroup and
asserts that button presence equals middle-click closure. It covers
hideOnly chrome, a plain side pane, the uncloseable workspace, and a
session tile. It reads closure from the layout tree, not from a spy, so
a wired-up mock cannot pass it. A regression that hides the button on a
closeable tab fails two of the four cases. The compiler rejects the
deleted prop, so the test carries no fixture for it. The pane-tab unit
test moves off the deleted prop.

Verified with the full apps/desktop vitest suite, npm run typecheck, and
npm run lint. Two electron process-spawn tests fail on this machine.
They also fail on a clean tree, and they do not touch the pane shell.
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have labels Aug 21, 2026
@ethernet8023
ethernet8023 merged commit 9815319 into main Aug 21, 2026
42 checks passed
@ethernet8023
ethernet8023 deleted the ethie/desktop-close branch August 21, 2026 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants