feat(transform): add element template transform#2517
Open
Yradex wants to merge 3 commits intolynx-family:mainfrom
Open
feat(transform): add element template transform#2517Yradex wants to merge 3 commits intolynx-family:mainfrom
Yradex wants to merge 3 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 82183f1 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 |
Contributor
📝 WalkthroughWalkthroughAdds a new swc_plugin_element_template crate (core, napi bridge, and tests), integrates an elementTemplate option into the React transform pipeline and typings, updates Cargo dependencies/features, adds JS tests validating elementTemplate behavior, and inserts an empty changeset marker. 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! |
Merging this PR will not alter performance
Comparing 
Footnotes
|
React External#748 Bundle Size — 680.2KiB (0%).82183f1(current) vs 989f345 main#736(baseline) Bundle metrics
|
| Current #748 |
Baseline #736 |
|
|---|---|---|
 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
Web Explorer#9204 Bundle Size — 900.02KiB (0%).82183f1(current) vs 989f345 main#9192(baseline) Bundle metrics
|
| Current #9204 |
Baseline #9192 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
 Modules |
230(+1.32%) |
227 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28%(-0.04%) |
27.29% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9204 |
Baseline #9192 |
|
|---|---|---|
| JS |
495.88KiB |
495.88KiB |
| 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 Example#7631 Bundle Size — 225.31KiB (0%).82183f1(current) vs 989f345 main#7619(baseline) Bundle metrics
|
| Current #7631 |
Baseline #7619 |
|
|---|---|---|
| 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 #7631 |
Baseline #7619 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
79.55KiB |
79.55KiB |
Bundle analysis report Branch Yradex:wt/element-template Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#763 Bundle Size — 196.47KiB (0%).82183f1(current) vs 989f345 main#751(baseline) Bundle metrics
|
| Current #763 |
Baseline #751 |
|
|---|---|---|
| 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 #763 |
Baseline #751 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
85.24KiB |
85.24KiB |
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
6be8863 to
67dbda2
Compare
Yradex
changed the title
Yradex
force-pushed
the
wt/element-template
branch
3 times, most recently
from
82add7a to
cf1d885
Compare
Yradex
force-pushed
the
wt/element-template
branch
from
cf1d885 to
0b5a0cb
Compare
![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: 11
🧹 Nitpick comments (15)
packages/react/transform/swc-plugin-reactlynx/index.d.ts (1)
30-46: Avoid duplicatingJsxTransformerConfig; alias or extend it.
ElementTemplateConfigis field-for-field identical toJsxTransformerConfig(lines 13-28). Any future option added to one backend will silently drift from the other. A type alias keeps the two in sync and preserves the@internaltag / separate doc identity at the call-site type.Proposed refactor
-/** `@internal` */ -export interface ElementTemplateConfig { - /** `@internal` */ - preserveJsx: boolean; - /** `@internal` */ - runtimePkg: string; - /** `@internal` */ - jsxImportSource?: string; - /** `@internal` */ - filename: string; - /** `@internal` */ - target: 'LEPUS' | 'JS' | 'MIXED'; - /** `@internal` */ - enableUiSourceMap?: boolean; - /** `@internal` */ - isDynamicComponent?: boolean; -} +/** `@internal` */ +export type ElementTemplateConfig = JsxTransformerConfig;If the shapes are expected to diverge soon (ET-only options planned), prefer
extends:/** `@internal` */ export interface ElementTemplateConfig extends JsxTransformerConfig {}As per coding guidelines: "Use TypeScript in strict mode as configured in tsconfig.json for all TypeScript source files" — an alias/extension keeps both interfaces strictly in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/swc-plugin-reactlynx/index.d.ts` around lines 30 - 46, ElementTemplateConfig duplicates JsxTransformerConfig; replace the duplicate interface with a single type alias or an extends declaration so the shapes stay in sync (e.g., make ElementTemplateConfig a type alias to JsxTransformerConfig or declare "export interface ElementTemplateConfig extends JsxTransformerConfig {}"), preserving the /** `@internal` */ comment and exported name so call-sites and docs remain unchanged; update any imports/usages if necessary to reference the unified ElementTemplateConfig symbol.packages/react/transform/crates/swc_plugin_element_template/slot.rs (2)
53-56: Numeric slot id is accepted even when non-integer.
*value as usizesilently truncates fractional or very largef64literals (e.g.,1.5 → 1,NaN → 0,>= u32::MAX). This should never happen given slot indices are produced by the compiler, but a stricter guard would make the parser fail loudly if a future pass ever emits a non-integer.Proposed tighter guard
- let slot_id = match &*call_expr.args[0].expr { - Expr::Lit(Lit::Num(Number { value, .. })) if *value >= 0.0 => *value as usize, - _ => return None, - }; + let slot_id = match &*call_expr.args[0].expr { + Expr::Lit(Lit::Num(Number { value, .. })) + if *value >= 0.0 && value.fract() == 0.0 && *value <= usize::MAX as f64 => + { + *value as usize + } + _ => return None, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/slot.rs` around lines 53 - 56, The current match accepts any non-negative f64 and uses *value as usize which silently truncates non-integers and mishandles NaN/inf; update the guard on the Expr::Lit(Lit::Num(Number { value, .. })) arm to only accept finite integral values within usize range (e.g., check value.is_finite(), value.fract() == 0.0, value >= 0.0, and value <= (usize::MAX as f64)) and only then convert to usize (slot_id) — otherwise return None; adjust the match arm that produces slot_id accordingly to use these stricter checks instead of the current *value as usize conversion.
36-48: Useto_id()comparison instead of sym-only matching for hygiene safety.At line 46, comparing only
ident.sym != slot_ident.symignoresSyntaxContext. While the__etSlotmarker is created viaprivate_ident!()with a private context, sym-only comparison could theoretically match user code if a variable with the same name exists. The codebase consistently usesto_id()for identifier matching elsewhere (seeswc_plugin_refresh,swc_plugin_worklet, etc.). Change to:if ident.to_id() != slot_ident.to_id() { return None; }This ensures proper hygiene handling and aligns with established patterns in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/slot.rs` around lines 36 - 48, In unwrap_et_slot_expr, replace the identifier equality check that compares only ident.sym with a hygiene-safe comparison using to_id(): ensure you call ident.to_id() and slot_ident.to_id() and return None when they differ; this mirrors usages elsewhere (e.g., swc_plugin_refresh) and ensures the private_ident!()-created __etSlot marker is compared including SyntaxContext rather than just the symbol.packages/react/transform/src/lib.rs (2)
500-519: Backend selection logic looks correct; just a small readability nit.
use_snapshot_plugin = snapshot_enabled && !use_element_template_plugincorrectly ensures at most one backend runs, and theactive_jsx_import_source/preserve_jsx/enable_ui_source_mapfan-outs all branch on the sameuse_element_template_pluginflag. Consistent.Optional: the three
if use_element_template_plugin { et.X } else { snap.X }blocks could be condensed by selecting a single&configreference up front, reducing the chance of future drift where one gets updated and others don't.🤖 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 500 - 519, The repeated conditional fan-out can be simplified: create a single reference like selected_config = if use_element_template_plugin { &element_template_plugin_config } else { &snapshot_plugin_config } and then derive jsx_backend_enabled, active_jsx_import_source, preserve_jsx, and enable_ui_source_map from that selected_config (e.g., selected_config.jsx_import_source.clone(), selected_config.preserve_jsx, selected_config.enable_ui_source_map.unwrap_or(false)), keeping the existing use_element_template_plugin, use_snapshot_plugin, and jsx_backend_enabled logic intact to avoid behavioral changes.
302-338: Two near-duplicateclone_*_ui_source_map_recordshelpers — collapse.The two functions differ only in the input record type and which of
snapshot_id/template_idis set. They can be unified via a small trait or a closure mapping the backend-specific record into the common shape, keeping the call sites at lines 822-829 / 839-846 readable.Proposed refactor sketch
trait IntoUiSourceMapRecord { fn into_ui_record(self, filename: &str) -> UiSourceMapRecord; } impl IntoUiSourceMapRecord for SnapshotCoreUISourceMapRecord { fn into_ui_record(self, filename: &str) -> UiSourceMapRecord { UiSourceMapRecord { ui_source_map: self.ui_source_map, filename: filename.to_string(), line_number: self.line_number, column_number: self.column_number, snapshot_id: Some(self.snapshot_id), template_id: None, } } } impl IntoUiSourceMapRecord for ElementTemplateCoreUISourceMapRecord { /* ...template_id version... */ } fn clone_ui_source_map_records<R: IntoUiSourceMapRecord + Clone>( records: &Rc<RefCell<Vec<R>>>, filename: &str, ) -> Vec<UiSourceMapRecord> { records.borrow().iter().cloned().map(|r| r.into_ui_record(filename)).collect() }🤖 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 302 - 338, Two nearly identical helpers clone_snapshot_ui_source_map_records and clone_element_template_ui_source_map_records should be collapsed into one generic function: define a trait (e.g. IntoUiSourceMapRecord) with a method into_ui_record(&self, filename: &str) -> UiSourceMapRecord, implement it for SnapshotCoreUISourceMapRecord and ElementTemplateCoreUISourceMapRecord (setting snapshot_id or template_id appropriately), then replace both helper functions with a single fn clone_ui_source_map_records<R: IntoUiSourceMapRecord + Clone>(records: &Rc<RefCell<Vec<R>>>, filename: &str) -> Vec<UiSourceMapRecord> that borrows, iterates, clones and maps via r.into_ui_record(filename); update call sites that used clone_snapshot_ui_source_map_records and clone_element_template_ui_source_map_records to call clone_ui_source_map_records instead.packages/react/transform/crates/swc_plugin_element_template/attr_name.rs (1)
71-83: Hoist the regex to avoid per-call recompilation.
get_event_type_and_nameis called fromFrom<String>/From<Str>/From<Ident>andfrom_nsfor every JSX attribute and re-compiles the same regex each time. The crate already depends ononce_cell; lifting it to aLazywould both amortize compilation and make the.unwrap()cost a one-time assertion.Proposed refactor
-use regex::Regex; - -use swc_core::ecma::ast::*; +use once_cell::sync::Lazy; +use regex::Regex; +use swc_core::ecma::ast::*; + +static EVENT_KEY_RE: Lazy<Regex> = Lazy::new(|| { + Regex::new(r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$").unwrap() +}); @@ fn get_event_type_and_name(props_key: &str) -> Option<(String, String)> { - let re = Regex::new(r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$").unwrap(); - if let Some(captures) = re.captures(props_key) { + if let Some(captures) = EVENT_KEY_RE.captures(props_key) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs` around lines 71 - 83, The function get_event_type_and_name currently recompiles the regex on every call; hoist the Regex into a static Lazy to compile once: add a static RE: Lazy<Regex> (using once_cell::sync::Lazy) initialized with Regex::new(r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$").unwrap(), then update get_event_type_and_name to use RE.captures(props_key) instead of Regex::new(...).unwrap(); keep the same captures.get(...) logic and return behavior so callers of get_event_type_and_name (and conversions From<String>/From<Str>/From<Ident>/from_ns) benefit from one-time compilation.packages/react/transform/crates/swc_plugin_element_template/lowering.rs (2)
94-131: Single-slot fast path duplicates the general-iteration arm.The
(1, Some(DynamicElementPart::Slot(expr, 0)))branch and the general iterator branch produce the same lowering — the only difference is that the fast path hardcodes0, but in that armelement_indexis already 0, so dropping the fast path would yield identical output with less code. This is not a bug; consolidating simplifies maintenance and keeps theself.used_slottoggle logic in a single place.♻️ Possible simplification
- match (dynamic_children.len(), dynamic_children.first()) { - (0, _) => {} - (1, Some(DynamicElementPart::Slot(expr, 0))) => { - let child = expr_to_jsx_child(expr.clone()); - if target != TransformTarget::LEPUS { - self.used_slot = true; - } - rendered_slot_children.push(wrap_in_slot(&self.slot_ident, 0, vec![child])); - } - _ => { - for dynamic_child in dynamic_children { - match dynamic_child { - DynamicElementPart::ListSlot(expr, element_index) - | DynamicElementPart::Slot(expr, element_index) => { - let child = expr_to_jsx_child(expr); - if target != TransformTarget::LEPUS { - self.used_slot = true; - } - rendered_slot_children.push(wrap_in_slot( - &self.slot_ident, - element_index, - vec![child], - )); - } - } - } - } - } + for dynamic_child in dynamic_children { + let (expr, element_index) = match dynamic_child { + DynamicElementPart::ListSlot(e, i) | DynamicElementPart::Slot(e, i) => (e, i), + }; + let child = expr_to_jsx_child(expr); + if target != TransformTarget::LEPUS { + self.used_slot = true; + } + rendered_slot_children.push(wrap_in_slot(&self.slot_ident, element_index, vec![child])); + }Note: if
ListSlotandSlotdiverge in lowering later (e.g., materialization work referenced by the PR as deferred), the or-pattern would need to split again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lowering.rs` around lines 94 - 131, The special-case branch handling (1, Some(DynamicElementPart::Slot(expr, 0))) is redundant because it performs the same lowering as the general iteration; remove that fast-path arm and let the fallback loop handle all DynamicElementPart variants so logic like setting self.used_slot and pushing wrap_in_slot(&self.slot_ident, element_index, vec![child]) lives only in the iterator branch. Keep using expr_to_jsx_child(expr) and preserve the TransformTarget::LEPUS check around setting self.used_slot; ensure both DynamicElementPart::Slot and DynamicElementPart::ListSlot are handled in the single for dynamic_child loop and that element_index remains 0 for the former case.
147-163: Keep the LEPUS lowering invariant documented.The
.expect("LEPUS ET children should already be lowered to slot arrays")at line 150 relies on the guarantee that everything inrendered_slot_childrencame fromwrap_in_slot(...)solower_lepus_et_children_exprwill always match. That invariant is currently only enforced by the surrounding code — if a future change ever pushes a non-slot-wrapped child intorendered_slot_childrenfor LEPUS, users will see a panic rather than a diagnostic. A one-line comment above theexpectpointing atwrap_in_slot/lower_lepus_et_children_expras the paired invariant would make this a lot easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lowering.rs` around lines 147 - 163, Add a one-line comment above the expect in the LEPUS branch documenting the invariant: state that every element of rendered_slot_children must originate from wrap_in_slot(...) so lower_lepus_et_children_expr(...) will always succeed; reference TransformTarget::LEPUS, rendered_slot_children, wrap_in_slot, and lower_lepus_et_children_expr in the comment to make the pairing explicit and help future maintainers avoid introducing non-slot children that would cause the expect to panic.packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs (1)
22-67: Consider installing a HANDLER to avoid latent panics on test-input expansion.
transform_fixturecallsmodule.visit_mut_with(&mut transformer)withoutHANDLER.set(...). All current inputs in this file are valid, so the transformer never hits a diagnostic path, but if the contract suite is later extended to exercise any invalid inputs (e.g., unsupported elements like the<page />case in the sibling test file), anyHANDLER.with(...)call in the transformer will panic.element_template.rsalready wires aDiagnosticCollector— factoring that helper and reusing it here (even if diagnostics are ignored) would remove a footgun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs` around lines 22 - 67, The test harness currently calls module.visit_mut_with(&mut transformer) without installing a SWC diagnostics HANDLER, which can cause panics if the transformer emits diagnostics; wrap the transformation in a HANDLER by creating a DiagnosticCollector (as used in element_template.rs) and calling HANDLER.set(&collector, || { module.visit_mut_with(&mut transformer); }) (or similarly install the collector for the closure that performs the emit), so diagnostics are captured rather than panicking; reference the existing DiagnosticCollector construction from element_template.rs and the symbols MODULE.visit_mut_with, JSXTransformer, and HANDLER.set to locate where to insert this.packages/react/transform/crates/swc_plugin_element_template/lib.rs (2)
376-381:validate_directivesis invoked for the module span and again for every top-level item.
self.comments.with_leading(span.lo, …)runs once pern.bodyitem plus once forn.span, which for large files means repeatedly scanning and parsing the same leading comments (most statements don't have@jsx*pragmas). Typically JSX pragma validation happens once at module scope. If the per-item pass is intentional (e.g., to catch directives attached to exports), a short comment explaining why would help; otherwise the loop can be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 376 - 381, The code calls validate_directives for the module span and then again for every top-level item, causing self.comments.with_leading(span.lo, …) to re-scan leading comments repeatedly; either remove the per-item loop in visit_mut_module so validate_directives is only called once for n.span, or if the per-item validation is required (e.g., to catch directives attached to exported statements), add a short comment above the loop explaining that intent and why repeated scans are necessary; update references to visit_mut_module, validate_directives, n.span, n.body, and self.comments.with_leading accordingly.
385-405: Lazy initialization side-effect is load-bearing.The
Lazy::<Expr>::get(&self.runtime_id)check here isSomeonly if theLazyhas been initialized, which happens on Line 244 viaself.runtime_id.clone()(auto-deref throughLazy<Expr> → Expr::clone). That's a clever way to gate the import on "at least one JSX element was rewritten", but it is entirely implicit. A short comment here (and near Line 244) documenting thatruntime_id.clone()is the init point, or replacing the pattern with an explicitself.used_runtime: boolflag, would make this much less fragile to future refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 385 - 405, The check using Lazy::<Expr>::get(&self.runtime_id) relies on the side-effect of initializing the Lazy via self.runtime_id.clone() elsewhere; make this explicit by either (A) adding clear comments at the initialization site (where self.runtime_id.clone() is called) and here (around the Lazy::<Expr>::get check) explaining that clone() triggers initialization and gates the import, or (B) replace the implicit side-effect with an explicit boolean flag (e.g., add self.used_runtime: bool, set it to true at the place where self.runtime_id.clone() is currently invoked, and change Lazy::<Expr>::get(&self.runtime_id) to check self.used_runtime) so the import gating is not fragile to refactoring and is easier to reason about (referencing runtime_id, Lazy::<Expr>::get, and self.runtime_id.clone()).packages/react/transform/crates/swc_plugin_element_template/template_definition.rs (2)
242-249: Fallback for missing slot index hides a contract violation.If
is_slot_placeholder(n)istruebutslot_placeholder_index(n)returnsNone, this silently allocates a freshelement_slot_index. Since the placeholder is compiler-private and is always emitted with the index attached by the ET pipeline, anyNonehere means the compile-time slot numbering is out of sync with what the runtime lowering emitted — quietly renumbering masks that bug. Consider panicking (orunreachable!) onNoneso extractor/lowering regressions surface immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 242 - 249, The current branch silently allocates a fresh slot when is_slot_placeholder(n) is true but slot_placeholder_index(n) returns None, which masks a contract violation; change the behavior to fail-fast by replacing the unwrap_or_else fallback with a hard panic/unreachable (e.g., call expect or unreachable! with a clear message) so that when slot_placeholder_index(n) is None the code aborts instead of mutating element_slot_index; update the code that uses is_slot_placeholder, slot_placeholder_index, element_slot_index, and element_template_element_slot to reflect this fail-fast behavior and include a descriptive message identifying the placeholder `n` and that the ET pipeline should have provided an index.
328-334: Consider extracting the text-tag list to a shared constant for maintainability.While this specific check appears localized to the element template plugin currently, having the text-tag set (
text,raw-text,inline-text,x-text,x-inline-text) hardcoded invites drift if new text-like tags are added in the future. Extracting it as a shared predicate inswc_plugins_shared(e.g., alongsidejsx_helpers) or a dedicated helper module would establish a single source of truth and make it easier to extend consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 328 - 334, The hardcoded text-tag check using tag_value and the is_text_tag boolean should be replaced with a shared predicate/constant to avoid drift; add a shared symbol (e.g., TEXT_LIKE_TAGS set and an is_text_like_tag(tag: &str) -> bool) in swc_plugins_shared (or jsx_helpers), then change the local check in template_definition.rs (the is_text_tag logic that compares tag_value to "text", "raw-text", "inline-text", "x-text", "x-inline-text") to call that shared is_text_like_tag(tag_value) instead so all plugins use the single source of truth.packages/react/transform/index.d.ts (1)
603-619:ElementTemplateConfigis a byte-for-byte duplicate ofJsxTransformerConfig.These two
@internalinterfaces (L587-602 and L603-619) currently have identical field lists. Since this file is auto-generated from the Rust structs, the duplication originates from the two separate configs incrates/swc_plugin_element_template/lib.rsand the snapshot plugin. If ET is expected to remain config-compatible with the snapshot JSX transformer long-term, consider having the Rust side define a single shared config struct (or a type alias) so the generated TS stays in sync automatically; otherwise, every future field added to one will have to be duplicated in the other.🤖 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 603 - 619, ElementTemplateConfig is byte-for-byte identical to JsxTransformerConfig; instead of maintaining two separate Rust structs that generate duplicated TS types, refactor the Rust source to use a single shared config struct or a type alias in crates/swc_plugin_element_template/lib.rs (or the snapshot plugin) so the codegen emits a single interface; after updating Rust, regenerate the TypeScript declarations so ElementTemplateConfig and JsxTransformerConfig are unified (or one becomes an alias) to prevent future drift.packages/react/transform/crates/swc_plugin_element_template/napi.rs (1)
120-121: Redundant default unwrap.
JSXTransformerConfig::default()setsenable_ui_source_map: Some(false)(Line 106), soval.enable_ui_source_map.unwrap_or(false)effectively mirrors the same default twice. Not a bug — just worth reconciling so the "optional → required" coercion lives in one place (either keepOption<bool>everywhere and unwrap at the leaf, or default toSome(false)and use.unwrap()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs` around lines 120 - 121, The code redundantly applies unwrap_or(false) for enable_ui_source_map while JSXTransformerConfig::default() already sets Some(false); change the coercion to a plain unwrap to keep the "optional → required" conversion in one place: replace val.enable_ui_source_map.unwrap_or(false) with val.enable_ui_source_map.unwrap() (or alternatively remove the Some(false) default and keep unwrap_or if you prefer coercion at the leaf), and ensure JSXTransformerConfig::default() and usages of enable_ui_source_map consistently treat it as Option<bool> until this single unwrap point.
🤖 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_element_template/asset.rs`:
- Around line 22-40: The builtin_raw_text_template_asset function currently sets
source_file to self.cfg.filename which causes identical builtin templates to be
stamped with different source_file values; change
ElementTemplateAsset.source_file in builtin_raw_text_template_asset to a
canonical sentinel (e.g., empty string, "builtin", or a dedicated constant like
BUILTIN_SOURCE) so that the builtin template (template_id
BUILTIN_RAW_TEXT_TEMPLATE_ID) is emitted with a consistent source_file across
modules and can be deduplicated by downstream consumers.
In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs`:
- Around line 57-68: The from_ns function currently panics for unknown
namespaced attributes and ignores the namespace; change it to safely handle
unknown namespaces by returning a generic Attr variant (or emitting a
diagnostic) instead of todo!(), and incorporate the namespace into the
discriminator for cases that depend on it (e.g., classify worklet ref only when
both ns and local name match expected values). Update AttrName::from_ns to
examine both _ns (Ident) and name (Ident), match "ref" and event/gesture only
when the namespace criteria are satisfied, and otherwise return Attr (or produce
a diagnostic) so unrecognized namespaced attributes do not abort the transform.
In `@packages/react/transform/crates/swc_plugin_element_template/Cargo.toml`:
- Line 22: Update the swc_core dependency features in Cargo.toml: remove
"ecma_minifier", replace the private "__visit" feature with the public
"ecma_visit", and replace "__testing_transform" with "testing_transform" so the
swc_core features list uses public feature names (e.g., the swc_core dependency
entry that currently lists "base", "ecma_codegen", "ecma_parser",
"ecma_minifier", "ecma_transforms_typescript", "ecma_utils", "ecma_quote",
"ecma_transforms_react", "ecma_transforms_optimization", "__visit",
"__testing_transform" should instead include "ecma_visit" and
"testing_transform" and omit "ecma_minifier").
In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs`:
- Around line 269-280: The match arm fallback in extractor.rs is dead because
AttrName::from_ns(...) panics for unsupported names, so replace the `_ =>
todo!()` branch with an explicit unreachable! macro (e.g., unreachable!("only
main-thread:ref, event bindings, and gesture are supported")) to document
intent; locate the JSXAttrName::JSXNamespacedName handling that builds `key` via
template_namespaced_attribute_descriptor_key` and matches on `AttrName::from_ns`
(matching AttrName::WorkletEvent | AttrName::WorkletRef | AttrName::Gesture and
calling self.push_dynamic_jsx_attr) and swap the `_` arm accordingly or remove
it entirely in favor of the unreachable! message.
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 143-146: JSXTransformer currently defaults content_hash to the
literal "test", which risks collisions; change construction so content_hash is
not a hardcoded sentinel: make JSXTransformer::new (or the relevant constructor)
require a content_hash parameter or set content_hash to an empty string and
enforce the boundary to call with_content_hash; update any factory/constructor
callers to pass the real hash and add a debug-only warning when content_hash is
empty (so template_uid generation—used where template_uid is formed as
`_et_<filename_hash>_<content_hash>_<counter>`—won't collapse across files), and
ensure with_content_hash remains available for callers that already set it.
- Around line 183-198: The code currently calls
css_id.parse::<f64>().expect(...) inside the pragma parsing loop, which will
panic on invalid user input; replace this panic with proper error handling:
instead of calling expect in the parsing of css_id in the loop that checks for
"@jsxCSSId", attempt to parse with match or if let Ok(_) and on Err emit a
compiler diagnostic using the same HANDLER pattern used in visit_mut_jsx_element
(create and emit an error with context and the offending css_id string), or
simply skip validation entirely if it’s unused; ensure you reference the pragma
parsing block (words.next(), if let Some("@jsxCSSId"), css_id.parse::<f64>())
when making the change.
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 180-187: with_ui_source_map_records currently replaces the
existing Rc<RefCell<Vec<...>>> on self.inner and self, dropping any records that
were captured earlier; to prevent silent data loss, before assigning
self.inner.ui_source_map_records = ui_source_map_records.clone() borrow the
existing inner.ui_source_map_records (via borrow() on the RefCell) and assert
that its len() == 0 (use debug_assert! or assert! with a clear message
referencing JSXTransformer::new_with_element_templates) so the replacement only
proceeds when there are no prior records; alternatively, consider making this
value a constructor parameter instead of a post-hoc setter if callers need to
provide an initial shared buffer.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 302-305: Replace the debug-only assertion with a hard assertion so
mismatches between the extractor and template builder fail in release builds:
change the debug_assert_eq! that compares slot_key and &key in
template_definition.rs to assert_eq! (so the contract between
ElementTemplateExtractor and element_template_from_jsx_element_impl is enforced
at runtime); ensure the assert message stays descriptive ("Template Definition
attr slot key must match extractor output") and reference the same symbols
slot_key and key to locate the check.
- Around line 262-313: The two-pass mismatch is caused by extractor reserving a
dynamic slot for "__lynx_part_id" while template_definition.rs skips it; update
the extractor so it mirrors the template walk by treating "__lynx_part_id" like
"key" and not reserving a slot: in the attribute retention loop in extractor.rs
(the loop that handles "key" and calls push_dynamic_jsx_attr_or_spread), detect
the attribute name "__lynx_part_id" and remove/skip it instead of calling
push_dynamic_jsx_attr_or_spread/push_dynamic_attr, ensuring
push_dynamic_jsx_attr_or_spread and push_dynamic_attr are not invoked for
"__lynx_part_id" so both walks stay in lock-step.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs`:
- Around line 594-596: Remove the unsafe environment mutation from tests: delete
the std::env::set_var("INSTA_UPDATE", "always") (and any similar
std::env::set_var calls) from the test bodies that check std::env::var("UPDATE")
and instead rely on callers to set INSTA_UPDATE externally (e.g., run tests with
INSTA_UPDATE=always cargo test or use cargo insta review); update the crate
README or contributing notes to document the recommended workflow and remove
these env mutations from any functions/tests referencing
std::env::set_var("INSTA_UPDATE", "always") or checking std::env::var("UPDATE").
In `@packages/react/transform/swc-plugin-reactlynx/src/lib.rs`:
- Around line 255-267: The inline allocation
Rc<RefCell<Vec<ElementTemplateAsset>>> passed to
ElementTemplateTransformer::new_with_element_templates is never retained on the
WASM path, so replace that Some(Rc::new(RefCell::new(vec![]))) argument with
None (or alternatively wire up a take_element_templates()-style extraction to
return the buffer) so you avoid the per-invocation allocation; update the call
site around ElementTemplateTransformer::new_with_element_templates (and any
related with_content_hash/use_element_template_plugin handling) to pass None
when templates are not consumed.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs`:
- Around line 71-83: The function get_event_type_and_name currently recompiles
the regex on every call; hoist the Regex into a static Lazy to compile once: add
a static RE: Lazy<Regex> (using once_cell::sync::Lazy) initialized with
Regex::new(r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$").unwrap(),
then update get_event_type_and_name to use RE.captures(props_key) instead of
Regex::new(...).unwrap(); keep the same captures.get(...) logic and return
behavior so callers of get_event_type_and_name (and conversions
From<String>/From<Str>/From<Ident>/from_ns) benefit from one-time compilation.
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 376-381: The code calls validate_directives for the module span
and then again for every top-level item, causing
self.comments.with_leading(span.lo, …) to re-scan leading comments repeatedly;
either remove the per-item loop in visit_mut_module so validate_directives is
only called once for n.span, or if the per-item validation is required (e.g., to
catch directives attached to exported statements), add a short comment above the
loop explaining that intent and why repeated scans are necessary; update
references to visit_mut_module, validate_directives, n.span, n.body, and
self.comments.with_leading accordingly.
- Around line 385-405: The check using Lazy::<Expr>::get(&self.runtime_id)
relies on the side-effect of initializing the Lazy via self.runtime_id.clone()
elsewhere; make this explicit by either (A) adding clear comments at the
initialization site (where self.runtime_id.clone() is called) and here (around
the Lazy::<Expr>::get check) explaining that clone() triggers initialization and
gates the import, or (B) replace the implicit side-effect with an explicit
boolean flag (e.g., add self.used_runtime: bool, set it to true at the place
where self.runtime_id.clone() is currently invoked, and change
Lazy::<Expr>::get(&self.runtime_id) to check self.used_runtime) so the import
gating is not fragile to refactoring and is easier to reason about (referencing
runtime_id, Lazy::<Expr>::get, and self.runtime_id.clone()).
In `@packages/react/transform/crates/swc_plugin_element_template/lowering.rs`:
- Around line 94-131: The special-case branch handling (1,
Some(DynamicElementPart::Slot(expr, 0))) is redundant because it performs the
same lowering as the general iteration; remove that fast-path arm and let the
fallback loop handle all DynamicElementPart variants so logic like setting
self.used_slot and pushing wrap_in_slot(&self.slot_ident, element_index,
vec![child]) lives only in the iterator branch. Keep using
expr_to_jsx_child(expr) and preserve the TransformTarget::LEPUS check around
setting self.used_slot; ensure both DynamicElementPart::Slot and
DynamicElementPart::ListSlot are handled in the single for dynamic_child loop
and that element_index remains 0 for the former case.
- Around line 147-163: Add a one-line comment above the expect in the LEPUS
branch documenting the invariant: state that every element of
rendered_slot_children must originate from wrap_in_slot(...) so
lower_lepus_et_children_expr(...) will always succeed; reference
TransformTarget::LEPUS, rendered_slot_children, wrap_in_slot, and
lower_lepus_et_children_expr in the comment to make the pairing explicit and
help future maintainers avoid introducing non-slot children that would cause the
expect to panic.
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 120-121: The code redundantly applies unwrap_or(false) for
enable_ui_source_map while JSXTransformerConfig::default() already sets
Some(false); change the coercion to a plain unwrap to keep the "optional →
required" conversion in one place: replace
val.enable_ui_source_map.unwrap_or(false) with val.enable_ui_source_map.unwrap()
(or alternatively remove the Some(false) default and keep unwrap_or if you
prefer coercion at the leaf), and ensure JSXTransformerConfig::default() and
usages of enable_ui_source_map consistently treat it as Option<bool> until this
single unwrap point.
In `@packages/react/transform/crates/swc_plugin_element_template/slot.rs`:
- Around line 53-56: The current match accepts any non-negative f64 and uses
*value as usize which silently truncates non-integers and mishandles NaN/inf;
update the guard on the Expr::Lit(Lit::Num(Number { value, .. })) arm to only
accept finite integral values within usize range (e.g., check value.is_finite(),
value.fract() == 0.0, value >= 0.0, and value <= (usize::MAX as f64)) and only
then convert to usize (slot_id) — otherwise return None; adjust the match arm
that produces slot_id accordingly to use these stricter checks instead of the
current *value as usize conversion.
- Around line 36-48: In unwrap_et_slot_expr, replace the identifier equality
check that compares only ident.sym with a hygiene-safe comparison using to_id():
ensure you call ident.to_id() and slot_ident.to_id() and return None when they
differ; this mirrors usages elsewhere (e.g., swc_plugin_refresh) and ensures the
private_ident!()-created __etSlot marker is compared including SyntaxContext
rather than just the symbol.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 242-249: The current branch silently allocates a fresh slot when
is_slot_placeholder(n) is true but slot_placeholder_index(n) returns None, which
masks a contract violation; change the behavior to fail-fast by replacing the
unwrap_or_else fallback with a hard panic/unreachable (e.g., call expect or
unreachable! with a clear message) so that when slot_placeholder_index(n) is
None the code aborts instead of mutating element_slot_index; update the code
that uses is_slot_placeholder, slot_placeholder_index, element_slot_index, and
element_template_element_slot to reflect this fail-fast behavior and include a
descriptive message identifying the placeholder `n` and that the ET pipeline
should have provided an index.
- Around line 328-334: The hardcoded text-tag check using tag_value and the
is_text_tag boolean should be replaced with a shared predicate/constant to avoid
drift; add a shared symbol (e.g., TEXT_LIKE_TAGS set and an
is_text_like_tag(tag: &str) -> bool) in swc_plugins_shared (or jsx_helpers),
then change the local check in template_definition.rs (the is_text_tag logic
that compares tag_value to "text", "raw-text", "inline-text", "x-text",
"x-inline-text") to call that shared is_text_like_tag(tag_value) instead so all
plugins use the single source of truth.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs`:
- Around line 22-67: The test harness currently calls module.visit_mut_with(&mut
transformer) without installing a SWC diagnostics HANDLER, which can cause
panics if the transformer emits diagnostics; wrap the transformation in a
HANDLER by creating a DiagnosticCollector (as used in element_template.rs) and
calling HANDLER.set(&collector, || { module.visit_mut_with(&mut transformer); })
(or similarly install the collector for the closure that performs the emit), so
diagnostics are captured rather than panicking; reference the existing
DiagnosticCollector construction from element_template.rs and the symbols
MODULE.visit_mut_with, JSXTransformer, and HANDLER.set to locate where to insert
this.
In `@packages/react/transform/index.d.ts`:
- Around line 603-619: ElementTemplateConfig is byte-for-byte identical to
JsxTransformerConfig; instead of maintaining two separate Rust structs that
generate duplicated TS types, refactor the Rust source to use a single shared
config struct or a type alias in crates/swc_plugin_element_template/lib.rs (or
the snapshot plugin) so the codegen emits a single interface; after updating
Rust, regenerate the TypeScript declarations so ElementTemplateConfig and
JsxTransformerConfig are unified (or one becomes an alias) to prevent future
drift.
In `@packages/react/transform/src/lib.rs`:
- Around line 500-519: The repeated conditional fan-out can be simplified:
create a single reference like selected_config = if use_element_template_plugin
{ &element_template_plugin_config } else { &snapshot_plugin_config } and then
derive jsx_backend_enabled, active_jsx_import_source, preserve_jsx, and
enable_ui_source_map from that selected_config (e.g.,
selected_config.jsx_import_source.clone(), selected_config.preserve_jsx,
selected_config.enable_ui_source_map.unwrap_or(false)), keeping the existing
use_element_template_plugin, use_snapshot_plugin, and jsx_backend_enabled logic
intact to avoid behavioral changes.
- Around line 302-338: Two nearly identical helpers
clone_snapshot_ui_source_map_records and
clone_element_template_ui_source_map_records should be collapsed into one
generic function: define a trait (e.g. IntoUiSourceMapRecord) with a method
into_ui_record(&self, filename: &str) -> UiSourceMapRecord, implement it for
SnapshotCoreUISourceMapRecord and ElementTemplateCoreUISourceMapRecord (setting
snapshot_id or template_id appropriately), then replace both helper functions
with a single fn clone_ui_source_map_records<R: IntoUiSourceMapRecord +
Clone>(records: &Rc<RefCell<Vec<R>>>, filename: &str) -> Vec<UiSourceMapRecord>
that borrows, iterates, clones and maps via r.into_ui_record(filename); update
call sites that used clone_snapshot_ui_source_map_records and
clone_element_template_ui_source_map_records to call clone_ui_source_map_records
instead.
In `@packages/react/transform/swc-plugin-reactlynx/index.d.ts`:
- Around line 30-46: ElementTemplateConfig duplicates JsxTransformerConfig;
replace the duplicate interface with a single type alias or an extends
declaration so the shapes stay in sync (e.g., make ElementTemplateConfig a type
alias to JsxTransformerConfig or declare "export interface ElementTemplateConfig
extends JsxTransformerConfig {}"), preserving the /** `@internal` */ comment and
exported name so call-sites and docs remain unchanged; update any imports/usages
if necessary to reference the unified ElementTemplateConfig symbol.
🪄 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: 37aec173-8820-4e4e-a2a7-65ced602393b
⛔ Files ignored due to path filters (27)
Cargo.lockis excluded by!**/*.lockpackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_generate_attribute_slots_for_dynamic_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_background_conditional_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_boolean_and_number_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_complex_text_structure.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_dataset_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_deeply_nested_user_components.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_dynamic_class_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_events_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_events_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_id_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_inline_styles.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_interpolated_text_with_siblings_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_mixed_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_nested_structure_and_dynamic_content.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_refs_js.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_refs_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_sibling_user_components.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_spread_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_handle_user_component.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_isolate_arrays_with_element_slot_placeholder.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_keep_code_and_template_attribute_slots_in_sync_for_spread.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_output_element_template_simple_lepus.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_output_template_with_static_attributes.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_verify_template_structure_complex.snapis excluded by!**/*.snappackages/react/transform/crates/swc_plugin_element_template/tests/__combined_snapshots__/should_verify_text_attribute_and_child_text_slots.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
.changeset/social-nails-retire.mdpackages/react/transform/Cargo.tomlpackages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/Cargo.tomlpackages/react/transform/crates/swc_plugin_element_template/asset.rspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/lowering.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/slot.rspackages/react/transform/crates/swc_plugin_element_template/template_attribute.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/template_slot.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/Cargo.tomlpackages/react/transform/swc-plugin-reactlynx/index.d.tspackages/react/transform/swc-plugin-reactlynx/src/lib.rs
![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: e9d6cd215a
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/react/transform/crates/swc_plugin_element_template/template_definition.rs (2)
251-255: Minor: silently falling back to""for a non-literal tag is brittle.
jsx_name(...)for a valid JSX element should always produceExpr::Lit(Str)here (custom components are diverted by the extractor). If that invariant ever breaks, the element gets emitted as{ kind: "element", type: "", ... }which will wedge downstream consumers with no diagnostic. Adebug_assert!orunreachable!on the_arm would surface regressions loudly without changing release behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 251 - 255, The code silently falls back to an empty string when tag_expr is not a string literal; tighten this by asserting the invariant and making failures visible in debug builds: in the match over *tag_expr (the call to jsx_name and the arms constructing tag_value), replace the `_ => "".into()` fallback with a debug_assert!(matches!(... , Expr::Lit(Lit::Str(_)))) or an unreachable!() (referring to jsx_name, tag_expr, tag_value, and the Expr::Lit(Lit::Str(s)) arm) so that non-literal tags fail loudly in debug while retaining release behavior.
48-67: Minor: use a match orlet ... elseonkv.keyinstead of uninitialized binding + continue.The
let key;+if PropName::Ident / else if PropName::Str / else { continue }pattern is unusual Rust and easy to get wrong on future edits (e.g., accidentally usingkeybefore the assigning branch). A match is equivalent and more idiomatic.♻️ Proposed refactor
- if let Prop::KeyValue(kv) = &**prop { - let key; - if let PropName::Ident(ident) = &kv.key { - key = ident.sym.as_str().to_string(); - } else if let PropName::Str(s) = &kv.key { - key = s.value.as_str().unwrap_or("").to_string(); - } else { - continue; - }; - let value = self.element_template_to_json(&kv.value); - map.insert(key, value); - } + if let Prop::KeyValue(kv) = &**prop { + let key = match &kv.key { + PropName::Ident(ident) => ident.sym.as_ref().to_string(), + PropName::Str(s) => s.value.as_ref().to_string(), + _ => continue, + }; + let value = self.element_template_to_json(&kv.value); + map.insert(key, value); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 48 - 67, In the Expr::Object branch inside template_definition.rs, replace the uninitialized `let key;` + if/else assignment with a single match (or `let ... else`) on `kv.key` to produce a concrete `key` String; for example, match `kv.key` to `PropName::Ident(ident)` and `PropName::Str(s)` and `_ => continue`, then call `self.element_template_to_json(&kv.value)` and `map.insert(key, value)` as before—this makes the binding for `key` explicit and idiomatic (affects the loop handling `prop`, `Prop::KeyValue`, and use of `element_template_to_json`).packages/react/transform/crates/swc_plugin_element_template/extractor.rs (1)
347-374: Optional: thevisit_mut_with(self)calls on freshly created placeholders are no-ops.Both
child.visit_mut_with(self)at Line 351 and Line 372 wrap aslot_placeholder_node(...); yourvisit_mut_jsx_elementshort-circuits onis_slot_placeholder(n)at Line 380. The visits don't cause bugs (the outerpush_dynamic_jsx_attr_or_spreadpass at Lines 440-442 stays guarded by the literal-preservation check), but dropping them makes the merging path a bit clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs` around lines 347 - 374, Two redundant visit_mut_with(self) calls are invoked on freshly created slot placeholders (the calls immediately after creating JSXElementChild via slot_placeholder_node in the merging path); since visit_mut_jsx_element short-circuits on is_slot_placeholder, these visits are no-ops. Remove the two child.visit_mut_with(self) invocations that follow construction of the slot placeholder (the ones that call visit_mut_with on the child built from slot_placeholder_node) so the merge path is clearer while leaving other logic (e.g., dynamic_part_visitor and push_dynamic_jsx_attr_or_spread) unchanged.packages/react/transform/crates/swc_plugin_element_template/attr_name.rs (2)
56-69: Minor: lift themain-threadguard once for readability.Each branch re-checks
ns_str == "main-thread"; hoisting the guard makes the namespace contract obvious and avoids the third redundant comparison.♻️ Proposed refactor
pub fn from_ns(ns: Ident, name: Ident) -> Self { let ns_str = ns.sym.as_ref(); - let name_str = name.sym.as_ref().to_string(); - if ns_str == "main-thread" && name_str == "ref" { - AttrName::WorkletRef - } else if ns_str == "main-thread" && get_event_type_and_name(name_str.as_str()).is_some() { - AttrName::WorkletEvent - } else if ns_str == "main-thread" && name_str == "gesture" { - AttrName::Gesture - } else { - AttrName::Attr - } + let name_str = name.sym.as_ref(); + if ns_str != "main-thread" { + return AttrName::Attr; + } + match name_str { + "ref" => AttrName::WorkletRef, + "gesture" => AttrName::Gesture, + _ if get_event_type_and_name(name_str).is_some() => AttrName::WorkletEvent, + _ => AttrName::Attr, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs` around lines 56 - 69, The AttrName::from_ns function repeats the ns_str == "main-thread" check in each branch; refactor by checking ns_str once and then matching on name_str to decide between WorkletRef, WorkletEvent (use get_event_type_and_name(name_str.as_str()).is_some()), and Gesture, falling back to Attr otherwise. Keep the same symbols (ns.sym → ns_str, name.sym → name_str) and preserve existing branch logic and returned variants (AttrName::WorkletRef, AttrName::WorkletEvent, AttrName::Gesture, AttrName::Attr).
72-84: Hoist the event-pattern regex into aLazy<Regex>to avoid per-call recompilation.
Regex::new(...)is executed every timeget_event_type_and_nameis called, which happens on every attribute/namespaced-attribute classification (viaFrom<String>,From<Ident>, andfrom_ns). Compiling the same regex for each attribute of each JSX element adds measurable overhead across a build.extractor.rsalready usesonce_cell::sync::Lazy; the same pattern applies cleanly here.♻️ Proposed refactor
use regex::Regex; +use once_cell::sync::Lazy; use swc_core::ecma::ast::*; @@ -fn get_event_type_and_name(props_key: &str) -> Option<(String, String)> { - let re = Regex::new(r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$").unwrap(); - if let Some(captures) = re.captures(props_key) { +static EVENT_PATTERN_RE: Lazy<Regex> = Lazy::new(|| { + Regex::new(r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$") + .expect("event pattern regex must compile") +}); + +fn get_event_type_and_name(props_key: &str) -> Option<(String, String)> { + if let Some(captures) = EVENT_PATTERN_RE.captures(props_key) { let event_type = if captures.get(1).unwrap().as_str().contains("capture") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs` around lines 72 - 84, get_event_type_and_name currently recompiles the same regex on every call; hoist the regex into a static once_cell::sync::Lazy<Regex> (or equivalent Lazy) so it's compiled once at startup and reused. Replace the local Regex::new(...) in get_event_type_and_name with a reference to the static Lazy regex (keeping the same pattern r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$"), and update get_event_type_and_name to use captures from that static to produce event_type and event_name as before.packages/react/transform/crates/swc_plugin_element_template/napi.rs (1)
98-110: Minor:is_dynamic_component: Some(false)default is surprising.Giving the
Option<bool>defaultSome(false)instead ofNoneblurs "not set" vs "explicitly disabled" when the value flows throughFrom<JSXTransformerConfig> for CoreElementTemplateTransformerConfigat Lines 112-124 (which passesis_dynamic_componentthrough as-is). If downstream ever distinguishesNonefromSome(false), this default hides the unset case. If it never does, consider justboolto remove the ambiguity.Same pattern applies to
enable_ui_source_map, which is immediately collapsed viaunwrap_or(false)at Line 120.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs` around lines 98 - 110, The defaults for JSXTransformerConfig currently set is_dynamic_component: Some(false) and enable_ui_source_map: Some(false), which masks the unset state when converted in the From<JSXTransformerConfig> for CoreElementTemplateTransformerConfig impl (which passes is_dynamic_component through and calls unwrap_or(false) for enable_ui_source_map); change these defaults to None (or change the fields to plain bool if you intend there to be no tri-state) so the "unset" case is preserved — update JSXTransformerConfig::default to set is_dynamic_component: None and enable_ui_source_map: None (or alter the field types to bool and set false) and ensure the From impl still handles the Option-to-bool conversion via unwrap_or(false) if you keep Option types.packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs (1)
50-122: Minor: the hand-rolled JSX expression parser is tricky; add a focused unit test.
first_attribute_slots_lenis clever (quote-aware, nested-bracket-aware, empty-array aware) but there's no direct coverage for its edge cases — only indirect coverage viaassert_attribute_slots_match_template. If ET codegen ever emits anattributeSlotsvalue with unusual shapes (template strings, JSX expressions, regex literals), silent drift here would only show up as flaky template assertions.A couple of tiny
#[test]s pinningfirst_attribute_slots_len(...)on crafted inputs would lock this helper's contract cheaply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs` around lines 50 - 122, Add focused unit tests for the helper first_attribute_slots_len to lock its contract: create a few #[test] functions in element_template.rs that call first_attribute_slots_len with crafted inputs covering template strings (backticks), JSX expression fragments, regex literals, escaped quotes, nested brackets/braces/paren and empty-array cases, and assert the exact Some(len) or None outcomes using assert_eq!; pick clear names like test_first_attribute_slots_len_template_and_regex, test_first_attribute_slots_len_nested_brackets, and test_first_attribute_slots_len_empty to make failures obvious.packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs (1)
98-125: Optional: collapse the twofirst_user_template_json*helpers.
first_user_template_json,first_user_template_json_with_cfg, andfirst_user_template_json_with_coderepeat the same "find first non-builtin template and serialize" pipeline. Collapsing them onto a single core helper reduces drift risk if the filter key or serialization shape ever changes.♻️ Proposed refactor
-fn first_user_template_json(input: &str) -> Value { - first_user_template_json_with_cfg(input, element_template_config()) -} - -fn first_user_template_json_with_cfg(input: &str, cfg: JSXTransformerConfig) -> Value { - let templates = transform_to_templates(input, cfg); - - templates - .into_iter() - .find(|template| template.template_id != BUILTIN_RAW_TEXT_TEMPLATE_ID) - .map(|template| { - serde_json::to_value(template.compiled_template).expect("compiled template to json") - }) - .expect("should collect a user template") -} - -fn first_user_template_json_with_code(input: &str, cfg: JSXTransformerConfig) -> (String, Value) { - let (templates, code) = transform_fixture(input, cfg); - let template = templates - .into_iter() - .find(|template| template.template_id != BUILTIN_RAW_TEXT_TEMPLATE_ID) - .map(|template| { - serde_json::to_value(template.compiled_template).expect("compiled template to json") - }) - .expect("should collect a user template"); - - (code, template) -} +fn first_user_template(templates: Vec<ElementTemplateAsset>) -> Value { + templates + .into_iter() + .find(|template| template.template_id != BUILTIN_RAW_TEXT_TEMPLATE_ID) + .map(|template| { + serde_json::to_value(template.compiled_template).expect("compiled template to json") + }) + .expect("should collect a user template") +} + +fn first_user_template_json(input: &str) -> Value { + first_user_template(transform_to_templates(input, element_template_config())) +} + +fn first_user_template_json_with_cfg(input: &str, cfg: JSXTransformerConfig) -> Value { + first_user_template(transform_to_templates(input, cfg)) +} + +fn first_user_template_json_with_code(input: &str, cfg: JSXTransformerConfig) -> (String, Value) { + let (templates, code) = transform_fixture(input, cfg); + (code, first_user_template(templates)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs` around lines 98 - 125, Collapse the duplicate "find first non-builtin template and serialize" logic into a single helper (e.g., extract_first_user_template) and use it from first_user_template_json, first_user_template_json_with_cfg, and first_user_template_json_with_code: call transform_to_templates or take the templates part of transform_fixture, pass the templates Vec/Iterator into extract_first_user_template which performs the .into_iter().find(|t| t.template_id != BUILTIN_RAW_TEXT_TEMPLATE_ID).map(|t| serde_json::to_value(t.compiled_template).expect(...)).expect(...), and return the Value; in first_user_template_json_with_code keep returning (code, template) by calling the helper for the template part. Ensure you reference the existing symbols: transform_to_templates, transform_fixture, BUILTIN_RAW_TEXT_TEMPLATE_ID, and compiled_template.
🤖 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_element_template/napi.rs`:
- Around line 180-191: The debug-only check in with_ui_source_map_records (the
debug_assert! that self.inner.ui_source_map_records.borrow().is_empty()) can
allow silent data loss in release builds; change that debug_assert! to a hard
assert! so the empty-buffer invariant is enforced in production as well — update
the assertion in the with_ui_source_map_records method (and keep the same
message) to use assert! instead of debug_assert!.
- Around line 54-75: The reverse impl From<CoreElementTemplateUISourceMapRecord>
for UISourceMapRecord in napi.rs is unused and lossy (it fabricates filename),
so either remove that impl or add a clear doc comment on
CoreElementTemplateUISourceMapRecord/From stating the conversion is
intentionally asymmetric and will not preserve filename; update or remove the
impl near the existing impl From<UISourceMapRecord> for
CoreElementTemplateUISourceMapRecord and mention
clone_element_template_ui_source_map_records in the comment as the canonical
place where UISourceMapRecord.filename is populated.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs`:
- Around line 204-234: The test reveals that numeric literals that overflow to
f64::INFINITY become serialized as Value::Null (because
serde_json::Number::from_f64 returns None), losing the distinction between an
intentional null and an overflowed numeric literal; update
template_definition.rs where numeric literal attributes are
classified/serialized (the path that produces the "attributesArray" descriptors
and uses serde_json::Number::from_f64) to explicitly check the f64 for
is_finite() before treating it as a static Number: if the value is not finite
(Infinity or NaN) either emit a diagnostic/error or mark the attribute binding
as non-static (so it isn't serialized as Value::Null) — implement the chosen
behavior in the numeric handling branch that currently calls Number::from_f64.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs`:
- Around line 56-69: The AttrName::from_ns function repeats the ns_str ==
"main-thread" check in each branch; refactor by checking ns_str once and then
matching on name_str to decide between WorkletRef, WorkletEvent (use
get_event_type_and_name(name_str.as_str()).is_some()), and Gesture, falling back
to Attr otherwise. Keep the same symbols (ns.sym → ns_str, name.sym → name_str)
and preserve existing branch logic and returned variants (AttrName::WorkletRef,
AttrName::WorkletEvent, AttrName::Gesture, AttrName::Attr).
- Around line 72-84: get_event_type_and_name currently recompiles the same regex
on every call; hoist the regex into a static once_cell::sync::Lazy<Regex> (or
equivalent Lazy) so it's compiled once at startup and reused. Replace the local
Regex::new(...) in get_event_type_and_name with a reference to the static Lazy
regex (keeping the same pattern
r"^(global-bind|bind|catch|capture-bind|capture-catch)([A-Za-z]+)$"), and update
get_event_type_and_name to use captures from that static to produce event_type
and event_name as before.
In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs`:
- Around line 347-374: Two redundant visit_mut_with(self) calls are invoked on
freshly created slot placeholders (the calls immediately after creating
JSXElementChild via slot_placeholder_node in the merging path); since
visit_mut_jsx_element short-circuits on is_slot_placeholder, these visits are
no-ops. Remove the two child.visit_mut_with(self) invocations that follow
construction of the slot placeholder (the ones that call visit_mut_with on the
child built from slot_placeholder_node) so the merge path is clearer while
leaving other logic (e.g., dynamic_part_visitor and
push_dynamic_jsx_attr_or_spread) unchanged.
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 98-110: The defaults for JSXTransformerConfig currently set
is_dynamic_component: Some(false) and enable_ui_source_map: Some(false), which
masks the unset state when converted in the From<JSXTransformerConfig> for
CoreElementTemplateTransformerConfig impl (which passes is_dynamic_component
through and calls unwrap_or(false) for enable_ui_source_map); change these
defaults to None (or change the fields to plain bool if you intend there to be
no tri-state) so the "unset" case is preserved — update
JSXTransformerConfig::default to set is_dynamic_component: None and
enable_ui_source_map: None (or alter the field types to bool and set false) and
ensure the From impl still handles the Option-to-bool conversion via
unwrap_or(false) if you keep Option types.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 251-255: The code silently falls back to an empty string when
tag_expr is not a string literal; tighten this by asserting the invariant and
making failures visible in debug builds: in the match over *tag_expr (the call
to jsx_name and the arms constructing tag_value), replace the `_ => "".into()`
fallback with a debug_assert!(matches!(... , Expr::Lit(Lit::Str(_)))) or an
unreachable!() (referring to jsx_name, tag_expr, tag_value, and the
Expr::Lit(Lit::Str(s)) arm) so that non-literal tags fail loudly in debug while
retaining release behavior.
- Around line 48-67: In the Expr::Object branch inside template_definition.rs,
replace the uninitialized `let key;` + if/else assignment with a single match
(or `let ... else`) on `kv.key` to produce a concrete `key` String; for example,
match `kv.key` to `PropName::Ident(ident)` and `PropName::Str(s)` and `_ =>
continue`, then call `self.element_template_to_json(&kv.value)` and
`map.insert(key, value)` as before—this makes the binding for `key` explicit and
idiomatic (affects the loop handling `prop`, `Prop::KeyValue`, and use of
`element_template_to_json`).
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs`:
- Around line 98-125: Collapse the duplicate "find first non-builtin template
and serialize" logic into a single helper (e.g., extract_first_user_template)
and use it from first_user_template_json, first_user_template_json_with_cfg, and
first_user_template_json_with_code: call transform_to_templates or take the
templates part of transform_fixture, pass the templates Vec/Iterator into
extract_first_user_template which performs the .into_iter().find(|t|
t.template_id != BUILTIN_RAW_TEXT_TEMPLATE_ID).map(|t|
serde_json::to_value(t.compiled_template).expect(...)).expect(...), and return
the Value; in first_user_template_json_with_code keep returning (code, template)
by calling the helper for the template part. Ensure you reference the existing
symbols: transform_to_templates, transform_fixture,
BUILTIN_RAW_TEXT_TEMPLATE_ID, and compiled_template.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs`:
- Around line 50-122: Add focused unit tests for the helper
first_attribute_slots_len to lock its contract: create a few #[test] functions
in element_template.rs that call first_attribute_slots_len with crafted inputs
covering template strings (backticks), JSX expression fragments, regex literals,
escaped quotes, nested brackets/braces/paren and empty-array cases, and assert
the exact Some(len) or None outcomes using assert_eq!; pick clear names like
test_first_attribute_slots_len_template_and_regex,
test_first_attribute_slots_len_nested_brackets, and
test_first_attribute_slots_len_empty to make failures obvious.
🪄 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: b7742d84-0366-4830-b85b-cfbd23f0476d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/react/transform/crates/swc_plugin_element_template/Cargo.tomlpackages/react/transform/crates/swc_plugin_element_template/asset.rspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/swc-plugin-reactlynx/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_element_template/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/transform/crates/swc_plugin_element_template/lib.rs
- packages/react/transform/swc-plugin-reactlynx/src/lib.rs
Yradex
force-pushed
the
wt/element-template
branch
from
65be7de to
7923582
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react/transform/crates/swc_plugin_element_template/extractor.rs (1)
405-422: Deadis_listbranch — the ListSlot arm here is unreachable.Reaching Line 406 requires
self.parent_element && jsx_has_dynamic_key(n)(from the outerifon Line 405). But the block at Line 389-400 already handlesself.parent_element && jsx_is_list(n)with an earlyreturnon Line 399, so whenever we get to Line 406 we're guaranteed!jsx_is_list(n)and thusis_list == false. The ListSlot push on Line 412-414 can never execute.Not a correctness issue today, but it's misleading to read and will silently desync if someone later changes the Line 389 guard (e.g., to skip lists without dynamic keys). Consider dropping the
is_listvariable and the unreachable branch, ordebug_assert!(!jsx_is_list(n))to document the invariant.♻️ Suggested simplification
if jsx_has_dynamic_key(n) && self.parent_element { - let is_list = jsx_is_list(n); + debug_assert!( + !jsx_is_list(n), + "dynamic-key <list> must be handled by the parent_element && jsx_is_list branch above", + ); n.visit_mut_with(self.dynamic_part_visitor); let expr = Expr::JSXElement(Box::new(n.take())); let slot_index = self.next_children_slot_index(); - if is_list { - self - .dynamic_children - .push(DynamicElementPart::ListSlot(expr, slot_index)); - } else { - self - .dynamic_children - .push(DynamicElementPart::Slot(expr, slot_index)); - } + self + .dynamic_children + .push(DynamicElementPart::Slot(expr, slot_index)); *n = slot_placeholder_node(slot_index, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs` around lines 405 - 422, The branch that pushes DynamicElementPart::ListSlot is unreachable because earlier code already returns when self.parent_element && jsx_is_list(n), so remove the dead is_list check and the ListSlot arm: inline the handling to always push DynamicElementPart::Slot and call slot_placeholder_node(slot_index, false), or alternatively add debug_assert!(!jsx_is_list(n)) after computing slot_index to document the invariant; update references around jsx_has_dynamic_key, jsx_is_list, dynamic_part_visitor, next_children_slot_index, DynamicElementPart::Slot/ListSlot and slot_placeholder_node accordingly so the code no longer contains the unreachable ListSlot branch.
🤖 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_element_template/napi.rs`:
- Around line 45-63: The UISourceMapRecord struct contains a filename field that
is silently ignored by the From<UISourceMapRecord> for
CoreElementTemplateUISourceMapRecord conversion; update the code to make this
explicit by adding a doc comment on the From impl (or on UISourceMapRecord)
stating that filename is intentionally excluded and is populated later by
clone_element_template_ui_source_map_records in the outer pipeline, or if
filename is truly unused on input, remove the filename field from
UISourceMapRecord to avoid the footgun; reference UISourceMapRecord, the
From<UISourceMapRecord> for CoreElementTemplateUISourceMapRecord impl, and
clone_element_template_ui_source_map_records when making the change.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 322-351: The text-tag optimization unconditionally pushes a static
"text" attribute and can duplicate an explicit user-provided "text" descriptor;
update the block that sets text_child_optimized (the code using is_text_tag,
valid_children, JSXElementChild and calling
element_template_static_attribute_descriptor / element_template_string_expr) to
first detect whether attribute_descriptors already contains a descriptor with
key "text" and either skip adding the optimized descriptor or deterministically
replace the existing entry: locate the existing descriptor by its key and either
do nothing (skip) or overwrite its value with the optimized string, ensuring
only one "text" descriptor remains in attribute_descriptors.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs`:
- Around line 405-422: The branch that pushes DynamicElementPart::ListSlot is
unreachable because earlier code already returns when self.parent_element &&
jsx_is_list(n), so remove the dead is_list check and the ListSlot arm: inline
the handling to always push DynamicElementPart::Slot and call
slot_placeholder_node(slot_index, false), or alternatively add
debug_assert!(!jsx_is_list(n)) after computing slot_index to document the
invariant; update references around jsx_has_dynamic_key, jsx_is_list,
dynamic_part_visitor, next_children_slot_index,
DynamicElementPart::Slot/ListSlot and slot_placeholder_node accordingly so the
code no longer contains the unreachable ListSlot branch.
🪄 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: e8c528d6-3ab4-4759-8825-f4054cbebd69
📒 Files selected for processing (5)
packages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs
Yradex
force-pushed
the
wt/element-template
branch
from
7923582 to
04e5ed6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/react/transform/src/lib.rs (1)
299-315: Always populatessnapshot_id = Some(..)— confirm TS type change is consistent.
clone_snapshot_ui_source_map_recordsunconditionally wrapsrecord.snapshot_idinSome, yet the type on the N-API side was changed sosnapshotIdis optional (to accommodate future ET-originated records). That's fine for now — only Snapshot populates records today — but if/when ET begins emitting UI source map records, consumers may start seeing records withoutsnapshotIdwhile this helper still guarantees it. Consider adding a short doc comment thatsnapshot_idis alwaysSomeon the snapshot path so the optionality is understood as a forward-compat signal, not current behavior.🤖 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 299 - 315, The helper clone_snapshot_ui_source_map_records currently always sets snapshot_id = Some(record.snapshot_id) even though the N-API/TS type made snapshotId optional; add a brief doc comment above clone_snapshot_ui_source_map_records (and/or on UiSourceMapRecord if appropriate) stating that snapshot_id is guaranteed to be Some for records produced via the Snapshot path today and the optionality on the TS side is a forward-compatibility signal for future ET-originated records; do not change the current behavior, just document the invariants so callers understand why this helper always populates Some.packages/react/transform/crates/swc_plugin_element_template/lib.rs (1)
276-280:assert_eq!on slot-cursor invariant will panic the compiler on a bug.A mismatch between
dynamic_attr_slot_cursoranddynamic_attr_slots.len()would hard-abort the SWC pass with a Rust panic that surfaces as an internal compiler error rather than a diagnostic. For an internal invariant that's already well covered by tests this is acceptable, but considerdebug_assert_eq!(or anHANDLER.with(...).struct_span_errin release) so a regression in a user build produces a clean error instead of a panic trace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 276 - 280, The assert_eq! on dynamic_attr_slot_cursor vs dynamic_attr_slots.len() will panic in release builds; replace it with a non-panicking check: use debug_assert_eq!(dynamic_attr_slot_cursor, dynamic_attr_slots.len()) for development builds and add a graceful diagnostic path for release by emitting a compiler-friendly error (e.g., HANDLER.with(|h| h.struct_span_err(...).emit())) when the lengths differ; update the check around the assert_eq! invocation (referencing dynamic_attr_slot_cursor, dynamic_attr_slots.len(), and the assert_eq! call) so that test-time panics still catch regressions but user builds produce a structured diagnostic instead of a panic.packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs (1)
27-33: Panicking diagnostic collector hides warnings in contract tests.
DiagnosticCollector::emitpanics on any diagnostic, which means a test fixture that accidentally triggers a non-fatal warning (e.g., an unused helper, a future deprecation notice from another pass) will fail with an opaque panic message rather than a structured assertion. For a contract-test harness that asserts on specific invariants, consider collecting diagnostics into aVec<String>and letting each test opt-in toassert!(collected.is_empty()), so unrelated warnings become readable test output instead of panics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs` around lines 27 - 33, DiagnosticCollector currently panics on any diagnostic in DiagnosticEmitter::emit, which hides non-fatal warnings; change DiagnosticCollector to store diagnostics instead of panicking by adding a field (e.g., diagnostics: Vec<String>) to DiagnosticCollector, update the DiagnosticEmitter::emit implementation to push db.message().to_string() (or formatted detail) into that Vec rather than calling panic!, and expose an accessor (e.g., collected() -> &Vec<String> or into_vec()) so tests can opt-in to assert on collected diagnostics (e.g., assert!(collected.is_empty()) or assert_eq! on specific messages).packages/react/transform/crates/swc_plugin_element_template/extractor.rs (1)
307-350: Consider renamingshould_merge/current_chunkfor clarity.The naming reads inverted to the logic:
should_merge == trueactually flushescurrent_chunkand emits the child directly, whileshould_merge == falseaccumulates dynamic children intocurrent_chunk(which later becomes a single slot). A name likeis_static_flushable/dynamic_runwould make the intent obvious and reduce bug risk for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs` around lines 307 - 350, Rename the inverted boolean and buffer variables to make their intent explicit: change should_merge to a name like is_static_flushable (or is_flushable_static) and current_chunk to dynamic_run (or dynamic_buffer), then update every use in the loop where should_merge is checked and where current_chunk is pushed/emptied; ensure the conditional branches that call current_chunk.visit_mut_with(self.dynamic_part_visitor), self.next_children_slot_index(), self.dynamic_children.push(DynamicElementPart::Slot(...)), jsx_children_to_expr(current_chunk.take()), and slot_placeholder_node(...) remain semantically identical but now reference the new names, and update calls to child.visit_mut_with(self) and merged_children.push(child) accordingly so the logic reads clearly (e.g., if is_static_flushable { flush dynamic_run; emit child } else { dynamic_run.push(child) }).packages/react/transform/crates/swc_plugin_element_template/attr_name.rs (1)
79-90: Nit —get_event_type_and_namereturn value is never consumed.The only callsites (
From<String>line 40 andfrom_nsline 73) useis_some(). Theevent_type/event_nametuple is dead. Consider simplifying tofn is_event_key(&str) -> boolto make intent obvious and avoid allocating the captured strings on every attribute check. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs` around lines 79 - 90, get_event_type_and_name currently builds and returns an unused (String, String) tuple; replace it with a simple predicate is_event_key(&str) -> bool that returns whether EVENT_KEY_RE matches the input (e.g., EVENT_KEY_RE.is_match(props_key)) to avoid allocating captured strings, and update callsites (the From<String> implementation and from_ns) to call is_event_key(props_key) instead of checking is_some() on the old function.packages/react/transform/__test__/fixture.spec.js (1)
190-197: Consider asserting a concrete shape forcompiledTemplate.
expect(template?.compiledTemplate).toEqual(expect.any(Object))plusArray.isArray(...) === falseis a loose contract; a future regression that replaces the descriptor JSON with, say,{}or an error-stub object would still pass. Consider asserting at least one expected key (e.g.,attributesArray/children) to lock the ET JSON contract at the JS boundary. Non-blocking; the Rust contract tests inelement_template_contract.rsalready cover deeper structure.🤖 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 190 - 197, The test currently only checks that result.elementTemplates exists and template.compiledTemplate is an object (and not an array); tighten this by asserting concrete keys on template.compiledTemplate (e.g., ensure template.compiledTemplate has properties like "attributesArray" and/or "children") so regressions replacing the descriptor with an empty object or stub will fail; update the assertions around result.elementTemplates / template (from the fixture.spec.js test) to use expect(...).toHaveProperty for those expected keys on template.compiledTemplate while keeping the existing templateId checks.packages/react/transform/crates/swc_plugin_element_template/napi.rs (1)
99-151:take_element_templatesandpub element_templateslook redundant with the shared collector.In the production path (
src/lib.rs), callers construct this wrapper vianew_with_element_templates(..., element_templates_collector.clone())and then drainelement_templates_collectordirectly after the SWC pass. That means:
JSXTransformer::take_element_templatesis never called from the NAPI consumer.pub element_templatesexposes an aliasedRc<RefCell<..>>that callers could mutate concurrently with the core transformer.Consider either removing
take_element_templates+ making the field private, or removing the duplicate drain insrc/lib.rsin favor of the wrapper method. Consolidating ownership avoids two code paths both laying claim to the same buffer. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs` around lines 99 - 151, Make the shared collector owned only by the transformer: remove the public visibility on the element_templates field (change pub element_templates -> element_templates) and preserve/export the take_element_templates(&self) method as the single drain API; then update any call sites that currently drain the Rc<RefCell<...>> directly to call JSXTransformer::take_element_templates() instead (locate usage by symbol element_templates_collector or direct .borrow_mut().drain(..) calls) so there is only one canonical place that drains the buffer and no external mutable aliasing.
🤖 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_element_template/napi.rs`:
- Around line 153-168: The wrapper JSXTransformer<C> currently implements a
fixed set of VisitMut methods (visit_mut_jsx_element, visit_mut_module_items,
visit_mut_module) and will silently miss future overrides; to fix, implement
Deref and DerefMut for JSXTransformer<C> delegating to its inner field so all
VisitMut methods automatically forward (implement std::ops::Deref for
JSXTransformer<C> { type Target = InnerType; fn deref(&self)->&Self::Target {
&self.inner } } and DerefMut similarly), or alternatively introduce a small
forwarding macro used both here and in lib.rs to mirror all VisitMut methods
(e.g., include visit_mut_jsx_fragment) with a compile-time check; reference
symbols: JSXTransformer<C>, inner, VisitMut, visit_mut_jsx_element,
visit_mut_jsx_fragment, and lib.rs when making the change.
In `@packages/react/transform/src/lib.rs`:
- Around line 763-792: The current logic in the element_templates_collector
closure collapses an empty templates Vec into None, losing the signal that the
element-template (ET) plugin ran; change the closure used to compute
element_templates (the code that builds `templates: Vec<ElementTemplateAsset>`
from `element_templates_collector`) so that if `element_templates_collector` is
Some you always return Some(templates) (even when templates.is_empty()), and
only return None when `element_templates_collector` itself is None; update the
`element_templates` assignment used in the TransformNodiffOutput so downstream
code can distinguish “ET ran but emitted nothing” from “ET not enabled”
(preserve all other behavior, including referencing
`use_element_template_plugin` and `clone_snapshot_ui_source_map_records`
unchanged).
---
Nitpick comments:
In `@packages/react/transform/__test__/fixture.spec.js`:
- Around line 190-197: The test currently only checks that
result.elementTemplates exists and template.compiledTemplate is an object (and
not an array); tighten this by asserting concrete keys on
template.compiledTemplate (e.g., ensure template.compiledTemplate has properties
like "attributesArray" and/or "children") so regressions replacing the
descriptor with an empty object or stub will fail; update the assertions around
result.elementTemplates / template (from the fixture.spec.js test) to use
expect(...).toHaveProperty for those expected keys on template.compiledTemplate
while keeping the existing templateId checks.
In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs`:
- Around line 79-90: get_event_type_and_name currently builds and returns an
unused (String, String) tuple; replace it with a simple predicate
is_event_key(&str) -> bool that returns whether EVENT_KEY_RE matches the input
(e.g., EVENT_KEY_RE.is_match(props_key)) to avoid allocating captured strings,
and update callsites (the From<String> implementation and from_ns) to call
is_event_key(props_key) instead of checking is_some() on the old function.
In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs`:
- Around line 307-350: Rename the inverted boolean and buffer variables to make
their intent explicit: change should_merge to a name like is_static_flushable
(or is_flushable_static) and current_chunk to dynamic_run (or dynamic_buffer),
then update every use in the loop where should_merge is checked and where
current_chunk is pushed/emptied; ensure the conditional branches that call
current_chunk.visit_mut_with(self.dynamic_part_visitor),
self.next_children_slot_index(),
self.dynamic_children.push(DynamicElementPart::Slot(...)),
jsx_children_to_expr(current_chunk.take()), and slot_placeholder_node(...)
remain semantically identical but now reference the new names, and update calls
to child.visit_mut_with(self) and merged_children.push(child) accordingly so the
logic reads clearly (e.g., if is_static_flushable { flush dynamic_run; emit
child } else { dynamic_run.push(child) }).
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 276-280: The assert_eq! on dynamic_attr_slot_cursor vs
dynamic_attr_slots.len() will panic in release builds; replace it with a
non-panicking check: use debug_assert_eq!(dynamic_attr_slot_cursor,
dynamic_attr_slots.len()) for development builds and add a graceful diagnostic
path for release by emitting a compiler-friendly error (e.g., HANDLER.with(|h|
h.struct_span_err(...).emit())) when the lengths differ; update the check around
the assert_eq! invocation (referencing dynamic_attr_slot_cursor,
dynamic_attr_slots.len(), and the assert_eq! call) so that test-time panics
still catch regressions but user builds produce a structured diagnostic instead
of a panic.
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 99-151: Make the shared collector owned only by the transformer:
remove the public visibility on the element_templates field (change pub
element_templates -> element_templates) and preserve/export the
take_element_templates(&self) method as the single drain API; then update any
call sites that currently drain the Rc<RefCell<...>> directly to call
JSXTransformer::take_element_templates() instead (locate usage by symbol
element_templates_collector or direct .borrow_mut().drain(..) calls) so there is
only one canonical place that drains the buffer and no external mutable
aliasing.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs`:
- Around line 27-33: DiagnosticCollector currently panics on any diagnostic in
DiagnosticEmitter::emit, which hides non-fatal warnings; change
DiagnosticCollector to store diagnostics instead of panicking by adding a field
(e.g., diagnostics: Vec<String>) to DiagnosticCollector, update the
DiagnosticEmitter::emit implementation to push db.message().to_string() (or
formatted detail) into that Vec rather than calling panic!, and expose an
accessor (e.g., collected() -> &Vec<String> or into_vec()) so tests can opt-in
to assert on collected diagnostics (e.g., assert!(collected.is_empty()) or
assert_eq! on specific messages).
In `@packages/react/transform/src/lib.rs`:
- Around line 299-315: The helper clone_snapshot_ui_source_map_records currently
always sets snapshot_id = Some(record.snapshot_id) even though the N-API/TS type
made snapshotId optional; add a brief doc comment above
clone_snapshot_ui_source_map_records (and/or on UiSourceMapRecord if
appropriate) stating that snapshot_id is guaranteed to be Some for records
produced via the Snapshot path today and the optionality on the TS side is a
forward-compatibility signal for future ET-originated records; do not change the
current behavior, just document the invariants so callers understand why this
helper always populates 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: 88964a45-9705-4007-a99c-e79ca297ecca
📒 Files selected for processing (10)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_element_template/template_definition.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/transform/index.d.ts
Yradex
force-pushed
the
wt/element-template
branch
from
04e5ed6 to
3cb50c9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/react/transform/crates/swc_plugin_element_template/lib.rs (1)
126-126:⚠️ Potential issue | 🟡 MinorDefault
content_hash: "test"sentinel is a latent collision risk.In release builds, any caller that constructs
JSXTransformer::new*without chainingwith_content_hash(...)produces_et_<filename_hash>_test_<counter>for every template. Uniqueness still holds within one file (filename_hash + counter), but template ids are not stable across content changes to the same file. The primary callers inpackages/react/transform/src/lib.rsLine 551 andnapi.rsdo chainwith_content_hash, so this is latent today. Consider defaulting to the empty string and adding a debug-only warning whencontent_hash.is_empty()on first template emission to surface accidental omissions in new call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` at line 126, The default sentinel content_hash: "test" creates latent ID collisions; change the default for content_hash in JSXTransformer constructors (JSXTransformer::new* and any factory that sets content_hash) to the empty string and ensure callers can override via with_content_hash(...). Additionally, emit a debug-only warning the first time a template is emitted when content_hash.is_empty() (e.g., in the template emission path) to surface missing with_content_hash usage while keeping release behavior stable; reference the content_hash field, the with_content_hash method, and the template id pattern `_et_<filename_hash>_test_<counter>` when making these changes.
🧹 Nitpick comments (5)
packages/react/transform/crates/swc_plugin_element_template/template_definition.rs (2)
217-230: Optional: drop theelement_template_from_jsx_element→_implindirection.
element_template_from_jsx_elementis a thin wrapper that just re-dispatches toelement_template_from_jsx_element_implwith the same arguments. Since the_implfunction isn't recursing through the public entry point (children recurse viaelement_template_from_jsx_children→_impldirectly), the indirection serves no purpose. Consider inlining the public method body or making_implpublic-visible and deleting the wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 217 - 230, The public method element_template_from_jsx_element is a no-op wrapper around element_template_from_jsx_element_impl; remove the indirection by either inlining the impl body into element_template_from_jsx_element and deleting element_template_from_jsx_element_impl, or make element_template_from_jsx_element_impl public (rename it to element_template_from_jsx_element) and delete the wrapper; update any internal callers (e.g., places that call element_template_from_jsx_element_impl directly such as element_template_from_jsx_children) to use the single remaining symbol so there are no duplicate entry points.
258-260: Extractor correctly mirrors__lynx_part_idskip without slot reservation.The extractor's
visit_mut_jsx_elementmethod removes__lynx_part_idin theretain_mutclosure before iterating over remaining attributes, ensuring no slot is reserved. This matches the template-definition behavior at lines 258-260. The contract testshould_skip_lynx_part_id_without_reserving_attr_slotvalidates this invariant. Consider adding a shared constant or documented comment linking both locations to prevent silent desynchronization if future changes are made to attribute handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 258 - 260, The two places handling the "__lynx_part_id" attribute (the extractor's visit_mut_jsx_element and the template-definition logic that skips "__lynx_part_id") can drift out of sync; define a shared constant (e.g. LYNX_PART_ID = "__lynx_part_id") and use that constant in both the visit_mut_jsx_element implementation and the template_definition skip logic, or add a clear comment in both locations referencing the other implementation and the contract test should_skip_lynx_part_id_without_reserving_attr_slot so future edits stay consistent.packages/react/transform/src/lib.rs (1)
342-343: Minor allocation waste:snapshot_ui_source_map_recordsbuffer is unconditionally allocated even when ET backend is active.At Line 342-343 the snapshot UI source map records buffer is created regardless of which backend runs. When
use_element_template_pluginis true, this buffer is never written to andclone_snapshot_ui_source_map_recordsis not called on it (Line 780-784 returnsvec![]). Consider making allocation lazy or scoping it to the snapshot branch to avoid the dead Rc/RefCell allocation on ET-only runs.Also applies to: 780-798
🤖 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 342 - 343, The snapshot_ui_source_map_records Rc<RefCell<Vec<SnapshotCoreUISourceMapRecord>>> is always allocated even when use_element_template_plugin is true and the buffer is never used; move the allocation into the branch that actually builds snapshots (or make it lazy) so it is only created when clone_snapshot_ui_source_map_records will be called, and remove the unconditional creation at snapshot_ui_source_map_records; apply the same scoping/laziness fix for the related logic around clone_snapshot_ui_source_map_records (the code region around where clone_snapshot_ui_source_map_records returns vec![] in the snapshot branch) to avoid dead Rc/RefCell allocations on ET-only runs.packages/react/transform/crates/swc_plugin_element_template/lib.rs (1)
283-292: TODO:cssId/entryNamemetadata elided from ET templates.The TODO at Line 283-284 documents that legacy
cssId/entryNamemetadata has no replacement channel yet. The contract testshould_not_inject_root_css_scope_attrs_for_element_templatepins the current "no metadata" shape, so any future runtime that re-introduces this via a dedicated field will need to update both the generator and that test. Consider tracking this TODO in an issue so the runtime/native contract work isn't lost.Want me to open a follow-up issue for the
cssId/entryNamereplacement channel and link it here?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 283 - 292, The TODO noting the removed legacy cssId/entryName metadata for element templates must be tracked in a follow-up issue so the runtime/native contract work isn't lost; open an issue describing the need for a dedicated replacement channel for cssId/entryName, reference the contract test should_not_inject_root_css_scope_attrs_for_element_template and the ElementTemplateAsset usage (template_id/compiled_template/source_file), then update the TODO comment in lib.rs to include the issue number/link and a short note that implementing the channel requires updating the generator and that test.packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs (1)
131-233: Brittle substring checks for absence of legacy metadata.
!code.contains("options={{")(Line 145, 220) and!code.contains("cssId")/!code.contains("entryName")reject any occurrence in the generated output. These will also reject legitimate occurrences of those identifiers in runtime imports, user identifiers, or comments that the generator might emit in the future — producing a false-negative test failure during unrelated refactors. Consider asserting on a parsed AST node (e.g., inspectingoptionsproperty on the template-creation call) or scoping the substring check to a known code window instead of the whole module output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs` around lines 131 - 233, The tests use global substring checks like !code.contains("options={{"), !code.contains("cssId"), and !code.contains("entryName") which are brittle; update the tests that call first_user_template_json_with_code (used with element_template_config and dynamic_component_element_template_config) to instead inspect the actual template-creation call or the specific nearby code window (e.g., parse the generated JS AST and locate the template creation expression, or extract the function call that creates the template and assert its options object has no cssId/entryName properties) and keep the existing JSON assertions on template["attributesArray"]; this ensures you only assert absence of legacy metadata on the actual template options rather than anywhere in the module output.
🤖 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_element_template/napi.rs`:
- Around line 86-97: Remove the unused reverse conversion implementation
From<CoreElementTemplateTransformerConfig> for JSXTransformerConfig by deleting
the entire impl block that defines fn from(val:
CoreElementTemplateTransformerConfig) -> Self; locate the impl referencing
CoreElementTemplateTransformerConfig and JSXTransformerConfig and remove it so
only the forward conversion (JSXTransformerConfig →
CoreElementTemplateTransformerConfig) remains.
In `@packages/react/transform/index.d.ts`:
- Around line 676-683: The TypeScript type for compiledTemplate is too narrow;
update the Rust N-API struct in
packages/react/transform/crates/swc_plugin_element_template/napi.rs by adding
the attribute #[napi(ts_type = "unknown")] to the compiled_template field so the
generated TS signature for ElementTemplateAsset aligns with serde_json::Value
(handled by element_template_to_json()). Ensure the attribute is placed directly
above the compiled_template field declaration to alter the emitted TS type
without changing the Rust field name or logic.
In `@packages/react/transform/src/lib.rs`:
- Around line 437-488: Detect when the caller explicitly enabled both backends
(options.snapshot and options.element_template are present and evaluate to true)
and emit a visible warning before selecting the precedence logic; specifically,
check the explicit flags you already parse into snapshot_enabled and
element_template_enabled and if both are true call the project-wide warning
mechanism (e.g. HANDLER.emit_warning(...) or push a message into the esbuild
warnings vector used in this module) indicating that elementTemplate takes
precedence and snapshot will be ignored, then proceed with the existing
precedence logic that sets use_element_template_plugin, use_snapshot_plugin,
jsx_backend_enabled, active_jsx_import_source, preserve_jsx, and
enable_ui_source_map.
---
Duplicate comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Line 126: The default sentinel content_hash: "test" creates latent ID
collisions; change the default for content_hash in JSXTransformer constructors
(JSXTransformer::new* and any factory that sets content_hash) to the empty
string and ensure callers can override via with_content_hash(...). Additionally,
emit a debug-only warning the first time a template is emitted when
content_hash.is_empty() (e.g., in the template emission path) to surface missing
with_content_hash usage while keeping release behavior stable; reference the
content_hash field, the with_content_hash method, and the template id pattern
`_et_<filename_hash>_test_<counter>` when making these changes.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 283-292: The TODO noting the removed legacy cssId/entryName
metadata for element templates must be tracked in a follow-up issue so the
runtime/native contract work isn't lost; open an issue describing the need for a
dedicated replacement channel for cssId/entryName, reference the contract test
should_not_inject_root_css_scope_attrs_for_element_template and the
ElementTemplateAsset usage (template_id/compiled_template/source_file), then
update the TODO comment in lib.rs to include the issue number/link and a short
note that implementing the channel requires updating the generator and that
test.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 217-230: The public method element_template_from_jsx_element is a
no-op wrapper around element_template_from_jsx_element_impl; remove the
indirection by either inlining the impl body into
element_template_from_jsx_element and deleting
element_template_from_jsx_element_impl, or make
element_template_from_jsx_element_impl public (rename it to
element_template_from_jsx_element) and delete the wrapper; update any internal
callers (e.g., places that call element_template_from_jsx_element_impl directly
such as element_template_from_jsx_children) to use the single remaining symbol
so there are no duplicate entry points.
- Around line 258-260: The two places handling the "__lynx_part_id" attribute
(the extractor's visit_mut_jsx_element and the template-definition logic that
skips "__lynx_part_id") can drift out of sync; define a shared constant (e.g.
LYNX_PART_ID = "__lynx_part_id") and use that constant in both the
visit_mut_jsx_element implementation and the template_definition skip logic, or
add a clear comment in both locations referencing the other implementation and
the contract test should_skip_lynx_part_id_without_reserving_attr_slot so future
edits stay consistent.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs`:
- Around line 131-233: The tests use global substring checks like
!code.contains("options={{"), !code.contains("cssId"), and
!code.contains("entryName") which are brittle; update the tests that call
first_user_template_json_with_code (used with element_template_config and
dynamic_component_element_template_config) to instead inspect the actual
template-creation call or the specific nearby code window (e.g., parse the
generated JS AST and locate the template creation expression, or extract the
function call that creates the template and assert its options object has no
cssId/entryName properties) and keep the existing JSON assertions on
template["attributesArray"]; this ensures you only assert absence of legacy
metadata on the actual template options rather than anywhere in the module
output.
In `@packages/react/transform/src/lib.rs`:
- Around line 342-343: The snapshot_ui_source_map_records
Rc<RefCell<Vec<SnapshotCoreUISourceMapRecord>>> is always allocated even when
use_element_template_plugin is true and the buffer is never used; move the
allocation into the branch that actually builds snapshots (or make it lazy) so
it is only created when clone_snapshot_ui_source_map_records will be called, and
remove the unconditional creation at snapshot_ui_source_map_records; apply the
same scoping/laziness fix for the related logic around
clone_snapshot_ui_source_map_records (the code region around where
clone_snapshot_ui_source_map_records returns vec![] in the snapshot branch) to
avoid dead Rc/RefCell allocations on ET-only runs.
🪄 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: 3609e4c1-8caf-4941-bdad-63d07239d054
📒 Files selected for processing (10)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/swc-plugin-reactlynx/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react/transform/test/fixture.spec.js
- packages/react/transform/crates/swc_plugin_element_template/extractor.rs
- packages/react/transform/crates/swc_plugin_element_template/attr_name.rs
Yradex
force-pushed
the
wt/element-template
branch
from
3cb50c9 to
69eb8d1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/src/lib.rs (1)
255-277:⚠️ Potential issue | 🟠 MajorET runs currently drop all UI source map metadata.
UiSourceMapRecordonly modelssnapshotId, and both ET branches hardcodeui_source_map_recordstovec![]. That makes the new backend incapable of returning thetemplateId-backed records described by this PR, so ET source mapping/debugging is lost entirely.Also applies to: 780-799
🤖 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 255 - 277, UiSourceMapRecord and TransformNodiffOutput currently drop all UI source map metadata (both ET branches set ui_source_map_records to vec![] and UiSourceMapRecord only models snapshotId), so templateId-backed records are never returned; update UiSourceMapRecord to include an Option<String> for template_id (with appropriate #[napi(js_name = "templateId")]), keep snapshot_id as-is, and then modify the ET branches that populate TransformNodiffOutput.ui_source_map_records to build and return Vec<UiSourceMapRecord> from the template/source-map metadata (mapping templateId/snapshotId, filename, line_number, column_number into UiSourceMapRecord) instead of hardcoding vec![] so the templateId-backed records are preserved.
🤖 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_element_template/lib.rs`:
- Around line 117-144: The development branch currently hardcodes
require('@lynx-js/react/internal') for the runtime_id; update
new_with_element_templates to use the configured runtime package instead by
capturing cfg.runtime_pkg (clone it into a local runtime_pkg before building
JSXTransformer) and use a move closure for Lazy::new that constructs the Expr
using that runtime_pkg (so runtime_id is built from the configured runtime_pkg
rather than the hardcoded string); modify the match arm for
TransformMode::Development in new_with_element_templates (the runtime_id
construction) accordingly so it mirrors how production/test use the configured
runtime package.
---
Outside diff comments:
In `@packages/react/transform/src/lib.rs`:
- Around line 255-277: UiSourceMapRecord and TransformNodiffOutput currently
drop all UI source map metadata (both ET branches set ui_source_map_records to
vec![] and UiSourceMapRecord only models snapshotId), so templateId-backed
records are never returned; update UiSourceMapRecord to include an
Option<String> for template_id (with appropriate #[napi(js_name =
"templateId")]), keep snapshot_id as-is, and then modify the ET branches that
populate TransformNodiffOutput.ui_source_map_records to build and return
Vec<UiSourceMapRecord> from the template/source-map metadata (mapping
templateId/snapshotId, filename, line_number, column_number into
UiSourceMapRecord) instead of hardcoding vec![] so the templateId-backed records
are preserved.
🪄 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: 42044d57-4afa-4d02-ba9e-3afa232b16dd
📒 Files selected for processing (10)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_element_template/extractor.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/src/lib.rs (1)
255-277:⚠️ Potential issue | 🟠 MajorET backend currently drops all
uiSourceMapRecords.
UiSourceMapRecordcan only carrysnapshotId, and both ET branches returnui_source_map_records: vec![]. OnceelementTemplateis active, downstream tooling loses source-map metadata entirely instead of getting a record keyed by the emitted template. Please plumb ET records here as well, with an optionaltemplateIdfield alongsidesnapshotId.Also applies to: 791-815
🤖 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 255 - 277, UiSourceMapRecord currently only carries snapshotId and TransformNodiffOutput always returns ui_source_map_records: vec![], which drops ET source-map metadata; add an optional templateId (e.g. template_id: Option<String> with #[napi(js_name = "templateId")]) to UiSourceMapRecord and populate TransformNodiffOutput.ui_source_map_records from element_templates when element_templates is present (map each ElementTemplateAsset to a UiSourceMapRecord including template_id and/or snapshot_id as available), and apply the same change in the other analogous output/struct definitions referenced in the review (the other block noted in the comment).
♻️ Duplicate comments (1)
packages/react/transform/crates/swc_plugin_element_template/lib.rs (1)
127-135:⚠️ Potential issue | 🟠 MajorDev mode still ignores
cfg.runtime_pkg.The
TransformMode::Developmentarm continues to hardcoderequire('@lynx-js/react/internal')while Production/Test paths honorself.cfg.runtime_pkg(lines 349, 377). The TODO at line 129 acknowledges this. Any consumer that overridesruntimePkgwill silently get divergent runtime targets between dev and prod, which is exactly the kind of footgun that only surfaces at runtime.Consider capturing
cfg.runtime_pkginto a local and using a move closure so the dev branch mirrors prod/test:♻️ Sketch
- runtime_id: match mode { - TransformMode::Development => { - // We should find a way to use `cfg.runtime_pkg` - Lazy::new(|| quote!("require('@lynx-js/react/internal')" as Expr)) - } - TransformMode::Production | TransformMode::Test => { - Lazy::new(|| Expr::Ident(private_ident!("ReactLynx"))) - } - }, + runtime_id: match mode { + TransformMode::Development => { + let runtime_pkg = cfg.runtime_pkg.clone(); + Lazy::new(Box::new(move || { + quote!( + "require($pkg)" as Expr, + pkg: Expr = Expr::Lit(Lit::Str(runtime_pkg.into())), + ) + })) + } + TransformMode::Production | TransformMode::Test => { + Lazy::new(|| Expr::Ident(private_ident!("ReactLynx"))) + } + },Note:
Lazy::newaccepts only a named function pointer or non-capturing closure by default — you'll needLazy<Expr, Box<dyn FnOnce() -> Expr>>(or equivalent) if you want to capture. If that's undesirable, move the call construction out of theLazyentirely and build it on first use from a storedruntime_pkgfield.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 127 - 135, The Development arm of runtime_id hardcodes require('@lynx-js/react/internal') and ignores self.cfg.runtime_pkg; capture self.cfg.runtime_pkg into a local (e.g. let runtime_pkg = self.cfg.runtime_pkg.clone()) and make the Development branch mirror Production/Test by constructing the Expr from that runtime_pkg rather than a hardcoded string. Either use a move closure that captures runtime_pkg when calling Lazy::new (adjust the Lazy type to accept a boxed capturing closure if needed) or build the Expr on first use outside of Lazy and store/return it so runtime_pkg is honored; update the TransformMode::Development branch (the runtime_id construction) and ensure the produced Expr/Ident creation mirrors the logic used in the Production/Test arms (where Expr::Ident(private_ident!("ReactLynx")) and runtime_pkg is used).
🤖 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_element_template/lib.rs`:
- Around line 191-215: In visit_mut_jsx_element, after emitting the diagnostic
for tag_str == "page" || tag_str == "component" (the
HANDLER.struct_span_err(...).emit() call), stop further processing by returning
immediately so the element isn't run through the counter increment,
ElementTemplateExtractor/element_template_from_jsx_element, lowering,
template-definition JSON creation, or pushed into self.element_templates; locate
the error emit inside visit_mut_jsx_element and add an early return right after
the emit() to prevent downstream poisoning of element_templates and potential
panics in ElementTemplateExtractor.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 329-355: The text-child optimization incorrectly runs when a
spread prop is present; update the guard so it only optimizes when there is no
explicit text prop AND no spread in the opening attributes: compute a has_spread
(iterate n.opening.attrs and detect the spread variant of JSXAttrOrSpread) and
change the condition from if is_text_tag && !has_explicit_text_attr to if
is_text_tag && !has_explicit_text_attr && !has_spread; this ensures the code
path that builds the "text" attribute descriptor
(attribute_descriptors.push(...) and text_child_optimized) is skipped whenever a
spread could inject a runtime text prop.
---
Outside diff comments:
In `@packages/react/transform/src/lib.rs`:
- Around line 255-277: UiSourceMapRecord currently only carries snapshotId and
TransformNodiffOutput always returns ui_source_map_records: vec![], which drops
ET source-map metadata; add an optional templateId (e.g. template_id:
Option<String> with #[napi(js_name = "templateId")]) to UiSourceMapRecord and
populate TransformNodiffOutput.ui_source_map_records from element_templates when
element_templates is present (map each ElementTemplateAsset to a
UiSourceMapRecord including template_id and/or snapshot_id as available), and
apply the same change in the other analogous output/struct definitions
referenced in the review (the other block noted in the comment).
---
Duplicate comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 127-135: The Development arm of runtime_id hardcodes
require('@lynx-js/react/internal') and ignores self.cfg.runtime_pkg; capture
self.cfg.runtime_pkg into a local (e.g. let runtime_pkg =
self.cfg.runtime_pkg.clone()) and make the Development branch mirror
Production/Test by constructing the Expr from that runtime_pkg rather than a
hardcoded string. Either use a move closure that captures runtime_pkg when
calling Lazy::new (adjust the Lazy type to accept a boxed capturing closure if
needed) or build the Expr on first use outside of Lazy and store/return it so
runtime_pkg is honored; update the TransformMode::Development branch (the
runtime_id construction) and ensure the produced Expr/Ident creation mirrors the
logic used in the Production/Test arms (where
Expr::Ident(private_ident!("ReactLynx")) and runtime_pkg is used).
🪄 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: e3eacf84-b663-4cdb-9985-1a97a8cbe56b
📒 Files selected for processing (10)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/transform/crates/swc_plugin_element_template/extractor.rs
Yradex
force-pushed
the
wt/element-template
branch
2 times, most recently
from
3fbd1de to
c7c874e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/react/transform/crates/swc_plugin_element_template/template_definition.rs (1)
322-357:⚠️ Potential issue | 🟠 MajorText-child optimization still triggers when a spread prop is present.
The guard at Line 337 only checks
!has_explicit_text_attr. For<text {...props}>hello</text>:
- The spread descriptor is emitted at the slot index reserved by the extractor.
- Then this block unconditionally pushes a static
text="hello"descriptor afterwards.- At runtime,
props.textcan no longer override because the statictextdescriptor wins.This shifts observable behavior versus the source JSX. Add a
has_spread_attrcheck to disable the optimization whenever a spread could inject atextprop.🛡️ Proposed fix
let has_explicit_text_attr = n.opening.attrs.iter().any(|attr| { matches!( attr, JSXAttrOrSpread::JSXAttr(attr) if template_attribute_descriptor_key(&attr.name) == "text" ) }); + let has_spread_attr = n + .opening + .attrs + .iter() + .any(|attr| matches!(attr, JSXAttrOrSpread::SpreadElement(_))); let mut text_child_optimized = false; - if is_text_tag && !has_explicit_text_attr { + if is_text_tag && !has_explicit_text_attr && !has_spread_attr {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 322 - 357, The text-child optimization currently ignores spread attributes and can incorrectly emit a static "text" descriptor; update the guard in the block that sets text_child_optimized (the is_text_tag / !has_explicit_text_attr check) to also check for a spread by computing a has_spread_attr boolean from n.opening.attrs (detect JSXAttrOrSpread::SpreadElement or equivalent) and require !has_spread_attr before emitting the static attribute via element_template_static_attribute_descriptor and element_template_string_expr; this prevents the static text descriptor from overriding a runtime-injected props.text.packages/react/transform/crates/swc_plugin_element_template/lib.rs (1)
213-237:⚠️ Potential issue | 🟡 Minor
<page>/<component>diagnostic doesn't stop further processing.After
HANDLER.emit()at Line 229, control falls through to Line 239 and the element is still run throughtemplate_counter++,ElementTemplateExtractor,element_template_from_jsx_element, and theassert_eq!at Line 298-302 — and ifself.element_templatesisSome, a garbageElementTemplateAssetfor the unsupported tag lands in the shared collector. The compile will ultimately fail because of the diagnostic, but:
- Subsequent invariant asserts (e.g. Line 298-302) could panic first on some
<page>shapes with dynamic attrs, surfacing to users as a compiler crash instead of the clean "<page /> is not supported" error.- The collector sees a template entry for a tag that should have been rejected.
should_report_page_element_as_unsupportedhappens to use a static<page>, which avoids the assert paths today. Adding an earlyreturn;afteremit()would harden this without changing the observable diagnostic.🛡️ Proposed fix
if tag_str == "page" || tag_str == "component" { HANDLER.with(|handler| { handler .struct_span_err( node.opening.name.span(), &format!("<{tag_str} /> is not supported"), ) .emit() }); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 213 - 237, The diagnostic for unsupported tags inside visit_mut_jsx_element (triggered via HANDLER.with(...) .struct_span_err(...).emit()) currently continues processing and can lead to later panics or stray ElementTemplateAsset entries; after emitting the error for tag_str == "page" || tag_str == "component" add an immediate return to stop further work on this JSXElement (i.e., return from visit_mut_jsx_element right after the emit) so the element is not passed into the template_counter/ElementTemplateExtractor/element_template_from_jsx_element flow and nothing is pushed into self.element_templates for rejected tags.
🤖 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/src/lib.rs`:
- Around line 804-817: In the Err arm that returns TransformNodiffOutput,
preserve the ET invariant by draining the element_templates_collector and
setting element_templates to Some(...) when use_element_template_plugin is true
(instead of None); mirror the success-path behavior (use the same logic that the
Ok branch uses to clone/collect element_templates and to compute
ui_source_map_records via
clone_snapshot_ui_source_map_records(&snapshot_ui_source_map_records,
&options.filename)), so downstream code can distinguish "ET ran" vs "ET
disabled" and partial templates are not dropped.
---
Duplicate comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 213-237: The diagnostic for unsupported tags inside
visit_mut_jsx_element (triggered via HANDLER.with(...)
.struct_span_err(...).emit()) currently continues processing and can lead to
later panics or stray ElementTemplateAsset entries; after emitting the error for
tag_str == "page" || tag_str == "component" add an immediate return to stop
further work on this JSXElement (i.e., return from visit_mut_jsx_element right
after the emit) so the element is not passed into the
template_counter/ElementTemplateExtractor/element_template_from_jsx_element flow
and nothing is pushed into self.element_templates for rejected tags.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 322-357: The text-child optimization currently ignores spread
attributes and can incorrectly emit a static "text" descriptor; update the guard
in the block that sets text_child_optimized (the is_text_tag /
!has_explicit_text_attr check) to also check for a spread by computing a
has_spread_attr boolean from n.opening.attrs (detect
JSXAttrOrSpread::SpreadElement or equivalent) and require !has_spread_attr
before emitting the static attribute via
element_template_static_attribute_descriptor and element_template_string_expr;
this prevents the static text descriptor from overriding a runtime-injected
props.text.
🪄 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: 5e13d043-cf54-4ce9-bbdf-26b8c56e05e3
📒 Files selected for processing (11)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react/transform/swc-plugin-reactlynx/index.d.ts
- packages/react/transform/test/fixture.spec.js
- packages/react/transform/index.d.ts
- packages/react/transform/crates/swc_plugin_element_template/attr_name.rs
Yradex
force-pushed
the
wt/element-template
branch
from
c7c874e to
8c9e519
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react/transform/crates/swc_plugin_element_template/napi.rs (1)
87-98:⚠️ Potential issue | 🟡 MinorRemove the unused reverse
Fromimpl.The
impl From<CoreElementTemplateTransformerConfig> for JSXTransformerConfigat lines 87-98 has no callers anywhere in the codebase. Only the forward direction (JSXTransformerConfig → CoreElementTemplateTransformerConfig) is used when constructing transformers. This reverse impl should be removed to eliminate dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs` around lines 87 - 98, Remove the unused reverse implementation impl From<CoreElementTemplateTransformerConfig> for JSXTransformerConfig: delete the entire impl block that constructs JSXTransformerConfig from CoreElementTemplateTransformerConfig (the From implementation for those two types), leaving only the existing forward conversion (JSXTransformerConfig → CoreElementTemplateTransformerConfig) in place; this eliminates dead code since there are no callers of that reverse conversion.
🧹 Nitpick comments (5)
packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs (1)
50-122:first_attribute_slots_lenhand-rolled parser has subtle corner cases — consider documenting scope.The character-level parser at Line 50-122 treats template literals (`) as opaque string regions, so commas inside
${ ... }interpolations are ignored. For top-level array element counting that is actually correct (a whole template literal counts as one slot), and regex lookahead avoids full JS tokenization. Two minor nits:
code.find(prefix)will matchattributeSlots={[anywhere in the output, including unlikely occurrences inside a string literal preamble — OK for ET-generated tests, but a comment documenting the assumption ("we search ET-generated output, not arbitrary JS") would guard future reuse.- JSX expression containers like
slot={a, b}(parenthesized comma expressions) aren't expected in ET output but would be miscounted. Since ET doesn't emit those, this is purely defensive.No change required; flagging so the limits are visible if this helper is reused elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs` around lines 50 - 122, The helper first_attribute_slots_len makes simplifying assumptions (it searches for the prefix "attributeSlots={[", treats template literals as opaque, and does not handle comma expressions inside JSX expression containers), so add a concise doc comment above the first_attribute_slots_len function explaining these constraints: that the function expects ET-generated output (not arbitrary JS), that template literal interpolations `${...}` are treated as opaque and commas inside them are ignored, and that parenthesized/comma expressions like slot={a, b} are not supported; mention this is intentional and defensive to avoid future misuse if the helper is reused elsewhere.packages/react/transform/src/lib.rs (1)
528-580: Minor: deadSnapshotJSXTransformer/ElementTemplateTransformerconstruction in the disabled arms.When
use_snapshot_pluginis false (resp.use_element_template_pluginis false), theelsebranches at Line 545-554 and Line 571-580 still construct a full transformer instance just to wrap it inOptional::new(..., false). SinceOptionalwithfalsenever runs the inner pass, this is wasted allocation/setup per transform call.Consider using
Optional::new(..., false)with a cheap placeholder pass (e.g.,noop_pass()orvisit_mut_pass(noop())) in the disabled branch, or restructuring so the transformer is only built when enabled.🤖 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 528 - 580, The else-branches currently construct full SnapshotJSXTransformer and ElementTemplateTransformer instances even when disabled; replace those heavy constructions with a cheap no-op pass so we don't allocate/setup unused transformers. Concretely, in the disabled arms for SnapshotJSXTransformer and ElementTemplateTransformer, pass a lightweight no-op transformer to Optional::new (e.g., use visit_mut_pass(noop_pass()) or visit_mut_pass(noop()) or an existing no-op pass helper) instead of SnapshotJSXTransformer::new(...) / ElementTemplateTransformer::new(...), keeping the false flag; update imports if needed and leave the enabled branches unchanged.packages/react/transform/crates/swc_plugin_element_template/napi.rs (1)
154-173: Wrapper forwards 4 coreVisitMutoverrides — considerDeref/DerefMutor a macro for future-proofing.The manual delegation covers today's core overrides (
visit_mut_program,visit_mut_jsx_element,visit_mut_module_items,visit_mut_module). If the coreJSXTransformer<C>grows a new override (e.g.,visit_mut_jsx_fragment), the NAPI wrapper will silently fall back to the default no-opVisitMutimpl and the transformer will stop running on that node class without any compile-time hint.A
Deref/DerefMuttoCoreElementTemplateTransformer<C>(or a small forwarding macro shared withlib.rs) would avoid the drift risk at zero runtime cost. Deferring is fine under chill review, but worth tracking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs` around lines 154 - 173, The wrapper JSXTransformer<C> currently forwards specific VisitMut methods (visit_mut_program, visit_mut_jsx_element, visit_mut_module_items, visit_mut_module) which risks silent drift if VisitMut gains new methods; change JSXTransformer<C> to delegate via Deref/DerefMut to the inner CoreElementTemplateTransformer<C> (or introduce a small forwarding macro shared with lib.rs) so all VisitMut methods are forwarded automatically; update impl<C> VisitMut for JSXTransformer<C> to either remove manual forwards and instead implement Deref/DerefMut to CoreElementTemplateTransformer<C>, or replace the hand-written methods with the macro to ensure future overrides (e.g., visit_mut_jsx_fragment) are covered without manual edits.packages/react/transform/crates/swc_plugin_element_template/lib.rs (1)
168-206: Directive validation runs twice on module-level leading comments.
visit_mut_modulecallsself.validate_directives(n.span)at Line 352, then loops over every top-level item at Line 353-356 and callsself.validate_directives(item.span)again. Block comments attached as leading on the first top-level item get visited by both passes (the module's low position and the first item's low position often coincide), so a single bad@jsxCSSIdcan produce duplicate diagnostics for the same source location.For chill review, not blocking — but dropping the
n.spancall (or skipping the first-item span) would keep diagnostics clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 168 - 206, The module-level directive validation is run twice because visit_mut_module calls validate_directives(n.span) and then validate_directives(item.span) for the first top-level item, causing duplicate diagnostics; update visit_mut_module to skip calling validate_directives(item.span) when item.span.lo == n.span.lo (or alternatively remove the initial validate_directives(n.span) call) so block comments attached to the first item aren’t validated twice; refer to the functions/locals validate_directives, visit_mut_module, n.span, and item.span when making the change.packages/react/transform/crates/swc_plugin_element_template/extractor.rs (1)
27-35: Minor: avoid allocatingStringper name.
NO_FLATTEN_ATTRIBUTESis aHashSet<String>over static attribute names, so each static name is heap-allocated once at init and every membership check at Line 222/237 does aString::from(...)-style comparison againstStringkeys. Since the set is read-only and all entries are&'static str, aHashSet<&'static str>(or just a[&str; N]searched linearly — the set is 5 entries) avoids the allocation and simplifies lookups.♻️ Suggested refactor
-static NO_FLATTEN_ATTRIBUTES: Lazy<HashSet<String>> = Lazy::new(|| { - HashSet::from([ - "name".to_string(), - "clip-radius".to_string(), - "overlap".to_string(), - "exposure-scene".to_string(), - "exposure-id".to_string(), - ]) -}); +const NO_FLATTEN_ATTRIBUTES: &[&str] = &[ + "name", + "clip-radius", + "overlap", + "exposure-scene", + "exposure-id", +];Update the membership checks in
ensure_flatten_attrto useNO_FLATTEN_ATTRIBUTES.iter().any(|a| *a == name)(or.contains(&name.as_str())if kept asHashSet<&'static str>).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs` around lines 27 - 35, NO_FLATTEN_ATTRIBUTES currently stores heap-allocated String entries and callers like ensure_flatten_attr perform comparisons involving String allocations; change NO_FLATTEN_ATTRIBUTES to hold &'static str (e.g., HashSet<&'static str> or a static [&'static str; 5]) and update membership checks in ensure_flatten_attr to use contains(&name.as_str()) or NO_FLATTEN_ATTRIBUTES.iter().any(|a| *a == name) (or a simple linear search over the static array) so no per-check String allocation occurs.
🤖 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_element_template/extractor.rs`:
- Around line 385-458: The dynamic-key branch replaces *n with
slot_placeholder_node(slot_index, false) but then falls through to
ensure_flatten_attr and later visitor logic, risking unintended mutations;
update the block guarded by jsx_has_dynamic_key(n) && self.parent_element
(around the replacement of *n) to mirror the other branches by either
immediately calling n.visit_mut_with(self) followed by return, or simply
returning right after assigning the slot placeholder so the subsequent
ensure_flatten_attr/attr handling and child visitation are skipped; reference
functions/fields: jsx_has_dynamic_key, slot_placeholder_node,
ensure_flatten_attr, visit_mut_with, is_slot_placeholder, and the parent_element
flag when making the change.
---
Duplicate comments:
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 87-98: Remove the unused reverse implementation impl
From<CoreElementTemplateTransformerConfig> for JSXTransformerConfig: delete the
entire impl block that constructs JSXTransformerConfig from
CoreElementTemplateTransformerConfig (the From implementation for those two
types), leaving only the existing forward conversion (JSXTransformerConfig →
CoreElementTemplateTransformerConfig) in place; this eliminates dead code since
there are no callers of that reverse conversion.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs`:
- Around line 27-35: NO_FLATTEN_ATTRIBUTES currently stores heap-allocated
String entries and callers like ensure_flatten_attr perform comparisons
involving String allocations; change NO_FLATTEN_ATTRIBUTES to hold &'static str
(e.g., HashSet<&'static str> or a static [&'static str; 5]) and update
membership checks in ensure_flatten_attr to use contains(&name.as_str()) or
NO_FLATTEN_ATTRIBUTES.iter().any(|a| *a == name) (or a simple linear search over
the static array) so no per-check String allocation occurs.
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 168-206: The module-level directive validation is run twice
because visit_mut_module calls validate_directives(n.span) and then
validate_directives(item.span) for the first top-level item, causing duplicate
diagnostics; update visit_mut_module to skip calling
validate_directives(item.span) when item.span.lo == n.span.lo (or alternatively
remove the initial validate_directives(n.span) call) so block comments attached
to the first item aren’t validated twice; refer to the functions/locals
validate_directives, visit_mut_module, n.span, and item.span when making the
change.
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 154-173: The wrapper JSXTransformer<C> currently forwards specific
VisitMut methods (visit_mut_program, visit_mut_jsx_element,
visit_mut_module_items, visit_mut_module) which risks silent drift if VisitMut
gains new methods; change JSXTransformer<C> to delegate via Deref/DerefMut to
the inner CoreElementTemplateTransformer<C> (or introduce a small forwarding
macro shared with lib.rs) so all VisitMut methods are forwarded automatically;
update impl<C> VisitMut for JSXTransformer<C> to either remove manual forwards
and instead implement Deref/DerefMut to CoreElementTemplateTransformer<C>, or
replace the hand-written methods with the macro to ensure future overrides
(e.g., visit_mut_jsx_fragment) are covered without manual edits.
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs`:
- Around line 50-122: The helper first_attribute_slots_len makes simplifying
assumptions (it searches for the prefix "attributeSlots={[", treats template
literals as opaque, and does not handle comma expressions inside JSX expression
containers), so add a concise doc comment above the first_attribute_slots_len
function explaining these constraints: that the function expects ET-generated
output (not arbitrary JS), that template literal interpolations `${...}` are
treated as opaque and commas inside them are ignored, and that
parenthesized/comma expressions like slot={a, b} are not supported; mention this
is intentional and defensive to avoid future misuse if the helper is reused
elsewhere.
In `@packages/react/transform/src/lib.rs`:
- Around line 528-580: The else-branches currently construct full
SnapshotJSXTransformer and ElementTemplateTransformer instances even when
disabled; replace those heavy constructions with a cheap no-op pass so we don't
allocate/setup unused transformers. Concretely, in the disabled arms for
SnapshotJSXTransformer and ElementTemplateTransformer, pass a lightweight no-op
transformer to Optional::new (e.g., use visit_mut_pass(noop_pass()) or
visit_mut_pass(noop()) or an existing no-op pass helper) instead of
SnapshotJSXTransformer::new(...) / ElementTemplateTransformer::new(...), keeping
the false flag; update imports if needed and leave the enabled branches
unchanged.
🪄 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: 100d44b4-e617-49f8-a40b-93595575cb90
📒 Files selected for processing (11)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_element_template/template_definition.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react/transform/test/fixture.spec.js
- packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs
- packages/react/transform/crates/swc_plugin_element_template/attr_name.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react/transform/crates/swc_plugin_element_template/extractor.rs (1)
385-396:⚠️ Potential issue | 🟡 MinorMinor — dynamic-key branch still falls through to attr/child processing on the placeholder.
After
*n = slot_placeholder_node(slot_index, false), execution continues toensure_flatten_attr, the attr retention loop,push_dynamic_jsx_attr_or_spread, andvisit_mut_children_with(self). In practice the placeholder carries no attrs/children so these are no-ops, but unlike the list branch (Lines 380-382) and the custom-element branch (Lines 455-456) this branch does notreturnafter the replacement. Ifslot_placeholder_nodeever grows attrs/children, oris_slot_placeholderdetection changes, this path would start silently mutating the placeholder and double-count against slot counters.A one-line
return;(or mirroring the other branches withn.visit_mut_with(self); return;) after Line 395 would make the three replacement branches symmetrical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs` around lines 385 - 396, In the jsx_is_custom/jsx_has_dynamic_key branch in extractor.rs (the block that calls n.visit_mut_with(self.dynamic_part_visitor), creates Expr::JSXElement, pushes DynamicElementPart::Slot, and sets *n = slot_placeholder_node(slot_index, false)), add an early exit (e.g., return; or mirror other branches by calling n.visit_mut_with(self); return;) immediately after replacing n with slot_placeholder_node so the placeholder is not further processed by ensure_flatten_attr, the attr retention loop, push_dynamic_jsx_attr_or_spread, or visit_mut_children_with(self); this keeps behavior consistent with the list and custom-element branches and prevents accidental mutation or double-counting of slot placeholders.
🧹 Nitpick comments (4)
packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs (1)
27-33: Nit —DiagnosticCollectorpanics during emission and can double-panic on unwind.
panic!insideemit(&mut self, db: &mut DiagnosticBuilder<'_>)will start unwinding while the caller still owns theDiagnosticBuilder. When the builder is dropped during unwind, some SWC versions re-invoke the emitter, which panics again and aborts the test with a less readable failure. If this ever fires, the real diagnostic message gets buried.For a more robust "no diagnostics allowed" guard, consider pushing into a
Vec<String>captured byRefCelland asserting emptiness aftertransform_fixturereturns, or calldb.cancel()before panicking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs` around lines 27 - 33, The current DiagnosticCollector implementation panics inside DiagnosticEmitter::emit which can double-panic during unwinding; instead, change emit to avoid panicking: record the diagnostic message into a shared collection (e.g., push db.message() into a RefCell<Vec<String>> owned by the test) or call db.cancel() before panicking so the builder won't re-invoke the emitter; then after transform_fixture returns assert the Vec<String> is empty (or fail the test using the collected messages) — update references in the test to use DiagnosticCollector, DiagnosticEmitter::emit, DiagnosticBuilder, and transform_fixture accordingly.packages/react/transform/crates/swc_plugin_element_template/napi.rs (1)
61-72: Nit —Defaultinconsistency between napi config and its consumer inlib.rs.
JSXTransformerConfig::default()setsfilename: Default::default()(empty string), but every call site inswc-plugin-reactlynx/src/lib.rs(per the relevant code snippet at Lines 155-185) overrides filename explicitly fromoptions.filename. Meanwhile the top-level NAPIsrc/lib.rsdoes the same viaElementTemplateTransformerConfig { filename: options.filename.clone(), ..Default::default() }.So the default filename is never the intended value in practice. Not a bug today, but if a future caller calls
JSXTransformerConfig::default()without overriding filename, they'll get an empty filename that flows intocalc_hash(&cfg.filename)and produces a deterministic-but-meaningless filename hash. Consider either makingfilenamenon-Default(remove the field from theDefaultimpl via a builder) or adding a debug-only assert at construction time that the filename is non-empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs` around lines 61 - 72, JSXTransformerConfig::default() sets filename to an empty default which can flow into calc_hash(&cfg.filename) and produce meaningless hashes; fix by ensuring filename is never empty at construction: either remove filename from the Default impl and require callers to set it (e.g., use ElementTemplateTransformerConfig { filename: ..., ..Default::default() } pattern) or add a debug-only assert/validation in JSXTransformerConfig::default() or in the constructor for JSXTransformerConfig that panics/logs if filename.is_empty(); update references in code that rely on Default (e.g., places that call JSXTransformerConfig::default() or create ElementTemplateTransformerConfig) to supply a real filename.packages/react/transform/crates/swc_plugin_element_template/template_definition.rs (1)
154-215: Nit —JSXSpreadChildhandling diverges between extractor and template definition.Extractor
visit_mut_jsx_element_childs(extractor.rsLines 326-328) doesunreachable!("JSXSpreadChild is not supported yet"), while this file allocates anelementSlotforJSXSpreadChild(Lines 206-210). In practice the extractor would panic before this branch is ever reached, so the discrepancy is latent, but if support forJSXSpreadChildis ever added to the extractor, this branch needs to stay in sync with however the extractor allocates the slot index (or theelement_slot_indexhere will drift from the extractor'selement_slot_counter).Either add an
unreachable!mirror here or leave a comment explaining the expected pairing so a future contributor doesn't have to cross-reference both files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs` around lines 154 - 215, The JSXSpreadChild branch in element_template_from_jsx_children currently allocates an element slot but the extractor's visit_mut_jsx_element_childs treats JSXSpreadChild as unreachable; update the JSXSpreadChild handling in element_template_from_jsx_children to mirror the extractor by either replacing the slot allocation with an unreachable!("JSXSpreadChild is not supported yet") call or add a clear comment referencing visit_mut_jsx_element_childs in extractor.rs and explaining that if support is added there, element_slot_index allocation here must be updated in lockstep (ensure references to element_template_from_jsx_children, visit_mut_jsx_element_childs, and element_slot_index/element_slot_counter are included).packages/react/transform/src/lib.rs (1)
256-264:UiSourceMapRecord.snapshot_idis alwaysSome(...)on the Rust side — consider whetherOptionis needed.
clone_snapshot_ui_source_map_recordsunconditionally setssnapshot_id: Some(record.snapshot_id)(Line 327), and when ET is active the whole vector is replaced withvec rather than emitting records withsnapshot_id: None. So in practice consumers will never observesnapshotId === undefinedfrom this path today.The optional field is fine as a forward-compat hook for future
templateId-tagged records (per PR objectives), but it's worth a// TODO: populate templateId when ET lowering emits source map recordscomment near the struct so the intent stays documented, rather than leaving the field permanentlySome(...).Also applies to: 314-330
🤖 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 256 - 264, UiSourceMapRecord currently exposes snapshot_id as Option<String> but in practice clone_snapshot_ui_source_map_records always sets snapshot_id = Some(...) and the ET path replaces the vector rather than emitting None, so add a concise TODO comment on the UiSourceMapRecord definition (and the duplicate definition/usage around the other occurrence) stating that snapshot_id is retained as Option for forward-compat (future templateId-tagged records) and that templateId population should occur when ET lowering emits source map records; reference UiSourceMapRecord and clone_snapshot_ui_source_map_records in the comment for future implementers so they know where to update behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react/transform/crates/swc_plugin_element_template/extractor.rs`:
- Around line 385-396: In the jsx_is_custom/jsx_has_dynamic_key branch in
extractor.rs (the block that calls n.visit_mut_with(self.dynamic_part_visitor),
creates Expr::JSXElement, pushes DynamicElementPart::Slot, and sets *n =
slot_placeholder_node(slot_index, false)), add an early exit (e.g., return; or
mirror other branches by calling n.visit_mut_with(self); return;) immediately
after replacing n with slot_placeholder_node so the placeholder is not further
processed by ensure_flatten_attr, the attr retention loop,
push_dynamic_jsx_attr_or_spread, or visit_mut_children_with(self); this keeps
behavior consistent with the list and custom-element branches and prevents
accidental mutation or double-counting of slot placeholders.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/napi.rs`:
- Around line 61-72: JSXTransformerConfig::default() sets filename to an empty
default which can flow into calc_hash(&cfg.filename) and produce meaningless
hashes; fix by ensuring filename is never empty at construction: either remove
filename from the Default impl and require callers to set it (e.g., use
ElementTemplateTransformerConfig { filename: ..., ..Default::default() }
pattern) or add a debug-only assert/validation in
JSXTransformerConfig::default() or in the constructor for JSXTransformerConfig
that panics/logs if filename.is_empty(); update references in code that rely on
Default (e.g., places that call JSXTransformerConfig::default() or create
ElementTemplateTransformerConfig) to supply a real filename.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 154-215: The JSXSpreadChild branch in
element_template_from_jsx_children currently allocates an element slot but the
extractor's visit_mut_jsx_element_childs treats JSXSpreadChild as unreachable;
update the JSXSpreadChild handling in element_template_from_jsx_children to
mirror the extractor by either replacing the slot allocation with an
unreachable!("JSXSpreadChild is not supported yet") call or add a clear comment
referencing visit_mut_jsx_element_childs in extractor.rs and explaining that if
support is added there, element_slot_index allocation here must be updated in
lockstep (ensure references to element_template_from_jsx_children,
visit_mut_jsx_element_childs, and element_slot_index/element_slot_counter are
included).
In
`@packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs`:
- Around line 27-33: The current DiagnosticCollector implementation panics
inside DiagnosticEmitter::emit which can double-panic during unwinding; instead,
change emit to avoid panicking: record the diagnostic message into a shared
collection (e.g., push db.message() into a RefCell<Vec<String>> owned by the
test) or call db.cancel() before panicking so the builder won't re-invoke the
emitter; then after transform_fixture returns assert the Vec<String> is empty
(or fail the test using the collected messages) — update references in the test
to use DiagnosticCollector, DiagnosticEmitter::emit, DiagnosticBuilder, and
transform_fixture accordingly.
In `@packages/react/transform/src/lib.rs`:
- Around line 256-264: UiSourceMapRecord currently exposes snapshot_id as
Option<String> but in practice clone_snapshot_ui_source_map_records always sets
snapshot_id = Some(...) and the ET path replaces the vector rather than emitting
None, so add a concise TODO comment on the UiSourceMapRecord definition (and the
duplicate definition/usage around the other occurrence) stating that snapshot_id
is retained as Option for forward-compat (future templateId-tagged records) and
that templateId population should occur when ET lowering emits source map
records; reference UiSourceMapRecord and clone_snapshot_ui_source_map_records in
the comment for future implementers so they know where to update behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5245572a-2fa5-4a05-abb4-63c9f686e573
📒 Files selected for processing (11)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_element_template/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react/transform/test/fixture.spec.js
- packages/react/transform/crates/swc_plugin_element_template/attr_name.rs
- packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs
Yradex
force-pushed
the
wt/element-template
branch
from
8c9e519 to
8b33d6c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_element_template/attr_name.rs`:
- Around line 48-53: In the impl From<Str> for AttrName (the From::from
function) replace the current Atom conversion that uses
name.value.to_string_lossy().into_owned() with the documented pattern using
name.value.as_str().unwrap_or("").to_string() so it matches the conversion used
in template_definition.rs and uses the public Atom API; update the conversion
expression inside AttrName::from accordingly.
In
`@packages/react/transform/crates/swc_plugin_element_template/template_definition.rs`:
- Around line 194-210: The two match arms in
element_template_from_jsx_element_impl that handle
JSXExprContainer(JSXExpr::Expr(_)) and JSXElementChild::JSXSpreadChild currently
allocate a new index from the local element_slot_index counter and push
element_template_element_slot(idx); replace those arms with
unreachable!("dynamic child should have been lowered to a slot placeholder by
ElementTemplateExtractor") so we fail loudly if a dynamic child wasn't replaced
by ElementTemplateExtractor (the real slot index should be read via
slot_placeholder_index applied by the extractor, not allocated here). Ensure you
update the exact arms that currently use element_slot_index and
element_template_element_slot and keep unchanged the
JSXExprContainer(JSXExpr::JSXEmptyExpr(_)) arm.
🪄 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: a3c29348-886c-4168-a959-123ba3bf7874
📒 Files selected for processing (11)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ 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 (3)
- packages/react/transform/swc-plugin-reactlynx/index.d.ts
- packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs
- packages/react/transform/crates/swc_plugin_element_template/napi.rs
Yradex
force-pushed
the
wt/element-template
branch
from
8b33d6c to
82183f1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react/transform/crates/swc_plugin_element_template/attr_name.rs (1)
79-90:get_event_type_and_nameallocates twoStrings per call but its return value is never consumed.Both call sites in this crate (Line 40 and Line 73) only use
get_event_type_and_name(...).is_some(); the(event_type, event_name)tuple — including the asymmetric"…Event"suffix logic for non-captureprefixes vs. the raw prefix forcapture-*— is built up only to be discarded. That makes every classification allocate and then throw away twoStrings on the hot path of every JSX attribute, and it also leaves the suffix policy untested since no caller observes it.If the tuple is genuinely intended for future callers, consider exposing it and adding coverage. Otherwise, simplify to a boolean matcher:
♻️ Proposed refactor — reduce to a predicate
-impl From<String> for AttrName { - fn from(name: String) -> Self { +impl From<String> for AttrName { + fn from(name: String) -> Self { if name.strip_prefix("data-").is_some() { @@ - } else if get_event_type_and_name(name.as_str()).is_some() { + } else if is_event_key(name.as_str()) { AttrName::Event } else { AttrName::Attr } } } @@ match name_str { "ref" => AttrName::WorkletRef, "gesture" => AttrName::Gesture, - _ if get_event_type_and_name(name_str).is_some() => AttrName::WorkletEvent, + _ if is_event_key(name_str) => AttrName::WorkletEvent, _ => AttrName::Attr, } } } -fn get_event_type_and_name(props_key: &str) -> Option<(String, String)> { - if let Some(captures) = EVENT_KEY_RE.captures(props_key) { - let event_type = if captures.get(1).unwrap().as_str().contains("capture") { - captures.get(1).unwrap().as_str().to_string() - } else { - format!("{}Event", captures.get(1).unwrap().as_str()) - }; - let event_name = captures.get(2).unwrap().as_str().to_string(); - return Some((event_type, event_name)); - } - None -} +fn is_event_key(props_key: &str) -> bool { + EVENT_KEY_RE.is_match(props_key) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs` around lines 79 - 90, get_event_type_and_name currently allocates two Strings every call even though both call sites only check presence via get_event_type_and_name(...).is_some(); change this to a predicate to avoid allocations: replace get_event_type_and_name(props_key: &str) -> Option<(String, String)> with a boolean matcher (e.g., is_event(props_key: &str) -> bool) that simply tests EVENT_KEY_RE against props_key (or returns captures.is_some()), remove the `"…Event"` suffix logic and String construction, and update the two call sites that now do get_event_type_and_name(...).is_some() to call the new is_event(...) predicate instead.
🤖 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_element_template/extractor.rs`:
- Around line 447-459: The branch only converts custom JSX into a slot when
self.parent_element is true, leaving top-level custom components unhandled and
later serialized as invalid templates; update the logic in extractor.rs so that
when encountering a JSX element with a non-string tag at the root (when
self.parent_element is false) you perform the same handling as inside the
parent_element branch: call self.next_children_slot_index(), push
DynamicElementPart::Slot (wrapping Expr::JSXElement(Box::new(n.take()))) onto
self.dynamic_children, replace *n with slot_placeholder_node(slot_index, true),
and call n.visit_mut_with(self); alternatively emit a clear diagnostic if you
prefer not to slot root custom components—use the same symbols
(dynamic_part_visitor, parent_element, next_children_slot_index,
dynamic_children, DynamicElementPart::Slot, slot_placeholder_node,
visit_mut_with) to locate and implement the fix.
In `@packages/react/transform/src/lib.rs`:
- Around line 256-264: The UiSourceMapRecord currently only models snapshotId
and code paths hard-code ui_source_map_records to [] when
use_element_template_plugin (elementTemplate) is true, dropping UI-to-source
mapping; update UiSourceMapRecord to include the full set of metadata fields
required by the backend (preserve ui_source_map, filename, line_number,
column_number, snapshot_id and any other template-scoped ID fields the backend
expects) and change the return paths that set ui_source_map_records to [] to
instead populate it from the original source mapping data (don't special-case
use_element_template_plugin to drop mappings); locate and fix the
constructors/serializers that create UiSourceMapRecord and the places that build
ui_source_map_records so they pass through template-scoped IDs and snapshot_id
when elementTemplate/use_element_template_plugin is enabled.
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/attr_name.rs`:
- Around line 79-90: get_event_type_and_name currently allocates two Strings
every call even though both call sites only check presence via
get_event_type_and_name(...).is_some(); change this to a predicate to avoid
allocations: replace get_event_type_and_name(props_key: &str) -> Option<(String,
String)> with a boolean matcher (e.g., is_event(props_key: &str) -> bool) that
simply tests EVENT_KEY_RE against props_key (or returns captures.is_some()),
remove the `"…Event"` suffix logic and String construction, and update the two
call sites that now do get_event_type_and_name(...).is_some() to call the new
is_event(...) predicate instead.
🪄 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: f9e315a4-4b5b-4f64-8ad0-175f067c33ba
📒 Files selected for processing (11)
packages/react/transform/__test__/fixture.spec.jspackages/react/transform/crates/swc_plugin_element_template/attr_name.rspackages/react/transform/crates/swc_plugin_element_template/extractor.rspackages/react/transform/crates/swc_plugin_element_template/lib.rspackages/react/transform/crates/swc_plugin_element_template/napi.rspackages/react/transform/crates/swc_plugin_element_template/template_definition.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/react/transform/index.d.tspackages/react/transform/src/lib.rspackages/react/transform/swc-plugin-reactlynx/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react/transform/crates/swc_plugin_element_template/napi.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/transform/crates/swc_plugin_element_template/lib.rs
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
Bug Fixes
Overview
elementTemplateconfig surface.Key Points
swc_plugin_element_templatecrate owns ET-specific extraction, Template Definition generation, runtime JSX lowering, asset export, and NAPI config conversion. Snapshot no longer needs to preserve ET-only slot bookkeeping or output branches.attributeSlotspayload share one compile-time source of slot indices.wrapper, so user JSX remains distinguishable from compiler-private transport nodes.Generated Output Contract
For input like:
ET now emits a Template Definition asset that describes static host structure and dynamic slots, not runtime patch behavior:
{ "kind": "element", "type": "view", "attributesArray": [ { "kind": "attribute", "key": "id", "binding": "slot", "attrSlotIndex": 0 }, { "kind": "attribute", "key": "class", "binding": "static", "value": "card" }, { "kind": "spread", "binding": "slot", "attrSlotIndex": 1 } ], "children": [ { "kind": "element", "type": "text", "attributesArray": [{ "kind": "attribute", "key": "text", "binding": "static", "value": "Hello" }], "children": [] }, { "kind": "elementSlot", "type": "slot", "elementSlotIndex": 0 } ] }The lowered runtime JSX passes only the dynamic values, using the same compile-time indices:
The transform output also separates Snapshot and ET metadata instead of reusing Snapshot fields:
The important invariants are:
attrSlotIndexandelementSlotIndexare independent numbering spaces.Checklist