feat(react): support ReactLynx::hooks::setState trace for function components#2198
feat(react): support ReactLynx::hooks::setState trace for function components#2198
ReactLynx::hooks::setState trace for function components#2198Conversation
- Fix functional component hook interception logic. - Ensure correct access to hook list (legacy vs ). - Fix constant mismatch. - Add comprehensive tests covering edge cases (manual vnodes, non-array values, error handling). - improve code coverage to 100% for profile.ts.
🦋 Changeset detectedLatest commit: a4ea38a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds hook-related opcode constants and runtime types, instruments hook state changes during DIFFED to emit hook setState profiling events, tightens profiling guards, and expands tests covering class/function setState and serialization cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/react/runtime/__test__/debug/trace.test.jsx`:
- Around line 40-44: The afterEach cleanup currently restores mocks and
options.diffed but does not remove test globals; add deletion of the properties
set on globalThis (globalThis.triggerUpdate, globalThis.updateFunc,
globalThis.updateCircular, globalThis.updateMissing, globalThis.updateUnknown)
inside the existing afterEach callback so each test run removes those globals
and prevents cross-test leakage; keep the existing vi.restoreAllMocks() and
options.diffed restoration and append safe delete operations for the listed
globalThis properties.
In `@packages/react/runtime/src/debug/profile.ts`:
- Line 84: Remove the stray console.log in profile.ts that prints
'isClassComponent', isClassComponent, type; specifically delete the
console.log(...) line so production code no longer logs on every class-component
setState; if logging is needed keep a gated debug/logger check instead (e.g.,
use an existing debug flag or logger) but do not leave console.log in the
function that references isClassComponent and type.
🧹 Nitpick comments (7)
packages/react/runtime/__test__/debug/trace.test.jsx (2)
249-276:initProfileHook()is called twice — once inbeforeEach, once here.Lines 251–253 override
options.diffedwith a fresh mock and re-initialize. ThebeforeEachcall toinitProfileHook()is wasted for this test (and the two following tests). This isn't a bug but is slightly misleading. Consider extracting a helper or skippinginitProfileHook()inbeforeEachfor tests that manually reinitialize.
278-304: Test triggers setter but doesn't assert profiling behavior.Line 301 sets
hookState[NEXT_VALUE] = ['new-value']to hit the setter branch, but there's no assertion onprofileMarkbeing called or on the resulting state. This test only provides coverage without verifying correctness. Consider adding an assertion on the expectedprofileMarkcall.packages/react/runtime/src/debug/profile.ts (5)
94-94: Remove emptyelse {}block — dead code.♻️ Proposed fix
- } else {} + }
176-182:buildSetStateProfileMarkArgsproduces misleading output for primitive hook values.
buildSetStateProfileMarkArgscallsObject.keys()on its arguments, which was designed for class component state objects. WhencurrentValue/nextValueare primitives (e.g.,0,'hello'),Object.keys()returns[], so the diff keys in the profile mark will always be empty arrays — misleading for anyone reading the trace. Consider guarding with a type check or skipping this call for non-object values.♻️ Proposed fix — skip state-key diff for primitives
...buildSetStateProfileMarkArgs( - currentValue as Record<string, unknown>, - nextValue as Record<string, unknown>, + typeof currentValue === 'object' && currentValue !== null + ? currentValue as Record<string, unknown> + : {}, + typeof nextValue === 'object' && nextValue !== null + ? nextValue as Record<string, unknown> + : {}, ),
134-134: Use aSymbolinstead of the string'internalNextValue'for the backing store.Attaching a plain string property to Preact's internal hook state objects risks colliding with current or future Preact internals. A module-scoped
Symbol('internalNextValue')would be collision-proof.♻️ Proposed fix
Add a symbol at module scope (e.g., near line 70):
const sInternalNextValue = Symbol('internalNextValue');Then replace all three occurrences of
'internalNextValue':- hookState['internalNextValue'] = hookState[NEXT_VALUE]; + hookState[sInternalNextValue] = hookState[NEXT_VALUE];- get: () => hookState['internalNextValue'], + get: () => hookState[sInternalNextValue],- hookState['internalNextValue'] = value; + hookState[sInternalNextValue] = value;
151-164: HoistformatandsafeJsonStringifyout of theforEachloop.Both are pure functions with no closure over loop variables. Currently they're recreated on every hook state iteration inside every
diffedcall. Move them to module scope or at least outside theforEachto avoid unnecessary allocations on a hot path.
189-189: Emptycatchsilently swallows all errors — consider logging in dev mode.While this is intentional for resilience (e.g., non-configurable properties), silently dropping errors makes it hard to diagnose unexpected failures. A
console.warngated behind a dev-mode check would help.
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
🤖 Fix all issues with AI agents
In `@packages/react/runtime/src/debug/profile.ts`:
- Around line 143-146: The code claims to support legacy hooks._list but only
uses hooks.__ and the ComponentHooks type lacks _list; update both the runtime
check and the type: in the hook callback (hook(options, DIFFED, (old, vnode) =>
{ ... })) change the hooks extraction to fall back to hooks._list when hooks.__
is undefined (e.g., const hookList = hooks?.__ ?? hooks?._list) and add a
corresponding optional _list?: HookState[] property to the ComponentHooks
interface in internal-preact.d.ts so the legacy path is both typed and
implemented.
🧹 Nitpick comments (3)
packages/react/runtime/src/debug/profile.ts (3)
25-61:buildSetStateProfileMarkArgsproduces misleading "keys" for primitive hook state.When called from the hooks instrumentation path (line 178),
currentValue/nextValuecan be primitives (e.g.,useState(0)).Object.keys(42 as Record<string, unknown>)returns[], so the"current state keys"/"next state keys"/"changed … keys"fields will always be empty arrays for primitive state — noise that can confuse profiling consumers.Consider either skipping the keys fields when the value isn't a plain object, or documenting that the keys fields are only meaningful for object-shaped state.
♻️ Suggested guard
function buildSetStateProfileMarkArgs( currentState: unknown, nextState: unknown, ): Record<string, string> { - const EMPTY_OBJ = {}; - - const currentStateObj = (currentState ?? EMPTY_OBJ) as Record<string, unknown>; - const nextStateObj = (nextState ?? EMPTY_OBJ) as Record<string, unknown>; - - return { - 'current state keys': JSON.stringify(Object.keys(currentStateObj)), - 'next state keys': JSON.stringify(Object.keys(nextStateObj)), - 'changed (shallow diff) state keys': JSON.stringify( - // the setState is in assign manner, we assume nextState is a superset of currentState - Object.keys(nextStateObj).filter( - key => currentStateObj[key] !== nextStateObj[key], - ), - ), - currentValue: safeJsonStringify(format(currentState)), - nextValue: safeJsonStringify(format(nextState)), - }; + const result: Record<string, string> = { + currentValue: safeJsonStringify(format(currentState)), + nextValue: safeJsonStringify(format(nextState)), + }; + + if ( + currentState !== null && typeof currentState === 'object' + && nextState !== null && typeof nextState === 'object' + ) { + const currentStateObj = currentState as Record<string, unknown>; + const nextStateObj = nextState as Record<string, unknown>; + result['current state keys'] = JSON.stringify(Object.keys(currentStateObj)); + result['next state keys'] = JSON.stringify(Object.keys(nextStateObj)); + result['changed (shallow diff) state keys'] = JSON.stringify( + Object.keys(nextStateObj).filter( + key => currentStateObj[key] !== nextStateObj[key], + ), + ); + } + + return result; }
148-191: Use aSymbolinstead of the string key'internalNextValue'to avoid property collisions.
hookStateis aRecord<string, unknown>from Preact's internals. Using the plain string'internalNextValue'as the backing-store key could collide with a future Preact internal property or any other code touching hook state objects. ASymbol(likesFlowIDelsewhere in this file) would be collision-proof.♻️ Suggested change
Add a symbol at the top of the block (near line 87):
const sInternalNextValue = Symbol('INTERNAL_NEXT_VALUE');Then replace all three occurrences of
hookState['internalNextValue'](lines 151, 164, 185) withhookState[sInternalNextValue].
189-189: Silentcatch (e) {}swallows all errors — at minimum log them.If
Object.definePropertyor any other operation inside thetrythrows for an unexpected reason (not just "property is non-configurable"), this empty catch will silently discard the error, making debugging very difficult. Consider at least logging at a debug level.♻️ Suggested improvement
- } catch (e) {} + } catch (e) { + if (typeof __ALOG__ !== 'undefined' && __ALOG__) { + console.alog?.('[ReactLynxDebug] Failed to instrument hook state:', e); + } + }
Merging this PR will degrade performance by 27.13%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#7767 Bundle Size — 383.74KiB (0%).a4ea38a(current) vs 98b2504 main#7766(baseline) Bundle metrics
|
| Current #7767 |
Baseline #7766 |
|
|---|---|---|
 Initial JS |
154.88KiB |
154.88KiB |
| Initial CSS |
35.06KiB |
35.06KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
 Modules |
237(-0.42%) |
238 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.99% |
2.99% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7767 |
Baseline #7766 |
|
|---|---|---|
| JS |
252.83KiB |
252.83KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.06KiB |
35.06KiB |
Bundle analysis report Branch fix/react-lynx-profile-coverage Project dashboard
Generated by RelativeCI Documentation Report issue
upupming
changed the title
ReactLynx::hooks::setState trace for function components.
upupming
changed the title
ReactLynx::hooks::setState trace for function components.ReactLynx::hooks::setState trace for function components
upupming
force-pushed
the
fix/react-lynx-profile-coverage
branch
from
bb86758 to
8b662f3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (6)
packages/react/runtime/src/debug/profile.ts (4)
119-119: Remove the emptyelse {}block.This empty branch serves no purpose and adds visual noise.
♻️ Suggested fix
if (isClassComponent) { profileMark('ReactLynx::setState', { flowId: this[sFlowID] ??= profileFlowId(), args: buildSetStateProfileMarkArgs( this.state, this[NEXT_STATE], ), }); - } else {} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/debug/profile.ts` at line 119, Remove the empty "else { }" branch in the conditional located in packages/react/runtime/src/debug/profile.ts (the empty else at the shown diff); simply delete the else block so the if-statement stands alone and adjust surrounding whitespace/formatting as needed to keep linting/formatting happy (no other logic changes required).
197-197: Emptycatchblock silently swallows all errors.While acceptable for debug/profiling code to avoid breaking the host app, consider logging to aid troubleshooting when profiling unexpectedly stops working. Even a minimal
console.debugguarded by a flag would help. This is just a suggestion, not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/debug/profile.ts` at line 197, An empty catch in packages/react/runtime/src/debug/profile.ts is swallowing errors; update the catch block that currently reads "catch (e) {}" to log the caught error (e.g., console.debug or a debug logger) and guard the log with the existing debug/profiling flag (or __DEV__) so profiling failures are visible without affecting production; locate the try/catch in the profiling code path and replace the silent swallow with a minimal guarded debug log referencing the caught exception.
159-159: Consider using aSymbolinstead of the string key'internalNextValue'to avoid polluting hook state objects.The existing pattern in this file already uses
Symbol('FLOW_ID')(line 96) for private properties. Using a module-scoped Symbol here would be consistent and prevent any accidental collision with Preact internals or future properties.♻️ Suggested change
+const sInternalNextValue = Symbol('INTERNAL_NEXT_VALUE'); + // ... inside the DIFFED hook ... - hookState['internalNextValue'] = hookState[NEXT_VALUE]; + hookState[sInternalNextValue] = hookState[NEXT_VALUE]; Object.defineProperty(hookState, NEXT_VALUE, { - get: () => hookState['internalNextValue'], + get: () => hookState[sInternalNextValue], set: (value) => { // ... setter body ... - hookState['internalNextValue'] = value; + hookState[sInternalNextValue] = value; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/debug/profile.ts` at line 159, Replace the string-keyed assignment to hookState['internalNextValue'] with a module-scoped Symbol to avoid polluting hook state; declare a const like INTERNAL_NEXT_VALUE = Symbol('INTERNAL_NEXT_VALUE') at top of the module and update the assignment hookState['internalNextValue'] = hookState[NEXT_VALUE] to hookState[INTERNAL_NEXT_VALUE] = hookState[NEXT_VALUE], and update any other reads/writes that reference 'internalNextValue' to use the INTERNAL_NEXT_VALUE symbol instead (referencing NEXT_VALUE and hookState to locate the usages).
151-200:Object.definePropertyis re-invoked on every DIFFED call for every hook — consider guarding against redundant redefinition.The DIFFED hook runs on every render cycle. Each time, it iterates all hook states and redefines the
NEXT_VALUEproperty descriptor, even though the getter/setter logic is identical. Whileconfigurable: truepermits this, it's unnecessary work. A simple guard (e.g., checking for an already-defined sentinel) would avoid the overhead:♻️ Suggested guard to skip redundant defineProperty
if (Array.isArray(hookList)) { hookList.forEach((hookState, hookIdx: number) => { try { + // Skip if already intercepted + if ('internalNextValue' in hookState) return; hookState['internalNextValue'] = hookState[NEXT_VALUE]; // define a setter for __N to track the next value of the hook Object.defineProperty(hookState, NEXT_VALUE, {However, verify that early-returning here doesn't break correctness — specifically that
hookIdxremains stable across renders (it should, since conditional hooks are a user-level bug).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/debug/profile.ts` around lines 151 - 200, The DIFFED handler currently calls Object.defineProperty for NEXT_VALUE on every render for every hookState, causing redundant redefinition; guard against this by skipping hookStates already patched (e.g. check a sentinel like hookState['__nextValuePatched'] or test Object.getOwnPropertyDescriptor(hookState, NEXT_VALUE) before calling Object.defineProperty) and set the sentinel after defining the getter/setter so subsequent DIFFED runs no-op; ensure you still copy/restore the current NEXT_VALUE into hookState['internalNextValue'] for unpatched and patched cases so existing value/state is preserved and hookIdx logic remains unchanged (refs: hook(..., DIFFED, ...), hookState, NEXT_VALUE, internalNextValue, sFlowID, profileMark).packages/react/runtime/__test__/debug/profile.test.jsx (2)
22-26: OnlyprofileMarkis cleared — other spies (profileStart,profileEnd,profileFlowId) accumulate across tests.If any future test asserts call counts or specific call arguments for
profileStart/profileEnd, it will see calls from prior tests. Consider clearing all performance spies inbeforeEachfor isolation, or document that onlyprofileMarkassertions are safe across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/debug/profile.test.jsx` around lines 22 - 26, The test's beforeEach only clears lynx.performance.profileMark, leaving other jest spies to accumulate; update the setup to also clear lynx.performance.profileStart, lynx.performance.profileEnd, and lynx.performance.profileFlowId (or call jest.clearAllMocks()) so each test starts with zero calls — modify the beforeEach that currently calls lynx.performance.profileMark.mockClear() to additionally call mockClear() on profileStart, profileEnd, and profileFlowId to ensure test isolation.
273-303:options.diffedis overwritten but never restored, creating implicit ordering dependency between tests.Both this test and the next one ("should handle unknown component name") wrap
options.diffedwithout restoring the original after the test completes. Each successive test captures the previous test's wrapper, building a chain. If test order changes or a test is added in between, behavior may silently break.Consider restoring in an
afterEachor at the end of each test:♻️ Suggested cleanup pattern
test('should handle missing component instance', async () => { let capturedComponent; const profileWrapper = options.diffed; options.diffed = (vnode) => { if (vnode.__c && typeof vnode.type === 'function' && vnode.type.name === 'App') { capturedComponent = vnode.__c; } profileWrapper?.(vnode); }; // ... test body ... expect(() => { updateMissing(); }).toThrow(); + + options.diffed = profileWrapper; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/debug/profile.test.jsx` around lines 273 - 303, The test overwrites options.diffed (via profileWrapper) but never restores it, causing cross-test interference; save the original options.diffed before wrapping and restore it after the test (either in an afterEach or a try/finally at the end of the test) so subsequent tests are unaffected—refer to the options.diffed wrapper used in the 'should handle missing component instance' and the profileWrapper capture to implement the restore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react/runtime/__test__/debug/profile.test.jsx`:
- Around line 22-26: The test's beforeEach only clears
lynx.performance.profileMark, leaving other jest spies to accumulate; update the
setup to also clear lynx.performance.profileStart, lynx.performance.profileEnd,
and lynx.performance.profileFlowId (or call jest.clearAllMocks()) so each test
starts with zero calls — modify the beforeEach that currently calls
lynx.performance.profileMark.mockClear() to additionally call mockClear() on
profileStart, profileEnd, and profileFlowId to ensure test isolation.
- Around line 273-303: The test overwrites options.diffed (via profileWrapper)
but never restores it, causing cross-test interference; save the original
options.diffed before wrapping and restore it after the test (either in an
afterEach or a try/finally at the end of the test) so subsequent tests are
unaffected—refer to the options.diffed wrapper used in the 'should handle
missing component instance' and the profileWrapper capture to implement the
restore.
In `@packages/react/runtime/src/debug/profile.ts`:
- Line 119: Remove the empty "else { }" branch in the conditional located in
packages/react/runtime/src/debug/profile.ts (the empty else at the shown diff);
simply delete the else block so the if-statement stands alone and adjust
surrounding whitespace/formatting as needed to keep linting/formatting happy (no
other logic changes required).
- Line 197: An empty catch in packages/react/runtime/src/debug/profile.ts is
swallowing errors; update the catch block that currently reads "catch (e) {}" to
log the caught error (e.g., console.debug or a debug logger) and guard the log
with the existing debug/profiling flag (or __DEV__) so profiling failures are
visible without affecting production; locate the try/catch in the profiling code
path and replace the silent swallow with a minimal guarded debug log referencing
the caught exception.
- Line 159: Replace the string-keyed assignment to
hookState['internalNextValue'] with a module-scoped Symbol to avoid polluting
hook state; declare a const like INTERNAL_NEXT_VALUE =
Symbol('INTERNAL_NEXT_VALUE') at top of the module and update the assignment
hookState['internalNextValue'] = hookState[NEXT_VALUE] to
hookState[INTERNAL_NEXT_VALUE] = hookState[NEXT_VALUE], and update any other
reads/writes that reference 'internalNextValue' to use the INTERNAL_NEXT_VALUE
symbol instead (referencing NEXT_VALUE and hookState to locate the usages).
- Around line 151-200: The DIFFED handler currently calls Object.defineProperty
for NEXT_VALUE on every render for every hookState, causing redundant
redefinition; guard against this by skipping hookStates already patched (e.g.
check a sentinel like hookState['__nextValuePatched'] or test
Object.getOwnPropertyDescriptor(hookState, NEXT_VALUE) before calling
Object.defineProperty) and set the sentinel after defining the getter/setter so
subsequent DIFFED runs no-op; ensure you still copy/restore the current
NEXT_VALUE into hookState['internalNextValue'] for unpatched and patched cases
so existing value/state is preserved and hookIdx logic remains unchanged (refs:
hook(..., DIFFED, ...), hookState, NEXT_VALUE, internalNextValue, sFlowID,
profileMark).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/profile-hooks-coverage.mdpackages/react/runtime/__test__/debug/hook.jspackages/react/runtime/__test__/debug/profile.test.jsxpackages/react/runtime/src/debug/profile.tspackages/react/runtime/src/renderToOpcodes/constants.ts
💤 Files with no reviewable changes (1)
- packages/react/runtime/test/debug/hook.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/profile-hooks-coverage.md
upupming
force-pushed
the
fix/react-lynx-profile-coverage
branch
from
8b662f3 to
fa79428
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react/runtime/__test__/debug/profile.test.jsx (1)
28-52: Restore options hooks in afinallyblock.If the expectation throws,
options[DIFF/DIFF2/RENDER/DIFFED]will remain overridden and can leak into later tests (Line 28).♻️ Safer restore pattern
options[DIFF] = noop; options[DIFF2] = noop; options[RENDER] = noop; options[DIFFED] = noop; - - render( - null, - scratch, - ); - - expect(noop).toBeCalledTimes(4); - - options[DIFF] = oldDiff; - options[DIFF2] = oldDiff2; - options[RENDER] = oldRender; - options[DIFFED] = oldDiffed; + try { + render( + null, + scratch, + ); + + expect(noop).toBeCalledTimes(4); + } finally { + options[DIFF] = oldDiff; + options[DIFF2] = oldDiff2; + options[RENDER] = oldRender; + options[DIFFED] = oldDiffed; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/debug/profile.test.jsx` around lines 28 - 52, The test overrides options[DIFF], options[DIFF2], options[RENDER], and options[DIFFED] but restores them only after the assertion, which can leak if the expectation throws; wrap the override and test invocation in a try...finally: save oldDiff/oldDiff2/oldRender/oldDiffed, set options[...] = noop, call render and assertions inside the try block, and restore the originals in the finally block so options is always reset even on failure (refer to options, DIFF, DIFF2, RENDER, DIFFED, oldDiff, oldDiff2, oldRender, oldDiffed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/__test__/debug/profile.test.jsx`:
- Around line 273-343: Both tests temporarily override options.diffed but never
restore it; save the original into profileWrapper (as already done) and restore
it after the sabotage/update completes. For the tests "should handle missing
component instance" and "should handle unknown component name", wrap the
override so that after you call updateMissing/updateUnknown (or in a finally
block) you set options.diffed = profileWrapper to avoid leaking the wrapper into
subsequent tests; reference the existing profileWrapper and options.diffed
variables to locate the change.
In `@packages/react/runtime/src/debug/profile.ts`:
- Around line 107-119: The code uses a non-null assertion this.__v! to read type
when building the setState profile mark; replace that with optional chaining and
guard the path so you don't access properties when __v is undefined: compute
const type = this.__v?.type and only evaluate isClassComponent/emit profileMark
when type is defined (or short-circuit early), keeping references to the same
symbols (isClassComponent, profileMark, sFlowID, profileFlowId,
buildSetStateProfileMarkArgs, this.state, this[NEXT_STATE]) to avoid unsafe
access.
---
Nitpick comments:
In `@packages/react/runtime/__test__/debug/profile.test.jsx`:
- Around line 28-52: The test overrides options[DIFF], options[DIFF2],
options[RENDER], and options[DIFFED] but restores them only after the assertion,
which can leak if the expectation throws; wrap the override and test invocation
in a try...finally: save oldDiff/oldDiff2/oldRender/oldDiffed, set options[...]
= noop, call render and assertions inside the try block, and restore the
originals in the finally block so options is always reset even on failure (refer
to options, DIFF, DIFF2, RENDER, DIFFED, oldDiff, oldDiff2, oldRender,
oldDiffed).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/profile-hooks-coverage.mdpackages/react/runtime/__test__/debug/hook.jspackages/react/runtime/__test__/debug/profile.test.jsxpackages/react/runtime/src/debug/profile.tspackages/react/runtime/src/renderToOpcodes/constants.ts
💤 Files with no reviewable changes (1)
- packages/react/runtime/test/debug/hook.js
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/profile-hooks-coverage.md
- packages/react/runtime/src/renderToOpcodes/constants.ts
upupming
force-pushed
the
fix/react-lynx-profile-coverage
branch
from
a604bee to
a505671
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Yiming Li <yimingli.cs@gmail.com>
upupming
force-pushed
the
fix/react-lynx-profile-coverage
branch
from
a505671 to
30f2ccc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/react/runtime/src/debug/profile.ts (2)
108-120:⚠️ Potential issue | 🟠 MajorAvoid non-null assertion on the component vnode.
this[VNODE]!can be undefined even whenDIRTYis set, which would throw in profiling mode. Guard it before checking class-ness.🐛 Proposed fix
- const type = this[VNODE]!.type; - const isClassComponent = typeof type === 'function' && ('prototype' in type) - && ('render' in type.prototype); + const type = this[VNODE]?.type; + const isClassComponent = !!type + && typeof type === 'function' + && ('prototype' in type) + && ('render' in type.prototype);#!/bin/bash # Verify whether Component.__v is optional in types and how it's assigned. fd 'internal-preact.d.ts' packages/react/runtime/types --exec rg -n "__v" {} rg -n "__v\\s*=|\\.\\__v" packages/react/runtime/src -C 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/debug/profile.ts` around lines 108 - 120, The profiling code uses this[VNODE]! which can be undefined and will throw; modify the block in setState profiling to first read const vnode = this[VNODE]; guard vnode !== undefined before computing const type = vnode.type and the isClassComponent check, and only call profileMark('ReactLynx::setState', { flowId: this[sFlowID] ??= profileFlowId(), args: buildSetStateProfileMarkArgs(this.state, this[NEXT_STATE]) }) when vnode exists and is a class component; keep the existing symbols (VNODE, DIRTY, sFlowID, NEXT_STATE, buildSetStateProfileMarkArgs, profileMark) and otherwise skip profiling so behavior remains unchanged.
153-156:⚠️ Potential issue | 🟠 MajorAdd legacy
hooks._listfallback.
The PR goal states support for bothhooks.__andhooks._list, but this only readshooks[LIST]. Add a fallback and extend the hook type accordingly.🐛 Proposed fix
- const hookList = hooks?.[LIST]; + const hookList = hooks?.[LIST] ?? hooks?._list;#!/bin/bash # Verify ComponentHooks type surface for legacy _list support. fd 'internal-preact.d.ts' packages/react/runtime/types --exec rg -n "ComponentHooks|_list|__\\s*\\?:" {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/debug/profile.ts` around lines 153 - 156, The code currently only reads hooks[LIST] from vnode[COMPONENT]?.[HOOKS] but the PR must support the legacy name hooks._list; update the access to compute hookList by checking both keys (e.g., const hookList = hooks?.[LIST] ?? hooks?._list) so legacy data is read, and update the ComponentHooks type to include an optional _list property alongside the existing LIST/__ variants; ensure references to vnode[COMPONENT], HOOKS, and LIST use the new fallback and that the Hook list type accepts either property.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/debug/profile.test.jsx (1)
28-52: Wrap options overrides in try/finally.
If an assertion fails, the globaloptionshooks won’t be restored. Atry/finallykeeps later tests isolated.♻️ Suggested pattern
- options[DIFF] = noop; - options[DIFF2] = noop; - options[RENDER] = noop; - options[DIFFED] = noop; - - render( - null, - scratch, - ); - - expect(noop).toBeCalledTimes(4); - - options[DIFF] = oldDiff; - options[DIFF2] = oldDiff2; - options[RENDER] = oldRender; - options[DIFFED] = oldDiffed; + options[DIFF] = noop; + options[DIFF2] = noop; + options[RENDER] = noop; + options[DIFFED] = noop; + try { + render( + null, + scratch, + ); + + expect(noop).toBeCalledTimes(4); + } finally { + options[DIFF] = oldDiff; + options[DIFF2] = oldDiff2; + options[RENDER] = oldRender; + options[DIFFED] = oldDiffed; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/debug/profile.test.jsx` around lines 28 - 52, Wrap the temporary overrides of the global options hooks in a try/finally to guarantee restoration even if the assertion fails: in the test 'original options hooks should be called', after saving oldDiff, oldDiff2, oldRender, oldDiffed and before assigning options[DIFF], options[DIFF2], options[RENDER], options[DIFFED] to noop, open a try block that contains the render call and the expect call, and move the assignments that restore options[DIFF], options[DIFF2], options[RENDER], options[DIFFED] into the finally block so the original hooks are always restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/src/debug/profile.ts`:
- Around line 164-169: The code indexes hookState[VALUE] as a tuple without
verifying its shape; add a runtime guard to ensure hookState[VALUE] is an array
before treating it as [state, dispatch]. Specifically, in the block that checks
Array.isArray(value), also check Array.isArray(hookState[VALUE]) (or otherwise
confirm it's an array/tuple) before reading currentValue = (hookState[VALUE] as
unknown[])[0]; if the guard fails, skip tuple-specific logic or handle the
non-tuple case appropriately to avoid runtime exceptions in functions like the
profiling code that reference hookState and VALUE.
---
Duplicate comments:
In `@packages/react/runtime/src/debug/profile.ts`:
- Around line 108-120: The profiling code uses this[VNODE]! which can be
undefined and will throw; modify the block in setState profiling to first read
const vnode = this[VNODE]; guard vnode !== undefined before computing const type
= vnode.type and the isClassComponent check, and only call
profileMark('ReactLynx::setState', { flowId: this[sFlowID] ??= profileFlowId(),
args: buildSetStateProfileMarkArgs(this.state, this[NEXT_STATE]) }) when vnode
exists and is a class component; keep the existing symbols (VNODE, DIRTY,
sFlowID, NEXT_STATE, buildSetStateProfileMarkArgs, profileMark) and otherwise
skip profiling so behavior remains unchanged.
- Around line 153-156: The code currently only reads hooks[LIST] from
vnode[COMPONENT]?.[HOOKS] but the PR must support the legacy name hooks._list;
update the access to compute hookList by checking both keys (e.g., const
hookList = hooks?.[LIST] ?? hooks?._list) so legacy data is read, and update the
ComponentHooks type to include an optional _list property alongside the existing
LIST/__ variants; ensure references to vnode[COMPONENT], HOOKS, and LIST use the
new fallback and that the Hook list type accepts either property.
---
Nitpick comments:
In `@packages/react/runtime/__test__/debug/profile.test.jsx`:
- Around line 28-52: Wrap the temporary overrides of the global options hooks in
a try/finally to guarantee restoration even if the assertion fails: in the test
'original options hooks should be called', after saving oldDiff, oldDiff2,
oldRender, oldDiffed and before assigning options[DIFF], options[DIFF2],
options[RENDER], options[DIFFED] to noop, open a try block that contains the
render call and the expect call, and move the assignments that restore
options[DIFF], options[DIFF2], options[RENDER], options[DIFFED] into the finally
block so the original hooks are always restored.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react/runtime/__test__/debug/profile.test.jsxpackages/react/runtime/src/debug/profile.ts
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/react@0.116.4 ### Patch Changes - Support `ReactLynx::hooks::setState` trace for function components. ([#2198](#2198)) - fix: properly cleanup `__DestroyLifetime` listeners and listCallbacks in `snapshotDestroyList`. ([#2224](#2224)) ## @lynx-js/qrcode-rsbuild-plugin@0.4.6 ### Patch Changes - Print all entries with all schema URLs in non-TTY environments instead of only showing the first entry's QR code. ([#2227](#2227)) ## @lynx-js/react-rsbuild-plugin@0.12.9 ### Patch Changes - Add alias for `use-sync-external-store/with-selector.js` and `use-sync-external-store/shim/with-selector.js` pointing to @lynx-js/use-sync-external-store. ([#2200](#2200)) - Updated dependencies \[[`9033e2d`](9033e2d)]: - @lynx-js/template-webpack-plugin@0.10.4 - @lynx-js/react-alias-rsbuild-plugin@0.12.9 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 - @lynx-js/react-webpack-plugin@0.7.4 - @lynx-js/css-extract-webpack-plugin@0.7.0 ## @lynx-js/css-serializer@0.1.4 ### Patch Changes - Move `cssChunksToMap` implementation from `@lynx-js/template-webpack-plugin` to `@lynx-js/css-serializer` for future reuse. ([#2269](#2269)) ## @lynx-js/web-core-wasm@0.0.4 ### Patch Changes - Refactor web element templates and server-side rendering logic ([#2205](#2205)) - Updated dependencies \[[`94e5779`](94e5779), [`9033e2d`](9033e2d)]: - @lynx-js/web-elements@0.11.3 - @lynx-js/css-serializer@0.1.4 ## @lynx-js/web-elements@0.11.3 ### Patch Changes - fix: firefox 147+ layout issue ([#2205](#2205)) ## @lynx-js/template-webpack-plugin@0.10.4 ### Patch Changes - Move `cssChunksToMap` implementation from `@lynx-js/template-webpack-plugin` to `@lynx-js/css-serializer` for future reuse. ([#2269](#2269)) - Updated dependencies \[[`9033e2d`](9033e2d)]: - @lynx-js/css-serializer@0.1.4 ## @lynx-js/react-alias-rsbuild-plugin@0.12.9 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
This PR addresses gaps in the profiling implementation for functional components in ReactLynx and ensures full test coverage.
Changes
vnode.__c.__Hto intercept hook state in functional components.NEXT_VALUEaligns with Preact's internal__Nproperty.__BACKGROUND__is true.Testing & Coverage
__test__/debug/trace.test.jsxconsolidating all functionality and coverage tests.useStatetracing (updates, flowId generation).src/debug/profile.ts.Verification
Run tests:
pnpm test