Refactor common fetch logic into baseFetch function#560
Conversation
|
|
Warning Rate limit exceeded@wKovacs64 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 52 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (7)
WalkthroughAdds a new typed Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (module using fetchFromApi)
participant API as fetchFromApi (haveibeenpwned / pwnedpasswords)
participant Base as baseFetch
participant Net as fetch / Network
participant PKG as package-info (optional)
Caller->>API: invoke fetchFromApi(params)
API->>Base: call baseFetch(baseUrl, endpoint, headers, timeoutMs, userAgent, queryParams)
Base->>PKG: (conditionally) read package name/version for User-Agent
Base->>Net: perform fetch(fullUrl, requestInit with headers + AbortSignal)
Net-->>Base: response
Base-->>API: Response
API-->>Caller: parsed/validated result or thrown error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 21 +1
Lines 131 137 +6
Branches 45 45
=========================================
+ Hits 131 137 +6
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/api/base-fetch.ts (2)
29-40: Consider defensive handling for endpoint without leading slash.If
endpointdoesn't start with/, the URL concatenation on line 31 could produce malformed URLs (e.g.,https://example.compathinstead ofhttps://example.com/path). While current callers appear to always use leading slashes, adding defensive handling would make this utility more robust.function buildUrl(baseUrl: string, endpoint: string, queryParams?: Record<string, string>): string { const base = baseUrl.replace(/\/$/g, ''); - const url = new URL(`${base}${endpoint}`); + const normalizedEndpoint = endpoint.startsWith('/') ? endpoint : `/${endpoint}`; + const url = new URL(`${base}${normalizedEndpoint}`); if (queryParams) { for (const [key, value] of Object.entries(queryParams)) { url.searchParams.set(key, value); } } return url.toString(); }
1-2: Consider adding a type declaration for the generated file.The
@ts-ignoreworks, but a.d.tsfile forpackage-info.jswould provide better type safety and IDE support. This is a minor improvement that can be deferred.You could add
src/api/haveibeenpwned/package-info.d.ts:export declare const PACKAGE_NAME: string; export declare const PACKAGE_VERSION: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/base-fetch.ts(1 hunks)src/api/haveibeenpwned/__tests__/fetch-from-api.test.ts(1 hunks)src/api/haveibeenpwned/fetch-from-api.ts(2 hunks)src/api/pwnedpasswords/__tests__/fetch-from-api.test.ts(1 hunks)src/api/pwnedpasswords/fetch-from-api.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/api/pwnedpasswords/fetch-from-api.ts (1)
src/api/base-fetch.ts (1)
baseFetch(4-27)
src/api/haveibeenpwned/fetch-from-api.ts (1)
src/api/base-fetch.ts (1)
baseFetch(4-27)
🔇 Additional comments (6)
src/api/pwnedpasswords/__tests__/fetch-from-api.test.ts (1)
9-13: LGTM!The updated error expectation correctly reflects the standardized
TypeError: Invalid URLthrown by theURLconstructor inbaseFetch.buildUrl()when given an invalid base URL. This is cleaner than testing for implementation-specific error messages.src/api/haveibeenpwned/__tests__/fetch-from-api.test.ts (1)
53-58: LGTM!Consistent with the pwnedpasswords test update—the standardized
TypeError: Invalid URLexpectation properly reflects the centralized URL handling inbaseFetch.src/api/pwnedpasswords/fetch-from-api.ts (1)
45-55: Clean refactor to use centralizedbaseFetch.The implementation correctly delegates request construction while preserving the original behavior. Header handling is appropriately minimal.
One minor observation:
modeis always passed as a query parameter (defaulting to'sha1'). If the API treats absence of themodeparameter as defaulting to SHA-1, you could consider only including it when explicitly set to'ntlm'to reduce URL noise. However, being explicit is also a valid choice.src/api/haveibeenpwned/fetch-from-api.ts (1)
74-83: LGTM!The refactor cleanly delegates request construction to
baseFetchwhile preserving all existing response handling and error logic. The conditionalHIBP-API-Keyheader is handled appropriately.src/api/base-fetch.ts (2)
42-52: Well-designed cross-environment User-Agent handling.The conditional logic correctly handles three scenarios: custom User-Agent provided, Node environment (uses package default), and browser environment (lets browser set its own). This is a good pattern for isomorphic JavaScript.
4-27: LGTM!The
baseFetchfunction is well-structured with clear parameter types and clean delegation to helper functions. UsingAbortSignal.timeoutfor request timeouts is the modern, recommended approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/api/base-fetch.ts (2)
1-2: Prefer@ts-expect-errorover@ts-ignorefor type suppressions.TypeScript best practices recommend using
@ts-expect-errorinstead of@ts-ignorebecause it will error if the suppression becomes unnecessary, helping keep the codebase clean.Additionally, note that this creates a coupling where the base fetch utility depends on a generated file within the
haveibeenpwnedsubdirectory. This is workable but means the base utility isn't fully independent of specific API modules.Apply this diff:
-// @ts-ignore - package-info.js is generated +// @ts-expect-error - package-info.js is generated import { PACKAGE_NAME, PACKAGE_VERSION } from './haveibeenpwned/package-info.js';
29-41: Minor: Remove unnecessary global flag from regex.The regular expression
/\/$/guses the global flagg, but since it only matches at the end of the string (due to the$anchor), the global flag is unnecessary. Using/\/$/would be equivalent and slightly more precise.Apply this diff:
- const base = baseUrl.replace(/\/$/g, ''); + const base = baseUrl.replace(/\/$/, '');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/api/base-fetch.ts(1 hunks)
🔇 Additional comments (2)
src/api/base-fetch.ts (2)
43-53: LGTM!The header building logic is clean and handles the User-Agent appropriately:
- Explicit
userAgentparameter takes precedence- Falls back to package name/version in non-browser environments
- Allows browsers to use their default User-Agent
4-27: No compatibility issue withAbortSignal.timeout()for the target environment.The project targets Node.js >= 20.19.0 (per package.json), which fully supports
AbortSignal.timeout()(available since v17.3.0). No polyfill or compatibility check is needed.Likely an incorrect or invalid review comment.
wKovacs64
force-pushed
the
unified-fetch
branch
from
1721bf7 to
6c11729
Compare
wKovacs64
force-pushed
the
unified-fetch
branch
from
6c11729 to
824e262
Compare
1 participant
This pull request refactors the API fetch logic by introducing a shared
baseFetchutility, which centralizes request setup, header construction, and URL building. The change reduces code duplication, improves maintainability, and standardizes how requests are made across thehaveibeenpwnedandpwnedpasswordsmodules. Additionally, related tests are updated to reflect improved error handling.API Request Refactoring:
baseFetchutility insrc/api/base-fetch.tsto handle request construction, headers (including defaultUser-Agent), timeouts, and query parameters in a reusable way.fetchFromApiinsrc/api/haveibeenpwned/fetch-from-api.tsandsrc/api/pwnedpasswords/fetch-from-api.tsto usebaseFetch, removing duplicated code for URL and header setup. [1] [2] [3] [4]Testing and Error Handling:
src/api/haveibeenpwned/__tests__/fetch-from-api.test.tsandsrc/api/pwnedpasswords/__tests__/fetch-from-api.test.tsto expect the new standardized error message (TypeError: Invalid URL) from the improved URL validation inbaseFetch. [1] [2]Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.