fix(core): prevent duplicate setInterval in dispatchEvent#683
Merged
ENvironmentSet merged 2 commits intomainfrom Mar 12, 2026
Merged
fix(core): prevent duplicate setInterval in dispatchEvent#683ENvironmentSet merged 2 commits intomainfrom
ENvironmentSet merged 2 commits intomainfrom
Conversation
When dispatchEvent was called rapidly (e.g., push → push → pop), each call created a new setInterval without clearing the previous one. This caused N concurrent intervals running aggregate + isEqual every 16ms, linearly increasing CPU load during transitions. Fix by adding an `intervalRunning` flag so only one interval exists at a time. Since all intervals read from the same `events.value`, a single interval handles all queued events correctly. Resolves FEP-1958 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: e723f02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a concurrency guard for the dispatch interval by tracking the active interval in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
Deploying stackflow-demo with
|
| Latest commit: |
e723f02
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://22a3c921.stackflow-demo.pages.dev |
| Branch Preview URL: | https://analyze-corestore.stackflow-demo.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | e723f02 | Commit Preview URL | Mar 12 2026, 07:12 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/makeCoreStore.ts (1)
109-118:⚠️ Potential issue | 🟠 MajorUse the latest stack state before clearing the only interval.
nextStackValueis computed beforesetStackValue(), butsetStackValue()can synchronously run post-effect hooks that calldispatchEvent()again. With this new guard, that nested dispatch will reuse the current timer instead of starting a new one. If the oldnextStackValuewas"idle", Line 116 can still clear that timer even thoughstack.valuehas already moved back to a non-idle transition, leaving the store stuck until another external event arrives.Suggested fix
const interval = setInterval(() => { const nextStackValue = aggregate(events.value, new Date().getTime()); if (!isEqual(stack.value, nextStackValue)) { setStackValue(nextStackValue); } - if (nextStackValue.globalTransitionState === "idle") { + if (stack.value.globalTransitionState === "idle") { clearInterval(interval); intervalRunning = false; } }, INTERVAL_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/makeCoreStore.ts` around lines 109 - 118, The interval callback computes nextStackValue then calls setStackValue and immediately clears the timer if nextStackValue.globalTransitionState === "idle", but setStackValue can synchronously trigger nested dispatches that change stack.value; update the guard to read the latest stack state after calling setStackValue (e.g., examine stack.value or a fresh value derived after setStackValue) before calling clearInterval(interval) and flipping intervalRunning, so you only clear the timer when the current stack.globalTransitionState is still "idle"; references: aggregate, events.value, setStackValue, stack.value, nextStackValue, interval, intervalRunning, dispatchEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/src/makeCoreStore.ts`:
- Around line 109-118: The interval callback computes nextStackValue then calls
setStackValue and immediately clears the timer if
nextStackValue.globalTransitionState === "idle", but setStackValue can
synchronously trigger nested dispatches that change stack.value; update the
guard to read the latest stack state after calling setStackValue (e.g., examine
stack.value or a fresh value derived after setStackValue) before calling
clearInterval(interval) and flipping intervalRunning, so you only clear the
timer when the current stack.globalTransitionState is still "idle"; references:
aggregate, events.value, setStackValue, stack.value, nextStackValue, interval,
intervalRunning, dispatchEvent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eae45893-bf6b-44fc-bc31-6a2882de1f22
📒 Files selected for processing (1)
core/src/makeCoreStore.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
orionmiz
requested changes
Mar 12, 2026
Collaborator
There was a problem hiding this comment.
Review: Request Changes
The problem being solved is real
Duplicate setInterval accumulation on rapid successive dispatchEvent calls is a valid performance issue.
However, the intervalRunning flag introduces a regression
onChanged is the one post-effect hook that fires from inside the interval callback (via setStackValue → triggerPostEffectHooks). When a plugin dispatches an event (e.g. pop()) from onChanged, the following occurs:
1. Interval tick → aggregate() detects idle → setStackValue()
2. → triggerPostEffectHooks() → onChanged()
3. → Plugin calls pop() → dispatchEvent("Popped")
4. → Event pushed + setStackValue() executes normally
5. → intervalRunning === true → early return (no new interval created)
6. Control returns to original interval tick → stale nextStackValue is still "idle"
7. → clearInterval() + intervalRunning = false
8. No interval exists to poll the pop transition → globalTransitionState permanently stuck in "loading"
Impact
usePreventTouchDuringTransition: Touch events arepreventDefault-ed whileglobalTransitionState !== "idle"→ touch input permanently blocked on mobileSerialNavigationProcess:captureNavigationOpportunityreturns[]when not idle → history navigation queue blockedbasicUIPlugin:computedTransitionDurationstays at full duration instead of0ms→ CSS transition timing errors
Reproduction
Unit test (add to makeCoreStore.spec.ts):
test("dispatchEvent from onChanged (interval reentry) must reach idle", async () => {
let popTriggered = false;
const { actions } = makeCoreStore({
initialEvents: [
makeEvent("Initialized", { transitionDuration: 150, eventDate: enoughPastTime() }),
makeEvent("ActivityRegistered", { activityName: "hello", eventDate: enoughPastTime() }),
makeEvent("Pushed", { activityId: "a1", activityName: "hello", activityParams: {}, eventDate: enoughPastTime() }),
],
plugins: [
() => ({
key: "reentrancy-test",
onChanged({ actions: { getStack, pop } }) {
const stack = getStack();
if (!popTriggered && stack.activities.length === 2 && stack.globalTransitionState === "idle") {
popTriggered = true;
pop();
}
},
}),
],
});
actions.push({ activityId: "a2", activityName: "hello", activityParams: {} });
await new Promise((res) => setTimeout(res, 600));
expect(popTriggered).toBe(true);
expect(actions.getStack().globalTransitionState).toBe("idle"); // FAIL: received "loading"
});Visual reproduction in demo (step by step):
- Add this plugin to
demo/src/stackflow/Stack.tsxas the first plugin:
(() => {
let popTriggered = false;
return () => ({
key: "reentrancy-bug-repro",
onChanged({ actions: { getStack, pop } }) {
const stack = getStack();
(window as any).__state = stack.globalTransitionState;
if (
!popTriggered &&
stack.activities.length === 2 &&
stack.globalTransitionState === "idle"
) {
popTriggered = true;
console.log("[bug-repro] push idle detected → calling pop()");
pop();
const monitor = setInterval(() => {
const s = getStack().globalTransitionState;
console.log(`[monitor] globalTransitionState: ${s}`);
if (s === "idle") {
console.log("OK: recovered to idle");
clearInterval(monitor);
}
}, 16);
}
},
});
})(),- Run
yarn dev - Open the demo in a browser and open DevTools console
- Click any feed card
- Observe the console:
[monitor] globalTransitionState: loadingrepeats indefinitely — it never reaches"idle" - Type
window.__statein the console to confirm it is"loading"
Suggested fix: local interval reference with guard
let currentInterval: ReturnType<typeof setInterval> | null = null;
dispatchEvent(name, params) {
// ...
events.value.push(newEvent);
setStackValue(nextStackValue);
if (currentInterval !== null) {
clearInterval(currentInterval);
}
const interval = setInterval(() => {
const nextStackValue = aggregate(events.value, new Date().getTime());
if (!isEqual(stack.value, nextStackValue)) {
setStackValue(nextStackValue);
}
if (nextStackValue.globalTransitionState === "idle") {
clearInterval(interval);
if (currentInterval === interval) {
currentInterval = null;
}
}
}, INTERVAL_MS);
currentInterval = interval;
},Key points:
- Prevents interval accumulation: clears previous interval before creating a new one
clearInterval(interval)uses a local variable — only clears itself, never kills a newer interval created by a reentrant callcurrentInterval === intervalguard — if reentrancy already replacedcurrentInterval, don't null it out
The intervalRunning flag caused a regression when onChanged hooks triggered reentrant dispatchEvent calls: the nested dispatch would skip interval creation, but the original interval would clear itself on stale "idle" state, leaving no interval to poll the new transition. Switch to clearInterval(currentInterval) + create new approach, which safely handles reentrancy by always replacing the previous interval. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/makeCoreStore.ts (1)
87-121: Add a nested-dispatch regression test.This handoff is subtle: a plugin hook can synchronously reenter
dispatchEvent(), and correctness now depends on the interval replacement plus thecurrentInterval === intervalguard. Since that path already regressed once, I’d lock it down with a test that dispatches again from a post-effect hook and verifies the newer timer remains the active one until idle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/makeCoreStore.ts` around lines 87 - 121, Add a regression test that reproduces a nested/synchronous reentry into dispatchEvent from a plugin post-effect hook: create a test that wires a post-effect (or equivalent hook used by your plugins) to call dispatchEvent synchronously during the first dispatch, use fake timers (or control time via INTERVAL_MS) to advance timers, and assert that after the nested dispatch the newer timer is the active one (i.e., the original interval was cleared and currentInterval now references the new interval) and that the stack eventually reaches globalTransitionState === "idle" only after the newer timer completes. Target the dispatchEvent implementation and the currentInterval behavior so this catches regressions in the interval-replacement + currentInterval === interval guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/makeCoreStore.ts`:
- Around line 87-121: Add a regression test that reproduces a nested/synchronous
reentry into dispatchEvent from a plugin post-effect hook: create a test that
wires a post-effect (or equivalent hook used by your plugins) to call
dispatchEvent synchronously during the first dispatch, use fake timers (or
control time via INTERVAL_MS) to advance timers, and assert that after the
nested dispatch the newer timer is the active one (i.e., the original interval
was cleared and currentInterval now references the new interval) and that the
stack eventually reaches globalTransitionState === "idle" only after the newer
timer completes. Target the dispatchEvent implementation and the currentInterval
behavior so this catches regressions in the interval-replacement +
currentInterval === interval guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75c49007-95a3-456a-9a66-7a3df6d64c17
📒 Files selected for processing (2)
.changeset/fix-duplicate-interval.mdcore/src/makeCoreStore.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-duplicate-interval.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dispatchEvent가 호출될 때마다 새setInterval이 생성되지만 이전 인터벌을 정리하지 않아, 빠른 연속 액션 시 여러 인터벌이 동시 실행되며 불필요한aggregate()+isEqual()호출이 선형 증가하는 성능 문제 수정intervalRunning플래그를 추가하여 이미 인터벌이 돌고 있으면 새로 생성하지 않도록 변경events.value를 aggregate하므로 새 이벤트도 자동 처리됨Resolves FEP-1958
Test plan
🤖 Generated with Claude Code