fix(angular-query): ensure initial mutation pending state is emitted #9098
Conversation
ThiloAschebrock
force-pushed
the
bugfix/mutation-skips-pending-state
branch
from
b4722f2 to
5c6a9de
Compare
|
Removed the last |
lachlancollins
force-pushed
the
main
branch
from
5d7869e to
18a12ca
Compare
ThiloAschebrock
force-pushed
the
bugfix/mutation-skips-pending-state
branch
from
527574a to
a058bf7
Compare
🦋 Changeset detectedLatest commit: 5da0ab7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@ThiloAschebrock has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 12 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
WalkthroughReworks injectMutation internal state and lifecycle: replaces effect-based signals with a single linked result signal, adds DestroyRef cleanup and ngZone-managed subscriptions, introduces pending-task tracking, and updates tests to use async timer advancement and app.whenStable(); adds a test for constructor-triggered mutation pending state. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component
participant Z as ngZone
participant M as MutationObserver
participant S as linkedResultSignal
rect rgba(220,240,255,0.35)
Note over C,S: Previous (effect-based) flow
C->>M: trigger mutate() (constructor/ngOnInit)
M-->>S: synchronous update (subscription timing race)
Note right of S: pending state may be missed
end
rect rgba(220,255,220,0.35)
Note over C,S: New (lifecycle-aware) flow
C->>Z: trigger mutate() (constructor/ngOnInit)
Z->>Z: runOutsideAngular queues observer subscription
Z->>S: set pending task immediately
Z->>M: subscribe (post-init) and observe results
M-->>Z: onComplete
Z->>S: clear pending / propagate final result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/angular-query-experimental/src/inject-mutation.ts (2)
137-145: Consider setting state before throwing to preserve observable state.Today, when
throwOnErrortriggers, you throw beforeresult.set(state), so consumers might not observe the error state update. Optionally set the state first, then throw in a microtask to keep behavior while maintaining state visibility.- if ( - state.isError && - shouldThrowError(observer.options.throwOnError, [state.error]) - ) { - ngZone.onError.emit(state.error) - throw state.error - } - - result.set(state) + if (state.isError) { + result.set(state) + if (shouldThrowError(observer.options.throwOnError, [state.error])) { + queueMicrotask(() => { + ngZone.run(() => ngZone.onError.emit(state.error)) + throw state.error + }) + return + } + return + } + result.set(state)If you prefer current semantics, feel free to keep as‑is.
94-101: Make non‑throwing mutate intent explicit.Prefix with
voidso the returned promise isn’t accidentally relied upon by callers.- return (variables, mutateOptions) => { - observer.mutate(variables, mutateOptions).catch(noop) - } + return (variables, mutateOptions) => { + void observer.mutate(variables, mutateOptions).catch(noop) + }packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (1)
392-421: Harden the constructor‑pending test against flakiness.Use async timers and trigger change detection before assertions to avoid timing races.
- vi.advanceTimersByTime(1) - expect(span.nativeElement.textContent).toEqual('pending') + await vi.advanceTimersByTimeAsync(1) + fixture.detectChanges() + expect(span.nativeElement.textContent).toEqual('pending')Optionally add a sibling test for
ngOnInitto mirror the reported scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts(1 hunks)packages/angular-query-experimental/src/inject-mutation.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (1)
packages/angular-query-experimental/src/inject-mutation.ts (1)
injectMutation(45-182)
packages/angular-query-experimental/src/inject-mutation.ts (3)
packages/query-core/src/mutationObserver.ts (2)
MutationObserver(23-211)state(145-159)packages/angular-query-experimental/src/pending-tasks-compat.ts (1)
PendingTaskRef(7-7)packages/query-core/src/utils.ts (1)
shouldThrowError(446-456)
ThiloAschebrock
force-pushed
the
bugfix/mutation-skips-pending-state
branch
from
1d86a66 to
437f509
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (1)
390-419: Consider using async timer advancement for consistency.The test uses
vi.advanceTimersByTime(1)(synchronous) on line 412, while most other tests useawait vi.advanceTimersByTimeAsync(...). In zoneless change detection mode, using the async version ensures that all async operations, including change detection updates, have completed before asserting.Apply this diff for consistency:
- vi.advanceTimersByTime(1) + await vi.advanceTimersByTimeAsync(1) expect(span.nativeElement.textContent).toEqual('pending')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts(8 hunks)packages/angular-query-experimental/src/inject-mutation.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (1)
packages/angular-query-experimental/src/inject-mutation.ts (1)
injectMutation(44-182)
packages/angular-query-experimental/src/inject-mutation.ts (2)
packages/query-core/src/mutationObserver.ts (2)
MutationObserver(23-211)state(145-159)packages/query-core/src/utils.ts (2)
noop(82-82)shouldThrowError(446-456)
🔇 Additional comments (6)
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (2)
425-443: Verify signal access pattern for destructured properties.The test uses
status()(calling it as a signal) buterror(without calling). Iferroris also a signal from the proxied result, it should be called aserror()for consistency. Please verify whether the destructurederrorproperty requires the call operator.If both are signals, apply this diff:
expect(status()).toBe('error') - expect(error).toBe(err) + expect(error()).toBe(err)Similar changes needed at lines 460 and 477.
569-570: LGTM: Proper async synchronization.The use of
await app.whenStable()after timer advancement correctly ensures that all pending tasks (including mutations) complete before assertions. This pattern aligns well with the new pending task tracking introduced in the fix.packages/angular-query-experimental/src/inject-mutation.ts (4)
81-89: LGTM: Efficient observer instance reuse.The updated logic correctly reuses a single
MutationObserverinstance and callssetOptions()when options change, rather than recreating the observer. This improves efficiency while maintaining reactivity.
98-98: LGTM: Proper promise handling.Using
void observer.mutate(...).catch(noop)correctly prevents unhandled promise rejections for the fire-and-forget mutation pattern.
112-162: Excellent fix: Initial pending state now captured correctly.The
linkedResultSignalimplementation resolves the root cause by:
- Line 120: Immediately seeding
pendingTaskRefwhencurrentResult.isPendingis true—this ensures that mutations triggered in constructor/ngOnInit don't miss the pending state.- Line 119: Cleaning up previous subscription before creating a new one, preventing leaks.
- Lines 122-149: Subscribing outside Angular zone for performance while updating state inside the zone for change detection.
This addresses the past review comment about seeding the pending task reference.
Based on learnings (past review comments).
174-174: LGTM: Cleanup properly invoked on destroy.Wrapping
cleanup()in a lambda ensures the latest cleanup implementation is invoked when the component is destroyed, rather than capturing the initialnoopreference. This addresses the past review comment about preventing resource leaks.Based on learnings (past review comments).
ThiloAschebrock
force-pushed
the
bugfix/mutation-skips-pending-state
branch
3 times, most recently
from
1d23c78 to
840b44f
Compare
|
Hi @arnoud-dv, I would appreciate if you find some time to review my proposed changes. |
ThiloAschebrock
force-pushed
the
bugfix/mutation-skips-pending-state
branch
from
840b44f to
5da0ab7
Compare
2 participants
🎯 Changes
Fixes #9020
The issue was that the effect that subscribes to the observer runs after the observer emits the first state change if the mutation is triggered in the
constructorof a component or withinngOnInit. This is fixed replacing effects with computed signals that also handle the subscriptions.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Refactor
Tests