[refactor!]: removed protected abortController + _handleDestory methods…#82
[refactor!]: removed protected abortController + _handleDestory methods…#82js2me wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR refactors the destroyable lifecycle in mobx-tanstack-query, removing the LinkedAbortController dependency and replacing it with direct AbortSignal handling. The protected ChangesDestroyable Lifecycle Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/base-query.ts (1)
585-601:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPossible hang in
start()when noabortSignalis supplied and the query is destroyed mid-flight.Previously,
signalwas the (always-defined)abortController.signaland was aborted by the oldhandleDestroy(), which would reject thewhen(...)in-flight. After this refactor,destroy()only setsthis._destroyed = trueand never aborts any signal — andthis._abortSignalisundefinedwhenever the caller didn't pass one inconfig.abortSignal. In that case, callingdestroy()whileresult.isFetchingis true leaves thiswhenwaiting indefinitely (or untilisFetchingbecomes false on its own), so thestart()promise never settles.Consider re-checking
_destroyedinside the predicate so destruction unblocks the wait:🛡️ Proposed fix
if (this.result.isFetching) { - await when(() => !this.result.isFetching, { + await when(() => this._destroyed || !this.result.isFetching, { signal: this._abortSignal, }); + if (this._destroyed) { + return this.result; + } const throwableError = this.getCurrentThrowableError();Alternatively, manage an internal AbortController inside
Destroyable.destroy()and pass its signal here sowhenis reliably cancelled regardless of the consumer-provided signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/base-query.ts` around lines 585 - 601, The wait in start() can hang if destroy() is called without an abort signal because _abortSignal may be undefined; update the when() predicate used in start() (the call that currently awaits when(() => !this.result.isFetching, { signal: this._abortSignal })) to also check this._destroyed (e.g., predicate returns !this.result.isFetching || this._destroyed) so destruction unblocks the wait, and after the wait re-check this._destroyed and handle as appropriate (throw or return) to avoid leaving start() pending; alternatively, implement an internal AbortController in Destroyable.destroy() and expose its signal as this._abortSignal so the existing when(...) is cancelled reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mutation.ts`:
- Around line 199-208: The current tempAc lifecycle races because
recreateTempAc()/abortTempAc() mutate a single shared tempAc used by all
concurrent mutationFn calls; change to create a per-call AbortController inside
the mutationFn wrapper (capture it in a local variable or store it in a WeakMap
keyed by the mutation context/result) and pass that specific controller.signal
to the underlying mutationFn, then ensure the onSettled wrapper only aborts that
same per-call controller instead of calling the global abortTempAc(); keep the
existing behavior when this._abortSignal (config.abortSignal) is provided by
using that shared signal as-is, otherwise always create and own a fresh per-call
controller so overlapping mutations do not cancel each other (refer to
recreateTempAc, abortTempAc, mutationFn, onSettled, tempAc and this._abortSignal
to locate the changes).
In `@src/utils/destroyable.ts`:
- Around line 5-20: The constructor currently ignores pre-aborted signals and
destroy() is not idempotent; update constructor (the class constructor that
assigns this._abortSignal and this._destroyed) to immediately call
this.destroy() if this._abortSignal?.aborted is true, then still add the abort
listener as before, and change destroy() to be guarded (if (this._destroyed)
return) before setting this._destroyed = true so repeated calls (from manual
disposal, Symbol.dispose/using, or the abort listener) are safe; reference the
constructor, this._abortSignal, and destroy() so subclasses like Query,
InfiniteQuery, and Mutation rely on the idempotent super.destroy().
---
Outside diff comments:
In `@src/base-query.ts`:
- Around line 585-601: The wait in start() can hang if destroy() is called
without an abort signal because _abortSignal may be undefined; update the when()
predicate used in start() (the call that currently awaits when(() =>
!this.result.isFetching, { signal: this._abortSignal })) to also check
this._destroyed (e.g., predicate returns !this.result.isFetching ||
this._destroyed) so destruction unblocks the wait, and after the wait re-check
this._destroyed and handle as appropriate (throw or return) to avoid leaving
start() pending; alternatively, implement an internal AbortController in
Destroyable.destroy() and expose its signal as this._abortSignal so the existing
when(...) is cancelled reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e576a5f-ea33-4505-b8c4-228df41435cf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.changeset/refactor-destroyable-internals.mdpackage.jsonsrc/base-query.tssrc/inifinite-query.tssrc/mutation.tssrc/query.tssrc/utils/destroyable.ts
| onSettled: (...args) => { | ||
| this.abortTempAc(); | ||
| return this.mutationOptions.onSettled?.(...args); | ||
| }, | ||
| mutationFn: (variables, context) => | ||
| mutationFn?.(variables, { | ||
| ...context, | ||
| signal: this.abortController.signal, | ||
| signal: this._abortSignal ?? this.recreateTempAc().signal, | ||
| } satisfies MutationFunctionContext), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @tanstack/query-core MutationObserver 5.90.2 support concurrent in-flight mutations from the same observer, or does mutate() supersede a previous in-flight mutation?
💡 Result:
Yes—MutationObserver (and thus @tanstack/query-core’s mutate/mutateAsync behavior) supports concurrent in-flight mutations from the same MutationObserver; calling mutate does not supersede/cancel a previous in-flight mutation. Evidence: The React TanStack Query docs state that “Per default, all mutations run in parallel - even if you invoke.mutate of the same mutation multiple times,” and that to avoid this you must use a scope.id so they run in serial [1][2]. Additionally, the core MutationObserver implementation shows that each mutate call removes the prior observer link, builds/chooses a mutation from the mutation cache, and then executes it; it doesn’t cancel the previous one, it just updates which mutation the observer is attached to [3]. So if you call observer.mutate(x) then observer.mutate(y) while the first is still pending, both mutation executions can be in-flight concurrently; they are not automatically replaced by the latest call. If you want “only one at a time” semantics, use a scope with an id to queue in serial [1][2], otherwise concurrency is the default.
Citations:
- 1: https://tanstack.com/query/latest/docs/framework/react/guides/mutations
- 2: https://tanstack.com/query/v5/docs/framework/react/guides/mutations
- 3: https://github.com/TanStack/query/blob/d4fd9417/packages/query-core/src/mutationObserver.ts
tempAc lifecycle has a cross-mutation race when calls overlap.
@tanstack/query-core's MutationObserver supports concurrent in-flight mutations by default—calling mutate() does not cancel a prior in-flight call. However, this code assumes single-flight semantics and will break under concurrent load:
-
Overlapping mutations cancel each other's signals. Each
mutationFninvocation callsrecreateTempAc(), which aborts the previoustempAc. If the sameMutationinstance has an in-flight call (A) andmutate()is invoked again (B) before A settles, A's signal is aborted by B'srecreateTempAc()even though both are executing concurrently. Then when A eventually settles, the wrappedonSettledcallsabortTempAc()and aborts B's signal — corrupting an unrelated in-flight mutation. -
Asymmetric signal semantics. When
config.abortSignalis provided, all calls share_abortSignalandabortTempAc()becomes a no-op (no per-call cancellation). When it isn't, a freshtempAcis created per call. The signal a consumer'smutationFnreceives therefore behaves differently depending on whetherabortSignalwas passed in config — this is observable and surprising.
A safer approach is to track the controller per in-flight call (e.g., capture it in a closure inside mutationFn and pass that exact reference to onSettled via a WeakMap keyed on the result/context, or wrap mutationFn itself so each invocation owns its own controller and aborts only that one on settle).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mutation.ts` around lines 199 - 208, The current tempAc lifecycle races
because recreateTempAc()/abortTempAc() mutate a single shared tempAc used by all
concurrent mutationFn calls; change to create a per-call AbortController inside
the mutationFn wrapper (capture it in a local variable or store it in a WeakMap
keyed by the mutation context/result) and pass that specific controller.signal
to the underlying mutationFn, then ensure the onSettled wrapper only aborts that
same per-call controller instead of calling the global abortTempAc(); keep the
existing behavior when this._abortSignal (config.abortSignal) is provided by
using that shared signal as-is, otherwise always create and own a fresh per-call
controller so overlapping mutations do not cancel each other (refer to
recreateTempAc, abortTempAc, mutationFn, onSettled, tempAc and this._abortSignal
to locate the changes).
| constructor(abortSignal?: AbortSignal) { | ||
| this.abortController = new LinkedAbortController(abortSignal); | ||
|
|
||
| action(this, 'handleDestroy'); | ||
| makeObservable(this); | ||
|
|
||
| this.abortController.signal.addEventListener('abort', () => { | ||
| this.handleDestroy(); | ||
| }); | ||
| this._abortSignal = abortSignal; | ||
| this._destroyed = false; | ||
|
|
||
| this._abortSignal?.addEventListener( | ||
| 'abort', | ||
| () => { | ||
| this.destroy(); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| } | ||
|
|
||
| destroy() { | ||
| this.abortController?.abort(); | ||
| this._destroyed = true; | ||
| } |
There was a problem hiding this comment.
Two lifecycle edge cases worth handling explicitly.
-
Pre-aborted signal is silently ignored. Per the DOM spec,
addEventListener('abort', ...)on an already-abortedAbortSignaldoes not fire. So if a consumer passes a signal that's already aborted tonew Query({ abortSignal }), the instance is never marked_destroyedand never tears down. -
Double
destroy()invocation. Ifdestroy()is called manually (e.g. viausing/Symbol.dispose, or a subclass's own teardown path) and the signal later aborts, theonce: truelistener still fires and re-entersdestroy(). Subclasses (Query,InfiniteQuery,Mutation) re-run cleanup and re-fireonQueryDestroy/onInfiniteQueryDestroy/onMutationDestroyhooks.
🛡️ Proposed fix — guard idempotency and handle pre-aborted signals
constructor(abortSignal?: AbortSignal) {
this._abortSignal = abortSignal;
this._destroyed = false;
- this._abortSignal?.addEventListener(
- 'abort',
- () => {
- this.destroy();
- },
- { once: true },
- );
+ if (abortSignal?.aborted) {
+ // Defer so subclass constructors finish before destroy() runs.
+ queueMicrotask(() => this.destroy());
+ } else {
+ abortSignal?.addEventListener(
+ 'abort',
+ () => this.destroy(),
+ { once: true },
+ );
+ }
}
destroy() {
+ if (this._destroyed) return;
this._destroyed = true;
}The early-return in destroy() makes subclass overrides naturally idempotent (each one starts with super.destroy()).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(abortSignal?: AbortSignal) { | |
| this.abortController = new LinkedAbortController(abortSignal); | |
| action(this, 'handleDestroy'); | |
| makeObservable(this); | |
| this.abortController.signal.addEventListener('abort', () => { | |
| this.handleDestroy(); | |
| }); | |
| this._abortSignal = abortSignal; | |
| this._destroyed = false; | |
| this._abortSignal?.addEventListener( | |
| 'abort', | |
| () => { | |
| this.destroy(); | |
| }, | |
| { once: true }, | |
| ); | |
| } | |
| destroy() { | |
| this.abortController?.abort(); | |
| this._destroyed = true; | |
| } | |
| constructor(abortSignal?: AbortSignal) { | |
| this._abortSignal = abortSignal; | |
| this._destroyed = false; | |
| if (abortSignal?.aborted) { | |
| // Defer so subclass constructors finish before destroy() runs. | |
| queueMicrotask(() => this.destroy()); | |
| } else { | |
| abortSignal?.addEventListener( | |
| 'abort', | |
| () => this.destroy(), | |
| { once: true }, | |
| ); | |
| } | |
| } | |
| destroy() { | |
| if (this._destroyed) return; | |
| this._destroyed = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/destroyable.ts` around lines 5 - 20, The constructor currently
ignores pre-aborted signals and destroy() is not idempotent; update constructor
(the class constructor that assigns this._abortSignal and this._destroyed) to
immediately call this.destroy() if this._abortSignal?.aborted is true, then
still add the abort listener as before, and change destroy() to be guarded (if
(this._destroyed) return) before setting this._destroyed = true so repeated
calls (from manual disposal, Symbol.dispose/using, or the abort listener) are
safe; reference the constructor, this._abortSignal, and destroy() so subclasses
like Query, InfiniteQuery, and Mutation rely on the idempotent super.destroy().
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/base-query.ts">
<violation number="1" location="src/base-query.ts:590">
P1: `this._abortSignal` can be `undefined` when no external abort signal is provided to the constructor. This means `when()` won't be cancelled on `destroy()`, leaving a dangling observation/promise. The mutation class handles this with `this._abortSignal ?? this.recreateTempAc().signal` as a fallback.</violation>
</file>
<file name="src/mutation.ts">
<violation number="1" location="src/mutation.ts:199">
P1: Race condition: when multiple mutations are triggered concurrently (without `_abortSignal`), the options-level `onSettled` from an earlier mutation will abort `tempAc` that belongs to the later in-flight mutation, prematurely signalling abort to it. Consider tracking which `tempAc` instance the `onSettled` should abort (e.g., capture a reference at call time) rather than always aborting the current one.</violation>
</file>
<file name="src/utils/destroyable.ts">
<violation number="1" location="src/utils/destroyable.ts:9">
P2: Two lifecycle edge cases:
1. If a pre-aborted signal is passed, `addEventListener('abort', ...)` will never fire (the event was already dispatched per DOM spec), so the instance is never destroyed automatically.
2. No idempotency guard — if `destroy()` is called manually and the signal later aborts, the listener re-enters `destroy()`, causing subclass cleanup and hooks to fire twice.
Consider checking `abortSignal?.aborted` in the constructor (and deferring via `queueMicrotask` so subclass constructors finish), and adding an early `if (this._destroyed) return;` guard in `destroy()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if (this.result.isFetching) { | ||
| await when(() => !this.result.isFetching, { | ||
| signal: this.abortController.signal, | ||
| signal: this._abortSignal, |
There was a problem hiding this comment.
P1: this._abortSignal can be undefined when no external abort signal is provided to the constructor. This means when() won't be cancelled on destroy(), leaving a dangling observation/promise. The mutation class handles this with this._abortSignal ?? this.recreateTempAc().signal as a fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/base-query.ts, line 590:
<comment>`this._abortSignal` can be `undefined` when no external abort signal is provided to the constructor. This means `when()` won't be cancelled on `destroy()`, leaving a dangling observation/promise. The mutation class handles this with `this._abortSignal ?? this.recreateTempAc().signal` as a fallback.</comment>
<file context>
@@ -587,7 +587,7 @@ export abstract class BaseQuery<
if (this.result.isFetching) {
await when(() => !this.result.isFetching, {
- signal: this.abortController.signal,
+ signal: this._abortSignal,
});
const throwableError = this.getCurrentThrowableError();
</file context>
| // @ts-expect-error | ||
| >(queryClient, { | ||
| ...this.mutationOptions, | ||
| onSettled: (...args) => { |
There was a problem hiding this comment.
P1: Race condition: when multiple mutations are triggered concurrently (without _abortSignal), the options-level onSettled from an earlier mutation will abort tempAc that belongs to the later in-flight mutation, prematurely signalling abort to it. Consider tracking which tempAc instance the onSettled should abort (e.g., capture a reference at call time) rather than always aborting the current one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mutation.ts, line 199:
<comment>Race condition: when multiple mutations are triggered concurrently (without `_abortSignal`), the options-level `onSettled` from an earlier mutation will abort `tempAc` that belongs to the later in-flight mutation, prematurely signalling abort to it. Consider tracking which `tempAc` instance the `onSettled` should abort (e.g., capture a reference at call time) rather than always aborting the current one.</comment>
<file context>
@@ -193,10 +196,14 @@ export class Mutation<
// @ts-expect-error
>(queryClient, {
...this.mutationOptions,
+ onSettled: (...args) => {
+ this.abortTempAc();
+ return this.mutationOptions.onSettled?.(...args);
</file context>
| this._abortSignal = abortSignal; | ||
| this._destroyed = false; | ||
|
|
||
| this._abortSignal?.addEventListener( |
There was a problem hiding this comment.
P2: Two lifecycle edge cases:
- If a pre-aborted signal is passed,
addEventListener('abort', ...)will never fire (the event was already dispatched per DOM spec), so the instance is never destroyed automatically. - No idempotency guard — if
destroy()is called manually and the signal later aborts, the listener re-entersdestroy(), causing subclass cleanup and hooks to fire twice.
Consider checking abortSignal?.aborted in the constructor (and deferring via queueMicrotask so subclass constructors finish), and adding an early if (this._destroyed) return; guard in destroy().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/destroyable.ts, line 9:
<comment>Two lifecycle edge cases:
1. If a pre-aborted signal is passed, `addEventListener('abort', ...)` will never fire (the event was already dispatched per DOM spec), so the instance is never destroyed automatically.
2. No idempotency guard — if `destroy()` is called manually and the signal later aborts, the listener re-enters `destroy()`, causing subclass cleanup and hooks to fire twice.
Consider checking `abortSignal?.aborted` in the constructor (and deferring via `queueMicrotask` so subclass constructors finish), and adding an early `if (this._destroyed) return;` guard in `destroy()`.</comment>
<file context>
@@ -1,26 +1,24 @@
+ this._abortSignal = abortSignal;
+ this._destroyed = false;
+
+ this._abortSignal?.addEventListener(
+ 'abort',
+ () => {
</file context>
1 participant
… in entities
Summary by cubic
Refactored the destroy/abort lifecycle across queries and mutations to remove the internal
abortController, use_abortSignal/_destroyed, and standardize ondestroy()for cleanup. This is a breaking change for subclass implementers and simplifies runtime dependencies.Migration
handleDestroy()overrides withdestroy()and callsuper.destroy()first.this._abortSignal(may be undefined) andthis._destroyedinstead ofabortController.makeObservable(this)in subclass constructors if you rely on MobX annotations.AbortSignalwhen creating the instance; otherwise a temporary one is used internally.Dependencies
linked-abort-controllertodevDependenciesinmobx-tanstack-query.Written for commit 5eab613. Summary will update on new commits.
Summary by CodeRabbit
destroy()withsuper.destroy()call instead of previous lifecycle method