Change race() methods to return PromiseLike instead of Promise#39
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis pull request adds comprehensive test coverage for signal handling, disposer cleanup, and abort behaviors across the allSettled, all, and flow APIs. Production code changes include: lazy initialization of wrap context in BaseExecution; optimization of Promise handling in RunExecution; refactoring race method signatures in SignalController and TimeoutController to return PromiseLike instead of Promise; and early abort detection in resolveWithAbort utility function. These changes enhance test coverage for edge cases while modifying internal implementation details related to asynchronous handling and resource cleanup. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
adelrodriguez
force-pushed
the
03-07-change_race_methods_to_return_promiselike_instead_of_promise_
branch
from
3d554eb to
5b0f1eb
Compare
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b0f1ebcab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/executors/shared.ts (1)
112-121:⚠️ Potential issue | 🟠 MajorReplace synchronous throws with Promise rejections to preserve middleware expectations.
Line 118 throws synchronously in a non-async function returning
Promise<TResult>. Unlikeasyncfunctions, which automatically convert exceptions into rejected promises, synchronous throws here escape as immediate exceptions rather than promise rejections. This breaks the expected behavior for wrap middleware that treatsexecuteCore()as a Promise-returning call.Suggested fix
protected override executeCore(): Promise<TResult> { // Orchestration executors still share outer wraps/cancellation checks even // though their task execution strategies differ. const controlBeforeExecution = this.checkDidControlFail() if (controlBeforeExecution) { - throw controlBeforeExecution + return Promise.reject(controlBeforeExecution) } - return this.executeTasks() + try { + return this.executeTasks() + } catch (error) { + return Promise.reject(error) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/shared.ts` around lines 112 - 121, The synchronous throw in executeCore (calling checkDidControlFail) should be converted to a Promise rejection so callers/middleware see a rejected Promise instead of an immediate exception; replace the throw of controlBeforeExecution with returning Promise.reject(controlBeforeExecution) (or otherwise ensure executeCore always returns a rejected Promise when checkDidControlFail() indicates failure) and then return this.executeTasks() otherwise, keeping references to executeCore, checkDidControlFail, and executeTasks to locate the change.
🧹 Nitpick comments (1)
src/__tests__/all.test.ts (1)
444-458: Fold this into the existing successful disposer test.This covers the same success path as the earlier disposer-cleanup case at Lines 429-442. Moving the
resultassertion into that test would keep the suite smaller without losing coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/all.test.ts` around lines 444 - 458, Remove the duplicate test "allows disposer usage in a successful task without external cancellation" and fold its assertions into the existing successful-disposer test that already registers a disposer via this.$disposer.defer; specifically, in that existing test (the one that verifies disposer cleanup on success) call or reuse the same try$.all invocation, add the expectation expect(result).toEqual({ a: 1 }) immediately after the await, and keep the existing expect(cleaned).toBe(true) assertion so the success path and cleanup are both verified in one test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/executors/shared.ts`:
- Around line 112-121: The synchronous throw in executeCore (calling
checkDidControlFail) should be converted to a Promise rejection so
callers/middleware see a rejected Promise instead of an immediate exception;
replace the throw of controlBeforeExecution with returning
Promise.reject(controlBeforeExecution) (or otherwise ensure executeCore always
returns a rejected Promise when checkDidControlFail() indicates failure) and
then return this.executeTasks() otherwise, keeping references to executeCore,
checkDidControlFail, and executeTasks to locate the change.
---
Nitpick comments:
In `@src/__tests__/all.test.ts`:
- Around line 444-458: Remove the duplicate test "allows disposer usage in a
successful task without external cancellation" and fold its assertions into the
existing successful-disposer test that already registers a disposer via
this.$disposer.defer; specifically, in that existing test (the one that verifies
disposer cleanup on success) call or reuse the same try$.all invocation, add the
expectation expect(result).toEqual({ a: 1 }) immediately after the await, and
keep the existing expect(cleaned).toBe(true) assertion so the success path and
cleanup are both verified in one test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efa630ef-2d7a-4191-aa98-b6ad471dc0c4
📒 Files selected for processing (9)
src/__tests__/all-settled.test.tssrc/__tests__/all.test.tssrc/__tests__/flow.test.tssrc/lib/__tests__/coverage-exceptions.test.tssrc/lib/executors/base.tssrc/lib/executors/run.tssrc/lib/executors/shared.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/executors/shared.ts (1)
112-121:⚠️ Potential issue | 🟠 MajorKeep
executeCore()rejection-based.Line 118 now throws synchronously when cancellation/timeout is already set. Before this change, the method would return a rejected
Promise, allowing code afternext()in wraps to complete before the rejection propagates. Sync throws skip post-next()cleanup in the wrap chain, breaking the observable error propagation contract.🐛 Minimal fix
protected override executeCore(): Promise<TResult> { // Orchestration executors still share outer wraps/cancellation checks even // though their task execution strategies differ. const controlBeforeExecution = this.checkDidControlFail() if (controlBeforeExecution) { - throw controlBeforeExecution + return Promise.reject(controlBeforeExecution) } return this.executeTasks() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/shared.ts` around lines 112 - 121, The method executeCore currently throws synchronously when checkDidControlFail() returns an Error, which bypasses post-next() wrap cleanup; change it to preserve rejection-based behavior by returning a rejected Promise instead of throwing — i.e., in executeCore(), after calling const controlBeforeExecution = this.checkDidControlFail(), if controlBeforeExecution is truthy return Promise.reject(controlBeforeExecution) (referencing executeCore and checkDidControlFail and executeTasks) so wraps that await next() can complete their after-next cleanup before the rejection propagates.
🧹 Nitpick comments (3)
src/lib/executors/base.ts (1)
255-257: Let TypeScript infer the private getter type.
#wrapCtxis private, and inference already gives the correct type here, so the explicit annotation just adds noise.♻️ Suggested cleanup
- get `#wrapCtx`(): WrapCtx { + get `#wrapCtx`() { this.#wrapCtxCache ??= BaseExecution.createWrapContext(this.ctx) return this.#wrapCtxCache }As per coding guidelines,
**/*.{ts,tsx}: Prefer type inference whenever possible in TypeScript; Do not add explicit return types unless required by tooling, declaration emit, or a public API contract; Do not use explicit return types when inference already produces the correct result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/base.ts` around lines 255 - 257, Remove the explicit return type on the private getter get `#wrapCtx`(): WrapCtx and let TypeScript infer the type; modify the getter in the BaseExecution-derived class so it simply declares get `#wrapCtx`() { this.#wrapCtxCache ??= BaseExecution.createWrapContext(this.ctx); return this.#wrapCtxCache; } (keeping references to `#wrapCtxCache`, BaseExecution.createWrapContext and this.ctx intact) to comply with the inference guideline.src/lib/__tests__/coverage-exceptions.test.ts (1)
139-195: Add one bare-thenable regression here.These cases only exercise native
Promiseinstances, but the production change is specifically aboutPromiseLike. A small{ then(...) { ... } }case in this block would catch any remaining Promise-only assumptions inresolveWithAbort()and the updatedrace()paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/coverage-exceptions.test.ts` around lines 139 - 195, Add a bare-thenable variant of the existing Promise tests to exercise PromiseLike handling: create a thenable object (e.g., pendingThenable with a then(resolve) implementation that stores the resolve function), pass it into resolveWithAbort(controller.signal, pendingThenable, createAbortResult) and assert the same behaviors as the Promise tests (returns the resolved value if thenable settles before abort, returns the abort value if abort wins, and ensure the abort factory is called exactly once when abort wins); reuse the same helper names and assertions (resolveWithAbort, controller.abort, and the abort factory counter) so the test sits alongside the existing cases and will catch any Promise-only assumptions in resolveWithAbort and race paths.src/__tests__/all.test.ts (1)
444-458: Consider consolidating with existing test.This test appears to largely duplicate the existing test at lines 429-442 ("runs disposer cleanup after all tasks complete"). Both tests:
- Use a successful task with
this.$disposer.defer()- Verify
cleanedbecomestrue- Do not use external cancellation
The only difference is the additional
expect(result).toEqual({ a: 1 })assertion, which is already covered by numerous other tests in this file.If there's a specific behavioral distinction intended by "allows disposer usage in a successful task without external cancellation," consider adding a clarifying comment or renaming the existing test to better distinguish them. Otherwise, this test could be removed to avoid redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/all.test.ts` around lines 444 - 458, The test "allows disposer usage in a successful task without external cancellation" duplicates the earlier "runs disposer cleanup after all tasks complete" test; either remove this redundant test block or consolidate its additional assertion into the existing test: merge the expect(result).toEqual({ a: 1 }) into the earlier test that uses try$.all and this.$disposer.defer (the test function that sets cleaned and verifies cleanup), or alternatively rename the earlier test to reflect both cleanup and return-value expectations and add a clarifying comment; ensure you update or remove the duplicate test that references try$.all, this.$disposer.defer, cleaned and result accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/executors/shared.ts`:
- Around line 112-121: The method executeCore currently throws synchronously
when checkDidControlFail() returns an Error, which bypasses post-next() wrap
cleanup; change it to preserve rejection-based behavior by returning a rejected
Promise instead of throwing — i.e., in executeCore(), after calling const
controlBeforeExecution = this.checkDidControlFail(), if controlBeforeExecution
is truthy return Promise.reject(controlBeforeExecution) (referencing executeCore
and checkDidControlFail and executeTasks) so wraps that await next() can
complete their after-next cleanup before the rejection propagates.
---
Nitpick comments:
In `@src/__tests__/all.test.ts`:
- Around line 444-458: The test "allows disposer usage in a successful task
without external cancellation" duplicates the earlier "runs disposer cleanup
after all tasks complete" test; either remove this redundant test block or
consolidate its additional assertion into the existing test: merge the
expect(result).toEqual({ a: 1 }) into the earlier test that uses try$.all and
this.$disposer.defer (the test function that sets cleaned and verifies cleanup),
or alternatively rename the earlier test to reflect both cleanup and
return-value expectations and add a clarifying comment; ensure you update or
remove the duplicate test that references try$.all, this.$disposer.defer,
cleaned and result accordingly.
In `@src/lib/__tests__/coverage-exceptions.test.ts`:
- Around line 139-195: Add a bare-thenable variant of the existing Promise tests
to exercise PromiseLike handling: create a thenable object (e.g.,
pendingThenable with a then(resolve) implementation that stores the resolve
function), pass it into resolveWithAbort(controller.signal, pendingThenable,
createAbortResult) and assert the same behaviors as the Promise tests (returns
the resolved value if thenable settles before abort, returns the abort value if
abort wins, and ensure the abort factory is called exactly once when abort
wins); reuse the same helper names and assertions (resolveWithAbort,
controller.abort, and the abort factory counter) so the test sits alongside the
existing cases and will catch any Promise-only assumptions in resolveWithAbort
and race paths.
In `@src/lib/executors/base.ts`:
- Around line 255-257: Remove the explicit return type on the private getter get
`#wrapCtx`(): WrapCtx and let TypeScript infer the type; modify the getter in the
BaseExecution-derived class so it simply declares get `#wrapCtx`() {
this.#wrapCtxCache ??= BaseExecution.createWrapContext(this.ctx); return
this.#wrapCtxCache; } (keeping references to `#wrapCtxCache`,
BaseExecution.createWrapContext and this.ctx intact) to comply with the
inference guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2864024a-f32f-4782-9963-ecdd943d9e36
📒 Files selected for processing (9)
src/__tests__/all-settled.test.tssrc/__tests__/all.test.tssrc/__tests__/flow.test.tssrc/lib/__tests__/coverage-exceptions.test.tssrc/lib/executors/base.tssrc/lib/executors/run.tssrc/lib/executors/shared.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.ts
adelrodriguez
force-pushed
the
03-07-change_race_methods_to_return_promiselike_instead_of_promise_
branch
from
5b0f1eb to
3e41d91
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e41d912df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
adelrodriguez
force-pushed
the
03-07-change_race_methods_to_return_promiselike_instead_of_promise_
branch
from
3e41d91 to
d18c51f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d18c51fcb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/__tests__/all-settled.test.ts (1)
247-250: Make the wrap cleanup assertion cover sync throws too.
Promise.resolve(next())only reaches.finally(...)ifnext()returns. A synchronous throw bypasses the cleanup, so this test currently proves the async path only.Promise.resolve().then(() => next()).finally(...)covers both.Suggested fix
.wrap((_, next) => - Promise.resolve(next()).finally(() => { - cleaned = true - }) + Promise.resolve() + .then(() => next()) + .finally(() => { + cleaned = true + }) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/all-settled.test.ts` around lines 247 - 250, The cleanup in the .wrap call currently uses Promise.resolve(next()).finally(...), which won't run .finally if next() throws synchronously; change it to use Promise.resolve().then(() => next()).finally(...) so the finally handler (setting cleaned = true) runs for both synchronous throws and async rejections — update the .wrap((_, next) => ...) invocation that sets cleaned to true accordingly and keep the existing finally body.src/__tests__/flow.test.ts (1)
240-243: Use the same cleanup wrapper shape that handles sync throws.This has the same gap as the
allSettled()test:Promise.resolve(next())skips.finally(...)ifnext()throws synchronously. Wrapping the call in.then(() => next())makes the cleanup assertion resilient to both sync and async failure paths.Suggested fix
.wrap((_, next) => - Promise.resolve(next()).finally(() => { - cleaned = true - }) + Promise.resolve() + .then(() => next()) + .finally(() => { + cleaned = true + }) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/flow.test.ts` around lines 240 - 243, The cleanup wrapper currently uses Promise.resolve(next()) which bypasses the .finally handler when next() throws synchronously; update the wrapper used in the .wrap(...) call to invoke next() through a resolved promise (e.g., use Promise.resolve().then(() => next())) so that .finally(...) always runs and the cleaned flag assertion (variable cleaned) is resilient to both sync and async failures — mirror the same fix applied in the allSettled() test to ensure synchronous throws from next() still trigger cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/utils.ts`:
- Around line 36-38: The early-return when signal.aborted returns
createAbortResult() without handling the provided input promise, which can leave
a later rejection unobserved; update the pre-aborted path in the function (the
branch that checks signal.aborted and calls createAbortResult) to
observe/suppress the input promise rejection the same way the Promise.race path
does—e.g., wrap the provided input in Promise.resolve(input).catch(() => {}) or
start a detached Promise.race([input, new Promise(()=>{})]) so the input’s
rejection is handled/consumed before returning createAbortResult(); ensure you
reference the same symbols (signal, createAbortResult, and the input parameter)
so behavior matches the non-aborted code path.
---
Nitpick comments:
In `@src/__tests__/all-settled.test.ts`:
- Around line 247-250: The cleanup in the .wrap call currently uses
Promise.resolve(next()).finally(...), which won't run .finally if next() throws
synchronously; change it to use Promise.resolve().then(() =>
next()).finally(...) so the finally handler (setting cleaned = true) runs for
both synchronous throws and async rejections — update the .wrap((_, next) =>
...) invocation that sets cleaned to true accordingly and keep the existing
finally body.
In `@src/__tests__/flow.test.ts`:
- Around line 240-243: The cleanup wrapper currently uses
Promise.resolve(next()) which bypasses the .finally handler when next() throws
synchronously; update the wrapper used in the .wrap(...) call to invoke next()
through a resolved promise (e.g., use Promise.resolve().then(() => next())) so
that .finally(...) always runs and the cleaned flag assertion (variable cleaned)
is resilient to both sync and async failures — mirror the same fix applied in
the allSettled() test to ensure synchronous throws from next() still trigger
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1443af16-b73d-4eb1-96e7-66e09768477a
📒 Files selected for processing (9)
src/__tests__/all-settled.test.tssrc/__tests__/all.test.tssrc/__tests__/flow.test.tssrc/lib/__tests__/coverage-exceptions.test.tssrc/lib/executors/base.tssrc/lib/executors/run.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.tssrc/lib/utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/all.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/executors/run.ts
- src/lib/executors/base.ts
adelrodriguez
force-pushed
the
03-07-change_race_methods_to_return_promiselike_instead_of_promise_
branch
from
d18c51f to
e0980cb
Compare
adelrodriguez
deleted the
03-07-change_race_methods_to_return_promiselike_instead_of_promise_
branch
1 participant
No description provided.