refactor: impl __SNAPSHOT__ at runtime #1736
Conversation
🦋 Changeset detectedLatest commit: 9d43c03 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
📝 WalkthroughWalkthroughRemoves the unresolved_mark field/parameter from JSXTransformer and its plumbing, updates transform plugin code and tests to the new constructor arity, adds a global test helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-21T07:21:51.621ZApplied to files:
📚 Learning: 2025-08-12T16:09:32.413ZApplied to files:
🧬 Code graph analysis (1)packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_list_toplevel.js (1)
🔇 Additional comments (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Web Explorer#5256 Bundle Size — 364.83KiB (-0.09%).9d43c03(current) vs 75693e4 main#5246(baseline) Bundle metrics
|
| Current #5256 |
Baseline #5246 |
|
|---|---|---|
 Initial JS |
145.41KiB(-0.16%) |
145.65KiB |
 Initial CSS |
31.89KiB |
31.89KiB |
 Cache Invalidation |
56.24% |
16.33% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
219(+0.92%) |
217 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.37% |
3.37% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 improvement
| Current #5256 |
Baseline #5246 |
|
|---|---|---|
| JS |
238.92KiB (-0.14%) |
239.25KiB |
| Other |
94.02KiB |
94.02KiB |
| CSS |
31.89KiB |
31.89KiB |
Bundle analysis report Branch refactor/impl-snapshot-marcro-at... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#5263 Bundle Size — 237.52KiB (0%).9d43c03(current) vs 75693e4 main#5253(baseline) Bundle metrics
|
| Current #5263 |
Baseline #5253 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
162 |
162 |
| Duplicate Modules |
65 |
65 |
| Duplicate Code |
46.71% |
46.71% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #5263 |
Baseline #5253 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.76KiB |
91.76KiB |
Bundle analysis report Branch refactor/impl-snapshot-marcro-at... Project dashboard
Generated by RelativeCI Documentation Report issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/react/runtime/__test__/utils/globals.js (1)
136-139: Guard against clobbering and handle undefined input.Avoid overwriting an existing
__SNAPSHOT__(some environments may pre-inject it) and tolerate falsysnapshot.- globalThis.__SNAPSHOT__ = (snapshot) => { - return snapshot.type; - }; + if (!globalThis.__SNAPSHOT__) { + globalThis.__SNAPSHOT__ = (snapshot) => snapshot?.type; + }packages/react/runtime/__test__/list.test.jsx (1)
236-257: Inline snapshots assert unstable IDs/tokens; prefer stable expectations.Most diffs are ID/token churn (e.g., list IDs -6/-5/-8 and event handler tokens). These are brittle. Recommend asserting structure and key fields while ignoring generated IDs.
Example refactor (one representative block), replacing a long inline snapshot with a shape check:
- expect(listRef).toMatchInlineSnapshot(` - <list - id="list" - update-list-info={ - [ - { - "insertAction": [ - { - "item-key": "key-0", - "position": 0, - "type": "__Card__:__snapshot_a94a8_test_21", - }, - ... - ], - "removeAction": [], - "updateAction": [], - }, - ] - } - > - <list-item item-key="key-4"> ... </list-item> - <list-item item-key="key-5"> ... </list-item> - <list-item item-key="key-2"> ... </list-item> - <list-item item-key="key-3"> ... </list-item> - </list> - `); + expect(listRef).toMatchObject({ + type: 'list', + props: { + id: 'list', + 'update-list-info': expect.arrayContaining([ + expect.objectContaining({ + insertAction: expect.arrayContaining([ + expect.objectContaining({ position: 0, type: expect.stringMatching(/__snapshot_/) }), + ]), + }), + ]), + }, + children: expect.arrayContaining([ + expect.objectContaining({ type: 'list-item', props: expect.objectContaining({ 'item-key': 'key-4' }) }), + expect.objectContaining({ type: 'list-item', props: expect.objectContaining({ 'item-key': 'key-5' }) }), + ]), + });If you want, I can batch-generate diffs to replace the most brittle snapshots with pattern-based assertions.
Also applies to: 272-299, 307-322, 355-378, 569-631, 1562-1583, 3666-3721
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/spotty-experts-smoke.md(1 hunks)packages/react/runtime/__test__/list.test.jsx(20 hunks)packages/react/runtime/__test__/utils/globals.js(1 hunks)packages/react/transform/src/lib.rs(0 hunks)packages/react/transform/src/swc_plugin_list/mod.rs(1 hunks)packages/react/transform/src/swc_plugin_snapshot/mod.rs(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react/transform/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/spotty-experts-smoke.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: upupming
PR: lynx-family/lynx-stack#1562
File: packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs:261-283
Timestamp: 2025-08-21T07:21:51.621Z
Learning: In packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs, the team prefers to keep the original unreachable! logic for JSXSpreadChild in jsx_is_children_full_dynamic function rather than implementing defensive error handling.
📚 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/__test__/utils/globals.js
📚 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/__test__/utils/globals.js
📚 Learning: 2025-08-21T07:21:51.621Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1562
File: packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs:261-283
Timestamp: 2025-08-21T07:21:51.621Z
Learning: In packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs, the team prefers to keep the original unreachable! logic for JSXSpreadChild in jsx_is_children_full_dynamic function rather than implementing defensive error handling.
Applied to files:
packages/react/transform/src/swc_plugin_snapshot/mod.rspackages/react/transform/src/swc_plugin_list/mod.rs
🧬 Code graph analysis (1)
packages/react/transform/src/swc_plugin_snapshot/mod.rs (3)
packages/react/transform/src/swc_plugin_compat/mod.rs (1)
new(410-422)packages/react/transform/src/swc_plugin_compat_post/mod.rs (1)
new(57-62)packages/react/transform/src/swc_plugin_inject/mod.rs (1)
new(106-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (2)
packages/react/transform/src/swc_plugin_list/mod.rs (1)
185-185: Approve import; confirm no remaining 4‑arg JSXTransformer::new callsSingleThreadedComments is used in tests to instantiate JSXTransformer::<&SingleThreadedComments>::new(...); import is correct. Ripgrep produced no output — re-run one of the checks below and attach results:
rg -n --type=rust -C2 'JSXTransformer::new\(' rg -nP --type=rust 'JSXTransformer::new\s*\((?:[^()]*,){3}'packages/react/transform/src/swc_plugin_snapshot/mod.rs (1)
1028-1049: Public API change — constructor now (cfg, comments, mode); update callers, docs, and release notes.
- Ensure every caller uses the new 3‑arg signature — notable call sites: packages/react/transform/src/lib.rs:481 and many visit_mut_pass(JSXTransformer::new(...)) occurrences in packages/react/transform/src/swc_plugin_snapshot/mod.rs; run: rg -nP --type=rust '\bJSXTransformer::new\s*(' packages/react/transform -S.
- Add a changeset and bump semver (minor/major as appropriate for a public API change).
- Update README/API docs to document the new signature (cfg, comments, mode).
CodSpeed Performance ReportMerging #1736 will not alter performanceComparing 🎉 Hooray!
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/react/runtime/__test__/utils/globals.js (1)
136-139: Harden SNAPSHOT helper (optional).Guard against unexpected inputs and avoid accidental reassignment by defining a non-enumerable property. Keeps tests resilient while remaining overridable if needed.
- globalThis.__SNAPSHOT__ = (snapshot) => { - return snapshot.type; - }; + Object.defineProperty(globalThis, '__SNAPSHOT__', { + value: (snapshot) => snapshot?.type ?? snapshot, + configurable: true, + });packages/react/runtime/__test__/list.test.jsx (1)
525-631: General note: consider stabilizing brittle snapshot fields (optional).If these IDs continue to churn due to internal ordering, consider asserting structure with expect.objectContaining/arrayContaining or targeted fields to reduce maintenance overhead. Keep exact IDs where they’re semantically significant.
Also applies to: 1062-1097, 1489-1538, 1665-1719, 1748-1855, 1864-1942, 2206-2282, 2359-2463, 2541-2710, 2757-2792, 2834-2854, 2910-2956, 2999-3033, 3055-3088, 3151-3187, 3331-3387, 3456-3501, 3511-3583, 3603-3611, 3733-3767, 3784-3911
packages/react/transform/src/swc_plugin_list/mod.rs (3)
97-108: Use a hygienic private ident for children to avoid name capture.
Ident::from("__c")can collide with user code. Prefer a fresh, hygienic ident.- let children_ident = Ident::from("__c"); + let children_ident = private_ident!("c");
167-179: Skip adding the runtime import if it already exists.If a file already imports '@lynx-js/react/runtime-components', this will add a duplicate import. Guard the insertion.
fn visit_mut_module_items(&mut self, n: &mut Vec<ModuleItem>) { - let mut new_items: Vec<ModuleItem> = vec![]; + let has_runtime_import = n.iter().any(|item| { + matches!( + item, + ModuleItem::ModuleDecl(ModuleDecl::Import(ImportDecl { src, .. })) + if &*src.value == "@lynx-js/react/runtime-components" + ) + }); + let mut new_items: Vec<ModuleItem> = vec![]; for item in n.iter_mut() { item.visit_mut_with(self); new_items.push(item.take()); } - if let Some(module_item) = &self.runtime_components_module_item { - new_items.insert(0, module_item.clone()); + if let Some(module_item) = &self.runtime_components_module_item && !has_runtime_import { + new_items.insert(0, module_item.clone()); } *n = new_items; }
64-74: Align the comment with the actualrenderListItemsignature.The code generates
($children) => ..., not(spread, children) => .... Update the comment to prevent confusion.- // renderListItem={(spread, children) => <list-item ... {...spread}>{children}</list-item>} + // renderListItem={(children) => <list-item ...>{children}</list-item>}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/spotty-experts-smoke.md(1 hunks)packages/react/runtime/__test__/list.test.jsx(20 hunks)packages/react/runtime/__test__/utils/globals.js(1 hunks)packages/react/transform/src/lib.rs(0 hunks)packages/react/transform/src/swc_plugin_list/mod.rs(1 hunks)packages/react/transform/src/swc_plugin_snapshot/mod.rs(1 hunks)packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static_snapshot_extract.js(0 hunks)packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static_snapshot_extract_it.js(0 hunks)packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_list_toplevel.js(2 hunks)
💤 Files with no reviewable changes (3)
- packages/react/transform/tests/swc_snapshots/src/swc_plugin_snapshot/mod.rs/basic_full_static_snapshot_extract_it.js
- packages/react/transform/src/lib.rs
- packages/react/transform/tests/swc_snapshots/src/swc_plugin_snapshot/mod.rs/basic_full_static_snapshot_extract.js
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
For contributions, always generate a changeset and commit the resulting markdown file(s)
Files:
.changeset/spotty-experts-smoke.md
🧠 Learnings (3)
📚 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/__test__/utils/globals.js
📚 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:
.changeset/spotty-experts-smoke.md
📚 Learning: 2025-08-21T07:21:51.621Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1562
File: packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs:261-283
Timestamp: 2025-08-21T07:21:51.621Z
Learning: In packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs, the team prefers to keep the original unreachable! logic for JSXSpreadChild in jsx_is_children_full_dynamic function rather than implementing defensive error handling.
Applied to files:
packages/react/transform/src/swc_plugin_snapshot/mod.rspackages/react/transform/src/swc_plugin_list/mod.rs
🧬 Code graph analysis (2)
packages/react/transform/src/swc_plugin_snapshot/mod.rs (3)
packages/react/transform/src/swc_plugin_list/mod.rs (1)
new(28-34)packages/react/transform/src/swc_plugin_compat_post/mod.rs (1)
new(57-62)packages/react/transform/src/swc_plugin_inject/mod.rs (1)
new(106-114)
packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_list_toplevel.js (1)
packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_list.js (4)
__snapshot_da39a_test_1(30-51)__snapshot_da39a_test_2(11-22)__snapshot_da39a_test_3(2-10)__snapshot_da39a_test_4(23-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
🔇 Additional comments (12)
.changeset/spotty-experts-smoke.md (1)
1-3: Empty changeset acknowledged and OK for CI.Per prior repo practice, an empty front matter changeset is acceptable to satisfy CI without generating release notes when touching packages under src/. No action needed.
packages/react/runtime/__test__/utils/globals.js (1)
86-143: Global test shims look consistent.The new SNAPSHOT aligns with tests that wrap JSX in SNAPSHOT(...). No other behavior changes introduced here.
packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_list_toplevel.js (1)
34-37: Snapshot fixture updates are consistent with runtime SNAPSHOT usage.Wrapping snapshot JSX with SNAPSHOT(...) matches the refactor removing transformer-time rewrites. Looks good.
Also applies to: 48-48
packages/react/runtime/__test__/list.test.jsx (5)
236-257: Pending updates ID reindexing acknowledged.The negative list IDs in __pendingListUpdates.values were re-numbered; expectations updated accordingly. No functional change detected.
Also applies to: 272-299, 307-322, 355-377
575-576: Event bind IDs re-indexed.Inline snapshot updates for "bindEvent:tap" IDs are consistent with the new allocation order.
Also applies to: 590-591, 605-606, 620-621
2036-2076: “list bug” expectations updated to new list IDs.The -5 key and associated actions reflect the new numbering; logic remains the same.
Also applies to: 2083-2117, 2125-2148
3568-3578: Lifecycle handler IDs shifted as expected.onComponentAtIndex/onRecycleComponent handlerName IDs updated; behavior unchanged.
3666-3686: Nested list: multiple list ID renumberings OK.Snapshot assertions for parent/child attached lists reflect the new stable order.
Also applies to: 3687-3697, 3699-3709, 3711-3721, 3731-3741, 3743-3753, 3755-3765
packages/react/transform/src/swc_plugin_snapshot/mod.rs (3)
1516-1550: Import injection still guarded and correct.The runtime import insertion remains intact post-refactor. No issues spotted.
1568-1711: Tests align with the new API and runtime behavior.Resolver/react passes remain unchanged; JSXTransformer tests updated appropriately.
Also applies to: 1713-1782, 1784-1830, 1832-1876, 1878-1935, 1937-2002, 2004-2033, 2035-2110, 2112-2161, 2163-2207, 2209-2255, 2257-2315, 2317-2355, 2357-2410, 2412-2463, 2465-2515, 2517-2566, 2568-2642
1028-1050: Constructor signature change looks correct; all call sites updated.Search of packages/react/transform/src found 20 JSXTransformer::new call sites — all use 3 arguments; no lingering 4-arg usages. SNAPSHOT occurs only in packages/react/transform/src/swc_plugin_snapshot/mod.rs; visit_mut_expr is still used in several plugins (expected).
packages/react/transform/src/swc_plugin_list/mod.rs (1)
185-185: Import update LGTM.Switching tests to
SingleThreadedCommentsaligns with the newJSXTransformer::<&SingleThreadedComments>::new(_, None, TransformMode)API.
...eact/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_list_toplevel.js
Outdated
Show resolved
Hide resolved
A cherry-pick of #1458
Summary by CodeRabbit
Tests
Refactor
Chores
No user-facing features or bug fixes in this release.
Checklist