feat(sidebar): confirm and undo a session dragged into a folder - #4617
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A real, user-reported harm; the ack-gated, drop-only offer lifecycle is the right shape, and the earlier CAS surface was correctly removed. Suggestions
[DESIGN-REVIEWED] 09d6871 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/pages/ChatSidebar.tsx:3030 -- delayed ACKs still display FINDING -- website/src/components/SessionMoveUndoBar.tsx:152 -- FINDING -- temp-screenshots/session-move-undo/5-narrow-180.png:1 -- the claimed post-drop narrow capture shows the session unmoved and no Undo bar -> Fix: regenerate it after the capture script’s asserted narrow drag. [GPT-REVIEWED] 09d6871 False positive or not applicable? A repository writer can comment: |
20ccdda to
fc8a7ee
Compare
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS The bar names the destination, offers a true inverse, pauses under hover, survives 180px and dark theme — the label keeps every promise it makes. [UX-REVIEWED] 09d6871 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of Reading done — contract, intent, patch, and the surrounding repo (toast/undo mechanisms, sibling drag paths, capture-script conventions, the reduced-motion rule cited as rationale). Findings verified by grep counts where the contract demands them. Final review follows. First-Principles-Verdict: CONCERNS Hover-hold and the ⌘Z chord ride beyond the declared fix, and the same silent-drag cause has two counted unfixed siblings. What this change shipsIntent: after a mis-aimed drag files a session into an unseen folder, tell the user where it went and let them take it back — an ADDITION responding to a named harm.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 09d6871 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've falsified both candidates against the actual code. Candidate 1 (countdown flashes to full for one paint on hover): The claimed flash depends on React 18 flushing a passive effect after an intermediate browser paint AND on framer-motion applying the Candidate 2 (single last-write boolean for hold): The code is genuinely a last-write-wins boolean and cannot represent "hover OR focus," so the documented contract is violated in the abstract. But the trigger requires a specific mixed interleaving — pointer over the bar while keyboard focus sits on the Undo button, then the pointer leaves — which is not a flow that occurs in ordinary use (a pure-mouse or pure-keyboard interaction, the two common paths, both work). And the observable failure (Undo expiring out from under the user) further requires the remaining frozen time to elapse while they dawdle: on unhover the timer resumes for No new grounded defect found in Step 2: the freeze/resume deadline math, the one-way lifecycle latching, and the stale-offer id guard all re-derive as correct. No findings. [OPUS-REVIEWED] 09d6871 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
fc8a7ee to
0884888
Compare
|
Round 1 dispositions — head is now GPT 5.6, BLOCKING ( The 8s deadline moved out of the bar and into the sidebar as part of the same fix — a subtler leg of the same defect: an offer whose optimistic move never became visible (request failed and rolled back) had no bar mounted to run a timer, so it could sit around indefinitely waiting for a later unrelated move to make it match. It now dies on the same clock whether or not it was ever visible. New regression test ( First Principles, CONCERNS — both items addressed.
Design ✅, UX ✅, Opus ✅ — no action. Unrelated red, for whoever merges: |
0884888 to
a55fdd5
Compare
|
Round 2 dispositions — head is now GPT 5.6, BLOCKING (
Tests: Design Review watch item — FIXED. Good catch: undo replayed First Principles CONCERNS — subtraction taken. The 14 intermediate GIF frames are gone from UX Review suggestion (silent expiry) — no change, per its own "no change needed now"; the countdown line is the warning and the row menu is the recovery path. Unrelated red, unchanged: |
|
First Principles round 2 ( Every other lane is green on this head: GPT 5.6 ✅ (its round-2 blocking finding is closed by the compare-and-set), Design ✅, UX ✅, Opus ✅. |
a55fdd5 to
365994c
Compare
fe46554 to
2fd4dfb
Compare
|
Round 7 — head is now
Those are the same deletion. GPT is right that a folder-value comparison cannot distinguish "still where I put it" from "moved away and back"; First Principles is right that the surface it costs is disproportionate to an 8-second window with one caller. Defending it further would have been defending machinery neither reviewer wanted, and What still protects the user, all client-side and all mutation-verified:
What is now exposed, stated plainly: a move this client has not been told about yet. That is the same broadcast gap the row menus, the session header, and drag itself already live with on Design Review CONCERNS — both items addressed, one dissolved. The spec gap you identified was real and is the reason I am glad you flagged it: First Principles — the Watch is the change above. The earlier Also re-instated: the client-side deleted-origin degrade, which I had removed in round 5 because the server owned it. With the server branch gone, replaying a deleted origin would be rejected as an unknown folder and Undo would do nothing — so the guard is back, with its mutation-verified test. Gates: |
2fd4dfb to
b58eca0
Compare
|
Round 8 — head is now GPT 5.6 BLOCKING ( I did not take the suggested remedy (remove the const undoDragMove = useCallback((offerId: number) => {
const dragMove = dragMoveRef.current // NOT the closure's copy
if (!dragMove || dragMove.id !== offerId) returnNew suite Being straight about coverage: the guard has two parts and only one is pinned. The current-state read is what closes the reported defect. The id comparison on top of it covers a narrower case — a new offer armed inside the exit window, where current state is non-null and a bare "is there an offer?" check would undo the wrong one. I could not construct that case: retiring the first offer moves the session out of the target folder, whose board drop zone then unmounts, so a second drag has nothing to aim at. It is kept as defence in depth and the test file says so rather than pretending otherwise. UX CONCERNS — both taken.
Gates: |
|
Round 9 on The sequence, all on the same anchor (
Round 9's finding is round 2's finding, and its remedy is "delete the feature". Conditional was rejected as insufficient; unconditional is rejected as unsafe; the only remaining shape is a monotonic placement revision, which I declined to bolt onto undo alone (it belongs on all folder writes — the row menus, the session header, and drag itself are all unconditional on Every other lane is now green on this head, including the two that were amber last round:
Remaining CI: 31 pass / 0 fail apart from GPT and the PR Readiness roll-up that aggregates it (23 still running at the time of writing). The decision is a repository writer's, not mine:
I am not self-authorizing the override, and I am not re-adding the CAS that both blocking reviewers asked me to delete. |
|
Maintainer decision (diwm), recorded so the standoff has a resting state: The feature stays. GPT 5.6's round-9 remedy — "revert the undo-write feature hunk" — is declined, not deferred. Its own round-2 finding demanded the opposite (make the write conditional), the CAS built for it was then deleted at its round-3/6/7 request and at First Principles' request, and round 9 re-raised round 2. Conditional judged insufficient, unconditional judged unsafe: there is no revision of this feature that satisfies the sequence, so continuing to iterate it produces churn rather than safety. What is being accepted, stated plainly: undo writes unconditionally, like every other folder move in this product today (row menus, session header, and drag itself are all unconditional on Within that, the lifecycle is what keeps the offer honest, and all of it is mutation-verified: it arms only on the server's acknowledgement, a placement observed while pending that is neither origin nor destination is latched (so away-and-back never arms), an armed offer is dropped the moment live state diverges and dropped is final, and undo re-checks the offer's identity against current state so the 150ms exit window cannot fire a stale one. First Principles' Watch (three sibling silent drags: folder re-parent via Everything else is green on |
b58eca0 to
9787cc6
Compare
Dragging a session onto a folder was the one folder move with no feedback:
the row left the list, nothing said where it landed, and a drop one row off
the intended target could only be found by opening folders one at a time.
Every DRAG-initiated move now parks its inverse and the sidebar offers it
back for 8s: a bar naming the destination folder, an Undo button, and the
platform undo chord (⌘Z / Ctrl+Z — not ⌘C, which is copy). The button face
reads "Undo" and nothing else; the chord lives in its tooltip and in
aria-keyshortcuts. Menu moves ("Move to folder…") name their destination
already and do not arm it.
Placement is the design decision, not the bar itself. It renders as a
sibling AFTER the session lanes and BEFORE the "Older Sessions" footer, and
outside every scroll container, so it covers neither that persistent control
nor the row that just moved — which is the row the user needs to see to judge
the drop. The cost is that the footer shifts down by ~30px while the bar is
up; a 150ms height transition pays for it, and the alternative (a floating
toast) buys the stable layout by hiding the evidence. Below a 220px sidebar
the "Moved to" prefix is dropped so the DESTINATION survives rather than
being the first thing truncated.
The offer's lifecycle is one-way and is documented as a state machine in
SessionMoveUndoBar's doc comment, because it is the part a future editor is
most likely to break:
- It arms only on the server's ACKNOWLEDGEMENT of the move, never on the
optimistic write. Arming optimistically let the user undo while the
original PATCH was still in flight, and the original write would then
land afterwards and silently reverse the undo.
- While pending, any placement that is neither the origin nor the
destination is another client's move landing inside the window, and is
latched: by acknowledgement time live state may match the destination
again (a move away and back) and nothing later could tell.
- An armed offer is dropped the moment live state stops matching its
destination, and dropped is final — never re-validated — so a retired
offer cannot come back and replay its inverse over a newer move.
Undo writes unconditionally, like every other folder move in the product.
An earlier revision of this branch added an `expected_folder_id`
compare-and-set to the folder endpoint; it is gone. Both blocking reviewers
independently arrived at the same smaller shape — a folder-VALUE comparison
cannot distinguish "still where I put it" from "moved away and back", and
the surface it costs (a 409 contract, a degrade-to-unfiled branch, hook
plumbing) served one consumer for an 8s window. What is left protecting the
user is the lifecycle above; what remains exposed is a move this client has
not been told about yet, which is the same broadcast gap every other folder
write here already lives with, and where a wrong undo is visible on screen
and re-correctable. Closing that properly needs a monotonic placement
revision on the slot, applied to all folder writes rather than to undo alone.
A drop onto the folder a session already lives in arms nothing. An origin
folder deleted inside the window degrades to unfiled rather than replaying a
dead id the endpoint would reject.
Tests: 12 sidebar cases (drop arms it only after the ack, a failed move arms
nothing, a supersede latched while pending is never armed, same-folder drop,
retirement on close, expiry on the sidebar's own clock, undo posts the
original folder, deleted-origin degrade, and the two placement contracts)
and 13 component cases (labels, the bare "Undo" face, both undo paths, the
Mac vs non-Mac chord, the text-field and redo guards, compact mode, unmount
cleanup). All mutation-verified — including two tests that first passed for
the wrong reason and were rewritten: a placement assertion using
compareDocumentPosition, which this DOM answers with the DISCONNECTED bit
set either way, and a supersede test using a single away-move, which the
armed-offer check already caught.
website/scripts/capture-session-move-undo.mjs drives the real built SPA with
real pointer events: it CLICKS the button and asserts both halves of what a
click must do (the session leaves the folder AND the bar goes away), proves
the unlabelled chord still fires, asserts the placement geometrically
(bar bottom above footer top), and measures the destination's rendered width
at a 180px sidebar.
Three more from review: a retired offer stayed actionable for the 150ms
AnimatePresence exit (the exiting instance keeps the props it had while live, so
a click or ⌘Z in that window fired a stale undo) — undo now re-checks the offer's
identity against CURRENT state at invocation time instead of trusting the closure
it was created in. The 8s deadline is suspended while the pointer is over the bar
or focus is inside it, so it cannot expire under a hand already reaching for Undo.
And the root case is its own sentence, "Removed from folder", rather than "Moved
to Unfiled" — the sidebar's own drop zone says "remove from folder" and "Unfiled"
is Artifacts vocabulary this surface never shows.
9787cc6 to
09d6871
Compare
|
Round 11 — head UX Review 🟡 CONCERNS — "countdown drains while the deadline is suspended" — FIXED. This was real and it was mine: the hold I added suspended the sidebar's deadline but the bar's countdown was a one-shot Fixed by giving the two one clock instead of making two clocks agree. The sidebar owns the remainder and hands it down ( The hold and the remainder are now keyed to the offer id rather than reset when a new one arrives. That is deliberate: the pointer never leaves a bar that is replaced, so nothing else would clear the hold, and a boolean would have let the next offer inherit a suspended clock and never expire. Keying it means a non-matching id reads as "full, running" by construction — there is no reset branch left to forget. Stated plainly: the cross-offer case carries no test, because a second drag needs a board drop zone and the zones unmount once the first move lands; that is exactly why the shape is structural instead of a guard. Noted in the code. Verified in a real browser, not only jsdom — the countdown's First Principles 🟡 CONCERNS — "two-thirds of this diff is an undeclared second feature (the bounded pane hydrate)" — REBUTTED, and the cause is now gone. The diff never contained it. Its earlier Watch — the three sibling silent drags ( GPT 5.6 🔴 BLOCKING — |
|
Round 12 on First Principles — item 4, "hover-hold is undeclared in What changed" — FIXED (description only). Fair hit, and the omission was mine: the hold arrived as the fix for UX review's earlier finding and never made it into the body. The description now declares it explicitly — that hovering or focusing the bar suspends the 8s clock, that the countdown freezes with it, that releasing resumes from the remainder rather than a fresh window, that the hold and the remainder are keyed to the offer's id so a new drag cannot inherit a suspended clock, and that the zero option is a bar that simply expires on the 8s clock — which is exactly what the first revision did, until UX review pointed out that hovering is the primary path to the button, so the un-held version drained the bar under the hand reaching for it. Naming the zero option is the part I owed you. While in there I also fixed a contradiction the same reading exposed: one bullet still described an " Item 3, "the ⌘Z chord's only support is platform convention; it forced the text-field guard, the redo guard, the Mac fork and ~10 tests" — ACCEPTED AS A MAINTAINER DECISION, not removed. The cost accounting is correct and worth having on the record. The chord is nonetheless a deliberate request from the repository owner, who first asked for ⌘C and accepted the correction to ⌘Z precisely because ⌘C is copy and would fire whenever text was selected in the list. So this is not an unexamined convention-follow: it is a decision made with the alternative in view. I am not subtracting it on an advisory. Item 1, "two unfixed siblings of the root cause" ( Subtraction, "drop the exported Design Review's suggestion — "extract the ~150-line offer state machine into a |
Problem / Motivation
Dragging a session onto a folder in the sessions sidebar is the only folder move that gives no feedback. The row leaves the list, nothing on screen says where it went, and the folder it landed in may well be collapsed. Drop it one row off the folder you aimed at and the session is simply gone from view — the only way back is opening folders one at a time until it turns up.
Every other route into a folder names its destination: the row menu's "Move to folder…", the session-header dropdown. Only the coarse, mis-aimable gesture is silent.
Why it matters
A drag is easy to get wrong (small targets, a moving list, auto-expanding folders mid-drag) and the failure is invisible rather than noisy: the user does not learn they made a mistake, they learn a session disappeared. That turns a 200ms slip into a hunt through the folder tree, and there is no undo — the move is already persisted.
What changed (motivation → approach → change)
Goal: after a drag, the user should know where the session went and be able to take it back without hunting.
Approach — where it goes, decided against two alternatives. A floating toast keeps the layout stable but has to cover something, and at the bottom of the sidebar the two things it can cover are the persistent "Older Sessions" footer control and the last rows of the list — including the row that just moved, which is exactly the evidence needed to judge the drop. A minimal one-line note avoids that but is too quiet for the mistake it exists to catch. So the bar renders in the flow: a sibling AFTER the session lanes and BEFORE the footer separator, outside every scroll container. It occludes nothing; it pushes the footer down ~30px while it is up, softened by a 150ms height transition.
Change:
website/src/components/SessionMoveUndoBar.tsx:↳ Moved to 🗀 <Folder>+ anUndobutton + a 2px countdown for the 8s window.ChatSidebarroutes both drag paths (list-view dnd-kit and board-view native drop) through amoveByDragwrapper that performs the move via the existinguseMoveSlotToFolderhook and records its inverse. Menu moves are untouched — they name their destination already.aria-keyshortcuts.ChatInputowns its own undo history, and ignores ⇧⌘Z (redo).en-XA.Two behaviours worth calling out because they are deliberate, not incidental:
scaleX, not a CSS animation. The globalprefers-reduced-motionrule inindex.cssclamps every CSS animation to0.01ms, which would drain the bar instantly and read as "already expired" for exactly the users least able to re-aim a drag.A drop onto the folder a session already lives in arms nothing — there would be nothing to undo.
Undo writes unconditionally, like every other folder move in the product — and the offer's lifecycle is what keeps it honest. It arms only on the server's ACKNOWLEDGEMENT (arming on the optimistic write let undo fire while the original PATCH was in flight, so the original write landed afterwards and silently reversed it). While pending, any placement that is neither the origin nor the destination is another client's move landing inside the window and is latched — by ack time live state may match the destination again (a move away and back) and nothing later could tell. An armed offer is dropped the moment live state stops matching, and dropped is final, so a retired offer cannot replay its inverse over a newer move.
An earlier revision of this branch added an
expected_folder_idcompare-and-set to the folder endpoint. It is gone, and the PR is frontend-only again: both blocking reviewers independently arrived at the same smaller shape — a folder-value comparison cannot distinguish "still where I put it" from "moved away and back", and the surface it cost (a 409 contract, a degrade-to-unfiled branch, hook plumbing) served one consumer for an 8-second window. What remains exposed is a move this client has not been told about yet: the same broadcast gap every other folder write here already lives with, where a wrong undo is visible on screen and re-correctable. Closing it properly needs a monotonic placement revision on the slot, applied to all folder writes rather than to undo alone.A drop onto the folder a session already lives in arms nothing. An origin folder deleted inside the window degrades to unfiled rather than replaying a dead id the endpoint would reject.
Tests
website/src/test/SessionMoveUndoBar.test.tsx(13 cases) — destination naming, the unfiled label, the live-region announcement, undo by click, undo by chord, the Mac (⌘Z) vs non-Mac (Ctrl+Z) binding, the text-field guard, the redo/bare-key guard, expiry firing once at the deadline, and both unmount cleanups.website/src/test/ChatSidebar.moveUndo.test.tsx(12 cases) — a drop performs the move AND arms the bar naming the destination; undo posts the original folder back and retires the offer; a same-folder drop arms nothing; the offer retires when the session is closed; and the two placement contracts (earlier in document order than the footer, and no scrolling ancestor).Every case was mutation-verified — including one that had to be rewritten because it was vacuous: the placement assertion originally used
compareDocumentPosition, which this DOM implementation answers with theDISCONNECTEDbit set, so it reported "the footer follows the bar" even after the bar was moved below the footer. It now compares indices in document order, and fails on that mutation.Manual verification
node website/scripts/capture-session-move-undo.mjsdrives the real built SPA (website/dist,/api/**stubbed) with real pointer events — dnd-kit's sensors are pointer-based, so there is no synthetic shortcut — and asserts, exiting non-zero otherwise:Archive;bar bottom 896 vs footer top 897) — the placement claim as a measurement, not an eyeball;Archiveand the bar goes away — and the button's rendered text is exactlyUndo; a second drag then proves the unlabelled chord still fires;Full local gates:
tsc -bclean,eslint0 errors,i18n:checkexit 0 (all 18 checks, incl. the diff-scoped untranslated-passthrough gate),check-theme-colorsclean for the new file,jscpdno new clones, and the fullvitestsuite at 21948 passed / 1412 files. Two failures insrc/test/ThemeSelfRepair.test.tsxunder full-suite parallelism pass in isolation and touch no code in this diff.Screenshots / video
The value here is a sequence, so the GIF is the primary evidence — drag out of the list, land in
Archive(note the folder count going 0 → 1), the bar naming the destination, the countdown draining, then Ctrl+Z putting the session back:The bar in place, directly above the untouched "Older Sessions" footer:
At
SIDEBAR_MIN(180px), where the prefix and the shortcut label are dropped so the destination survives rather than being the first thing truncated:Dark theme — every surface in the bar is a theme token, so this is the proof none of it is a light-mode literal:
Before the drag, and after undo (the bar retired, session back in the list)
Related Issues
N/A — reported directly by a user hitting it in the dashboard.
Checklist