Skip to content

fix: read refs inside setTimeout to prevent stale closure in debounced save#623

Open
hobostay wants to merge 1 commit into
heygen-com:mainfrom
hobostay:fix/stale-closure-debounced-save
Open

fix: read refs inside setTimeout to prevent stale closure in debounced save#623
hobostay wants to merge 1 commit into
heygen-com:mainfrom
hobostay:fix/stale-closure-debounced-save

Conversation

@hobostay
Copy link
Copy Markdown
Contributor

@hobostay hobostay commented May 5, 2026

Summary

  • Fix stale closure bug in handleContentChange where the debounced save writes content to the wrong file
  • projectId and filePath were captured from refs at scheduling time; if the user switches files during the 600ms debounce window, the save writes to the previous file

Details

Affected file: packages/studio/src/App.tsx (lines 631-651)

The setTimeout closure captured pid and path from the outer scope at the time the timeout was scheduled. If the user edits file A, then switches to file B within 600ms, the debounced save would write file B's content to file A's path.

The fix moves the ref reads inside the setTimeout callback so they always reflect the current state at execution time.

Test plan

  • Edit file A, quickly switch to file B (< 600ms), verify file A is unchanged and file B has the new content
  • Verify normal editing and saving still works

🤖 Generated with Claude Code

…d save

The debounced save in handleContentChange captured projectId and
filePath from refs at scheduling time (outside setTimeout). If the
user switched files within the 600ms debounce window, the save
would write the new content to the previous file path.

Move the ref reads inside the setTimeout callback so the save
always uses the current project ID and file path at the time
the save actually fires.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

Verified the stale-closure bug at App.tsx:639–650: pid and path are captured at scheduling time, so a file switch inside the 600ms debounce window writes content into the previous file. Reading from projectIdRef/editingPathRef inside the timeout fires fixes it; the early-return guard for missing values is also a correct safety net. LGTM.

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Additive review at 0a36c417@jrusso1020 already verified and approved.

Audited

  • packages/studio/src/App.tsx:631-651 (the debounced save callback)
  • The ref-read sites: projectIdRef.current, editingPathRef.current semantics

Strength

James covered the bug shape. The fix is the right pattern — moving const pid = projectIdRef.current; const path = editingPathRef.current inside the setTimeout callback ensures they're read at execution time, not scheduling time. The early-return guard for missing values is a correct safety net.

Important — Rule 2 contract audit: are there other stale-closure call sites in App.tsx?

This is the kind of bug class where a similar pattern likely exists elsewhere. A quick grep on the same file for setTimeout(.*ref or setTimeout with ref captures would surface neighbors. From a quick scan, I see at least one other debounced path (handleAutosave in the same component, ~line 700-ish based on file size) that should be checked. If those sites read refs in the outer closure too, they have the same race.

Not asking you to fix them in this PR — but worth filing as a follow-up audit. The bug class is identifiable.

Nit

  • A unit test would be nice here — pin the contract with a "switch file mid-debounce, verify the right file was written" test using fake timers. The bug is regression-prone (debounce + refs + async = common forgetting site).

Verdict

Verdict: COMMENT
Reasoning: Fix is correct (James verified the stale-closure shape). External-contributor PR, single-purpose, CI green — Vance check before merging. Recommend a follow-up Rule 2 audit on the rest of App.tsx for sibling stale-ref patterns.

— Vai

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.

3 participants