Skip to content

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

Open
dwu96 wants to merge 1 commit into
mainfrom
feat/session-move-undo
Open

feat(sidebar): confirm and undo a session dragged into a folder#4617
dwu96 wants to merge 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 ⌘Z 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).
  • 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 is a compare-and-set, not a blind write. Another client can move the session on inside the 8s window and its broadcast need not have arrived here, so undo sends the folder it expects the session to still be in: PATCH /api/chat/slots/{slot}/folder takes an optional expected_folder_id, refuses a stale one with 409 folder_conflict, and reports the authoritative folder — which the move hook applies to the store, so the sidebar shows where the session actually is rather than a third invented placement. The compare and the write are separated by no await, so they are atomic on the event loop. Live "Move to folder…" omits the field and stays unconditional, exactly as before. A fromFolderId whose folder was deleted inside the window degrades to unfiled rather than replaying a dead id.

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 (8 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).

test/test_chat_slot_folder_expected.py (7 cases) covers the compare-and-set on the endpoint: the write lands when the expectation holds, a stale expectation is refused and mutates nothing, unfiled is a real expectation rather than an omission, and omitting the field stays unconditional. ChatSidebar.moveToFolder.test.tsx gains 3: the expectation is passed through, a folder_conflict lands on the server's folder rather than the caller's, and a non-conflict 409 still rolls back.

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 2f5e6002eeefd19bd1553cff810903b10db7f185 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real, well-evidenced UX harm, fixed at the right layer with a proportionate compare-and-set; alternatives and failure modes are argued, not assumed.

Suggestions

  • PATCH /api/chat/slots/{slot}/folder gained public contract surface (expected_folder_id, 409 folder_conflict, conditional-write degrade-to-unfiled) — the checklist's "no spec covers the sidebar's folder drag surface" is true of the frontend, but this backend endpoint sits in learn-cron-dashboard.md's catalog row alongside the other slots/{slot}/* endpoints; add the one spec line in this commit so the next endpoint change doesn't rediscover the semantics from comments.

[DESIGN-REVIEWED] 2f5e600

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 2f5e6002eeefd19bd1553cff810903b10db7f185.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/chat_folders.py:632 -- Value-only CAS permits an ABA overwrite
if expected != (slot.folder_id or ""):
Tab A drags A→B; another client moves B→C→B; Tab A undoes → stale undo overwrites the newer placement with A.
Anchor: residual/crash-data-loss-corruption
Fix: Revert conditional undo until its precondition includes a monotonic folder-move revision.
[BLOCK-MERGE] 2f5e600
[GPT-REVIEWED] 2f5e600
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 2f5e6002eeefd19bd1553cff810903b10db7f185: <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 2f5e6002eeefd19bd1553cff810903b10db7f185 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

A silent, mis-aimable gesture gains a named destination and an in-flow undo; strings, states, keyboard path, and screenshots all hold up on cold read.

Suggestions

  • SessionMoveUndoBar countdown vs. real deadline: the 8s setTimeout in ChatSidebar is keyed on dragMove?.id (armed at the drop), but the scaleX track animates a full MOVE_UNDO_MS from mount (which waits for server ack via live) — under ack latency the bar vanishes while the track still shows time, so a user aiming at Undo loses it early. Feed the track the remaining time at mount instead of the full duration.
  • When undo's compare-and-set is refused (folder_conflict), undoDragMove clears the bar and the hook lands the session on the server's folder with no word — the user clicked Undo and the session appears in a third place unexplained. Rare, but a one-line status ("Already moved to ") would close the loop.

[UX-REVIEWED] 2f5e600

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 2f5e6002eeefd19bd1553cff810903b10db7f185 — 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.

All evidence gathered — patch read, intent read, repository cross-checked (existing undo/toast mechanisms, the temp-screenshots convention, the drag handlers around moveFolderTo, i18n catalogs, consumer counts for the new API surface). Producing the review.

First-Principles-Verdict: CONCERNS

Session drags get named feedback and undo — earned; but folder drags share the same silent-disappearance cause and stay unfixed, and the server CAS field is permanent surface for a broadcast-lag window.

What this change ships

