fix: replace any types and @ts-ignore in SDK clients#583
fix: replace any types and @ts-ignore in SDK clients#583dev-punia-altimate wants to merge 1 commit intomainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
📝 WalkthroughWalkthroughThe JS SDK client modules replace untyped request handlers that used TypeScript ignores with properly typed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Category: type-safety Severity: high Repo: altimate-code
dev-punia-altimate
force-pushed
the
proposal/sdk-client-type-safety
branch
from
b915112 to
f962ec9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk/js/src/client.ts`:
- Around line 10-12: The customFetch function contains dead code: the type
assertion and assignment (Request & { timeout: boolean }).timeout = false have
no effect on Node.js/undici fetch; remove that assertion and assignment from
customFetch and either leave customFetch as `const customFetch = (req: Request)
=> fetch(req)` or, if you need timeout control, implement it by accepting
RequestInit/AbortSignal (use AbortController.signal) or configuring an undici
dispatcher with headersTimeout/bodyTimeout; update any callers of customFetch
accordingly (refer to the customFetch function and its Request type assertion).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6163ee81-34a1-41b6-819f-dc31f349ce47
📒 Files selected for processing (2)
packages/sdk/js/src/client.tspackages/sdk/js/src/v2/client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/js/src/v2/client.ts
| const customFetch = (req: Request) => { | ||
| ;(req as Request & { timeout: boolean }).timeout = false | ||
| return fetch(req) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js undici fetch request timeout property type
💡 Result:
In Undici's fetch implementation (used in Node.js), there is no direct "timeout" property on the fetch request options (RequestInit). Timeouts are configured via the optional "dispatcher" property, which accepts a Dispatcher instance (e.g., new Agent({ ... })) or similar. The relevant timeout properties on the dispatcher (e.g., Agent or Client constructor options, or DispatchOptions) are of type "number | null": - headersTimeout: number | null (time to receive headers, default 300000 ms) - bodyTimeout: number | null (time between body data, default 300000 ms) - connect.timeout: number (for connection timeout, nested under "connect" option) These are specified in Undici's Dispatcher.DispatchOptions and ClientOptions. Use 0 to disable. For fetch example: fetch(url, { dispatcher: new Agent({ headersTimeout: 10000, bodyTimeout: 10000 }) }) AbortSignal in RequestInit can also be used for overall request abortion, but it's not a numeric timeout property on the request itself.
Citations:
- 1: https://undici-docs.vramana.dev/docs/api/Client
- 2: https://app.unpkg.com/undici-types@7.20.0/files/dispatcher.d.ts
- 3: https://undici-docs.vramana.dev/docs/api/Dispatcher
- 4: https://stackoverflow.com/questions/76755913/increase-timeout-in-nodejs-built-in-fetch-api
- 5: Default fetch timeout too short nodejs/undici#1373
- 6: https://github.com/nodejs/undici
🏁 Script executed:
cat -n packages/sdk/js/src/client.ts | head -30Repository: AltimateAI/altimate-code
Length of output: 1079
🏁 Script executed:
rg "timeout" packages/sdk/js/src/ -A 3 -B 3Repository: AltimateAI/altimate-code
Length of output: 6926
🏁 Script executed:
rg "customFetch" packages/sdk/js/src/ -A 3 -B 3Repository: AltimateAI/altimate-code
Length of output: 1548
🏁 Script executed:
# Check if there are any comments or documentation about this
rg "timeout.*false|false.*timeout" packages/sdk/js/ -A 2 -B 2Repository: AltimateAI/altimate-code
Length of output: 1877
🏁 Script executed:
git log --oneline -p packages/sdk/js/src/client.ts | grep -A 10 -B 10 "timeout" | head -50Repository: AltimateAI/altimate-code
Length of output: 713
🏁 Script executed:
# Check if there are any TODOs or comments in the file explaining this
grep -n "TODO\|FIXME\|NOTE\|HACK" packages/sdk/js/src/client.tsRepository: AltimateAI/altimate-code
Length of output: 50
Remove dead code: timeout property has no effect on Node.js fetch behavior.
Setting timeout = false on the Request object has no effect in Node.js's undici implementation. Timeouts are configured via:
AbortSignalinRequestInit(e.g.,fetch(req, { signal: controller.signal }))dispatcheroption withheadersTimeout/bodyTimeoutproperties- Not via a
timeoutproperty on the Request object itself
The type assertion Request & { timeout: boolean } masks this non-functional property. This code should either be removed or replaced with proper timeout handling using AbortSignal or dispatcher configuration if timeout control is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk/js/src/client.ts` around lines 10 - 12, The customFetch function
contains dead code: the type assertion and assignment (Request & { timeout:
boolean }).timeout = false have no effect on Node.js/undici fetch; remove that
assertion and assignment from customFetch and either leave customFetch as `const
customFetch = (req: Request) => fetch(req)` or, if you need timeout control,
implement it by accepting RequestInit/AbortSignal (use AbortController.signal)
or configuring an undici dispatcher with headersTimeout/bodyTimeout; update any
callers of customFetch accordingly (refer to the customFetch function and its
Request type assertion).
1 participant
What does this PR do?
Removes bare
anytypes and@ts-ignorecomments from the SDK client fetch wrappers in both v1 and v2 clients.Type of change
How did you verify your code works?
Checklist
Issue for this PR
Identified by QA Autopilot code scan — both SDK clients use
anytypes that bypass TypeScript type checking.Category: type-safety
Severity: high
Files: packages/sdk/js/src/client.ts, packages/sdk/js/src/v2/client.ts
What I Found
Both SDK client files (
client.tsandv2/client.ts) define acustomFetchfunction with bareanytypes and a@ts-ignorecomment to setreq.timeout = false. This defeats TypeScript's type system at a critical boundary (the HTTP client).Fix
const customFetch: any = (req: any) =>withconst customFetch = (req: Request): Promise<Response> =>@ts-ignore+ direct property set with a safeRecord<string, unknown>assertionAuto-generated by QA Autopilot Proposal Monitor. Open for 30-day human review.
Summary by CodeRabbit