refactor(react/runtime): use lynx.performance.profileStart#1533
refactor(react/runtime): use lynx.performance.profileStart#1533hzy merged 1 commit intolynx-family:mainfrom
lynx.performance.profileStart#1533Conversation
|
📝 WalkthroughWalkthroughReplaces PROFILE-guarded console.profile/console.profileEnd with new profiling hooks ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 refactors profiling instrumentation throughout the React/runtime package to use the new lynx.performance.profileStart API instead of the native console.profile and console.profileEnd methods. The changes introduce centralized profiling utilities and provide better performance monitoring capabilities.
- Replaces all
console.profile/console.profileEndcalls withlynx.performance.profileStart/profileEnd - Creates a new debug utilities module with wrapper functions for profiling
- Adds a
runWithProfilehelper function for automatic profile start/end management
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react/runtime/src/debug/utils.ts | New module providing profiling utilities and wrapper functions |
| packages/react/runtime/src/utils.ts | Adds noop utility function for fallback scenarios |
| Multiple runtime files | Replace console profiling calls with new lynx.performance API usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
packages/react/runtime/src/lifecycle/patch/commit.ts (1)
184-205: Ensure profiling is balanced even on exceptions (use try/finally).If JSON.stringify or subsequent logic throws,
profileEnd()won’t run, leaving an open slice in profilers. Wrap the block with try/finally.if (__PROFILE__) { profileStart('ReactLynx::commitChanges'); } - markTiming('packChangesStart'); - const obj: { - data: string; - patchOptions: PatchOptions; - } = { - data: JSON.stringify(patchList), - patchOptions: { - ...patchOptions, - reloadVersion: getReloadVersion(), - }, - }; - markTiming('packChangesEnd'); - if (globalPipelineOptions) { - obj.patchOptions.pipelineOptions = globalPipelineOptions; - setPipeline(undefined); - } - if (__PROFILE__) { - profileEnd(); - } - - return obj; + try { + markTiming('packChangesStart'); + const obj: { + data: string; + patchOptions: PatchOptions; + } = { + data: JSON.stringify(patchList), + patchOptions: { + ...patchOptions, + reloadVersion: getReloadVersion(), + }, + }; + markTiming('packChangesEnd'); + if (globalPipelineOptions) { + obj.patchOptions.pipelineOptions = globalPipelineOptions; + setPipeline(undefined); + } + return obj; + } finally { + if (__PROFILE__) { + profileEnd(); + } + }Optional: You could also use
runWithProfile('ReactLynx::commitChanges', () => { ... })to remove the__PROFILE__checks here and centralize logic.packages/react/runtime/src/lifecycle/destroy.ts (1)
12-33: Balance profiling with try/finally to handle unexpected errors.If any step throws,
profileEnd()won’t execute. Wrap the body in try/finally.function destroyBackground(): void { if (__PROFILE__) { profileStart('ReactLynx::destroyBackground'); } - - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - render(null, __root as any); - - globalCommitTaskMap.forEach(task => { - task(); - }); - globalCommitTaskMap.clear(); - // Clear delayed events which should not be executed after destroyed. - // This is important when the page is performing a reload. - delayedLifecycleEvents.length = 0; - if (delayedEvents) { - delayedEvents.length = 0; - } - if (__PROFILE__) { - profileEnd(); - } + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-argument + render(null, __root as any); + + globalCommitTaskMap.forEach(task => { + task(); + }); + globalCommitTaskMap.clear(); + // Clear delayed events which should not be executed after destroyed. + // This is important when the page is performing a reload. + delayedLifecycleEvents.length = 0; + if (delayedEvents) { + delayedEvents.length = 0; + } + } finally { + if (__PROFILE__) { + profileEnd(); + } + } }packages/react/runtime/src/lifecycle/reload.ts (1)
56-63: Include jsReadyEventIdSwap in firstScreen reload event to avoid receiver crashtt.ts dereferences jsReadyEventIdSwap during event replay. Sending only { root } risks a runtime error. Ensure the payload includes a defined swap map.
Suggested change (outside the changed hunk):
__OnLifecycleEvent([ LifecycleConstant.firstScreen, { root: JSON.stringify(__root), jsReadyEventIdSwap: {}, // or import from jsReady.ts if available }, ]);Complementary hardening on the receiving side is also advisable: default destructure
jsReadyEventIdSwap = {}and guard the while loop.
🧹 Nitpick comments (4)
packages/react/runtime/src/utils.ts (1)
83-83: Add a brief JSDoc and mark as internal.This is a simple no-op used as a safe fallback by the profiling utilities. A tiny doc comment helps future readers and avoids accidental public consumption.
-export function noop(): void {} +/** + * No-op function used as a safe fallback for optional hooks (e.g., profiling). + * @internal + */ +export function noop(): void {}packages/react/runtime/src/lynx/env.ts (1)
53-56: Unify profiling slice naming for consistency.Other modules use the “ReactLynx::” prefix (e.g., commit/destroy). Consider aligning this label for easier filtering in tooling.
- if (__PROFILE__) { - profileStart('processData'); - } + if (__PROFILE__) { + profileStart('ReactLynx::processData'); + }Also applies to: 73-74
packages/react/runtime/src/backgroundSnapshot.ts (1)
264-265: Ensure profileEnd is always paired, even on exceptionsThere are multiple exits and potential throws inside setAttribute. If an exception occurs before hitting the explicit profileEnd, the profiling session would remain open. Consider wrapping the method body with a try/finally or using runWithProfile for this method to guarantee profileEnd is invoked.
Example pattern (illustrative, outside the changed hunk):
if (__PROFILE__) { profileStart('ReactLynx::BSI::setAttribute'); } try { // existing body (including early returns) } finally { if (__PROFILE__) { profileEnd(); } }Or:
return runWithProfile('ReactLynx::BSI::setAttribute', () => { // existing body (including early returns) });Also applies to: 283-284
packages/react/runtime/src/lifecycle/reload.ts (1)
30-31: Prefer runWithProfile or try/finally to guarantee profileEnd on exceptionsStarting and ending profiling under PROFILE is fine, but if an exception occurs mid-function, profileEnd won’t run. Consider runWithProfile or a try/finally block in both reloadMainThread and reloadBackground.
Example (illustrative):
runWithProfile('ReactLynx::reloadMainThread', () => { // current body without the __PROFILE__ guards });or
if (__PROFILE__) profileStart('ReactLynx::reloadBackground'); try { // current body } finally { if (__PROFILE__) profileEnd(); }Also applies to: 75-76, 92-93
📜 Review details
Configuration used: .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 settings in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(2 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/src/utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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.tspackages/react/runtime/src/lifecycle/render.tspackages/react/runtime/src/debug/utils.ts
🧬 Code Graph Analysis (9)
packages/react/runtime/src/lifecycle/destroy.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-18)profileEnd(20-29)
packages/react/runtime/src/lifecycle/reload.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-18)profileEnd(20-29)
packages/react/runtime/src/lifecycle/patch/commit.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-18)profileEnd(20-29)
packages/react/runtime/src/lynx/env.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-18)profileEnd(20-29)
packages/react/runtime/src/lifecycle/event/jsReady.ts (4)
packages/react/runtime/src/lynx-api.ts (1)
root(83-116)packages/react/runtime/src/debug/utils.ts (1)
runWithProfile(31-42)packages/react/runtime/src/internal.ts (1)
__root(18-18)packages/web-platform/web-tests/shell-project/mainthread-test.ts (1)
__OnLifecycleEvent(147-148)
packages/react/runtime/src/lynx/tt.ts (5)
packages/react/runtime/src/debug/utils.ts (1)
runWithProfile(31-42)packages/react/runtime/src/lifecycleConstant.ts (1)
FirstScreenData(13-16)packages/react/runtime/src/lynx/performance.ts (4)
beginPipeline(161-161)PipelineOrigins(157-157)PerformanceTimingFlags(156-156)markTiming(162-162)packages/react/runtime/src/snapshot.ts (1)
SerializedSnapshotInstance(260-266)packages/react/runtime/src/backgroundSnapshot.ts (2)
hydrate(364-509)BackgroundSnapshotInstance(36-362)
packages/react/runtime/src/backgroundSnapshot.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-18)profileEnd(20-29)
packages/react/runtime/src/lifecycle/render.ts (3)
packages/react/runtime/src/debug/utils.ts (1)
runWithProfile(31-42)packages/react/runtime/src/renderToOpcodes/index.ts (1)
renderToString(44-84)packages/react/runtime/src/opcodes.ts (1)
renderOpcodesInto(100-155)
packages/react/runtime/src/debug/utils.ts (1)
packages/react/runtime/src/utils.ts (1)
noop(83-83)
🔇 Additional comments (13)
packages/react/runtime/src/lifecycle/patch/commit.ts (1)
34-34: LGTM: switched to centralized profiling utilities.Importing from
../../debug/utils.jsaligns this module with the new profiling API.packages/react/runtime/src/lifecycle/destroy.ts (1)
10-10: LGTM: import new profiling utilities.packages/react/runtime/src/lynx/env.ts (1)
4-4: LGTM: centralized profiling import.packages/react/runtime/src/lifecycle/event/jsReady.ts (1)
4-4: LGTM: centralized profiling importImporting runWithProfile from debug/utils aligns with the new profiling strategy and keeps instrumentation consistent across modules.
packages/react/runtime/src/backgroundSnapshot.ts (2)
13-13: LGTM: switched to profileStart/profileEndImporting the centralized helpers is consistent with the new profiling approach.
214-217: Correct replacement of console.profile with profileStartProfiling begins only when PROFILE is truthy, matching the intended gating.
packages/react/runtime/src/lynx/tt.ts (3)
11-11: LGTM: adopt runWithProfileImport aligns with the shared profiling utilities and reduces duplication.
55-61: LGTM: profile lifecycle dispatch wrapperWrapping OnLifecycleEvent with runWithProfile preserves the error reporting and guarantees balanced start/end, improving observability without altering behavior.
69-82: LGTM: scoped hydration profiling with proper timing marksThe hydrate path is neatly isolated under a single profiling slice, with timing marks intact and the result returned from within the wrapper. Any errors bubble to the outer OnLifecycleEvent try/catch.
packages/react/runtime/src/lifecycle/render.ts (3)
11-11: LGTM: centralized profiling importBrings render.ts in line with the unified profiling utilities.
18-27: LGTM: renderToOpcodes profiling with preserved error handlingThe wrapper cleanly encapsulates the work; errors are still reported and the fallback [] is maintained.
38-41: LGTM: profile the renderOpcodesInto phaseClear delineation of phases improves profiling clarity and mirrors the earlier section.
packages/react/runtime/src/lifecycle/reload.ts (1)
26-26: LGTM: profiling helpers importConsistent with the rest of the runtime instrumentation.
CodSpeed Performance ReportMerging #1533 will degrade performances by 14.22%Comparing Summary
Benchmarks breakdown
Footnotes |
Web Explorer#4790 Bundle Size — 367.43KiB (0%).fcc4317(current) vs fd90b20 main#4773(baseline) Bundle metrics
|
| Current #4790 |
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 #4790 |
Baseline #4773 |
|
|---|---|---|
| JS |
235.43KiB |
235.43KiB |
| Other |
100.16KiB |
100.16KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch hzy:p/hzy/migrate_to_performance... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4799 Bundle Size — 237.57KiB (+0.21%).fcc4317(current) vs fd90b20 main#4782(baseline) Bundle metrics
|
| Current #4799 |
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.84%(+0.02%) |
45.83% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #4799 |
Baseline #4782 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.81KiB (+0.56%) |
91.3KiB |
Bundle analysis report Branch hzy:p/hzy/migrate_to_performance... Project dashboard
Generated by RelativeCI Documentation Report issue
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
4028f37 to
0802c1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react/runtime/vitest.config.ts (2)
77-92: Consolidate debug exclusions with a single glob to reduce maintenanceInstead of listing each debug file, exclude the folder. This avoids future churn when adding more profiling/debug files.
Apply this diff:
'src/root.ts', - 'src/debug/component-stack.ts', - 'src/debug/debug.ts', - 'src/debug/utils.ts', + 'src/debug/**', 'src/lynx/calledByNative.ts',Note: If the top-level 'debug' entry (Line 78) was intended to cover src/debug, it likely doesn’t. You can keep it if there is a separate top-level debug directory in this package; otherwise consider removing it in a follow-up.
2-2: Align Node built-in import style with repo convention (default imports for built-ins)Per repository preference, use default imports for Node built-ins instead of namespace imports.
-import * as path from 'node:path'; +import path from 'node:path';
📜 Review details
Configuration used: .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 settings in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(2 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/lifecycle/event/jsReady.ts
- packages/react/runtime/src/lynx/env.ts
- packages/react/runtime/src/debug/utils.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/src/lifecycle/reload.ts
- packages/react/runtime/src/lynx/tt.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/runtime/vitest.config.ts
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/runtime/vitest.config.ts
📚 Learning: 2025-07-18T04:27:18.291Z
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/vitest.config.ts
📚 Learning: 2025-08-13T11:46:43.697Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1523
File: vitest.config.ts:5-6
Timestamp: 2025-08-13T11:46:43.697Z
Learning: In the lynx-stack codebase, default imports are consistently used for Node.js built-in modules (e.g., `import os from 'node:os'`, `import fs from 'node:fs'`). The TypeScript configuration supports esModuleInterop and allowSyntheticDefaultImports, making default imports the preferred pattern over namespace imports for Node.js built-ins.
Applied to files:
packages/react/runtime/vitest.config.ts
🔇 Additional comments (2)
packages/react/runtime/vitest.config.ts (2)
91-91: LGTM: Excluding src/debug/utils.ts from coverage is appropriate for debug-only profiling wrappersThis keeps 100% coverage thresholds intact while avoiding noise from environment-gated code. No runtime behavior impact.
91-91: Confirmed: excluded file exists and exports the profiling helpersFound packages/react/runtime/src/debug/utils.ts — it exports profileStart, profileEnd, and runWithProfile, and is imported by:
- packages/react/runtime/src/backgroundSnapshot.ts (imports profileEnd, profileStart)
Vitest config exclusion (unchanged):
'src/debug/utils.ts',
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
0802c1f to
723181e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/react/runtime/src/lynx/tt.ts (1)
69-82: Hydration profiling is correct; consider guarding optional jsReadyEventIdSwap usage.The runWithProfile wrapper correctly scopes hydration work and timings. One edge case: reloadMainThread triggers firstScreen with only { root }, but later code consumes jsReadyEventIdSwap when flushing delayedEvents. If delayedEvents exists during reload, indexing jsReadyEventIdSwap[idStr] will throw when jsReadyEventIdSwap is undefined.
Recommendation (local guard):
If this scenario is possible, guard the while loop to avoid crashes. Example outside the changed lines:
// Before (Line ~91) while (jsReadyEventIdSwap[idStr!]) idStr = jsReadyEventIdSwap[idStr!]?.toString(); // After const swap = jsReadyEventIdSwap ?? {}; while (swap[idStr!]) { idStr = swap[idStr!]!.toString(); }Minor nit: avoid reusing the same identifier name inside and outside the profile block to reduce shadowing. Example diff within this hunk:
- const snapshotPatch = hydrate( + const patch = hydrate( before, __root as BackgroundSnapshotInstance, ); markTiming('diffVdomEnd'); - return snapshotPatch; + return patch;packages/react/runtime/src/lifecycle/reload.ts (4)
30-31: Start profiling on reloadMainThread — consider try/finally for guaranteed end.Current start/end calls are fine. To ensure a balanced end even on errors, wrap the body with try/finally.
Example outside the changed lines:
let __profileActive = false; if (__PROFILE__) { profileStart('ReactLynx::reloadMainThread'); __profileActive = true; } try { // ... existing body ... } finally { if (__profileActive) profileEnd(); }Also optional: since profileStart/profileEnd are no-ops when performance hooks are missing, the explicit PROFILE guard is redundant. Keeping the guard is fine if you prefer the minimal call-site overhead.
68-69: Balanced profiling end on reloadMainThread.Matches the start and stays gated under PROFILE. If you adopt try/finally as suggested above, this explicit end call would move into the finally block.
75-76: Start profiling on reloadBackground — same try/finally suggestion applies.Gated start is correct. Consider wrapping the function body with try/finally to guarantee profileEnd.
Example outside the changed lines:
let __profileActive = false; if (__PROFILE__) { profileStart('ReactLynx::reloadBackground'); __profileActive = true; } try { // ... existing body ... } finally { if (__profileActive) profileEnd(); }
92-93: Balanced profiling end on reloadBackground.Symmetric with start. The try/finally refactor above would make this robust to early throws as well.
📜 Review details
Configuration used: .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 (11)
packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(2 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/lifecycle/event/jsReady.ts
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/runtime/src/lynx/env.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- packages/react/runtime/src/debug/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/react/runtime/src/lifecycle/reload.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
packages/react/runtime/src/lynx/tt.ts (5)
packages/react/runtime/src/debug/utils.ts (1)
runWithProfile(25-36)packages/react/runtime/src/lifecycleConstant.ts (1)
FirstScreenData(13-16)packages/react/runtime/src/lynx/performance.ts (4)
beginPipeline(161-161)PipelineOrigins(157-157)PerformanceTimingFlags(156-156)markTiming(162-162)packages/react/runtime/src/snapshot.ts (1)
SerializedSnapshotInstance(260-266)packages/react/runtime/src/backgroundSnapshot.ts (2)
hydrate(364-509)BackgroundSnapshotInstance(36-362)
🔇 Additional comments (3)
packages/react/runtime/src/lynx/tt.ts (2)
11-11: Centralized profiling import looks good.Adopting runWithProfile from the shared debug utils aligns this file with the PR’s profiling approach. Keeps instrumentation consistent and tree-shakeable under the PROFILE flag.
55-61: Good use of runWithProfile wrapping with internal try/catch.Catching inside the callback ensures errors are reported while the finally in runWithProfile guarantees profileEnd is called. Behavior mirrors previous console.profile blocks and is safer on exceptions.
Confirm that dropping profiling around the commit phase was intentional. If you still want coverage there, consider wrapping the commit section with runWithProfile('ReactLynx::commit', () => { ... }).
packages/react/runtime/src/lifecycle/reload.ts (1)
26-26: Switch to shared profiling utils is consistent and future-proof.Importing profileStart/profileEnd from the utilities centralizes instrumentation and avoids direct console.* usage. Good alignment with the rest of the PR.
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
723181e to
347e3c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/react/runtime/src/lynx/tt.ts (1)
75-90: Avoid dangling inner profile when hydrate path throws by wrapping in try/finally.If an error occurs between
profileStart('ReactLynx::hydrate')and the currentprofileEnd()(e.g., duringbeginPipeline, JSON.parse, orhydrate), control jumps to the caller’s catch and only oneprofileEnd()is invoked there, which would close the innermost profile and leave the outerOnLifecycleEventprofile open (or vice versa depending on stack semantics). Guard the inner start/end with a local try/finally to keep the profiling stack balanced.Apply this diff:
- if (__PROFILE__) { - profileStart('ReactLynx::hydrate'); - } - beginPipeline(true, PipelineOrigins.reactLynxHydrate, PerformanceTimingFlags.reactLynxHydrate); - markTiming('hydrateParseSnapshotStart'); - const before = JSON.parse(lepusSide) as SerializedSnapshotInstance; - markTiming('hydrateParseSnapshotEnd'); - markTiming('diffVdomStart'); - const snapshotPatch = hydrate( - - before, - - __root as BackgroundSnapshotInstance, - - ); - if (__PROFILE__) { - profileEnd(); - } + let snapshotPatch!: ReturnType<typeof hydrate>; + if (__PROFILE__) { + profileStart('ReactLynx::hydrate'); + } + try { + beginPipeline(true, PipelineOrigins.reactLynxHydrate, PerformanceTimingFlags.reactLynxHydrate); + markTiming('hydrateParseSnapshotStart'); + const before = JSON.parse(lepusSide) as SerializedSnapshotInstance; + markTiming('hydrateParseSnapshotEnd'); + markTiming('diffVdomStart'); + snapshotPatch = hydrate( + before, + __root as BackgroundSnapshotInstance, + ); + } finally { + if (__PROFILE__) { + profileEnd(); + } + } markTiming('diffVdomEnd');
♻️ Duplicate comments (1)
packages/react/runtime/src/lifecycle/event/jsReady.ts (1)
18-27: Fix: Always send a defined jsReadyEventIdSwap and avoid root naming confusionReceivers index into jsReadyEventIdSwap and will throw if it’s undefined. Also, naming the local variable “root” is easily confused with __root. Send a defined map and clarify naming.
Apply this diff:
- const root = JSON.stringify(__root); + const serializedRoot = JSON.stringify(__root); @@ - root, - jsReadyEventIdSwap, + root: serializedRoot, + jsReadyEventIdSwap: jsReadyEventIdSwap ?? {},Optional (extra safety): If __OnLifecycleEvent could be async and keep a reference, consider sending a shallow copy to avoid post-dispatch mutation effects:
jsReadyEventIdSwap: { ...(jsReadyEventIdSwap ?? {}) },Run this to audit other firstScreen dispatchers for missing jsReadyEventIdSwap fields:
#!/bin/bash set -euo pipefail echo "All call sites of LifecycleConstant.firstScreen with nearby payload context:" rg -n -C5 $'LifecycleConstant\\.firstScreen' -- packages | sed 's/^/ /' echo echo "Heuristic: show occurrences where jsReadyEventIdSwap is not present within nearby context:" rg -n -C5 $'LifecycleConstant\\.firstScreen' -- packages \ | awk 'BEGIN{block=""} /^[^:]+:\d+:/ {print block; block=$0; next} {block=block RS $0} END{print block}' \ | rg -n $'jsReadyEventIdSwap' -n || trueIf you prefer to address defensively at the receiver too, make tt.ts destructure with a default:
const { root: lepusSide, jsReadyEventIdSwap = {} } = data as FirstScreenData;
🧹 Nitpick comments (3)
packages/react/runtime/src/lifecycle/event/jsReady.ts (2)
8-10: Initialize module-level state to explicit defaultsMake the contract explicit and avoid undefined states on first use.
-let isJSReady: boolean; -let jsReadyEventIdSwap: Record<string | number, number>; +let isJSReady: boolean = false; +let jsReadyEventIdSwap: Record<string | number, number> = {};
14-21: Explicitly end named profiles; optionally ensure balanced profiles with try/finallyTo be explicit and resilient across implementations, pass the label to profileEnd. Optionally, ensure the outer profile is always closed even if __OnLifecycleEvent throws.
Minimal, explicit labels:
if (__PROFILE__) { profileStart('ReactLynx::transferRoot'); profileStart('ReactLynx::serializeRoot'); } const serializedRoot = JSON.stringify(__root); if (__PROFILE__) { - profileEnd(); + profileEnd('ReactLynx::serializeRoot'); } @@ if (__PROFILE__) { - profileEnd(); + profileEnd('ReactLynx::transferRoot'); }Optional (more robust): wrap the outer section in try/finally to guarantee closing the 'transferRoot' profile even if __OnLifecycleEvent throws.
Can you confirm lynx.performance.profileEnd accepts an optional label? If it requires one, the above change is essential.
Also applies to: 29-31
packages/react/runtime/src/lynx/tt.ts (1)
59-67: Prefer try/finally to guaranteeprofileEnd()pairing at the event level.While the catch logs and execution continues to the current
profileEnd(), a future early-return or refactor after the try/catch could accidentally skip ending the outer profile. A finally block makes the pairing robust.Apply this diff:
try { onLifecycleEventImpl(type, data); } catch (e) { lynx.reportError(e as Error); - } - - if (__PROFILE__) { - profileEnd(); - } + } finally { + if (__PROFILE__) { + profileEnd(); + } + }
📜 Review details
Configuration used: .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 (11)
packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(3 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(5 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/runtime/src/debug/utils.ts
- packages/react/runtime/src/lynx/env.ts
- packages/react/runtime/src/lifecycle/reload.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/react/runtime/src/lifecycle/event/jsReady.ts (3)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)packages/react/runtime/src/lynx-api.ts (1)
root(83-116)packages/react/runtime/src/internal.ts (1)
__root(18-18)
packages/react/runtime/src/lynx/tt.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
🔇 Additional comments (3)
packages/react/runtime/src/lifecycle/event/jsReady.ts (1)
4-4: LGTM: adoption of profileStart/profileEnd utilitiesImport looks consistent with the new debug utils and PROFILE gating.
packages/react/runtime/src/lynx/tt.ts (2)
11-11: Profiling utils import looks correct and consistent with ESM/NodeNext style.The wrappers are typed and safely no-op when
lynx.performanceisn’t available. No concerns here.
55-57: Event-level profiling start is well-scoped.Labeling with
OnLifecycleEvent::${type}under__PROFILE__is clean and avoids overhead when disabled.
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
347e3c9 to
bb63b5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react/runtime/src/lynx/env.ts (1)
55-56: Always pair start/end; move profileEnd to a finally and pass the labelGuarantees the end marker even if error reporting throws; passing the label avoids ambiguity if multiple profiles are active.
try { if (processorName) { r = dataProcessorDefinition?.dataProcessors?.[processorName]?.(data) as InitData ?? data; } else { r = dataProcessorDefinition?.defaultDataProcessor?.(data) ?? data; } } catch (e: any) { lynx.reportError(e as Error); // when there is an error // we should perform like dataProcessor returns nothing // so use `{}` rather than `data` r = {}; - } - - if (__PROFILE__) { - profileEnd(); - } + } finally { + if (__PROFILE__) { + profileEnd('processData'); + } + }If profileEnd does not accept a label, keep it no-arg but still move it into the finally block.
Also applies to: 73-75
📜 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 (11)
packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(3 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(5 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/debug/utils.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/src/lifecycle/event/jsReady.ts
- packages/react/runtime/src/lifecycle/reload.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react/runtime/src/lynx/env.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
🔇 Additional comments (1)
packages/react/runtime/src/lynx/env.ts (1)
4-4: Import looks correct; verify TS module resolutionUsing a .js extension from a .ts file is fine under NodeNext/Bundler ESM. Please confirm tsconfig/moduleResolution aligns so this resolves in all builds (node, bundler, tests).
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
bb63b5a to
612b0c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/react/runtime/src/lynx-api.ts (1)
98-105: Always pair profileStart/profileEnd even on exceptionsIf
render(jsx, __root as any)throws,profileEnd()won’t run, leaving an open profiler section. Wrap with a nestedtry/finallyso end is guaranteed.- try { - if (__PROFILE__) { - profileStart('ReactLynx::renderBackground'); - } - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - render(jsx, __root as any); - if (__PROFILE__) { - profileEnd(); - } - (preactProcess as (() => void) | undefined)?.(); - } finally { + try { + let __started = false; + if (__PROFILE__) { + profileStart('ReactLynx::renderBackground'); + __started = true; + } + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-argument + render(jsx, __root as any); + } finally { + if (__started) { + profileEnd(); + } + } + (preactProcess as (() => void) | undefined)?.(); + } finally { options.debounceRendering = oldDebounceRendering!; }benchmark/react/src/patchProfile.ts (2)
12-27: Ignore rules: avoid caching under the 'undefined' keySmall tidy-up: don’t store
ignored[undefined]. Compute the ignore flag directly whennameis falsy.- hook(lynx.performance, 'profileStart', (old, name, option) => { + hook(lynx.performance, 'profileStart', (old, name, option) => { old!.call(lynx.performance, name, option); - if ((ignored[name] ??= shouldIgnoreBenchmark(name))) { + const ignore = name ? (ignored[name] ??= shouldIgnoreBenchmark(name)) : true; + if (ignore) { stack.push('__IGNORED__'); } else { stack.push(name); depth++; if (depth > 1) { - console.log( + console.warn( `Benchmark ${name} is ignored because it is nested, the stack: ${ stack.join(', ') }`, ); } else { Codspeed.startBenchmark(); } } });
48-65: Guard against stack underflow and negative depthAdd a defensive check to avoid runtime errors if
profileEndfires without a matchingprofileStart(e.g., in test flakiness). Also ensuredepthnever goes negative.- hook(lynx.performance, 'profileEnd', (old) => { - const name = stack.pop()!; + hook(lynx.performance, 'profileEnd', (old) => { + const name = stack.pop(); + if (!name) { + // No matching start; fall back to original behavior. + old!.call(lynx.performance); + return; + } if (name === '__IGNORED__') { // Codspeed.zeroStats(); } else { - if (depth > 1) { + if (depth > 1) { // ignored } else { Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${PREFIX}::${__webpack_chunkname__}-${name.replace(/::/g, '__')}`, - ); + const chunk = (typeof __webpack_chunkname__ === 'string' && __webpack_chunkname__) || 'unknown'; + Codspeed.setExecutedBenchmark(`${PREFIX}::${chunk}-${name.replace(/::/g, '__')}`); } - depth--; + depth = Math.max(0, depth - 1); } old!.call(lynx.performance); });benchmark/react/lynx.config.js (1)
13-21: Ensure minification is truly off during profiling
js: trueconflicts withminimizerOptions.minify: false(many minifiers don’t recognize aminifyflag inside options). If the goal is fully unminified output for stable profiles, disable JS minification at the top level.output: { - minify: { - js: true, - jsOptions: { - minimizerOptions: { - mangle: false, - minify: false, - }, - }, - }, + minify: { + js: false, + },Action: Build once and confirm the emitted bundles retain whitespace and identifiers. If you still need partial minification, use the minifier’s concrete flags (e.g., disable compress/mangle) instead of a generic
minifytoggle inside options.
📜 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 (16)
benchmark/react/lynx.config.js(1 hunks)benchmark/react/src/RunBenchmarkUntil.tsx(1 hunks)benchmark/react/src/patchProfile.ts(1 hunks)packages/lynx/benchx_cli/scripts/build.mjs(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(3 hunks)packages/react/runtime/src/lynx-api.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(5 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/react/runtime/src/debug/utils.ts
- packages/react/runtime/src/lifecycle/reload.ts
- packages/react/runtime/src/lynx/tt.ts
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/react/runtime/src/lifecycle/event/jsReady.ts
- packages/react/runtime/src/lynx/env.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
packages/lynx/benchx_cli/scripts/build.mjs
📚 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-api.ts
📚 Learning: 2025-08-21T08:46:54.494Z
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.494Z
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-api.ts
📚 Learning: 2025-08-11T05:57:18.212Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/testing-library/testing-environment/src/index.ts:255-258
Timestamp: 2025-08-11T05:57:18.212Z
Learning: In the ReactLynx testing environment (`packages/testing-library/testing-environment/src/index.ts`), the dual assignment pattern `target.console.method = console.method = () => {}` is required for rstest compatibility. This is because rstest provides `console` in an IIFE (Immediately Invoked Function Expression), and both the target and global console need to have these methods defined for proper test execution.
Applied to files:
packages/react/runtime/src/lynx-api.ts
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Applied to files:
benchmark/react/lynx.config.js
📚 Learning: 2025-08-13T11:36:12.075Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1523
File: vitest.config.ts:52-72
Timestamp: 2025-08-13T11:36:12.075Z
Learning: The lynx-stack project requires Node.js >=22 as specified in package.json engines, so Node.js compatibility fallbacks for features introduced before v22 are unnecessary.
Applied to files:
benchmark/react/lynx.config.js
🔇 Additional comments (5)
benchmark/react/src/RunBenchmarkUntil.tsx (2)
5-6: Import source is correct for Lynx hooksUsing
@lynx-js/reactis consistent with the rest of the benchmark code. No issues.
11-17: Simple mount toggle for stop condition looks goodClean and sufficient for the benchmark signal.
packages/react/runtime/src/lynx-api.ts (1)
10-10: Housekeeping: changeset likely requiredThis PR touches
packages/react/runtime/src/**. Per repo CI conventions, add a (possibly empty) changeset to satisfy checks.If applicable, add a minimal changeset file under
.changeset/(as used previously for internal-only runtime refactors).benchmark/react/src/patchProfile.ts (1)
29-46: Codspeed types/global declarationsIf not already present in ambient types, declare globals for
Codspeed,__webpack_chunkname__, and__REPO_FILEPATH__to keep TS happy in strict mode.If needed, add a
global.d.tsunderbenchmark/react/:declare const Codspeed: { startBenchmark(): void; stopBenchmark(): void; setExecutedBenchmark(name: string): void; // zeroStats? // if you decide to re-enable it }; declare const __webpack_chunkname__: string | undefined; declare const __REPO_FILEPATH__: string;benchmark/react/lynx.config.js (1)
31-34: Droppingevent-target-polyfillfrom 002-hello-reactLynx—please confirm runtime coverageBench harnesses that run in older browsers or non‑DOM Node builds could still require
EventTarget. If all runners are modern (Node ≥22 and evergreen browsers), this is fine; otherwise add a conditional polyfill at the app entry.Outside this file, you can defensively gate-load the polyfill (example for TS/ESM):
// in src/patchProfile.ts (or earliest entry) if (typeof globalThis.EventTarget === 'undefined') { await import('event-target-polyfill'); }
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
612b0c3 to
9882350
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/src/lynx/tt.ts (1)
74-88: Ensure inner hydration profiling is always closed (avoid leaking nested profiles).If an error occurs before the current
profileEnd(), the inner segment stays open and the outerprofileEnd()may pop the wrong frame. Wrap the inner segment intry/finallyand liftsnapshotPatchout of thetryscope.- if (__PROFILE__) { - profileStart('ReactLynx::hydrate'); - } - beginPipeline(true, PipelineOrigins.reactLynxHydrate, PerformanceTimingFlags.reactLynxHydrate); - markTiming('hydrateParseSnapshotStart'); - const before = JSON.parse(lepusSide) as SerializedSnapshotInstance; - markTiming('hydrateParseSnapshotEnd'); - markTiming('diffVdomStart'); - const snapshotPatch = hydrate( - before, - __root as BackgroundSnapshotInstance, - ); - if (__PROFILE__) { - profileEnd(); - } + if (__PROFILE__) { + profileStart('ReactLynx::hydrate'); + } + let snapshotPatch!: ReturnType<typeof hydrate>; + try { + beginPipeline(true, PipelineOrigins.reactLynxHydrate, PerformanceTimingFlags.reactLynxHydrate); + markTiming('hydrateParseSnapshotStart'); + const before = JSON.parse(lepusSide) as SerializedSnapshotInstance; + markTiming('hydrateParseSnapshotEnd'); + markTiming('diffVdomStart'); + snapshotPatch = hydrate( + before, + __root as BackgroundSnapshotInstance, + ); + } finally { + if (__PROFILE__) { + profileEnd(); + } + } markTiming('diffVdomEnd');
🧹 Nitpick comments (2)
packages/react/runtime/src/lynx/tt.ts (1)
55-67: Balance outer profiling with finally to guarantee closure.Move
profileEnd()into afinallyso it always runs, even on exceptions.if (__PROFILE__) { profileStart(`OnLifecycleEvent::${type}`); } - try { + try { onLifecycleEventImpl(type, data); } catch (e) { lynx.reportError(e as Error); - } - - if (__PROFILE__) { - profileEnd(); + } finally { + if (__PROFILE__) { + profileEnd(); + } }packages/react/runtime/src/lifecycle/patch/commit.ts (1)
184-206: Ensure profileEnd always runs by wrapping in try/finally.Prevents dangling profiling sessions if an exception occurs during packing/stringify.
Apply this diff:
@@ - if (__PROFILE__) { - profileStart('ReactLynx::commitChanges'); - } - markTiming('packChangesStart'); - const obj: { - data: string; - patchOptions: PatchOptions; - } = { - data: JSON.stringify(patchList), - patchOptions: { - ...patchOptions, - reloadVersion: getReloadVersion(), - }, - }; - markTiming('packChangesEnd'); - if (globalPipelineOptions) { - obj.patchOptions.pipelineOptions = globalPipelineOptions; - setPipeline(undefined); - } - if (__PROFILE__) { - profileEnd(); - } - - return obj; + if (__PROFILE__) { + profileStart('ReactLynx::commitChanges'); + } + try { + markTiming('packChangesStart'); + const obj: { + data: string; + patchOptions: PatchOptions; + } = { + data: JSON.stringify(patchList), + patchOptions: { + ...patchOptions, + reloadVersion: getReloadVersion(), + }, + }; + markTiming('packChangesEnd'); + if (globalPipelineOptions) { + obj.patchOptions.pipelineOptions = globalPipelineOptions; + setPipeline(undefined); + } + return obj; + } finally { + if (__PROFILE__) { + profileEnd(); + } + }
📜 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 (16)
benchmark/react/lynx.config.js(1 hunks)benchmark/react/src/RunBenchmarkUntil.tsx(1 hunks)benchmark/react/src/patchProfile.ts(1 hunks)packages/lynx/benchx_cli/scripts/build.mjs(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(3 hunks)packages/react/runtime/src/lynx-api.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(5 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- benchmark/react/lynx.config.js
- packages/react/runtime/src/lynx/env.ts
- packages/react/runtime/src/lifecycle/render.ts
- packages/lynx/benchx_cli/scripts/build.mjs
- packages/react/runtime/src/backgroundSnapshot.ts
- benchmark/react/src/RunBenchmarkUntil.tsx
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/src/lifecycle/event/jsReady.ts
- benchmark/react/src/patchProfile.ts
- packages/react/runtime/src/lifecycle/reload.ts
- packages/react/runtime/src/debug/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react/runtime/src/lynx/tt.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
packages/react/runtime/src/lifecycle/patch/commit.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
🔇 Additional comments (2)
packages/react/runtime/src/lynx/tt.ts (1)
11-11: Good switch to centralized profiling utilities.Importing
profileStart/profileEndfrom../debug/utils.jskeeps callsites clean and no-op safe when the runtime lacks profiling.packages/react/runtime/src/lifecycle/patch/commit.ts (1)
34-34: TS module resolution validated: inheritedmoduleResolution: node16(via @tsconfig/node20) supports explicit.jsspecifiers, so../../debug/utils.jsresolves in both build and tests.
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
2 times, most recently
from
5a28d0f to
ba10b26
Compare
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
ba10b26 to
636b4eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/react/runtime/src/lynx/tt.ts (3)
11-11: Guard import-time profiling hooks against missing globallynx.
profileStart/profileEndare resolved via an IIFE in debug/utils, which readslynx.performanceat import time. If this module loads before thelynxglobal is initialized, it can throw even when__PROFILE__is false. Prefer resolving throughglobalThis.lynx?.performancein the utils to avoid a ReferenceError.Example (outside this file, in debug/utils.ts):
export const profileStart = /* @__PURE__ */ (() => { const p = (globalThis as any).lynx?.performance; return typeof p?.profileStart === 'function' ? p.profileStart.bind(p) : noop; })(); export const profileEnd = /* @__PURE__ */ (() => { const p = (globalThis as any).lynx?.performance; return typeof p?.profileEnd === 'function' ? p.profileEnd.bind(p) : noop; })();
55-57: Nit: make the profile label human-readable.If
LifecycleConstantis a numeric enum,${type}will print a number. Consider resolving the name for easier traces (optional).
73-89: Ensure hydration profiling always ends (use finally).If
JSON.parseorhydratethrows, the innerprofileEnd()is skipped, leaving an open profile block. Wrap the block intry/finallysoprofileEnd()always runs.case LifecycleConstant.firstScreen: { const { root: lepusSide, jsReadyEventIdSwap } = data as FirstScreenData; if (__PROFILE__) { profileStart('ReactLynx::hydrate'); } - beginPipeline(true, PipelineOrigins.reactLynxHydrate, PerformanceTimingFlags.reactLynxHydrate); - markTiming('hydrateParseSnapshotStart'); - const before = JSON.parse(lepusSide) as SerializedSnapshotInstance; - markTiming('hydrateParseSnapshotEnd'); - markTiming('diffVdomStart'); - const snapshotPatch = hydrate( - before, - __root as BackgroundSnapshotInstance, - ); - if (__PROFILE__) { - profileEnd(); - } + try { + beginPipeline(true, PipelineOrigins.reactLynxHydrate, PerformanceTimingFlags.reactLynxHydrate); + markTiming('hydrateParseSnapshotStart'); + const before = JSON.parse(lepusSide) as SerializedSnapshotInstance; + markTiming('hydrateParseSnapshotEnd'); + markTiming('diffVdomStart'); + const snapshotPatch = hydrate( + before, + __root as BackgroundSnapshotInstance, + ); + } finally { + if (__PROFILE__) { + profileEnd(); + } + } markTiming('diffVdomEnd');Optional follow-up: add a small helper in debug/utils like
withProfile(label, fn)to DRY this pattern.benchmark/react/src/patchProfile.ts (2)
29-31: Preserve originalthisby calling the old method with the hooked receiver, notlynx.performance.Safer if the receiver changes or is proxied.
- old!.call(lynx.performance, name, option); + old?.apply(this as any, [name, option]);- old!.call(lynx.performance); + old?.apply(this as any);Also applies to: 67-68
7-9: Declare missing global typings
Add a single ambient‐declaration file (e.g.types/globals.d.ts) with these globals—none of__REPO_FILEPATH__,__webpack_chunkname__,Codspeedorlynx.performanceare declared elsewhere (aside from the one inlinedeclare const lynxinwebpack-dev-transport/client/transport.ts).
📜 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 (16)
benchmark/react/lynx.config.js(1 hunks)benchmark/react/src/RunBenchmarkUntil.tsx(1 hunks)benchmark/react/src/patchProfile.ts(1 hunks)packages/lynx/benchx_cli/scripts/build.mjs(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(3 hunks)packages/react/runtime/src/lynx-api.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(5 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/react/runtime/src/debug/utils.ts
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/lynx/env.ts
- packages/react/runtime/src/lifecycle/destroy.ts
- packages/lynx/benchx_cli/scripts/build.mjs
- packages/react/runtime/src/lifecycle/reload.ts
- packages/react/runtime/src/lifecycle/event/jsReady.ts
- benchmark/react/src/RunBenchmarkUntil.tsx
- packages/react/runtime/src/lifecycle/render.ts
- benchmark/react/lynx.config.js
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react/runtime/src/lynx/tt.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
benchmark/react/src/patchProfile.ts (1)
benchmark/react/src/hook.ts (1)
hook(5-16)
🔇 Additional comments (1)
packages/react/runtime/src/lynx/tt.ts (1)
55-67: LGTM: standardized profiling around OnLifecycleEvent.Balanced start/end around the try/catch is correct; end executes even on errors.
hzy
force-pushed
the
p/hzy/migrate_to_performance_profile
branch
from
636b4eb to
fcc4317
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/react/runtime/src/lifecycle/event/jsReady.ts (1)
14-31: Balance nested profiling, prevent undefined swap crash, and avoid “root” naming confusion
- If jsReadyEventIdSwap is undefined here, downstream index access throws. Also, ensure profileEnd always runs even on exceptions, and rename the local variable to serializedRoot to avoid confusion with __root.
Apply this diff within the shown lines:
- if (__PROFILE__) { - profileStart('ReactLynx::transferRoot'); - profileStart('ReactLynx::serializeRoot'); - } - const root = JSON.stringify(__root); - if (__PROFILE__) { - profileEnd(); - } + if (__PROFILE__) { + profileStart('ReactLynx::transferRoot'); + } + try { + if (__PROFILE__) { + profileStart('ReactLynx::serializeRoot'); + } + const serializedRoot = JSON.stringify(__root); + if (__PROFILE__) { + profileEnd(); + } @@ - root, - jsReadyEventIdSwap, + root: serializedRoot, + jsReadyEventIdSwap: jsReadyEventIdSwap ?? {}, @@ - if (__PROFILE__) { - profileEnd(); - } + } finally { + if (__PROFILE__) { + profileEnd(); + } + }Additionally (outside the changed hunk), initialize the map at declaration to make the contract explicit:
let jsReadyEventIdSwap: Record<string | number, number> = {};benchmark/react/src/patchProfile.ts (3)
13-26: Fix TS narrowing: guardnamebefore anystartsWithcalls.In strict mode, the current OR-chain doesn’t narrow
name;name.startsWith(...)can error whennameisundefined. Add an early return to narrow.- function shouldIgnoreBenchmark(name: string | undefined) { - if ( - !name - || name === 'ReactLynx::commit' - || name === 'ReactLynx::commitChanges' - || name === 'ReactLynx::transferRoot' - || name === 'ReactLynx::BSI::setAttribute' - || name.startsWith('OnLifecycleEvent::') - || name.startsWith('ReactLynx::diff::') - || name.startsWith('ReactLynx::render::') - ) { - return true; - } - return false; - } + function shouldIgnoreBenchmark(name: string | undefined): boolean { + if (!name) return true; + if ( + name === 'ReactLynx::commit' || + name === 'ReactLynx::commitChanges' || + name === 'ReactLynx::transferRoot' || + name === 'ReactLynx::BSI::setAttribute' || + name.startsWith('OnLifecycleEvent::') || + name.startsWith('ReactLynx::diff::') || + name.startsWith('ReactLynx::render::') + ) { + return true; + } + return false; + }
29-45: Don’t indexignoredwith possibly-undefinedname; also downgrade nested log to a warning.Avoid creating an
"undefined"cache entry and useconsole.warnfor the nested notice. Pass through to original via optional chaining to avoid NPEs.- hook(lynx.performance, 'profileStart', (old, name, option) => { - old!.call(lynx.performance, name, option); - if ((ignored[name] ??= shouldIgnoreBenchmark(name))) { - stack.push('__IGNORED__'); - } else { - stack.push(name); - depth++; - if (depth > 1) { - console.log( - `Benchmark ${name} is ignored because it is nested, the stack: ${ - stack.join(', ') - }`, - ); - } else { - Codspeed.startBenchmark(); - } - } - }); + hook(lynx.performance, 'profileStart', (old, name, option) => { + old?.apply(this as any, [name, option]); + const isIgnored = name ? (ignored[name] ??= shouldIgnoreBenchmark(name)) : true; + if (isIgnored) { + stack.push('__IGNORED__'); + return; + } + stack.push(name as string); + depth++; + if (depth > 1) { + console.warn( + `Benchmark ${name} is ignored because it is nested, the stack: ${stack.join(', ')}` + ); + } else { + Codspeed.startBenchmark(); + } + });
48-68: Guard against unbalancedprofileEndand prevent negativedepth.
stack.pop()!can beundefined; also clampdepthto avoid going negative. Use optional chaining when calling the original.- hook(lynx.performance, 'profileEnd', (old) => { - const name = stack.pop()!; - if (name === '__IGNORED__') { - // Codspeed.zeroStats(); - } else { - if (depth > 1) { - // ignored - } else { - Codspeed.stopBenchmark(); - Codspeed.setExecutedBenchmark( - `${PREFIX}::${__webpack_chunkname__}-${ - name - .replace(/^ReactLynx::/, '') - .replace(/::/g, '__') - }`, - ); - } - depth--; - } - old!.call(lynx.performance); - }); + hook(lynx.performance, 'profileEnd', (old) => { + const name = stack.pop(); + if (!name) { + console.warn('profileEnd without matching profileStart; skipping Codspeed stop.'); + old?.apply(this as any); + return; + } + if (name === '__IGNORED__') { + // Codspeed.zeroStats(); + } else { + if (depth > 1) { + // ignored + } else { + Codspeed.stopBenchmark(); + Codspeed.setExecutedBenchmark( + `${PREFIX}::${__webpack_chunkname__}-${name + .replace(/^ReactLynx::/, '') + .replace(/::/g, '__')}`, + ); + } + if (depth > 0) depth--; + } + old?.apply(this as any); + });
🧹 Nitpick comments (3)
benchmark/react/src/patchProfile.ts (2)
7-8: Declare ambient globals (build-time constants) to avoid TS compile errors and runtime REFs.
__REPO_FILEPATH__and__webpack_chunkname__are assumed to exist. Add ambient declarations (or a small d.ts in benchmark/react/src/types.d.ts) so TS doesn’t error; also ensures safe usage across bundlers.Add once in a .d.ts file:
declare const __REPO_FILEPATH__: string; declare const __webpack_chunkname__: string; declare const Codspeed: { startBenchmark(): void; stopBenchmark(): void; setExecutedBenchmark(name: string): void; }; declare const lynx: { performance: { profileStart?: (name?: string, options?: unknown) => void; profileEnd?: () => void; }; };Also applies to: 58-63
32-32: Extract sentinel'__IGNORED__'to a constant for consistency.Minor readability win and avoids typos in multiple sites.
+const IGNORED_SENTINEL = '__IGNORED__'; ... - stack.push('__IGNORED__'); + stack.push(IGNORED_SENTINEL); ... - if (name === '__IGNORED__') { + if (name === IGNORED_SENTINEL) {Also applies to: 50-50
packages/react/runtime/src/lynx/env.ts (1)
55-75: Always pair start/end and improve label granularityWrap with
finallysoprofileEnd()runs even on unexpected errors, and includeprocessorNamein the label to distinguish profiles.Apply inside the
globalThis.processDatabody:- if (__PROFILE__) { - profileStart('processData'); - } - - let r: InitData | InitDataRaw; - try { + if (__PROFILE__) { + profileStart(processorName ? `processData:${processorName}` : 'processData'); + } + + let r: InitData | InitDataRaw; + try { if (processorName) { r = dataProcessorDefinition?.dataProcessors?.[processorName]?.(data) as InitData ?? data; } else { r = dataProcessorDefinition?.defaultDataProcessor?.(data) ?? data; } } catch (e: any) { lynx.reportError(e as Error); // when there is an error // we should perform like dataProcessor returns nothing // so use `{}` rather than `data` r = {}; - } - - if (__PROFILE__) { - profileEnd(); - } + } finally { + if (__PROFILE__) { + profileEnd(); + } + }
📜 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 (16)
benchmark/react/lynx.config.js(1 hunks)benchmark/react/src/RunBenchmarkUntil.tsx(1 hunks)benchmark/react/src/patchProfile.ts(1 hunks)packages/lynx/benchx_cli/scripts/build.mjs(1 hunks)packages/react/runtime/src/backgroundSnapshot.ts(4 hunks)packages/react/runtime/src/debug/utils.ts(1 hunks)packages/react/runtime/src/lifecycle/destroy.ts(2 hunks)packages/react/runtime/src/lifecycle/event/jsReady.ts(2 hunks)packages/react/runtime/src/lifecycle/patch/commit.ts(3 hunks)packages/react/runtime/src/lifecycle/reload.ts(3 hunks)packages/react/runtime/src/lifecycle/render.ts(3 hunks)packages/react/runtime/src/lynx-api.ts(2 hunks)packages/react/runtime/src/lynx/env.ts(3 hunks)packages/react/runtime/src/lynx/tt.ts(5 hunks)packages/react/runtime/src/snapshot/gesture.ts(0 hunks)packages/react/runtime/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/snapshot/gesture.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/react/runtime/src/lifecycle/patch/commit.ts
- packages/react/runtime/src/lifecycle/reload.ts
- packages/lynx/benchx_cli/scripts/build.mjs
- packages/react/runtime/src/backgroundSnapshot.ts
- packages/react/runtime/vitest.config.ts
- packages/react/runtime/src/lynx-api.ts
- packages/react/runtime/src/lynx/tt.ts
- benchmark/react/src/RunBenchmarkUntil.tsx
- packages/react/runtime/src/debug/utils.ts
- benchmark/react/lynx.config.js
- packages/react/runtime/src/lifecycle/destroy.ts
🧰 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/env.tspackages/react/runtime/src/lifecycle/render.ts
📚 Learning: 2025-08-19T11:25:36.127Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1558
File: .changeset/solid-squids-fall.md:2-2
Timestamp: 2025-08-19T11:25:36.127Z
Learning: In the lynx-family/lynx-stack repository, changesets should use the exact package name from package.json#name, not generic or unscoped names. Each package has its own specific scoped name (e.g., "lynx-js/react-transform" for packages/react/transform).
Applied to files:
packages/react/runtime/src/lynx/env.ts
🧬 Code graph analysis (4)
packages/react/runtime/src/lynx/env.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
packages/react/runtime/src/lifecycle/event/jsReady.ts (2)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)packages/react/runtime/src/lynx-api.ts (1)
root(84-123)
benchmark/react/src/patchProfile.ts (1)
benchmark/react/src/hook.ts (1)
hook(5-16)
packages/react/runtime/src/lifecycle/render.ts (1)
packages/react/runtime/src/debug/utils.ts (2)
profileStart(9-15)profileEnd(17-23)
🔇 Additional comments (5)
packages/react/runtime/src/lifecycle/render.ts (2)
11-11: Good switch to debug utils (profileStart/profileEnd).Importing from the centralized utils with no-op fallbacks is correct and keeps builds safe when
lynx.performanceis unavailable.
20-31: Profiling aroundrenderToStringis correctly balanced.Guarded start +
finally-ended stop ensures symmetry even on exceptions. Looks solid.packages/react/runtime/src/lifecycle/event/jsReady.ts (1)
4-4: LGTM: switch to shared profiling utilsCentralizes the profiling shim via debug/utils; consistent with repo style.
packages/react/runtime/src/lynx/env.ts (2)
55-56: LGTM: migrated toprofileStartunder__PROFILE__The switch to engine-backed profiling is correct and consistent with the PR goals.
4-4: Refactor profileStart/end for lazy lynx.performance lookup
- Change
profileStart/profileEndto functions that accessglobalThis.lynx?.performanceat call-time to avoid import-time throws iflynxisn’t injected. e.g.:- export const profileStart = /* @__PURE__ */ (() => { - let p; - if (!(p = lynx.performance) || typeof p.profileStart !== 'function') { - return noop; - } - return p.profileStart.bind(p); - })() as typeof lynx.performance.profileStart; + export function profileStart(name: string): void { + const p = (globalThis as any).lynx?.performance; + if (typeof p?.profileStart === 'function') p.profileStart(name); + }- No
console.profileorconsole.profileEndoccurrences remain inpackages/react/runtime(verified via grep).- Verify
packages/react/runtime/tsconfig.json(and any base configs it extends) permit.jsimport specifiers (e.g. viaallowImportingTsExtensionsorverbatimModuleSyntax) to ensure theimport '../debug/utils.js'works.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
Note: No user-facing API changes.
Checklist