Skip to content

feat(sidebar): confirm and undo a session dragged into a folder - #4617

Merged
bolichen97 merged 1 commit into
mainfrom
feat/session-move-undo
Aug 22, 2026
Merged

feat(sidebar): confirm and undo a session dragged into a folder#4617
bolichen97 merged 1 commit into
mainfrom
feat/session-move-undo

Conversation

@dwu96

@dwu96 dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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:

  • New website/src/components/SessionMoveUndoBar.tsx: ↳ Moved to 🗀 <Folder> + an Undo button + a 2px countdown for the 8s window.
  • ChatSidebar routes both drag paths (list-view dnd-kit and board-view native drop) through a moveByDrag wrapper that performs the move via the existing useMoveSlotToFolder hook and records its inverse. Menu moves are untouched — they name their destination already.
  • The button face reads "Undo" and nothing else; the chord is a power shortcut, not part of the label, so it lives in the button's tooltip and in aria-keyshortcuts.
  • ⌘Z / Ctrl+Z fires undo. Not ⌘C (that is copy; binding undo to it would fire whenever the user copied text in the sidebar). The chord stands down while focus is in a text field, since ChatInput owns its own undo history, and ignores ⇧⌘Z (redo).
  • Hovering the bar (or focusing into it) suspends the 8s clock, and the countdown freezes with it rather than draining under a deadline that has stopped — otherwise the bar empties out under a hand that is already reaching for Undo, telling the slower reader the offer expired at the moment it is guaranteed alive. Releasing resumes from the remainder, not a fresh window. This is the most stateful part of the diff (the hold and the remainder are keyed to the offer's id so a new drag cannot inherit a suspended clock); the zero option is a bar that simply expires on the 8s clock, which is what the first revision did until UX review showed the hover path is the primary way Undo gets clicked.
  • 3 new i18n keys, translated into all 11 non-English catalogs + regenerated en-XA.

Two behaviours worth calling out because they are deliberate, not incidental:

  • The offer retires itself when the recorded move stops being the session's last one — closed, or moved again from another surface. Otherwise Undo could drag a session back out of a folder the user later put it in on purpose.
  • The countdown is a framer-motion scaleX, not a CSS animation. The global prefers-reduced-motion rule in index.css clamps every CSS animation to 0.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_id compare-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 the DISCONNECTED bit 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.mjs drives 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:

  1. no bar before the drag;
  2. after the drop the bar names the destination and the session really is inside Archive;
  3. geometrically that the bar's bottom edge is above the footer's top edge (bar bottom 896 vs footer top 897) — the placement claim as a measurement, not an eyeball;
  4. clicking Undo (the primary path) does both halves of what it promises — the session leaves Archive and the bar goes away — and the button's rendered text is exactly Undo; a second drag then proves the unlabelled chord still fires;
  5. at a 180px sidebar the destination is still there, the decoration is gone, and the folder name has real rendered width (44px) rather than being truncated to nothing.

Full local gates: tsc -b clean, eslint 0 errors, i18n:check exit 0 (all 18 checks, incl. the diff-scoped untranslated-passthrough gate), check-theme-colors clean for the new file, jscpd no new clones, and the full vitest suite at 21948 passed / 1412 files. Two failures in src/test/ThemeSelfRepair.test.tsx under 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:

drag a session into a folder, then undo

The bar in place, directly above the untouched "Older Sessions" footer:

undo bar above the 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:

the bar at a 180px sidebar

Dark theme — every surface in the bar is a theme token, so this is the proof none of it is a light-mode literal:

undo bar on the dark theme

Before the drag, and after undo (the bar retired, session back in the list)

before the drag

after undo

Related Issues

N/A — reported directly by a user hitting it in the dashboard.

Checklist

  • Single commit with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no spec covers the sidebar's folder drag surface
  • No secrets, credentials, or internal references in the diff

@dwu96
dwu96 requested a review from a team August 20, 2026 04:31
@dwu96
dwu96 requested a review from a team as a code owner August 20, 2026 04:31
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 20, 2026
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 09d68719b4aa4e68cb8ede24f9ae64b67475268b — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • The offer state machine (~150 lines: pending/latch/live/hold/deadline across three effects and four state cells) is self-contained — extract it to a useDragMoveUndo hook rather than growing the already-monolithic ChatSidebar, which would also let the admittedly untested cross-offer reset case be tested directly instead of needing a board drop zone.

[DESIGN-REVIEWED] 09d6871

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 09d68719b4aa4e68cb8ede24f9ae64b67475268b and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/pages/ChatSidebar.tsx:3030 -- delayed ACKs still display "MOVE_UNDO_MS", so the bar disappears before its countdown finishes -> Fix: derive the initial live remainder from undoDeadlineRef.

FINDING -- website/src/components/SessionMoveUndoBar.tsx:152 -- "onHoldChange?.(false)" resumes expiry when focus remains inside after pointer leave, or hover remains after blur -> Fix: track hover and focus separately and emit their union.

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:
/ai-review override gpt 09d68719b4aa4e68cb8ede24f9ae64b67475268b: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from 20ccdda to fc8a7ee Compare August 20, 2026 04:35
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 09d68719b4aa4e68cb8ede24f9ae64b67475268b — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 09d68719b4aa4e68cb8ede24f9ae64b67475268b — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 ships

Intent: 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.

  1. Bar names the destination after a drag-move — justified (silent, invisible mis-drop)
  2. Undo button with an 8s window — justified
  3. ⌘Z / Ctrl+Z fires undo while the bar is up — declared; inherited (convention), second spelling of the button
  4. Hover/focus freezes the expiry clock and countdown — undeclared (absent from "What changed")
  5. Draining countdown strip via framer-motion — justified (index.css:1285 reduced-motion clamp is real)
  6. Compact bar below 220px — justified (SIDEBAR_MIN = 180, ChatSidebar.tsx:1239)
  7. onCommitted ack option on useMoveSlotToFolder — one consumer, generalized
  8. Offer retires on close / any later move (pending-window latch) — justified (multi-client gateway is a named boundary)
  9. Playwright harness + committed screenshots/GIF — matches convention (209 sibling capture-*.mjs, 2058 tracked shots)
  10. 3 i18n keys × 13 catalogs — mandated invariant

Watch

  • Two unfixed siblings of the root cause (a drag-to-folder that names no destination): artifact→folder at ArtifactsPage.tsx:2195 (moveArtifact) and folder→folder at ChatSidebar.tsx:3094 (moveFolderTo, into a possibly collapsed folder) stay silent with no confirmation or undo. Grepped folder-drop handlers; count: 2. A shared fix is genuinely larger — accepted-and-deferred, but the point-patch shape should be a conscious choice.
  • The hover-hold suspension is the most stateful item in the diff (heldOffer, spent, undoDeadlineRef, an id-keyed effect with a deps suppression) and the description never mentions it; its zero option is a bar that simply expires on the 8s clock.
  • The chord's only support is platform convention — the drag that arms it is pointer-only, so the button already serves the hand in play; the chord is what forced the text-field guard, redo guard, and Mac/non-Mac fork plus ~10 of the tests.

Subtractions

  • Drop the exported MoveSlotOptions bag in useMoveSlotToFolder.ts — one field, one real consumer (ChatSidebar.tsx:2972); take a bare optional onCommitted?: () => void parameter.

[FIRST-PRINCIPLES-REVIEWED] 09d6871

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 09d68719b4aa4e68cb8ede24f9ae64b67475268b — this comment is updated in place on each push.

Review details

I'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 scaleX:1 target to the DOM before the follow-up setSpent re-render coalesces it away. framer-motion renders its transforms on its own rAF loop, so the 1 target set at commit is very likely superseded by the 0.85 target from the passive-effect re-render before any frame is painted. The candidate itself concedes it "could not confirm React 18 paints the intermediate frame." This is inherently timing-dependent — (c) resolves to "might be observable," not a deterministic wrong outcome. Below threshold; dropped.

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 spent.remaining, not zero, so expiry is not guaranteed. Reachability (a) and observable outcome (c) both compound conditions rather than hold deterministically. Below threshold; dropped.

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 09d68719b4aa4e68cb8ede24f9ae64b67475268b.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 09d68719b4aa4e68cb8ede24f9ae64b67475268b: <one-sentence reason>

@dwu96
dwu96 force-pushed the feat/session-move-undo branch from fc8a7ee to 0884888 Compare August 20, 2026 05:04
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@dwu96

dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Round 1 dispositions — head is now 08848884c.

GPT 5.6, BLOCKING (ChatSidebar.tsx:2831, a superseded undo offer can revive) — FIXED. Correct, and the walk-through was exactly right: with visibility derived from live state, drag A→B then a menu move B→C→B made the old A inverse match again, and its Undo would have overwritten the newer intentional move. The offer now has a one-way lifecycle instead of a derived one: a live flag flips true when the store first shows the session in the destination (needed because the move is optimistic and lands a tick after the drop), and once live the offer is dropped, never re-validated, the moment live state stops matching.

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 (never revives an offer once the session has been moved again) reproduces the reported sequence and is mutation-verified: restoring the re-validating branch fails it, and removing the deadline fails the new expiry test.

First Principles, CONCERNS — both items addressed.

  • Subtraction — drop the durationMs prop: done. Zero callers passed it, both test files drove the real constant anyway; the bar reads MOVE_UNDO_MS directly and the prop is gone.
  • Watch — the same silent-drag cause survives in the artifacts library and for folder-into-folder moves: accepted-and-deferred, now tracked in Drag into a folder is still silent in the artifacts library and for folder-into-folder moves #4626 with the specific ArtifactsPage.tsx drop targets and moveFolderTo, and a note that the right fix is one reusable inverse-move primitive rather than three copies of this bar. Agreed it is out of scope here.

Design ✅, UX ✅, Opus ✅ — no action.

Unrelated red, for whoever merges: Backend Lint & Type Check fails on 3 baselined file(s) are now black-clean. Remove them so the baseline keeps shrinking (src/kiro_crew/cli_commands.py, src/kiro_crew/memory.py, +1). This diff contains no Python — main is red for the identical reason (run 32330500851 on 67652990f), so it is pre-existing drift, fixed by python3 scripts/check_black_formatting.py --update-baseline in a separate change. I have deliberately not folded that into a sidebar PR.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from 0884888 to a55fdd5 Compare August 20, 2026 06:45
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@dwu96

dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Round 2 dispositions — head is now a55fdd502.

GPT 5.6, BLOCKING (ChatSidebar.tsx:2828, undo can overwrite a newer folder move) — FIXED, though not by the suggested remedy. The mechanism is real: another client moves B→C, its broadcast has not arrived, our offer still looks live, Undo writes A unconditionally and C is lost. The suggested fix — "revert unconditional undo until the endpoint supports conditional updates" — would delete the feature, so I made the endpoint support conditional updates instead:

  • PATCH /api/chat/slots/{slot}/folder accepts an optional expected_folder_id. On mismatch it returns 409 folder_conflict with the authoritative folder_id and mutates nothing.
  • Undo passes expectFolderId: toFolderId. useMoveSlotToFolder applies the conflict's authoritative folder to the store instead of its recorded prev, so a lost race leaves the sidebar showing where the session actually is — not the stale origin, and not the refused optimistic value.
  • The compare and the assignment are separated by no await, so unlike the folder-existence check above them they cannot interleave with another handler on the loop. This is a real CAS, not a narrowed window.
  • Live "Move to folder…" omits the field and stays unconditional — a user choosing a destination now has nothing to be conditional about. The call is still two-arg on that path, so the existing contract is unchanged.

Tests: test/test_chat_slot_folder_expected.py (4 cases: write lands on a held expectation; stale expectation refused and mutates nothing; unfiled is a real expectation, not an omission; omission stays unconditional) plus 3 in ChatSidebar.moveToFolder.test.tsx (expectation passed through; folder_conflict lands on the server's folder; a non-conflict 409 still rolls back). Mutation-verified: neutering the CAS fails 2 backend cases, and dropping the conflict branch fails the store-lands-on-server case.

Design Review watch item — FIXED. Good catch: undo replayed fromFolderId blindly, and useMoveSlotToFolder does not degrade a deleted folder — the endpoint rejects an unknown id with 400, and the sidebar maps unknown ids to unfiled, so the session would have been left carrying a placement no view can show. Undo now checks the origin still exists and posts null when it does not; new test degrades a deleted origin folder to unfiled instead of replaying a dead id (mutation-verified).

First Principles CONCERNS — subtraction taken. The 14 intermediate GIF frames are gone from temp-screenshots/session-move-undo/; the harness now writes them to the OS temp dir, so the committed evidence is the assembled GIF and the four PNGs only. (The earlier Watch item — the same silent-drag cause on the artifacts library and folder-into-folder moves — remains accepted-and-deferred in #4626.)

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: Backend Lint & Type Check still fails on baselined file(s) are now black-clean … prune them. My Python change adds 0 new offenders (black gate: 0 new offender(s)); main fails identically (run 32330500851 on 67652990f), and PR #4618 already carries the baseline prune. Deliberately not duplicated here.

@dwu96

dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

First Principles round 2 (a55fdd502) — the only open item is the Watch, and it is the one already accepted-and-deferred: #4626 tracks exactly these two surfaces, and it names moveFolderTo and the four folder-drop targets in ArtifactsPage.tsx explicitly, plus the reason a shared fix is bigger than this PR (it wants one reusable last-move-plus-inverse primitive, not three copies of this bar). Nothing new to fix here; the class is open and tracked rather than forgotten.

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 ✅.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from a55fdd5 to 365994c Compare August 20, 2026 07:12
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Aug 20, 2026
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from fe46554 to 2fd4dfb Compare August 20, 2026 21:23
@dwu96

dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Round 7 — head is now 2fd4dfb9e. The compare-and-set is gone and the PR is frontend-only again. Both blocking reviewers converged on the same answer from opposite directions, so I took it.

  • GPT 5.6 (chat_folders.py:632, folder-only CAS permits stale undo after an ABA move): "Revert the undo/CAS hunks until the request can compare a monotonic placement revision."
  • First Principles (Watch): the CAS stack is "permanent one-way-door endpoint surface with exactly one real consumer" and names the smaller shape as deleting expected_folder_id, folderConflict(), MoveSlotOptions.expectFolderId, and the backend conditional branch.

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 git diff against the branch base now shows zero change under src/kiro_crew/ and test/test_chat_slot_folder_expected.py is deleted and the endpoint is byte-identical to base.

What still protects the user, all client-side and all mutation-verified:

  • the offer arms only on the server's acknowledgement (round 5 — closes the slow-PATCH self-reversal);
  • a placement observed while pending that is neither origin nor destination is latched, so the away-and-back case never arms (round 6);
  • an armed offer is dropped the moment live state stops matching, and dropped is final.

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 main, and a wrong undo there is visible on screen and re-correctable. Closing it properly wants a monotonic placement revision applied to all folder writes, not bolted onto undo alone — that is a bigger, separate change and I have not smuggled a half of it in here.

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: PATCH /api/chat/slots/{slot}/folder gained contract surface (409 folder_conflict, silent degrade-to-unfiled on success) that lived only in code comments while learn-cron-dashboard.md specs the sibling endpoints in detail. With the CAS deleted the endpoint has no new contract, so there is nothing to spec — the honest fix rather than the documented one. Your Suggestion is done as asked: the pending → live → dropped lifecycle, including the latch, is now one state machine in SessionMoveUndoBar's doc comment instead of four inline comments.

First Principles — the Watch is the change above. The earlier moveFolderTo/artifacts-library generalisation stays deferred in #4626.

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: tsc -b clean, eslint 0 errors, i18n:check exit 0, flake8/mypy/black clean on the (now unmodified) Python, 28 targeted frontend tests, and all 15 real-browser harness assertions green on the rebuilt dist.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 20, 2026
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from 2fd4dfb to b58eca0 Compare August 20, 2026 21:51
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@dwu96

dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Round 8 — head is now b58eca09f. Design flipped to ✅ PASS on the previous head; three findings fixed here.

GPT 5.6 BLOCKING (ChatSidebar.tsx:5244, retired undo remains actionable during its exit animation) — FIXED. Real, and a good catch: AnimatePresence keeps the bar mounted for its 150ms exit, and that exiting instance still holds the props it had while live — so a click or ⌘Z in that window fired a genuine undo for an offer that had already been retired.

I did not take the suggested remedy (remove the AnimatePresence wrapper), because it closes the symptom rather than the defect: it happens to shorten the window to zero while leaving the same class of bug reachable any time the component outlives the state it was rendered from. Instead undo now re-checks the offer's identity against current state at invocation time — via a ref, not the closure, precisely because the closure is the thing that goes stale:

const undoDragMove = useCallback((offerId: number) => {
  const dragMove = dragMoveRef.current   // NOT the closure's copy
  if (!dragMove || dragMove.id !== offerId) return

New suite ChatSidebar.moveUndoStale.test.tsx captures the onUndo the sidebar handed the bar and calls it after retirement — which is exactly what the exiting instance does, and is testable without depending on animation timing. Mutation-verified: reading through the closure instead of the ref fails it.

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.

  • No hover/focus pause on the countdown. Correct, and the sharpest framing of it is yours: the deadline expiring under a hand already reaching for Undo recreates the exact "hunt through folders" failure the bar exists to prevent, for the users least able to re-aim a drag. The deadline is now suspended while the pointer is over the bar or focus is inside it (onHoldChange → the sidebar's timer effect), mutation-verified.
  • "Unfiled" is Artifacts vocabulary. Also correct — and the fix is not a synonym, because "Moved to Removed from folder" does not parse. The root case is now its own sentence, "Removed from folder", matching the sidebar's own drop zone (chatSidebar.drop_here_to_remove_from_folder); the unfiled key is replaced by removed_from_folder across en + 11 locales with en-XA regenerated, and the tooltip follows the same branch.

Gates: tsc -b clean, eslint 0 errors, i18n:check exit 0 ([changed-passthrough] 0), 34 targeted frontend tests, 15/15 real-browser harness assertions on the rebuilt dist. Python untouched — the diff is still frontend-only after the CAS deletion.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@dwu96

dwu96 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Round 9 on b58eca09f — stopping here for a maintainer's call, because GPT 5.6's demands have now closed a full circle and there is no revision of this feature that satisfies all of them.

The sequence, all on the same anchor (residual/crash-data-loss-corruption):

Round GPT's finding GPT's remedy What I did
2 Undo writes unconditionally and can clobber a concurrent move Make the write conditional Built expected_folder_id compare-and-set
3, 6, 7 A folder-value CAS cannot detect an ABA move Revert the CAS until a monotonic revision exists Deleted the CAS (First Principles independently asked for the same deletion)
9 (now) The unconditional write can lose a newer placement Revert the undo-write feature hunk

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 main today, so this PR is not introducing the exposure, it is adding a feature that shares it).

Every other lane is now green on this head, including the two that were amber last round:

  • Design Review ✅ PASS — spec-gap Watch dissolved with the CAS, state machine consolidated into the component doc comment.
  • UX Review ✅ PASS — hover/focus now suspends the 8s deadline; the root case reads "Removed from folder" in the sidebar's own vocabulary.
  • First Principles ✅ PASS — the "permanent one-way-door endpoint surface" Watch went away with the CAS deletion.
  • Opus 4.8 ✅ no blocking findings.

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:

  1. /ai-review override gpt b58eca09f31a07c9e9b5cf460d9e367c537eeebb: <reason> — accepting a broadcast-gap window that every existing folder write already has, where a wrong undo is visible on screen and re-correctable.
  2. Land a monotonic folder_rev on the slot, applied to all folder writes, as its own change — then undo compares it and this class closes for the whole surface rather than for one button.

I am not self-authorizing the override, and I am not re-adding the CAS that both blocking reviewers asked me to delete.

@dwu96

dwu96 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

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 main). The residual exposure is a move this client has not been told about yet — the broadcast gap — where a wrong undo is visible on screen and re-correctable. Closing it properly wants a monotonic placement revision applied to all folder writes, tracked separately; bolting it onto undo alone would protect one button and leave the pattern.

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 moveFolderTo, artifact→folder and folder-nesting via ArtifactsPage) — accepted-and-deferred with no change to this PR, per the same reasoning: it is a general pattern, and the shared mechanism belongs in its own change.

Everything else is green on b58eca09f: 57 checks pass, Design ✅, UX ✅, Opus 4.8 ✅, and the only red is GPT plus the PR Readiness roll-up that aggregates it.

@dwu96
dwu96 force-pushed the feat/session-move-undo branch from b58eca0 to 9787cc6 Compare August 22, 2026 07:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 22, 2026
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.
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from 9787cc6 to 09d6871 Compare August 22, 2026 08:23
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 22, 2026
@dwu96

dwu96 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Round 11 — head 09d68719b, rebased onto 24775e7ff. Three verdicts on 9787cc607, dispositioned individually.

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 transition from its own mount, so hovering — the exact path to Undo — drained the bar to empty while the offer was guaranteed alive, and the release then re-armed a full 8s behind an already-empty bar.

Fixed by giving the two one clock instead of making two clocks agree. The sidebar owns the remainder and hands it down (remainingMs, paused); the countdown runs linearly from remaining / MOVE_UNDO_MS over exactly that remainder, which keeps scaleX == remaining / MOVE_UNDO_MS true at every instant, and freezes at that fraction while held. A resume continues from the frozen width rather than restarting.

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 transform is read from the live page, which is the one thing a jsdom test cannot do since framer's animation never runs there. Harness is now 19/19: running 0.846 → hover freezes at 0.837 → still 0.837 after 1.5s of hold → release resumes 0.837 → 0.800. Pre-fix that freeze assertion fails. Plus 36/36 vitest across the four suites and 451 across all 57 ChatSidebar suites; the two new cases are mutation-verified in both directions (re-arming the full window on release, and a countdown that ignores paused, each turn their test red).

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. 8c751bda1 (#3240) landed on main at 07:40:49Z, 71 seconds before 9787cc607 was pushed, so the review harness read refs/pull/4617/merge — which had already absorbed it — against a base snapshot taken before it landed, and attributed main's own commit to this PR. The arithmetic matches the report exactly: this PR's real code is ~1,394 lines, 8c751bda1 is 2,917, and 1394 + 2917 ≈ the "4,497-line patch" it read, with 2917/4497 = 65% ≈ "roughly two-thirds". On the branch itself git diff origin/main...HEAD was 26 files with zero occurrences of PANE_HYDRATE_LIMIT, and 8c751bda1 was not an ancestor of that head at all. It is an ancestor now: the rebase onto 24775e7ff puts it in the base, so items 6–10 of that reading cannot recur.

Its earlier Watch — the three sibling silent drags (moveFolderTo, artifact→folder, folder nesting) — remains accepted-and-deferred with no change here, per the maintainer decision above: it is a general pattern and the shared mechanism belongs in its own change.

GPT 5.6 🔴 BLOCKING — ChatSidebar.tsx:2999, "remove the unconditional undo write and associated offer" — DECLINED, unchanged. Same finding and same remedy as round 2 and round 9; the maintainer decision recorded above is the answer, and re-stating it here rather than re-opening it. Conditional was judged insufficient (rounds 3/6/7, and the compare-and-set built for it was deleted at this reviewer's own request), unconditional is judged unsafe (rounds 9/11): no revision of the feature satisfies the sequence. The accepted residual is the broadcast gap every folder write on main already carries, where a wrong undo is visible and re-correctable; closing it properly wants a monotonic placement revision on all folder writes, not bolted onto one button. Clearing this needs /ai-review override gpt 09d68719b4aa4e68cb8ede24f9ae64b67475268b: <reason> from a repository writer — not something I will self-authorize.

@dwu96

dwu96 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Round 12 on 09d68719b — GPT 5.6 is ✅ no findings, UX flipped to ✅ PASS, Design ✅, Opus ✅ (it falsified both of its own candidates). The remaining verdicts are advisory; each one answered individually below. No code change in this round — every item is either prose or a deferral, and I am not re-pushing a green SHA to make a cosmetic edit.

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 "Undo ⌘Z button" two lines above the bullet that says the face reads Undo and nothing else.

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" (ArtifactsPage.tsx:2195 artifact→folder, ChatSidebar.tsx:3094 folder→folder) — ACCEPTED-AND-DEFERRED, tracked in #4626. Your framing is the right one and I'll state the choice consciously rather than let it read as an oversight: the shared fix is a confirm-and-undo affordance for every folder-drop handler, which needs a common owner for the offer lifecycle rather than three copies of it. The point-patch here is deliberate because the session sidebar is where the harm was actually reported, and shipping the pattern once — with the ack-gating, the pending-window latch and the drop-only lifecycle worked out — is what makes the shared version cheap instead of speculative. Note your count moved from 3 to 2 between SHAs: the third (ChatSidebar.tsx:2967/2974) is the same moveFolderTo you now cite once.

Subtraction, "drop the exported MoveSlotOptions bag for a bare onCommitted?: () => void" — AGREED IN PRINCIPLE, DEFERRED, and I want to be straight about why. You are right that it is one field with one real consumer, and the bag is speculative generality of exactly the kind worth removing. It is also a ~10-line cosmetic edit, and landing it means a new SHA that re-arms all 60 checks and all five review lanes on a PR whose blocking reviewer cleared for the first time in eleven rounds — after a finding that had cycled between "make the write conditional" and "make it unconditional" since round 2. Spending that on a type alias is a bad trade, and it is the maintainer's call rather than mine, so I am surfacing it instead of quietly doing either thing. If diwm would rather have the subtraction than the current green, say so and it goes in.

Design Review's suggestion — "extract the ~150-line offer state machine into a useDragMoveUndo hook rather than growing the monolithic ChatSidebar" — ACCEPTED-AND-DEFERRED, same reason, and it would genuinely help one specific thing. A hook is the right home for it, and it would make the one gap I documented testable: the cross-offer case (a second drag while the first bar is still held) has no test because a second drag needs a board drop zone and those unmount once the first move lands — a hook could be driven directly, with no DOM at all. That is a real argument for doing it. It is also a larger refactor than this PR's remit, on the same green SHA, and it is the natural first step of the shared-mechanism change that #4626 already tracks. Folding it in there keeps the extraction and its consumers in one reviewable change instead of splitting them across two.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 22, 2026
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