refactor(sidebar): extract the move-undo primitive behind one hook

[iamwhatever]

Why no screenshot: pure refactor with no rendered delta -- the bar's markup, Tailwind classes, i18n keys and data-testids are byte-for-byte unchanged; only the module it lives in and which hook owns its state moved.

Stage 1 of #4626. A pure refactor: zero observable behaviour change, zero new surface armed.

Why extract before arming anything

The drag-move undo that landed in #4617 works for session drags only. Its substance is not the bar — it is a lifecycle that is invisible on a happy-path read:

• an offer stays PENDING until the server acknowledges, because the move is optimistic and an offer armed on the store would let undo fire while the original PATCH is still in flight (the original write then lands and silently reverses the undo);
• it latches superseded when a third party's placement arrives inside that window, because by ack time live state may match the destination again (a move away and back) and nothing later could tell;
• once LIVE it is dropped, never re-validated — deriving visibility from live state let a retired offer come back and overwrite a newer, intentional move.

Stage 2 arms artifacts and folder-into-folder moves. moveFolderTo moves a whole subtree, so it needs exactly these staleness guards — which is why the mechanism is extracted rather than copied. A second hand-rolled copy would get one of the three subtly wrong.

What changed

• website/src/hooks/useMoveUndo.ts (new) owns the offer state machine, the MOVE_UNDO_MS deadline, and the hover hold that suspends it. Callers inject only the surface-specific parts:
  • locate(itemKey) — where the item sits now; undefined = gone (retire), null = unfiled root
  • apply(itemKey, folderId, { onCommitted }) — the optimistic move, acknowledging on server success
  • folderExists(id) — so undo degrades a deleted origin to unfiled instead of posting a 400
• SessionMoveUndoBar → MoveUndoBar, MovedSession → MovedItem, and its slotKey / sessionTitle fields → itemKey / itemTitle. Those two names would be lies the moment an artifact or a folder subtree arms the same offer.
• ChatSidebar is the SOLE caller. ~120 lines of offer machinery collapse to three memoised deps plus a descriptor built at the drag site.

MOVE_UNDO_MS and the one-way live / superseded semantics are preserved exactly, including two details a re-implementation would drop:

• the deadline effect's deps stay pinned to [offer?.id, paused] — flipping live, or writing the frozen remainder, must not restart the clock;
• onHoldChange captures the id that was current when it was handed to a bar, so a bar retiring through its 150ms AnimatePresence exit cannot freeze a newer offer's clock.

Deliberately NOT renamed

The components.sessionMoveUndoBar.* i18n keys and the session-move-undo* test ids keep their names. The strings are translated in every locale catalogue, and the ids are what website/scripts/capture-session-move-undo.mjs selects on — renaming them would be churn plus breakage for no behavioural gain.

Proof of no behaviour change

The existing tests pass with their assertions untouched — only the module path, the component identifier, and the two renamed field names change:

• website/src/test/MoveUndoBar.test.tsx (renamed from SessionMoveUndoBar.test.tsx) — 15 tests
• website/src/test/ChatSidebar.moveUndo.test.tsx — 15 tests
• website/src/test/ChatSidebar.moveUndoStale.test.tsx — 2 tests

Testing

• Targeted: 32/32 green across the three suites above
• Full frontend suite: 1665 passed / 1666 files, 26,340 tests passed
  • one pre-existing failure, src/i18n/productName.test.ts, is main-inherited — it flags product-name strings in pullRequestPanel.owner_not_configured_guidance and sttSettings.the_bundled_audio_decoder_is_missing_or_damaged, fails identically on pristine d7b7d65c3, and this diff touches no locale file
• npm run typecheck clean, npm run lint 0 errors

Out of scope (stage 2, separate PR against the same issue)

• ArtifactsPage.tsx handleDragEnd — moveArtifact and folder nesting via updateFolderMut, both currently silent
• moveFolderTo in the sidebar and its drag routes + menu picks

no linked issue: intentional. This is stage 1 of two, and the tracked issue
(#4626) must stay open until stage 2 arms artifacts drags and
folder-into-folder moves, which is what its report actually asks for. The closing
trailer therefore belongs on the stage 2 PR, not this one.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A genuinely subtle lifecycle extracted behind a minimal three-function seam, with a concrete named second caller — proportionate, faithful, and reversible.

Suggestions

• Dependency arrow is inverted: the reusable primitive (useMoveUndo) imports MOVE_UNDO_MS and MovedItem from the presentation component it exists to feed (import { MOVE_UNDO_MS, type MovedItem } from '../components/MoveUndoBar'). Since the hook owns the deadline ("The DEADLINE is enforced by the owner of the offer"), declare the constant and the type in the hook and have the bar import them — otherwise every future headless consumer of the offer state machine drags in a framer-motion component module.

[DESIGN-REVIEWED] ba45913

[github-actions]

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

Pure refactor: the bar's markup, strings, i18n keys, timing, and undo lifecycle are byte-identical; nothing a user sees or does changes.

Verified against the diff: the only user-facing string touchpoints are sessionTitle → itemTitle renames feeding the same tooltip interpolation; the 8s deadline, hover hold, pending-until-ack gating, and same-folder no-op drop all survive the extraction with identical semantics, and the test diffs contain renames only.

[UX-REVIEWED] ba45913

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ba45913

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] ba45913

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ba45913b82bc048fea791663af6d29e76c7c5192 — 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 counts are run. Composing the final review.

First-Principles-Verdict: CONCERNS

The extraction is faithful and well-argued, but the generalized hook ships with exactly one consumer — its entire justification is a stage 2 that hasn't landed.

What this change ships

