Conversation
🦋 Changeset detectedLatest commit: 94c2876 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
Contributor
📝 WalkthroughWalkthroughRefactors and splits the snapshot subsystem into a new snapshot/ subdirectory (definitions, types, utils, background manager), introduces a global lifecycle state module, updates many imports/exports to new module paths, adds several new modules, and updates numerous tests to the consolidated snapshot API. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/react/runtime/src/lifecycle/patch/globalState.ts (1)
14-16: Clarify setter semantics (replace vs mutate-in-place).
setGlobalBackgroundSnapshotInstancesToRemove()currently replaces the array reference. Consider naming/documentation that makes this explicit (e.g.,replace...) to prevent misuse by future callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/patch/globalState.ts` around lines 14 - 16, The setter setGlobalBackgroundSnapshotInstancesToRemove currently replaces the array reference which can surprise callers—either make the semantics explicit by renaming the function (e.g., replaceGlobalBackgroundSnapshotInstancesToRemove) and update docs/comments to state it replaces the array, or change the implementation to mutate the existing array in-place (clear and push into the existing globalBackgroundSnapshotInstancesToRemove) and document that it mutates; update usages/tests accordingly to reflect the chosen semantic and ensure callers relying on reference identity are adjusted.packages/react/runtime/src/backgroundSnapshot.ts (1)
59-75: Consider defensive check before non-null assertion onvalues.get(id).Line 61 uses
values.get(id)!which assumes the id exists. IfupdateIdis called with a non-existent id, this would cause a silent failure when settingsi.__id = newIdonundefined.🛡️ Optional defensive check
updateId(id: number, newId: number) { const values = this.values; const si = values.get(id)!; + if (!si) { + if (__DEV__) { + console.warn(`backgroundSnapshotInstanceManager.updateId: id ${id} not found`); + } + return; + } // For PreactDevtools, on first hydration,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/backgroundSnapshot.ts` around lines 59 - 75, The non-null assertion on values.get(id) in updateId assumes the entry exists; add a defensive check for si (the result of values.get(id)) before using it: retrieve si via values.get(id), if it's undefined handle gracefully (e.g., log an error via lynx/processLogger or throw an informative error) and return early to avoid accessing si.__id on undefined, then proceed with the existing emit, values.delete/set, and si.__id = newId when si is present; reference symbols: updateId, values, si, and the emitted event name onBackgroundSnapshotInstanceUpdateId.packages/react/runtime/src/snapshot/definition.ts (1)
81-107: Usingnullas a Map key for the anonymous text snapshot.The
null as unknown as stringcast is a deliberate workaround to usenullas a key in aMap<string, Snapshot>. While this works at runtime (Maps accept any value as key), it circumvents TypeScript's type system. Consider using a sentinel string constant like'__raw_text__'instead for better type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/definition.ts` around lines 81 - 107, Replace the unsafe `null as unknown as string` map key with a typed sentinel string (e.g. '__raw_text__') and use that constant everywhere the anonymous text snapshot key is referenced; update the Map<string, Snapshot> usage to rely on that sentinel instead of casting, and ensure any lookup code that previously expected null handles the sentinel (look for the array entry with the first element `null as unknown as string` and the snapshot object with `create`, `update`, `slot`, `isListHolder` to change references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/src/snapshot.ts`:
- Around line 5-9: The JSDoc for SnapshotInstance is contradictory; update the
comment so it correctly describes that SnapshotInstance is the main-thread
implementation (the mirror of BackgroundSnapshotInstance which runs in the
background thread). Locate the header block above the SnapshotInstance
declaration and replace the wording "Main thread snapshot implementation that
runs in the background thread" with a clear description stating that
SnapshotInstance is the main-thread implementation and that
BackgroundSnapshotInstance (in backgroundSnapshot.ts) is the background-thread
counterpart.
---
Nitpick comments:
In `@packages/react/runtime/src/backgroundSnapshot.ts`:
- Around line 59-75: The non-null assertion on values.get(id) in updateId
assumes the entry exists; add a defensive check for si (the result of
values.get(id)) before using it: retrieve si via values.get(id), if it's
undefined handle gracefully (e.g., log an error via lynx/processLogger or throw
an informative error) and return early to avoid accessing si.__id on undefined,
then proceed with the existing emit, values.delete/set, and si.__id = newId when
si is present; reference symbols: updateId, values, si, and the emitted event
name onBackgroundSnapshotInstanceUpdateId.
In `@packages/react/runtime/src/lifecycle/patch/globalState.ts`:
- Around line 14-16: The setter setGlobalBackgroundSnapshotInstancesToRemove
currently replaces the array reference which can surprise callers—either make
the semantics explicit by renaming the function (e.g.,
replaceGlobalBackgroundSnapshotInstancesToRemove) and update docs/comments to
state it replaces the array, or change the implementation to mutate the existing
array in-place (clear and push into the existing
globalBackgroundSnapshotInstancesToRemove) and document that it mutates; update
usages/tests accordingly to reflect the chosen semantic and ensure callers
relying on reference identity are adjusted.
In `@packages/react/runtime/src/snapshot/definition.ts`:
- Around line 81-107: Replace the unsafe `null as unknown as string` map key
with a typed sentinel string (e.g. '__raw_text__') and use that constant
everywhere the anonymous text snapshot key is referenced; update the Map<string,
Snapshot> usage to rely on that sentinel instead of casting, and ensure any
lookup code that previously expected null handles the sentinel (look for the
array entry with the first element `null as unknown as string` and the snapshot
object with `create`, `update`, `slot`, `isListHolder` to change references).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d65bbf4-f45f-4a1f-b39a-265ff8ea0ee4
📒 Files selected for processing (30)
.changeset/thirty-snakes-doubt.mdpackages/react/runtime/__test__/compat.test.jsxpackages/react/runtime/__test__/compat/initData.test.jsxpackages/react/runtime/__test__/debug/backgroundSnapshot-profile.test.jsxpackages/react/runtime/__test__/debug/printSnapshot.test.jsxpackages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsxpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/__test__/lifecycle.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/preact.test.jsxpackages/react/runtime/__test__/render.test.jsxpackages/react/runtime/__test__/snapshot/event.test.jsxpackages/react/runtime/__test__/snapshot/spread.test.jsxpackages/react/runtime/__test__/utils/envManager.tspackages/react/runtime/src/backgroundSnapshot.tspackages/react/runtime/src/debug/printSnapshot.tspackages/react/runtime/src/internal.tspackages/react/runtime/src/lifecycle/patch/commit.tspackages/react/runtime/src/lifecycle/patch/error.tspackages/react/runtime/src/lifecycle/patch/globalState.tspackages/react/runtime/src/lifecycle/patch/updateMainThread.tspackages/react/runtime/src/lifecycle/reload.tspackages/react/runtime/src/lynx/calledByNative.tspackages/react/runtime/src/lynx/tt.tspackages/react/runtime/src/snapshot.tspackages/react/runtime/src/snapshot/constants.tspackages/react/runtime/src/snapshot/definition.tspackages/react/runtime/src/snapshot/dynamicPartType.tspackages/react/runtime/src/snapshot/types.tspackages/react/runtime/src/snapshot/utils.ts
HuJean
changed the title
Merging this PR will degrade performance by 21.19%
Performance Changes
Comparing 
Footnotes
|
React MTF Example#24 Bundle Size — 207.47KiB (0%).94c2876(current) vs a32bc28 main#13(baseline) Bundle metrics
|
| Current #24 |
Baseline #13 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
 Modules |
