fix(table-core): don't fire onExpandedChange when resetExpanded is a no-op - #6519
fix(table-core): don't fire onExpandedChange when resetExpanded is a no-op#6519Faithfinder wants to merge 2 commits into
Conversation
…no-op table_resetExpanded built a fresh state object and always routed it through onExpandedChange, without comparing against the current expanded state. Its siblings all compare first: table_resetPageIndex and table_resetPageSize early-return when the value already matches, and row_toggleExpanded and table_toggleAllRowsExpanded gained the same guard in TanStack#6501. Since TanStack#6499 wired the expansion auto-reset into createCoreRowModel, that unguarded write fires on every data reference change. For a controlled table whose data is not referentially stable, the new-but-equal map re-renders the consumer, which produces another new data reference, and the cycle repeats without bound. Compare the target state against table.atoms.expanded?.get() and return early when they match, for both defaultState branches. Expanded-all compares by identity, maps key-by-key.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthrough
ChangesExpanded reset behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
we already have code for shallow checking in tanstack store. I might look into more broadly applying that in our setState functions instead of re-implementing shallow checks throughout the entire codebase like this. |
Replace the hand-rolled expanded-state comparison with shallow from @tanstack/store, which table-core already uses as the table store's compare in constructTable. Object.is covers the expanded-all identity case, the typeof guard covers true against a map, and Object.keys plus hasOwnProperty keep null-prototype state maps safe.
Swapped to existing shallow compare function. As for moving it to
|
MILLERMARRU
left a comment
There was a problem hiding this comment.
Went and checked shallow() in @tanstack/store directly since the whole fix hinges on it comparing the two ExpandedState values correctly. It does Object.is first, then falls through to a key-by-key comparison for plain objects, and bails to false if the types don't match (so true vs {} correctly comes out unequal). Since ExpandedState here is always either true or a plain Record<string, boolean> built through makeObjectMap, that comparison path is exactly the right one, no Map/Set/Date branch needed.
This also brings table_resetExpanded in line with row.toggleExpanded and table.toggleAllRowsExpanded, which already skip firing the change handler on a no-op, so it's fixing an actual inconsistency rather than adding new behavior.
The regression test at the bottom (should not loop when a data identity change resets controlled state that already matches) is the one that sells the fix for me. It reproduces the real scenario from the changeset, core row model auto-resetting expanded on every data identity change, feeding back into a controlled table that never converges, and it caps onExpandedChange calls at 5 so a regression fails loudly instead of hanging the suite. That's exactly the kind of test I'd want to see before trusting a "stop an infinite loop" fix.
should reset when the expanded ids differ at the same count is also a good addition on its own, it rules out a lazy same-length-means-equal shortcut, which shallow doesn't take anyway but is worth locking in with a test.
Nothing here looks wrong to me, the fallback to {} on table.atoms.expanded?.get() ?? {} is safe since expanded state is never false or null by type, just true or a map.
…ateSlice (#6532) * refactor(table-core): centralize no-op state update guarding in setStateSlice Route every table.setX state router through a single setStateSlice util that resolves the updater once against the slice's current value, structurally compares the result, and skips the onXChange handler entirely when nothing changed. This removes the class of render loops where auto resets fired change handlers with freshly allocated but semantically identical values (e.g. autoResetExpanded after a data reference change), and deletes the scattered ad-hoc guards that previously protected individual slices. For uncontrolled slices the default handler receives the pre-resolved value, so updaters run exactly once. User handlers and externally owned slices receive the original updater untouched to preserve functional update composition and independent base-atom fallback semantics. Supersedes #6519. Co-Authored-By: Dmitrii Kartashev <dikartashev@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix test and regen docs * add date filters to filter examples * omit row selection from new check * auto fix lint issues * ci: apply automated fixes * format * lint fix * uncomment * address stale updater bug, simplify * still skip autoResetPageIndex in controlled state scenarios * regen docs again * refactor(table-core): make stateSlicesEqual the setStateSlice default, call unguarded handlers directly setStateSlice now always guards, with isEqual as an override hook for custom feature slices. Slices that deliberately skip guarding (rowSelection, columnSizing, columnResizing, globalFilter, and the expanded/cellSelection setters) call their change handler directly instead of routing through a no-op wrapper, keeping pointer-frequency paths free of the routing overhead. rowPinning picks up the guard via the new default, matching columnPinning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Dmitrii Kartashev <dikartashev@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
closed in favor of #6532 |
table_resetExpandedbuilds a fresh state object and always callsonExpandedChange, without comparing against the current state. Every sibling compares first:table_resetPageIndex/table_resetPageSize— early-return when the value already matchesrow_toggleExpanded/table_toggleAllRowsExpanded— early-return, added in fix(table-core): correct expanded/paginated state contents and sorting toggle defaults #6501table_resetExpanded— no guardSince #6499 wired the expansion auto-reset into
createCoreRowModel, that unguarded write fires on everydatareference change:dataidentity changes → core row model recomputes →table_autoResetExpanded→table_resetExpanded→onExpandedChange(newEmptyMap)→ consumer re-renders → newdatareference → repeat.Measured with this repo's
react-tableon React 19.2, controlledexpandedviauseState,getRowCanExpand: () => true, and one row-model read during render, capped at 50 renders:dataonExpandedChangecallsitems ?? []items ?? []+ this fixitems ?? STABLE_EMPTY_ARRAYitems ?? []+autoResetExpanded: falseStable
datais documented, and the loop needs an unstable reference — this is a user mistake in the first instance. The case for guarding anyway is that the punishment is disproportionate and near-undiagnosable: nothing in the stack points at expansion, and the tab is too unresponsive to profile. The pagination resets already absorb exactly this mistake, and v8 absorbed it too — it passedtable.initialState?.expandedthrough by reference, sosetExpanded(sameRef)hit the identity check inuseState. v9 clones it, so the reference is always new.Change
table_resetExpandedcompares its target againsttable.atoms.expanded?.get()(asrow_toggleExpandedalready does) and returns early when they match —trueby identity, maps key-by-key, bothdefaultStatebranches.Two existing tests asserted that the reset fires when the current state already equals the target. They now diverge the state first, so they still cover the reset value.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests