fix: prevent stale re-renders of #each items during sequence update (#468)#501
fix: prevent stale re-renders of #each items during sequence update (#468)#501dupontbertrand wants to merge 4 commits into
Conversation
|
@dupontbertrand sorry for the delay I was occupied with a release the last two weeks. I will need a free timeslot this week to test with our projects then adding my review. We are close to 3.1.0 🚀 |
There was a problem hiding this comment.
Pull request overview
This PR addresses #468 by preventing #each item subtrees from re-rendering with a “mixed” data context during Tracker.flush() (parent context already updated, but ObserveSequence hasn’t yet removed/updated stale items).
Changes:
- Add
onInvalidate,beforeDiff, andafterDiffhooks toObserveSequence.observeso callers can coordinate sequence transitions. - In
Blaze.Each, freeze existing item views on sequence invalidation and unfreeze surviving views after the diff is applied. - Add a guard in
Blaze._materializeView’sdoRenderautorun to skip re-rendering views while an#eachsequence update is pending. - Add Tinytests covering stale renders for “different IDs” and
#each item in ...syntax.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/observe-sequence/observe_sequence.js |
Adds lifecycle hooks around invalidation and diffing for sequence observers. |
packages/blaze/builtins.js |
Uses the new hooks in Blaze.Each to freeze/unfreeze item views during sequence transitions. |
packages/blaze/view.js |
Adds a render guard to avoid stale re-renders while an #each update is pending. |
packages/spacebars-tests/template_tests.js |
Adds regression tests validating no stale renders occur in key #each scenarios. |
packages/spacebars-tests/template_tests.html |
Adds templates used by the new regression tests (plus an unused pair). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip re-render if this view or an ancestor is an #each item | ||
| // that's pending a sequence update. This prevents stale renders | ||
| // where an item's helpers re-run before ObserveSequence has had | ||
| // a chance to remove it. See meteor/blaze#468. | ||
| if (!c.firstRun) { | ||
| let v = view; | ||
| while (v) { | ||
| if (v._eachItemPendingUpdate) return; | ||
| v = v.parentView; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed along these lines. Instead of returning silently, the deferred render now depend()s on a per-each-view Tracker.Dependency that afterDiff fires once the pending flag clears — so the computation is guaranteed to re-run and re-collect its real dependencies. This also surfaced a real regression the silent return introduced: surviving items (same _id, changedAt path) were frozen and never re-rendered; there's a new regression test for that case.
| // Allow callers to prepare for the diff (e.g., freeze item views | ||
| // that are about to be removed). See meteor/blaze#468. | ||
| if (callbacks.beforeDiff) { | ||
| callbacks.beforeDiff(lastSeqArray, seqArray); | ||
| } | ||
|
|
||
| diffArray(lastSeqArray, seqArray, callbacks); | ||
|
|
||
| if (callbacks.afterDiff) { | ||
| callbacks.afterDiff(); | ||
| } |
There was a problem hiding this comment.
Done — wrapped the diff in try/finally so afterDiff always runs (clearing the pending flag and firing the revival dependency) even if a diff callback throws. lastSeq/lastSeqArray are advanced only on the success path, so a partial failure keeps the last consistent baseline. Added an observe-sequence test covering the throwing-callback recovery.
| <template name="spacebars_template_test_each_stale_parent2"> | ||
| {{> spacebars_template_test_each_stale_child2 foo=mode}} | ||
| </template> | ||
| <template name="spacebars_template_test_each_stale_child2"> | ||
| {{#each getItems}} | ||
| <span>{{msg}}-{{logRender msg}}</span> | ||
| {{/each}} | ||
| </template> |
There was a problem hiding this comment.
Good catch — they're no longer unused: parent2/child2 now back a new regression test for the surviving-item (changedAt) case, so there's nothing dead left in the fixtures.
When the data source of a parent template changes, Tracker can re-run helpers inside #each item views before ObserveSequence has had a chance to diff and remove stale items. This causes items to briefly render with inconsistent data (e.g., item shows "foo" while parent data says "bar"). Fix by freezing item views during sequence transitions: - observe_sequence.js: add onInvalidate, beforeDiff, and afterDiff callbacks so callers can hook into the sequence update lifecycle - builtins.js: use onInvalidate to mark item views with _eachItemPendingUpdate immediately when the sequence is invalidated (before Tracker flush), and afterDiff to clear the flag on survivors - view.js: skip doRender re-runs when the view or an ancestor has _eachItemPendingUpdate set Tested with 5 scenarios: different IDs, same ID with changed content, multiple items removed, new-style #each (each-in), and nested #each. Fixes meteor#468
Add two Tinytest cases that verify #each item views don't re-render with stale data when the parent data context changes: - Different IDs (removedAt + addedAt) — the original bug report - New-style #each item in items syntax
…eteor#468) The pending-update guard added for meteor#468 skipped an item view's re-render by returning from `doRender` before `view._render()` ran. That left the computation with zero reactive dependencies, so a surviving item view whose helpers read the parent data context never re-rendered again (confirmed by a regression test): it was frozen, not just delayed. Instead of returning silently, the deferred render now depends on a per-each revival `Tracker.Dependency`, which `afterDiff` fires once the pending flag is cleared. The computation is therefore guaranteed to re-run and re-collect its real dependencies with fresh data. Also wrap the diff in try/finally in observe-sequence so `afterDiff` always runs (clearing the flag and firing the revival dependency) even if a diff callback throws; the `lastSeq`/`lastSeqArray` baseline is only advanced on the success path, so a partial failure keeps the last consistent baseline.
meteor#468) Add a regression test (using the previously unused parent2/child2 fixtures) for a surviving #each item view (same _id, changed data) that must re-render to its new data after a sequence update instead of freezing. Add an observe-sequence test asserting `afterDiff` still runs when a diff callback throws, so callers are never left permanently frozen.
dupontbertrand
force-pushed
the
fix/each-stale-data-context
branch
from
336cfd1 to
64c4b45
Compare
|
Hi @jankapunkt 👋 following up on your suggestion to build an app that reproduces #468 in every way I could think of, then swaps in the local Blaze to confirm the fix 👍 Repro app: https://github.com/dupontbertrand/blaze-468-repro It's a Meteor 3 + Blaze app that reproduces the stale #each data context across 10 scenarios (different _ids · same-_id changedAt · partial removal · {{#each x in xs}} · nested · Mongo
Building it actually surfaced something worth flagging: the first version of the guard fixed the removed-item case from the report, but regressed surviving items (same _id, changedAt) : they froze instead of re-rendering, because the silent early-return left their computation with no reactive dependencies. I reworked the fix to defer the render via a revival Tracker.Dependency that afterDiff fires (rather than dropping it — the approach Copilot also suggested), and wrapped the diff in try/finally so a throwing diff callback can't leave items permanently frozen. That's force-pushed to the PR now; CI is green on both the native-DOM and jQuery backends One honest scope note (also in the repro's README): this fixes staleness of the data context. A helper with a direct reactive dependency on the same source the sequence reads (e.g. a closure over a global ReactiveVar) is a separate race the deferral doesn't catch — happy to discuss whether that's worth chasing on its own Whenever you have a slot to test against your projects, the repro should make it quick to confirm 😉 |
Summary
Fixes #468
When the data source of a parent template changes, Tracker can re-run helpers inside
#eachitem views beforeObserveSequencehas had a chance to diff and remove stale items. This causes items to briefly render with inconsistent data — e.g., an item'smsgshows"foo"while the parent data context already says"bar".Root cause
During
Tracker.flush(), autoruns are re-run in invalidation order. When a parent data context changes:#eachitem views are invalidated (viabindDataContext→Blaze.getData()→dataVar.get())ObserveSequenceautorun is also invalidated (it depends on the sequence function)Fix
Freeze item views during sequence transitions using 3 hooks:
onInvalidate(new callback inObserveSequence.observe): called immediately when the sequence source is invalidated, BEFORETracker.flush()re-runs other autoruns. Marks all current item views with_eachItemPendingUpdate = true.afterDiff(new callback): clears the flag on surviving item views after the diff is applied.doRenderguard (in_materializeView): skips re-render if the view or any ancestor has_eachItemPendingUpdateset.This ensures stale item views never re-render — they are either destroyed by
removedAtor updated bychangedAt, and the flag is cleared afterward.Files changed
packages/observe-sequence/observe_sequence.jsonInvalidate,beforeDiff,afterDiffcallback hookspackages/blaze/builtins.jsBlaze.Eachto freeze/unfreeze item viewspackages/blaze/view.jsdoRenderto skip re-render when pending updateTest plan
#each item in items#each(outer + inner)