174(+3.57%) |
168 |
 Duplicate Modules |
68(+3.03%) |
66 |
| Duplicate Code |
46.09%(-0.09%) |
46.13% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #24 |
Baseline #13 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
96.24KiB |
96.24KiB |
Bundle analysis report Branch p/refactor-snapshot Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8468 Bundle Size — 727.89KiB (0%).94c2876(current) vs a32bc28 main#8457(baseline) Bundle metrics
|
| Current #8468 |
Baseline #8457 |
|
|---|---|---|
| Initial JS |
42.62KiB |
42.62KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
149 |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
34.73% |
34.73% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8468 |
Baseline #8457 |
|
|---|---|---|
| Other |
384.4KiB |
384.4KiB |
| JS |
341.33KiB |
341.33KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch p/refactor-snapshot Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#6890 Bundle Size — 237.89KiB (0%).94c2876(current) vs a32bc28 main#6879(baseline) Bundle metrics
|
| Current #6890 |
Baseline #6879 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
180(+3.45%) |
174 |
| Duplicate Modules |
71(+2.9%) |
69 |
| Duplicate Code |
46.4%(-0.09%) |
46.44% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #6890 |
Baseline #6879 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
92.13KiB |
92.13KiB |
Bundle analysis report Branch p/refactor-snapshot Project dashboard
Generated by RelativeCI Documentation Report issue
HuJean
force-pushed
the
p/refactor-snapshot
branch
4 times, most recently
from
8d68861 to
4e961b6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/src/snapshot/definition.ts`:
- Around line 30-39: Snapshot.slot's current type ([DynamicPartType, number][])
is too strict; update it to accept null entries and tuples that may omit the
numeric index. Change the slot declaration on the Snapshot interface to a union
like Array<null | [DynamicPartType] | [DynamicPartType, number]> (i.e., allow
null and both single-element and two-element tuples) so consumers don't need
non-null assertions and the type matches the runtime guards and tests
referencing DynamicPartType and Snapshot.slot.
In `@packages/react/runtime/src/snapshot/index.ts`:
- Line 7: The internal entrypoint is missing re-exports for the new snapshot
helpers; add exports for setupPage and clearPage to
packages/react/runtime/src/internal.ts so callers of the old internal API
continue to work. Locate the existing snapshot-related exports (note __page and
__pageId are already exported) and add export { setupPage, clearPage } from
'./snapshot/definition.js'; ensuring the symbols setupPage and clearPage are
re-exported from internal.ts alongside the existing __page and __pageId exports.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d43f74b-38a8-41c3-a9ba-1ca04a14ef7f
📒 Files selected for processing (56)
.changeset/thirty-snakes-doubt.mdpackages/react/runtime/__test__/clone.test.jsxpackages/react/runtime/__test__/compat.test.jsxpackages/react/runtime/__test__/compat/initData.test.jsxpackages/react/runtime/__test__/debug/backgroundSnapshot-profile.test.jsxpackages/react/runtime/__test__/debug/printSnapshot.test.jsxpackages/react/runtime/__test__/element.test.jsxpackages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsxpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/__test__/lifecycle.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/preact.test.jsxpackages/react/runtime/__test__/render.test.jsxpackages/react/runtime/__test__/snapshot/event.test.jsxpackages/react/runtime/__test__/snapshot/spread.test.jsxpackages/react/runtime/__test__/snapshotPatch.test.jsxpackages/react/runtime/__test__/utils/envManager.tspackages/react/runtime/src/alog/render.tspackages/react/runtime/src/debug/printSnapshot.tspackages/react/runtime/src/document.tspackages/react/runtime/src/hydrate.tspackages/react/runtime/src/internal.tspackages/react/runtime/src/lifecycle/patch/commit.tspackages/react/runtime/src/lifecycle/patch/error.tspackages/react/runtime/src/lifecycle/patch/globalState.tspackages/react/runtime/src/lifecycle/patch/snapshotPatchApply.tspackages/react/runtime/src/lifecycle/patch/updateMainThread.tspackages/react/runtime/src/lifecycle/ref/delay.tspackages/react/runtime/src/lifecycle/reload.tspackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/list.tspackages/react/runtime/src/listUpdateInfo.tspackages/react/runtime/src/lynx/calledByNative.tspackages/react/runtime/src/lynx/injectLepusMethods.tspackages/react/runtime/src/lynx/suspense.tspackages/react/runtime/src/lynx/tt.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/root.tspackages/react/runtime/src/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/constants.tspackages/react/runtime/src/snapshot/definition.tspackages/react/runtime/src/snapshot/dynamicPartType.tspackages/react/runtime/src/snapshot/event.tspackages/react/runtime/src/snapshot/gesture.tspackages/react/runtime/src/snapshot/index.tspackages/react/runtime/src/snapshot/list.tspackages/react/runtime/src/snapshot/platformInfo.tspackages/react/runtime/src/snapshot/ref.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/runtime/src/snapshot/snapshotInstanceHydrationMap.tspackages/react/runtime/src/snapshot/spread.tspackages/react/runtime/src/snapshot/types.tspackages/react/runtime/src/snapshot/utils.tspackages/react/runtime/src/snapshot/workletEvent.tspackages/react/runtime/src/snapshot/workletRef.tspackages/react/testing-library/src/vitest-global-setup.js
✅ Files skipped from review due to trivial changes (42)
- packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts
- .changeset/thirty-snakes-doubt.md
- packages/react/runtime/test/element.test.jsx
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/runtime/src/root.ts
- packages/react/runtime/test/render.test.jsx
- packages/react/runtime/src/snapshot/platformInfo.ts
- packages/react/runtime/src/hydrate.ts
- packages/react/runtime/src/snapshot/ref.ts
- packages/react/runtime/src/alog/render.ts
- packages/react/runtime/test/clone.test.jsx
- packages/react/runtime/src/lifecycle/patch/updateMainThread.ts
- packages/react/runtime/src/lynx/injectLepusMethods.ts
- packages/react/runtime/src/snapshot/event.ts
- packages/react/runtime/src/snapshot/gesture.ts
- packages/react/runtime/src/listUpdateInfo.ts
- packages/react/runtime/src/list.ts
- packages/react/runtime/src/snapshot/workletRef.ts
- packages/react/runtime/src/snapshot/list.ts
- packages/react/runtime/src/lifecycle/ref/delay.ts
- packages/react/runtime/src/document.ts
- packages/react/runtime/src/snapshot/spread.ts
- packages/react/runtime/src/snapshot/dynamicPartType.ts
- packages/react/runtime/src/lifecycle/patch/error.ts
- packages/react/runtime/src/snapshot/constants.ts
- packages/react/runtime/test/hooks/useLynxGlobalEventListener.test.jsx
- packages/react/runtime/src/snapshot/workletEvent.ts
- packages/react/runtime/test/lynx/suspense.test.jsx
- packages/react/runtime/test/snapshot/event.test.jsx
- packages/react/runtime/test/debug/printSnapshot.test.jsx
- packages/react/testing-library/src/vitest-global-setup.js
- packages/react/runtime/test/compat/initData.test.jsx
- packages/react/runtime/test/compat.test.jsx
- packages/react/runtime/src/lifecycle/patch/globalState.ts
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/test/lifecycle.test.jsx
- packages/react/runtime/test/preact.test.jsx
- packages/react/runtime/src/snapshot/types.ts
- packages/react/runtime/src/snapshot/utils.ts
- packages/react/runtime/src/debug/printSnapshot.ts
- packages/react/runtime/src/lynx/suspense.ts
- packages/react/runtime/src/lynx/calledByNative.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/react/runtime/test/hydrate.test.jsx
- packages/react/runtime/test/utils/envManager.ts
- packages/react/runtime/src/lifecycle/reload.ts
- packages/react/runtime/test/debug/backgroundSnapshot-profile.test.jsx
- packages/react/runtime/src/internal.ts
- packages/react/runtime/src/lifecycle/patch/commit.ts
HuJean
force-pushed
the
p/refactor-snapshot
branch
from
4e961b6 to
2999b7e
Compare
HuJean
force-pushed
the
p/refactor-snapshot
branch
from
2999b7e to
94c2876
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react/runtime/src/snapshot/definition.ts (1)
30-33:⚠️ Potential issue | 🟠 Major
Snapshot.slotis still narrower than the runtime contract.The exported type only allows
[DynamicPartType, number][], but the snapshot runtime already handles nullable slots and list-holder entries without an index. Keeping the narrower signature forces downstream casts and non-null assertions around a shared public type. As per coding guidelines,**/*.{ts,tsx}: Use TypeScript in strict mode as configured in tsconfig.json for all TypeScript source files.Also applies to: 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/definition.ts` around lines 30 - 33, The Snapshot.slot tuple type is too narrow: update the Snapshot interface's slot declaration so it accepts nullable entries and list-holder tuples that may omit an index (i.e., allow [DynamicPartType] and [DynamicPartType, number] forms and nulls) instead of only [DynamicPartType, number][]. Modify the slot type in the Snapshot interface (exported symbol Snapshot, property slot) to a union type that permits single-element tuples and null entries to match the runtime contract and remove the need for downstream casts/non-null assertions.
🧹 Nitpick comments (2)
packages/react/runtime/src/lifecycle/patch/commit.ts (1)
30-31: Consider combining imports from the same module.These two import statements from
./snapshotPatch.jscan be merged for conciseness.♻️ Suggested change
-import { takeGlobalSnapshotPatch } from './snapshotPatch.js'; -import type { SnapshotPatch } from './snapshotPatch.js'; +import { takeGlobalSnapshotPatch, type SnapshotPatch } from './snapshotPatch.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/lifecycle/patch/commit.ts` around lines 30 - 31, Combine the two imports from snapshotPatch into a single import to reduce redundancy: replace the separate imports of takeGlobalSnapshotPatch and SnapshotPatch in commit.ts with one statement that imports takeGlobalSnapshotPatch and the type SnapshotPatch together from './snapshotPatch.js' (i.e., import { takeGlobalSnapshotPatch, type SnapshotPatch } from './snapshotPatch.js'), removing the duplicate import line.packages/react/runtime/src/snapshot/definition.ts (1)
14-23: Model the cleared page state instead of casting it away.
clearPage()deliberately unsets__page, but the export still claims it is always aFiberElement, and Line 57 needs__page!to suppress that mismatch. Making the typeFiberElement | undefined/nullwould let strict mode catch reads beforesetupPage()has run. As per coding guidelines,**/*.{ts,tsx}: Use TypeScript in strict mode as configured in tsconfig.json for all TypeScript source files.Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/definition.ts` around lines 14 - 23, The exported __page variable is typed as FiberElement but clearPage assigns undefined, causing unsafe non-null assertions elsewhere; change the declaration of __page to allow absence (e.g., export let __page: FiberElement | undefined or FiberElement | null), update any places relying on __page! to handle the possibly undefined/null value (or assert only after setupPage), and leave setupPage and clearPage to set/reset __page and __pageId as they are so strict mode will catch invalid reads before setupPage runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/src/snapshot/definition.ts`:
- Around line 44-47: The registry currently types snapshotManager as values:
Map<string, Snapshot> but code seeds values with a null key (via null as unknown
as string), so change the type to include null: update the declaration to
values: Map<string | null, Snapshot> (and any related type references) and
remove the cast where the null key is inserted or replace it with an explicit
null key (e.g., use null directly) so the Map signature matches the actual keys;
ensure any code using snapshotManager.values is updated to accept string | null
keys and the Snapshot symbol remains unchanged.
---
Duplicate comments:
In `@packages/react/runtime/src/snapshot/definition.ts`:
- Around line 30-33: The Snapshot.slot tuple type is too narrow: update the
Snapshot interface's slot declaration so it accepts nullable entries and
list-holder tuples that may omit an index (i.e., allow [DynamicPartType] and
[DynamicPartType, number] forms and nulls) instead of only [DynamicPartType,
number][]. Modify the slot type in the Snapshot interface (exported symbol
Snapshot, property slot) to a union type that permits single-element tuples and
null entries to match the runtime contract and remove the need for downstream
casts/non-null assertions.
---
Nitpick comments:
In `@packages/react/runtime/src/lifecycle/patch/commit.ts`:
- Around line 30-31: Combine the two imports from snapshotPatch into a single
import to reduce redundancy: replace the separate imports of
takeGlobalSnapshotPatch and SnapshotPatch in commit.ts with one statement that
imports takeGlobalSnapshotPatch and the type SnapshotPatch together from
'./snapshotPatch.js' (i.e., import { takeGlobalSnapshotPatch, type SnapshotPatch
} from './snapshotPatch.js'), removing the duplicate import line.
In `@packages/react/runtime/src/snapshot/definition.ts`:
- Around line 14-23: The exported __page variable is typed as FiberElement but
clearPage assigns undefined, causing unsafe non-null assertions elsewhere;
change the declaration of __page to allow absence (e.g., export let __page:
FiberElement | undefined or FiberElement | null), update any places relying on
__page! to handle the possibly undefined/null value (or assert only after
setupPage), and leave setupPage and clearPage to set/reset __page and __pageId
as they are so strict mode will catch invalid reads before setupPage runs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76cff43c-6265-49f3-8a6f-025e22617ebd
📒 Files selected for processing (59)
.changeset/thirty-snakes-doubt.mdpackages/react/runtime/__test__/clone.test.jsxpackages/react/runtime/__test__/compat.test.jsxpackages/react/runtime/__test__/compat/initData.test.jsxpackages/react/runtime/__test__/debug/backgroundSnapshot-profile.test.jsxpackages/react/runtime/__test__/debug/printSnapshot.test.jsxpackages/react/runtime/__test__/element.test.jsxpackages/react/runtime/__test__/hooks/useLynxGlobalEventListener.test.jsxpackages/react/runtime/__test__/hydrate.test.jsxpackages/react/runtime/__test__/lifecycle.test.jsxpackages/react/runtime/__test__/lynx/suspense.test.jsxpackages/react/runtime/__test__/preact.test.jsxpackages/react/runtime/__test__/render.test.jsxpackages/react/runtime/__test__/snapshot/event.test.jsxpackages/react/runtime/__test__/snapshot/spread.test.jsxpackages/react/runtime/__test__/snapshotPatch.test.jsxpackages/react/runtime/__test__/utils/envManager.tspackages/react/runtime/src/alog/render.tspackages/react/runtime/src/debug/printSnapshot.tspackages/react/runtime/src/document.tspackages/react/runtime/src/hydrate.tspackages/react/runtime/src/internal.tspackages/react/runtime/src/lifecycle/patch/commit.tspackages/react/runtime/src/lifecycle/patch/error.tspackages/react/runtime/src/lifecycle/patch/globalState.tspackages/react/runtime/src/lifecycle/patch/snapshotPatchApply.tspackages/react/runtime/src/lifecycle/patch/updateMainThread.tspackages/react/runtime/src/lifecycle/ref/delay.tspackages/react/runtime/src/lifecycle/reload.tspackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/list.tspackages/react/runtime/src/listUpdateInfo.tspackages/react/runtime/src/lynx/calledByNative.tspackages/react/runtime/src/lynx/injectLepusMethods.tspackages/react/runtime/src/lynx/suspense.tspackages/react/runtime/src/lynx/tt.tspackages/react/runtime/src/opcodes.tspackages/react/runtime/src/root.tspackages/react/runtime/src/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/constants.tspackages/react/runtime/src/snapshot/definition.tspackages/react/runtime/src/snapshot/dynamicPartType.tspackages/react/runtime/src/snapshot/event.tspackages/react/runtime/src/snapshot/gesture.tspackages/react/runtime/src/snapshot/index.tspackages/react/runtime/src/snapshot/list.tspackages/react/runtime/src/snapshot/platformInfo.tspackages/react/runtime/src/snapshot/ref.tspackages/react/runtime/src/snapshot/snapshot.tspackages/react/runtime/src/snapshot/snapshotInstanceHydrationMap.tspackages/react/runtime/src/snapshot/spread.tspackages/react/runtime/src/snapshot/types.tspackages/react/runtime/src/snapshot/utils.tspackages/react/runtime/src/snapshot/workletEvent.tspackages/react/runtime/src/snapshot/workletRef.tspackages/react/testing-library/src/__tests__/act.test.jsxpackages/react/testing-library/src/__tests__/end-to-end.test.jsxpackages/react/testing-library/src/pure.jsxpackages/react/testing-library/src/vitest-global-setup.js
✅ Files skipped from review due to trivial changes (48)
- packages/react/runtime/src/debug/printSnapshot.ts
- packages/react/runtime/src/hydrate.ts
- packages/react/runtime/test/render.test.jsx
- packages/react/testing-library/src/tests/end-to-end.test.jsx
- packages/react/runtime/test/element.test.jsx
- packages/react/runtime/src/lifecycle/patch/snapshotPatchApply.ts
- packages/react/runtime/src/lifecycle/patch/error.ts
- packages/react/testing-library/src/tests/act.test.jsx
- packages/react/runtime/src/listUpdateInfo.ts
- packages/react/runtime/src/lifecycle/patch/updateMainThread.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/testing-library/src/pure.jsx
- packages/react/runtime/src/document.ts
- packages/react/runtime/src/snapshot/workletEvent.ts
- packages/react/runtime/src/alog/render.ts
- packages/react/runtime/src/snapshot/event.ts
- packages/react/runtime/test/snapshotPatch.test.jsx
- packages/react/runtime/src/lynx/suspense.ts
- packages/react/runtime/src/root.ts
- packages/react/runtime/src/list.ts
- packages/react/runtime/test/hooks/useLynxGlobalEventListener.test.jsx
- packages/react/runtime/src/snapshot/workletRef.ts
- packages/react/runtime/src/snapshot/gesture.ts
- packages/react/runtime/test/compat.test.jsx
- packages/react/runtime/src/snapshot/list.ts
- packages/react/runtime/src/lynx/injectLepusMethods.ts
- packages/react/runtime/src/snapshot/ref.ts
- packages/react/runtime/src/snapshot/platformInfo.ts
- packages/react/runtime/test/debug/printSnapshot.test.jsx
- packages/react/runtime/src/snapshot/spread.ts
- packages/react/runtime/test/compat/initData.test.jsx
- packages/react/runtime/src/lifecycle/patch/globalState.ts
- packages/react/runtime/src/snapshot/dynamicPartType.ts
- packages/react/runtime/src/opcodes.ts
- packages/react/runtime/src/lifecycle/ref/delay.ts
- packages/react/runtime/src/snapshot/constants.ts
- packages/react/runtime/test/lynx/suspense.test.jsx
- packages/react/runtime/src/lifecycle/reload.ts
- packages/react/testing-library/src/vitest-global-setup.js
- packages/react/runtime/test/hydrate.test.jsx
- packages/react/runtime/test/lifecycle.test.jsx
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/test/snapshot/event.test.jsx
- packages/react/runtime/src/snapshot/index.ts
- .changeset/thirty-snakes-doubt.md
- packages/react/runtime/src/snapshot/types.ts
- packages/react/runtime/test/preact.test.jsx
- packages/react/runtime/src/snapshot/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/react/runtime/src/lynx/calledByNative.ts
- packages/react/runtime/test/clone.test.jsx
- packages/react/runtime/test/snapshot/spread.test.jsx
- packages/react/runtime/test/utils/envManager.ts
- packages/react/runtime/src/snapshot/utils.ts
- packages/react/runtime/src/internal.ts
- packages/react/runtime/src/snapshot/backgroundSnapshot.ts
Yradex
approved these changes
Mar 27, 2026
upupming
approved these changes
Mar 27, 2026
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
3 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 by CodeRabbit
Refactor
Tests
Checklist