refactor(studio): isolate clip drag lifecycle - #2698
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
259b0f1 to
bad5a54
Compare
c54611f to
5e02427
Compare
bad5a54 to
ac1bbdd
Compare
5e02427 to
c327ee5
Compare
ac1bbdd to
3e11275
Compare
c327ee5 to
d9d7106
Compare
3e11275 to
81e3a70
Compare
d9d7106 to
a224fdc
Compare
81e3a70 to
dcbe193
Compare
a224fdc to
a289615
Compare
dcbe193 to
f869766
Compare
a289615 to
da1c1be
Compare
f869766 to
3467adf
Compare
da1c1be to
2913f87
Compare
52f5acf to
b4d9414
Compare
9eaec45 to
c3b04be
Compare
b4d9414 to
4ddc5bd
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade A — rubric CORRECT
Role in stack: D3 refactor — clip-drag pointer-session lifecycle extracted to a mount-only helper so D5-D8 can virtualize rows without shredding an in-flight drag.
Root-cause fit: The extracted mountTimelineClipDragGestureLifecycle attaches pointermove/pointerup/pointercancel/keydown to window (never to a row DOM node), and every piece of live state is read through refs. Callsite is useMountEffect, the sanctioned single-effect wrapper. Net: a row scroll-out cannot detach listeners or nullify state — the survival property D5-D8 leans on.
Claims verified (file:line at HEAD 4ddc5bd6):
- Extraction only, no semantic drift:
useTimelineClipDrag.ts:1-6,29deletes localresolveTimelineDragEscapeimport + inline handlers, delegates at:271-296. - Window-scoped listeners with symmetric cleanup incl.
capture:trueon keydown:timelineClipDragGestureLifecycle.ts:236-249. - Terminal cleanup null-first-then-commit blocks double-commit on duplicate up events:
commitDragPointerUp(:191-206),commitResizePointerUp(:136-172) — ref cleared BEFORE commit; a second up seesnull. - Escape resets all four refs + group session in one pass:
handleWindowKeyDown:213-232. - File-list/additions match PR envelope (+287/-216 → +259 new file, +28/-216 modified).
Adversarial findings:
- P2 (note) — Non-ref params captured at mount:
updateElement(Zustand, stable),restoreGroupResizeMembers(useCallback stable), React setters (stable).setShowPopoveris a prop; if any parent supplies an inline function, the mount closure holds the first-render one for the hook's lifetime. Grep the caller before D5 lands to confirm stabilization. - P2 (note) — PR body checks "Unit tests added/updated" but diff has zero test files. Behavior is exercised by the existing Timeline interaction suite, but the row-unmount-mid-drag guarantee itself — D3's core contribution — has no explicit test.
CI state: All required checks green.
Suggested next step: Land as-is. Before D5 merges, add one focused unit test that mounts the hook, starts a drag, unmounts+remounts the row's pointerdown wiring, moves the pointer, and asserts the drag preview updates + commits — that's the invariant this PR sets up and D5-D8 will silently rely on.
— Review by Via
4ddc5bd to
6958e61
Compare
c3b04be to
287a998
Compare
|
Addressed at
|
|
Reviewed the exact #2698 delta (287a998..6958e61), not the rest of the stack.\n\nThe extraction preserves the prior gesture semantics while moving pointermove/up/cancel and Escape handling to window scope. Listener installation/removal is symmetric, capture options match, autoscroll is stopped on terminal paths, refs are cleared before commits, and the sole caller supplies Reacts stable state setter. The new lifecycle test now removes the source row mid-gesture and proves the window-scoped move/up path still previews and commits exactly once.\n\nOne non-blocking lifecycle note: clearSuppressedClick still schedules an untracked requestAnimationFrame, so dispose cannot cancel that callback. This behavior predates the extraction, but tracking/canceling it would make the helper fully self-contained under StrictMode/remount churn.\n\nCurrent exact-head lightweight checks are green/skipped as expected; Graphite mergeability remains pending because #2697 is downstack. This PR must not land until #2697s blocker is resolved, but I found no blocker in this delta.\n\nGitHub will not let the shared author identity submit a formal approval, so recording the verdict as a PR comment.\n\nVerdict: APPROVE\nReasoning: The clip-drag lifecycle extraction is behavior-preserving, cleans up its owned global listeners, and now covers row unmount during an active gesture.\n\n— Magi |
6958e61 to
9b9e15e
Compare
abb921f to
a870cf4
Compare
8c1dd61 to
dd56d5b
Compare
0011368 to
4aaf015
Compare
dd56d5b to
6b92049
Compare

What
Extract clip-drag pointer-session lifecycle handling into one focused helper without changing drag semantics.
Why
Virtualized rows can unmount during a pointer session. The drag lifecycle needs one stable actor that owns capture, movement, cancellation, and exactly-once terminal cleanup before rendering becomes windowed.
How
This is D3 of the Family D draft review sequence. It targets current main after Family C landed; its Family D patch was replayed without semantic changes. It supersedes legacy PR #2634, which remains open for audit until landing.
Test plan
Validated on the exact Family D tip through the focused Timeline interaction suite, full Studio suite, typechecks, formatting, lint, file-size checks, and the strict 50,000-element Chromium gate.
2026-07-29 rebase verification
No merge was performed; this PR remains a draft.