Skip to content

@remotion/studio: Fix sequence props watcher warning on scrub#6947

Merged
JonnyBurger merged 4 commits intomainfrom
fix/sequence-props-watcher-refcount
Mar 31, 2026
Merged

@remotion/studio: Fix sequence props watcher warning on scrub#6947
JonnyBurger merged 4 commits intomainfrom
fix/sequence-props-watcher-refcount

Conversation

@JonnyBurger
Copy link
Copy Markdown
Member

Summary

  • Reference-count sequence props watchers so that multiple subscribers to the same AST node path (e.g. <Video> inside .map()) share one file watcher, which is only removed when the last subscriber unsubscribes
  • Add cancelled flag to the client-side subscription hook to prevent stale promise resolutions from writing to refs after effect cleanup
  • Add cascading-premount test composition reproducing the scenario from Premounting does not work as indicator would lead you to believe #6727

Closes #6727

Test plan

  • Open cascading-premount composition in Studio with visual mode enabled
  • Scrub through the timeline — no "unsubscribe for sequence props watcher that does not exist" warning in the server console
  • bun run build passes
  • bun run stylecheck passes

🤖 Generated with Claude Code

Multiple timeline items mapping to the same AST node path (e.g. Videos
inside a .map()) caused the server to deduplicate their watchers into
one, but each item still sent its own unsubscribe on unmount. The first
unsubscribe removed the watcher; subsequent ones hit "watcher does not
exist".

Fix: reference-count watchers per key so the file watcher is only
removed when the last subscriber unsubscribes.

Also adds a `cancelled` flag to the client-side subscription hook to
prevent stale promise resolutions from writing to refs after cleanup,
and a test composition reproducing #6727.

Closes #6727

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 31, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
bugs Ready Ready Preview, Comment Mar 31, 2026 1:51pm
remotion Ready Ready Preview, Comment Mar 31, 2026 1:51pm

Request Review

@pullfrog
Copy link
Copy Markdown
Contributor

pullfrog bot commented Mar 31, 2026

Reviewed the refcount and cancellation changes. Submitted one inline comment about a minor session-scoped refCount leak when a subscribe response arrives after effect cleanup — low severity since unsubscribeClientSequencePropsWatchers cleans everything up on disconnect. The latest commit (removing the test composition) introduces no new concerns.

Task list (5/5 completed)
  • Read the diff for all 4 changed files
  • Review sequence-props-watchers.ts refcount logic
  • Review use-sequence-props-subscription.ts cancellation logic
  • Review test composition files
  • Submit review

Pullfrog  | View workflow run | Triggered by Pullfrog | Using Big Pickle (free) | 𝕏

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refcount approach and cancellation flag are both correct. One minor concern about a potential refCount leak when a subscribe response arrives after effect cleanup — detailed in the inline comment. Not blocking since it's bounded by client session lifetime.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Big Pickle (free) | 𝕏

`computeSequencePropsStatusFromContent` throws when the AST node path
is no longer valid (e.g. after formatting moves the JSX element). This
uncaught error inside the file watcher callback crashed the server.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The flag prevents stale writes but leaks a refCount on the server
since the compensating unsubscribe is never sent. The race window is
narrow (localhost RTT) and disconnect cleanup handles it, so remove it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@JonnyBurger JonnyBurger enabled auto-merge (squash) March 31, 2026 13:49
@JonnyBurger JonnyBurger disabled auto-merge March 31, 2026 13:52
@JonnyBurger JonnyBurger enabled auto-merge March 31, 2026 13:52
@JonnyBurger JonnyBurger merged commit 1ed1b77 into main Mar 31, 2026
17 of 18 checks passed
@JonnyBurger JonnyBurger deleted the fix/sequence-props-watcher-refcount branch March 31, 2026 13:58
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.

Premounting does not work as indicator would lead you to believe

1 participant