Conversation
|
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. |
|
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 Before mergeMust fix
Should fix
Small ones
Your "For review" questions
Rebase#517 and #519 landed in 1.34.0 together with a fix commit, 7917273, so this now conflicts in
What I checked locallyAt this head, the three tab-layout unit files (68/68) and |
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.
2fcbf2f to
bd4a1e9
Compare
|
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 fixes are in one commit on top (bd4a1e9):
I left Ctrl+Shift+{ / } on its clamped swap and updated the Thanks also for the answers on Shift+F10 and the touch path. |
|
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, What I checked
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
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. |
|
Thanks for the careful pass, and for checking each guard on its own. I fixed the constructor comment at |
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:
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:There are no server changes.
Unchanged: the flat rail's markup with no groups, the tree semantics and single roving tab stop from #519,
sessionOrderand 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 withnpm run test:browser.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
test/inline-rename.test.tshas one browser failure ("Vertical rail paints typing in an unclamped editor restores clamp on cancel"). It fails the same way without this commit.