Intent: after a mis-aimed drag files a session into a folder, the user should see where it went and be able to take it back. ADDITION (titled feat, honestly framed).

  1. A bar below the lanes names the folder a dragged session landed in — justified
  2. An Undo button puts the session back, for 8 seconds — justified
  3. ⌘Z/Ctrl+Z fires undo while the bar is up, standing down in text fields — justified
  4. Undo refuses to overwrite a newer placement (server compare-and-set, 409) — one consumer, thin-window premise
  5. A conditional write to a deleted origin folder now lands unfiled instead of 400 — justified, declared
  6. The "Older Sessions" footer shifts down ~30px while the bar is up — declared cost of the in-flow placement
  7. Three new strings in all 13 catalogs — mandated by the i18n invariant
  8. Capture script + committed screenshots/GIF — matches the temp-screenshots/ convention (191 sibling capture-*.mjs scripts)
  9. Move hook gains expectFolderId/onCommitted options — one consumer each, both this feature
  10. A refused conflict lands the sidebar on the server's folder, not the caller's — justified

Watch

  • Point patch on the root cause "a drag never names its destination": folder re-parent drags stay silent — counted 2 drag-initiated moveFolderTo sites (website/src/pages/ChatSidebar.tsx:2927, :2934; the FolderMoveSubmenu sites name their target and don't count). A dragged folder disappears into a collapsed target exactly like a session. Accepted-and-deferred: the general fix is genuinely larger.
  • expected_folder_id is forever-honored public API whose harm window is another client's move landing before its push_slots_update broadcast arrives — this PR's own live-state retire (ChatSidebar.tsx, the dragMove.live effect) already kills the offer the moment the broadcast lands, so the field only covers the lag itself. Real when a tab's socket lags, but a human should weigh permanent endpoint surface against that window; the smaller shape is deleting expected_folder_id, MoveSlotOptions.expectFolderId, and folderConflict() and letting the client-side retire carry it.

[FIRST-PRINCIPLES-REVIEWED] 2f5e600

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2f5e6002eeefd19bd1553cff810903b10db7f185 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 2f5e600

Verdict parsed from the review's SHA-scoped output markers for commit 2f5e6002eeefd19bd1553cff810903b10db7f185.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 2f5e6002eeefd19bd1553cff810903b10db7f185: <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 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 365994c to a540b23 Compare August 20, 2026 07:31
@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 3 dispositions — head is now a540b23be. One fixed, one rebutted; the rebuttal needs a repository writer's call.

GPT B1 (chat_folders.py:624, missing-target validation masks stale undo conflicts) — FIXED. Correct and worth catching: with the destination validated first, an undo whose origin folder had meanwhile been deleted came back as 400 "folder not found", which says nothing about where the session is — so the client fell back to its stale placement while the server held a newer one. The compare-and-set is now checked before the destination check, exactly as suggested, so that case returns 409 carrying the authoritative folder. New test test_a_stale_expectation_outranks_a_missing_destination, mutation-verified: restoring the old order fails it.

GPT B2 (chat_folders.py:626, folder equality cannot identify the move) — REBUTTED, not fixed. The mechanism is real and I am not disputing it: a folder-value expectation cannot distinguish "still the B I dropped it in" from "moved away and back to B by someone else inside the broadcast window". Identifying a specific move needs a per-slot placement revision (folder_rev bumped on every folder write, persisted, carried in the slots payload, recorded at arm time, compared on undo). I am declining to build that here, for three reasons:

  1. The final state in that scenario is what the user asked for. The sequence is B→C→B elsewhere, and our user then presses Undo on a bar that says "Moved to B". The other client's last intent is B — the same value our expectation names — and our user's explicit, on-screen instruction is "put it back in A". Honouring the local explicit action is defensible; it is not silently losing a placement the user never saw.
  2. The window is a triple coincidence, not a condition that occurs in practice: another client has to move the session out of and back into the same folder inside the ~200ms broadcast window, and our user has to press Undo inside that same window.
  3. The remedy's blast radius exceeds the feature's. A persisted per-session revision field, in the slots payload for every session, to disambiguate an 8-second affordance — on a PR whose reviewers have already (correctly) asked me to delete an unused prop as unearned surface. The same last-write-wins hazard exists today on every folder write in the product: the row menu's "Move to folder…" and drag itself are both unconditional on main. This PR strictly narrows that class; B2 asks it to close a residue the rest of the product does not close.

If a maintainer disagrees, the honest options are (a) accept the residue with the CAS as-is, (b) land the revision counter as a follow-up covering all folder writes rather than just undo, or (c) /ai-review override gpt a540b23be: <reason>. I will not self-authorize the override — that call belongs to a repository writer.

Everything else on the previous head was green: Design ✅, UX ✅, Opus ✅, First Principles 🟡 with only the already-deferred #4626 item, and the Backend Lint red is gone (it was main's black baseline, since fixed; the one real offender it then surfaced — my new test file under the repo-pinned black==26.3.1 — is formatted).

@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 a540b23 to c767508 Compare August 20, 2026 08:05
@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 4 dispositions — head is now c76750830. Both findings fixed, and B2 from round 3 is no longer raised.

BLOCKING (SessionMoveUndoBar.tsx:94, destination disappears at the supported 180px sidebar) — FIXED. Real, and the worst possible thing to lose: SIDEBAR_MIN is 180 and at that width the Moved to prefix plus the button plus the ⌘Z label consume the row, so the folder name — the one thing this bar exists to say — was the first thing to truncate. Below 220px the bar now drops the prefix and the shortcut LABEL, keeping ↳ 🗀 Archive [Undo]; the chord still fires, and the full sentence moves to the row's tooltip. This follows the header's own compactHeader (<256) / tinyHeader (<200) ladder rather than inventing a mechanism.

Evidence and guards, because "it fits now" is exactly the claim a still frame can fake:

  • The harness opens a 180px sidebar (seeded via mc-sidebar-width), asserts the destination is present, asserts the decoration is gone, and measures the folder name's bounding box (44px) so a truncated-to-nothing span cannot pass. Committed as 5-narrow-180.png and embedded in the PR body.
  • Mutation-verified three ways: showing the prefix unconditionally fails the component test, showing the shortcut unconditionally fails it, and hard-coding compact={false} fails the harness at 180px.

FINDING (ChatSidebar.tsx:2838, a remotely deleted origin leaves folders.some(...) stale → dead id → 400 → Undo does nothing) — FIXED, on the server as you suggested. The client-side check cannot be authoritative (its folder list can be a broadcast behind), so the fix belongs where the truth is: a conditional write whose destination no longer exists now lands as unfiled rather than 400. That is the right semantic for a write that RESTORES a placement instead of choosing one, and it is already what the sidebar shows for an unknown folder id. An unconditional write is a live user choice and still gets the 400 — silently redirecting that to unfiled would file a session somewhere the user did not pick. Two new backend cases cover both halves; the client-side degradation stays as a cheap short-circuit when we already know the folder is gone.

Backend suite for this endpoint is now 7 cases, all mutation-verified. Local gates green: tsc -b, eslint 0 errors, i18n:check exit 0, black gate passed (and the whole-file reformat black wanted was reverted — it would have added ~80 lines of unrelated churn and graduated chat_folders.py out of the baseline), flake8/mypy clean on the touched module.

First Principles was ⚠️ could not complete on the previous head — a model-call failure, not a finding; rerunning that lane.

@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 c767508 to 88d0f67 Compare August 20, 2026 08:27
@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
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). 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.

