fix: falsy queue items, AsyncRetryer memory leak, wait bypass, and Awaited return types - #246
Merged
Merged
Conversation
…Awaited return types Closes #200, #198, #188, #156 Co-Authored-By: Simon Meyer <33751665+simonmeyerrr@users.noreply.github.com> Co-Authored-By: Calum Jarvis <92122419+caluhm@users.noreply.github.com> Co-Authored-By: Vincent Ricard <vincent.ricard@albertapp.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 (13)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe PR fixes ChangesAsync utility behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pacer/src/async-queuer.ts (1)
434-475: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTrack active executions by identity.
Line 444 stores only the item value in
activeItems. Theexecutecleanup removes every active entry equal to the completed item.If two equal items execute concurrently, such as two
0values, the first completion removes both entries. A later tick can then exceedconcurrency.Track each scheduled execution with a unique identity. Remove only the matching execution when it settles. Add a concurrency test with duplicate primitive values.
🤖 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 `@packages/pacer/src/async-queuer.ts` around lines 434 - 475, Update the scheduling flow around peekNextItem, activeItems, and execute so each scheduled execution has a unique identity rather than tracking only the item value. Ensure settlement cleanup removes only the matching execution, preserving accurate concurrency when duplicate primitive items run concurrently. Add a test covering duplicate values such as two 0 items with concurrent execution.
🤖 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 `@docs/framework/preact/reference/functions/useAsyncThrottledCallback.md`:
- Around line 22-23: Update the return-value description for
useAsyncThrottledCallback to state that disabled calls resolve with undefined,
while throttled calls using trailing: true schedule a trailing execution whose
promise resolves or rejects with the callback result. Remove the claim that
every throttled call resolves with undefined and document the trailing behavior
explicitly.
In `@packages/pacer/src/async-debouncer.ts`:
- Around line 375-380: The async debouncer flow around
currentAsyncRetryer.execute must distinguish a successful result from undefined
returned for disabled or non-throwing retry failures. Only update
lastResult/successCount and invoke onSuccess when execution actually succeeds;
otherwise preserve the failure path and avoid passing undefined to onSuccess.
Add coverage for a rejected callback with asyncRetryerOptions.throwOnError set
to false.
In `@packages/pacer/src/async-queuer.ts`:
- Around line 676-684: Restore the tick chain for every flush outcome in
AsyncQueuer: at packages/pacer/src/async-queuer.ts:676-684, use
Promise.allSettled for direct execute calls, restore pendingTick and restart
processing before re-throwing any captured failure; at
packages/pacer/src/async-queuer.ts:694-697, move state restoration into finally
and call `#tick`() when the queuer is running and items were added while
batchFunction awaited.
---
Outside diff comments:
In `@packages/pacer/src/async-queuer.ts`:
- Around line 434-475: Update the scheduling flow around peekNextItem,
activeItems, and execute so each scheduled execution has a unique identity
rather than tracking only the item value. Ensure settlement cleanup removes only
the matching execution, preserving accurate concurrency when duplicate primitive
items run concurrently. Add a test covering duplicate values such as two 0 items
with concurrent execution.
🪄 Autofix
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 Plus
Run ID: d04db3b5-7d17-436c-adb5-315c6854972f
📒 Files selected for processing (47)
.changeset/async-queuer-falsy-items.md.changeset/async-queuer-pending-tick.md.changeset/awaited-return-types.md.changeset/retryer-key-guard.mddocs/framework/preact/reference/functions/useAsyncDebouncedCallback.mddocs/framework/preact/reference/functions/useAsyncRateLimitedCallback.mddocs/framework/preact/reference/functions/useAsyncThrottledCallback.mddocs/framework/react/reference/functions/useAsyncDebouncedCallback.mddocs/framework/react/reference/functions/useAsyncRateLimitedCallback.mddocs/framework/react/reference/functions/useAsyncThrottledCallback.mddocs/reference/classes/AsyncDebouncer.mddocs/reference/classes/AsyncQueuer.mddocs/reference/classes/AsyncRateLimiter.mddocs/reference/classes/AsyncRetryer.mddocs/reference/classes/AsyncThrottler.mddocs/reference/functions/asyncDebounce.mddocs/reference/functions/asyncQueue.mddocs/reference/functions/asyncRateLimit.mddocs/reference/functions/asyncRetry.mddocs/reference/functions/asyncRetryerOptions.mddocs/reference/functions/asyncThrottle.mddocs/reference/interfaces/AsyncDebouncerOptions.mddocs/reference/interfaces/AsyncDebouncerState.mddocs/reference/interfaces/AsyncRateLimiterOptions.mddocs/reference/interfaces/AsyncRateLimiterState.mddocs/reference/interfaces/AsyncRetryerOptions.mddocs/reference/interfaces/AsyncRetryerState.mddocs/reference/interfaces/AsyncThrottlerOptions.mddocs/reference/interfaces/AsyncThrottlerState.mdpackages/pacer/src/async-debouncer.tspackages/pacer/src/async-queuer.tspackages/pacer/src/async-rate-limiter.tspackages/pacer/src/async-retryer.tspackages/pacer/src/async-throttler.tspackages/pacer/src/queuer.tspackages/pacer/tests/async-debouncer.test.tspackages/pacer/tests/async-queuer.test.tspackages/pacer/tests/async-rate-limiter.test.tspackages/pacer/tests/async-retryer.test.tspackages/pacer/tests/async-throttler.test.tspackages/pacer/tests/queuer.test.tspackages/preact-pacer/src/async-debouncer/useAsyncDebouncedCallback.tspackages/preact-pacer/src/async-rate-limiter/useAsyncRateLimitedCallback.tspackages/preact-pacer/src/async-throttler/useAsyncThrottledCallback.tspackages/react-pacer/src/async-debouncer/useAsyncDebouncedCallback.tspackages/react-pacer/src/async-rate-limiter/useAsyncRateLimitedCallback.tspackages/react-pacer/src/async-throttler/useAsyncThrottledCallback.ts
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness and ergonomics issues across TanStack Pacer’s async utilities and framework adapters, focusing on queue item handling, wait semantics, memory usage, and TypeScript return types.
Changes:
- Fix AsyncQueuer/Queuer item handling so falsy values are not skipped and
nullno longer crashes default priority handling. - Remove AsyncRetryer’s devtools event-client integration to prevent unbounded listener/queued-event memory growth from per-execution retryers.
- Align async utility return types (and react/preact hooks) to
Promise<Awaited<ReturnType<TFn>> | undefined>and update docs/tests accordingly.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-pacer/src/async-throttler/useAsyncThrottledCallback.ts | Update hook return type to awaited + undefined and simplify callback wrapper. |
| packages/react-pacer/src/async-rate-limiter/useAsyncRateLimitedCallback.ts | Update hook return type to awaited + undefined. |
| packages/react-pacer/src/async-debouncer/useAsyncDebouncedCallback.ts | Update hook return type to awaited + undefined and simplify callback wrapper. |
| packages/preact-pacer/src/async-throttler/useAsyncThrottledCallback.ts | Update hook return type to awaited + undefined. |
| packages/preact-pacer/src/async-rate-limiter/useAsyncRateLimitedCallback.ts | Update hook return type to awaited + undefined. |
| packages/preact-pacer/src/async-debouncer/useAsyncDebouncedCallback.ts | Update hook return type to awaited + undefined. |
| packages/pacer/tests/queuer.test.ts | Add regression test for null items in sync Queuer. |
| packages/pacer/tests/async-throttler.test.ts | Add devtools-registry and type-inference tests for AsyncThrottler. |
| packages/pacer/tests/async-retryer.test.ts | Add test ensuring AsyncRetryer does not register in devtools registry. |
| packages/pacer/tests/async-rate-limiter.test.ts | Add devtools-registry and type-inference tests for AsyncRateLimiter. |
| packages/pacer/tests/async-queuer.test.ts | Add tests for falsy/null items, wait semantics, error continuity, and devtools registry behavior. |
| packages/pacer/tests/async-debouncer.test.ts | Add devtools-registry and type-inference tests for AsyncDebouncer. |
| packages/pacer/src/queuer.ts | Make default priority access null-safe for null items. |
| packages/pacer/src/async-throttler.ts | Use Awaited<ReturnType<TFn>> in state/returns and stop keying internal retryers for devtools. |
| packages/pacer/src/async-retryer.ts | Remove devtools event-client integration; clarify key as identifier-only. |
| packages/pacer/src/async-rate-limiter.ts | Use Awaited<ReturnType<TFn>> in state/returns and stop keying internal retryers for devtools. |
| packages/pacer/src/async-queuer.ts | Fix falsy-item peeking, wait/pendingTick behavior, error swallowing in tick chain, null-safe priority access, and flush chain restart. |
| packages/pacer/src/async-debouncer.ts | Use Awaited<ReturnType<TFn>> in state/returns and stop keying internal retryers for devtools. |
| docs/reference/interfaces/AsyncThrottlerState.md | Update lastResult docs to awaited type. |
| docs/reference/interfaces/AsyncThrottlerOptions.md | Update onSuccess result type docs to awaited type. |
| docs/reference/interfaces/AsyncRetryerState.md | Update source line references due to code changes. |
| docs/reference/interfaces/AsyncRetryerOptions.md | Update retryer key documentation and source references. |
| docs/reference/interfaces/AsyncRateLimiterState.md | Update lastResult docs to awaited type. |
| docs/reference/interfaces/AsyncRateLimiterOptions.md | Update onSuccess result type docs to awaited type. |
| docs/reference/interfaces/AsyncDebouncerState.md | Update lastResult docs to awaited type. |
| docs/reference/interfaces/AsyncDebouncerOptions.md | Update onSuccess result type docs to awaited type. |
| docs/reference/functions/asyncThrottle.md | Update helper return type docs to awaited type and source refs. |
| docs/reference/functions/asyncRetryerOptions.md | Update source refs. |
| docs/reference/functions/asyncRetry.md | Update source refs. |
| docs/reference/functions/asyncRateLimit.md | Update helper return type docs to awaited type. |
| docs/reference/functions/asyncQueue.md | Update source refs. |
| docs/reference/functions/asyncDebounce.md | Update helper return type docs to awaited type and source refs. |
| docs/reference/classes/AsyncThrottler.md | Update method return type docs + source refs. |
| docs/reference/classes/AsyncRetryer.md | Update source refs after devtools integration removal. |
| docs/reference/classes/AsyncRateLimiter.md | Update method return type docs. |
| docs/reference/classes/AsyncQueuer.md | Update source refs after tick/flush changes. |
| docs/reference/classes/AsyncDebouncer.md | Update method return type docs + source refs. |
| docs/framework/react/reference/functions/useAsyncThrottledCallback.md | Update hook signature and description to include undefined. |
| docs/framework/react/reference/functions/useAsyncRateLimitedCallback.md | Update hook signature to include undefined. |
| docs/framework/react/reference/functions/useAsyncDebouncedCallback.md | Update hook signature and description to include undefined. |
| docs/framework/preact/reference/functions/useAsyncThrottledCallback.md | Update hook signature and description to include undefined. |
| docs/framework/preact/reference/functions/useAsyncRateLimitedCallback.md | Update hook signature to include undefined. |
| docs/framework/preact/reference/functions/useAsyncDebouncedCallback.md | Update hook signature and description to include undefined. |
| .changeset/retryer-key-guard.md | Changeset for removing retryer devtools event-client integration to fix memory growth. |
| .changeset/awaited-return-types.md | Changeset for awaited return types across async utilities + react/preact adapters. |
| .changeset/async-queuer-pending-tick.md | Changeset for AsyncQueuer wait/pendingTick semantics and chain resilience. |
| .changeset/async-queuer-falsy-items.md | Changeset for falsy/null item handling in Queuer/AsyncQueuer. |
Suppressed comments (1)
packages/pacer/src/async-queuer.ts:698
flushAsBatch()clears timeouts (killing the tick chain) and only setspendingTick: falseafter awaitingbatchFunction, but it never restarts#tick()if items are enqueued during that await. IfpendingTickwas true whenflushAsBatch()started (common when a wait timer was pending), anaddItem()during the batch will not trigger#tick(), and after the batch completes the queue can remain stuck until another item is added orstart()is called again.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
View your CI Pipeline Execution ↗ for commit 4ab7a8f
☁️ Nx Cloud last updated this comment at |
…and callback docs Addresses review feedback: flush()/flushAsBatch() now restore the tick chain even when a task or batch function rejects (flush uses allSettled and rethrows after state restoration, with remaining items resuming at the normal wait spacing); execute() removes a single activeItems occurrence so duplicate item values cannot exceed the concurrency limit; react/preact async callback docstrings no longer claim throttled/superseded calls resolve with undefined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Addressed the CodeRabbit review in aaf064b:
New coverage: 9 tests around flush robustness (rejection, partial flush, batch rejection, stopped queuer, mid-batch adds) and duplicate-item concurrency accounting. 551 tests passing. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/pacer/src/async-queuer.ts (2)
435-447: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCopy
activeItemsbefore mutation.Line 435 takes the array reference from store state. Line 444 pushes into that same array, so the store state is mutated in place before
#setStateruns. Subscribers that compareactiveItemsby reference cannot detect the change, and the state beforesetStateis already dirty.executeat lines 657-663 correctly copies before mutating; use the same pattern here.♻️ Proposed change
- const activeItems = this.store.state.activeItems + const activeItems = [...this.store.state.activeItems] while ( activeItems.length < this.#getConcurrency() && this.store.state.items.length > 0 ) { const nextItem = this.peekNextItem() if (nextItem === undefined) { break } activeItems.push(nextItem) this.#setState({ - activeItems, + activeItems: [...activeItems], })🤖 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 `@packages/pacer/src/async-queuer.ts` around lines 435 - 447, Copy this.store.state.activeItems into a new array before the while loop in the active-item scheduling flow, then mutate that copy and pass it to `#setState`. Follow the existing copy-before-mutation pattern used by execute, while preserving the current concurrency and next-item checks.
717-727: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider matching the wait spacing that
flushapplies.
flushrestarts the chain through asetTimeoutwhenwait > 0(lines 689-700).flushAsBatchcalls#tick()directly, so the first remaining item runs with no wait spacing after the batch settles. Align both paths if the wait contract must hold after every flush variant.🤖 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 `@packages/pacer/src/async-queuer.ts` around lines 717 - 727, Update flushAsBatch’s finally block to restart the tick chain using the same wait-aware scheduling behavior as flush, rather than calling `#tick`() directly. Reuse the existing wait handling from flush while preserving the current running-state and non-empty-items checks.packages/pacer/tests/async-queuer.test.ts (1)
1459-1471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen this test so it verifies the
finallyrestart.
flushAsBatchdoes not setpendingTickbefore it awaits. Line 1464 callsstart(), which seespendingTick === falseand starts the tick chain right away.'late'therefore processes before the batch settles, and the assertion passes even if thefinallyblock at lines 721-727 is removed. Assert the intermediate state, or drive the item addition after the queuer is already running.💚 Proposed test tightening
asyncQueuer.addItem('late') asyncQueuer.start() + // 'late' must not be processed until the batch settles and the chain restarts + expect(results).toEqual([]) + await vi.advanceTimersByTimeAsync(50) await batchPromise await vi.advanceTimersByTimeAsync(100)🤖 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 `@packages/pacer/tests/async-queuer.test.ts` around lines 1459 - 1471, Strengthen the flushAsBatch test around asyncQueuer.flushAsBatch, asyncQueuer.addItem, and asyncQueuer.start so it proves the queued “late” item is processed only after the batch promise settles and the finally-based restart runs. Assert the intermediate batches/results state before awaiting batchPromise, or add the item after the queuer is already running, while preserving the final expected batch and result assertions.
🤖 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.
Nitpick comments:
In `@packages/pacer/src/async-queuer.ts`:
- Around line 435-447: Copy this.store.state.activeItems into a new array before
the while loop in the active-item scheduling flow, then mutate that copy and
pass it to `#setState`. Follow the existing copy-before-mutation pattern used by
execute, while preserving the current concurrency and next-item checks.
- Around line 717-727: Update flushAsBatch’s finally block to restart the tick
chain using the same wait-aware scheduling behavior as flush, rather than
calling `#tick`() directly. Reuse the existing wait handling from flush while
preserving the current running-state and non-empty-items checks.
In `@packages/pacer/tests/async-queuer.test.ts`:
- Around line 1459-1471: Strengthen the flushAsBatch test around
asyncQueuer.flushAsBatch, asyncQueuer.addItem, and asyncQueuer.start so it
proves the queued “late” item is processed only after the batch promise settles
and the finally-based restart runs. Assert the intermediate batches/results
state before awaiting batchPromise, or add the item after the queuer is already
running, while preserving the final expected batch and result assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee971725-03c6-4f5f-a7ef-ceb4f55b4372
📒 Files selected for processing (12)
docs/framework/preact/reference/functions/useAsyncDebouncedCallback.mddocs/framework/preact/reference/functions/useAsyncThrottledCallback.mddocs/framework/react/reference/functions/useAsyncDebouncedCallback.mddocs/framework/react/reference/functions/useAsyncThrottledCallback.mddocs/reference/classes/AsyncQueuer.mddocs/reference/functions/asyncQueue.mdpackages/pacer/src/async-queuer.tspackages/pacer/tests/async-queuer.test.tspackages/preact-pacer/src/async-debouncer/useAsyncDebouncedCallback.tspackages/preact-pacer/src/async-throttler/useAsyncThrottledCallback.tspackages/react-pacer/src/async-debouncer/useAsyncDebouncedCallback.tspackages/react-pacer/src/async-throttler/useAsyncThrottledCallback.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/preact-pacer/src/async-debouncer/useAsyncDebouncedCallback.ts
- packages/react-pacer/src/async-throttler/useAsyncThrottledCallback.ts
- packages/preact-pacer/src/async-throttler/useAsyncThrottledCallback.ts
- packages/react-pacer/src/async-debouncer/useAsyncDebouncedCallback.ts
- docs/reference/functions/asyncQueue.md
- docs/framework/react/reference/functions/useAsyncThrottledCallback.md
- docs/reference/classes/AsyncQueuer.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/pacer/src/async-queuer.ts:666
isExecutingis always set tofalsewhen an execution settles, even if other executions are still in-flight (e.g. concurrency > 1 or a manualexecute/flushcall overlapping). This makesstore.state.isExecutinginaccurate for consumers.
undefined is the internal "no item" sentinel used by peekNextItem/getNextItem, so an enqueued undefined item would wedge the processing loop and block every item behind it. addItem now rejects undefined through the standard rejection path (rejectionCount + onReject + return false) in both queuers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated no new comments.
Suppressed comments (5)
packages/pacer/tests/async-throttler.test.ts:1028
- This test enables fake timers via
vi.useFakeTimers()in the describe-levelbeforeEach, but never restores real timers. That can leak fake-timer state into subsequent tests in this file and cause hangs/flakiness. Ensure timers are restored (ideally viatry/finallyin the test body or anafterEachthat callsvi.useRealTimers()).
packages/preact-pacer/src/async-throttler/useAsyncThrottledCallback.ts:16 - The JSDoc claims that calls made during the throttle wait window “share the trailing execution's result”. In
AsyncThrottler.maybeExecute, each new call resolves the previous pending promise immediately via#resolvePreviousPromiseInternal()(with the most recentlastResult), so only the most recent call can resolve with the trailing execution’s result. The docs should reflect the actual promise resolution behavior.
docs/framework/react/reference/functions/useAsyncThrottledCallback.md:23 - This reference doc says calls during the wait period “share the trailing execution's result”, but
AsyncThrottler.maybeExecuteresolves the previous pending promise immediately on each new call (with the most recentlastResult). Only the most recent call can resolve with the trailing execution’s result, so this description should be updated.
regardless of how many times it is called. Calls made during the wait period can schedule a
single trailing execution with the latest arguments when `trailing` is enabled (the default),
and those calls share the trailing execution's result. The returned function always returns
a promise that resolves or rejects with the result of the original async function, and
resolves with `undefined` when the throttler is disabled.
docs/framework/preact/reference/functions/useAsyncThrottledCallback.md:23
- This reference doc says calls during the wait period “share the trailing execution's result”, but
AsyncThrottler.maybeExecuteresolves the previous pending promise immediately on each new call (with the most recentlastResult). Only the most recent call can resolve with the trailing execution’s result, so this description should be updated.
regardless of how many times it is called. Calls made during the wait period can schedule a
single trailing execution with the latest arguments when `trailing` is enabled (the default),
and those calls share the trailing execution's result. The returned function always returns
a promise that resolves or rejects with the result of the original async function, and
resolves with `undefined` when the throttler is disabled.
packages/react-pacer/src/async-throttler/useAsyncThrottledCallback.ts:16
- The JSDoc claims that calls made during the throttle wait window “share the trailing execution's result”. In
AsyncThrottler.maybeExecute, each new call resolves the previous pending promise immediately via#resolvePreviousPromiseInternal()(with the most recentlastResult), so only the most recent call can resolve with the trailing execution’s result. The docs should reflect the actual promise resolution behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/pacer/src/queuer.ts`:
- Line 393: Update the addItem() return documentation in
packages/pacer/src/queuer.ts near lines 393-393 to state that it returns false
when the queue is full or when the item is undefined; regenerate or apply the
same return-contract update in docs/reference/classes/Queuer.md lines 174-177,
docs/framework/angular/reference/interfaces/QueuedSignal.md lines 41-42, and
docs/reference/functions/queue.md lines 73-74.
🪄 Autofix
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 Plus
Run ID: e13d8ea1-3350-4228-ac4d-0023a767e512
📒 Files selected for processing (10)
docs/framework/angular/reference/interfaces/AsyncQueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedSignal.mddocs/reference/classes/AsyncQueuer.mddocs/reference/classes/Queuer.mddocs/reference/functions/asyncQueue.mddocs/reference/functions/queue.mdpackages/pacer/src/async-queuer.tspackages/pacer/src/queuer.tspackages/pacer/tests/async-queuer.test.tspackages/pacer/tests/queuer.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/reference/functions/asyncQueue.md
- packages/pacer/src/async-queuer.ts
- packages/pacer/tests/queuer.test.ts
|
|
||
| /** | ||
| * Adds an item to the queue. If the queue is full, the item is rejected and onReject is called. | ||
| * `undefined` cannot be queued (it is the internal "no item" sentinel) and is always rejected. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the addItem() rejection documentation.
The new undefined branch returns false when the queue is not full. Update the source JSDoc and regenerate the affected references so the return contract includes both full-queue rejection and undefined rejection.
packages/pacer/src/queuer.ts#L393-L393: update the return description near Line 396.docs/reference/classes/Queuer.md#L174-L177: update the return description near Line 180.docs/framework/angular/reference/interfaces/QueuedSignal.md#L41-L42: update the return description near Line 45.docs/reference/functions/queue.md#L73-L74: update the return description near Line 77.
📍 Affects 4 files
packages/pacer/src/queuer.ts#L393-L393(this comment)docs/reference/classes/Queuer.md#L174-L177docs/framework/angular/reference/interfaces/QueuedSignal.md#L41-L42docs/reference/functions/queue.md#L73-L74
🤖 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 `@packages/pacer/src/queuer.ts` at line 393, Update the addItem() return
documentation in packages/pacer/src/queuer.ts near lines 393-393 to state that
it returns false when the queue is full or when the item is undefined;
regenerate or apply the same return-contract update in
docs/reference/classes/Queuer.md lines 174-177,
docs/framework/angular/reference/interfaces/QueuedSignal.md lines 41-42, and
docs/reference/functions/queue.md lines 73-74.
…executions, and throttled callback docs #tick now copies activeItems before mutating so store state is never dirtied in place and array-reference selectors observe changes. AsyncQueuer and AsyncRateLimiter keep isExecuting true until every overlapping execution settles (derived from the live retryer count) instead of clearing it when the first one finishes. The flushAsBatch mid-batch test now pins the queue via pendingTick so it genuinely exercises the finally-restart. Throttled callback docstrings now describe the actual promise resolution: only the most recent call gets the trailing result; earlier calls resolve with the previous result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Addressed the latest CodeRabbit nitpicks and Copilot suppressed comments in edb6b61 (these were review-body comments, not resolvable threads): Fixed
Declined
555 tests passing. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/pacer/src/queuer.ts:436
- When
getPriorityis left at its default,addItembypassesdefaultOptions.getPriority(which defaults missing/nullish priorities to0) and instead reads(item as any)?.prioritydirectly. This changes semantics: items with nopriority(ornull) end up withpriority === undefined, skipping the priority-insertion branch and potentially ordering incorrectly vs negative/zero priorities. Align the fast-path withdefaultOptions.getPriorityby applying?? 0(and same forexistingPriority).
packages/pacer/src/async-queuer.ts:522 - Like
Queuer.addItem, the defaultgetPriorityisitem?.priority ?? 0, but the fast-path here reads(item as any)?.prioritydirectly whengetPriorityis unchanged. That meansnullitems (and items with missingpriority) getpriority === undefinedinstead of0, which can skip priority insertion or mis-order items compared to negative/zero priorities. Use?? 0to match the documented default behavior (and apply the same change toexistingPriority).
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@tanstack/pacer
0,'',false) are no longer silently skipped by the processing loop (=== undefinedcheck), andaddItem(null)no longer throws reading.priority. Closes AsyncQueuer silently drops falsy queue items (0, "", false, null) #200"<key>-retryer-N", or"undefined-retryer-N"with no key), and queued devtools events — unbounded memory growth, worst in Node. No devtools package ever consumed retryer events, so no functionality is lost; parents now passasyncRetryerOptionsthrough unmodified andkeyremains a plain identifier. Closes AsyncQueuer leaks memory in Node.js via unintended retryer key propagation #198pendingTicknow staystruewhile executions or wait timers are pending (matching the sync Queuer), soaddItem()during thewaitwindow no longer triggers immediate processing. Task errors no longer kill the processing chain or emit unhandled rejections, andflush/flushAsBatchrestart the tick chain they interrupt. Closes Bug: AsyncQueuer addItem() bypasses wait period when called during active processing #188maybeExecute,flush,lastResultstate, andonSuccess(plus theasyncDebounce/asyncThrottle/asyncRateLimithelpers) now useAwaited<ReturnType<TFn>>instead of double-wrapping promises. Closes Incorrect inferred type for the async functions #156@tanstack/react-pacer, @tanstack/preact-pacer
useAsyncDebouncedCallback,useAsyncThrottledCallback, anduseAsyncRateLimitedCallbacknow returnPromise<Awaited<ReturnType<TFn>> | undefined>, matching the angular adapter and actual runtime behavior.Builds on the diagnoses in #201, #199, #189, and #157 — authors are co-credited on the commit. #189's approach needed extension: without error handling in the tick chain and
pendingTickreconciliation inflush, the queue could deadlock.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
undefinedqueue items with appropriate rejection handling.undefinedwhen applicable.Documentation
undefinedvalues.