fix: 优化文件下载器方法 #6617
fix: 优化文件下载器方法 #6617
Conversation
lghuahua
requested review from
anncwb,
vince292007,
mynetfan and
jinmao88
as code owners
|
WalkthroughFileDownloader.download now ensures an explicit HTTP method (default 'GET') and dispatches requests through a generic client.request(...) when available; otherwise it resolves the method name and calls the corresponding client method (e.g., get/post) or throws if the method is unsupported. Tests updated to cover these flows. Changes
Sequence Diagram(s)sequenceDiagram
participant FD as FileDownloader
participant RC as RequestClient
rect rgba(0,128,96,0.06)
Note over FD,RC: Preparation
FD->>FD: merge config, ensure method='GET' if missing
end
rect rgba(0,102,204,0.06)
Note over FD,RC: Preferred path
FD->>RC: request(url, finalConfig)
RC-->>FD: response (body/blob)
end
rect rgba(204,102,0,0.06)
Note over FD,RC: Fallback (method-specific)
FD->>RC: call RC.<method>(...) alt method in {post, put}
RC-->>FD: response
end
rect rgba(204,0,0,0.06)
Note over FD,RC: Unsupported method
FD->>FD: detect missing client method
FD-->>FD: throw "method not implemented" error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/request/src/request-client/modules/downloader.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test (ubuntu-latest)
packages/effects/request/src/request-client/modules/downloader.ts
[failure] 36-36: packages/effects/request/src/request-client/modules/downloader.test.ts > fileDownloader > should merge provided config with default config
TypeError: this.client.request is not a function
❯ FileDownloader.download packages/effects/request/src/request-client/modules/downloader.ts:36:40
❯ packages/effects/request/src/request-client/modules/downloader.test.ts:49:41
[failure] 36-36: packages/effects/request/src/request-client/modules/downloader.test.ts > fileDownloader > should download a file and return a Blob
TypeError: this.client.request is not a function
❯ FileDownloader.download packages/effects/request/src/request-client/modules/downloader.ts:36:40
❯ packages/effects/request/src/request-client/modules/downloader.test.ts:28:41
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (1)
packages/effects/request/src/request-client/modules/downloader.ts (1)
29-34: Default method placement and binary response config validated
method: 'GET'is set before...config, allowing callers to override the HTTP methodresponseType: 'blob'is after...config, enforcing binary download responses- Verified that
RequestClientexposesget,post,put,delete, and a genericrequestmethod that forwards themethodfield to Axios
packages/effects/request/src/request-client/modules/downloader.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/effects/request/src/request-client/modules/downloader.ts (1)
40-42: Nice: Safe preference for genericrequestwhen available.This addresses the earlier regression concern about calling a non-existent
requestmethod directly. Good backward‑compatibility guard.
🧹 Nitpick comments (3)
packages/effects/request/src/request-client/modules/downloader.test.ts (2)
98-115: Double‑check RequestClient.request signature consistency.This test assumes a
(url, config)call shape forrequest. If your RequestClient aims to mirror Axios, note Axios usesrequest(config)instead. Since this codebase appears to intentionally diverge, please ensure the RequestClient contract is clearly documented to prevent confusion with Axios semantics.
117-142: POST path coverage looks good; consider adding PATCH coverage.Behavior for body-carrying methods should be uniform. A PATCH test will prevent regressions given the slightly different Axios signature for
patch(url, data, config)vs.get(url, config).Apply this diff to add a PATCH test:
@@ it('should call post', async () => { @@ }); + it('should call patch with body and config', async () => { + const url = 'https://example.com/file'; + const mockAxiosInstance = { + patch: vi.fn(), + } as any; + fileDownloader = new FileDownloader(mockAxiosInstance); + const customConfig: AxiosRequestConfig = { + method: 'PATCH', + data: { name: 'aa' }, + }; + await fileDownloader.download(url, customConfig); + expect(mockAxiosInstance.patch).toHaveBeenCalledWith( + url, + { name: 'aa' }, + { + method: 'PATCH', + responseType: 'blob', + responseReturn: 'body', + }, + ); + });packages/effects/request/src/request-client/modules/downloader.ts (1)
54-56: Optional: Enrich unsupported-method error with available methods.Including discovered method names on the client would improve diagnosability without leaking internals.
Apply this diff:
- throw new Error( - `RequestClient does not support method "${method}". Please ensure the method is properly implemented in your RequestClient instance.`, - ); + const available = Object.keys(clientAny) + .filter((k) => typeof clientAny[k] === 'function') + .join(', '); + throw new Error( + `RequestClient does not support method "${method}". Ensure it is implemented on your RequestClient instance. Available methods: [${available}]`, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/effects/request/src/request-client/modules/downloader.test.ts(3 hunks)packages/effects/request/src/request-client/modules/downloader.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (2)
packages/effects/request/src/request-client/modules/downloader.test.ts (1)
33-36: LGTM: Default method is now explicit and propagated in GET path.Asserting
method: 'GET'in the config is a solid safety net and avoids relying on client defaults. Both tests read well.Also applies to: 55-58
packages/effects/request/src/request-client/modules/downloader.ts (1)
31-34: LGTM: Explicit default method.Setting
method: 'GET'infinalConfigmakes behavior deterministic and simplifies downstream dispatch.
| await expect(() => | ||
| fileDownloader.download(url, { method: 'postt' }), | ||
| ).rejects.toThrow( | ||
| 'RequestClient does not support method "POSTT". Please ensure the method is properly implemented in your RequestClient instance.', | ||
| ); |
There was a problem hiding this comment.
Fix async rejection assertion (use a Promise, not a function).
rejects matcher expects a Promise: wrap the awaited call directly. Current form may not fail as intended.
Apply this diff:
- await expect(() =>
- fileDownloader.download(url, { method: 'postt' }),
- ).rejects.toThrow(
+ await expect(
+ fileDownloader.download(url, { method: 'postt' }),
+ ).rejects.toThrow(
'RequestClient does not support method "POSTT". Please ensure the method is properly implemented in your RequestClient instance.',
);📝 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.
| await expect(() => | |
| fileDownloader.download(url, { method: 'postt' }), | |
| ).rejects.toThrow( | |
| 'RequestClient does not support method "POSTT". Please ensure the method is properly implemented in your RequestClient instance.', | |
| ); | |
| await expect( | |
| fileDownloader.download(url, { method: 'postt' }), | |
| ).rejects.toThrow( | |
| 'RequestClient does not support method "POSTT". Please ensure the method is properly implemented in your RequestClient instance.', | |
| ); |
🤖 Prompt for AI Agents
In packages/effects/request/src/request-client/modules/downloader.test.ts around
lines 151 to 155, the test uses expect(() => await ...) with the rejects matcher
which expects a Promise; change the assertion to pass the Promise directly by
calling fileDownloader.download(...) (no wrapper function) and chain
.rejects.toThrow(...) (ensure you do not await the promise before passing it to
expect). This fixes the async rejection assertion so the test correctly verifies
the download promise rejects with the expected error message.
| if (typeof clientAny[lower] === 'function') { | ||
| if (['POST', 'PUT'].includes(method)) { | ||
| const { data, ...rest } = finalConfig as Record<string, any>; | ||
| return await clientAny[lower](url, data, rest); | ||
| } | ||
|
|
||
| return await clientAny[lower](url, finalConfig); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PATCH is treated like GET here — body is dropped.
Axios-style patch(url, data, config) requires passing data separately. Current code only special‑cases POST and PUT, so PATCH will incorrectly call client.patch(url, finalConfig), losing the request body.
Apply this diff to include PATCH and avoid sending data with GET:
- if (typeof clientAny[lower] === 'function') {
- if (['POST', 'PUT'].includes(method)) {
- const { data, ...rest } = finalConfig as Record<string, any>;
- return await clientAny[lower](url, data, rest);
- }
-
- return await clientAny[lower](url, finalConfig);
- }
+ if (typeof clientAny[lower] === 'function') {
+ if (['POST', 'PUT', 'PATCH'].includes(method)) {
+ const { data, ...rest } = finalConfig as Record<string, any>;
+ return await clientAny[lower](url, data, rest);
+ }
+ if (method === 'GET') {
+ // Avoid accidental bodies on GET
+ const { data: _ignored, ...rest } = finalConfig as Record<string, any>;
+ return await clientAny[lower](url, rest);
+ }
+ return await clientAny[lower](url, finalConfig);
+ }📝 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.
| if (typeof clientAny[lower] === 'function') { | |
| if (['POST', 'PUT'].includes(method)) { | |
| const { data, ...rest } = finalConfig as Record<string, any>; | |
| return await clientAny[lower](url, data, rest); | |
| } | |
| return await clientAny[lower](url, finalConfig); | |
| } | |
| if (typeof clientAny[lower] === 'function') { | |
| if (['POST', 'PUT', 'PATCH'].includes(method)) { | |
| const { data, ...rest } = finalConfig as Record<string, any>; | |
| return await clientAny[lower](url, data, rest); | |
| } | |
| if (method === 'GET') { | |
| // Avoid accidental bodies on GET | |
| const { data: _ignored, ...rest } = finalConfig as Record<string, any>; | |
| return await clientAny[lower](url, rest); | |
| } | |
| return await clientAny[lower](url, finalConfig); | |
| } |
🤖 Prompt for AI Agents
In packages/effects/request/src/request-client/modules/downloader.ts around
lines 45 to 52, the PATCH method is currently treated like GET and its request
body is dropped; update the conditional that special-cases methods with a
request body to include 'PATCH' (i.e., treat POST, PUT, PATCH the same) and
destructure finalConfig into { data, ...rest } for those methods so you call
client[lower](url, data, rest); for methods that do not send a body (GET,
DELETE, etc.) call client[lower](url, finalConfig) so data is not sent
incorrectly.
支持除GET以外的其他方法，由用户自己决定
Summary by CodeRabbit