Intent: consolidate the drag-move undo lifecycle into one reusable primitive so artifacts and folder moves can arm it next — an ADDITION of internal surface with zero claimed behavior change; description and diff agree.

1. Nothing a user can see or do changes — markup, i18n keys, test ids, timing all retained — justified
2. New public hook useMoveUndo any component may call — one consumer, generalized
3. SessionMoveUndoBar renamed MoveUndoBar; MovedSession/slotKey/sessionTitle renamed MovedItem/itemKey/itemTitle — declared, same justification as item 2
4. Three new exported types (MoveUndoOffer, MoveUndoDeps, MoveUndoController) — zero consumers
5. ~120 lines of inline offer machinery deleted from ChatSidebar — justified

Watch

• The locate/apply/folderExists deps contract is a prediction with zero second callers to check it against: apply's (key, folderId, {onCommitted}) shape is exactly useMoveSlotToFolder's signature, while the named stage-2 surfaces use different shapes (updateFolderMut.mutate({id, body}) at ArtifactsPage.tsx:1570; moveFolderTo(folderId, parentId) at ChatSidebar.tsx:3983 — grepped, 2 unarmed drag surfaces). If stage 2 reshapes the deps, this PR's contract gets rewritten once more.
• No duplicate exists: grep for undo primitives under website/src/hooks and components finds no prior mechanism — this is the first spelling, not a second.

Subtractions

• Defer useMoveUndo.ts into the stage-2 PR that adds its second caller, so the injected-deps contract is validated by a real consumer instead of a predicted one; the renames could still land alone.
• Unexport MoveUndoOffer, MoveUndoDeps, MoveUndoController — 0 consumers outside useMoveUndo.ts (grepped MoveUndo(Offer|Deps|Controller) across website/src: only the defining file matches).

[FIRST-PRINCIPLES-REVIEWED] ba45913

[chenmingwei23]

Tier 1 auto-approve: refactor (6 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: code-move refactor, extract move-undo primitive behind one hook, no behaviour change.

[bolichen97]

Tier 1 auto-approve: refactor (6 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: extracts the sidebar move-undo lifecycle behind a single reusable useMoveUndo hook (code-move, no behaviour change).

[dwu96]

Tier 1 auto-approve: refactor (6 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: code-move only -- the drag-move undo lifecycle is lifted verbatim out of ChatSidebar into website/src/hooks/useMoveUndo.ts and the bar is renamed session-agnostic (MovedSession -> MovedItem); i18n keys and test ids deliberately unchanged, existing suites updated for the rename only.

[iamwhatever]

Dispositions — First Principles Review 🟡 CONCERNS

Answering both subtractions for the record, since this landed before the verdict was dispositioned. Both are advisory; neither blocked readiness.

Subtraction 1 — "defer useMoveUndo.ts into the stage-2 PR" → rebutted (disproportional)

The observation holds: the hook shipped with one consumer, and the locate / apply / folderExists contract had no second caller to validate it against. The proposed remedy is what I disagree with, on two grounds.

The staging is a deliberate maintainer decision, not an oversight. Stage 1 was scoped as extract the primitive and rewire exactly one existing caller, stop there, precisely so the mechanism lands reviewable in isolation. Deferring the hook empties stage 1 of its content and leaves only renames — which reverses the split rather than improving it.

The "contract gets rewritten" risk does not survive contact with the two named sites. The review is right that their shapes differ from apply(key, folderId, {onCommitted}):

• moveFolderTo(folderId, parentId) → updateFolderMutation.mutate({ id, body: { parent_id } }) (ChatSidebar.tsx)
• updateFolderMut.mutate({ id, body: { parent_id: target } }) (ArtifactsPage.tsx)

But apply is an adapter seam, not a signature a caller must already match. Each site conforms in a lambda, with no change to the hook:

apply: useCallback((id, parentId, opts) => {
  updateFolderMut.mutate(
    { id, body: { parent_id: parentId ?? '' } },
    { onSuccess: () => opts?.onCommitted?.() },
  )
}, [updateFolderMut])

That the ack channel is available at both sites is the load-bearing part, and it is: these are react-query mutations, whose per-call onSuccess is exactly the mechanism useMoveSlotToFolder already uses to implement onCommitted today. So the deps are a minimal semantic vocabulary — where is it / move it / does the folder still exist — not a prediction about call signatures.

Subtraction 2 — "unexport MoveUndoOffer, MoveUndoDeps, MoveUndoController" → accepted-and-deferred

Legitimate and correct. I verified the grep independently: those three have zero consumers outside website/src/hooks/useMoveUndo.ts, while MovedItem has three real ones (the bar's Props, the hook's arm(), and the bar's test fixture) and rightly stays exported. Exporting the other three was surface predicted for stage 2 rather than surface anything uses.

I prepared and verified the change (drop the three export keywords; tsc -b clean, 32/32 undo tests green), but this PR merged before it could be pushed. Rather than spend a full 67-check round on a three-token type-visibility edit, it is folded into stage 2, which touches this exact file to add the second caller — and which is then the thing that decides, from a real consumer, which of the three deserves to be exported at all. That is the same reasoning the subtraction argues for, applied to its own timing.

Note on the issue link

#4626 stays open deliberately. The tracked report is that artifacts drags and folder-into-folder moves are still silent, and only stage 2 answers it; this PR carried no closing trailer for that reason.

[bolichen97]

Tier 1 auto-approve: refactor (6 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: code-move only -- extracts the sidebar drag-move undo state into useMoveUndo.ts with markup, Tailwind classes, i18n keys and data-testids byte-for-byte unchanged per the PR's no-visual-delta declaration; tests updated to the new module boundary. [Note: the auto-merge arm is denied by this cron agent's permission posture; merge deferred to a human maintainer.]