Skip to content

Fix reactive core effect lifecycle: track signals, restore tracking context - #131

Open
nakasyou wants to merge 1 commit into
mainfrom
devin/1790237723-reactive-effect-lifecycle
Open

nakasyou wants to merge 1 commit into
mainfrom
devin/1790237723-reactive-effect-lifecycle

Conversation

@nakasyou

Copy link
Copy Markdown
Member

Summary

Fixes the standalone effect/signal in core/reactive/core.ts (exported via eclipsa/client), which had a broken lifecycle:

  • signal.get() added the effect to signal.effects but never added the signal to effect.signals, so the FinalizationRegistry cleanup that iterates effect.signals was a permanent no-op → effects were never unsubscribed.
  • effect() set currentEffect without try/finally: a throwing body left tracking dangling (next unrelated get() subscribed the dead effect), and successful runs reset to null instead of the previous effect, breaking nested effect() inside an outer effect.
  • The registry target was fn, but the held value (newEffect) references fn, so fn could never be collected and cleanup could never fire.

Changes:

get() { if (currentEffect) { this.effects.add(currentEffect); currentEffect.signals.add(this) } }

effect(fn): () => void {
  const previous = currentEffect
  currentEffect = newEffect
  try { newEffect.fn() } finally { currentEffect = previous }
  const dispose = () => { registry.unregister(dispose); unsubscribeEffect(newEffect) }
  registry.register(dispose, newEffect, dispose)   // token is the dispose handle, not fn
  return dispose
}

effect now returns a dispose function for explicit unsubscription; GC-based cleanup runs when the caller drops the handle.

Adds unit tests for dispose/unsubscribe, throwing bodies, and nested effects.

Link to Devin session: https://app.devin.ai/sessions/50e38e97b4534695ba2c96c67ba6c585
Open in Devin Desktop: https://app.devin.ai/desktop/session/50e38e97b4534695ba2c96c67ba6c585?variant=devin
Requested by: @nakasyou

Co-Authored-By: Shotaro Nakamura <syoutarou1103@outlook.jp>
Copilot AI lite review requested due to automatic review settings September 24, 2026 08:15
@nakasyou nakasyou self-assigned this Sep 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

Website preview: https://pr-131-eclipsa.xiarenda61.workers.dev

Commit: 8757e5b

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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