React 19 bugs#87
Conversation
📝 WalkthroughWalkthroughThis PR fixes duplicate view-model creation under React Suspense/lazy loading by adding orphan-eviction logic to ChangesCore Store and Hook Fixes
React Regression Test Suite Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReactComponent
participant useCreateViewModel
participant ViewModelStoreBase
participant TempHeap
ReactComponent->>useCreateViewModel: render (lazy/Suspense)
useCreateViewModel->>ViewModelStoreBase: attach(model)
ViewModelStoreBase->>ViewModelStoreBase: getParentId(model)
ViewModelStoreBase->>ViewModelStoreBase: evictOrphans(constructor, parentId)
ViewModelStoreBase->>TempHeap: detach/remove stale orphan VMs
ViewModelStoreBase-->>useCreateViewModel: attach complete
ReactComponent->>useCreateViewModel: unmount (effect cleanup)
useCreateViewModel->>useCreateViewModel: requestAnimationFrame(cleanup)
useCreateViewModel->>useCreateViewModel: check lastAttachedInstanceRef
alt still last attached
useCreateViewModel->>ViewModelStoreBase: detach(detachId)
else superseded instance
useCreateViewModel->>useCreateViewModel: skip cleanup
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/react-regressions/react18/package.json (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePackage name still uses "type-regression" naming.
The package is named
consumer-type-regression-react18but lives undertests/react-regressions/. Consider renaming toconsumer-react-regression-react18for consistency with the new directory structure and thetest:react-regressionscript name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/react-regressions/react18/package.json` at line 2, The package name still uses the old type-regression label; update the name in the react18 package manifest from the current consumer-type-regression style to the consumer-react-regression convention so it matches tests/react-regressions and the test:react-regression script. Keep the change limited to the package naming field in the package.json for this React 18 regression test package.tests/react-regressions/react19/src/suspense-vm-duplicate.test.tsx (1)
1-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting shared test setup to reduce duplication.
The React 18 and React 19 test files are nearly identical (same mock classes, same test structure, same assertions). Any future test changes must be applied to both files. Consider extracting the shared
ViewModelBaseMock,ViewModelStoreBaseMock,RouteStore, and the two test case bodies into a shared module that both fixture packages import, parameterized by React version if needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/react-regressions/react19/src/suspense-vm-duplicate.test.tsx` around lines 1 - 217, The React 19 suspense regression test duplicates the same mock setup and test bodies found in the React 18 fixture, so future changes must be made in two places. Extract the साझा test scaffolding and assertions around ViewModelBaseMock, ViewModelStoreBaseMock, RouteStore, and the two suspense/withViewModel scenarios into a shared helper module, then have both React version test files import it and supply any version-specific setup if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/view-model/view-model.store.base.ts`:
- Around line 362-393: evictOrphans currently treats any same-class, same-parent
instance as an orphan and deletes live siblings, which conflicts with the
multiple-instance behavior used by markToBeAttached, attach, getIds, and getAll.
Update evictOrphans in view-model.store.base.ts to only evict stale duplicates
for the exact model being attached, not all matching constructor/parentId
siblings; preserve existing attached ViewModelSimple and root ViewModel
instances when they are still live, and only remove the prior temp-heap
placeholder or the previous instance that is being replaced.
In `@packages/react/src/hooks/use-create-view-model.ts`:
- Around line 184-200: The RAF-deferred cleanup in useIsomorphicLayoutEffect can
leave a VM attached or accidentally tear down a live one when the callback is
delayed. Update the cleanup around instance, lastAttachedInstanceRef, and the
requestAnimationFrame detach logic to use a guaranteed/unified cleanup path or
cancelable fallback so detach()/unmount() always runs reliably and cannot fire
against a remounted instance.
In `@tests/react-regressions/react18/src/suspense-vm-duplicate.test.tsx`:
- Around line 183-197: The second Suspense regression test is missing the
intermediate PageVM assertion that mirrors the React 19 case. In the test around
the navigate/resolve flow using routeStore.navigate, resolveModule,
vmStore.getIds(LayoutVM), and vmStore.getIds(PageVM), add an assertion after
navigation but before module resolution to verify PageVM is still absent during
the loading state, matching the first test’s expectations.
- Around line 117-131: The Suspense regression test is missing the intermediate
PageVM assertion while the lazy page is still unresolved. In
suspense-vm-duplicate.test.tsx, update the test around routeStore.navigate and
resolveModule to assert vmStore.getIds(PageVM) has length 0 immediately after
the loading fallback is shown, before the module resolves. Keep the existing
LayoutVM and final PageVM assertions so the test still verifies no duplicate VM
is created during the fallback state.
---
Nitpick comments:
In `@tests/react-regressions/react18/package.json`:
- Line 2: The package name still uses the old type-regression label; update the
name in the react18 package manifest from the current consumer-type-regression
style to the consumer-react-regression convention so it matches
tests/react-regressions and the test:react-regression script. Keep the change
limited to the package naming field in the package.json for this React 18
regression test package.
In `@tests/react-regressions/react19/src/suspense-vm-duplicate.test.tsx`:
- Around line 1-217: The React 19 suspense regression test duplicates the same
mock setup and test bodies found in the React 18 fixture, so future changes must
be made in two places. Extract the साझा test scaffolding and assertions around
ViewModelBaseMock, ViewModelStoreBaseMock, RouteStore, and the two
suspense/withViewModel scenarios into a shared helper module, then have both
React version test files import it and supply any version-specific setup if
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60ec0753-7a22-4751-ba8c-6ae97bed219e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.gitignorepackage.jsonpackages/core/src/view-model/view-model.store.base.tspackages/react/src/hooks/use-create-view-model.tspnpm-workspace.yamlscripts/test-react-type-regression.mjstests/react-regressions/react18/package.jsontests/react-regressions/react18/src/repro.tsxtests/react-regressions/react18/src/suspense-vm-duplicate.test.tsxtests/react-regressions/react18/tsconfig.jsontests/react-regressions/react18/vitest.config.tstests/react-regressions/react19/package.jsontests/react-regressions/react19/src/repro.tsxtests/react-regressions/react19/src/suspense-vm-duplicate.test.tsxtests/react-regressions/react19/tsconfig.jsontests/react-regressions/react19/vitest.config.tstests/type-regressions/consumer-react18/package.jsontests/type-regressions/consumer-react18/pnpm-workspace.yamltests/type-regressions/consumer-react19/package.jsontests/type-regressions/consumer-react19/pnpm-workspace.yaml
💤 Files with no reviewable changes (4)
- tests/type-regressions/consumer-react18/package.json
- tests/type-regressions/consumer-react19/pnpm-workspace.yaml
- tests/type-regressions/consumer-react19/package.json
- tests/type-regressions/consumer-react18/pnpm-workspace.yaml
| protected getParentId(vm: VMBase | AnyViewModelSimple) { | ||
| return isViewModel(vm) ? vm.parentViewModel?.id : null | ||
| } | ||
|
|
||
| protected evictOrphans( | ||
| model: VMBase | AnyViewModelSimple, | ||
| modelId: string, | ||
| ): void { | ||
| const isInTempHeap = this.viewModelsTempHeap.has(modelId); | ||
| const constructor = (model as any).constructor as Class<any, any>; | ||
| const vmIds = this.viewModelIdsByClasses.get(constructor); | ||
| if (!vmIds) return; | ||
|
|
||
| const parentId = this.getParentId(model); | ||
|
|
||
| for (const existingId of [...vmIds]) { | ||
| if (existingId === modelId) continue; | ||
| const existingVm = | ||
| this.viewModels.get(existingId) ?? | ||
| this.viewModelsTempHeap.get(existingId); | ||
| if (!existingVm) continue; | ||
| if (this.getParentId(existingVm) !== parentId) continue; | ||
|
|
||
| if (isInTempHeap && this.viewModels.has(existingId)) { | ||
| void this.detach(existingId); | ||
| } else if (this.viewModelsTempHeap.has(existingId)) { | ||
| this.viewModelsTempHeap.delete(existingId); | ||
| this.dettachVMConstructor(existingVm); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ViewModelSimple for payloadChanged, and look for multi-instance/simple-VM eviction tests.
fd -t f 'typeguards' packages/core/src | xargs -I{} sed -n '1,40p' {}
rg -nP '\bpayloadChanged\b' packages/core/src -C2
rg -nP 'evictOrphans|getParentId' packages/core -C3
# Any tests attaching >1 instance of same VM class under same parent, or simple VMs?
rg -nP 'attach\(|markToBeAttached|ViewModelSimple' packages/core/src/view-model/view-model.store.base.test.ts -C2Repository: js2me/mobx-view-model
Length of output: 11433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the store and test files first, then inspect the eviction/attach logic and any tests
# that exercise multiple instances of the same class or simple view models.
ast-grep outline packages/core/src/view-model/view-model.store.base.ts --view expanded
ast-grep outline packages/core/src/view-model/view-model.store.base.test.ts --view expanded
# Read the relevant implementation sections with line numbers.
sed -n '330,460p' packages/core/src/view-model/view-model.store.base.ts | cat -n
sed -n '1,260p' packages/core/src/view-model/view-model.store.base.test.ts | cat -n
# Search for any tests or code paths that imply same-class siblings are allowed under one parent.
rg -n "getIds|getId\(|viewModelIdsByClasses|instanceAttachedCount|evictOrphans|detach\(" packages/core/src -C 2
rg -n "ViewModelSimple|isViewModeSimpleClass|payloadChanged|parentViewModel" packages/core/src -C 2Repository: js2me/mobx-view-model
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the simple VM tests and the parent-related APIs.
sed -n '1,220p' packages/core/src/view-model/view-model-simple.test.ts | cat -n
sed -n '1,220p' packages/core/src/view-model/view-model-simple.ts | cat -n
sed -n '1,120p' packages/core/src/view-model/view-model.ts | cat -n
# Narrow search for explicit multi-instance or one-per-parent assumptions.
rg -n "one per|multiple instances|same parent|parentViewModelId|getIds\(" packages/core/src/view-model -C 2Repository: js2me/mobx-view-model
Length of output: 11895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where markToBeAttached is called and whether temp-heap attachment is the normal path.
rg -n "markToBeAttached\(" packages/core/src -C 3
sed -n '1,220p' packages/core/src/view-model/view-model.base.ts | cat -n
sed -n '220,360p' packages/core/src/view-model/view-model.store.base.ts | cat -nRepository: js2me/mobx-view-model
Length of output: 14925
evictOrphans can remove live siblings of the same class. In packages/core/src/view-model/view-model.store.base.ts:366-386, markToBeAttached() + attach() only keys on (constructor, parentId), so a new ViewModelSimple or root ViewModel with the same parent key can evict an existing same-class instance. The plural lookup APIs (getIds()/getAll()) and the simple-VM tests both allow multiple same-class instances to coexist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/view-model/view-model.store.base.ts` around lines 362 -
393, evictOrphans currently treats any same-class, same-parent instance as an
orphan and deletes live siblings, which conflicts with the multiple-instance
behavior used by markToBeAttached, attach, getIds, and getAll. Update
evictOrphans in view-model.store.base.ts to only evict stale duplicates for the
exact model being attached, not all matching constructor/parentId siblings;
preserve existing attached ViewModelSimple and root ViewModel instances when
they are still live, and only remove the prior temp-heap placeholder or the
previous instance that is being replaced.
| useIsomorphicLayoutEffect(() => { | ||
| return () => { | ||
| const detachInstance = instance; | ||
| const detachId = instance.id; | ||
| requestAnimationFrame(() => { | ||
| if (lastAttachedInstanceRef.current === detachInstance) { | ||
| if (viewModels) { | ||
| void viewModels.detach(detachId); | ||
| } else { | ||
| detachInstance.unmount(); | ||
| } | ||
| lastAttachedInstanceRef.current = null; | ||
| } | ||
| }); | ||
| }; | ||
| }, [instance, viewModels]); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does requestAnimationFrame callback run when the browser tab is hidden or backgrounded in React 19?
💡 Result:
No, the requestAnimationFrame callback does not run (or is heavily throttled/paused) when a browser tab is hidden or backgrounded [1][2]. This behavior is not specific to React 19; it is a standard performance and battery-saving optimization implemented by all major modern browsers [1][2]. When a tab is hidden, the browser pauses requestAnimationFrame callbacks because they are intended to coordinate animations with the display refresh rate, which is unnecessary when the content is not visible [1][3]. In the context of React, this can lead to specific issues where tasks gated by requestAnimationFrame are delayed until the tab becomes visible again [4][5]. For example: 1. Suspense Boundaries: In some cases, React's internal mechanisms (such as those used for streaming Suspense) have historically relied on requestAnimationFrame to flush updates [4]. If the tab is hidden while these updates are pending, the hydration or reveal might be delayed indefinitely until the user returns to the tab [4][6]. 2. Batching and State Updates: Since React often uses requestAnimationFrame internally to batch state updates or schedule rendering, these processes will be deferred while the tab is backgrounded [7][5]. When the tab returns to the foreground, React may then process a backlog of updates, which can occasionally lead to performance hitches if many updates were queued while the tab was inactive [5]. Modern versions of React, including React 19, are designed to be aware of these browser limitations and often include fallback mechanisms (such as using setTimeout) to ensure that critical work eventually proceeds even when requestAnimationFrame is not firing [7][6]. If you are encountering issues with delayed execution in background tabs, it is generally recommended to avoid relying on requestAnimationFrame for non-animation logic or to implement visibility-aware checks (using the Page Visibility API) to handle essential tasks differently when the tab is hidden [4][6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Window/requestAnimationFrame
- 2: https://stackoverflow.com/questions/15871942/how-do-browsers-pause-change-javascript-when-tab-or-window-is-not-active
- 3: https://stackoverflow.com/questions/46765937/run-requestanimationframe-in-background
- 4: Bug: Streamed Suspense boundaries never reveal/hydrate in hidden or occluded tabs ($RV reveal and hydration retry are gated on requestAnimationFrame) react/react#36741
- 5: Context subscribers block thread for long period after restoring tab to foreground react/react#14888
- 6: [Fizz] Fix hidden-tab Suspense reveals: initialise $RT synchronously and via visibilitychange react/react#36751
- 7: Fall back to 'setTimeout' when 'requestAnimationFrame' is not called react/react#13091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines
FILE="packages/react/src/hooks/use-create-view-model.ts"
wc -l "$FILE"
sed -n '1,280p' "$FILE"
# Find related uses of requestAnimationFrame and instance lifecycle handling
rg -n "requestAnimationFrame|cancelAnimationFrame|lastAttachedInstanceRef|detach\(|unmount\(" packages/react/src -S
# Map nearby hook/function structure in the target file
ast-grep outline "$FILE" --view expandedRepository: js2me/mobx-view-model
Length of output: 11157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for Offscreen/Activity/re-attach handling and tests around this hook
rg -n "Offscreen|Activity|reappear|re-attach|reattach|lastAttachedInstanceRef|useValue\\(|requestAnimationFrame" packages/react/src -S
# Inspect the hook tests and the shared hook helper implementation
FILE1="packages/react/src/hooks/use-create-view-model.test.tsx"
FILE2="packages/react/src/lib/hooks/index.ts"
FILE3="packages/react/src/lib/hooks/use-value.ts"
for f in "$FILE1" "$FILE2" "$FILE3"; do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
fi
done
# Read the relevant test file sections if present
if [ -f "$FILE1" ]; then
sed -n '1,260p' "$FILE1"
sed -n '260,560p' "$FILE1"
fi
# Read helper exports and implementation if present
if [ -f "$FILE2" ]; then
sed -n '1,220p' "$FILE2"
fi
if [ -f "$FILE3" ]; then
sed -n '1,260p' "$FILE3"
fiRepository: js2me/mobx-view-model
Length of output: 18386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any requestAnimationFrame mocks or test setup that affects timing
rg -n "requestAnimationFrame|cancelAnimationFrame|vi\\.useFakeTimers|advanceTimers|runAllTimers|jsdom" \
packages/react test tests -S
# Inspect the hook test around the unmount cases and any timing-related helpers
FILE="packages/react/src/hooks/use-create-view-model.test.tsx"
sed -n '300,520p' "$FILE"
sed -n '520,760p' "$FILE"
# Inspect any shared test setup files that might patch RAF
fd -a "setup*.{ts,tsx,js,jsx}" .
fd -a "*test*setup*"Repository: js2me/mobx-view-model
Length of output: 1164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the hook helper and any timing-specific test setup without broad output
sed -n '1,220p' packages/react/src/lib/hooks/use-isomorphic-layout-effect.ts
sed -n '300,520p' packages/react/src/hooks/use-create-view-model.test.tsx
# Search for any direct RAF shims or cleanup cancellation patterns in the react package
rg -n "cancelAnimationFrame|requestAnimationFrame|raf|useIsomorphicLayoutEffect" packages/react/src packages/react/test -S
# Read the react vitest config and setup files if present
sed -n '1,220p' packages/react/vitest.config.ts
fd -a "setup*.ts" packages/reactRepository: js2me/mobx-view-model
Length of output: 6398
RAF-deferred cleanup can leak or tear down a live VM. requestAnimationFrame can be paused in hidden tabs, so detach()/unmount() may never run after unmount. The lastAttachedInstanceRef guard also doesn’t protect a preserved Offscreen reappear: if the instance is shown again before the callback fires, the queued detach can still hit the live VM. Use a guaranteed cleanup path or a cancelable fallback instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react/src/hooks/use-create-view-model.ts` around lines 184 - 200,
The RAF-deferred cleanup in useIsomorphicLayoutEffect can leave a VM attached or
accidentally tear down a live one when the callback is delayed. Update the
cleanup around instance, lastAttachedInstanceRef, and the requestAnimationFrame
detach logic to use a guaranteed/unified cleanup path or cancelable fallback so
detach()/unmount() always runs reliably and cannot fire against a remounted
instance.
| await act(async () => { | ||
| routeStore.navigate('page'); | ||
| }); | ||
|
|
||
| expect(screen.getByTestId('loading')).toBeDefined(); | ||
|
|
||
| await act(async () => { | ||
| resolveModule({ default: PageComponent }); | ||
| }); | ||
|
|
||
| expect(vmStore.getIds(LayoutVM)).toHaveLength(1); | ||
| expect(vmStore.getIds(PageVM)).toHaveLength(1); | ||
|
|
||
| expect(screen.getByTestId('page')).toBeDefined(); | ||
| expect(screen.queryByTestId('loading')).toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add missing PageVM count assertion during Suspense state.
The React 19 version of this test (line 116) includes expect(vmStore.getIds(PageVM)).toHaveLength(0); after navigating to the page but before resolving the lazy module. This assertion verifies that no duplicate VM is created during the Suspense fallback state — a core part of the regression being tested. The React 18 test omits this check, weakening its coverage.
🛡️ Proposed fix
expect(screen.getByTestId('loading')).toBeDefined();
+ expect(vmStore.getIds(PageVM)).toHaveLength(0);
await act(async () => {
resolveModule({ default: PageComponent });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await act(async () => { | |
| routeStore.navigate('page'); | |
| }); | |
| expect(screen.getByTestId('loading')).toBeDefined(); | |
| await act(async () => { | |
| resolveModule({ default: PageComponent }); | |
| }); | |
| expect(vmStore.getIds(LayoutVM)).toHaveLength(1); | |
| expect(vmStore.getIds(PageVM)).toHaveLength(1); | |
| expect(screen.getByTestId('page')).toBeDefined(); | |
| expect(screen.queryByTestId('loading')).toBeNull(); | |
| await act(async () => { | |
| routeStore.navigate('page'); | |
| }); | |
| expect(screen.getByTestId('loading')).toBeDefined(); | |
| expect(vmStore.getIds(PageVM)).toHaveLength(0); | |
| await act(async () => { | |
| resolveModule({ default: PageComponent }); | |
| }); | |
| expect(vmStore.getIds(LayoutVM)).toHaveLength(1); | |
| expect(vmStore.getIds(PageVM)).toHaveLength(1); | |
| expect(screen.getByTestId('page')).toBeDefined(); | |
| expect(screen.queryByTestId('loading')).toBeNull(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/react-regressions/react18/src/suspense-vm-duplicate.test.tsx` around
lines 117 - 131, The Suspense regression test is missing the intermediate PageVM
assertion while the lazy page is still unresolved. In
suspense-vm-duplicate.test.tsx, update the test around routeStore.navigate and
resolveModule to assert vmStore.getIds(PageVM) has length 0 immediately after
the loading fallback is shown, before the module resolves. Keep the existing
LayoutVM and final PageVM assertions so the test still verifies no duplicate VM
is created during the fallback state.
| await act(async () => { | ||
| routeStore.navigate('page'); | ||
| }); | ||
|
|
||
| expect(screen.getByTestId('loading')).toBeDefined(); | ||
|
|
||
| await act(async () => { | ||
| resolveModule({ default: PageComponent }); | ||
| }); | ||
|
|
||
| expect(vmStore.getIds(LayoutVM)).toHaveLength(1); | ||
| expect(vmStore.getIds(PageVM)).toHaveLength(1); | ||
|
|
||
| expect(screen.getByTestId('page')).toBeDefined(); | ||
| expect(screen.queryByTestId('loading')).toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add missing PageVM count assertion in second test during Suspense state.
Same as the first test case — the React 19 version (line 205) asserts expect(vmStore.getIds(PageVM)).toHaveLength(0); after navigating but before resolving the module. This check is absent here.
🛡️ Proposed fix
expect(screen.getByTestId('loading')).toBeDefined();
+ expect(vmStore.getIds(PageVM)).toHaveLength(0);
await act(async () => {
resolveModule({ default: PageComponent });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await act(async () => { | |
| routeStore.navigate('page'); | |
| }); | |
| expect(screen.getByTestId('loading')).toBeDefined(); | |
| await act(async () => { | |
| resolveModule({ default: PageComponent }); | |
| }); | |
| expect(vmStore.getIds(LayoutVM)).toHaveLength(1); | |
| expect(vmStore.getIds(PageVM)).toHaveLength(1); | |
| expect(screen.getByTestId('page')).toBeDefined(); | |
| expect(screen.queryByTestId('loading')).toBeNull(); | |
| await act(async () => { | |
| routeStore.navigate('page'); | |
| }); | |
| expect(screen.getByTestId('loading')).toBeDefined(); | |
| expect(vmStore.getIds(PageVM)).toHaveLength(0); | |
| await act(async () => { | |
| resolveModule({ default: PageComponent }); | |
| }); | |
| expect(vmStore.getIds(LayoutVM)).toHaveLength(1); | |
| expect(vmStore.getIds(PageVM)).toHaveLength(1); | |
| expect(screen.getByTestId('page')).toBeDefined(); | |
| expect(screen.queryByTestId('loading')).toBeNull(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/react-regressions/react18/src/suspense-vm-duplicate.test.tsx` around
lines 183 - 197, The second Suspense regression test is missing the intermediate
PageVM assertion that mirrors the React 19 case. In the test around the
navigate/resolve flow using routeStore.navigate, resolveModule,
vmStore.getIds(LayoutVM), and vmStore.getIds(PageVM), add an assertion after
navigation but before module resolution to verify PageVM is still absent during
the loading state, matching the first test’s expectations.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="package.json">
<violation number="1" location="package.json:17">
P0: CI workflow `.github/workflows/main.yml` still runs `pnpm test:type-regression` but this script was renamed to `test:react-regression` in `package.json`. CI will fail because `test:type-regression` no longer exists. Update the workflow to use the new script name.</violation>
</file>
<file name="packages/core/src/view-model/view-model.store.base.ts">
<violation number="1" location="packages/core/src/view-model/view-model.store.base.ts:363">
P2: Simple view models under different parents can evict each other because `getParentId` ignores `ViewModelSimple.parentViewModel` and returns `null` for every non-`ViewModel`. Using the shared `parentViewModel?.id` property here would keep orphan eviction scoped to the actual parent.</violation>
<violation number="2" location="packages/core/src/view-model/view-model.store.base.ts:383">
P2: `evictOrphans` matches by `(constructor, parentId)`, so attaching a second instance of the same VM class under the same parent (or two root VMs of the same class, both with `parentId = null`) will evict the first live instance. Since the store's plural lookup APIs (`getIds()`, `getAll()`) and the simple-VM pattern both support multiple same-class instances coexisting, this eviction logic is too broad and could remove legitimate siblings.</violation>
<violation number="3" location="packages/core/src/view-model/view-model.store.base.ts:386">
P2: `evictOrphans` calls `void this.detach(existingId)` which is a fire-and-forget async call. The `detach()` method is `async` and calls `await this.unmount(model)` internally. If `unmount()` throws (e.g., user code in `willUnmount` or `didUnmount` throws), the resulting promise rejection is unhandled — it is neither caught nor awaited, causing an unhandled promise rejection that in Node.js 15+ can terminate the process. Consider catching the error or using a `.catch()` handler.</violation>
</file>
<file name="tests/react-regressions/react19/src/suspense-vm-duplicate.test.tsx">
<violation number="1" location="tests/react-regressions/react19/src/suspense-vm-duplicate.test.tsx:71">
P3: Redundant assertion: `screen.getByTestId('page')` already validates element existence by throwing on not-found, so `.toBeDefined()` on its return value always passes and provides no additional safety. Consider `screen.findByTestId('page')` for async assertions, or simply drop `.toBeDefined()` and rely on `getByTestId`'s throw.</violation>
</file>
<file name="packages/react/src/hooks/use-create-view-model.ts">
<violation number="1" location="packages/react/src/hooks/use-create-view-model.ts:188">
P1: A VM can be detached/unmounted after it has already reappeared or been remounted before the next frame, because the deferred cleanup is never canceled and its per-hook ref still points at the old instance. Consider tracking/canceling the pending detach on effect setup or using a mount generation so stale RAF callbacks cannot tear down an active ViewModel.</violation>
</file>
<file name="tests/react-regressions/react18/src/suspense-vm-duplicate.test.tsx">
<violation number="1" location="tests/react-regressions/react18/src/suspense-vm-duplicate.test.tsx:121">
P3: The React 19 variant of this test asserts `expect(vmStore.getIds(PageVM)).toHaveLength(0)` during the Suspense fallback state, verifying no premature VM is created. This assertion is missing here in the React 18 test, reducing its ability to catch the specific regression being tested.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "test:watch": "pnpm -r --filter './packages/*' run test:watch", | ||
| "test:coverage": "pnpm -r --filter './packages/*' run test:coverage", | ||
| "test:type-regression": "node ./scripts/test-type-regression.mjs", | ||
| "test:react-regression": "node ./scripts/test-react-type-regression.mjs && pnpm -r --filter './tests/react-regressions/*' run test", |
There was a problem hiding this comment.
P0: CI workflow .github/workflows/main.yml still runs pnpm test:type-regression but this script was renamed to test:react-regression in package.json. CI will fail because test:type-regression no longer exists. Update the workflow to use the new script name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At package.json, line 17:
<comment>CI workflow `.github/workflows/main.yml` still runs `pnpm test:type-regression` but this script was renamed to `test:react-regression` in `package.json`. CI will fail because `test:type-regression` no longer exists. Update the workflow to use the new script name.</comment>
<file context>
@@ -14,7 +14,7 @@
"test:watch": "pnpm -r --filter './packages/*' run test:watch",
"test:coverage": "pnpm -r --filter './packages/*' run test:coverage",
- "test:type-regression": "node ./scripts/test-type-regression.mjs",
+ "test:react-regression": "node ./scripts/test-react-type-regression.mjs && pnpm -r --filter './tests/react-regressions/*' run test",
"docs": "pnpm build && pnpm --filter mobx-view-model-docs run dev",
"docs:build": "pnpm build && pnpm --filter mobx-view-model-docs run build",
</file context>
| return () => { | ||
| const detachInstance = instance; | ||
| const detachId = instance.id; | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
P1: A VM can be detached/unmounted after it has already reappeared or been remounted before the next frame, because the deferred cleanup is never canceled and its per-hook ref still points at the old instance. Consider tracking/canceling the pending detach on effect setup or using a mount generation so stale RAF callbacks cannot tear down an active ViewModel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react/src/hooks/use-create-view-model.ts, line 188:
<comment>A VM can be detached/unmounted after it has already reappeared or been remounted before the next frame, because the deferred cleanup is never canceled and its per-hook ref still points at the old instance. Consider tracking/canceling the pending detach on effect setup or using a mount generation so stale RAF callbacks cannot tear down an active ViewModel.</comment>
<file context>
@@ -197,6 +181,23 @@ const useCreateViewModelBase = (
+ return () => {
+ const detachInstance = instance;
+ const detachId = instance.id;
+ requestAnimationFrame(() => {
+ if (lastAttachedInstanceRef.current === detachInstance) {
+ if (viewModels) {
</file context>
| } | ||
|
|
||
| protected getParentId(vm: VMBase | AnyViewModelSimple) { | ||
| return isViewModel(vm) ? vm.parentViewModel?.id : null |
There was a problem hiding this comment.
P2: Simple view models under different parents can evict each other because getParentId ignores ViewModelSimple.parentViewModel and returns null for every non-ViewModel. Using the shared parentViewModel?.id property here would keep orphan eviction scoped to the actual parent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/view-model/view-model.store.base.ts, line 363:
<comment>Simple view models under different parents can evict each other because `getParentId` ignores `ViewModelSimple.parentViewModel` and returns `null` for every non-`ViewModel`. Using the shared `parentViewModel?.id` property here would keep orphan eviction scoped to the actual parent.</comment>
<file context>
@@ -358,6 +359,38 @@ export class ViewModelStoreBase<VMBase extends AnyViewModel = AnyViewModel>
}
+ protected getParentId(vm: VMBase | AnyViewModelSimple) {
+ return isViewModel(vm) ? vm.parentViewModel?.id : null
+ }
+
</file context>
| return isViewModel(vm) ? vm.parentViewModel?.id : null | |
| return vm.parentViewModel?.id ?? null; |
| if (this.getParentId(existingVm) !== parentId) continue; | ||
|
|
||
| if (isInTempHeap && this.viewModels.has(existingId)) { | ||
| void this.detach(existingId); |
There was a problem hiding this comment.
P2: evictOrphans calls void this.detach(existingId) which is a fire-and-forget async call. The detach() method is async and calls await this.unmount(model) internally. If unmount() throws (e.g., user code in willUnmount or didUnmount throws), the resulting promise rejection is unhandled — it is neither caught nor awaited, causing an unhandled promise rejection that in Node.js 15+ can terminate the process. Consider catching the error or using a .catch() handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/view-model/view-model.store.base.ts, line 386:
<comment>`evictOrphans` calls `void this.detach(existingId)` which is a fire-and-forget async call. The `detach()` method is `async` and calls `await this.unmount(model)` internally. If `unmount()` throws (e.g., user code in `willUnmount` or `didUnmount` throws), the resulting promise rejection is unhandled — it is neither caught nor awaited, causing an unhandled promise rejection that in Node.js 15+ can terminate the process. Consider catching the error or using a `.catch()` handler.</comment>
<file context>
@@ -358,6 +359,38 @@ export class ViewModelStoreBase<VMBase extends AnyViewModel = AnyViewModel>
+ if (this.getParentId(existingVm) !== parentId) continue;
+
+ if (isInTempHeap && this.viewModels.has(existingId)) {
+ void this.detach(existingId);
+ } else if (this.viewModelsTempHeap.has(existingId)) {
+ this.viewModelsTempHeap.delete(existingId);
</file context>
| void this.detach(existingId); | |
| if (isInTempHeap && this.viewModels.has(existingId)) { | |
| this.detach(existingId).catch(() => {}); | |
| } else if (this.viewModelsTempHeap.has(existingId)) { |
| this.viewModels.get(existingId) ?? | ||
| this.viewModelsTempHeap.get(existingId); | ||
| if (!existingVm) continue; | ||
| if (this.getParentId(existingVm) !== parentId) continue; |
There was a problem hiding this comment.
P2: evictOrphans matches by (constructor, parentId), so attaching a second instance of the same VM class under the same parent (or two root VMs of the same class, both with parentId = null) will evict the first live instance. Since the store's plural lookup APIs (getIds(), getAll()) and the simple-VM pattern both support multiple same-class instances coexisting, this eviction logic is too broad and could remove legitimate siblings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/view-model/view-model.store.base.ts, line 383:
<comment>`evictOrphans` matches by `(constructor, parentId)`, so attaching a second instance of the same VM class under the same parent (or two root VMs of the same class, both with `parentId = null`) will evict the first live instance. Since the store's plural lookup APIs (`getIds()`, `getAll()`) and the simple-VM pattern both support multiple same-class instances coexisting, this eviction logic is too broad and could remove legitimate siblings.</comment>
<file context>
@@ -358,6 +359,38 @@ export class ViewModelStoreBase<VMBase extends AnyViewModel = AnyViewModel>
+ this.viewModels.get(existingId) ??
+ this.viewModelsTempHeap.get(existingId);
+ if (!existingVm) continue;
+ if (this.getParentId(existingVm) !== parentId) continue;
+
+ if (isInTempHeap && this.viewModels.has(existingId)) {
</file context>
|
|
||
| const PageComponent = observer(() => { | ||
| const vm = useCreateViewModel(PageVM); | ||
| return <span data-testid="page">{vm.id}</span>; |
There was a problem hiding this comment.
P3: Redundant assertion: screen.getByTestId('page') already validates element existence by throwing on not-found, so .toBeDefined() on its return value always passes and provides no additional safety. Consider screen.findByTestId('page') for async assertions, or simply drop .toBeDefined() and rely on getByTestId's throw.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/react-regressions/react19/src/suspense-vm-duplicate.test.tsx, line 71:
<comment>Redundant assertion: `screen.getByTestId('page')` already validates element existence by throwing on not-found, so `.toBeDefined()` on its return value always passes and provides no additional safety. Consider `screen.findByTestId('page')` for async assertions, or simply drop `.toBeDefined()` and rely on `getByTestId`'s throw.</comment>
<file context>
@@ -0,0 +1,217 @@
+
+ const PageComponent = observer(() => {
+ const vm = useCreateViewModel(PageVM);
+ return <span data-testid="page">{vm.id}</span>;
+ });
+
</file context>
| routeStore.navigate('page'); | ||
| }); | ||
|
|
||
| expect(screen.getByTestId('loading')).toBeDefined(); |
There was a problem hiding this comment.
P3: The React 19 variant of this test asserts expect(vmStore.getIds(PageVM)).toHaveLength(0) during the Suspense fallback state, verifying no premature VM is created. This assertion is missing here in the React 18 test, reducing its ability to catch the specific regression being tested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/react-regressions/react18/src/suspense-vm-duplicate.test.tsx, line 121:
<comment>The React 19 variant of this test asserts `expect(vmStore.getIds(PageVM)).toHaveLength(0)` during the Suspense fallback state, verifying no premature VM is created. This assertion is missing here in the React 18 test, reducing its ability to catch the specific regression being tested.</comment>
<file context>
@@ -0,0 +1,199 @@
+ routeStore.navigate('page');
+ });
+
+ expect(screen.getByTestId('loading')).toBeDefined();
+
+ await act(async () => {
</file context>
1 participant
Summary by cubic
Fixes duplicate ViewModel instances and infinite remount loops with React 19 Suspense/lazy by changing when VMs attach and by evicting duplicates in the store. Adds React 18/19 regression tests and a
test:react-regressionscript to prevent future regressions.Bug Fixes
useCreateViewModel, defer detach to a requestAnimationFrame cleanup, and guard against Offscreen re-attach; prevents split MobX reactions and remount loops in React 19.ViewModelStoreBase, evict same-constructor siblings with the same parent (evictOrphans), consider the temp heap inisAbleToRenderView, skip mount if already mounted, and tighten attach/detach cleanup.Refactors
tests/react-regressions/*(Suspense/lazy +observer/withViewModelcases), and updated workspace and.gitignore.scripts/test-react-type-regression.mjsandpnpm run test:react-regressionto install ephemeral fixtures and run the new suites.Written for commit f373ab5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores