fix: native tag default event handlers spread#3036
Conversation
🦋 Changeset detectedLatest commit: 8f61663 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a change-set file and a test fixture for a spread-on-native-tag scenario. Introduces a helper 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/runtime-tags/src/translator/visitors/function.ts:
- Around line 177-184: The hasSpreadAttributeAfter function is off-by-one and
unsafe: change the loop to start after the current attribute (use attr.key + 1)
and add a guard validating attr.key is a number before using it; e.g., if typeof
attr.key !== "number" return false (or handle appropriately), then iterate for
(let i = attr.key + 1; i < attrs.length; i++) checking attrs[i].type ===
"MarkoSpreadAttribute".
- Around line 108-109: The predicate that detects event handler attributes is
too restrictive: change the regex used on markoRoot.node.name in the conditional
that also calls hasSpreadAttributeAfter(markoRoot) from /^on[A-Z-]/ to a
case-insensitive pattern (e.g., /^on[A-Z\-]/i) so it matches lowercase handlers
like "onmouseout" while preserving the original intent; update the regex in the
same conditional inside the function in function.ts where markoRoot.node.name is
tested.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (4)
.changeset/ready-jars-switch.mdpackages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/template.markopackages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/test.tspackages/runtime-tags/src/translator/visitors/function.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-tags/src/translator/visitors/function.ts (2)
packages/compiler/src/babel-types/generator/patch.js (1)
attr(207-207)packages/runtime-tags/src/translator/index.ts (2)
MarkoAttribute(94-96)MarkoTag(86-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test: node@20
- GitHub Check: test: node@22
- GitHub Check: test: node@24
🔇 Additional comments (2)
packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/template.marko (1)
1-2: Test fixture appropriately demonstrates the fix.The template correctly sets up the scenario where an event handler attribute (
onClick) is followed by a spread operator (...rest), which is exactly the case this PR addresses.packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/test.ts (1)
1-5: The test adequately validates the fix through snapshot comparison.The snapshot mechanism captures all critical aspects of the fix:
- Event handler registration despite trailing spread (visible in
EventAttributes:#button/0with click handler)- Spread attributes applied correctly (shown as
rest: {}in snapshot)- Handler execution and resulting mutations (button textContent becomes "clicked")
The test's steps array
[{}, click]performs the initial render followed by a click action, and the mutation tracker logs the resulting DOM changes for snapshot validation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3036 +/- ##
=======================================
Coverage 89.16% 89.16%
=======================================
Files 375 375
Lines 47047 47057 +10
Branches 4030 4034 +4
=======================================
+ Hits 41950 41960 +10
Misses 5049 5049
Partials 48 48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DylanPiercey
force-pushed
the
native-tag-default-event-handlers-spread
branch
from
9e65b08 to
8f61663
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime-tags/src/translator/visitors/function.ts (1)
177-184: LGTM! Helper function correctly identifies trailing spreads.The implementation correctly iterates through subsequent attributes to detect
MarkoSpreadAttributenodes. The logic is sound and handles edge cases appropriately (e.g., when the attribute is last in the list).Optional: Consider adding a type guard for the index cast.
The
attr.key as numberassertion on line 179 bypasses TypeScript's type checking. While this is safe in Babel's traversal API (wherekeyis the numeric index for array elements), adding a runtime check or type guard would make the code more defensive:♻️ Optional: Add runtime validation
function hasSpreadAttributeAfter(attr: t.NodePath<t.MarkoAttribute>) { const attrs = (attr.parent as t.MarkoTag).attributes; - for (let i = (attr.key as number) + 1; i < attrs.length; i++) { + const index = attr.key; + if (typeof index !== 'number') return false; + for (let i = index + 1; i < attrs.length; i++) { if (attrs[i].type === "MarkoSpreadAttribute") return true; } return false; }However, if this casting pattern is standard in the Marko compiler codebase, feel free to keep it as-is for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (4)
.changeset/ready-jars-switch.mdpackages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/template.markopackages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/test.tspackages/runtime-tags/src/translator/visitors/function.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/ready-jars-switch.md
- packages/runtime-tags/src/tests/fixtures/spread-trailing-native-tag-default-function/test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-tags/src/translator/visitors/function.ts (1)
packages/runtime-tags/src/translator/index.ts (2)
MarkoAttribute(94-96)MarkoTag(86-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test: node@22
- GitHub Check: test: node@20
- GitHub Check: test: node@24
🔇 Additional comments (2)
packages/runtime-tags/src/__tests__/fixtures/spread-trailing-native-tag-default-function/template.marko (1)
1-2: LGTM! Test fixture appropriately demonstrates the fix.This test case properly exercises the scenario where a default event handler precedes a spread attribute on a native tag. The button's inline
onClickhandler followed by...restis exactly the pattern that requires proper registration to handle potential overrides from the spread.packages/runtime-tags/src/translator/visitors/function.ts (1)
108-109: LGTM! Correctly prevents early bailout when spread follows.The additional
!hasSpreadAttributeAfter(markoRoot)condition ensures that event handlers on native tags are properly registered when a subsequent spread attribute could potentially override them. This fix addresses the core issue described in the PR.
1 participant
Description
Fix issue with registering default event handlers before a spread on a native tag.