fix(react): avoid retaining transformed worklet ctx#2591
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughNested worklet contexts are stored as ChangesWeak worklet context retention
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🦋 Changeset detectedLatest commit: 0ca730e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
Comparing 
Footnotes
|
React MTF Example#1192 Bundle Size — 206.65KiB (+0.02%).0ca730e(current) vs e51f9f1 main#1188(baseline) Bundle metrics
|
| Current #1192 |
Baseline #1188 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
46.16% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
192 |
192 |
| Duplicate Modules |
77 |
77 |
| Duplicate Code |
44.36% |
44.36% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #1192 |
Baseline #1188 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
95.42KiB (+0.05%) |
95.37KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-weak... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#8061 Bundle Size — 235.77KiB (0%).0ca730e(current) vs e51f9f1 main#8057(baseline) Bundle metrics
|
| Current #8061 |
Baseline #8057 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
197 |
197 |
| Duplicate Modules |
80 |
80 |
| Duplicate Code |
44.85% |
44.85% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8061 |
Baseline #8057 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
90.01KiB |
90.01KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-weak... Project dashboard
Generated by RelativeCI Documentation Report issue
React External#1174 Bundle Size — 690.27KiB (0%).0ca730e(current) vs e51f9f1 main#1170(baseline) Bundle metrics
|
| Current #1174 |
Baseline #1170 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch Yradex:wt/run-on-background-weak... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example with Element Template#327 Bundle Size — 197.79KiB (0%).0ca730e(current) vs e51f9f1 main#323(baseline) Bundle metrics
|
| Current #327 |
Baseline #323 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
81(+1.25%) |
80 |
| Duplicate Modules |
23 |
23 |
| Duplicate Code |
40.29%(-0.05%) |
40.31% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #327 |
Baseline #323 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
52.03KiB |
52.03KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-weak... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9634 Bundle Size — 900.04KiB (0%).0ca730e(current) vs e51f9f1 main#9630(baseline) Bundle metrics
|
| Current #9634 |
Baseline #9630 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229(+0.88%) |
227 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.27%(-0.04%) |
27.28% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9634 |
Baseline #9630 |
|
|---|---|---|
| JS |
495.91KiB |
495.91KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:wt/run-on-background-weak... Project dashboard
Generated by RelativeCI Documentation Report issue
Yradex
force-pushed
the
wt/run-on-background-weak-ctx
branch
from
68a306f to
b94bc1d
Compare
Yradex
force-pushed
the
wt/run-on-background-weak-ctx
branch
from
b94bc1d to
9d29a29
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/__test__/worklet-runtime/runOnBackground.test.js`:
- Around line 40-69: Add a new test in runOnBackground.test.js that mirrors the
existing "hydrate nested worklet ctx" case but uses the legacy .ctx shape
instead of ctxRef: create firstScreenChildCtx with _wkltId 'child' and
_jsFn._jsFn1._isFirstScreen true, attach it to firstScreenWorklet.child via
Object.assign(function(){}, { ctx: firstScreenChildCtx }), create a fresh
worklet object with matching child {_wkltId:'child',
_jsFn:{'_jsFn1':{_jsFnId:1}}, _execId:8}, call
globalThis.lynxWorkletImpl._hydrateCtx(worklet, firstScreenWorklet) and assert
that firstScreenChildCtx._jsFn._jsFn1 has _isFirstScreen false, _jsFnId 1 and
_execId 8 to verify the legacy .ctx fallback works.
In `@packages/react/runtime/src/worklet-runtime/hydrate.ts`:
- Around line 54-56: The hydration code reads only ctxRef?.deref() and misses
the legacy .ctx fallback; update the expression that computes firstScreenValue
(the access of firstScreenCtxObj[key]) to return the weak-ref deref OR, if that
yields undefined, fall back to the legacy fn.ctx value (e.g., use
ctxRef?.deref() ?? fn.ctx casted to ClosureValueType) so nested worklet contexts
hydrated from older payloads are preserved; change the branch that checks typeof
firstScreenCtxObj[key] === 'function' to use this combined fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f0d2434-5ddd-4b53-a45b-d8b7dbb0f5cb
📒 Files selected for processing (5)
.changeset/weak-worklet-context.mdpackages/react/runtime/__test__/worklet-runtime/runOnBackground.test.jspackages/react/runtime/src/worklet-runtime/bindings/types.tspackages/react/runtime/src/worklet-runtime/hydrate.tspackages/react/runtime/src/worklet-runtime/workletRuntime.ts
|
@coderabbitai review The PR description and changeset have been updated to align the stated scope with the implementation. Please re-review the latest head. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/react/runtime/src/worklet-runtime/hydrate.ts (1)
54-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing backward compatibility fallback to legacy
.ctxproperty.The PR objectives state that "Legacy
.ctxhandling is accounted for where present", but the current implementation only reads fromctxRef?.deref()without the fallback to.ctx.This breaks compatibility when first-screen contexts were transformed by older code (which stored
.ctxinstead of.ctxRef), causing hydration to fail withundefinedand skip hydrating nested worklet contexts.🔧 Proposed fix to add the compatibility fallback
} else { const firstScreenValue = typeof firstScreenCtxObj[key] === 'function' - ? (firstScreenCtxObj[key] as { ctxRef?: WeakRef<object> }).ctxRef?.deref() as ClosureValueType + ? ((firstScreenCtxObj[key] as { ctxRef?: WeakRef<object>; ctx?: ClosureValueType }).ctxRef?.deref() + ?? (firstScreenCtxObj[key] as { ctx?: ClosureValueType }).ctx) as ClosureValueType : firstScreenCtxObj[key]; hydrateCtxImpl(ctxObj[key], firstScreenValue, execId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/src/worklet-runtime/hydrate.ts` around lines 54 - 56, The current extraction of firstScreenValue in hydrate.ts only reads ctxRef?.deref() and misses legacy .ctx, so update the logic that computes firstScreenValue (for key on firstScreenCtxObj) to, when value is a function-shaped closure, first try (value as { ctxRef?: WeakRef<object> }).ctxRef?.deref() and if that is undefined fall back to the legacy .ctx property (e.g., (value as any).ctx); keep the non-function branch unchanged and ensure the resulting type still matches ClosureValueType so nested worklet contexts hydrate correctly.packages/react/runtime/__test__/worklet-runtime/runOnBackground.test.js (1)
40-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConsider adding test coverage for legacy
.ctxfallback once implemented.The past review comment correctly noted that backward compatibility test coverage is missing. Once the
.ctxfallback is implemented inhydrate.ts(see the critical issue flagged in that file), add a test case that verifies hydration works when the first-screen context has a function with.ctxinstead of.ctxRef.📝 Suggested test structure
it('should hydrate nested worklet ctx from legacy ctx property', () => { const firstScreenChildCtx = { _wkltId: 'child', _jsFn: { '_jsFn1': { '_isFirstScreen': true }, }, }; const firstScreenWorklet = { _wkltId: 'parent', child: Object.assign(function() {}, { ctx: firstScreenChildCtx, // legacy .ctx instead of ctxRef }), }; const worklet = { _wkltId: 'parent', child: { _wkltId: 'child', _jsFn: { '_jsFn1': { '_jsFnId': 1 }, }, }, _execId: 8, }; globalThis.lynxWorkletImpl._hydrateCtx(worklet, firstScreenWorklet); expect(firstScreenChildCtx._jsFn._jsFn1._isFirstScreen).toBe(false); expect(firstScreenChildCtx._jsFn._jsFn1._jsFnId).toBe(1); expect(firstScreenChildCtx._jsFn._jsFn1._execId).toBe(8); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/__test__/worklet-runtime/runOnBackground.test.js` around lines 40 - 69, Add a test that verifies the legacy ".ctx" fallback in the hydration logic: in packages/react/runtime/__test__/worklet-runtime/runOnBackground.test.js create a new it(...) that mirrors the existing "should hydrate nested worklet ctx from a weak ctx ref" test but set the first-screen function property to ctx (not ctxRef) on the firstScreenWorklet function, then call globalThis.lynxWorkletImpl._hydrateCtx(worklet, firstScreenWorklet) and assert the same expectations (update of _isFirstScreen, _jsFnId, and _execId). This ensures the _hydrateCtx implementation supports the legacy ctx fallback once hydrate.ts is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/react/runtime/__test__/worklet-runtime/runOnBackground.test.js`:
- Around line 40-69: Add a test that verifies the legacy ".ctx" fallback in the
hydration logic: in
packages/react/runtime/__test__/worklet-runtime/runOnBackground.test.js create a
new it(...) that mirrors the existing "should hydrate nested worklet ctx from a
weak ctx ref" test but set the first-screen function property to ctx (not
ctxRef) on the firstScreenWorklet function, then call
globalThis.lynxWorkletImpl._hydrateCtx(worklet, firstScreenWorklet) and assert
the same expectations (update of _isFirstScreen, _jsFnId, and _execId). This
ensures the _hydrateCtx implementation supports the legacy ctx fallback once
hydrate.ts is updated.
In `@packages/react/runtime/src/worklet-runtime/hydrate.ts`:
- Around line 54-56: The current extraction of firstScreenValue in hydrate.ts
only reads ctxRef?.deref() and misses legacy .ctx, so update the logic that
computes firstScreenValue (for key on firstScreenCtxObj) to, when value is a
function-shaped closure, first try (value as { ctxRef?: WeakRef<object>
}).ctxRef?.deref() and if that is undefined fall back to the legacy .ctx
property (e.g., (value as any).ctx); keep the non-function branch unchanged and
ensure the resulting type still matches ClosureValueType so nested worklet
contexts hydrate correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23f84ecf-818b-46ce-b832-fa39f67ac7bb
📒 Files selected for processing (5)
.changeset/weak-worklet-context.mdpackages/react/runtime/__test__/worklet-runtime/runOnBackground.test.jspackages/react/runtime/src/worklet-runtime/bindings/types.tspackages/react/runtime/src/worklet-runtime/hydrate.tspackages/react/runtime/src/worklet-runtime/workletRuntime.ts
|
@coderabbitai review The previous compatibility requirement came from stale PR wording, not from the intended supported contract of this change. The latest PR description and changeset now describe the actual scope: weak internal metadata for context recovery without adding an extra fallback path. Please re-review with that scope. |
|
✏️ Learnings added
✅ Actions performedFull review triggered. |
2 participants
Summary by CodeRabbit
Bug Fixes
Tests
Summary
workletCachevalue points back to the context it was meant to avoid retaining.WeakRef, so hydration can recover a still-live first-screen context without extending the lifetime of list-item worklet contexts.Key Points
The root cause is the extra strong context edge added after nested worklet transformation:
The bound function no longer needs the original object for execution, but that metadata kept the original object alive through any cache that retained the transformed function.
The transformed function now stores the original nested worklet context through weak metadata:
This preserves first-screen context recovery while avoiding a cache value that strongly retains the original context.
Hydration consumes the weak metadata from transformed first-screen functions:
This does not change public ReactLynx APIs, serialized native payloads, or the worklet registration contract.
The new runtime test covers the retain-cycle-sensitive shape directly: a transformed nested worklet function should expose weak context metadata, should not expose the old strong context metadata, and the weak reference should point to the original nested worklet context while it is still reachable.
Checklist