Details worth knowing:
- The offer retires itself when the recorded move stops being the session's
  last one (closed, or moved again from another surface), so Undo can never
  drag a session out of somewhere it was later put on purpose.
- A drop onto the folder a session already lives in arms nothing.
- The countdown is a framer-motion scaleX, not a CSS animation: the global
  prefers-reduced-motion rule clamps CSS animations to 0.01ms, which would
  empty the bar instantly and read as "already expired".
- The chord stands down while focus is in a text field (ChatInput owns its
  own undo history) and ignores ⇧⌘Z.

Tests: 12 component cases (labels, both undo paths, the Mac vs non-Mac
chord, the text-field and redo guards, expiry, unmount cleanup) and 6
sidebar cases (drop arms it, undo posts the ORIGINAL folder, no-op drop,
retirement on close, and the two placement contracts). All mutation-verified.
`website/scripts/capture-session-move-undo.mjs` drives the real built SPA
with real pointer events and asserts the placement geometrically, so the
committed frames cannot go stale silently.

Review round 1: the offer's lifecycle is one-way (a `live` flag set once the
store shows the destination, then dropped and never re-validated) because a
derived-visibility version could revive: drag A→B, move B→C→B from a menu, and
the old A inverse matched again and would have overwritten the newer move. The
8s deadline moved to the sidebar for the same reason — an offer whose optimistic
move never became visible has no bar to run a timer and must still die. The
unused `durationMs` prop is gone; the bar reads MOVE_UNDO_MS directly.

