fix(react): run all pending renderComponent before hydrate#1438
fix(react): run all pending renderComponent before hydrate#1438hzy merged 1 commit intolynx-family:mainfrom
renderComponent before hydrate#1438Conversation
|
📝 WalkthroughWalkthroughDefers throwing errors from a background Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ 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.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
2 times, most recently
from
994d773 to
1dacd1b
Compare
CodSpeed Performance ReportMerging #1438 will degrade performances by 13.99%Comparing Summary
Benchmarks breakdown
Footnotes |
Web Explorer#4794 Bundle Size — 367.43KiB (0%).8d705f3(current) vs fd90b20 main#4773(baseline) Bundle metrics
|
| Current #4794 |
Baseline #4773 |
|
|---|---|---|
 Initial JS |
144.23KiB |
144.23KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
218(-0.46%) |
219 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.33% |
3.33% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4794 |
Baseline #4773 |
|
|---|---|---|
| JS |
235.43KiB |
235.43KiB |
| Other |
100.16KiB |
100.16KiB |
| 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#4803 Bundle Size — 237.5KiB (+0.19%).8d705f3(current) vs fd90b20 main#4782(baseline) Bundle metrics
|
| Current #4803 |
Baseline #4782 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
38.51% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
160(+1.27%) |
158 |
 Duplicate Modules |
