feat(et): add template definition transform#2506
Closed
Yradex wants to merge 2 commits intolynx-family:mainfrom
Closed
feat(et): add template definition transform#2506Yradex wants to merge 2 commits intolynx-family:mainfrom
Yradex wants to merge 2 commits intolynx-family:mainfrom
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 opt-in element-template support across the React JSX transformer: new Rust module and types for template assets, transformer wiring and APIs, N‑API bindings, TS surface updates, and new JS/Rust tests to collect and validate element-template assets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Yradex
force-pushed
the
wt/element-template
branch
from
5ca2e8c to
0ac339c
Compare
React Example#7550 Bundle Size — 225.23KiB (0%).8355f1e(current) vs 30f0277 main#7546(baseline) Bundle metrics
|
| Current #7550 |
Baseline #7546 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.57% |
44.57% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7550 |
Baseline #7546 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
79.47KiB |
79.47KiB |
Bundle analysis report Branch Yradex:wt/element-template Project dashboard
Generated by RelativeCI Documentation Report issue
React External#668 Bundle Size — 679.93KiB (0%).8355f1e(current) vs 30f0277 main#664(baseline) Bundle metrics
|
| Current #668 |
Baseline #664 |
|
|---|---|---|
| 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/element-template Project dashboard
Generated by RelativeCI Documentation Report issue
Yradex
force-pushed
the
wt/element-template
branch
from
0ac339c to
b875ef5
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
Web Explorer#9122 Bundle Size — 900.11KiB (0%).8355f1e(current) vs 30f0277 main#9118(baseline) Bundle metrics
|
| Current #9122 |
Baseline #9118 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
 Cache Invalidation |
