fix(react): run all pending renderComponent before hydrate
#1438
Conversation
|
📝 WalkthroughWalkthroughAdded deferred error handling for a call to preact.process during the firstScreen lifecycle, simplified non-main-thread render by removing debounce interception, and introduced/updated tests to flush pending renders before hydration and to ensure correct Suspense/render behaviors and timing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a timing issue in ReactLynx by ensuring all pending renderComponent operations are processed before hydration occurs. This prevents unwanted node removals when the main thread renders more components than the background thread's root.render.
- Adds a
process()call before hydration to flush pending render operations - Imports the
processfunction from preact to support this functionality
Comments suppressed due to low confidence (1)
packages/react/runtime/src/lynx/tt.ts:4
- The
processfunction is not a standard export from the 'preact' library. Please verify that this function exists in the version of preact being used, or check if it should be imported from a different module or package.
import { process, render } from 'preact';
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/runtime/src/lynx/tt.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts:95-99
Timestamp: 2025-07-16T06:28:26.463Z
Learning: In the lynx-stack codebase, CSS selectors in SSR hydration are generated by their own packages, ensuring a predictable format that makes simple string manipulation safe and preferable over regex for performance reasons.
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.291Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
📚 Learning: the component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork ...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.291Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
Applied to files:
packages/react/runtime/src/lynx/tt.ts
📚 Learning: in the lynx-stack ssr implementation, each createlynxview instance is used to render once and then d...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Applied to files:
packages/react/runtime/src/lynx/tt.ts
🔇 Additional comments (1)
packages/react/runtime/src/lynx/tt.ts (1)
4-4: Check thatprocessis actually exported by the Preact build you ship.Up-stream Preact ( v10.x ) does not export a symbol named
process; common helpers areflush,options, etc.
If your fork / patched build added this export you’re fine—but if not, TS/rollup will fail at compile time.-import { process, render } from 'preact'; +// If the patched Preact exposes `flush`, alias it locally: +// import { flush as process, render } from 'preact';Please confirm the package version; otherwise wrap the import in a local shim to avoid a hard build break.
| process(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing scheduler & clarify intent.
Calling process() flushes all queued renderComponent work before hydration—exactly what this PR intends.
However, if the symbol is absent (see comment above) or returns a Promise, hydration will silently proceed with stale work. Consider:
if (typeof process === 'function') {
// Explicitly flush synchronously; add explanatory comment.
process();
} else {
lynx.reportError(new Error('Expected pre-hydrate scheduler flush is unavailable'));
}Adding the defensive check (or at least a short inline comment) prevents future refactors from accidentally breaking the sequencing guarantee you are establishing here.
🤖 Prompt for AI Agents
In packages/react/runtime/src/lynx/tt.ts around lines 72 to 73, the call to
process() should be guarded to ensure the scheduler exists and is a function.
Modify the code to check if process is a function before calling it; if not,
report an error using lynx.reportError with a clear message indicating the
expected scheduler flush is unavailable. Add an inline comment explaining that
this synchronous flush is required before hydration to maintain correct
sequencing.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
2 times, most recently
from
994d773 to
1dacd1b
Compare
CodSpeed Performance ReportMerging #1438 will not alter performanceComparing Summary
|
Web Explorer#3770 Bundle Size — 342.64KiB (0%).40041d4(current) vs 6baeb9f main#3761(baseline) Bundle metrics
|
| Current #3770 |
Baseline #3761 |
|
|---|---|---|
 Initial JS |
142.33KiB |
142.33KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
 Modules |
212(+0.47%) |
211 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.96% |
3.96% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #3770 |
Baseline #3761 |
|
|---|---|---|
| JS |
227.85KiB |
227.85KiB |
| Other |
82.95KiB |
82.95KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch hzy:p/hzy/process_before_hydrate Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#3777 Bundle Size — 235.3KiB (+0.02%).40041d4(current) vs 6baeb9f main#3769(baseline) Bundle metrics
|
| Current #3777 |
Baseline #3769 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
38.04% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
159 |
159 |
| Duplicate Modules |
64 |
64 |
| Duplicate Code |
45.8%(-0.02%) |
45.81% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #3777 |
Baseline #3769 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
89.54KiB (+0.04%) |
89.5KiB |
Bundle analysis report Branch hzy:p/hzy/process_before_hydrate Project dashboard
Generated by RelativeCI Documentation Report issue
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
1dacd1b to
40041d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react/runtime/__test__/lifecycle/updateData.test.jsx(1 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/lynx/tt.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core/src/uiThread/createRenderAllOnUI.ts:95-99
Timestamp: 2025-07-16T06:28:26.463Z
Learning: In the lynx-stack codebase, CSS selectors in SSR hydration are generated by their own packages, ensuring a predictable format that makes simple string manipulation safe and preferable over regex for performance reasons.
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1238
File: packages/react/runtime/src/debug/component-stack.ts:70-90
Timestamp: 2025-07-18T04:27:18.291Z
Learning: The component-stack.ts file in packages/react/runtime/src/debug/component-stack.ts is a direct fork from https://github.com/preactjs/preact/blob/main/debug/src/component-stack.js. The team prefers to keep it aligned with the upstream Preact version and may contribute improvements back to Preact in the future.
📚 Learning: in the lynx-stack ssr implementation, each createlynxview instance is used to render once and then d...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Applied to files:
packages/react/runtime/__test__/lifecycle/updateData.test.jsx
🔇 Additional comments (2)
packages/react/runtime/__test__/lifecycle/updateData.test.jsx (2)
853-953: Verify test alignment with PR objectives.This test appears to verify that pending renders are processed before hydration (which aligns with the PR objective), but the test structure and assertions should be clearer about this intent. The test verifies that after running pending renders before hydration, no redundant
triggerDataUpdatedflag is sent in the hydration patch.The test logic seems sound for validating the PR's core functionality, but please confirm:
- Does this test adequately verify that pending
renderComponentcalls are executed before hydration?- Is the expected behavior that
triggerDataUpdatedshould NOT be present in the hydration patch when updates were already processed?
845-851: Test setup follows consistent pattern.The beforeEach/afterEach hooks properly set up the
jsReadytiming mode for this test suite, which is consistent with the existing test patterns in the file.
| const initData = useInitData(); | ||
|
|
||
| return <text>{initData.msg}</text>; | ||
| } | ||
|
|
||
| // main thread render | ||
| { | ||
| __root.__jsx = <Comp />; | ||
| renderPage({ msg: 'init' }); | ||
| expect(__root.__element_root).toMatchInlineSnapshot(` | ||
| <page | ||
| cssId="default-entry-from-native:0" | ||
| > | ||
| <text> | ||
| <raw-text | ||
| text="init" | ||
| /> | ||
| </text> | ||
| </page> | ||
| `); | ||
| } | ||
|
|
||
| // main thread updatePage | ||
| { | ||
| __root.__jsx = <Comp />; | ||
| updatePage({ msg: 'update' }); | ||
| expect(__root.__element_root).toMatchInlineSnapshot(` | ||
| <page | ||
| cssId="default-entry-from-native:0" | ||
| > | ||
| <text> | ||
| <raw-text | ||
| text="update" | ||
| /> | ||
| </text> | ||
| </page> | ||
| `); | ||
| } | ||
|
|
||
| // background render | ||
| { | ||
| globalEnvManager.switchToBackground(); | ||
| render(<Comp />, __root); | ||
| } | ||
|
|
||
| // LifecycleConstant.jsReady | ||
| { | ||
| globalEnvManager.switchToMainThread(); | ||
| rLynxJSReady(); | ||
| } | ||
|
|
||
| // background updateCardData | ||
| { | ||
| globalEnvManager.switchToBackground(); | ||
| lynxCoreInject.tt.updateCardData({ msg: 'update' }); | ||
| } | ||
|
|
||
| // hydrate | ||
| { | ||
| globalEnvManager.switchToBackground(); | ||
| // LifecycleConstant.firstScreen | ||
| lynxCoreInject.tt.OnLifecycleEvent(...globalThis.__OnLifecycleEvent.mock.calls[0]); | ||
| } | ||
|
|
||
| // rLynxChange | ||
| { | ||
| globalEnvManager.switchToMainThread(); | ||
| globalThis.__OnLifecycleEvent.mockClear(); | ||
| const rLynxChange = lynx.getNativeApp().callLepusMethod.mock.calls[0]; | ||
| globalThis[rLynxChange[0]](rLynxChange[1]); | ||
| expect(rLynxChange[1]).toMatchInlineSnapshot(` | ||
| { | ||
| "data": "{"patchList":[{"snapshotPatch":[],"id":24}]}", | ||
| "patchOptions": { | ||
| "isHydration": true, | ||
| "pipelineOptions": { | ||
| "dsl": "reactLynx", | ||
| "needTimestamps": true, | ||
| "pipelineID": "pipelineID", | ||
| "pipelineOrigin": "reactLynxHydrate", | ||
| "stage": "hydrate", | ||
| }, | ||
| "reloadVersion": 0, | ||
| }, | ||
| } | ||
| `); | ||
| expect(__root.__element_root).toMatchInlineSnapshot(` | ||
| <page | ||
| cssId="default-entry-from-native:0" | ||
| > | ||
| <text> | ||
| <raw-text | ||
| text="update" | ||
| /> | ||
| </text> | ||
| </page> | ||
| `); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test logic contradicts the test name and description.
The test is named "should not send triggerDataUpdated when updateData after hydration", but the actual test sequence shows updateCardData being called before hydration (line 909), not after. The hydration occurs later on line 916.
This creates confusion about what the test is actually verifying and may not align with the PR objective of ensuring pending renderComponent calls are executed before hydration.
Consider one of these fixes:
Option 1: Fix the test name to match the actual logic:
- it('should not send triggerDataUpdated when updateData after hydration', async function() {
+ it('should not send triggerDataUpdated when updateData before hydration', async function() {Option 2: Fix the test logic to match the name:
Move the updateCardData call to occur after the hydration step if you want to test the "after hydration" scenario.
📝 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.
| describe('flush pending `renderComponent` before hydrate', () => { | |
| beforeEach(() => { | |
| globalThis.__FIRST_SCREEN_SYNC_TIMING__ = 'jsReady'; | |
| }); | |
| afterEach(() => { | |
| globalThis.__FIRST_SCREEN_SYNC_TIMING__ = 'immediately'; | |
| }); | |
| it('should not send triggerDataUpdated when updateData after hydration', async function() { | |
| function Comp() { | |
| const initData = useInitData(); | |
| return <text>{initData.msg}</text>; | |
| } | |
| // main thread render | |
| { | |
| __root.__jsx = <Comp />; | |
| renderPage({ msg: 'init' }); | |
| expect(__root.__element_root).toMatchInlineSnapshot(` | |
| <page | |
| cssId="default-entry-from-native:0" | |
| > | |
| <text> | |
| <raw-text | |
| text="init" | |
| /> | |
| </text> | |
| </page> | |
| `); | |
| } | |
| // main thread updatePage | |
| { | |
| __root.__jsx = <Comp />; | |
| updatePage({ msg: 'update' }); | |
| expect(__root.__element_root).toMatchInlineSnapshot(` | |
| <page | |
| cssId="default-entry-from-native:0" | |
| > | |
| <text> | |
| <raw-text | |
| text="update" | |
| /> | |
| </text> | |
| </page> | |
| `); | |
| } | |
| // background render | |
| { | |
| globalEnvManager.switchToBackground(); | |
| render(<Comp />, __root); | |
| } | |
| // LifecycleConstant.jsReady | |
| { | |
| globalEnvManager.switchToMainThread(); | |
| rLynxJSReady(); | |
| } | |
| // background updateCardData | |
| { | |
| globalEnvManager.switchToBackground(); | |
| lynxCoreInject.tt.updateCardData({ msg: 'update' }); | |
| } | |
| // hydrate | |
| { | |
| globalEnvManager.switchToBackground(); | |
| // LifecycleConstant.firstScreen | |
| lynxCoreInject.tt.OnLifecycleEvent(...globalThis.__OnLifecycleEvent.mock.calls[0]); | |
| } | |
| // rLynxChange | |
| { | |
| globalEnvManager.switchToMainThread(); | |
| globalThis.__OnLifecycleEvent.mockClear(); | |
| const rLynxChange = lynx.getNativeApp().callLepusMethod.mock.calls[0]; | |
| globalThis[rLynxChange[0]](rLynxChange[1]); | |
| expect(rLynxChange[1]).toMatchInlineSnapshot(` | |
| { | |
| "data": "{"patchList":[{"snapshotPatch":[],"id":24}]}", | |
| "patchOptions": { | |
| "isHydration": true, | |
| "pipelineOptions": { | |
| "dsl": "reactLynx", | |
| "needTimestamps": true, | |
| "pipelineID": "pipelineID", | |
| "pipelineOrigin": "reactLynxHydrate", | |
| "stage": "hydrate", | |
| }, | |
| "reloadVersion": 0, | |
| }, | |
| } | |
| `); | |
| expect(__root.__element_root).toMatchInlineSnapshot(` | |
| <page | |
| cssId="default-entry-from-native:0" | |
| > | |
| <text> | |
| <raw-text | |
| text="update" | |
| /> | |
| </text> | |
| </page> | |
| `); | |
| } | |
| }); | |
| }); | |
| it('should not send triggerDataUpdated when updateData before hydration', async function() { |
🤖 Prompt for AI Agents
In packages/react/runtime/__test__/lifecycle/updateData.test.jsx between lines
844 and 954, the test named "should not send triggerDataUpdated when updateData
after hydration" incorrectly calls updateCardData before hydration,
contradicting the test name and intent. To fix this, either rename the test to
reflect that updateCardData is called before hydration or move the
updateCardData call to after the hydration step to properly test the "after
hydration" scenario as described.
Run all pending `renderComponent` before hydrate, which ensures some immediate update can be applied in `hydrate`. As background info, ReactLynx will use tree in background-thread as the source-of-truth, so this PR is helpful if main-thread renders more than background-thread's `root.render` by avoiding unwanted node removals.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
40041d4 to
23c3d37
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react/runtime/__test__/lifecycle/updateData.test.jsx (1)
877-978: Test name contradicts the flow: update happens before hydration, not afterIn this new suite,
updateCardData({ msg: 'update' })is invoked before the hydration step, but the test name says “after hydration”. This mirrors an earlier issue flagged on the same file.Choose one:
- Rename to reflect actual behavior (“before hydration”), or
- Move the
updateCardDatacall to after the hydration rLynxChange is applied.Apply one of the following diffs:
Option A. Rename test to match logic:
- it('should not send triggerDataUpdated when updateData after hydration', async function() { + it('should not send triggerDataUpdated when updateData before hydration', async function() {Option B. Move update after hydration (ensures “after hydration” is tested):
- // background updateCardData - { - globalEnvManager.switchToBackground(); - lynxCoreInject.tt.updateCardData({ msg: 'update' }); - } - // hydrate { globalEnvManager.switchToBackground(); // LifecycleConstant.firstScreen lynxCoreInject.tt.OnLifecycleEvent(...globalThis.__OnLifecycleEvent.mock.calls[0]); } // rLynxChange { globalEnvManager.switchToMainThread(); globalThis.__OnLifecycleEvent.mockClear(); const rLynxChange = lynx.getNativeApp().callLepusMethod.mock.calls[0]; globalThis[rLynxChange[0]](rLynxChange[1]); expect(rLynxChange[1]).toMatchInlineSnapshot(` { "data": "{\"patchList\":[{\"snapshotPatch\":[],\"id\":24}]}", "patchOptions": { "isHydration": true, "pipelineOptions": { "dsl": "reactLynx", "needTimestamps": true, "pipelineID": "pipelineID", "pipelineOrigin": "reactLynxHydrate", "stage": "hydrate", }, "reloadVersion": 0, }, } `); expect(__root.__element_root).toMatchInlineSnapshot(` <page cssId="default-entry-from-native:0" > <text> <raw-text text="update" /> </text> </page> `); } + // background updateCardData (after hydration) + { + globalEnvManager.switchToBackground(); + lynx.getNativeApp().callLepusMethod.mockClear(); + lynxCoreInject.tt.updateCardData({ msg: 'update' }); + await waitSchedule(); + expect(lynx.getNativeApp().callLepusMethod).toHaveBeenCalledTimes(1); + expect( + JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data) + ).not.toHaveProperty('flushOptions.triggerDataUpdated'); + }
🧹 Nitpick comments (5)
packages/react/runtime/__test__/render.test.jsx (1)
64-65: Flush pending renders to realize synchronous state loopsCalling
process()afterroot.render(<Comp />)ensures the self-schedulingsetStateloop settles before assertions. This makes the "synchronously" test meaningful and stable.If you expect to add more tests needing flushes, consider a small helper to abstract the preact scheduler detail:
- root.render(<Comp />); - process(); + root.render(<Comp />); + // Encapsulate scheduler flush to avoid direct preact-coupling across tests. + process();You can later move the wrapper to a shared test util without changing call sites.
packages/react/runtime/__test__/lynx/suspense.test.jsx (3)
250-305: Snapshot brittleness: assert operations, not IDsThe updated inline snapshot includes specific element IDs (e.g., 2, 3, 7, -3). These are often incidental and may change with scheduler or traversal tweaks, causing flaky tests.
Prefer asserting the sequence and types of operations and key attributes while relaxing exact IDs. For example, parse
snapshotPatchand assert the presence/order of:
- CreateElement(wrapper)
- InsertBefore relationships (parentId -1 to wrapper)
- RemoveChild of the old placeholder
- CreateElement of the content card
452-458: Background snapshot key expectations: verify stability across runsThe expected keys changed to include 2 and keep -1. If these keys are implementation details, consider asserting only presence/absence patterns (e.g., non-empty positive IDs and a sentinel root id) rather than exact values to reduce flakiness.
491-496: AssertingbackgroundSnapshotInstanceManagercontains-1and element typeThe check
expect(backgroundSnapshotInstanceManager.values.get(4).type).toBe('div');is good, but the reliance on ID 4 is brittle. Prefer querying for the element by type/ownership if possible, or assert that one of the remaining instances has type 'div'.packages/react/runtime/__test__/lifecycle/updateData.test.jsx (1)
943-976: Avoid false positives: assert call count and index the hydration call deterministicallyBefore reading
mock.calls[0], assert the expected call count to ensure you’re asserting against the hydration call, not an earlier update emission.// rLynxChange { globalEnvManager.switchToMainThread(); globalThis.__OnLifecycleEvent.mockClear(); - const rLynxChange = lynx.getNativeApp().callLepusMethod.mock.calls[0]; + expect(lynx.getNativeApp().callLepusMethod).toHaveBeenCalledTimes(1); + const rLynxChange = lynx.getNativeApp().callLepusMethod.mock.calls[0]; globalThis[rLynxChange[0]](rLynxChange[1]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/react/runtime/__test__/lifecycle/updateData.test.jsx(1 hunks)packages/react/runtime/__test__/lynx/suspense.test.jsx(9 hunks)packages/react/runtime/__test__/render.test.jsx(2 hunks)packages/react/runtime/src/lynx-api.ts(1 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/lynx/tt.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/react/runtime/__test__/lifecycle/updateData.test.jsx (4)
packages/react/runtime/src/lynx-api.ts (1)
useInitData(169-169)packages/react/runtime/__test__/utils/envManager.ts (1)
globalEnvManager(86-86)packages/react/runtime/src/renderToOpcodes/index.ts (1)
render(333-333)packages/react/runtime/__test__/snapshot/event.test.jsx (2)
rLynxJSReady(1139-1139)rLynxChange(184-184)
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
packages/react/runtime/src/lynx/suspense.ts (1)
Suspense(14-44)
🔇 Additional comments (4)
packages/react/runtime/__test__/render.test.jsx (1)
5-5: Good addition: explicitly import and useprocessto flush pending updatesImporting
processfrom preact aligns with the PR objective to flush pending renders before hydration. This keeps the test deterministic.packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
359-369: Minor JSX formatting change LGTMWrapping the returned JSX in an extra pair of parentheses with a short-circuit expression is fine and equivalent. The semantics for falsy rendering remain correct in Preact.
1304-1308: Child content passed as children toSuspenderLGTMSwitching to
<Suspender>{content}</Suspender>simplifies diffing and keeps the test intent clear.packages/react/runtime/src/lynx-api.ts (1)
90-99: Background-thread render: verified — no prod debounce hooks; process() is called before hydrateQuick findings:
- Repo search:
options.debounceRenderingis only set in tests (packages/react/runtime/test/lynx/timing.test.jsx) — no production interception remains.- onLifecycleEventImpl calls process() before hydrate (packages/react/runtime/src/lynx/tt.ts:
try { process(); } catch (e) { processErr = e; }), then performs hydration and callsflushDelayedLifecycleEvents().- root.render background path remains unchanged (packages/react/runtime/src/lynx-api.ts) — it renders on the background thread and either calls
flushDelayedLifecycleEvents()when__FIRST_SCREEN_SYNC_TIMING__ === 'immediately'or signalsjsReadyotherwise.Conclusion: follow-ups satisfied; no changes required.
| // background updateCardData | ||
| { | ||
| globalEnvManager.switchToBackground(); | ||
| lynxCoreInject.tt.updateCardData({ msg: 'update' }); | ||
| } | ||
|
|
||
| // hydrate |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertion: prove no background rLynxChange is dispatched pre-hydration
To validate the PR’s intent (pending updates are flushed into hydration rather than sent early), explicitly assert that no rLynxChange call is made from updateCardData before hydration.
// background updateCardData
{
globalEnvManager.switchToBackground();
- lynxCoreInject.tt.updateCardData({ msg: 'update' });
+ lynx.getNativeApp().callLepusMethod.mockClear();
+ lynxCoreInject.tt.updateCardData({ msg: 'update' });
+ await waitSchedule();
+ expect(lynx.getNativeApp().callLepusMethod).toHaveBeenCalledTimes(0);
}📝 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.
| // background updateCardData | |
| { | |
| globalEnvManager.switchToBackground(); | |
| lynxCoreInject.tt.updateCardData({ msg: 'update' }); | |
| } | |
| // hydrate | |
| // background updateCardData | |
| { | |
| globalEnvManager.switchToBackground(); | |
| lynx.getNativeApp().callLepusMethod.mockClear(); | |
| lynxCoreInject.tt.updateCardData({ msg: 'update' }); | |
| await waitSchedule(); | |
| expect(lynx.getNativeApp().callLepusMethod).toHaveBeenCalledTimes(0); | |
| } | |
| // hydrate |
Run all pending
renderComponentbefore hydrate, which ensures some immediate update can be applied inhydrate.As background info, ReactLynx will use tree in background-thread as the source-of-truth, so this PR is helpful if main-thread renders more than background-thread's
root.renderby avoiding unwanted node removals.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist