query icon indicating copy to clipboard operation
query copied to clipboard

fix(angular-query): ensure initial mutation pending state is emitted

Open ThiloAschebrock opened this issue 9 months ago • 4 comments

🎯 Changes

Fixes https://github.com/TanStack/query/issues/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 constructor of a component or within ngOnInit. This is fixed replacing effects with computed signals that also handle the subscriptions.

✅ Checklist

  • [x] I have followed the steps in the Contributing guide.
  • [x] I have tested this code locally with pnpm run test:pr.

🚀 Release Impact

  • [x] This change affects published code, and I have generated a changeset.
  • [ ] This change is docs/CI/dev-only (no release).

Summary by CodeRabbit

  • Refactor

    • Reworked internal mutation handling for more efficient observer reuse and stronger cleanup tied to component destruction.
    • Improved pending-task and error propagation behavior while keeping the public API backward compatible.
  • Tests

    • Updated tests for more reliable lifecycle synchronization using async timer advancement and stability checks.
    • Added tests verifying pending state transitions and observable status/error propagation.

ThiloAschebrock avatar May 03 '25 04:05 ThiloAschebrock

Removed the last effect of injectMutation in 9fcd7dd that was causing scenarios where the mutation function could be out of sync with the provided options.

ThiloAschebrock avatar May 24 '25 00:05 ThiloAschebrock

🦋 Changeset detected

Latest commit: 5da0ab7a3c6de3e0cd014d2a45c043125e3d09fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@tanstack/angular-query-experimental Minor
@tanstack/angular-query-persist-client Major

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

changeset-bot[bot] avatar Oct 26 '25 01:10 changeset-bot[bot]

[!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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 54aa976c3d16f9e4d2b8f437a784b8a19cad31c7 and 5da0ab7a3c6de3e0cd014d2a45c043125e3d09fb.

📒 Files selected for processing (3)
  • .changeset/puny-melons-deny.md (1 hunks)
  • packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts (6 hunks)
  • packages/angular-query-experimental/src/inject-mutation.ts (6 hunks)

Walkthrough

Reworks 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

Cohort / File(s) Summary
Core mutation injection implementation
packages/angular-query-experimental/src/inject-mutation.ts
Replaces effect-driven signal wiring with a computed-linked result signal, registers DestroyRef cleanup, uses ngZone.runOutsideAngular for observer subscriptions, manages pending tasks and cleanup, consolidates result propagation, and retains public API signatures.
Test suite updates
packages/angular-query-experimental/src/__tests__/inject-mutation.test.ts
Removes TestBed.tick() usage; synchronizes with fake timers + app.whenStable() and advanceTimersByTimeAsync; adds test validating pending state when mutation runs in constructor; destructures and asserts status() and error() observables.

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

  • Pay attention to signal composition and reactivity correctness in inject-mutation.ts.
  • Verify DestroyRef teardown and ngZone subscription boundaries (runOutsideAngular vs run).
  • Validate pending-task lifecycle and test timing changes in inject-mutation.test.ts.

Possibly related PRs

  • TanStack/query#9666 — touches inject-mutation.ts and tests with overlapping lifecycle / pending-task handling changes.

Suggested reviewers

  • arnoud-dv

Poem

🐰 In constructor's dusk a mutation stirred,

Pending was hidden — no longer blurred.
DestroyRef tended the signal's bloom,
ngZone queued calm, escaped the gloom.
Hooray — tests now wait, then celebrate the tune.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix(angular-query): ensure initial mutation pending state is emitted" is clear, concise, and directly summarizes the main objective of the PR. It uses the conventional "fix" prefix, specifies the affected package (angular-query), and precisely describes what was fixed (ensuring pending state is emitted in early lifecycle hooks). The title accurately reflects the core problem being addressed in the changeset—that mutations triggered in constructors or ngOnInit were not showing their pending state.
Linked Issues Check ✅ Passed The code changes directly address the requirements from linked issue #9020. The implementation adds a new test ("should have pending state when mutating in constructor") that reproduces and validates the exact scenario described in the issue—mutations triggered in component constructors and ngOnInit now properly emit the pending state. The main fix replaces the effect-based subscription pattern (which was delayed relative to the observer's initial emit) with an immediate subscription via linkedResultSignal, ensuring the initial pending state is captured. The refactoring of the subscription architecture, pending task management, and destroy cleanup all support fixing the core issue where mutations were incorrectly appearing as idle when triggered in early lifecycle hooks.
Out of Scope Changes Check ✅ Passed All code changes in this PR are scoped to addressing the core issue in #9020. The test additions directly validate the fix for mutations in constructors and early lifecycle hooks; the replacement of effect-based subscriptions with an immediate subscription pattern fixes the root cause; pending task management and destroy cleanup support proper lifecycle handling; and adjustments to timer synchronization in tests are necessary to validate the new behavior. While the signal architecture refactoring is comprehensive, it is justified by the need to remove the effect-based delay that caused the initial pending state to be missed. No changes appear to address unrelated concerns or introduce functionality outside the scope of fixing the pending state emission issue.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed The pull request description includes all required sections from the template: the 🎯 Changes section clearly explains the issue and fix with reference to the specific bug report; the ✅ Checklist section shows both items properly marked as completed, confirming the author followed the Contributing guide and tested the code locally; and the 🚀 Release Impact section correctly indicates this affects published code and a changeset has been generated. The description is well-structured, provides sufficient detail about the problem (effect timing issue in constructor/ngOnInit), the solution (replacing effects with computed signals), and all required information is present and complete.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 26 '25 01:10 coderabbitai[bot]

Hi @arnoud-dv, resolved conflicts and aligned my changes with your approach of mimicking linked signal of #9808.

I would appreciate if you find some time to review my proposed changes.

ThiloAschebrock avatar Oct 26 '25 02:10 ThiloAschebrock