Conversation
… enhanced rendering control
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds per-item lifecycle callbacks to ForEachBinding: implements Changes
Sequence Diagram(s)sequenceDiagram
participant Binding as ForEachBinding
participant DOM as DOM / Template Engine
participant Callback as User Callbacks
rect rgba(200,230,201,0.5)
Binding->>Binding: computeMoves(oldList, newList)
Binding->>Callback: call beforeMove(node, newIndex, value)
end
rect rgba(187,222,251,0.5)
Binding->>DOM: apply add/remove/move operations
DOM-->>Binding: DOM updated
end
rect rgba(255,224,178,0.5)
Binding->>Callback: call afterMove(node, newIndex, value)
Binding->>Callback: flush pendingAfterRender: afterRender(nodes, value)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/binding.foreach/spec/eachBehavior.ts (1)
894-929: Coverage works, but consider broadening the move scenarios.The single splice-at-0 case correctly exercises both phases —
parentText: 'first'before and'addedfirst'after — which neatly captures the processQueue ordering (beforeMove → adds → afterMove, seepackages/binding.foreach/src/foreach.ts:241-272). One scenario does leave gaps worth a follow-up:
- No assertion that
beforeMove/afterMovefire for multi-node moves (e.g.,obs.reverse()on the existing "sorting complex data moves all DOM nodes" fixture).- No coverage that the callbacks are not invoked when an item's index is unchanged (only true moves should fire).
- No guard against the registration optimization in
processMoves— i.e., that move detection still works when only one ofbeforeMove/afterMoveis registered.♻️ Example additional case
it('fires for every retained node when the array is reversed', function () { const items = ['a', 'b', 'c'].map(name => ({ name })) const calls: Array<{ phase: string; index: number; value: any }> = [] const arr = observableArray(items) const target = $( "<ul data-bind='foreach: { data: arr, beforeMove: beforeMove, afterMove: afterMove }'><li data-bind='text: name'></li></div>" ) applyBindings( { arr, beforeMove: (_n, index, value) => calls.push({ phase: 'before', index, value }), afterMove: (_n, index, value) => calls.push({ phase: 'after', index, value }) }, target[0] ) arr.reverse() // Expect 6 entries: 3 before + 3 after, each with the new index of the retained node assert.equal(calls.length, 6) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/spec/eachBehavior.ts` around lines 894 - 929, Add broader move-scenario tests to eachBehavior.ts: add a test that reverses an observableArray to assert beforeMove/afterMove fire for every retained node (e.g., expect 3 before + 3 after for a 3-item array), a test ensuring callbacks are not invoked when an item's index remains unchanged, and a test where only beforeMove or only afterMove is provided to ensure move-detection still registers (refer to the existing beforeMove/afterMove handlers and the processMoves logic). Locate the foreach binding setup in the current spec (the target/applyBindings block) and duplicate similar setups for these scenarios, use assertions on calls length and entries (phase, index, value, parentText where applicable) to validate correct ordering and invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/binding.foreach/spec/eachBehavior.ts`:
- Around line 894-929: Add broader move-scenario tests to eachBehavior.ts: add a
test that reverses an observableArray to assert beforeMove/afterMove fire for
every retained node (e.g., expect 3 before + 3 after for a 3-item array), a test
ensuring callbacks are not invoked when an item's index remains unchanged, and a
test where only beforeMove or only afterMove is provided to ensure
move-detection still registers (refer to the existing beforeMove/afterMove
handlers and the processMoves logic). Locate the foreach binding setup in the
current spec (the target/applyBindings block) and duplicate similar setups for
these scenarios, use assertions on calls length and entries (phase, index,
value, parentText where applicable) to validate correct ordering and invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5afcc42c-4fd4-4c4f-82db-83575547161d
📒 Files selected for processing (1)
packages/binding.foreach/spec/eachBehavior.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/binding.foreach/src/foreach.ts (3)
342-349: Defensive reset forpendingAfterRenderif a user callback throws.If
this.afterRenderthrows partway through the loop,pendingAfterRenderis never cleared and stale entries will re-fire on the nextprocessQueue. Wrapping the iteration intry { … } finally { this.pendingAfterRender = new Array() }would isolate user-callback failures. Niche edge case (and consistent with other callback sites here that don't catch), so feel free to defer.🛡️ Optional hardening
flushPendingAfterRender() { - if (typeof this.afterRender === 'function') { - arrayForEach(this.pendingAfterRender, item => { - this.afterRender(item.nodes, item.value) - }) - } - this.pendingAfterRender = new Array() + const queue = this.pendingAfterRender + this.pendingAfterRender = new Array() + if (typeof this.afterRender === 'function') { + arrayForEach(queue, item => { + this.afterRender(item.nodes, item.value) + }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/src/foreach.ts` around lines 342 - 349, The loop in flushPendingAfterRender uses this.afterRender and if a user callback throws it never clears this.pendingAfterRender; wrap the arrayForEach invocation (or the whole conditional block in flushPendingAfterRender) in a try { ... } finally { this.pendingAfterRender = new Array() } so pendingAfterRender is always reset even on exceptions from afterRender (refer to flushPendingAfterRender, this.pendingAfterRender and this.afterRender).
328-340: ConsolidategetNodesForEntrywith the existinggetNodesForIndex.The new helper duplicates the sibling-walk logic in
getNodesForIndex(lines 457–467); only the input shape differs. Consider sharing the implementation, e.g.:♻️ Proposed consolidation
- getNodesForEntry(entry: { first: Node; last: Node }) { - const result: Node[] = [] - let ptr: Node | null = entry.first - const last = entry.last - result.push(ptr) - while (ptr && ptr !== last) { - ptr = ptr.nextSibling - if (ptr) { - result.push(ptr) - } - } - return result - } @@ - getNodesForIndex(index) { - const result = new Array() - let ptr = this.firstLastNodesList[index].first - const last = this.firstLastNodesList[index].last - result.push(ptr) - while (ptr && ptr !== last) { - ptr = ptr.nextSibling! - result.push(ptr) - } - return result - } + getNodesForEntry(entry: { first: Node; last: Node }): Node[] { + const result: Node[] = [entry.first] + let ptr: Node | null = entry.first + while (ptr && ptr !== entry.last) { + ptr = ptr.nextSibling + if (ptr) result.push(ptr) + } + return result + } + + getNodesForIndex(index: number): Node[] { + return this.getNodesForEntry(this.firstLastNodesList[index]) + }Also applies to: 457-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/src/foreach.ts` around lines 328 - 340, Duplicate sibling-walk logic exists in getNodesForEntry and getNodesForIndex; consolidate by reusing one implementation: either extract a shared helper (e.g., walkSiblingRange(first: Node, last: Node): Node[]) containing the loop and push logic, then have getNodesForEntry(entry) call walkSiblingRange(entry.first, entry.last) and have getNodesForIndex(index) call the same helper (or adapt getNodesForIndex to accept first/last), or make getNodesForEntry construct the same shape that getNodesForIndex expects and forward to it; update references to getNodesForEntry and getNodesForIndex accordingly to remove the duplicated loop.
309-326: Add multi-root template tests forbeforeMove/afterMoveto explicitly document per-node callback invocation.The code correctly invokes
beforeMove/afterMoveonce per DOM node in a moved nodeset (not once per item likeafterRender). Tests confirm this behavior for single-root templates, but multi-root cases are not covered. Add specs for multi-root templates to lock in this behavior and clarify the contract in documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/src/foreach.ts` around lines 309 - 326, Add tests that verify beforeMove/afterMove callbacks are invoked once per DOM node for multi-root templates by exercising fireMoveCallback (which uses firstLastNodesList and getNodesForEntry). Specifically, create multi-root template scenarios where a logical item corresponds to multiple DOM nodes and assert that the callback passed into fireMoveCallback is called per node (not per item) with the correct node, newIndex, and value; cover both lookupKey variants ('oldIndex' and 'newIndex') and beforeMove/afterMove usage to lock the documented contract.packages/binding.foreach/spec/eachBehavior.ts (2)
859-880: Tighten the reused-nodeset assertion.
includeMembers([a, c])+notInclude(b)allows extra unexpected entries past index 3 to slip through silently. Pinningcalls.length(or assertingcalls.slice(3).length === 2) would lock the behavior more precisely against future regressions.♻️ Proposed tightening
arr.reverse() assert.equal(target.text(), 'cba') - const reusedValues = calls.slice(3).map(call => call.value) + const reusedCalls = calls.slice(3) + assert.equal(reusedCalls.length, 2) + const reusedValues = reusedCalls.map(call => call.value) assert.includeMembers(reusedValues, [a, c]) assert.notInclude(reusedValues, b)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/spec/eachBehavior.ts` around lines 859 - 880, The assertion on reused nodes is too loose; tighten it by asserting the exact number of afterRender calls for the reinsertion step. After the existing assert.equal(calls.length, 3) and arr.reverse(), add an assertion that either calls.slice(3).length === 2 or assert.equal(calls.length, 5), then keep the existing check that the reusedValues (calls.slice(3).map(...)) contains exactly the expected items [a, c] and does not include b; reference the calls array, arr.reverse(), the afterRender callback cb, and reusedValues to locate where to add the length check.
917-975: Move-callback coverage looks good; consider a multi-root template case.The single-root template here exercises one callback per move, which masks the per-DOM-node firing in
fireMoveCallback. A spec with a multi-root template (e.g.,<li>x</li><li>y</li>per item, or a virtual block with multiple nodes) would lock down whether the callback should fire once per node or once per item — useful given this is a documented deviation point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/spec/eachBehavior.ts` around lines 917 - 975, Add a new spec that uses a multi-root template to verify move callbacks fire per DOM node: create observableArray with an object item, define beforeMove and afterMove handlers that record node.textContent, index and value, use a template that renders multiple roots per item (e.g., "<li data-bind='text: name'></li><li data-bind='text: name'></li>" or a virtual multi-node block), applyBindings with applyBindings(..., target[0]), perform an arr.splice to cause a shift, and assert that the recorded calls from beforeMove/afterMove (and therefore fireMoveCallback) equal the number of DOM nodes moved and contain the expected node texts, indices and values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/binding.foreach/spec/eachBehavior.ts`:
- Around line 859-880: The assertion on reused nodes is too loose; tighten it by
asserting the exact number of afterRender calls for the reinsertion step. After
the existing assert.equal(calls.length, 3) and arr.reverse(), add an assertion
that either calls.slice(3).length === 2 or assert.equal(calls.length, 5), then
keep the existing check that the reusedValues (calls.slice(3).map(...)) contains
exactly the expected items [a, c] and does not include b; reference the calls
array, arr.reverse(), the afterRender callback cb, and reusedValues to locate
where to add the length check.
- Around line 917-975: Add a new spec that uses a multi-root template to verify
move callbacks fire per DOM node: create observableArray with an object item,
define beforeMove and afterMove handlers that record node.textContent, index and
value, use a template that renders multiple roots per item (e.g., "<li
data-bind='text: name'></li><li data-bind='text: name'></li>" or a virtual
multi-node block), applyBindings with applyBindings(..., target[0]), perform an
arr.splice to cause a shift, and assert that the recorded calls from
beforeMove/afterMove (and therefore fireMoveCallback) equal the number of DOM
nodes moved and contain the expected node texts, indices and values.
In `@packages/binding.foreach/src/foreach.ts`:
- Around line 342-349: The loop in flushPendingAfterRender uses this.afterRender
and if a user callback throws it never clears this.pendingAfterRender; wrap the
arrayForEach invocation (or the whole conditional block in
flushPendingAfterRender) in a try { ... } finally { this.pendingAfterRender =
new Array() } so pendingAfterRender is always reset even on exceptions from
afterRender (refer to flushPendingAfterRender, this.pendingAfterRender and
this.afterRender).
- Around line 328-340: Duplicate sibling-walk logic exists in getNodesForEntry
and getNodesForIndex; consolidate by reusing one implementation: either extract
a shared helper (e.g., walkSiblingRange(first: Node, last: Node): Node[])
containing the loop and push logic, then have getNodesForEntry(entry) call
walkSiblingRange(entry.first, entry.last) and have getNodesForIndex(index) call
the same helper (or adapt getNodesForIndex to accept first/last), or make
getNodesForEntry construct the same shape that getNodesForIndex expects and
forward to it; update references to getNodesForEntry and getNodesForIndex
accordingly to remove the duplicated loop.
- Around line 309-326: Add tests that verify beforeMove/afterMove callbacks are
invoked once per DOM node for multi-root templates by exercising
fireMoveCallback (which uses firstLastNodesList and getNodesForEntry).
Specifically, create multi-root template scenarios where a logical item
corresponds to multiple DOM nodes and assert that the callback passed into
fireMoveCallback is called per node (not per item) with the correct node,
newIndex, and value; cover both lookupKey variants ('oldIndex' and 'newIndex')
and beforeMove/afterMove usage to lock the documented contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cdac2ee-d63d-441c-a9d2-476d58d91714
📒 Files selected for processing (3)
packages/binding.foreach/spec/eachBehavior.tspackages/binding.foreach/src/foreach.tstko.io/src/content/docs/bindings/foreach-binding.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tko.io/src/content/docs/bindings/foreach-binding.md
…factoring node retrieval
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/binding.foreach/src/foreach.ts (1)
254-287:⚠️ Potential issue | 🟠 MajorCleanup scope is too narrow — beforeMove/loop/afterMove throws can wedge the binding.
The
try/finallyonly wrapsflushPendingAfterRender()+endQueueFlush(), but three other call sites inprocessQueuecan run user code that throws:
- Line 260:
fireMoveCallback(this.beforeMove, …)- Line 262–267:
arrayForEach(this.changeQueue, …)(which invokesadded()→applyBindingsToDescendants, plusbeforeRemove)- Line 276:
fireMoveCallback(this.afterMove, …)If any of these throws,
this.changeQueueis never cleared. Worse, ifbeforeMoveor the change-queue loop throws (i.e., before line 269),this.rendering_queuedstaystrue, so subsequentonArrayChangecalls will keep appending tochangeQueuebut will never schedule anotherprocessQueue(the guard at lines 229–230 short-circuits). The binding is effectively wedged until dispose.The new spec at lines 959-992 (
updates conditional state and does not replay a completed queue after afterRender throws) covers only theafterRenderpath;beforeMove/afterMoveerrors hit the unprotected window.🛡️ Suggested wider cleanup scope
processQueue() { const isEmpty = !unwrap(this.data).length let lowestIndexChanged = MAX_LIST_SIZE const moves = this.computeMoves(unwrap(this.data) || []) - this.startQueueFlush() - this.fireMoveCallback(this.beforeMove, moves, 'oldIndex') - - arrayForEach(this.changeQueue, changeItem => { - if (typeof changeItem.index === 'number') { - lowestIndexChanged = Math.min(lowestIndexChanged, changeItem.index) - } - this[changeItem.status](changeItem) - }) - this.flushPendingDeletes() - this.rendering_queued = false - - // Update our indexes. - if (this.$indexHasBeenRequested) { - this.updateIndexes(lowestIndexChanged) - } - - this.fireMoveCallback(this.afterMove, moves, 'newIndex') try { + this.startQueueFlush() + this.fireMoveCallback(this.beforeMove, moves, 'oldIndex') + + arrayForEach(this.changeQueue, changeItem => { + if (typeof changeItem.index === 'number') { + lowestIndexChanged = Math.min(lowestIndexChanged, changeItem.index) + } + this[changeItem.status](changeItem) + }) + this.flushPendingDeletes() + + if (this.$indexHasBeenRequested) { + this.updateIndexes(lowestIndexChanged) + } + + this.fireMoveCallback(this.afterMove, moves, 'newIndex') this.flushPendingAfterRender() this.endQueueFlush() } finally { + this.rendering_queued = false this.changeQueue = new Array() - // Update the conditional exposed on the domData if (isEmpty !== !this.isNotEmpty()) { this.isNotEmpty(!isEmpty) } } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5fb5956-876e-41e7-8527-df228f4695be
📒 Files selected for processing (2)
packages/binding.foreach/spec/eachBehavior.tspackages/binding.foreach/src/foreach.ts
Done. |
1 participant
Summary
Closes #329.
Adds the documented
foreachlifecycle callbacks that were missing fromForEachBindingin the reference build, without breaking the existingafterAdd/beforeRemovebatch contract:afterRender(nodes, value)— fires once per item when a new nodeset is rendered from the template (initial render and later additions).beforeMove(node, newIndex, value)/afterMove(node, newIndex, value)— fire per retained node whose index changed; move detection compares the trackedfirstLastNodesListentries against the current array values and only runs if at least one move callback is registered.afterAdd({ nodeOrArrayInserted, foreachInstance })andbeforeRemove({ nodesToRemove, foreachInstance })are unchanged, including the tko-specific thenable-return shortcut for deferred node removal.foreach-binding.mdNote 8) now calls out the reference build's batch-shapedafterAdd/beforeRemoveand the thenable extension as deviations from the original Knockout contract.Test plan
bun run verify(5395 tests pass, 0 failures)afterRenderfires for initial and added items with(nodes, value)beforeMove/afterMovefire with(node, newIndex, value)when retained items shiftafterAdd/beforeRemovetests continue to pass unchanged (batch signature + thenable removal)verified-behaviors.jsonupdatedpatch, additive only)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests