fix(angular): ensure table options updates are not missed during first mount window - #6534
fix(angular): ensure table options updates are not missed during first mount window#6534riccardoperra wants to merge 5 commits into
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesAngular options synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to This change improves table option updates during initial mounting and refines lazy initialization with targeted tests; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant AngularEffect
participant injectTable
participant Table
AngularEffect->>injectTable: evaluate computed options
injectTable->>Table: initialize with current options
AngularEffect->>injectTable: detect changed option object
injectTable->>Table: apply changed options
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit 0844a9f
☁️ Nx Cloud last updated this comment at |
421a4c1 to
8b49bd6
Compare
riccardoperra
changed the title
8b49bd6 to
c4bb4a5
Compare
🚀 Changeset Version Preview1 package(s) bumped directly, 0 bumped as dependents. 🟩 Patch bumps
|
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
The mechanism here is solid. Wrapping _options in computed() is what makes the previousOptions === currentOptions reference check meaningful in the first place, calling the raw factory twice always returns two different object literals, so without the memoization the comparison would never be equal and every effect run would apply. With computed(), a real signal change is what produces a new reference, so the effect's very first run can now tell "options actually changed since I captured the initial snapshot" from "nothing changed, this is just the effect settling," instead of unconditionally skipping the first run like isMount did. Traced the timeline in #6530 and this correctly handles the specific race described there, an update that lands in the gap between construction and the effect's first flush is no longer silently dropped.
Worth being precise about scope though, since the changeset and the linked issue don't quite line up. #6530 describes two problems: the primary one is lazySignalInitializer.ts's queueMicrotask eager-init firing before Angular's input bindings are applied, throwing an uncaught NG0950 in test environments with an async beforeEach. The isMount swallow is called out as a secondary issue, one that only surfaces once you work around the first one. This PR only touches injectTable.ts, lazySignalInitializer.ts isn't part of the diff, so it fixes proposal (3) from the issue but not proposal (1). The reported symptom, the uncaught NG0950 in TestBed suites, is still there after this merges. Worth linking as "part of #6530" rather than "closes #6530", or the eager-init half is going to look resolved when it isn't.
|
Just went through the new commit, this closes the gap I mentioned earlier. Traced through a few sequences by hand (options changing before the table is ever constructed, options changing right after) and the effect's |
e8ebb4a to
87a5785
Compare
87a5785 to
5238e94
Compare
5238e94 to
8c28d35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (2)
packages/angular-table/src/lazySignalInitializer.ts (1)
6-8: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Declare
rawValueas nullable before initialization.The test asserts that
rawValueis null beforeTestBed.tick(), but this API declaresrawValueasT. This hides a reachable null value from consumers.injectTablecurrently dereferenceslazyTable.rawValue, so it relies on Angular effect ordering to avoid a null dereference.Change
rawValuetoT | null. Update consumers to accessvalueonly after their update guard decides that initialization is required.Proposed fix
): { - readonly rawValue: T + readonly rawValue: T | null readonly value: T readonly initialized: boolean } { @@ return { value: proxy, get rawValue() { - return object as T + return object },- const tableInstance = lazyTable.rawValue if (previousOptions === currentOptions) return untracked(() => - tableInstance.setOptions((previous) => ({ + lazyTable.value.setOptions((previous) => ({ ...previous, ...currentOptions, })),Also applies to: 55-59
packages/angular-table/tests/lazy-init.test.ts (1)
16-36: LGTM!Also applies to: 50-50, 70-77, 104-114
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4f11a9a-b4eb-47dc-bbc3-adb1abeaea47
📒 Files selected for processing (3)
packages/angular-table/src/injectTable.tspackages/angular-table/src/lazySignalInitializer.tspackages/angular-table/tests/lazy-init.test.ts
|
@KevinVandy pr ready |
2 participants
This pull request addresses a key issue in the
@tanstack/angular-tablepackage where options updates could be missed during the initial mount of a table. The changes ensure that option updates are reliably captured from the very first render, and also refactor the lazy initialization logic to provide more robust state tracking and improved test coverage.Core improvements to options reactivity and initialization:
injectTablefunction now uses a computed signal for options and ensures updates are not missed during the initial mount, fixing a bug where the first update could be dropped. This involves tracking previous options and applying updates only when they change.lazyInitutility is refactored to return an object withvalue,rawValue, andinitializedproperties, allowing for more precise control and introspection of the lazy initialization state. [1] [2] [3]Testing and reliability improvements:
Documentation:
Summary by CodeRabbit
Bug Fixes
Behavior Updates
Release
@tanstack/angular-table.