feat(react): support element template events#2589
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: e759547 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-template attribute-slot plans, attribute adapters, reserved handle allocation, background raw-slot buffering and lazy preparation, manager lookup for event-values, queued runtime event dispatch with lifecycle controls, conditioned hydration flush, native wiring/teardown, and extensive tests/fixtures. ChangesElement Template Event Attribute System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Yradex
force-pushed
the
slice/element-template/08
branch
from
12a0d13 to
c2dd2ad
Compare
Merging this PR will improve performance by 18.17%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
React Example#8155 Bundle Size — 236.51KiB (0%).e759547(current) vs ae67fd1 main#8152(baseline) Bundle metrics
|
| Current #8155 |
Baseline #8152 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
197 |
197 |
| Duplicate Modules |
80 |
80 |
| Duplicate Code |
44.87% |
44.87% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8155 |
Baseline #8152 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
90.75KiB |
90.75KiB |
Bundle analysis report Branch Yradex:slice/element-template/08 Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9730 Bundle Size — 901.38KiB (0%).e759547(current) vs ae67fd1 main#9727(baseline) Bundle metrics
|
| Current #9730 |
Baseline #9727 |
|
|---|---|---|
| Initial JS |
45.06KiB |
45.06KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229 |
229 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.22% |
27.22% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9730 |
Baseline #9727 |
|
|---|---|---|
| JS |
497.1KiB |
497.1KiB |
| Other |
402.06KiB |
402.06KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:slice/element-template/08 Project dashboard
Generated by RelativeCI Documentation Report issue
React External#1270 Bundle Size — 693.04KiB (0%).e759547(current) vs ae67fd1 main#1267(baseline) Bundle metrics
|
| Current #1270 |
Baseline #1267 |
|
|---|---|---|
| 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:slice/element-template/08 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example with Element Template#422 Bundle Size — 199.83KiB (+1.03%).e759547(current) vs ae67fd1 main#419(baseline) Bundle metrics
|
| Current #422 |
Baseline #419 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
26.31% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
85(+6.25%) |
80 |
 Duplicate Modules |
