TEMPLATE-570: preserve attributes on repeat unbind - #128
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
This PR fixes repeat’s unbind behavior so it no longer restores elements by replacing the entire node from the first-render HTML snapshot (which was reverting later attribute edits and detaching the live DOM node). Instead, unbind restores only the template’s inner content and removes render-time bookkeeping while keeping the original element instance and its current attributes, aligning behavior with foreach.
Changes:
- Update
repeatunbind to restore inner template content without replacing the element node (preserves attribute edits; avoids detached-node issues). - Explicitly strip render-time attributes (
hidden,data-repeat-template-id) during unbind. - Add regression tests covering attribute preservation (including table cells), idempotent handle/unbind cycles, and
hiddencleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/dom/attribute-manager/attributes/loop-attrs/repeat-attr/index.js | Makes repeat unbind non-destructive by restoring inner content and removing bookkeeping attributes rather than replacing the node. |
| src/dom/attribute-manager/attributes/loop-attrs/repeat-attr/tests/test-repeat-attr.js | Adds regression tests for TEMPLATE-570 and related unbind/rebind edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // template form. The old replaceWith(originalHTML) removed it implicitly; since we now | ||
| // keep the live node we must strip it explicitly. Removed here (after the sibling | ||
| // cleanup above, which still needs the id) rather than in the `if (id)` block. | ||
| $el.removeAttr('data-' + templateIdAttr); |
There was a problem hiding this comment.
I dont think this is an issue
| $rootNode.children().length.should.equal(3); | ||
| }); | ||
|
|
||
| it('should preserve edits to the element`s own attributes instead of reverting them (TEMPLATE-570)', ()=> { |
The repeat attribute's unbind handler restored the element by replacing it wholesale with an HTML snapshot captured at first render ($el.replaceWith(originalHTML)). This reverted any attribute edited on the element after that snapshot and detached the live node. In the interface builder, that meant changing the variable (data-f-repeat) on a table row via the dropdown was silently discarded — the write was reverted on the rebind, and the follow-up bindAll operated on a dead node, so nothing saved or refreshed.
The fix makes unbind non-destructive: instead of replacing the element, it restores only the inner template content and strips render-time bookkeeping (hidden, data-repeat-template-id) while keeping the live node and its current attributes — mirroring how the sibling foreach handler already behaves. Attribute edits now survive an unbind/rebind cycle, so selections save and the preview refreshes. This also fixes the same detach bug on the runtime restoreOriginal path. Covered by new regression tests (attribute edit preserved, table-element case, idempotent repeat cycles, hidden cleanup)