Skip to content

feat(tabs): edit groups in the vertical rail - #525

Open
aakhter wants to merge 3 commits into
Ark0N:masterfrom
aakhter:pr/grouped-rail-edit
Open

aakhter wants to merge 3 commits into
Ark0N:masterfrom
aakhter:pr/grouped-rail-edit

Conversation

@aakhter

@aakhter aakhter commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Based on master (#517 and #519 shipped in 1.34.0).

This makes the grouped vertical rail editable from the browser. You can create, rename, reorder and delete groups, and move tabs between groups or out to Ungrouped. Edits can be made three ways:

  • the row menu;
  • a group menu (Shift+F10, ContextMenu, right-click or the header's hover glyph; F2 renames inline);
  • dragging with mouse or pen in the grouped rail.

A flat rail gets "Move to new group" in its row menu, which is how the first group is made.

Saving. Every edit is a named operation, applied to the rail at once and saved through the existing PUT /api/tab-layout:

  • One write is in flight at a time, on the version the server last returned.
  • A version conflict replays the pending operations onto the layout the server sent back and retries, so a concurrent edit from another device survives. An operation that no longer applies is dropped with a toast.
  • A reload triggered by SSE waits for an in-flight write.
  • Unsaved edits survive a page reload through a keepalive PUT plus a sessionStorage copy that replays harmlessly.

There are no server changes.

Unchanged: the flat rail's markup with no groups, the tree semantics and single roving tab stop from #519, sessionOrder and Alt+N, and the HTML5 drag in the flat rail and the header strip. Group menus close on Escape, a click outside, Tab, focus loss, a resize or any re-render. New strings have zh-CN entries, and group names only ever reach the DOM as text.

Tests

  • test/tab-layout-editing.test.ts: operations, write coordination, conflict rebase, drop mapping, menus, rename, dismissal, reload recovery.
  • test/tab-layout-editing.browser.test.ts: real pointer drags, editor paint and menu Escape. Run with npm run test:browser.
  • Checked in Chromium against an isolated instance: after every create, rename, move, drag, reorder and delete, the UI matched GET /api/tab-layout. A second page followed over SSE. A conflicting PUT from curl mid-session was rebased, and both edits survived.

For review

  • In the grouped tree, Shift+F10 on a web tab now opens a small menu ("Web tab settings" plus the moves) instead of the settings modal directly.
  • A layout re-read that fails while edits are unsaved keeps the held layout and editor, and re-reads once the write settles, so a 409 still gets its rebase. Anything that does drop pending work shows a toast.
  • Touch drag is not included; tablets use the menus.
  • test/inline-rename.test.ts has one browser failure ("Vertical rail paints typing in an unclamped editor restores clamp on cancel"). It fails the same way without this commit.

@aakhter

aakhter commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Hi @Ark0N, this is the third PR of the grouped-rail stack (on #519, which is on #517; only the last commit is new). GitHub wouldn't let me open it as a regular PR ("does not have the correct permissions to execute CreatePullRequest"), and as a draft it also refuses to let me mark it ready for review. So it's sitting as a draft only because of that, not because it's unfinished. It's ready for review whenever you get to it, and please mark it ready on your side if that's easier.

@Ark0N

Ark0N commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thanks for this, @aakhter. This makes the grouped rail editable end to end: create, rename, reorder and delete groups and move tabs, from menus, the keyboard and a pointer drag, all through the existing PUT /api/tab-layout. Saving edits as named operations, with one write in flight and a 409 replayed onto the server's layout, is the right shape. Keeping placement: 'manual' through the round trip is a subtle catch, and the real Chromium drag tests are much appreciated.

Before merge

Must fix

  1. A drag released outside the rail leaves state behind, and a later drag swallows every Escape (src/web/public/app.js:6726, 6731-6790). pointerup is only listened for on the container, and capture is only taken after 6 px of movement inside it. If you press on a row, flick out of the rail in one move and release there, _tabLayoutDrag stays set. Hovering back with no button pressed then starts a phantom drag (dimmed row, drop marks following the cursor), because pointermove never checks e.buttons. The next real drag replaces _tabLayoutDragKeydown without removing the first listener. That orphan sits in document capture and calls stopImmediatePropagation() on every Escape until reload, so Escape never reaches the terminal and Claude Code can't be interrupted from the browser. I reproduced this against a live instance: 0 of 2 Escape keydowns reached xterm's textarea. Fix: cancel any previous drag at the top of _onTabLayoutPointerDown, listen for pointerup/pointercancel on window while a press is pending, cancel in pointermove when (e.buttons & 1) === 0, and remove any existing keydown listener before adding one.
  2. Committing a group rename by clicking elsewhere pulls focus back to the header (app.js:6706, 6409-6411, 6691). During blur, document.activeElement is <body>, so editTabLayout takes the edit for one made from a menu and asks for refocus; the unchanged-name path always does. The re-render focuses the group header inside the blur handler, and Chrome drops the original focus move. Live: after clicking into the terminal to commit, focus was on the header, and the next Enter collapsed the group instead of reaching the terminal. Fix: a blur commit should not request refocus (pass a flag from the blur listener). Enter and Escape keep today's behaviour.

Should fix

  1. A failed layout read during a write drops the edit silently (app.js:6250-6252). _applyTabLayout(null) disposes the editor even while a PUT is in flight. If that PUT then gets a 409, it is never rebased or retried, there is no toast, and the rail goes flat. Reproduced: one PUT attempt, editor gone, no toast. Fix: on null, if this._tabLayoutEditor?.hasPending(), keep the held layout and editor, set _tabLayoutReloadPending and return; toast whenever pending work is dropped.
  2. Menu labels that read the same (app.js:6503, 6431). A group left with the default name shows "Move to New group" right next to "Move to new group". In zh-CN both become 移到新分组, because the catalog lookup is case-insensitive. A group named "ungrouped" collides with "Move to Ungrouped" the same way. Quoting the name in the label (plus a matching i18n pattern) fixes both.
  3. The group menu has no visible entry point on touch tablets (src/web/public/styles.css:712-724). The glyph is opacity: 0 without hover, and as far as I know iPadOS Safari doesn't turn a long press into a contextmenu event. Show the glyph under @media (hover: none).

Small ones

  1. The sessionStorage replay copy (app.js:6841-6871) isn't scoped to the owner and has no age limit. Storing { owner, baseVersion, savedAt } and ignoring a copy for another user or older than about a minute would close that off. Also, a Move to <group> with no anchor stores a concrete end index (tab-layout-browser.js:346). Omitting index would keep the row last after a rebase.
  2. A 400 that survives the re-read is reported as "Tab groups kept changing elsewhere" (tab-layout-browser.js:552-561). Something like "Could not save tab groups" would be accurate.
  3. The group menu shares the .tab-rail-action-menu class, so closeTabRailActionMenu() (tab-rail-resize.js:301, also called on every session:deleted) can remove the group menu's DOM while _tabGroupMenu and its document listeners stay attached. It heals on the next render, but scoping that selector is cheap.
  4. Stale comments: app.js:6181 ("read-only") and app.js:6885 ("Grouped editing comes with its own drag model").
  5. Cancelling "Delete group" leaves focus on <body> (app.js:6444).

Your "For review" questions

  • Shift+F10 on a web tab opening a small menu: fine with me. It matches session rows, whose Shift+F10 already opens the action menu, and "Web tab settings" as the first item keeps the modal two keys away. The flat rail is unchanged.
  • Edits discarded without a toast when a re-read fails: please don't discard them. That is item 3 above. In practice the in-flight case is the one that loses work, because a 409 never gets its rebase.
  • No touch drag: agreed, menus are the right touch path. They just need to be reachable on a tablet (item 5).
  • test/inline-rename.test.ts: confirmed this isn't caused by the PR. The same test fails the same way at the feat(tabs): full-row activation and tree semantics for the grouped rail #519 head and on current master (fix(tabs): grouped rail interaction fixes for inline rename #526 fixes it).

Rebase

#517 and #519 landed in 1.34.0 together with a fix commit, 7917273, so this now conflicts in docs/architecture-invariants.md, src/web/public/app.js and src/web/public/tab-layout-browser.js. Please rebase onto master and keep 7917273's behaviour:

  • _applyTabLayout rebuilds only when _isTabGroupStructureStale(). Put your restore block after that check, and keep _adoptEditedTabLayout rendering unconditionally.
  • The header markup keeps master's leaf handling (no aria-expanded/aria-owns on a group with no open rows; a new empty group is exactly that case). Add your oncontextmenu to it.
  • In the docs, your side carries the old text of the "Only the grouped rail is a tree" and "One tab stop" bullets. Keep master's versions, replace only the "Drag-reorder is off" bullet with yours, carry its Ctrl+Shift clamp sentence over, and add your two new bullets.
  • The fileoverview keeps master's "capped, backed-off retry" wording plus your item 4.
  • Failed reads now back off at 5/10/20/40 s and then stop, and every failure still calls _applyTabLayout(null). That is where the fix for item 3 goes.
  • Ctrl+Shift+{ / } stays clamped to the active session's own group, which agrees with your moves. Please update the _canSwapActiveTabWith comment, since "drag is off" now means the HTML5 drag. Optional: route that shortcut through moveTabRef in the grouped rail, so every grouped order edit shares your one writer.
  • Two test harnesses need one line each after the rebase. makeApp() in test/tab-layout-editing.test.ts must call _fullRenderSessionTabs() once, and the browser test's beforeEach must reset _lastTabGroupStructureKey before _applyTabLayout, because master skips a rebuild when nothing visible changed. With those, a trial merge passed 83/83 unit and 21/21 browser tests.

What I checked locally

At this head, the three tab-layout unit files (68/68) and npm run test:browser -- test/tab-layout-editing.browser.test.ts test/tab-activation.browser.test.ts (18/18) pass, as do the frontend syntax, public-asset and browser-exclusion checks. Items 1 to 3 were reproduced with Playwright on your fake-page harness, and items 1 and 2 again against an isolated Codeman instance with groups created through PUT /api/tab-layout. Item 4 was checked with i18n.js in jsdom. I also did a trial merge onto master and ran the suites there.

The grouped vertical rail can now be edited from the browser: groups are
created, renamed, reordered and deleted, and tabs are moved between them, by
menu, keyboard or pointer drag. Every edit is saved through the existing
PUT /api/tab-layout; there are no server changes.

Saving (tab-layout-browser.js, pure):
- Edits are named operations (createGroup, renameGroup, deleteGroup,
  reorderGroup, moveRef) applied to the rail at once, mirroring the server
  model: a moved session takes the sessions that still follow it, and a
  hand-moved child is marked placement 'manual'. normalizeLayout now keeps
  placement and updatedAt, since whole layouts are written back.
- createEditCoordinator keeps ONE PUT {baseVersion, layout} in flight. Edits
  made in the same turn share a write; edits made while one is in flight go
  out on the version it returns. A 409 replays the operations onto the
  layout the server returned and retries (bounded); an operation that no
  longer applies is dropped and reported. A 400 re-reads first; any other
  failure reports and re-reads.
- dropOperation maps a finished drag to one operation, or null for a drop
  that changes nothing.

Wiring (app.js, tab-rail-resize.js):
- The session row menu gains Move up/down, Move to <group>, Move to
  Ungrouped and Move to new group in the vertical rail. Before the first
  group exists it offers only "Move to new group", which is how a flat rail
  becomes grouped; the header strip's menu is unchanged.
- A group header opens its menu with Shift+F10 / ContextMenu, right-click or
  a hover glyph (a non-focusable aria-hidden span, so the treeitem still
  holds no interactive child): Rename, New group, Move group up/down,
  Delete. F2 renames inline. A web tab row's Shift+F10 opens its settings
  plus the same moves.
- The menu closes on Escape (consumed before the global Escape handler, focus
  back to its row or header), a pointer outside, Tab, focus leaving it, a
  resize, a second open and any full re-render.
- Inline group rename shares the session rename's ownership handle, so only
  the current editor releases the render guard. Enter or blur commits,
  Escape cancels, IME composition keys are left to the IME, and the label
  becomes a flex slot so the editor gets the full width while typing.
- Pointer drag (mouse and pen) in the grouped rail only: rows before/after a
  row or into a group, a header drag reorders groups. Escape cancels; the
  click that ends a drag neither selects nor toggles. The flat rail and the
  header strip keep their HTML5 drag untouched.
- A tab:layoutChanged read is deferred while a write is in flight and run
  once it settles; a read otherwise rebases unsaved edits. On pagehide,
  unconfirmed edits go out in a keepalive PUT and into sessionStorage, and
  replay after reload (a no-op when the keepalive landed).
- New strings have zh-CN entries; group names reach the DOM only as text.

Unchanged: the flat rail's markup when no group exists, the tree semantics
and single roving tab stop, sessionOrder and Alt+N.

Tests: test/tab-layout-editing.test.ts (operations, coordinator, drop
mapping, menus, rename, dismissal, SSE deferral, reload recovery, flat-rail
identity) and test/tab-layout-editing.browser.test.ts (real pointer drags,
editor paint, menu Escape), listed in BROWSER_TEST_GLOBS.
- Pointer drag: a press released outside the rail no longer lingers. The
  release is heard on window while a press is pending, a move with the
  primary button up cancels it, a new press cancels any previous drag, and
  an existing Escape listener is removed before another is added, so no
  orphaned capture listener can swallow Escape before the terminal.
- Inline group rename: a commit by blur leaves focus where the user put it;
  Enter and Escape still return focus to the header.
- A failed layout read while edits are pending keeps the held layout and the
  editor and re-reads once the write settles, so a 409 is still rebased.
  Dropping unsaved work now always says so in a toast.
- "Move to <group>" quotes the group name (with a matching zh-CN pattern), so
  a group named "New group" or "ungrouped" no longer reads or translates like
  the fixed entries.
- The group menu glyph stays visible under (hover: none).
- The sessionStorage replay copy carries { owner, baseVersion, savedAt } and is
  ignored for another owner, after 60 s, or against an older layout. A move
  with no anchor carries no index, so a replay keeps the row last.
- A 400 that survives the re-read is reported as "Could not save tab groups."
- closeTabRailActionMenu() no longer removes the group menu's DOM.
- Cancelling "Delete group" returns focus to the header.
- Stale comments updated.
@aakhter
aakhter force-pushed the pr/grouped-rail-edit branch from 2fcbf2f to bd4a1e9 Compare October 5, 2026 00:24
@aakhter

aakhter commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review, and for reproducing these against a live instance; that made each one easy to pin down.

Rebase. This is rebased onto master and keeps 7917273's behaviour as you described:

  • The rail rebuilds only when the structure key changed, with the restore block after that check, and _adoptEditedTabLayout still renders every time.
  • The header keeps your leaf handling, plus the oncontextmenu.
  • The docs keep your tree and tab-stop bullets; the drag bullet is replaced, and the Ctrl+Shift clamp sentence is carried over.
  • Both harness one-liners are in. The rebased commit passes the same 83 unit and 21 browser tests as your trial merge.

The fixes are in one commit on top (bd4a1e9):

  1. Aborted drag. A drag released outside the rail no longer leaves state behind. A new press cancels any previous drag; the release is heard on window while a press is pending; a move with the primary button up cancels; and an existing Escape listener is removed before another is added. A Chromium test replays your sequence (press, flick out, release, hover back, then a cancelled and a completed drag) and checks that both Escapes reach the terminal's textarea.
  2. Blur commit. Committing a rename by clicking elsewhere no longer requests refocus, so focus stays where the click put it. Enter and Escape still return to the header.
  3. Failed read during a write. The held layout and editor are kept, and the rail re-reads after the write settles, so the 409 still gets its rebase. Anything that does drop pending work shows a toast.
  4. Labels. Group names in "Move to" labels are quoted, with a matching zh-CN pattern, so "New group" and "ungrouped" no longer collide with the fixed entries in either language.
  5. Touch. The group glyph stays visible under (hover: none).
  6. Replay copy. The sessionStorage copy now stores owner, baseVersion and savedAt, and is ignored for another owner, after about a minute, or against a newer layout. An anchorless "Move to " no longer stores an index.
  7. 400. A 400 that survives the re-read now says "Could not save tab groups."
  8. Menu scope. closeTabRailActionMenu() no longer touches the group menu.
  9. Comments. Both stale comments are updated.
  10. Delete cancel. Cancelling "Delete group" puts focus back on the group header.

I left Ctrl+Shift+{ / } on its clamped swap and updated the _canSwapActiveTabWith comment. Routing it through moveTabRef would change what it steps over (web tabs, a parent's followers) and what it means in a sorted rail. That felt like its own change, and I'm happy to follow up if you'd like it.

Thanks also for the answers on Shift+F10 and the touch path.

@Ark0N

Ark0N commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Thanks for the quick turnaround, @aakhter. I went through bd4a1e9 item by item and all ten are fixed. The rebase keeps 7917273's behaviour exactly as described: the structure-key guard with your restore block after it, _adoptEditedTabLayout rendering every time, leaf headers without aria-expanded/aria-owns, the docs bullets, the backoff path and the _canSwapActiveTabWith comment.

What I checked

  • Live, against an isolated instance (three shell sessions, groups created through PUT /api/tab-layout, vertical rail, Playwright). The press, flick out, release outside, hover back sequence now leaves nothing behind; on 97cb5b5 it still started a phantom drag with a dimmed row and a drop mark. The real drag after it lands its PUT, and 2 of 2 Escapes reach the terminal (0 of 2 before). Renaming a group and then clicking into the terminal leaves focus on the terminal, and the next Enter goes there instead of collapsing the group.
  • Tests: the 11 tab-layout unit files plus i18n-branding pass (212/212), and test/tab-layout-editing.browser.test.ts with test/tab-activation.browser.test.ts pass (24/24).
  • The new tests fail without their fixes. Reverting item 1 fails three browser tests, both new ones included. Reverting item 2 fails the blur-commit test, and reverting item 3 fails the failed-read unit test. Taking the item 1 guards out one at a time, the pre-cancel, the buttons check and the window listeners each fail a test. The keydown-listener removal alone fails nothing, but only because the pre-cancel already removes that listener before a new drag can start, so it's belt and braces rather than a gap.
  • Items 4 to 10 by reading: the quoted label with /^Move to "(.+)"$/ in i18n.js, the (hover: none) rule winning on source order at equal specificity, the replay copy checked for owner (@single in single-user mode, matching the server), age and base version, the single 400 re-read, the scoped closeTabRailActionMenu() selector, and focus back on the header after a cancelled delete.

Agreed on keeping Ctrl+Shift+{ / } as the clamped swap. That can be its own change if it ever needs one.

Three small text-only things, none blocking

  1. The constructor comment at src/web/public/app.js:593 still says the tab layout is "read-only here".
  2. The PR description still opens with "Stacked on feat(tabs): full-row activation and tree semantics for the grouped rail #519 ... Only the last commit (2fcbf2fc) is new", and the last "For review" bullet still says edits are discarded without a toast, which item 3 fixed.
  3. Optional: nothing tests the keydown-listener removal on its own (see above). Fine to leave.

If 1 and 2 aren't in by the time I land the next batch, I'll fix them on the way in. This is ready to merge, with #526 right behind it.

@Ark0N
Ark0N marked this pull request as ready for review October 5, 2026 12:37
@aakhter

aakhter commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful pass, and for checking each guard on its own. I fixed the constructor comment at app.js:593 in ea80c5f: it now says the browser reads the layout and edits it from the vertical rail, and that sessionOrder stays the tab order. The description no longer mentions the old stack or the missing toast. I left out the optional keydown test. As you said, the pre-cancel in _onTabLayoutPointerDown removes that listener before any new drag can start, so reverting just that removal changes nothing a test could observe without reaching into internals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants