Skip to content

fix(ui): cleanup leaking setTimeout / WS listeners in effects#19

Open
latteine1217 wants to merge 1 commit into
treeleaves30760:mainfrom
latteine1217:worktree-fix-effect-leaks
Open

fix(ui): cleanup leaking setTimeout / WS listeners in effects#19
latteine1217 wants to merge 1 commit into
treeleaves30760:mainfrom
latteine1217:worktree-fix-effect-leaks

Conversation

@latteine1217
Copy link
Copy Markdown
Collaborator

Summary

Fix the 3 react-doctor/effect-needs-cleanup errors flagged by react-doctoruseEffect hooks that attach subscriptions or timers without returning a cleanup, leaking the registration on every re-run and on unmount.

React-doctor score: 76 → 78 / 100 (3 errors → 0).

Test plan

  • pnpm test — 109 / 109 passing
  • pnpm exec tsc -b --noEmit — clean
  • pnpm build — succeeds
  • npx react-doctor@latest . — 0 errors
  • End-to-end browser test with chrome-devtools MCP:
    • Multi-tab create / switch / close — no console errors
    • Loaded example + Run — execution_start / node_status / execution_error all delivered to correct tab's log
    • DialogContainer open/close cycles — input auto-focus works
    • SubgraphEditor open/close cycles — fitView frames the architecture correctly

🤖 Generated with Claude Code

Three useEffect hooks attached subscriptions or timers without returning a
cleanup, leaking the registration on every re-run and on unmount.

- DialogContainer / SubgraphEditorModal: capture setTimeout id and clear
  it in cleanup.
- useGraphExecution: restructure into a single empty-deps effect that
  subscribes to tabStore directly. Per-tab WS listeners are now attached
  when a tab is created and detached when it is removed, preserving the
  "background tab still receives execution events" design while making
  the cleanup contract explicit.

Verified with vitest (109/109), tsc -b, pnpm build, and react-doctor
(0 errors, score 76 -> 78). End-to-end smoke test in the browser
confirms multi-tab execution, dialog focus, and SubgraphEditor fitView
all still work.

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

Please help me to run the newest test and make sure it passes all tests. Also, next time, if you find any problem, you may first create the issue so I can understand what this branch fixed. Thanks a lot.

@latteine1217
Copy link
Copy Markdown
Collaborator Author

Thanks for the feedback! Two responses:

1. Latest test results on 7ef218d (current HEAD of this branch):

Check Result
pnpm install --frozen-lockfile ✅ no lockfile drift
pnpm exec tsc -b ✅ clean
pnpm test (vitest) 109 / 109 passing
pnpm build ✅ succeeds (built in 6.52s)

The Actions CI is also green (pnpm build + tsc + test, 35s — the earlier red was a queued runner that hit the 15-minute timeout before being assigned, not a real failure).

I also re-ran npx react-doctor@latest . after the fix: score went from 76 → 78 / 100, and the 3 effect-needs-cleanup errors are gone (0 errors).

2. On the "open an issue first" workflow:

Understood — sorry for skipping that. Next time I'll open an issue describing the problem (in this case: react-doctor flagged 3 effect-cleanup leaks — one setTimeout in DialogContainer, one in SubgraphEditorModal, and missing detach on ws.on() subscriptions in useGraphExecution) before sending the PR, so you can see the scope and tradeoffs ahead of time.

For visibility on what this branch actually fixed, here's the short version:

  • DialogContainer.tsx:30 and SubgraphEditorModal.tsx:480 — bare setTimeout without clearTimeout, leaking on every re-run.
  • useGraphExecution.ts — per-tab WebSocket listeners attached via ws.on(...) were never detached. The trade-off was that the original "background tabs keep receiving execution events" design relied on never tearing down listeners, so I couldn't just add cleanup naively. The fix restructures into a single empty-deps effect that subscribes to tabStore directly: listeners attach when a tab is created and detach when it is removed, which preserves the original behaviour (verified end-to-end with multi-tab Run in a browser) while making the cleanup contract explicit and lint-clean.

🤖 Generated with Claude Code

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.

2 participants