Review round 2: undo is now a COMPARE-AND-SET. Another client can move the
session on inside the 8s window, and its broadcast need not have arrived, so an
unconditional undo could overwrite that newer placement. PATCH
/api/chat/slots/{slot}/folder takes an optional `expected_folder_id`; when it no
longer matches, the server refuses with 409 `folder_conflict` and reports the
authoritative folder, which the move hook applies to the store so the sidebar
shows where the session actually is. The compare and the write are separated by
no `await`, so they are atomic on the loop. Live "Move to folder…" omits the
field and stays unconditional.

Also: undo degrades a `fromFolderId` whose folder was deleted inside the window
to unfiled rather than posting a dead id (Design Review's watch item), and the
GIF's 14 intermediate frames no longer ride along in the screenshot dir — the
harness writes them to the OS temp dir (First Principles' subtraction).

Review round 3: the compare-and-set is checked BEFORE the destination folder is
validated. With the old order, an undo whose origin folder had meanwhile been
deleted was refused as 400 "folder not found" — a refusal that says nothing about
where the session is, so the client fell back to its stale idea of the placement
while the server held a newer one. Answering the staleness question first turns
that case into a 409 carrying the authoritative folder.

Review round 4: two more from GPT.

The bar had a narrow-width defect — at SIDEBAR_MIN (180px) the "Moved to" prefix
plus the Undo control consumed the row and truncated the destination, i.e. the
one thing the bar exists to say was the first thing to go. It now drops the
prefix and the ⌘Z label below 220px, keeping the folder glyph, the name and the
button; the chord still fires and the full sentence moves to the row's tooltip.
Same width ladder as the header's own compactHeader/tinyHeader steps. The
harness proves it at 180px and measures the name's rendered width.

A CONDITIONAL write whose destination folder no longer exists now lands as
unfiled instead of 400. Undo restores a placement rather than choosing one, and
the client's folder list can be a broadcast behind, so it cannot know the origin
was deleted — a refusal left the button doing nothing. An unconditional write is
a live choice and still gets the 400.

Per review of the rendered bar: the button face reads "Undo" and nothing else.
The chord is a power shortcut, not part of the label, so it moved to the button's
tooltip and to aria-keyshortcuts — still discoverable, no longer decoration. The
harness now drives the PRIMARY path by CLICKING the button in a real browser and
asserts both halves of what a click must do (the session leaves the folder AND
the bar goes away), then drags again and undoes from the keyboard to prove the
unlabelled chord still fires.

Review round 5: the offer goes live only once the SERVER has acknowledged the
drag move, not when the optimistic store write lands. Arming on the optimistic
write let the user undo while the original PATCH was still in flight: undo's
compare-and-set would be refused (the server still held the old folder) and the
original write would then land, silently reversing the undo. `useMoveSlotToFolder`
now reports the acknowledgement through an `onCommitted` callback.

Also dropped the client-side deleted-origin check: the server already degrades a
conditional write to a missing folder to unfiled, so the client copy was a second
mechanism doing one job — and the server's is the only one that can see a
deletion this client has not heard about yet.
@dwu96
dwu96 force-pushed the feat/session-move-undo branch from 88d0f67 to 2f5e600 Compare August 20, 2026 08:55
@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 5 dispositions — head is now 2f5e6002e.

GPT BLOCKING (ChatSidebar.tsx:2887, undo activates before the original move is acknowledged) — FIXED. Correct, and a genuinely nastier version of the race than the earlier ones: the offer went live on the OPTIMISTIC store write, so with a slow drag PATCH the user could undo while the original write was still in flight. Undo's compare-and-set would then be refused (the server still held the old folder) and the original PATCH would land afterwards — silently reversing the undo the user had just asked for, which is the one outcome this whole feature exists to prevent.

The offer now goes live only on the server's acknowledgement: useMoveSlotToFolder reports it through a per-call onCommitted callback, and the store-derived "went live" branch is gone. New test offers nothing until the server has acknowledged the move holds the PATCH open, asserts no bar exists while the optimistic write is already visible, then resolves it and asserts the bar appears — mutation-verified (arming with live: true fails it). A second case pins the failure side: a move that never lands offers nothing at all.

While doing this I dropped my own onFailed handler as unobservable: with live-gating there is no path from "failed" back to a visible bar, and I could not write a test that distinguished its presence from its absence. A mechanism I cannot pin is surface I should not ship.

First Principles subtraction (drop the client-side deleted-origin check) — TAKEN. Agreed, and your reasoning is the stronger half: the server already degrades a conditional write to a deleted folder to unfiled, and it is the only one of the two that can see a deletion this client has not heard about yet. The folders.some(...) guard and its frontend test are gone; test_a_conditional_write_to_a_deleted_folder_lands_unfiled is now the single owner of that behaviour.

First Principles Watch items — both stand, deliberately.

  • The ⌘Z chord is a duplicate trigger. True, and kept on the reporting user's explicit instruction: they asked for the shortcut annotation to come off the button face while the binding stayed. Convention rather than constraint is a fair description; it is also the convention the user asked for.
  • expected_folder_id is permanent API surface with one consumer. Accepted as stated — and your own note that "the race it closes is derived, so it stands" is the reason I am not removing it. It exists because rounds 2–5 of this review each surfaced a real way an unconditional undo loses a write.

Design Review was ⚠️ could not complete — a model-call failure, not a finding; rerunning that lane. UX ✅ and Opus ✅ on the previous head.

@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 6 — handing this back rather than iterating again, because GPT has returned to a finding it already made and I already answered.

GPT's only remaining blocking item on 2f5e6002e is the ABA window: another client moves the session out of the destination and back into it inside the 8s undo window, so a value-only compare-and-set still matches and undo overwrites that newer placement. Its remedy: "revert conditional undo until its precondition includes a monotonic folder-move revision."

This is the same finding as round 3's B2, verbatim in mechanism and remedy, and the two demands do not compose: round 2 blocked the merge until undo became conditional; round 6 blocks it until the conditional undo is reverted. Following both literally leaves no undo at all. So the decision is a maintainer's, not mine — I will not self-authorize an override.

The two honest ways forward:

  1. Override (/ai-review override gpt 2f5e6002e...: <reason>). What you would be accepting: a residual window that needs another client to move the session out of and back into the same folder within 8 seconds, and the local user to press Undo inside that same window — in which case the other client's final intent equals the destination our bar names, and the user's explicit on-screen instruction is honoured. For scale: every existing folder write in the product (the row menu, the header dropdown, drag itself on main) is unconditional today, so this PR is already strictly safer than the status quo it changes.
  2. Close it properly with a monotonic folder_rev. Bumped on every folder write, returned in the PATCH response, recorded by the client at arm time (the onCommitted callback added in round 5 is already the right seam), compared as an int instead of a value. Roughly 60–80 lines across state.py (attribute + the persisted-metadata allowlist), chat_folders.py, the api client, useMoveSlotToFolder, ChatSidebar, plus backend and frontend tests. It closes ABA for real — and note it would then be the only folder-write path in the product with that protection, which is either the start of a good invariant or an inconsistency, depending on whether the other paths follow.

Everything else on this head is green: 57 checks pass, 0 fail apart from GPT 5.6 Review and the PR Readiness roll-up that aggregates it. Design ✅ PASS, UX ✅ PASS, Opus 4.8 ✅ no findings, First Principles 🟡 CONCERNS with every item dispositioned (one subtraction taken, two Watch items answered, the moveFolderTo generalisation deferred to #4626).

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

Labels

readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant