fix(sequences): make the timeline playback clock StrictMode-remount safe

[vutuanlinh2k2]

Problem

Opening a sequence crashed the page with `Error: PlaybackClock is disposed` (route error boundary), thrown from `PlaybackClock.seek` during React's passive-effect remount:

Error: PlaybackClock is disposed
  at Object.seek
  at commitHookPassiveMountEffects
  at reconnectPassiveEffects
  at doubleInvokeEffectsOnFiber

`reconnectPassiveEffects` / `doubleInvokeEffectsOnFiber` are React StrictMode's dev-only mount → unmount → remount probe. (React Router's default `entry.client` wraps the app in ``, so this fires for any consumer that opens the timeline editor in dev.)

Root cause

`TimelineEditor` created the `PlaybackClock` in a `useMemo` and disposed it in a separate effect cleanup:

const clock = useMemo(() => createPlaybackClock(...), [fps, durationFrames])
useEffect(() => () => clock.dispose(), [clock])      // disposes on unmount
useEffect(() => { clock.seek(...); return clock.subscribe(...) }, [clock])  // seeks on mount

StrictMode's probe re-runs effects but not render, so `useMemo` is not recomputed between the simulated unmount and remount — the same clock instance is reused. The probe runs all cleanups, then all effect bodies: the dispose cleanup marks the instance `disposed`, then the remounted seek effect calls `clock.seek()` on it → throw.

This is dev/StrictMode-only (a single mount in production never disposes-then-reuses), but it's a genuine lifecycle defect — exactly what StrictMode exists to surface: an imperative resource whose create (`useMemo`) and destroy (effect cleanup) aren't co-located, so it can't survive a remount.

Fix

Move clock creation and disposal into one effect keyed on the engine config (`fps`, `durationFrames`) and expose the instance through state:

• Every (re)mount — StrictMode probe, duration change, or real remount — builds a fresh clock; the effect owns its instance and disposes the same one it created.
• Render-time consumers (preview canvas, transport button, scrub/seek handlers) read the clock from state, so they share the instance the effect drives.
• The prior playhead is restored from a ref across recreation (preserves the existing "duration change resumes from the prior playhead" behavior).

Testing

• Added a regression test that mounts `TimelineEditor` under `` and exercises the transport. It reproduces the throw on the prior code (verified red) and passes with the fix.
• `pnpm typecheck` ✅ · `pnpm test` ✅ (1219 passed) · `pnpm build` ✅

[tangletools]

✅ Auto-approved PR — 4338a24b

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-15T08:05:01Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	2 (2 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	263.0s (2 bridge agents)
Total	263.0s

💰 Value — sound-with-nits

Fixes the React StrictMode dev crash by co-locating PlaybackClock creation and disposal in one effect keyed on engine config, with a regression test; a good, targeted lifecycle fix.

• What it does: Moves PlaybackClock creation out of useMemo and into a useEffect keyed on fps/durationFrames (TimelineEditor.tsx:167-181), stores the resulting instance in React state so render-time consumers share it, and disposes/unsubscribes in the same effect cleanup. It preserves the playhead across clock recreations via a ref (playheadFrameRef at line 162) and adds a StrictMode regression test (timeline-edi
• Goals it achieves: Eliminates the dev-only "PlaybackClock is disposed" crash that occurs when React StrictMode's mount/unmount/remount probe disposes the clock in cleanup and then the remounted seek effect calls seek() on the same disposed instance. It also keeps the existing behavior where a recreated clock (e.g., after a duration change) resumes from the prior playhead.
• Assessment: Good change. The diagnosis is correct: useMemo survives the StrictMode probe while a separate effect cleanup disposes, so co-locating create/dispose in one effect keyed on the engine config is the idiomatic fix. The new test directly reproduces the failure mode and verifies play/pause still works. The code fits the existing React-hooks style of the component. pnpm typecheck passes and the sequence
• Better / existing approach: none — this is the right approach. I searched src/ for reusable disposable-resource hooks and StrictMode-safe lifecycle helpers (no matches except this component's own patterns). The codebase has no existing equivalent to reuse. The only refinement would be a no-op placeholder in the useState initializer instead of a real PlaybackClock, but the author explicitly calls out the placeholder trade-off

🎯 Usefulness — sound

Fixes a real React StrictMode crash in the timeline editor by co-locating PlaybackClock creation and disposal in a config-keyed effect, backed by a regression test.

• Integration: TimelineEditor is the root component of the ./sequences-react subpath (src/sequences-react/index.ts:22, src/sequences-react/components/index.tsx:7, package.json:165-169, tsup.config.ts:31), so the fix is immediately reachable by any product route that mounts the editor; the new test exercises it under StrictMode (tests/sequences/timeline-editor-components.test.ts:457-479).
• Fit with existing patterns: The effect-owned lifecycle matches React's imperative-resource pattern and replaces the anti-pattern that caused the bug: creating the clock in useMemo and disposing it in a separate effect cleanup (src/sequences-react/components/TimelineEditor.tsx:149-181). Other components in the same module already manage resources via effects (PreviewCanvas.tsx:47).
• Real-world viability: Holds up for StrictMode probes, duration changes, and real unmounts: every (re)mount builds a fresh clock, the cleanup unsubscribes and disposes the same instance, and playheadFrameRef preserves playhead position across clock recreations.

💰 Value Audit

🟡 Initial placeholder clock is created but never disposed [maintenance] ``

TimelineEditor.tsx:158-160 creates a real PlaybackClock in the useState initializer. The effect replaces it on mount and the placeholder is never disposed, leaking one lightweight instance per component mount (two in StrictMode because React double-invokes initializers). The comment acknowledges this. A future cleanup could either dispose the replaced state value in the effect or use a no-op sentinel, but this is not a blocker.

🟡 Same StrictMode lifecycle bug remains for the owned VideoFrameProvider [maintenance] ``

TimelineEditor.tsx:183-195 still creates the owned frame provider in useMemo (with a ref cache) and disposes it in a separate effect cleanup. That is the same create-in-render/dispose-in-effect pattern the PR fixes for the clock, so it has the same latent StrictMode-remount defect. It is out of scope for this narrowly-scoped fix but worth a follow-up.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260615T081106Z}

[tangletools]

✅ No Blockers — 4338a24b

Readiness 86/100 · Confidence 70/100 · 4 findings (4 low)

	deepseek	glm	aggregate
Readiness	92	86	86
Confidence	70	70	70
Correctness	92	86	86
Security	92	86	86
Testing	92	86	86
Architecture	92	86	86

Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision.

🟡 LOW Extra synchronous re-render on every mount from setClock inside the effect — src/sequences-react/components/TimelineEditor.tsx

Line 169 calls setClock(next) inside a mount effect, forcing a second commit before first paint resolves with the real clock. During the first commit, PreviewCanvas (line 596) and transport handlers (lines 498-501, 668, 690) close over the placeholder. This is functionally safe — the effect runs before user input and PreviewCanvas re-subscribes on c

🟡 LOW No test for clock recreation on duration/fps change — src/sequences-react/components/TimelineEditor.tsx

The added regression test (tests/sequences/timeline-editor-components.test.ts:457-479) covers StrictMode mount/unmount/remount but not the [fps, durationFrames] dep-change path (lines 167-181). The playhead-restore-via-playheadFrameRef logic (line 170) and the seek-driven isPlaying reset are unverified. A test that re-renders with a changed durationFrames and asserts the playhead is clamped/resumed would close the gap.

🟡 LOW Placeholder clock from useState initializer is never disposed — src/sequences-react/components/TimelineEditor.tsx

Lines 158-160: useState(() => createPlaybackClock(...)) mints a clock that the effect immediately replaces via setClock(next) (line 169). The original useEffect(() => () => clock.dispose(), [clock]) cleanup was removed, so the placeholder is never disposed — it lingers until GC. Impact is negligible (placeholder never plays, loses all subscribers when PreviewCanvas's subscribe-effect cleanup runs on the clock identity change, holds no rAF loop), but it is a resource-lifecycle smell and contradi

🟡 LOW StrictMode remount leaks one orphaned clock per mount cycle — src/sequences-react/components/TimelineEditor.tsx

Lines 158-160: useState initializer creates a placeholder clock. Under React StrictMode's mount/unmount/remount probe, the state initializer runs again during remount, creating a second clock that PreviewCanvas briefly subscribes to (via useEffect([clock, requestPaint]) at PreviewCanvas.tsx:160) before the effect at line 167 replaces it via setClock(next). PreviewCanvas's subscription on the orphaned clock is cleaned up, but the orphaned clock itself is never dispose()-d — its listener

---

_{tangletools · 2026-06-15T08:16:36Z · trace}

[tangletools]

✅ Auto-approved PR — 170fa4ff

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-15T08:22:12Z}

[vutuanlinh2k2]

Validated both value-audit concerns and addressed both:

1. Placeholder clock never disposed — valid, though "leak" overstates it (a never-played clock holds no rAF/listener/DOM resource and is GC-eligible the moment it's replaced). Fixed here in 170fa4f: the driving effect now disposes the clock it replaces via a mirror ref (dispose() is idempotent, so it covers the first-run placeholder and is a no-op for an already-disposed prior clock).

2. Same pattern on the owned VideoFrameProvider — valid as a latent defect, but not crash-equivalent: the provider is only consumed inside PreviewCanvas's async, try/caught paint loop, so a disposed-provider access becomes a swallowed drawError (degraded preview), never a synchronous throw into an effect/error boundary. Fixed as a stacked follow-up in #60, applying the same effect-owned lifecycle plus an ownership guard so caller-supplied providers are never disposed.

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	1 (1 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	433.9s (2 bridge agents)
Total	433.9s

💰 Value — sound

Makes the TimelineEditor playback clock StrictMode-remount safe by moving create/dispose into a single keyed effect, backed by a regression test.

• What it does: Moves PlaybackClock creation out of useMemo and into a useEffect keyed on [fps, timeline.sequence.durationFrames] at src/sequences-react/components/TimelineEditor.tsx:158-187. The instance is exposed through React state (so PreviewCanvas, transport, and scrub handlers share the live object) and tracked with a ref so the effect can dispose the previous instance before replacing it. A StrictMode reg
• Goals it achieves: Stops the dev-only crash "PlaybackClock is disposed" that occurs when React StrictMode mounts, unmounts, and remounts the editor (e.g., under React Router's default wrapper). It also preserves the existing behavior of resuming the playhead from the prior frame when the clock is recreated after a duration/fps config change.
• Assessment: Good change. It fixes a real lifecycle defect that StrictMode correctly surfaces, co-locates resource creation and disposal as React expects, and keeps the rest of the component contract unchanged. The added test directly exercises the failure mode (StrictMode remount + play click). The implementation is consistent with the surrounding component style and the engine/shell split in AGENTS.md — this
• Better / existing approach: none — this is the right approach. I searched src/sequences-react and the broader src tree for reusable StrictMode-safe imperative-resource hooks or utilities; there is no hooks directory and no existing use* helper (Glob src/**/use*.ts returned nothing). The VideoFrameProvider created nearby has a similar useMemo+dispose split, but it does not provide a working reusable pattern to adopt. Extracti

🎯 Usefulness — sound-with-nits

Fixes a real StrictMode lifecycle bug in the timeline editor's playback clock with an idiomatic effect-owned-resource pattern and a regression test that exercises the actual probe.

• Integration: The change is internal to TimelineEditor, which is the public root component of the ./sequences-react subpath: exported at src/sequences-react/components/index.tsx:7, code-split via src/sequences-react/lazy.tsx:10, and wired in package.json:165/tsup.config.ts:31. Every consumer that mounts the editor (or the lazy entry) gets the StrictMode-safe clock. The new test renders inside `<Stri
• Fit with existing patterns: Co-locating createPlaybackClock and dispose in a single effect keyed on fps/durationFrames is the standard React pattern for imperative disposable resources. It matches how the component already owns other engine objects (createCommandStack, createZoomMath) and preserves the existing PlaybackClock contract without adding new API surface. It does not duplicate or compete with any exis
• Real-world viability: The fix holds under the StrictMode mount/unmount/remount probe because every mount builds a fresh clock and the cleanup disposes the exact instance that effect created; dispose is idempotent (src/sequences-react/engine/playback.ts:141-146), and the playhead is carried across duration changes via a ref. The one adjacent rough edge is the default VideoFrameProvider right below it (`TimelineEdi

🎯 Usefulness Audit

🟡 Default VideoFrameProvider has the same StrictMode lifecycle defect [robustness] ``

The owned VideoFrameProvider is created in useMemo and disposed in a separate unmount cleanup (TimelineEditor.tsx:189-201). Under React StrictMode the same instance can be reused across the mount/unmount/remount probe, leaving it disposed when PreviewCanvas tries to draw. The draw path catches the error, so it does not crash the route like the clock did, but it is the same class of lifecycle bug and should be fixed with the same effect-owned pattern.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260615T083106Z}

[tangletools]

✅ No Blockers — 170fa4ff

Readiness 89/100 · Confidence 70/100 · 3 findings (3 low)

	deepseek	glm	aggregate
Readiness	92	89	89
Confidence	70	70	70
Correctness	92	89	89
Security	92	89	89
Testing	92	89	89
Architecture	92	89	89

Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision.

🟡 LOW Duration-change clock recreation path is untested — src/sequences-react/components/TimelineEditor.tsx

The effect at line 168 handles recreation when fps or durationFrames changes (clock disposed, new clock created, playhead resumed from playheadFrameRef). The test suite covers the StrictMode remount path (line 457) but has no test exercising a duration change mid-session — e.g., a host extending sequence length and verifying the playhead survives the clock swap. Impact: the resume-from-prior-playhead logic ([line 176](https://github.com/tangle-network/agent-app/blob/170fa4ff0fb4c68d59d575ec21ecc9435

🟡 LOW One-render stale clock config on simultaneous fps+durationFrames change — src/sequences-react/components/TimelineEditor.tsx

Lines 133,139,168-187: When props.timeline changes, fps (destructured from props on L133) captures the new value, but timeline.sequence.durationFrames (from useSyncExternalStore on L139) reflects the pre-reset state because the reset effect (L143-147) hasn't yet flushed the new timeline into the command stack. The clock effect (L168-187) runs with new fps + old durationFrames, producing a clock with mismatched config for one render. The effect re-runs with correct values on the next render after setClock(next) triggers a state update. No data loss — the stale clock is disposed and replaced immediately. Purely transient visual stat

🟡 LOW Assertion uses toBeTruthy() instead of a DOM-specific matcher — tests/sequences/timeline-editor-components.test.ts

Line 478: expect(screen.getByLabelText('Pause')).toBeTruthy() only proves the query returned a non-null node; getByLabelText already throws on miss, so the assertion is somewhat redundant. toBeInTheDocument() (from @testing-library/jest-dom) would express the intent more precisely and surface attach-state bugs. Not blocking: every other transport assertion in this describe block (lines 313-315) uses the same toBeTruthy() pattern, so the new test is consistent with local conventions. No correc

---

_{tangletools · 2026-06-15T08:32:46Z · trace}

[vutuanlinh2k2]

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 433.9s (2 bridge agents)
Total 433.9s

💰 Value — sound

Makes the TimelineEditor playback clock StrictMode-remount safe by moving create/dispose into a single keyed effect, backed by a regression test.

• What it does: Moves PlaybackClock creation out of useMemo and into a useEffect keyed on [fps, timeline.sequence.durationFrames] at src/sequences-react/components/TimelineEditor.tsx:158-187. The instance is exposed through React state (so PreviewCanvas, transport, and scrub handlers share the live object) and tracked with a ref so the effect can dispose the previous instance before replacing it. A StrictMode reg
• Goals it achieves: Stops the dev-only crash "PlaybackClock is disposed" that occurs when React StrictMode mounts, unmounts, and remounts the editor (e.g., under React Router's default wrapper). It also preserves the existing behavior of resuming the playhead from the prior frame when the clock is recreated after a duration/fps config change.
• Assessment: Good change. It fixes a real lifecycle defect that StrictMode correctly surfaces, co-locates resource creation and disposal as React expects, and keeps the rest of the component contract unchanged. The added test directly exercises the failure mode (StrictMode remount + play click). The implementation is consistent with the surrounding component style and the engine/shell split in AGENTS.md — this
• Better / existing approach: none — this is the right approach. I searched src/sequences-react and the broader src tree for reusable StrictMode-safe imperative-resource hooks or utilities; there is no hooks directory and no existing use* helper (Glob src/**/use*.ts returned nothing). The VideoFrameProvider created nearby has a similar useMemo+dispose split, but it does not provide a working reusable pattern to adopt. Extracti

🎯 Usefulness — sound-with-nits

Fixes a real StrictMode lifecycle bug in the timeline editor's playback clock with an idiomatic effect-owned-resource pattern and a regression test that exercises the actual probe.

• Integration: The change is internal to TimelineEditor, which is the public root component of the ./sequences-react subpath: exported at src/sequences-react/components/index.tsx:7, code-split via src/sequences-react/lazy.tsx:10, and wired in package.json:165/tsup.config.ts:31. Every consumer that mounts the editor (or the lazy entry) gets the StrictMode-safe clock. The new test renders inside `<Stri
• Fit with existing patterns: Co-locating createPlaybackClock and dispose in a single effect keyed on fps/durationFrames is the standard React pattern for imperative disposable resources. It matches how the component already owns other engine objects (createCommandStack, createZoomMath) and preserves the existing PlaybackClock contract without adding new API surface. It does not duplicate or compete with any exis
• Real-world viability: The fix holds under the StrictMode mount/unmount/remount probe because every mount builds a fresh clock and the cleanup disposes the exact instance that effect created; dispose is idempotent (src/sequences-react/engine/playback.ts:141-146), and the playhead is carried across duration changes via a ref. The one adjacent rough edge is the default VideoFrameProvider right below it (`TimelineEdi

🎯 Usefulness Audit

🟡 Default VideoFrameProvider has the same StrictMode lifecycle defect [robustness] ``

The owned VideoFrameProvider is created in useMemo and disposed in a separate unmount cleanup (TimelineEditor.tsx:189-201). Under React StrictMode the same instance can be reused across the mount/unmount/remount probe, leaving it disposed when PreviewCanvas tries to draw. The draw path catches the error, so it does not crash the route like the clock did, but it is the same class of lifecycle bug and should be fixed with the same effect-owned pattern.

What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?
Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260615T083106Z

Fixed in #60