0% |
11.72% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229(+0.44%) |
228 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.29% |
27.29% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9122 |
Baseline #9118 |
|
|---|---|---|
| JS |
495.97KiB |
495.97KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:wt/element-template Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#682 Bundle Size — 196.39KiB (0%).8355f1e(current) vs 30f0277 main#678(baseline) Bundle metrics
|
| Current #682 |
Baseline #678 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
44.07% |
44.07% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #682 |
Baseline #678 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
85.15KiB |
85.15KiB |
Bundle analysis report Branch Yradex:wt/element-template Project dashboard
Generated by RelativeCI Documentation Report issue
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b875ef59c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/transform/crates/swc_plugin_snapshot/lib.rs (1)
641-684:⚠️ Potential issue | 🟠 MajorAvoid consuming hidden ET attr slots for list-item platform attrs.
In ET mode this branch removes list-item platform attrs and pushes a
ListItemPlatformInfodynamic attr, butcompiledTemplateis generated later from the mutated JSX. That leavesattributeSlotswith a value that has no descriptor, and shifts later slot indices.🐛 Proposed fix
- if jsx_is_list_item(n) { + if jsx_is_list_item(n) && !self.enable_element_template { if has_spread_element { } else { let mut list_item_platform_info: Vec<JSXAttr> = vec![];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 641 - 684, The code currently removes list-item platform attributes from n.opening.attrs via the retain_mut closure (matching in jsx_is_list_item branch) which mutates the JSX and consumes hidden ET attribute slots; instead, change the logic so you collect/clone platform attrs into list_item_platform_info but do NOT remove them from n.opening.attrs — i.e., in the retain_mut closure for JSXAttrOrSpread::JSXAttr matches (symbols like "reuse-identifier", "full-span", "item-key", etc.) return true (keep the attr) while still pushing a cloned attr into list_item_platform_info, and only remove actual SpreadElement entries if needed; keep using push_dynamic_attr(Expr::Object(...), AttrName::ListItemPlatformInfo) as before but ensure compiledTemplate and attributeSlots remain consistent because you no longer mutate n.opening.attrs.
🧹 Nitpick comments (6)
packages/react/transform/crates/swc_plugin_snapshot/Cargo.toml (1)
28-29: Consider movinginstato workspace dependencies.All other deps in this crate use
{ workspace = true }. Ifinstais (or will be) used in other workspace crates for snapshot testing, consolidating the version in the workspaceCargo.tomlavoids version drift. Not a blocker for a dev-only dep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/Cargo.toml` around lines 28 - 29, Move the insta dev-dependency out of this crate's Cargo.toml and declare it centrally in the workspace Cargo.toml instead: remove the line `insta = { version = "1.34", features = ["json"] }` from packages/react/transform/crates/swc_plugin_snapshot/Cargo.toml and add an equivalent entry under `[dev-dependencies]` in the workspace Cargo.toml using the workspace reference (e.g. `insta = { workspace = true, features = ["json"] }` or declare version+features at workspace level), ensuring the `json` feature is preserved so tests depending on insta continue to work.packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs (1)
97-201: Tests re-transform the same fixture twice; share one invocation to avoid silent drift.Both
should_not_inject_root_css_scope_attrs_for_element_templateandshould_not_inject_root_entry_name_attr_for_dynamic_component_element_templateduplicate the fixture string between atransform_to_code(...)call and afirst_user_template_json[_with_cfg](...)call, then assert against each output independently. If someone later edits one copy but not the other, the code-level and template-JSON-level assertions would silently stop covering the same input.Since
transform_fixturealready returns both templates and code, expose a helper that returns both and point each contract test at a single fixture string:♻️ Suggested helper refactor
+fn first_user_template_and_code(input: &str, cfg: JSXTransformerConfig) -> (Value, String) { + let (templates, code) = transform_fixture(input, cfg); + let tpl = templates + .into_iter() + .find(|t| t.template_id != BUILTIN_RAW_TEXT_TEMPLATE_ID) + .map(|t| serde_json::to_value(t.compiled_template).expect("compiled template to json")) + .expect("should collect a user template"); + (tpl, code) +}Then each contract test becomes one call instead of two.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs` around lines 97 - 201, The tests duplicate the same fixture string by calling transform_to_code(...) and first_user_template_json[_with_cfg](...), which can drift silently; replace the pair of calls in should_not_inject_root_css_scope_attrs_for_element_template and should_not_inject_root_entry_name_attr_for_dynamic_component_element_template with a single call to a shared helper that returns both the transformed code and the template JSON (reuse/extend the existing transform_fixture/transform_to_code infrastructure to return (code, template) together), then assert against those two outputs so each test uses one canonical fixture string and no duplicated literals.packages/react/transform/src/lib.rs (1)
460-503: Branching logic is correct; minor dedup opportunity.The
enabled=falsebranch still constructs aJSXTransformer::new(...)that it immediately wraps inOptional::new(_, false)(i.e., never runs). This is functionally fine but wastes aJSXTransformer::newcall and duplicates config cloning. You can simplify by always going throughnew_with_element_templateswithelement_templates_collectorderived only from the flag:♻️ Optional simplification
- let (snapshot_plugin, element_templates_collector) = if enabled { - // ET template assets are a build artifact, not runtime JS. Keep collection - // behind the experimental flag so ordinary Snapshot transforms do not pay - // for the side channel or expose an empty metadata field. - let element_templates_collector = - export_element_templates.then(|| Rc::new(RefCell::new(vec![]))); - let transformer = JSXTransformer::new_with_element_templates( - snapshot_plugin_config.clone(), - Some(&comments), - options.mode.unwrap_or(TransformMode::Production), - Some(cm.clone()), - element_templates_collector.clone(), - ) - .with_content_hash(content_hash.clone()); - - let transformer = if enable_ui_source_map { - transformer.with_ui_source_map_records(ui_source_map_records.clone()) - } else { - transformer - }; - - ( - Optional::new(visit_mut_pass(transformer), true), - element_templates_collector, - ) - } else { - ( - Optional::new( - visit_mut_pass(JSXTransformer::new( - snapshot_plugin_config.clone(), - Some(&comments), - options.mode.unwrap_or(TransformMode::Production), - Some(cm.clone()), - )), - false, - ), - None, - ) - }; + // ET template assets are a build artifact, not runtime JS. Keep collection + // behind the experimental flag so ordinary Snapshot transforms do not pay + // for the side channel or expose an empty metadata field. + let element_templates_collector = (enabled && export_element_templates) + .then(|| Rc::new(RefCell::new(vec![]))); + let transformer = JSXTransformer::new_with_element_templates( + snapshot_plugin_config.clone(), + Some(&comments), + options.mode.unwrap_or(TransformMode::Production), + Some(cm.clone()), + element_templates_collector.clone(), + ) + .with_content_hash(content_hash.clone()); + let transformer = if enable_ui_source_map { + transformer.with_ui_source_map_records(ui_source_map_records.clone()) + } else { + transformer + }; + let snapshot_plugin = Optional::new(visit_mut_pass(transformer), enabled);Not a correctness concern either way —
Optional::new(_, false)safely no-ops the transformer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/src/lib.rs` around lines 460 - 503, The code duplicates construction of a JSXTransformer when enabled is false; instead always create element_templates_collector from export_element_templates (using export_element_templates.then(|| Rc::new(RefCell::new(vec![])))) and call JSXTransformer::new_with_element_templates once (passing snapshot_plugin_config.clone(), Some(&comments), options.mode.unwrap_or(TransformMode::Production), Some(cm.clone()), element_templates_collector.clone()), then wrap that transformer with visit_mut_pass and Optional::new using the enabled boolean; preserve the existing with_content_hash and with_ui_source_map conditional chaining and return the element_templates_collector alongside the Optional transformer as before.packages/react/transform/__test__/fixture.spec.js (1)
167-193: Strengthen the smoke-test assertions beyondlength > 0.This Node-side test is the only place that guards the N-API serialization contract for
elementTemplates(camelCase field names,compiledTemplatebeing a plain object, non-emptytemplateId). A bare length check will silently pass even if, say,sourceFilegets dropped orcompiledTemplatebecomes a string. Consider a few cheap shape assertions:♻️ Suggested tightening
- expect(result.elementTemplates?.length).toBeGreaterThan(0); + expect(result.elementTemplates?.length).toBeGreaterThan(0); + const [tpl] = result.elementTemplates; + expect(tpl).toEqual(expect.objectContaining({ + templateId: expect.stringMatching(/^(_et_|__et_builtin_raw_text__)/), + sourceFile: expect.any(String), + compiledTemplate: expect.any(Object), + })); + // Non-builtin templates should be referenced by the emitted JSX code. + for (const t of result.elementTemplates) { + if (t.templateId !== '__et_builtin_raw_text__') { + expect(result.code).toContain(t.templateId); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/__test__/fixture.spec.js` around lines 167 - 193, The test currently only checks result.elementTemplates?.length > 0; strengthen it by asserting the shape of the first template returned: verify result.elementTemplates is an array, take the first item (e.g., const tpl = result.elementTemplates[0]) and assert tpl.templateId is a non-empty string, tpl.compiledTemplate is a plain object (not a string), and tpl.sourceFile (or other expected fields) exist and are strings; update the test that calls transformReactLynx to perform these assertions to protect the N-API serialization contract for elementTemplates.packages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rs (2)
540-542:set_varfrom tests is a shared mutable side effect.
std::env::set_var("INSTA_UPDATE", "always")mutates process-wide state, which is safe on Rust 2021 but becomesunsafein 2024 edition and is inherently racy under parallel test execution. Prefer settingINSTA_UPDATE=1directly when invoking tests (the pattern insta itself documents), or set it once in a#[ctor]/std::sync::Onceguarded helper. Not a correctness issue today, but worth cleaning up before a 2024-edition bump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rs` around lines 540 - 542, The test currently calls std::env::set_var("INSTA_UPDATE", "always") which mutates process-wide state and is racy for parallel tests and Rust 2024; remove the direct set_var call in the element_template.rs test and instead either (A) instruct callers to set INSTA_UPDATE when invoking tests (i.e. export INSTA_UPDATE=1 / use UPDATE env externally) or (B) replace the ad-hoc set with a one-time guarded initializer (use a static std::sync::Once or a #[ctor] helper) that calls std::env::set_var exactly once; locate the conditional that checks std::env::var("UPDATE") and change the branch to use one of these two safe approaches rather than calling std::env::set_var unguarded.
54-133: Hand-rolledattributeSlotsparser is brittle; fine for current codegen but guard the underflows.
square_depth -= 1,brace_depth -= 1, andparen_depth -= 1can each underflow (panic in debug builds) if the scanned text ever has a closing delimiter without a matching open before it — e.g., if a future emitter change uses template literals containing${...}interpolations (the backtick branch currently swallows everything until the next backtick, including}chars, so top-level interpolated}s would skew state once the string ends). Consider saturating subtraction and/or a graceful bail-out so a regression in the transformer produces a clearNonerather than a noisy panic that masks the real diff:♻️ Defensive tweak
- ']' if square_depth == 0 && brace_depth == 0 && paren_depth == 0 => { + ']' if square_depth == 0 && brace_depth == 0 && paren_depth == 0 => { return Some(if has_content { len } else { 0 }); } - ']' => square_depth -= 1, + ']' => square_depth = square_depth.saturating_sub(1), '{' => { has_content = true; brace_depth += 1; } - '}' => brace_depth -= 1, + '}' => brace_depth = brace_depth.saturating_sub(1), '(' => { has_content = true; paren_depth += 1; } - ')' => paren_depth -= 1, + ')' => paren_depth = paren_depth.saturating_sub(1),Alternatively, parse the codegen output back with
swc_ecma_parserand count elements in theattributeSlotsarray AST — more work but far less fragile. Test-only code so this is strictly a nice-to-have.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rs` around lines 54 - 133, The parser in first_attribute_slots_len can panic on underflow when decrementing square_depth/brace_depth/paren_depth; update the closing-delimiter handling (the '-' lines: square_depth -= 1, brace_depth -= 1, paren_depth -= 1) to guard against underflow by checking if the depth is > 0 before decrementing and otherwise return None (or bail) so malformed/changed codegen yields a graceful None instead of a panic; change the three decrements in first_attribute_slots_len to either use usize::saturating_sub(1) or an if depth == 0 { return None } else { depth -= 1 } pattern and keep the rest of the function logic intact.
🤖 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/transform/crates/swc_plugin_snapshot/element_template.rs`:
- Around line 407-426: The code currently ignores shorthand boolean attributes
and bypasses the project’s JSX string normalization; change the attr handling so
that when attr.value is None you treat it as a shorthand boolean and set
static_value = Some(Expr::Lit(Lit::Bool(true))) (preserving the implicit true
for names like <view flatten />), and for JSXAttrValue::Str(s) and string
literals inside JSXExprContainer reuse the existing Snapshot JSX string
normalization helper (the same normalization used elsewhere in the Snapshot
path) instead of using s.clone() directly so multiline/escaped attributes are
normalized consistently; update the match for value (and the JSXAttrValue::Str
and Expr::Lit(Lit::Str) arms) to call that normalization helper and return
Expr::Lit(Lit::Str(normalized)) while keeping numeric/null/bool literal arms
as-is.
In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs`:
- Line 1417: The code sets let use_element_template =
self.cfg.experimental_enable_element_template without verifying the presence of
the element template collector, which causes emitting <_et_... /> with no
compiledTemplate when JSXTransformer::new was constructed without an
element_templates collector; change the check so use_element_template is true
only when both self.cfg.experimental_enable_element_template is true AND
self.element_templates (or the element_templates side-channel/collector used by
JSXTransformer) is Some/available, and update the emission/drop logic where
templates/assets are handled (the block that currently drops the template asset
and the emission of compiledTemplate) to guard on the same presence check so no
ET JSX is emitted unless element_templates exists.
In `@packages/react/transform/index.d.ts`:
- Around line 658-664: Add per-field JSDoc `/// `@internal`` annotations to the
ElementTemplateAsset interface fields (templateId, compiledTemplate, sourceFile)
and to the TransformNodiffOutput.elementTemplates field so they are marked
internal in the generated .d.ts; update the #[napi(object)] struct field
comments in the Rust types corresponding to ElementTemplateAsset and
TransformNodiffOutput in the napi layer (napi.rs and lib.rs locations referenced
in the review) to include `/// `@internal`` on each field, then regenerate the
TypeScript definitions so ElementTemplateAsset and
TransformNodiffOutput.elementTemplates are emitted as internal.
---
Outside diff comments:
In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs`:
- Around line 641-684: The code currently removes list-item platform attributes
from n.opening.attrs via the retain_mut closure (matching in jsx_is_list_item
branch) which mutates the JSX and consumes hidden ET attribute slots; instead,
change the logic so you collect/clone platform attrs into
list_item_platform_info but do NOT remove them from n.opening.attrs — i.e., in
the retain_mut closure for JSXAttrOrSpread::JSXAttr matches (symbols like
"reuse-identifier", "full-span", "item-key", etc.) return true (keep the attr)
while still pushing a cloned attr into list_item_platform_info, and only remove
actual SpreadElement entries if needed; keep using
push_dynamic_attr(Expr::Object(...), AttrName::ListItemPlatformInfo) as before
but ensure compiledTemplate and attributeSlots remain consistent because you no
longer mutate n.opening.attrs.
---
Nitpick comments:
In `@packages/react/transform/__test__/fixture.spec.js`:
- Around line 167-193: The test currently only checks
result.elementTemplates?.length > 0; strengthen it by asserting the shape of the
first template returned: verify result.elementTemplates is an array, take the
first item (e.g., const tpl = result.elementTemplates[0]) and assert
tpl.templateId is a non-empty string, tpl.compiledTemplate is a plain object
(not a string), and tpl.sourceFile (or other expected fields) exist and are
strings; update the test that calls transformReactLynx to perform these
assertions to protect the N-API serialization contract for elementTemplates.
In `@packages/react/transform/crates/swc_plugin_snapshot/Cargo.toml`:
- Around line 28-29: Move the insta dev-dependency out of this crate's
Cargo.toml and declare it centrally in the workspace Cargo.toml instead: remove
the line `insta = { version = "1.34", features = ["json"] }` from
packages/react/transform/crates/swc_plugin_snapshot/Cargo.toml and add an
equivalent entry under `[dev-dependencies]` in the workspace Cargo.toml using
the workspace reference (e.g. `insta = { workspace = true, features = ["json"]
}` or declare version+features at workspace level), ensuring the `json` feature
is preserved so tests depending on insta continue to work.
In
`@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs`:
- Around line 97-201: The tests duplicate the same fixture string by calling
transform_to_code(...) and first_user_template_json[_with_cfg](...), which can
drift silently; replace the pair of calls in
should_not_inject_root_css_scope_attrs_for_element_template and
should_not_inject_root_entry_name_attr_for_dynamic_component_element_template
with a single call to a shared helper that returns both the transformed code and
the template JSON (reuse/extend the existing transform_fixture/transform_to_code
infrastructure to return (code, template) together), then assert against those
two outputs so each test uses one canonical fixture string and no duplicated
literals.
In
`@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rs`:
- Around line 540-542: The test currently calls
std::env::set_var("INSTA_UPDATE", "always") which mutates process-wide state and
is racy for parallel tests and Rust 2024; remove the direct set_var call in the
element_template.rs test and instead either (A) instruct callers to set
INSTA_UPDATE when invoking tests (i.e. export INSTA_UPDATE=1 / use UPDATE env
externally) or (B) replace the ad-hoc set with a one-time guarded initializer
(use a static std::sync::Once or a #[ctor] helper) that calls std::env::set_var
exactly once; locate the conditional that checks std::env::var("UPDATE") and
change the branch to use one of these two safe approaches rather than calling
std::env::set_var unguarded.
- Around line 54-133: The parser in first_attribute_slots_len can panic on
underflow when decrementing square_depth/brace_depth/paren_depth; update the
closing-delimiter handling (the '-' lines: square_depth -= 1, brace_depth -= 1,
paren_depth -= 1) to guard against underflow by checking if the depth is > 0
before decrementing and otherwise return None (or bail) so malformed/changed
codegen yields a graceful None instead of a panic; change the three decrements
in first_attribute_slots_len to either use usize::saturating_sub(1) or an if
depth == 0 { return None } else { depth -= 1 } pattern and keep the rest of the
function logic intact.
In `@packages/react/transform/src/lib.rs`:
- Around line 460-503: The code duplicates construction of a JSXTransformer when
enabled is false; instead always create element_templates_collector from
export_element_templates (using export_element_templates.then(||
Rc::new(RefCell::new(vec![])))) and call
JSXTransformer::new_with_element_templates once (passing
snapshot_plugin_config.clone(), Some(&comments),
options.mode.unwrap_or(TransformMode::Production), Some(cm.clone()),
element_templates_collector.clone()), then wrap that transformer with
visit_mut_pass and Optional::new using the enabled boolean; preserve the
existing with_content_hash and with_ui_source_map conditional chaining and
return the element_templates_collector alongside the Optional transformer as
before.
🪄 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: 7b739c9b-8d22-46cb-9653-c11eaf65f786
⛔ Files ignored due to path filters (27)
Cargo.lockis excluded by!**/*.lockpackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_generate_attribute_slots_for_dynamic_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_background_conditional_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_boolean_and_number_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_complex_text_structure.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_dataset_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_dynamic_class_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_events_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_events_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_id_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_inline_styles.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_mixed_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_nested_structure_and_dynamic_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_page_element.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_refs_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_refs_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_sibling_user_components.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_spread_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_user_component.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_isolate_arrays_with_slot_wrapper.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_keep_code_and_template_attribute_slots_in_sync_for_spread.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_output_element_template_simple_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_output_template_with_static_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_verify_template_structure_complex.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_verify_text_attribute_and_child_text_slots.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/react/transform/Cargo.tomlpackages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_snapshot/Cargo.tomlpackages/react/transform/crates/swc_plugin_snapshot/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/lib.rspackages/react/transform/crates/swc_plugin_snapshot/napi.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/react/transform/crates/swc_plugin_snapshot/napi.rs (1)
18-25:⚠️ Potential issue | 🟡 MinorPer-field
@internalannotations onElementTemplateAssetstill missing.The struct-level
///@internalat Line 15 isn’t propagated by napi-rs for `#[napi(object)]` types; each field (`template_id`, `compiled_template`, `source_file`) needs its own `/// `@internalcomment for the markers to appear in the generatedindex.d.ts. Same issue applies toTransformNodiffOutput.element_templatesinpackages/react/transform/src/lib.rs. Past review comment on this was not addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/napi.rs` around lines 18 - 25, Add per-field internal doc comments to ensure napi-rs emits `@internal` for each property: update the ElementTemplateAsset struct by adding /// `@internal` above each field (template_id, compiled_template, source_file) so the generated index.d.ts marks them internal; likewise add per-field /// `@internal` for the element_templates field definition on TransformNodiffOutput (referencing TransformNodiffOutput.element_templates) so its type is also emitted as internal in the bindings.packages/react/transform/index.d.ts (1)
658-664:⚠️ Potential issue | 🟡 Minor
elementTemplatesandElementTemplateAssetstill leak into the public type surface without@internal.Line 658
elementTemplates?and theElementTemplateAssetinterface (plus its three fields) are generated without any@internalJSDoc, so they appear in the exported API alongside public types. This was raised against the previous commit and remains unresolved — napi-rs only propagates per-field///@internal`` annotations for#[napi(object)]structs, so the Rust-side needs per-field comments for the regeneration to pick them up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/index.d.ts` around lines 658 - 664, The generated types expose internal-only API; add per-field/internal JSDoc so napi-rs generates `@internal` annotations: mark the property elementTemplates on the containing interface as internal and mark the ElementTemplateAsset type and each of its fields (templateId, compiledTemplate, sourceFile) as internal by adding the corresponding per-field /// `@internal` comments on the Rust #[napi(object)] struct that produces ElementTemplateAsset so the regenerated index.d.ts contains `@internal` on elementTemplates, ElementTemplateAsset, templateId, compiledTemplate, and sourceFile.packages/react/transform/crates/swc_plugin_snapshot/lib.rs (1)
1417-1425:⚠️ Potential issue | 🟠 Major
use_element_templatestill doesn’t verify the collector is present.When a caller constructs
JSXTransformer::new_with_element_templates(..., None)(or usesJSXTransformer::newdirectly from core) withenable_element_template = true,use_element_templateis stilltrueat Line 1417, so the transformer rewrites JSX into<_et_... attributeSlots=... />and skipssnapshot_create_call, but Line 1741 silently drops the template asset becauseself.element_templatesisNone. The emitted module then references a template id that was never exported. This was raised previously and has not been addressed; the NAPI wrapper papers over it by always allocating a collector, but the core API still has the sharp edge.🐛 Proposed fix
- let use_element_template = self.cfg.enable_element_template; + let use_element_template = + self.cfg.enable_element_template && self.element_templates.is_some();Also applies to: 1741-1747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 1417 - 1425, The code sets use_element_template from cfg.enable_element_template without verifying a collector exists, causing emitted template IDs to reference missing exports when self.element_templates is None; update the logic that defines use_element_template (and any later branches that assume element templates, e.g., the path that skips snapshot_create_call and the emitter around snapshot_uid/template asset emission) to require both cfg.enable_element_template && self.element_templates.is_some(), falling back to the snapshot path when the collector is absent, and ensure any place using self.element_templates (like where template assets are pushed/emitted) first checks for Some before referencing or skipping snapshot_create_call.
🤖 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/transform/crates/swc_plugin_snapshot/lib.rs`:
- Around line 548-561: In visit_mut_jsx_element, when the guard jsx_is_list(n)
is true you currently push DynamicPart::Slot; change this to push
DynamicPart::ListSlot so list elements use the list lowering path. Locate the
block that calls n.visit_mut_with(self.dynamic_part_visitor), computes
element_slot_index via next_children_slot_index(), and pushes into
self.dynamic_parts; replace the DynamicPart::Slot variant with
DynamicPart::ListSlot (keeping the same Expr::JSXElement(Box::new(n.take())) and
element_slot_index) so the runtime lowers to
__DynamicPartListChildren/__DynamicPartListSlotV2 like the other branches that
check jsx_is_list.
---
Duplicate comments:
In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs`:
- Around line 1417-1425: The code sets use_element_template from
cfg.enable_element_template without verifying a collector exists, causing
emitted template IDs to reference missing exports when self.element_templates is
None; update the logic that defines use_element_template (and any later branches
that assume element templates, e.g., the path that skips snapshot_create_call
and the emitter around snapshot_uid/template asset emission) to require both
cfg.enable_element_template && self.element_templates.is_some(), falling back to
the snapshot path when the collector is absent, and ensure any place using
self.element_templates (like where template assets are pushed/emitted) first
checks for Some before referencing or skipping snapshot_create_call.
In `@packages/react/transform/crates/swc_plugin_snapshot/napi.rs`:
- Around line 18-25: Add per-field internal doc comments to ensure napi-rs emits
`@internal` for each property: update the ElementTemplateAsset struct by adding
/// `@internal` above each field (template_id, compiled_template, source_file) so
the generated index.d.ts marks them internal; likewise add per-field ///
`@internal` for the element_templates field definition on TransformNodiffOutput
(referencing TransformNodiffOutput.element_templates) so its type is also
emitted as internal in the bindings.
In `@packages/react/transform/index.d.ts`:
- Around line 658-664: The generated types expose internal-only API; add
per-field/internal JSDoc so napi-rs generates `@internal` annotations: mark the
property elementTemplates on the containing interface as internal and mark the
ElementTemplateAsset type and each of its fields (templateId, compiledTemplate,
sourceFile) as internal by adding the corresponding per-field /// `@internal`
comments on the Rust #[napi(object)] struct that produces ElementTemplateAsset
so the regenerated index.d.ts contains `@internal` on elementTemplates,
ElementTemplateAsset, templateId, compiledTemplate, and sourceFile.
🪄 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: ba62a847-bec6-482a-942e-28e379dc0d94
📒 Files selected for processing (7)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_snapshot/lib.rspackages/react/transform/crates/swc_plugin_snapshot/napi.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/test/fixture.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/react/transform/crates/swc_plugin_snapshot/lib.rs (3)
1741-1751: Redundantstrip_prefix/format!round-trip.Inside this branch
snapshot_uid_prefix == "_et"(L1422-1426), sosnapshot_uidalways starts with"_et_";strip_prefix("_et_")always succeeds andformat!("_et_{suffix}")re-prepends the same prefix. Thetemplate_idis identical tosnapshot_uidand the intermediatesuffixis dead.♻️ Simplification
- let suffix = snapshot_uid - .strip_prefix("_et_") - .unwrap_or(snapshot_uid.as_str()); - if let Some(element_templates) = &self.element_templates { element_templates.borrow_mut().push(ElementTemplateAsset { - template_id: format!("_et_{suffix}"), + template_id: snapshot_uid.clone(), compiled_template, source_file: self.cfg.filename.clone(), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 1741 - 1751, The code uses strip_prefix on snapshot_uid to produce suffix and then re-prepends "_et_" to build template_id, but snapshot_uid already begins with "_et_" (see snapshot_uid and the surrounding branch) so the suffix and round-trip are redundant; simplify by removing the strip_prefix call and directly set ElementTemplateAsset.template_id to snapshot_uid (keep compiled_template and source_file as-is and update any references in the element_templates.borrow_mut().push call to use snapshot_uid directly).
1673-1711: Avoid buildingsnapshot_create_call(and unwrappingsnapshot_creator_func) in ET mode.In the
use_element_templatebranch,snapshot_create_callis never appended tocurrent_snapshot_defs(see L1727-1759), so thequote!expansion,snapshot_creator_func.unwrap(), andto_updater(...)calls driven by thedynamic_part_attr.into_iter()loop (L1517-1575) are effectively dead work. Moving this block inside theelsebranch at L1752 would remove the unwrap-based coupling to DynamicPartExtractor's internal invariants on the ET path and cut meaningful work per snapshot. Not a correctness bug, but worth tightening while the ET contract is still fresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 1673 - 1711, The code constructs snapshot_creator (and calls snapshot_creator_func.unwrap()) and always builds snapshot_create_call even when use_element_template path never appends it; move the entire snapshot_creator/snapshot_create_call construction into the branch where snapshot_create_call is actually pushed (the else branch for the use_element_template check) so you only build snapshot_creator (and call snapshot_creator_func.unwrap()) when the snapshot is going to be emitted; this avoids calling DynamicPartExtractor-driven to_updater/dynamic_part_attr.into_iter() work and removes the unwrap coupling on the ET path.
641-686: Add clarifying comment explaining ET-mode list-item attribute handling.Gating list-item platform info bundling behind
!self.enable_element_templatecreates two distinct paths:
- Snapshot path (ET disabled): Extracts
item-key,reuse-identifier,full-span,sticky-top/bottom,estimated-*, andrecyclableand bundles them viaupdateListItemPlatformInfo()updater for recycling semantics.- ET path (ET enabled): Leaves these as regular HTML attributes in the template; the list runtime reads them via
getAttribute()(e.g.,XList.tslines 240, 259, 298).The test
should_not_consume_hidden_et_slots_for_list_item_platform_attrsconfirms this contract: in ET mode, these attributes appear as regular descriptors inattributesArray(dynamic slots for values likeitem-key={itemKey}, static for literals likerecyclable), not as a bundled platform-info updater call.A comment near this guard explaining why list virtualization works in both paths would help future readers understand this intentional divergence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 641 - 686, Add a concise comment above the guard that checks jsx_is_list_item(n) && !self.enable_element_template explaining the two intentional codepaths: when enable_element_template is false the code consumes list-item platform attributes (e.g., "item-key", "reuse-identifier", "full-span", "sticky-top", "sticky-bottom", "estimated-*", "recyclable") and bundles them into an object pushed via push_dynamic_attr(..., AttrName::ListItemPlatformInfo) (used by updateListItemPlatformInfo for recycling semantics), whereas when enable_element_template is true those same attributes are left on the template as normal attributes so the runtime (e.g., XList.ts getAttribute callers) can read them from attributesArray; mention the related test should_not_consume_hidden_et_slots_for_list_item_platform_attrs to make the contract explicit.packages/react/transform/crates/swc_plugin_snapshot/element_template.rs (2)
379-451: Unusedinject_root_metadataparameter — remove or wire it up.
inject_root_metadatais threaded throughelement_template_from_jsx_element→_impland then discarded at L451 withlet _ = inject_root_metadata;. The TODO at L1738-1739 inlib.rs("reintroduce cssId/entryName metadata once the runtime/native contract grows a dedicated replacement channel") confirms this is a placeholder. Either drop the parameter and reintroduce it when the channel lands, or at minimum convertlet _ = …;into a// TODO(element-template):comment that explains what will consume it, so the intent is not lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/element_template.rs` around lines 379 - 451, The parameter inject_root_metadata is unused in element_template_from_jsx_element_impl (and is threaded from element_template_from_jsx_element) — either remove it from the signature and update callers (element_template_from_jsx_element and any other callers) to stop passing it, or keep the parameter and replace the no-op "let _ = inject_root_metadata;" with a clear TODO comment that names the intended consumer (e.g. "// TODO(element-template): preserve inject_root_metadata for future cssId/entryName metadata injection in runtime/native contract") so intent isn't lost; adjust all references to the symbol element_template_from_jsx_element_impl and the caller element_template_from_jsx_element accordingly.
55-78: Nit: redundant slot-ident name check.
slot_identis constructed viaprivate_ident!("__etSlot")inJSXTransformer::new_with_element_templates(lib.rs L1315), soslot_ident.symis always"__etSlot". Theident.sym != slot_ident.sym && ident.sym.as_ref() != "__etSlot"guard is thereforea != x && a != x— the second conjunct is dead. Keeping just theslot_ident.symcomparison avoids the impression that__etSlotis a separately-matched free identifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/element_template.rs` around lines 55 - 78, In unwrap_et_slot_expr, remove the redundant second check against the literal "__etSlot" and only compare the callee identifier to the provided slot_ident; replace the current condition `if ident.sym != slot_ident.sym && ident.sym.as_ref() != "__etSlot"` with a single check against slot_ident (e.g., `if ident.sym != slot_ident.sym { return None; }`) so the function consistently relies on the passed-in slot_ident (keep function name unwrap_et_slot_expr and the existing early-return pattern).packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs (1)
250-253: Optional — loosen formatting-sensitive code assertions.
code.contains("attributeSlots={[\n cls\n]}")couples these contract tests to the SWC codegen indentation/line-break style. A futureswc_corebump that changes array pretty-printing will break these assertions without any real behavior change. Consider matching on normalized (whitespace-collapsed) code or on an AST-level shape instead. Same pattern at L308.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs` around lines 250 - 253, The test assertion in element_template_contract.rs is brittle because it checks for an exact pretty-printed snippet via code.contains("attributeSlots={[\n cls\n]}"); update the two assertions (the one shown and the similar at L308) to compare in a whitespace-agnostic way — e.g., normalize the generated code by collapsing runs of whitespace (or use a regex that treats \s+ as a single space) and then assert the normalized string contains the normalized attributeSlots pattern "attributeSlots={[ cls ]}" (or equivalent normalized form). This keeps the assertion tied to structure, not specific SWC pretty-print formatting.
🤖 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/transform/crates/swc_plugin_snapshot/element_template.rs`:
- Around line 167-215: The function element_template_to_json should avoid
panicking on non-finite numeric literals in Expr::Lit(Lit::Num) — replace the
direct serde_json::Number::from_f64(n.value).unwrap() call in
element_template_to_json with a check like if n.value.is_finite() then create a
Number from_f64 and use it (handling the Option), otherwise return
serde_json::Value::Null (or a string) so NaN/±Infinity from numeric literals do
not crash the transform; update the Expr::Lit(Lit::Num(n)) arm to perform this
check and early-return the safe Null/string fallback.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_snapshot/element_template.rs`:
- Around line 379-451: The parameter inject_root_metadata is unused in
element_template_from_jsx_element_impl (and is threaded from
element_template_from_jsx_element) — either remove it from the signature and
update callers (element_template_from_jsx_element and any other callers) to stop
passing it, or keep the parameter and replace the no-op "let _ =
inject_root_metadata;" with a clear TODO comment that names the intended
consumer (e.g. "// TODO(element-template): preserve inject_root_metadata for
future cssId/entryName metadata injection in runtime/native contract") so intent
isn't lost; adjust all references to the symbol
element_template_from_jsx_element_impl and the caller
element_template_from_jsx_element accordingly.
- Around line 55-78: In unwrap_et_slot_expr, remove the redundant second check
against the literal "__etSlot" and only compare the callee identifier to the
provided slot_ident; replace the current condition `if ident.sym !=
slot_ident.sym && ident.sym.as_ref() != "__etSlot"` with a single check against
slot_ident (e.g., `if ident.sym != slot_ident.sym { return None; }`) so the
function consistently relies on the passed-in slot_ident (keep function name
unwrap_et_slot_expr and the existing early-return pattern).
In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs`:
- Around line 1741-1751: The code uses strip_prefix on snapshot_uid to produce
suffix and then re-prepends "_et_" to build template_id, but snapshot_uid
already begins with "_et_" (see snapshot_uid and the surrounding branch) so the
suffix and round-trip are redundant; simplify by removing the strip_prefix call
and directly set ElementTemplateAsset.template_id to snapshot_uid (keep
compiled_template and source_file as-is and update any references in the
element_templates.borrow_mut().push call to use snapshot_uid directly).
- Around line 1673-1711: The code constructs snapshot_creator (and calls
snapshot_creator_func.unwrap()) and always builds snapshot_create_call even when
use_element_template path never appends it; move the entire
snapshot_creator/snapshot_create_call construction into the branch where
snapshot_create_call is actually pushed (the else branch for the
use_element_template check) so you only build snapshot_creator (and call
snapshot_creator_func.unwrap()) when the snapshot is going to be emitted; this
avoids calling DynamicPartExtractor-driven
to_updater/dynamic_part_attr.into_iter() work and removes the unwrap coupling on
the ET path.
- Around line 641-686: Add a concise comment above the guard that checks
jsx_is_list_item(n) && !self.enable_element_template explaining the two
intentional codepaths: when enable_element_template is false the code consumes
list-item platform attributes (e.g., "item-key", "reuse-identifier",
"full-span", "sticky-top", "sticky-bottom", "estimated-*", "recyclable") and
bundles them into an object pushed via push_dynamic_attr(...,
AttrName::ListItemPlatformInfo) (used by updateListItemPlatformInfo for
recycling semantics), whereas when enable_element_template is true those same
attributes are left on the template as normal attributes so the runtime (e.g.,
XList.ts getAttribute callers) can read them from attributesArray; mention the
related test should_not_consume_hidden_et_slots_for_list_item_platform_attrs to
make the contract explicit.
In
`@packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs`:
- Around line 250-253: The test assertion in element_template_contract.rs is
brittle because it checks for an exact pretty-printed snippet via
code.contains("attributeSlots={[\n cls\n]}"); update the two assertions (the
one shown and the similar at L308) to compare in a whitespace-agnostic way —
e.g., normalize the generated code by collapsing runs of whitespace (or use a
regex that treats \s+ as a single space) and then assert the normalized string
contains the normalized attributeSlots pattern "attributeSlots={[ cls ]}" (or
equivalent normalized form). This keeps the assertion tied to structure, not
specific SWC pretty-print formatting.
🪄 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: b799e283-4b56-437d-a2fe-cbd2e8cf4d2a
📒 Files selected for processing (8)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_snapshot/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/lib.rspackages/react/transform/crates/swc_plugin_snapshot/napi.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/transform/test/fixture.spec.js
- packages/react/transform/index.d.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/transform/crates/swc_plugin_snapshot/lib.rs (2)
1816-1822: Nit: semicolon inconsistency between ET/legacyattributeSlots/valuespush branches.The ET branch at Line 1808 terminates with
}));while the legacy branch at Line 1820 ends with}))(no trailing;). Both still compile because the outerif/elseresolves to()and the};on Line 1822 terminates the statement, but the asymmetry is noise. Align the two arms.✂️ Proposed fix
}))), })), - })) + })); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 1816 - 1822, There is a semicolon inconsistency between the ET branch and the legacy branch in the attributeSlots/values push code paths: the ET arm ends with `}));` while the legacy arm ends with `}))`; make them consistent by adding the missing trailing semicolon to the legacy branch that constructs/pushes `snapshot_values` (the same expression that currently ends with `}))`) so both arms terminate the expression the same way.
1676-1758: Avoid buildingsnapshot_create_call(and unwrappingsnapshot_creator_func) when ET is active.For non-JS targets the code unconditionally constructs
Expr::Fn { function: Box::new(snapshot_creator_func.unwrap()), .. }at Line 1681 and the fullsnapshot_create_callquote at Lines 1685-1714, but in ET mode that expression is never emitted — only theelsebranch at Lines 1751-1757 pushes it intocurrent_snapshot_defs. Two smells:
- Wasted AST construction on the ET hot path.
- The
.unwrap()at Line 1681 is a latent panic vector the moment any future change lets ET reach here withsnapshot_creator == None(e.g. the early-return ET-list branch at Line 549 skips the snapshot-creator accumulation); guarding it on!use_element_templatemakes the invariant explicit.Consider building
snapshot_creator/snapshot_create_callonly in the non-ET branch that actually consumes them.♻️ Sketch
- let snapshot_creator = if target == TransformTarget::JS { - Expr::Lit(Lit::Null(Null { span: DUMMY_SP })) - } else { - Expr::Fn(FnExpr { - ident: None, - function: Box::new(snapshot_creator_func.unwrap()), - }) - }; - - let snapshot_create_call = quote!( ... ); - ... if use_element_template { ... } else { + let snapshot_creator = if target == TransformTarget::JS { + Expr::Lit(Lit::Null(Null { span: DUMMY_SP })) + } else { + Expr::Fn(FnExpr { + ident: None, + function: Box::new( + snapshot_creator_func + .expect("snapshot_creator must be produced for non-ET non-JS targets"), + ), + }) + }; + let snapshot_create_call = quote!( ... ); let snapshot_def = ModuleItem::Stmt(quote!( r#"$snapshot_create_call"# as Stmt, snapshot_create_call: Expr = snapshot_create_call, )); self.current_snapshot_defs.push(snapshot_def); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_snapshot/lib.rs` around lines 1676 - 1758, The code eagerly builds snapshot_creator (using snapshot_creator_func.unwrap()) and snapshot_create_call even when use_element_template is true and those values are never used; move the construction of snapshot_creator and snapshot_create_call into the non-ET branch (the else that pushes snapshot_def into self.current_snapshot_defs) so they are only created when needed, and replace the unwrap by using snapshot_creator_func (or pattern-match Option) inside that branch to avoid a latent panic; update references to snapshot_creator, snapshot_creator_func, snapshot_create_call, use_element_template, and self.current_snapshot_defs accordingly.
🤖 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/transform/crates/swc_plugin_snapshot/lib.rs`:
- Around line 1816-1822: There is a semicolon inconsistency between the ET
branch and the legacy branch in the attributeSlots/values push code paths: the
ET arm ends with `}));` while the legacy arm ends with `}))`; make them
consistent by adding the missing trailing semicolon to the legacy branch that
constructs/pushes `snapshot_values` (the same expression that currently ends
with `}))`) so both arms terminate the expression the same way.
- Around line 1676-1758: The code eagerly builds snapshot_creator (using
snapshot_creator_func.unwrap()) and snapshot_create_call even when
use_element_template is true and those values are never used; move the
construction of snapshot_creator and snapshot_create_call into the non-ET branch
(the else that pushes snapshot_def into self.current_snapshot_defs) so they are
only created when needed, and replace the unwrap by using snapshot_creator_func
(or pattern-match Option) inside that branch to avoid a latent panic; update
references to snapshot_creator, snapshot_creator_func, snapshot_create_call,
use_element_template, and self.current_snapshot_defs accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 627019af-ca9d-4eed-ad23-b45f577cf7fc
📒 Files selected for processing (3)
packages/react/transform/crates/swc_plugin_snapshot/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/lib.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/transform/crates/swc_plugin_snapshot/element_template.rs
- packages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rs
Add the element template transform, generated template metadata surface, and coverage for static and dynamic template slots. Fold the follow-up flag rename, review fixes, and non-finite number handling into the initial implementation.
Yradex
force-pushed
the
wt/element-template
branch
from
666c7e0 to
8355f1e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/transform/crates/swc_plugin_snapshot/element_template.rs`:
- Around line 407-439: The code currently treats literal/string timing flags as
static which desynchronizes attributeSlots because DynamicPartExtractor always
emits a runtime slot for "__lynx_timing_flag"; update the logic that checks
static_value so that if the attribute key matches the timing flag identifier
(e.g., "__lynx_timing_flag") you bypass adding a static descriptor and instead
increment attr_slot_index and call
element_template_attribute_slot_descriptor(&key, idx) — keep use of
attribute_descriptors, attr_slot_index,
element_template_static_attribute_descriptor,
element_template_attribute_slot_descriptor and ensure the timing-flag path
always allocates a slot even when static_value is Some.
🪄 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: 54b31586-16a6-4e17-9c04-3891afdf2872
⛔ Files ignored due to path filters (27)
Cargo.lockis excluded by!**/*.lockpackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_generate_attribute_slots_for_dynamic_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_background_conditional_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_boolean_and_number_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_complex_text_structure.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_dataset_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_dynamic_class_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_events_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_events_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_id_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_inline_styles.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_mixed_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_nested_structure_and_dynamic_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_page_element.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_refs_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_refs_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_sibling_user_components.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_spread_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_handle_user_component.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_isolate_arrays_with_slot_wrapper.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_keep_code_and_template_attribute_slots_in_sync_for_spread.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_output_element_template_simple_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_output_template_with_static_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_verify_template_structure_complex.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_snapshot/tests/__combined_snapshots__/should_verify_text_attribute_and_child_text_slots.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/react/transform/Cargo.tomlpackages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_snapshot/Cargo.tomlpackages/react/transform/crates/swc_plugin_snapshot/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/lib.rspackages/react/transform/crates/swc_plugin_snapshot/napi.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template.rspackages/react/transform/crates/swc_plugin_snapshot/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/transform/crates/swc_plugin_snapshot/Cargo.toml
- packages/react/transform/index.d.ts
Comment on lines
+407
to
+439
| let static_value = match &attr.value { | ||
| None => Some(Expr::Lit(Lit::Bool(Bool { | ||
| span: DUMMY_SP, | ||
| value: true, | ||
| }))), | ||
| Some(JSXAttrValue::Str(s)) => Some(Expr::Lit(Lit::Str(Str { | ||
| span: s.span, | ||
| value: transform_jsx_attr_str(&s.value).into(), | ||
| raw: None, | ||
| }))), | ||
| Some(JSXAttrValue::JSXExprContainer(JSXExprContainer { | ||
| expr: JSXExpr::Expr(expr), | ||
| .. | ||
| })) => match &**expr { | ||
| Expr::Lit(Lit::Str(s)) => Some(Expr::Lit(Lit::Str(s.clone()))), | ||
| Expr::Lit(Lit::Num(n)) => Some(Expr::Lit(Lit::Num(n.clone()))), | ||
| Expr::Lit(Lit::Bool(b)) => Some(Expr::Lit(Lit::Bool(*b))), | ||
| Expr::Lit(Lit::Null(n)) => Some(Expr::Lit(Lit::Null(*n))), | ||
| // TODO: Support complex static values (Object, Array, Template Literal without expressions) | ||
| // See ElementTemplate/Todo-StaticAttributesOpts.md | ||
| _ => None, | ||
| }, | ||
| _ => None, | ||
| }; | ||
|
|
||
| if let Some(static_value) = static_value { | ||
| attribute_descriptors | ||
| .push(self.element_template_static_attribute_descriptor(&key, static_value)); | ||
| } else { | ||
| let idx = *attr_slot_index; | ||
| *attr_slot_index += 1; | ||
| attribute_descriptors.push(self.element_template_attribute_slot_descriptor(&key, idx)); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep timing-flag descriptors aligned with emitted attributeSlots.
DynamicPartExtractor always emits a runtime slot for __lynx_timing_flag, but this path marks literal/string timing flags as static. That leaves an extra attributeSlots value with no matching attrSlotIndex in the compiled template.
🐛 Proposed fix
if key == "__lynx_part_id" {
continue;
}
+ if key == "__lynx_timing_flag" {
+ let idx = *attr_slot_index;
+ *attr_slot_index += 1;
+ attribute_descriptors.push(self.element_template_attribute_slot_descriptor(&key, idx));
+ continue;
+ }
+
let static_value = match &attr.value {
None => Some(Expr::Lit(Lit::Bool(Bool {
span: DUMMY_SP,
value: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/transform/crates/swc_plugin_snapshot/element_template.rs`
around lines 407 - 439, The code currently treats literal/string timing flags as
static which desynchronizes attributeSlots because DynamicPartExtractor always
emits a runtime slot for "__lynx_timing_flag"; update the logic that checks
static_value so that if the attribute key matches the timing flag identifier
(e.g., "__lynx_timing_flag") you bypass adding a static descriptor and instead
increment attr_slot_index and call
element_template_attribute_slot_descriptor(&key, idx) — keep use of
attribute_descriptors, attr_slot_index,
element_template_static_attribute_descriptor,
element_template_attribute_slot_descriptor and ensure the timing-flag path
always allocates a slot even when static_value is Some.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
Overview
experimentalEnableElementTemplateis enabled, the transform still emits ReactLynx JSX, but also returns Template Definition assets throughelementTemplatesso build tooling can hand native/runtime a structured template description.snapshotCreatorMap, and existing Snapshot runtime behavior are preserved.attributeSlotsas theirattrSlotIndexdescriptors in the template JSON.Generated Output Contract
With the experimental flag enabled, transform output may include:
Each user template is emitted with an
_et_...template id. Raw text support is represented by a shared__et_builtin_raw_text__template asset when needed. The transformed JSX references the user template id as a component-like tag and passes dynamic runtime values through explicit slots, for example:The corresponding
compiledTemplateuses a Template Definition JSON shape, with static structure in the asset and dynamic values referenced by slot indices:{ "kind": "element", "type": "view", "attributesArray": [ { "kind": "attribute", "key": "id", "binding": "slot", "attrSlotIndex": 0 }, { "kind": "spread", "binding": "slot", "attrSlotIndex": 1 }, { "kind": "attribute", "key": "bindtap", "binding": "slot", "attrSlotIndex": 2 } ], "children": [ { "kind": "elementSlot", "type": "slot", "elementSlotIndex": 0 } ] }Key Constraints
attributeSlots[n]is the value for every template attribute or spread descriptor whoseattrSlotIndexisn. A JSX spread is therefore one slot at its original JSX position, not a merged props object that replaces the surrounding dynamic attrs.elementSlotIndexand are kept separate fromattributeSlots.__elementTemplateMap; template assets are exported through the transform result side channel instead.__etSlothelper import behavior unchanged. Resolving that runtime helper/export contract is a separate follow-up from the Template Definition asset format and spread slot alignment covered here.Key Points
transformReactLynxSync/transformReactLynxcan returnelementTemplatesonly when the experimental ET path is requested._et_prefix to distinguish ET assets from Snapshot creator functions.Checklist
@lynx-js/react-transformis private and the changed path is experimental/internal).