Fix x-model.parent when inside of x-teleport#4676
Conversation
willrowe
changed the title
x-model.parent when insided of x-teleportx-model.parent when inside of x-teleport
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fixed </h1> -> </h2> in test HTML (was opening h2, closing h1) - Added missing newline at end of file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: #4676 — Fix
|
| Suite | Tests | Pass | Fail |
|---|---|---|---|
| x-modelable.spec.js | 6 | 6 | 0 |
| x-model.spec.js | 33 | 33 | 0 |
| x-teleport.spec.js | 11 | 11 | 0 |
| CI | - | PASS | - |
Regression verified: stashed the fix, rebuilt, and confirmed tests 5 and 6 fail with "value is not defined" on main. Tests pass with the fix applied.
Code Review
lifecycle.js:69 — The key findClosest fix: when _x_teleportBack exists, recurse to the teleport source and return early. The old code did el = el._x_teleportBack then fell through to el.parentElement, which skipped the teleport template element itself for callback evaluation. The early return ensures the template element gets checked against the callback. Clean fix.
x-model.js:14 — findClosest(el, (element) => element !== el) is a clever way to say "find my logical parent." It starts at el, callback returns false (same element), then walks up through teleport boundaries or DOM, and returns the first different element. This is the right abstraction — it delegates all traversal knowledge to findClosest.
Test quality: Three good test cases covering (1) basic x-modelable inside teleport, (2) x-model.parent inside teleport, and (3) x-data directly on the template element. Tests 2 and 3 specifically target the bug. Test 1 already passed on main (since x-model without .parent uses closestRoot which already calls findClosest), but it's good documentation.
Style: No semicolons, uses let. Matches project conventions.
Security
No security concerns identified.
Verdict
Clean, surgical bug fix. The root cause is correctly identified (x-model.parent uses DOM parentNode which breaks for teleported elements), the fix is minimal (one import, one line change in x-model.js, one early-return in lifecycle.js), and it reuses existing infrastructure rather than introducing new concepts. Tests are solid and verified against regression. No regressions in related test suites. Merge it.
Reviewed by Claude
2 participants
This does two things:
findClosestto include the teleport element (usually atemplateelement) instead of skipping over it to its parent.x-model.parentto usefindClosestto determine the parent. This allows the current logic of traversing upwards through teleports to work correctly as well as automatically working with any future changes to traversal.