25(+8.7%) |
23 |
| Duplicate Code |
39.96%(-0.84%) |
40.3% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #422 |
Baseline #419 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
54.08KiB (+3.93%) |
52.03KiB |
Bundle analysis report Branch Yradex:slice/element-template/08 Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1288 Bundle Size — 207.46KiB (0%).e759547(current) vs ae67fd1 main#1285(baseline) Bundle metrics
|
| Current #1288 |
Baseline #1285 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
192 |
192 |
| Duplicate Modules |
77 |
77 |
| Duplicate Code |
44.38% |
44.38% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #1288 |
Baseline #1285 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
96.23KiB |
96.23KiB |
Bundle analysis report Branch Yradex:slice/element-template/08 Project dashboard
Generated by RelativeCI Documentation Report issue
Yradex
force-pushed
the
slice/element-template/08
branch
from
c2dd2ad to
ffb9f66
Compare
Yradex
requested review from
HuJean,
colinaaa,
gaoachao,
hzy,
luhc228 and
upupming
as code owners
![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: ffb9f66900
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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/crates/swc_plugin_element_template/lib.rs (1)
159-210:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winLine 161 breaks element-template in Development mode — the internal entry does not export required symbols.
The change on line 161 routes dev-mode
runtime_idthroughinternal_runtime_pkg(&cfg.runtime_pkg), which resolves to@lynx-js/react/element-template/internal. However, the element-template transformer generates code that callstransformRefonruntime_id(seelowering.rsline 62:$runtime_id.transformRef($value)). The element-template internal entry does not exporttransformRef— only the main@lynx-js/react/internalentry does. This causes runtime failures when dev builds execute.Additionally, the dev-mode initializers for
runtime_idandinternal_runtime_idare now identical (both resolve viainternal_runtime_pkg), creating unnecessary duplication.Proposed fix
runtime_id: match mode { TransformMode::Development => { - let runtime_pkg = internal_runtime_pkg(&cfg.runtime_pkg); + let runtime_pkg = cfg.runtime_pkg.clone(); lazy_runtime_id(move || { Expr::Call(CallExpr {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 159 - 210, The dev-mode branch incorrectly uses internal_runtime_pkg for runtime_id (making runtime_id point to the element-template internal entry which lacks transformRef) and duplicates both initializers; change the Development arm for runtime_id to use the public runtime package helper (use runtime_pkg(&cfg.runtime_pkg) or the equivalent non-internal helper) so runtime_id resolves to the main package that exports transformRef, while keeping internal_runtime_id using internal_runtime_pkg(&cfg.runtime_pkg); ensure the lazy_runtime_id closures differ (runtime_id -> require(main runtime pkg), internal_runtime_id -> require(internal runtime pkg)).
🧹 Nitpick comments (2)
packages/react/transform/crates/swc_plugin_element_template/lib.rs (2)
159-210: ⚡ Quick winDeduplicate the dev-mode
Lazyconstructions.The two
TransformMode::Developmentarms (lines 160–180 and 186–206) now build the samerequire(...)CallExpragainst the sameruntime_pkg. Even if the line-161 change is intentional, this is verbatim copy/paste; once they diverge for real reasons, they'll likely diverge by mistake. A small helper closure or functionfn require_runtime_expr(pkg: String) -> RuntimeIdInitializerwould collapse both arms to one line each.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 159 - 210, The two TransformMode::Development arms for runtime_id and internal_runtime_id duplicate the same lazy require(...) CallExpr construction; refactor by extracting a helper (e.g., fn require_runtime_expr(pkg: String) -> RuntimeIdInitializer or a closure) that builds the Expr::Call(CallExpr) using internal_runtime_pkg(&cfg.runtime_pkg) and reuse it in both lazy_runtime_id(...) calls so runtime_id and internal_runtime_id each call the helper instead of duplicating the require construction.
58-79: 💤 Low valueSimplify
internal_runtime_pkgto check only/internalsuffix.Backslash and extension variants in
INTERNAL_SUFFIXESare unreachable. Runtime module specifiers always use/regardless of OS, and no caller adds file extensions—all values are pure module specifiers like@lynx-js/react. Only the/internalsuffix is ever used in practice.Suggested simplification
fn internal_runtime_pkg(runtime_pkg: &str) -> String { - const INTERNAL_SUFFIXES: &[&str] = &[ - "/internal", - "/internal.ts", - "/internal.js", - "/internal.mjs", - "/internal.cjs", - "\\internal", - "\\internal.ts", - "\\internal.js", - "\\internal.mjs", - "\\internal.cjs", - ]; - if INTERNAL_SUFFIXES - .iter() - .any(|suffix| runtime_pkg.ends_with(suffix)) - { + if runtime_pkg.ends_with("/internal") { runtime_pkg.to_string() } else { format!("{runtime_pkg}/internal") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs` around lines 58 - 79, internal_runtime_pkg currently checks many unreachable suffix variants (backslashes and extensions); simplify it to only test for the "/internal" suffix. Replace the INTERNAL_SUFFIXES array and its .iter().any(...) call in the internal_runtime_pkg function with a single ends_with("/internal") check on runtime_pkg and return runtime_pkg.to_string() when true, otherwise return format!("{runtime_pkg}/internal"); keep the function name internal_runtime_pkg and behavior otherwise unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/src/element-template/prop-adapters/event.ts`:
- Around line 56-58: The pendingEvents queue (used when queuePendingEvents is
true) is unbounded and can grow indefinitely; add a MAX_PENDING_EVENTS constant
and enforce it in the push path for pendingEvents (e.g., before or after pushing
the new [eventValue, data], drop the oldest entries using pendingEvents.shift()
until length <= MAX_PENDING_EVENTS) so the queue never exceeds the cap; update
any related logic in the same module that reads pendingEvents to expect this
bounded behavior.
---
Outside diff comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 159-210: The dev-mode branch incorrectly uses internal_runtime_pkg
for runtime_id (making runtime_id point to the element-template internal entry
which lacks transformRef) and duplicates both initializers; change the
Development arm for runtime_id to use the public runtime package helper (use
runtime_pkg(&cfg.runtime_pkg) or the equivalent non-internal helper) so
runtime_id resolves to the main package that exports transformRef, while keeping
internal_runtime_id using internal_runtime_pkg(&cfg.runtime_pkg); ensure the
lazy_runtime_id closures differ (runtime_id -> require(main runtime pkg),
internal_runtime_id -> require(internal runtime pkg)).
---
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lib.rs`:
- Around line 159-210: The two TransformMode::Development arms for runtime_id
and internal_runtime_id duplicate the same lazy require(...) CallExpr
construction; refactor by extracting a helper (e.g., fn
require_runtime_expr(pkg: String) -> RuntimeIdInitializer or a closure) that
builds the Expr::Call(CallExpr) using internal_runtime_pkg(&cfg.runtime_pkg) and
reuse it in both lazy_runtime_id(...) calls so runtime_id and
internal_runtime_id each call the helper instead of duplicating the require
construction.
- Around line 58-79: internal_runtime_pkg currently checks many unreachable
suffix variants (backslashes and extensions); simplify it to only test for the
"/internal" suffix. Replace the INTERNAL_SUFFIXES array and its .iter().any(...)
call in the internal_runtime_pkg function with a single ends_with("/internal")
check on runtime_pkg and return runtime_pkg.to_string() when true, otherwise
return format!("{runtime_pkg}/internal"); keep the function name
internal_runtime_pkg and behavior otherwise 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: bf0a7efc-c77b-4f94-917b-dd00b7534d1d
⛔ Files ignored due to path filters (4)
packages/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_interpolated_text_with_siblings_js.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!**/*.snap
📒 Files selected for processing (31)
.changeset/empty-et-event-implementation.mdpackages/react/runtime/__test__/element-template/fixtures/background/event/conditional-direct-event/index.tsxpackages/react/runtime/__test__/element-template/fixtures/background/event/direct-event/index.tsxpackages/react/runtime/__test__/element-template/fixtures/render/react-example/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/react-example/output.txtpackages/react/runtime/__test__/element-template/fixtures/render/react-example/papi.txtpackages/react/runtime/__test__/element-template/native/index.test.tspackages/react/runtime/__test__/element-template/runtime/background/event/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/background/hydrate.test.tspackages/react/runtime/__test__/element-template/runtime/background/instance.test.tspackages/react/runtime/__test__/element-template/runtime/background/update/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/hydration/hydration-listener.test.tspackages/react/runtime/__test__/element-template/runtime/prop-adapters/event.test.tspackages/react/runtime/__test__/element-template/runtime/render/render-opcodes-into-element-template.et.test.tsxpackages/react/runtime/__test__/element-template/runtime/template/element-template-attr-slot-plan.test.tspackages/react/runtime/src/element-template/background/attr-slots.tspackages/react/runtime/src/element-template/background/destroy.tspackages/react/runtime/src/element-template/background/hydrate.tspackages/react/runtime/src/element-template/background/hydration-listener.tspackages/react/runtime/src/element-template/background/instance.tspackages/react/runtime/src/element-template/index.tspackages/react/runtime/src/element-template/internal.tspackages/react/runtime/src/element-template/native/index.tspackages/react/runtime/src/element-template/prop-adapters/event.tspackages/react/runtime/src/element-template/runtime/render/render-opcodes.tspackages/react/runtime/src/element-template/runtime/template/attr-slot-plan.tspackages/react/runtime/src/element-template/runtime/template/handle.tspackages/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/tests/element_template_contract.rspackages/webpack/react-webpack-plugin/test/background-loader.test.ts
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/element-template/index.ts
Yradex
force-pushed
the
slice/element-template/08
branch
from
ffb9f66 to
a666c27
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/transform/crates/swc_plugin_element_template/lowering.rs (2)
57-61: ⚡ Quick winRemove redundant conditional branch for
AttrName::Ref.The
AttrName::Refcase (line 57-58) now returnsvaluedirectly, which is identical to theelsebranch (lines 59-60). Since the behavior is the same, the entireelse if let AttrName::Refcheck can be removed.♻️ Proposed simplification
} => { let slot_value = if let AttrName::Event = attr_name { if target == TransformTarget::LEPUS { quote!("1" as Expr) } else { value } - } else if let AttrName::Ref = attr_name { - value } else { value };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/transform/crates/swc_plugin_element_template/lowering.rs` around lines 57 - 61, Remove the redundant conditional branch checking AttrName::Ref that returns the same value as the final else: locate the block containing "else if let AttrName::Ref" and the following "else" that both return value, delete the entire "else if let AttrName::Ref { ... }" branch and leave the existing else branch to return value; ensure references to AttrName::Ref are not required elsewhere in that expression (adjust surrounding control flow if needed) so only one branch returns value.
27-27: 💤 Low valueConsider removing unused
_runtime_idparameter if not needed for API stability.The
runtime_idparameter was renamed to_runtime_idto indicate it's unused after removing thetransformRefcall. If this parameter is not required for maintaining a consistent API signature or future use, it could be removed entirely.However, if other call sites or API contracts depend on this signature, keeping it is appropriate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/transform/crates/swc_plugin_element_template/lowering.rs` at line 27, The _runtime_id field is unused; remove the `_runtime_id: Expr` field declaration from the struct in lowering.rs and then update all related code (constructors, struct initializers, pattern matches, and any places referencing `runtime_id` or `_runtime_id`) to no longer pass or destructure that field; if external API compatibility must be preserved instead, leave the field but ensure it's consistently named `_runtime_id` and unused to avoid warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react/transform/crates/swc_plugin_element_template/lowering.rs`:
- Around line 57-61: Remove the redundant conditional branch checking
AttrName::Ref that returns the same value as the final else: locate the block
containing "else if let AttrName::Ref" and the following "else" that both return
value, delete the entire "else if let AttrName::Ref { ... }" branch and leave
the existing else branch to return value; ensure references to AttrName::Ref are
not required elsewhere in that expression (adjust surrounding control flow if
needed) so only one branch returns value.
- Line 27: The _runtime_id field is unused; remove the `_runtime_id: Expr` field
declaration from the struct in lowering.rs and then update all related code
(constructors, struct initializers, pattern matches, and any places referencing
`runtime_id` or `_runtime_id`) to no longer pass or destructure that field; if
external API compatibility must be preserved instead, leave the field but ensure
it's consistently named `_runtime_id` and unused to avoid warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6ebde11-20c9-40b4-87d5-d4eada8fad4c
⛔ Files ignored due to path filters (6)
packages/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_interpolated_text_with_siblings_js.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_keep_code_and_template_attribute_slots_in_sync_for_spread.snapis excluded by!**/*.snap
📒 Files selected for processing (32)
.changeset/empty-et-event-implementation.mdpackages/react/runtime/__test__/element-template/fixtures/background/event/conditional-direct-event/index.tsxpackages/react/runtime/__test__/element-template/fixtures/background/event/direct-event/index.tsxpackages/react/runtime/__test__/element-template/fixtures/render/react-example/index.js.txtpackages/react/runtime/__test__/element-template/fixtures/render/react-example/output.txtpackages/react/runtime/__test__/element-template/fixtures/render/react-example/papi.txtpackages/react/runtime/__test__/element-template/native/index.test.tspackages/react/runtime/__test__/element-template/runtime/background/event/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/background/hydrate.test.tspackages/react/runtime/__test__/element-template/runtime/background/instance.test.tspackages/react/runtime/__test__/element-template/runtime/background/update/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/hydration/hydration-listener.test.tspackages/react/runtime/__test__/element-template/runtime/prop-adapters/event.test.tspackages/react/runtime/__test__/element-template/runtime/render/render-opcodes-into-element-template.et.test.tsxpackages/react/runtime/src/element-template/background/attr-slots.tspackages/react/runtime/src/element-template/background/destroy.tspackages/react/runtime/src/element-template/background/hydrate.tspackages/react/runtime/src/element-template/background/hydration-listener.tspackages/react/runtime/src/element-template/background/instance.tspackages/react/runtime/src/element-template/index.tspackages/react/runtime/src/element-template/internal.tspackages/react/runtime/src/element-template/native/index.tspackages/react/runtime/src/element-template/prop-adapters/event.tspackages/react/runtime/src/element-template/runtime/render/render-opcodes.tspackages/react/runtime/src/element-template/runtime/template/attr-slot-plan.tspackages/react/runtime/src/element-template/runtime/template/handle.tspackages/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/tests/element_template.rspackages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rspackages/webpack/react-webpack-plugin/test/background-loader.test.ts
💤 Files with no reviewable changes (1)
- packages/react/runtime/src/element-template/index.ts
✅ Files skipped from review due to trivial changes (8)
- packages/react/transform/crates/swc_plugin_element_template/napi.rs
- packages/react/runtime/test/element-template/fixtures/render/react-example/papi.txt
- packages/webpack/react-webpack-plugin/test/background-loader.test.ts
- packages/react/runtime/src/element-template/background/destroy.ts
- .changeset/empty-et-event-implementation.md
- packages/react/transform/crates/swc_plugin_element_template/tests/element_template.rs
- packages/react/runtime/test/element-template/fixtures/render/react-example/output.txt
- packages/react/runtime/test/element-template/fixtures/background/event/conditional-direct-event/index.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/react/runtime/src/element-template/native/index.ts
- packages/react/runtime/src/element-template/internal.ts
- packages/react/runtime/test/element-template/runtime/background/update/compiled-fixtures.test.tsx
- packages/react/runtime/src/element-template/background/hydration-listener.ts
- packages/react/runtime/src/element-template/background/attr-slots.ts
- packages/react/runtime/test/element-template/runtime/prop-adapters/event.test.ts
- packages/react/runtime/src/element-template/runtime/render/render-opcodes.ts
- packages/react/runtime/src/element-template/runtime/template/handle.ts
- packages/react/runtime/test/element-template/native/index.test.ts
- packages/react/runtime/src/element-template/background/hydrate.ts
- packages/react/runtime/test/element-template/runtime/hydration/hydration-listener.test.ts
- packages/react/runtime/test/element-template/runtime/background/event/compiled-fixtures.test.tsx
- packages/react/runtime/src/element-template/runtime/template/attr-slot-plan.ts
- packages/react/transform/crates/swc_plugin_element_template/lib.rs
- packages/react/runtime/test/element-template/fixtures/background/event/direct-event/index.tsx
- packages/react/runtime/test/element-template/runtime/render/render-opcodes-into-element-template.et.test.tsx
- packages/react/transform/crates/swc_plugin_element_template/tests/element_template_contract.rs
- packages/react/runtime/test/element-template/runtime/background/hydrate.test.ts
- packages/react/runtime/src/element-template/prop-adapters/event.ts
- packages/react/runtime/src/element-template/background/instance.ts
Yradex
force-pushed
the
slice/element-template/08
branch
from
a666c27 to
1da2f26
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/react/runtime/src/element-template/background/attr-slots.ts (1)
32-44: 💤 Low valueMain logic is correct; consider optional defensive checks.
The attribute plan iteration and adapter application logic is sound. The double normalization and cloning at lines 32-35 is necessary to allow safe mutation when
attrPlanexists.Optionally, for robustness in case of malformed plans (lines 37-39), you could add bounds/type validation:
- Validate
attrSlotIndexis withinrawSlots.length- Validate adapter is callable
Given the experimental ET flag and internal usage where plans are transform-generated, these checks are not critical but would make the code more defensive.
♻️ Optional: add defensive validation
for (let planIndex = 0; planIndex < attrPlan.length; planIndex += 2) { const attrSlotIndex = attrPlan[planIndex] as number; const adapter = attrPlan[planIndex + 1] as EtAttrAdapter; + // Optional defensive checks: + // if (attrSlotIndex < 0 || attrSlotIndex >= rawSlots.length) { + // throw new RangeError(`Invalid attr slot index: ${attrSlotIndex}`); + // } + // if (typeof adapter !== 'function') { + // throw new TypeError(`Invalid adapter at plan index ${planIndex + 1}`); + // } const rawValue = rawSlots[attrSlotIndex]; preparedSlots[attrSlotIndex] = adapter(handleId, attrSlotIndex, rawValue); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/src/element-template/background/attr-slots.ts` around lines 32 - 44, Add defensive validation inside the attrPlan iteration: before calling adapter(handleId, attrSlotIndex, rawValue) in the loop that references attrPlan, check that attrSlotIndex is a finite integer within [0, rawSlots.length) and that adapter is a callable (function) matching EtAttrAdapter; if either check fails, skip that plan entry (leave preparedSlots[attrSlotIndex] unchanged) or optionally continue to next entry. Place these checks around the existing adapter invocation in the same loop so malformed plans won't throw when calling adapter or indexing rawSlots.packages/react/runtime/src/element-template/prop-adapters/event.ts (2)
50-56: 💤 Low valueClarify the unused
_componentIdparameter.The
_componentIdparameter is prefixed with underscore but never used. If this parameter is required for API compatibility or future extension, consider adding a brief comment explaining why it's present.📝 Suggested clarification
+// componentId parameter reserved for future component-scoped event routing export function publicComponentEvent( _componentId: string, eventValue: string, data: unknown, ): void { publishEvent(eventValue, data); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/src/element-template/prop-adapters/event.ts` around lines 50 - 56, The parameter _componentId in the publicComponentEvent function is unused which is confusing; add a brief inline comment next to the _componentId parameter (or above the function) stating that it is intentionally unused for API compatibility/future use, referencing the function name publicComponentEvent and the publishEvent call so reviewers know this was considered and left intentionally blank.
58-67: Event re-entrancy behavior during flush.Line 59 disables queuing before processing pending events. If an event handler invoked during the flush loop calls
publishEvent, that new event will either dispatch immediately or be silently dropped (depending on whether a handler exists). This prevents infinite loops but could lead to lost events if handlers trigger cascading events. This appears intentional, but worth noting if the behavior is not documented elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/runtime/src/element-template/prop-adapters/event.ts` around lines 58 - 67, flushPendingEvents currently disables queuing (queuePendingEvents = false) before iterating, so any publishEvent called from a handler may be dropped or dispatched immediately; change the flush logic so re-entrant publishEvent calls are enqueued and not lost: inside flushPendingEvents, process pendingEvents in a loop that continues until pendingEvents is empty, re-enabling queuing (set queuePendingEvents = true) while dispatching handlers so publishEvent will push new events onto pendingEvents, then after draining the current batch set queuePendingEvents = false and repeat until pendingEvents is empty; reference function flushPendingEvents and variables queuePendingEvents, pendingEvents, publishEvent, and dispatchEvent when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/src/element-template/background/attr-slots.ts`:
- Around line 9-20: The function normalizeAttributeSlots currently casts
rawSlots to SerializableValue[] without validating entries; add a runtime type
guard inside normalizeAttributeSlots to verify each non-undefined rawSlot is a
valid SerializableValue (using an existing isSerializableValue helper if
available, or a small runtime check) before returning or mutating
normalizedSlots, and if any entry fails validation either coerce/replace it or
throw/log a clear error indicating the offending slot index; update references
to rawSlots, rawSlot, and normalizedSlots in the function to use the guard so
the final return is safe without an unchecked "as SerializableValue[]" cast.
---
Nitpick comments:
In `@packages/react/runtime/src/element-template/background/attr-slots.ts`:
- Around line 32-44: Add defensive validation inside the attrPlan iteration:
before calling adapter(handleId, attrSlotIndex, rawValue) in the loop that
references attrPlan, check that attrSlotIndex is a finite integer within [0,
rawSlots.length) and that adapter is a callable (function) matching
EtAttrAdapter; if either check fails, skip that plan entry (leave
preparedSlots[attrSlotIndex] unchanged) or optionally continue to next entry.
Place these checks around the existing adapter invocation in the same loop so
malformed plans won't throw when calling adapter or indexing rawSlots.
In `@packages/react/runtime/src/element-template/prop-adapters/event.ts`:
- Around line 50-56: The parameter _componentId in the publicComponentEvent
function is unused which is confusing; add a brief inline comment next to the
_componentId parameter (or above the function) stating that it is intentionally
unused for API compatibility/future use, referencing the function name
publicComponentEvent and the publishEvent call so reviewers know this was
considered and left intentionally blank.
- Around line 58-67: flushPendingEvents currently disables queuing
(queuePendingEvents = false) before iterating, so any publishEvent called from a
handler may be dropped or dispatched immediately; change the flush logic so
re-entrant publishEvent calls are enqueued and not lost: inside
flushPendingEvents, process pendingEvents in a loop that continues until
pendingEvents is empty, re-enabling queuing (set queuePendingEvents = true)
while dispatching handlers so publishEvent will push new events onto
pendingEvents, then after draining the current batch set queuePendingEvents =
false and repeat until pendingEvents is empty; reference function
flushPendingEvents and variables queuePendingEvents, pendingEvents,
publishEvent, and dispatchEvent when making the change.
🪄 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: 388735b8-2d7e-446d-9c45-394d408957e1
📒 Files selected for processing (14)
.changeset/empty-et-event-implementation.mdpackages/react/runtime/__test__/element-template/native/index.test.tspackages/react/runtime/__test__/element-template/runtime/background/event/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/background/hydrate.test.tspackages/react/runtime/__test__/element-template/runtime/background/instance.test.tspackages/react/runtime/__test__/element-template/runtime/background/update/compiled-fixtures.test.tsxpackages/react/runtime/__test__/element-template/runtime/hydration/hydration-listener.test.tspackages/react/runtime/__test__/element-template/runtime/prop-adapters/event.test.tspackages/react/runtime/src/element-template/background/attr-slots.tspackages/react/runtime/src/element-template/background/destroy.tspackages/react/runtime/src/element-template/background/instance.tspackages/react/runtime/src/element-template/background/manager.tspackages/react/runtime/src/element-template/native/index.tspackages/react/runtime/src/element-template/prop-adapters/event.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/empty-et-event-implementation.md
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/react/runtime/src/element-template/background/destroy.ts
- packages/react/runtime/test/element-template/native/index.test.ts
- packages/react/runtime/test/element-template/runtime/background/hydrate.test.ts
- packages/react/runtime/test/element-template/runtime/hydration/hydration-listener.test.ts
- packages/react/runtime/test/element-template/runtime/background/update/compiled-fixtures.test.tsx
- packages/react/runtime/test/element-template/runtime/background/event/compiled-fixtures.test.tsx
- packages/react/runtime/src/element-template/native/index.ts
- packages/react/runtime/test/element-template/runtime/prop-adapters/event.test.ts
- packages/react/runtime/src/element-template/background/instance.ts
- packages/react/runtime/test/element-template/runtime/background/instance.test.ts
Yradex
force-pushed
the
slice/element-template/08
branch
2 times, most recently
from
2049e2d to
2e32d7c
Compare
HuJean
reviewed
May 13, 2026
HuJean
reviewed
May 13, 2026
Yradex
force-pushed
the
slice/element-template/08
branch
from
2e32d7c to
e759547
Compare
HuJean
approved these changes
May 14, 2026
gaoachao
approved these changes
May 14, 2026
upupming
approved these changes
May 14, 2026
This was referenced May 14, 2026
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
Overview
This PR adds the Element Template event implementation across transform output, runtime attribute-slot handling, background hydration/update behavior, and native bridge wiring. It keeps the work behind the existing experimental Element Template path while making direct event attributes flow through the ET runtime instead of being treated as ordinary static attributes.
Key Points
Checklist
Verification
git diff --check github/main..stack/element-templatepnpm --dir packages/react/runtime run test:etcargo test --test element_template --test element_template_contract