fix(react): wrap <wrapper> for JSX with dynamic key
#1541
Conversation
🦋 Changeset detectedLatest commit: a1c4fe8 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 |
📝 WalkthroughWalkthroughThe PR updates the JSX transform to alter wrapper/merge behavior for dynamic keys and adjusts related test snapshots across runtime and testing-library suites. A changeset marks a patch release for @lynx-js/react referencing issue #1371. No exported/public APIs are changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(None) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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: 0
🧹 Nitpick comments (8)
packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
147-152: Stabilize the “random” value to avoid accidental test flakinessUsing Math.random is fine since you interpolate the same value into both the input and the inline snapshot, but a fixed deterministic value makes intent clearer and prevents subtle issues if this gets refactored. Consider hard-coding the number.
- const random = Math.random(); + const random = 0.123456789; // deterministic for stable snapshotAlso applies to: 171-172
packages/react/transform/src/swc_plugin_snapshot/slot_marker.rs (1)
146-153: Wrap children when is_list OR has_dynamic_key to enforce a single dynamic slotAdding has_dynamic_key and wrapping children into a single JSXExprContainer when either is_list or has_dynamic_key (and children are non-empty) directly addresses #1371 by preventing multiple DynamicPartChildren entries under a keyed parent with multiple children.
A short comment here would help future readers understand the intent:
- if (is_list || has_dynamic_key) && !n.children.is_empty() { + // If the parent is a list or has a dynamic key, wrap all children into one + // expression container so downstream passes emit a single dynamic slot. + if (is_list || has_dynamic_key) && !n.children.is_empty() {packages/react/runtime/__test__/preact.test.jsx (2)
352-390: Background snapshots updated: verify brittleness and intentThe background inline snapshots now reflect the new grouping. Given the frequent renumbering of _snapshot* tokens and structural reshuffles, consider reducing brittleness by asserting structure and semantics (presence of a single grouping wrapper and correct child order) instead of exact IDs, where feasible.
Would you like a helper/assertion that walks the background snapshot tree to validate shape (count of wrappers, children order) instead of relying on token IDs?
674-709: Hydration opcodes changed (-88/-89 etc.): confirm opcode map consistencyHydration patch opcodes changed (e.g., -68/-69 → -88/-89) and associated value positions shifted. Please confirm these constants remain consistent with the opcode map in runtime and any tooling that consumes them. If those numbers are derived, a short comment or constant alias would help readability and future diffs.
packages/react/testing-library/src/__tests__/render.test.jsx (1)
51-90: Dynamic key wrapper moved inside keyed view — regression fixedThe wrapper now sits inside the keyed and groups the dynamic children. This directly addresses the regression described in #1371 by ensuring a single grouped dynamic-children part.
Optional: add a structural assertion to ensure exactly one exists under the keyed view and it contains both children, to guard against future regressions without depending on snapshot IDs.
packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/slot_marker.rs/should_wrap_dynamic_part.js (1)
23-27: Transformer snapshot aligns with wrapper-on-dynamic-key policy
- Dynamic keyed now wraps the dynamic content in an internal-slot + wrapper, ensuring a single grouped dynamic part.
- In subsequent cases, keyed is placed directly under internal-slot (no extra wrapper) where appropriate.
These match the updated transform logic and the intent to avoid multiple __DynamicPartChildren under the same slot.
Consider adding one more transformer test where a keyed element has a mix of text and element children to ensure the wrapper consistently groups all siblings in that slot.
packages/react/testing-library/src/__tests__/list.test.jsx (2)
88-89: UID expectation changed — verify rationaleUID advanced from 5 to 6. That’s expected if the additional wrapper participates in UI sign allocation. Just calling it out to confirm it’s deterministic across environments.
386-389: elementID changed 36 → 33 — confirm stabilityThe elementID in the flush payload changed. Likely due to the additional wrapper affecting ID allocation. Please confirm this remains deterministic under parallel test runs.
📜 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 (9)
.changeset/modern-bags-wait.md(1 hunks)packages/react/runtime/__test__/basic.test.jsx(4 hunks)packages/react/runtime/__test__/preact.test.jsx(11 hunks)packages/react/runtime/__test__/renderToOpcodes.test.jsx(2 hunks)packages/react/testing-library/src/__tests__/list.test.jsx(18 hunks)packages/react/testing-library/src/__tests__/render.test.jsx(8 hunks)packages/react/transform/src/swc_plugin_snapshot/slot_marker.rs(2 hunks)packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/should_wrap_dynamic_key.js(1 hunks)packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/slot_marker.rs/should_wrap_dynamic_part.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: upupming
PR: lynx-family/lynx-stack#1502
File: packages/react/testing-library/types/entry.d.ts:71-71
Timestamp: 2025-08-13T09:20:00.936Z
Learning: In lynx-js/react testing library, wrapper components must have children as a required prop because they are always called with `h(WrapperComponent, null, innerElement)` where innerElement is passed as children. The type `React.JSXElementConstructor<{ children: React.ReactNode }>` correctly requires children to be mandatory.
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1500
File: .changeset/curvy-dragons-appear.md:1-5
Timestamp: 2025-08-13T11:50:58.069Z
Learning: In the lynx-family/lynx-stack repository, all packages under packages/react/ should use "lynx-js/react" in changesets since they are all published together under the same package name, regardless of their individual package.json names. This includes packages/react/testing-library and other sub-packages.
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/modern-bags-wait.md
📚 Learning: 2025-08-13T11:50:58.069Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1500
File: .changeset/curvy-dragons-appear.md:1-5
Timestamp: 2025-08-13T11:50:58.069Z
Learning: In the lynx-family/lynx-stack repository, all packages under packages/react/ should use "lynx-js/react" in changesets since they are all published together under the same package name, regardless of their individual package.json names. This includes packages/react/testing-library and other sub-packages.
Applied to files:
.changeset/modern-bags-wait.md
📚 Learning: 2025-08-07T04:00:59.645Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.645Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Applied to files:
.changeset/modern-bags-wait.md
📚 Learning: 2025-08-13T09:23:36.222Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1502
File: packages/react/testing-library/types/entry.d.ts:97-97
Timestamp: 2025-08-13T09:23:36.222Z
Learning: React Testing Library's rerender function accepts React.ReactNode (including strings, numbers, null, etc.), not just React.ReactElement. The typing should match RTL's behavior by using React.ReactNode for maximum compatibility.
Applied to files:
packages/react/testing-library/src/__tests__/render.test.jsx
🧬 Code Graph Analysis (2)
packages/react/runtime/__test__/renderToOpcodes.test.jsx (1)
packages/react/transform/src/wasm.js (1)
view(24-28)
packages/react/transform/src/swc_plugin_snapshot/slot_marker.rs (1)
packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs (2)
jsx_is_custom(207-217)jsx_has_dynamic_key(219-239)
🔇 Additional comments (24)
.changeset/modern-bags-wait.md (1)
2-7: Changeset looks correct and scoped to @lynx-js/react
- Package name matches repo convention for react subpackages.
- Description concisely ties to issue #1371 and explains user-facing fix.
packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/should_wrap_dynamic_key.js (1)
51-51: Snapshot change aligns with single-slot semantics under a dynamic keyRemoving the extra wrapper around the inner dynamic element and keeping one wrapper for the free text ensures we don’t produce multiple __DynamicPartChildren entries for a keyed parent with multiple children. This matches the intended fix for #1371.
packages/react/runtime/__test__/basic.test.jsx (3)
372-398: “multiple slots 0” snapshot now shows a single wrapper under the keyed parent — this is the desired fixThe keyed now contains a single empty wrapper instead of multiple dynamic children slots. This prevents the runtime from seeing multiple __DynamicPartChildren entries, matching the bug’s root cause.
417-423: “multiple slots 2” snapshot update confirms consolidation to one top-level wrapperWith the key applied on the outer node, children are wrapped into a single wrapper slot. This is consistent with the new transform behavior and should render correctly on first paint.
452-456: “multiple slots 3” snapshot preserves existing wrapper under text and adds a single wrapper under the keyed viewThis keeps slot boundaries well-defined and avoids duplicated children slots under the keyed element.
packages/react/transform/src/swc_plugin_snapshot/slot_marker.rs (1)
85-86: Merging rule for JSXElement children simplified to ignore dynamic key presenceSetting should_merge = !jsx_is_custom(element) treats host elements as mergeable and custom elements as dynamic boundaries. Given the parent-level handling of dynamic keys introduced below, this simplification is sound and reduces wrapper churn.
If you want extra guardrails, we can add/verify a transform test where a host child has its own dynamic key but the parent also has a dynamic key, ensuring only a single slot is emitted for the parent’s children.
packages/react/runtime/__test__/preact.test.jsx (4)
283-296: ResultCell: wrapper placement around conditional block looks correctThe wrapper now encloses the conditional description branch only, leaving the rest of the structure untouched. This aligns with the goal of grouping dynamic children under a single wrapper and should prevent multiple dynamic-children parts.
599-616: Lazy test snapshots: wrapper staging between suspense phases looks consistentThe intermediate fallback and eventual resolved state snapshots reflect the staged wrapper behavior. No issues spotted.
647-671: Dual-runtime background snapshot: per-char wrappers and props — sanity checkThe per-character keyed text pieces each appear as their own wrapped card, which is consistent with the dynamic-key grouping strategy. Looks good.
711-746: Global snapshot patch expectations updated: verify no-op before init still holdsThe takeGlobalSnapshotPatch() expectations are adjusted and remain empty pre-init, as intended. No concerns beyond the opcode-map consistency noted above.
packages/react/testing-library/src/__tests__/render.test.jsx (4)
109-134: Dynamic key with preceding sibling: wrapper placement remains correctWith a sibling Hello {null} before the keyed view, the wrapper is still applied only within the keyed container. Looks good.
137-180: Dynamic key with following sibling: wrapper placement remains correctThe keyed container’s children are grouped, and unrelated trailing content is unaffected. This matches the intended isolation.
198-219: Root keyed container: inner grouping under wrapper is correctFor a keyed root view, the wrapper correctly groups the inner sibling views, preventing multiple dynamic-child parts from leaking out.
222-260: Nested keys: inner keyed child coexists with outer keyed wrapperThe outer keyed view has a wrapper grouping both its children; the inner keyed child remains intact. This ensures stable reconciliation across nested keyed elements with multiple children.
packages/react/testing-library/src/__tests__/list.test.jsx (10)
50-55: List basic: immediate child of list-item is now a wrapper — OKEach list-item’s immediate child is a wrapper that encloses the prior single . This improves consistency with the dynamic-key grouping behavior.
Also applies to: 69-74, 99-104, 129-134
198-207: update-list-info type IDs shifted — expected churnType identifiers moved (…_6 → …_7). Snapshot churn is expected with wrapper insertion. No action needed.
224-239: Complex list-item: wrapper groups both static siblings and dynamic slotThe wrapper now encloses both the two number texts and the “hello” subtree. This ensures a single dynamic part for the item content and should help reuse during list virtualization.
276-297: Reuse path after removal: wrapper maintained; attributes updated correctlyAfter reusing the previous element for a different item-key, the wrapper and children are intact; only attributes update. This is the expected minimal-diff behavior.
299-360: Attribute set calls: still minimal — goodThe number and nature of __SetAttribute calls remain minimal given the reuse. Wrapper insertion did not introduce extra attribute churn beyond the necessary updates.
398-481: Final list snapshot with wrappers per item — consistent across itemsAll visible items now render with a single wrapper containing their children. The structure is consistent and should be friendlier for hydration and reuse.
568-572: Deferred list-items: dynamic inline numbers are now wrapped — LGTMThe “Item {index+1}” dynamic parts are correctly wrapped, isolating them within the text node. This reduces ambiguity in patching and hydration.
Also applies to: 584-588, 600-604
661-664: Deferred switch test: update-list-info type IDs shifted — as expectedIDs updated to new snapshots for the list-item templates. No issues spotted.
719-722: Unmount-on-reuse test: type IDs advanced — expectedTemplate IDs changed due to the wrapper adjustments; behavior under reuse still validated by the test.
769-773: Unmount-on-reuse: inline number wrappers — OKDynamic numeric segments wrapped in are rendered as expected. This keeps the unmount/reuse logic unaffected while ensuring consistent dynamic-part grouping.
Also applies to: 785-789
CodSpeed Performance ReportMerging #1541 will not alter performanceComparing Summary
|
Web Explorer#4290 Bundle Size — 348.12KiB (0%).a1c4fe8(current) vs 3fe1708 main#4286(baseline) Bundle metrics
|
| Current #4290 |
Baseline #4286 |
|
|---|---|---|
 Initial JS |
143.37KiB |
143.37KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
| Modules |
211 |
211 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.94% |
3.94% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4290 |
Baseline #4286 |
|
|---|---|---|
| JS |
233.32KiB |
233.32KiB |
| Other |
82.95KiB |
82.95KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch colinaaa:colin/0815/slot-key Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4297 Bundle Size — 237.04KiB (0%).a1c4fe8(current) vs 3fe1708 main#4293(baseline) Bundle metrics
|
| Current #4297 |
Baseline #4293 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
158 |
158 |
| Duplicate Modules |
64 |
64 |
| Duplicate Code |
45.86% |
45.86% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4297 |
Baseline #4293 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.28KiB |
91.28KiB |
Bundle analysis report Branch colinaaa:colin/0815/slot-key Project dashboard
Generated by RelativeCI Documentation Report issue
Summary by CodeRabbit
Bug Fixes
Tests
Chores
This partially reverts #547.
close: #1371
Checklist