Resolves #4#20
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds configurable request timeout support to both smooth-api-py and smooth-api-ts packages. Python enforces timeouts via asyncio.wait_for (async-only, raising NotImplementedError for sync), while TypeScript uses AbortController. Also renames ResilientFetchConfig/resilientFetch to SmoothFetchConfig/smoothFetch, updates docs, versions, and tests. ChangesPython package (smooth-api-py)
TypeScript package (smooth-api-ts)
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant createSmoothFetch
participant AbortController
participant fetch as "global.fetch"
Caller->>createSmoothFetch: request(url, options)
createSmoothFetch->>AbortController: create controller
createSmoothFetch->>AbortController: setTimeout(timeoutMs) to abort
createSmoothFetch->>fetch: fetch(url, options with signal)
alt Timeout elapses
AbortController-->>fetch: abort()
fetch-->>createSmoothFetch: throws error
createSmoothFetch->>createSmoothFetch: check user signal.aborted
createSmoothFetch->>fetch: retry attempt
fetch-->>createSmoothFetch: response 200
else User aborts explicitly
Caller->>AbortController: abort()
fetch-->>createSmoothFetch: throws error
createSmoothFetch-->>Caller: re-throw immediately (no retry)
else Success
fetch-->>createSmoothFetch: response
createSmoothFetch-->>Caller: response
end
createSmoothFetch->>createSmoothFetch: clear timeout (finally)
Estimated code review effortEstimated code review effort: 3 (Moderate) | ~25 minutes Related issues: Suggested labels: enhancement, documentation, tests Suggested reviewers: AryanSharma48 Poem:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
packages/smooth-api-py/smooth_api/__init__.py (1)
206-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__per Ruff (RUF022).Static analysis flags the export list as unsorted.
♻️ Proposed fix
-__all__ = ['smooth_api', 'SmoothConfig', 'DeduplicationConfig', 'resilient_api', 'ResilientConfig'] +__all__ = ['DeduplicationConfig', 'ResilientConfig', 'SmoothConfig', 'resilient_api', 'smooth_api']🤖 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/smooth-api-py/smooth_api/__init__.py` around lines 206 - 207, The __all__ export list in smooth_api.__init__ is unsorted and should be reordered to satisfy Ruff RUF022. Update the __all__ assignment so the exported symbols are listed in sorted order, keeping the same names and using the existing __all__ definition as the location to fix.Source: Linters/SAST tools
🤖 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/smooth-api-py/smooth_api/__init__.py`:
- Around line 89-92: The timeout handling in the async wrapper currently treats
timeout_ms=0 as falsy, so an explicit zero timeout is skipped instead of
enforced. Update the conditional in the decorator logic around the
fn/asyncio.wait_for call to check config.timeout_ms with an explicit is not None
test, so both 0 and positive values use wait_for while None keeps the no-timeout
path.
In `@packages/smooth-api-ts/src/index.ts`:
- Around line 50-52: The retry path in `index.ts` leaves a bridged abort
listener attached to the caller’s `AbortSignal`, and the per-attempt controller
can also remain referenced after completion. Update the retry logic around the
`options.signal`/`userSignal.addEventListener('abort', ...)` bridge to store the
listener and remove it in the same `finally` block that clears the timeout,
ensuring each attempt cleans up both the abort listener and the controller
reference. Apply the same cleanup to the other retry block referenced by the
duplicate abort-listener setup.
- Around line 109-112: User cancellations are being counted as failures in the
circuit-breaker flow. In the request handling path inside the relevant logic in
index.ts, check the abort state from options?.signal before calling the failure
accounting path, and short-circuit aborted requests without incrementing failure
metrics. Update the surrounding error handling so the abort check happens before
the circuit-breaker failure record, keeping canceled calls from opening the
circuit.
In `@packages/smooth-api-ts/src/types.ts`:
- Line 41: The exported config type was renamed to SmoothFetchConfig, but the
deprecated ResilientFetchConfig alias is no longer exported, which breaks
existing TypeScript consumers. Update the types module to keep
ResilientFetchConfig exported as a deprecated alias alongside SmoothFetchConfig,
using the same underlying shape so both names remain available for imports.
- Around line 53-57: The timeoutMs field in the request types is currently too
permissive and can accept invalid values like 0 or NaN, which then bypass the
intended abort behavior. Update the type definition in types.ts for timeoutMs to
constrain it to a positive finite number, and ensure any validation or usage
around the request timeout path enforces that constraint before scheduling the
abort timer. Use the timeoutMs field and its related request-attempt timeout
handling as the key symbols to locate the fix.
---
Nitpick comments:
In `@packages/smooth-api-py/smooth_api/__init__.py`:
- Around line 206-207: The __all__ export list in smooth_api.__init__ is
unsorted and should be reordered to satisfy Ruff RUF022. Update the __all__
assignment so the exported symbols are listed in sorted order, keeping the same
names and using the existing __all__ definition as the location to fix.
🪄 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: d212b44a-0664-4c69-b6da-218bc0128fcd
📒 Files selected for processing (13)
packages/smooth-api-py/README.mdpackages/smooth-api-py/pyproject.tomlpackages/smooth-api-py/smooth_api/__init__.pypackages/smooth-api-py/smooth_api/config.pypackages/smooth-api-py/tests/test_timeout.pypackages/smooth-api-ts/README.mdpackages/smooth-api-ts/package.jsonpackages/smooth-api-ts/src/dedup.tspackages/smooth-api-ts/src/index.tspackages/smooth-api-ts/src/types.tspackages/smooth-api-ts/tests/deduplication.test.tspackages/smooth-api-ts/tests/resilience.test.tspackages/smooth-api-ts/tests/timeout.test.ts
Comment on lines
+89
to
+92
| if config.timeout_ms: | ||
| result = await asyncio.wait_for(fn(*args, **kwargs), timeout=config.timeout_ms / 1000.0) | ||
| else: | ||
| result = await fn(*args, **kwargs) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Explicit timeout_ms=0 is silently ignored.
if config.timeout_ms: treats 0 as falsy, so a caller who intentionally sets timeout_ms=0 gets no timeout enforcement at all, silently diverging from the configured value. Since the field is Optional[int], use an explicit is not None check.
🐛 Proposed fix
- if config.timeout_ms:
+ if config.timeout_ms is not None:
result = await asyncio.wait_for(fn(*args, **kwargs), timeout=config.timeout_ms / 1000.0)📝 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.
Suggested change
| if config.timeout_ms: | |
| result = await asyncio.wait_for(fn(*args, **kwargs), timeout=config.timeout_ms / 1000.0) | |
| else: | |
| result = await fn(*args, **kwargs) | |
| if config.timeout_ms is not None: | |
| result = await asyncio.wait_for(fn(*args, **kwargs), timeout=config.timeout_ms / 1000.0) | |
| else: | |
| result = await fn(*args, **kwargs) |
🤖 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/smooth-api-py/smooth_api/__init__.py` around lines 89 - 92, The
timeout handling in the async wrapper currently treats timeout_ms=0 as falsy, so
an explicit zero timeout is skipped instead of enforced. Update the conditional
in the decorator logic around the fn/asyncio.wait_for call to check
config.timeout_ms with an explicit is not None test, so both 0 and positive
values use wait_for while None keeps the no-timeout path.
Comment on lines
+50
to
+52
| if (options?.signal) { | ||
| const userSignal = options.signal; | ||
| userSignal.addEventListener('abort', () => controller?.abort(userSignal.reason)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the bridged abort listener in finally.
Each retry adds a listener to the caller’s AbortSignal and retains the attempt controller after completion. Clean it up with the timeout.
Abort listener cleanup
let timeoutId: ReturnType<typeof setTimeout> | undefined;
let currentOptions = options;
let controller: AbortController | undefined;
+ let removeUserAbortListener: (() => void) | undefined;
if (globalConfig.timeoutMs) {
controller = new AbortController();
if (options?.signal) {
const userSignal = options.signal;
- userSignal.addEventListener('abort', () => controller?.abort(userSignal.reason));
if (userSignal.aborted) {
controller.abort(userSignal.reason);
+ } else {
+ const abortFromUser = () => controller?.abort(userSignal.reason);
+ userSignal.addEventListener('abort', abortFromUser, { once: true });
+ removeUserAbortListener = () => userSignal.removeEventListener('abort', abortFromUser);
}
}
@@
} finally {
if (timeoutId) clearTimeout(timeoutId);
+ removeUserAbortListener?.();
}Also applies to: 118-119
🤖 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/smooth-api-ts/src/index.ts` around lines 50 - 52, The retry path in
`index.ts` leaves a bridged abort listener attached to the caller’s
`AbortSignal`, and the per-attempt controller can also remain referenced after
completion. Update the retry logic around the
`options.signal`/`userSignal.addEventListener('abort', ...)` bridge to store the
listener and remove it in the same `finally` block that clears the timeout,
ensuring each attempt cleans up both the abort listener and the controller
reference. Apply the same cleanup to the other retry block referenced by the
duplicate abort-listener setup.
Comment on lines
+109
to
+112
| // Do not retry if the user explicitly aborted the request | ||
| if (options?.signal?.aborted) { | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not count user cancellations as circuit-breaker failures.
Line 107 records a failure before Line 110 detects a caller abort, so repeated user cancellations can open the circuit and block later valid requests.
Move abort handling before failure accounting
} catch (err) {
lastError = err;
- breaker.recordFailure(domain);
// Do not retry if the user explicitly aborted the request
if (options?.signal?.aborted) {
throw err;
}
+
+ breaker.recordFailure(domain);
// Don't sleep after the final attempt📝 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.
Suggested change
| // Do not retry if the user explicitly aborted the request | |
| if (options?.signal?.aborted) { | |
| throw err; | |
| } | |
| // Do not retry if the user explicitly aborted the request | |
| if (options?.signal?.aborted) { | |
| throw err; | |
| } | |
| breaker.recordFailure(domain); |
🤖 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/smooth-api-ts/src/index.ts` around lines 109 - 112, User
cancellations are being counted as failures in the circuit-breaker flow. In the
request handling path inside the relevant logic in index.ts, check the abort
state from options?.signal before calling the failure accounting path, and
short-circuit aborted requests without incrementing failure metrics. Update the
surrounding error handling so the abort check happens before the circuit-breaker
failure record, keeping canceled calls from opening the circuit.
|
|
||
| // T types the fallback payload so callers get inference at the use site. | ||
| export interface ResilientFetchConfig<T = unknown> { | ||
| export interface SmoothFetchConfig<T = unknown> { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the deprecated alias is still exported from the TS package.
rg -nP '\b(ResilientFetchConfig|SmoothFetchConfig)\b' packages/smooth-api-ts/src packages/smooth-api-ts/tests packages/smooth-api-ts/README.mdRepository: AryanSharma48/smoothAPI
Length of output: 468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for ResilientFetchConfig across repo ==\n'
rg -nP '\bResilientFetchConfig\b' .
printf '\n== package files ==\n'
git ls-files 'packages/smooth-api-ts/*'
printf '\n== types.ts (relevant slice) ==\n'
sed -n '1,120p' packages/smooth-api-ts/src/types.ts
printf '\n== index.ts (relevant slice) ==\n'
sed -n '1,120p' packages/smooth-api-ts/src/index.ts
printf '\n== package.json ==\n'
sed -n '1,220p' packages/smooth-api-ts/package.jsonRepository: AryanSharma48/smoothAPI
Length of output: 212
Keep the deprecated ResilientFetchConfig alias exported. packages/smooth-api-ts/src/types.ts only exposes SmoothFetchConfig, so existing TypeScript consumers importing ResilientFetchConfig will break; add the deprecated alias alongside the new name.
🤖 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/smooth-api-ts/src/types.ts` at line 41, The exported config type was
renamed to SmoothFetchConfig, but the deprecated ResilientFetchConfig alias is
no longer exported, which breaks existing TypeScript consumers. Update the types
module to keep ResilientFetchConfig exported as a deprecated alias alongside
SmoothFetchConfig, using the same underlying shape so both names remain
available for imports.
Comment on lines
+53
to
+57
| /** | ||
| * Maximum duration in milliseconds before a request attempt is aborted. | ||
| * Applied per-attempt. If aborted, the request is considered a failure and may be retried. | ||
| */ | ||
| timeoutMs?: number; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether timeoutMs is validated before use.
rg -nP 'timeoutMs|Number\.isFinite|RangeError' packages/smooth-api-ts/srcRepository: AryanSharma48/smoothAPI
Length of output: 455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- packages/smooth-api-ts/src/index.ts ---\n'
wc -l packages/smooth-api-ts/src/index.ts packages/smooth-api-ts/src/types.ts
printf '\n--- outline: packages/smooth-api-ts/src/index.ts ---\n'
ast-grep outline packages/smooth-api-ts/src/index.ts --view expanded || true
printf '\n--- relevant excerpt: packages/smooth-api-ts/src/index.ts ---\n'
sed -n '1,120p' packages/smooth-api-ts/src/index.ts
printf '\n--- relevant excerpt: packages/smooth-api-ts/src/types.ts ---\n'
sed -n '1,120p' packages/smooth-api-ts/src/types.tsRepository: AryanSharma48/smoothAPI
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- packages/smooth-api-ts/src/index.ts line count ---'
wc -l packages/smooth-api-ts/src/index.ts
echo
echo '--- packages/smooth-api-ts/src/types.ts line count ---'
wc -l packages/smooth-api-ts/src/types.ts
echo
echo '--- outline: packages/smooth-api-ts/src/index.ts ---'
ast-grep outline packages/smooth-api-ts/src/index.ts --view expanded || true
echo
echo '--- relevant excerpt: packages/smooth-api-ts/src/index.ts ---'
sed -n '1,120p' packages/smooth-api-ts/src/index.ts
echo
echo '--- relevant excerpt: packages/smooth-api-ts/src/types.ts ---'
sed -n '1,120p' packages/smooth-api-ts/src/types.tsRepository: AryanSharma48/smoothAPI
Length of output: 7814
Constrain timeoutMs to a positive finite number. timeoutMs is only truthy-checked before scheduling the abort timer, so 0/NaN disable it and other invalid values still flow to setTimeout.
🤖 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/smooth-api-ts/src/types.ts` around lines 53 - 57, The timeoutMs
field in the request types is currently too permissive and can accept invalid
values like 0 or NaN, which then bypass the intended abort behavior. Update the
type definition in types.ts for timeoutMs to constrain it to a positive finite
number, and ensure any validation or usage around the request timeout path
enforces that constraint before scheduling the abort timer. Use the timeoutMs
field and its related request-attempt timeout handling as the key symbols to
locate the fix.
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
1 participant
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.
Description
This PR introduces configurable request timeout support across both the TypeScript and Python packages, allowing users to automatically abort requests that hang indefinitely. It also renames all legacy
Resilientconfigurations toSmoothto align with the new library branding, while keeping the old names as deprecated aliases to maintain backwards compatibility.Affected packages:
smooth-api-tssmooth-api-pyFixes #4
Type of Change
Please check the option that applies:
Checklist
Design & Parity
README.mdor general documentation where necessary.Quality & Testing
cd sandbox && node server.js) before running the tests.npm run build && npm testinsidepackages/smooth-api-ts) and all tests passed.pytest tests/insidepackages/smooth-api-py) and all tests passed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
1.3.0.