65(+1.56%) |
64 |
| Duplicate Code |
45.83% |
45.83% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #4803 |
Baseline #4782 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.74KiB (+0.48%) |
91.3KiB |
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.
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.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
23c3d37 to
4ca897e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react/runtime/src/lynx/tt.ts (1)
4-4: Confirm Preact’sprocessexport and sync flush semantics.Good direction. Please verify that our Preact build exports a synchronous
process()that flushes all queued renders. Optional: alias toflushto avoid confusion with Node’s globalprocess.Run:
#!/bin/bash # 1) Call sites importing `process` from preact rg -nP --type ts --type tsx -C2 "\bimport\s*\{[^}]*\bprocess\b[^}]*\}\s*from\s*['\"]preact['\"]" # 2) Any shim/re-export that defines `process` rg -nP -C2 "^\s*export\s+(?:const|function)\s+process\b|module\.exports\s*=\s*{[^}]*\bprocess\b" # 3) Sanity-check other imports from 'preact' in this repo (look for local shims/aliases) rg -nP --type ts --type tsx -C2 "from\s*['\"]preact(?:\/[^'\"]+)?['\"]"
🧹 Nitpick comments (1)
packages/react/runtime/src/lynx/tt.ts (1)
72-77: Type the deferred error and document the sequencing.Make the intent explicit and avoid implicit any when rethrowing.
- let processErr; - try { - process(); - } catch (e) { - processErr = e; - } + // Flush all queued renders before hydration so the main thread never runs ahead + // of the background thread's root.render. This must be synchronous. + let processErr: Error | undefined; + try { + process(); + } catch (e) { + processErr = e instanceof Error ? e : new Error(String(e)); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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 (4)
- packages/react/runtime/test/lifecycle/updateData.test.jsx
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/test/render.test.jsx
- packages/react/runtime/test/lynx/suspense.test.jsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.
Applied to files:
packages/react/runtime/src/lynx/tt.ts
📚 Learning: 2025-08-21T08:46:54.456Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.456Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/react/runtime/src/lynx/tt.ts
🧬 Code graph analysis (1)
packages/react/runtime/src/lynx/tt.ts (1)
packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: CodeQL Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/react/runtime/src/lynx/tt.ts (1)
138-140: LGTM on deferring the throw until after hydration/commit.The outer onLifecycleEvent catch will report the error. Ordering here preserves hydration sequencing while still surfacing the failure.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
2 times, most recently
from
29dc802 to
ae5751e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
249-304: Deflake: avoid hard-coded snapshot IDs; also align assertions with the comment about PreventDestroyThe inline snapshot hard-codes element IDs (2, 3, -1, -3, 7). These IDs are implementation details and will keep changing as we tweak scheduling/hydration ordering, causing flaky tests. Also, the test comment mentions asserting a PreventDestroy op, but the code doesn't assert it at all.
Suggest asserting operation types (and key structural relations) instead of raw IDs, and explicitly checking for (or documenting the absence of) PreventDestroy.
Apply this diff to stabilize the assertion:
- const data = JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data).patchList[0].snapshotPatch; - expect(prettyFormatSnapshotPatch(data)).toMatchInlineSnapshot(` - [ - { - "id": 2, - "op": "CreateElement", - "type": "wrapper", - }, - { - "id": 3, - "op": "CreateElement", - "type": "__Card__:__snapshot_a94a8_test_4", - }, - { - "id": 3, - "op": "SetAttributes", - "values": [ - "an attr", - ], - }, - { - "beforeId": null, - "childId": 3, - "op": "InsertBefore", - "parentId": 2, - }, - { - "beforeId": null, - "childId": 2, - "op": "InsertBefore", - "parentId": -1, - }, - { - "childId": -3, - "op": "RemoveChild", - "parentId": -1, - }, - { - "id": 7, - "op": "CreateElement", - "type": "__Card__:__snapshot_a94a8_test_5", - }, - { - "beforeId": null, - "childId": 7, - "op": "InsertBefore", - "parentId": 3, - }, - { - "beforeId": null, - "childId": 2, - "op": "InsertBefore", - "parentId": -1, - }, - ] - `); + const patch = JSON.parse( + lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data + ).patchList[0].snapshotPatch; + const ops = patch.map(p => p.op); + expect(ops).toEqual( + expect.arrayContaining(["CreateElement", "InsertBefore", "RemoveChild"]) + ); + // If PreventDestroy is still part of the protocol, assert it explicitly: + // expect(ops).toContain("PreventDestroy");If you adopt the change above, remove the now-unused import:
- import { prettyFormatSnapshotPatch } from '../../src/debug/formatPatch';Would you like me to follow up with a helper that validates parent/child relationships without relying on concrete IDs?
♻️ Duplicate comments (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
521-530: Same: explicit null instead of boolean returnsMirror the ternary style for consistency with the earlier test.
- return ( - show && ( - <Suspense fallback={<text>loading</text>}> - <view attr={`an attr`}> - <Suspender> - <text>foo</text> - </Suspender> - </view> - </Suspense> - ) - ); + return show ? ( + <Suspense fallback={<text>loading</text>}> + <view attr={`an attr`}> + <Suspender> + <text>foo</text> + </Suspender> + </view> + </Suspense> + ) : null;
🧹 Nitpick comments (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
359-369: Prefer explicit null over boolean returnsUsing show && (...) returns false when hidden. Be explicit to improve readability and avoid accidental boolean render values.
- return ( - show && ( - <Suspense fallback={<text>loading</text>}> - <view attr={`an attr`}> - <Suspender> - <text>foo</text> - </Suspender> - </view> - </Suspense> - ) - ); + return show ? ( + <Suspense fallback={<text>loading</text>}> + <view attr={`an attr`}> + <Suspender> + <text>foo</text> + </Suspender> + </view> + </Suspense> + ) : null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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(2 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/test/render.test.jsx
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/test/lifecycle/updateData.test.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/__test__/lynx/suspense.test.jsx
🧬 Code graph analysis (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (3)
packages/react/runtime/src/index.ts (1)
Suspense(85-85)packages/react/runtime/src/lynx/suspense.ts (1)
Suspense(14-44)packages/react/runtime/__test__/lifecycle/updateData.test.jsx (8)
rLynxChange(104-104)rLynxChange(159-159)rLynxChange(232-232)rLynxChange(431-431)rLynxChange(613-613)rLynxChange(668-668)rLynxChange(825-825)rLynxChange(947-947)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: CodeQL Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
895-895: LGTM: callback is invoked to complete the rLynxChange cycleConsistent with the other tests and required to flush the background-side callback.
1306-1306: LGTM: pass dynamic children through SuspenderKeeps the test focused on content replacement without altering the boundary behavior.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
ae5751e to
65cbf2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
451-458: Stop asserting exact background snapshot keys; assert invariants instead.Same concern previously raised; assert presence of root sentinel and shape, not exact IDs.
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 2, - 3, - 4, - -1, - 7, - ] - `); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + expect(keys).toEqual(expect.arrayContaining([-1])); // root sentinel + expect(keys.filter(k => k < 0 && k !== -1)).toHaveLength(0); // no extra negatives + expect(keys.some(k => k >= 0)).toBe(true); // has concrete elements + }
492-496: Same: avoid exact key ordering/IDs for background manager.Use structural assertions; also already suggested earlier.
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 4, - -1, - ] - `); - expect(backgroundSnapshotInstanceManager.values.get(4).type).toBe('div'); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + const positive = keys.filter(k => k >= 0); + expect(keys).toEqual(expect.arrayContaining([-1])); + expect(positive).toHaveLength(1); + expect(backgroundSnapshotInstanceManager.values.get(positive[0]).type).toBe('div'); + }packages/react/runtime/__test__/lifecycle/updateData.test.jsx (1)
877-879: Test name contradicts sequence; also missing explicit assertion abouttriggerDataUpdatedabsence.The test calls
updateCardDatabefore hydration, but the name says “after hydration”. Also, explicitly assert no pre-hydration rLynxChange and thatflushOptions.triggerDataUpdatedis absent in the hydration payload.- it('should not send triggerDataUpdated when updateData after hydration', async function() { + it('should not send triggerDataUpdated when updateData before hydration (pending renders flushed into hydrate)', async function() { @@ - // background updateCardData + // background updateCardData (should not dispatch rLynxChange pre-hydration) { globalEnvManager.switchToBackground(); - lynxCoreInject.tt.updateCardData({ msg: 'update' }); + lynx.getNativeApp().callLepusMethod.mockClear(); + lynxCoreInject.tt.updateCardData({ msg: 'update' }); + await waitSchedule(); + expect(lynx.getNativeApp().callLepusMethod).toHaveBeenCalledTimes(0); } @@ - 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, - }, - } - `); + const rLynxChange = lynx.getNativeApp().callLepusMethod.mock.calls[0]; + // Assert no triggerDataUpdated in hydration payload + expect(rLynxChange[1].flushOptions).toBeUndefined(); + // Parse and assert hydration options without depending on IDs + const payload = rLynxChange[1]; + const body = JSON.parse(payload.data); + expect(body).toHaveProperty('patchList'); + expect(Array.isArray(body.patchList)).toBe(true); + expect(payload.patchOptions).toMatchObject({ + isHydration: true, + pipelineOptions: expect.objectContaining({ + pipelineOrigin: 'reactLynxHydrate', + stage: 'hydrate', + }), + }); + globalThis[rLynxChange[0]](payload); expect(__root.__element_root).toMatchInlineSnapshot(` <page cssId="default-entry-from-native:0" > <text> <raw-text text="update" /> </text> </page> `);Also applies to: 930-934, 949-976
🧹 Nitpick comments (4)
packages/react/runtime/__test__/render.test.jsx (1)
5-5: Alias preact’s process to avoid shadowing Node’s globalprocess.Prevents confusion and accidental misuse in tests and future edits.
-import { render, Component, process } from 'preact'; +import { render, Component, process as flushPreact } from 'preact'; @@ - process(); + flushPreact();Also applies to: 64-64
packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
249-304: Deflake patch assertions: avoid relying on exact element IDs in snapshot.The inline snapshot over concrete IDs (e.g., 2, 3, -3) is brittle. Prefer structural checks on op/type/relationships.
- const data = JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data).patchList[0].snapshotPatch; - expect(prettyFormatSnapshotPatch(data)).toMatchInlineSnapshot(` - [ - { - "id": 2, - "op": "CreateElement", - "type": "wrapper", - }, - ... - { - "childId": -3, - "op": "RemoveChild", - "parentId": -1, - }, - ... - ] - `); + const ops = JSON.parse( + lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data + ).patchList[0].snapshotPatch; + // Must create a wrapper and a card element, then insert card under wrapper, + // and mount wrapper under root; no assumptions on numeric IDs. + expect(ops.some(o => o.op === 'CreateElement' && o.type === 'wrapper')).toBe(true); + expect(ops.some(o => o.op === 'CreateElement' && String(o.type).startsWith('__Card__'))).toBe(true); + expect(ops.some(o => o.op === 'InsertBefore' && o.parentId !== -1)).toBe(true); // insert card into wrapper + expect(ops.some(o => o.op === 'InsertBefore' && o.parentId === -1)).toBe(true); // mount wrapper under root + // Removing old placeholder is optional depending on impl; don't assert IDs.packages/react/runtime/__test__/lifecycle/updateData.test.jsx (2)
693-787: Nice coverage for jsReady path; consider asserting zero BG dispatch pre-hydration consistently.You already clear mocks and assert 0 calls once in this suite. Apply the same pattern wherever pre-hydration BG updates happen.
15-28: Test setup/teardown looks solid. Consider spyingpreact.processto prove flush-before-hydrate path executes.Optional: Spy and assert
processinvoked during firstScreen handling to directly verify the PR’s contract.Outside-this-hunk import example:
-import { Component, render } from 'preact'; +import * as Preact from 'preact'; +import { Component, render } from 'preact';Inside the new "flush pending ..." test, before hydration:
const processSpy = vi.spyOn(Preact as any, 'process'); ... lynxCoreInject.tt.OnLifecycleEvent(...globalThis.__OnLifecycleEvent.mock.calls[0]); expect(processSpy).toHaveBeenCalled(); processSpy.mockRestore();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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(2 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/src/lynx-api.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/__test__/lynx/suspense.test.jsx
🧬 Code graph analysis (2)
packages/react/runtime/__test__/lifecycle/updateData.test.jsx (2)
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/__test__/lynx/suspense.test.jsx (2)
packages/react/runtime/src/index.ts (1)
Suspense(85-85)packages/react/runtime/src/lynx/suspense.ts (1)
Suspense(14-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
359-369: LGTM: JSX grouping is clearer and semantically equivalent.No functional change; improves readability.
1303-1307: LGTM: Passingcontentas children is idiomatic.Simplifies the component shape without changing behavior.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
65cbf2b to
02e32f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
450-458: Deflake snapshot-instance keys: assert invariants, not exact IDsReiterating earlier feedback: exact key arrays are brittle across scheduler/ID changes.
Reuse the previously suggested diff:
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 2, - 3, - 4, - -1, - 7, - ] - `); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + // Root sentinel must exist + expect(keys).toEqual(expect.arrayContaining([-1])); + // No stray negative IDs besides the root + expect(keys.filter(k => k < 0 && k !== -1)).toHaveLength(0); + // There should be at least one concrete element + expect(keys.some(k => k >= 0)).toBe(true); + }
492-496: Deflake (part 2): avoid pinning to concrete instance ID 4The positive ID can shift; query it instead.
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 4, - -1, - ] - `); - expect(backgroundSnapshotInstanceManager.values.get(4).type).toBe('div'); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + const positive = keys.filter(k => k >= 0); + expect(keys).toEqual(expect.arrayContaining([-1])); + expect(positive).toHaveLength(1); + expect(backgroundSnapshotInstanceManager.values.get(positive[0]).type).toBe('div'); + }
🧹 Nitpick comments (3)
packages/react/runtime/__test__/lynx/suspense.test.jsx (3)
249-305: Deflake: assert structural invariants instead of full pretty-printed patch snapshotInline-snapshotting the entire patch makes the test brittle to ID allocation and minor ordering changes introduced by “process pending renders before hydrate”. Prefer partial object matchers on the parsed patch.
Apply this diff:
- const data = JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data).patchList[0].snapshotPatch; - expect(prettyFormatSnapshotPatch(data)).toMatchInlineSnapshot(` - [ - { - "id": 2, - "op": "CreateElement", - "type": "wrapper", - }, - { - "id": 3, - "op": "CreateElement", - "type": "__Card__:__snapshot_a94a8_test_4", - }, - { - "id": 3, - "op": "SetAttributes", - "values": [ - "an attr", - ], - }, - { - "beforeId": null, - "childId": 3, - "op": "InsertBefore", - "parentId": 2, - }, - { - "beforeId": null, - "childId": 2, - "op": "InsertBefore", - "parentId": -1, - }, - { - "childId": -3, - "op": "RemoveChild", - "parentId": -1, - }, - { - "id": 7, - "op": "CreateElement", - "type": "__Card__:__snapshot_a94a8_test_5", - }, - { - "beforeId": null, - "childId": 7, - "op": "InsertBefore", - "parentId": 3, - }, - { - "beforeId": null, - "childId": 2, - "op": "InsertBefore", - "parentId": -1, - }, - ] - `); + const patch = JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data).patchList[0].snapshotPatch; + // Assert shape, not volatile IDs/order. + expect(patch).toEqual(expect.arrayContaining([ + expect.objectContaining({ op: 'CreateElement', type: 'wrapper' }), + expect.objectContaining({ op: 'CreateElement', type: expect.stringMatching(/^__Card__:/) }), + expect.objectContaining({ op: 'InsertBefore', parentId: -1 }), + ]));Note: The test description mentions a PreventDestroy op; the current assertion no longer checks for it. Either update the description or add a specific matcher for that op if still expected.
359-369: Prefer explicit ternary over short-circuit for JSX visibilityTernary reads clearer in tests and avoids returning boolean false.
- return ( - show && ( - <Suspense fallback={<text>loading</text>}> - <view attr={`an attr`}> - <Suspender> - <text>foo</text> - </Suspender> - </view> - </Suspense> - ) - ); + return show ? ( + <Suspense fallback={<text>loading</text>}> + <view attr={`an attr`}> + <Suspender> + <text>foo</text> + </Suspender> + </view> + </Suspense> + ) : null;
521-531: Repeat: ternary over short-circuit for JSXSame readability/explicitness rationale as above.
- return ( - show && ( - <Suspense fallback={<text>loading</text>}> - <view attr={`an attr`}> - <Suspender> - <text>foo</text> - </Suspender> - </view> - </Suspense> - ) - ); + return show ? ( + <Suspense fallback={<text>loading</text>}> + <view attr={`an attr`}> + <Suspender> + <text>foo</text> + </Suspender> + </view> + </Suspense> + ) : null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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(2 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react/runtime/test/render.test.jsx
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/test/lifecycle/updateData.test.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/__test__/lynx/suspense.test.jsx
🧬 Code graph analysis (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
packages/react/runtime/src/lynx/suspense.ts (1)
Suspense(14-44)packages/react/runtime/__test__/lifecycle/updateData.test.jsx (8)
rLynxChange(104-104)rLynxChange(159-159)rLynxChange(232-232)rLynxChange(431-431)rLynxChange(613-613)rLynxChange(668-668)rLynxChange(825-825)rLynxChange(947-947)
🔇 Additional comments (2)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
893-897: Good: invoke rLynxChange callback and drain timers to complete the cycleEnsures background callback side-effects are applied before proceeding.
1304-1307: No-op JSX formatting change is fineInlining children via braces is equivalent; no functional impact.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
02e32f0 to
127e43e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/react/runtime/src/lynx/tt.ts (1)
73-79: Guardprocess()and keep sequencing intent obviousAdd a type/availability guard and an inline comment so future refactors don’t silently skip the pre-hydrate flush.
- let processErr; - try { - process(); - } catch (e) { - processErr = e; - } + // Flush any queued background renders synchronously before hydration. + let processErr: unknown; + if (typeof process === 'function') { + try { process(); } catch (e) { processErr = e; } + } else { + lynx.reportError(new Error('pre-hydrate flush unavailable: preact.process is not a function')); + }packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
451-458: Deflake: assert invariants, not exact snapshot IDsExact instance ID lists are brittle. Assert structure instead (root sentinel present, no stray negatives, at least one positive; or validate the positive node’s type).
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 2, - 3, - 4, - -1, - 7, - ] - `); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + expect(keys).toEqual(expect.arrayContaining([-1])); + expect(keys.filter(k => k < 0 && k !== -1)).toHaveLength(0); + expect(keys.some(k => k >= 0)).toBe(true); + }
492-496: Deflake follow-up: avoid pinning specific positive IDSame rationale; assert exactly one positive id remains and validate its type.
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 4, - -1, - ] - `); - expect(backgroundSnapshotInstanceManager.values.get(4).type).toBe('div'); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + const positive = keys.filter(k => k >= 0); + expect(keys).toEqual(expect.arrayContaining([-1])); + expect(positive).toHaveLength(1); + expect(backgroundSnapshotInstanceManager.values.get(positive[0]).type).toBe('div'); + }
🧹 Nitpick comments (3)
packages/react/runtime/src/lynx/tt.ts (2)
4-4: Type safety forprocessimportDoes your
preacttyping exposeprocess? If not, tsc will fail. Either add an ambient type or read it off the namespace with a typed fallback.Example ambient typing (new file types/preact-process.d.ts):
declare module 'preact' { export function process(): void; }Or inline fallback:
-import { process, render } from 'preact'; +import { render } from 'preact'; +import * as Preact from 'preact'; +const process = (Preact as any).process as undefined | (() => void);
136-138: Preserve observability when deferring the errorSince you’re deferring the throw, drop a timing marker (or breadcrumb) before rethrowing to aid debugging.
- if (processErr) { - throw processErr; - } + if (processErr) { + markTiming('preHydrateProcessError'); + throw processErr; + }packages/react/runtime/__test__/render.test.jsx (1)
5-5: Encapsulate scheduler flush for test stabilityRelying on
preact.processdirectly makes tests couple to an internal export. Wrap it in a tiny helper with a fallback to a microtask tick so tests don’t break if the export changes.-import { render, Component, process } from 'preact'; +import { render, Component, process } from 'preact'; +const flush = () => (typeof process === 'function' ? process() : undefined); @@ - process(); + await (async () => flush() ?? Promise.resolve())();If you prefer, move
flushto a shared test util.Also applies to: 64-65
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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(2 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/test/lifecycle/updateData.test.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/__test__/lynx/suspense.test.jsx
🧬 Code graph analysis (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
packages/react/runtime/src/index.ts (1)
Suspense(85-85)packages/react/runtime/src/lynx/suspense.ts (1)
Suspense(14-44)
🔇 Additional comments (5)
packages/react/runtime/__test__/lynx/suspense.test.jsx (5)
251-304: Suspicious duplicate InsertBefore; consider dedup or correct positioningPatch shows two InsertBefore ops for childId 2 into parent -1 (both with beforeId: null). That’s likely redundant and could mis-order siblings in non-trivial trees. Prefer a single InsertBefore with beforeId set to the node being replaced (e.g., -3), then RemoveChild -3.
Would you confirm the commit/patch composer dedups/moves correctly? If not, we should fix emission to avoid double insertions.
359-369: LGTM: clearer JSX with groupingWrapping the conditional return is fine and improves readability.
521-530: LGTM: JSX groupingSame readability win here. No functional change.
893-896: No-op formatting changeNo behavioral impact here.
1304-1307: LGTM: child passthrough via<Suspender>Switch to
{content}is correct and keeps the test focused on update semantics.
hzy
force-pushed
the
p/hzy/process_before_hydrate
branch
from
127e43e to
1792c25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
451-458: Deflake keys assertion in background snapshot manager (repeat of earlier feedback)Avoid matching exact IDs; assert invariants (root sentinel exists, no extra negatives, at least one positive).
- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 2, - 3, - 4, - -1, - 7, - ] - `); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + expect(keys).toEqual(expect.arrayContaining([-1])); // root sentinel + expect(keys.filter(k => k < 0 && k !== -1)).toHaveLength(0); // no stray negatives + expect(keys.some(k => k >= 0)).toBe(true); // at least one concrete element + }
492-496: Deflake final keys assertion and avoid hard-coded ID in type checkDon’t pin
[4,-1]or the literal4when checking type.- expect([...backgroundSnapshotInstanceManager.values.keys()]).toMatchInlineSnapshot(` - [ - 4, - -1, - ] - `); - expect(backgroundSnapshotInstanceManager.values.get(4).type).toBe('div'); + { + const keys = [...backgroundSnapshotInstanceManager.values.keys()]; + expect(keys).toEqual(expect.arrayContaining([-1])); + const positive = keys.filter(k => k >= 0); + expect(positive).toHaveLength(1); + const only = positive[0]; + expect(backgroundSnapshotInstanceManager.values.get(only).type).toBe('div'); + }
🧹 Nitpick comments (2)
packages/react/runtime/__test__/render.test.jsx (1)
5-5: Wrap directprocessusage behind a tiny test util to decouple from Preact internalsImporting
processfrompreactties tests to a non-public API. Consider a localflushPendingRenders()(callsprocess()if available) to keep tests resilient; oract()frompreact/test-utilsif compatible.packages/react/runtime/__test__/lynx/suspense.test.jsx (1)
250-304: Deflake: avoid exact patch ID snapshots; assert structural invariants insteadInline snapshots with concrete IDs are brittle (IDs/order change easily). Assert structure/ops instead. Also, the test comment mentions
PreventDestroybut the snapshot doesn't; reconcile comment or add an assertion for the intended op.Apply this refactor within this block:
- const data = JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data).patchList[0].snapshotPatch; - expect(prettyFormatSnapshotPatch(data)).toMatchInlineSnapshot(` - [ - { - "id": 2, - "op": "CreateElement", - "type": "wrapper", - }, - ... - ] - `); + const data = JSON.parse(lynx.getNativeApp().callLepusMethod.mock.calls[0][1].data).patchList[0].snapshotPatch; + // Required ops exist + const ops = data.map(p => p.op); + expect(ops).toEqual(expect.arrayContaining(['CreateElement', 'InsertBefore'])); + // A wrapper element is created and inserted at root + const wrapperCreate = data.find(p => p.op === 'CreateElement' && p.type === 'wrapper'); + expect(wrapperCreate).toBeTruthy(); + expect( + data.some(p => p.op === 'InsertBefore' && p.parentId === -1 && p.childId === wrapperCreate.id) + ).toBe(true); + // Background host container is created and attached under wrapper + expect( + data.some(p => p.op === 'CreateElement' && String(p.type).includes('__Card__')) + ).toBe(true); + // If relevant, ensure a previous negative sentinel is removed (don’t pin exact ID) + expect( + data.some(p => p.op === 'RemoveChild' && p.parentId === -1 && p.childId < 0) + ).toBe(true); + // Optional: if `PreventDestroy` is the required behavior, assert it explicitly + // expect(data.some(p => p.op === 'PreventDestroy')).toBe(true);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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(2 hunks)packages/react/runtime/src/lynx/tt.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/test/lifecycle/updateData.test.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/__test__/lynx/suspense.test.jsx
🧬 Code graph analysis (1)
packages/react/runtime/__test__/lynx/suspense.test.jsx (2)
packages/react/runtime/src/index.ts (1)
Suspense(85-85)packages/react/runtime/src/lynx/suspense.ts (1)
Suspense(14-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / clippy
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: code-style-check
- GitHub Check: CodeQL Analyze (javascript-typescript)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: zizmor
🔇 Additional comments (5)
packages/react/runtime/__test__/render.test.jsx (1)
62-66: LGTM: deterministic flushCalling
process()right afterroot.rendermakes the sync loop test deterministic (asserting88).packages/react/runtime/__test__/lynx/suspense.test.jsx (4)
359-369: LGTM: parenthesized conditional return improves readabilitySemantic no-op; keeps style consistent.
521-531: LGTM: consistent parentheses for conditional renderKeeps style uniform with the earlier case.
895-895: No functional changeWhitespace/position-only tweak around the callback; nothing to review.
1306-1306: LGTM: passcontentdirectly asSuspenderchildSimplifies JSX shape without changing semantics.
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
1792c25 to
8d705f3
Compare
- **fix(react): run all pending `renderComponent` before hydrate (#1438)** - **ci(benchmark/react): enable list in benchmark** - **ci(benchmark/react): add benchmark for various attribute update** <!-- Thank you for submitting a pull request! We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request. Upon submission, your pull request will be automatically assigned with reviewers. If you want to learn more about contributing to this project, please visit: https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md. --> <!-- The AI summary below will be auto-generated - feel free to replace it with your own. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * New Features * Added a new React benchmark case that exercises a variety of attribute updates with runtime timing, enabling more granular performance insights. * Introduced dedicated scripts to run and profile this benchmark case for quicker benchmarking and trace capture. * Chores * Updated benchmark configuration to register the new case in the build and run pipeline. * Exposed utilities for determining execution context to support benchmark behavior across environments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Checklist <!--- Check and mark with an "x" --> - [ ] Tests updated (or not required). - [ ] Documentation updated (or not required). - [ ] Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).
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