Feature/api #2455
Feature/api #2455
Conversation
…d set Content-Type only for requests with a request body.
…body - set 'Content-Type' to 'application/json' for 'POST', 'PUT', and 'DELETE' requests. - ensure 'x-accept-versions' is only added if a version is provided. - minor refactoring for improved readability. - make payload a required parameter for POST and PUT method. - remove unused deleteApi function and middleware.
… for better type safety
- options -> config - requestOptions -> requestConfig
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes refactor API interactions across multiple components by replacing the old Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component
participant UA as useApi Hook
participant AC as apiClient
C->>UA: Invoke API function (e.g., getApi)
UA->>AC: Call apiClient with method, url, headers, etc.
AC-->>UA: Return API response or error
UA-->>C: Return data to component
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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
🔭 Outside diff range comments (1)
pages/sites/[slug]/[locale]/all.tsx (1)
88-101: 🛠️ Refactor suggestionConsider updating the remaining getRequest call.
The
loadTreesDonatedfunction still uses the oldgetRequestmethod, while other similar functions have been updated to usegetApi.async function loadTreesDonated() { try { - const newTreesDonated = await getRequest<TreesDonated>({ - tenant: pageProps.tenantConfig.id, - url: `${process.env.WEBHOOK_URL}/platform/total-tree-count`, - }); + const newTreesDonated = await getApi<TreesDonated>( + `${process.env.WEBHOOK_URL}/platform/total-tree-count` + ); setTreesDonated(newTreesDonated); } catch (err) { setErrors(handleError(err as APIError)); } }
🧹 Nitpick comments (2)
src/utils/apiRequests/apiClient.ts (2)
25-25: Remove unnecessary empty fallback in spread operator.The empty fallback in the spread operator is unnecessary and can be simplified.
const headers: Record<string, string> = { - ...(config.additionalHeaders || {}), + ...(config.additionalHeaders || {}) };🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 25-25: src/utils/apiRequests/apiClient.ts#L25
Empty fallbacks in spreads are unnecessary. (eslint-plugin-unicorn/no-useless-fallback-in-spread)
30-34: Possible issue with body being included in GET requests.When using the ternary operation to conditionally include the body, ensure that GET requests don't include a body, as this may cause issues with some servers.
const response = await fetch(fullUrl, { method: config.method, headers, - body: ['POST', 'PUT'].includes(config.method) - ? JSON.stringify(config.data) - : undefined, + ...((['POST', 'PUT', 'DELETE'].includes(config.method) && config.data) + ? { body: JSON.stringify(config.data) } + : {}) });🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 32-32: src/utils/apiRequests/apiClient.ts#L32
"body" is not allowed when method is "GET". (eslint-plugin-unicorn/no-invalid-fetch-options)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
pages/sites/[slug]/[locale]/all.tsx(4 hunks)src/features/projectsV2/ProjectDetails/index.tsx(3 hunks)src/features/projectsV2/ProjectsContext.tsx(4 hunks)src/hooks/useServerApi.ts(5 hunks)src/utils/apiRequests/apiClient.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/features/projectsV2/ProjectDetails/index.tsx (2)
src/hooks/useServerApi.ts (1) (1)
useServerApi(83-236)src/features/common/types/plantLocation.d.ts (1) (1)
PlantLocation(66-69)
src/features/projectsV2/ProjectsContext.tsx (1)
src/hooks/useServerApi.ts (1) (1)
useServerApi(83-236)
🪛 Biome (1.9.4)
src/hooks/useServerApi.ts
[error] 185-185: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 193-193: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 200-200: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 207-207: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 GitHub Check: CodeFactor
src/utils/apiRequests/apiClient.ts
[notice] 25-25: src/utils/apiRequests/apiClient.ts#L25
Empty fallbacks in spreads are unnecessary. (eslint-plugin-unicorn/no-useless-fallback-in-spread)
[notice] 32-32: src/utils/apiRequests/apiClient.ts#L32
"body" is not allowed when method is "GET". (eslint-plugin-unicorn/no-invalid-fetch-options)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (27)
pages/sites/[slug]/[locale]/all.tsx (4)
30-30: Good: Added the useServerApi hook.This new import introduces the standardized API interaction method which is being used consistently across the codebase.
44-44: Good: Extracted getApi from the hook.Using destructuring to extract the needed method from the hook is a clean approach.
55-57: Improved API call implementation.The code has been simplified by replacing the
getRequestfunction withgetApi. This change removes the need to explicitly pass tenant information as it's now handled internally by theuseServerApihook.
73-75: Consistent use of getApi for tenant score.This change aligns with the standardization pattern being applied across the codebase.
src/features/projectsV2/ProjectsContext.tsx (4)
30-30: Good: Added the useServerApi hook.This new import introduces the standardized API interaction method which is being used consistently across the codebase.
115-115: Good: Extracted getApi from the hook.Using destructuring to extract the needed method from the hook is a clean approach.
135-135: Good: Added safety check for properties.purpose.The optional chaining operator (
?.) provides a safeguard against potential null/undefined errors when accessing thepurposeproperty.
205-212: Improved API call implementation.The change from
getRequesttogetApistandardizes the API interaction pattern. Using proper TypeScript generics for both the response type and query parameters provides better type safety.src/features/projectsV2/ProjectDetails/index.tsx (6)
29-29: Good: Added the useServerApi hook.This new import introduces the standardized API interaction method which is being used consistently across the codebase.
56-56: Good: Extracted getApi from the hook.Using destructuring to extract the needed method from the hook is a clean approach.
60-60: Good: Simplified function signature.Removing the
localeparameter fromloadProjectfunction is appropriate since the locale is now handled internally by theuseServerApihook.
64-70: Improved API call implementation with proper typing.The use of TypeScript generics with
getApi<ExtendedProject, Record<string, string>>provides better type safety for both the response and query parameters.
96-96: Code simplification: Removed locale parameter.Removed the locale parameter from the function call since it's no longer needed in the updated implementation.
103-109: Good: Added version parameter to API call.The use of a specific version (
'1.0.4') in the API call is a good practice for API versioning, ensuring compatibility with specific API endpoints.src/utils/apiRequests/apiClient.ts (3)
7-7: Improved type safety for the data property.Replacing
anywithRecord<string, string>provides better type safety by ensuring that the data object has string keys and string values.
13-15: Simplified apiClient function signature.The function has been streamlined to directly use the request configuration without middleware processing.
17-22: Simplified URL construction.The URL construction has been simplified by removing checks for the API endpoint and directly using the provided URL.
src/hooks/useServerApi.ts (10)
79-79: Good addition of next-intl's useLocale hookThis change better aligns with modern internationalization practices by leveraging the next-intl library, which provides more robust locale management than using localStorage.
86-86: Improved locale handlingUsing the useLocale hook provides more reliable locale detection compared to potentially stale localStorage values, ensuring consistent internationalization throughout the application.
101-102: Enhanced flexibility with additionalHeaders parameterAdding additionalHeaders to the RequestOptions makes the API more versatile by allowing consumers to pass custom headers when needed.
107-111: Improved header managementThe updated header initialization properly uses the locale from next-intl and correctly spreads additionalHeaders when provided.
113-119: Better header organization with conditional logicThe implementation now properly:
- Only adds version headers when a version is specified
- Sets Content-Type header only for methods that send a body (POST, PUT, DELETE)
This is a good improvement in header management.
133-137: Improved error handling for missing API_ENDPOINTThis defensive programming approach fails fast with a clear error message when a required environment variable is missing, preventing subtle runtime issues.
172-176: Enhanced type safety for postApiAuthenticatedMaking the payload required and additionalHeaders optional improves type safety and makes the API contract clearer.
🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
190-190: Better parameter naming and flexibilityChanging from
payloadtoqueryParamsand adding theversionparameter improves clarity and flexibility of the API.
219-225: Simplified deleteApiAuthenticated signatureRemoving the payload parameter from the deleteApiAuthenticated method aligns with REST conventions where DELETE operations typically don't include a request body.
227-236: Comprehensive API surfaceThe hook exports a complete set of methods for interacting with the API, covering all common HTTP methods with both authenticated and unauthenticated variants.
…ndling
- replaced unnecessary spread operator for additionalHeaders with ?? {} to avoid empty object spreading.
- used a spread operator { ...(condition ? { body: value } : {}) } for setting body, ensuring it is completely omitted for unsupported methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/apiRequests/apiClient.ts (1)
7-7: Consider using a more flexible typing for data propertyWhile changing from
anytoRecord<string, string>improves type safety, it might be too restrictive for API requests that need to send numbers, booleans, arrays, or nested objects.- data?: Record<string, string>; + data?: Record<string, unknown>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/apiRequests/apiClient.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/utils/apiRequests/apiClient.ts (4)
13-14: Simplification of function signature is a good improvementRemoving the middleware parameter simplifies the API client. This change makes the code more maintainable and easier to understand.
24-24: Good use of nullish coalescing operatorThe use of
??for default headers is a clean, modern approach.
28-32: Clean implementation of conditional request bodyThe conditional spread operator pattern is well-applied to include the request body only for POST and PUT methods. This is a good practice that improves code clarity.
21-21:❓ Verification inconclusive
Verify URL construction behavior across the application
The URL construction has been simplified, removing the previous checks for
API_ENDPOINTenvironment variable. Ensure this change doesn't affect any environments where the base URL was configured differently.
🏁 Script executed:
#!/bin/bash # Search for any references to API_ENDPOINT that might be affected rg "API_ENDPOINT" --type typescriptLength of output: 70
Action Required: Validate API Endpoint URL Configuration
It appears that the initial command using
--type typescriptproduced an error. To ensure that removing theAPI_ENDPOINTchecks doesn’t affect environments with a custom-configured base URL, please run the revised script below. This script uses a glob pattern over all TypeScript files to search for any references toAPI_ENDPOINT:After running this command, please verify if any instances of
API_ENDPOINTappear that might be impacted by this change.
🏁 Script executed:
#!/bin/bash # Search for any references to API_ENDPOINT in TypeScript files using glob patterns rg "API_ENDPOINT" --glob "*.ts"Length of output: 1656
API Endpoint Configuration Consistency – Please Verify Environment Settings
The changes in
src/utils/apiRequests/apiClient.tsnow construct the URL solely fromconfig.url, removing checks forprocess.env.API_ENDPOINT. However, our search shows that several other parts of the codebase (e.g., insrc/utils/apiRequests/api.ts,src/hooks/useServerApi.ts, and others) still depend onprocess.env.API_ENDPOINT. Please ensure that:
- Environment Consistency: The new
config.urlvalue is appropriately configured for all environments, and its usage in apiClient.ts is aligned with how API endpoints are handled elsewhere.- Configuration Separation: The coexistence of
config.urlandprocess.env.API_ENDPOINTis intentional. Review if this split in configuration sources might lead to inconsistencies or unexpected behavior in different deployment setups.- Documentation & Testing: Update any relevant documentation and perform end-to-end tests to guarantee that URL construction in all modules (both in apiClient.ts and in remaining references) produces the correct results.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/hooks/useServerApi.ts (2)
110-110: Merging user-provided headers last allows overriding default headers.
If you want to prevent important headers likeAuthorizationortenant-keyfrom being overwritten, consider filtering keys out ofadditionalHeadersbefore merging.
113-119: Minor nitpick on version assignment and content type logic.
Currently, the code sets'x-accept-versions' = version ? version : '1.0.3'insideif (version) { ... }, which never uses'1.0.3'unlessversionis a falsy value but still meets theif (version)condition. You might simplify by removing theifblock and using the nullish coalescing operator:- if (version) { - headers['x-accept-versions'] = version ? version : '1.0.3'; - } + headers['x-accept-versions'] = version ?? '1.0.3';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useServerApi.ts(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/hooks/useServerApi.ts (2)
src/utils/apiRequests/apiClient.ts (1) (1)
RequestOptions(4-11)src/utils/apiRequests/impersonation.tsx (1) (1)
ImpersonationData(1-4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
src/hooks/useServerApi.ts (16)
79-79: Importing the next-intluseLocalehook is a welcome improvement.
This will provide a more robust and standardized mechanism for retrieving the current locale.
86-86: Good use of theuseLocalehook.
Retrieving the locale within the hook ensures consistent and dynamic localization across API requests.
101-102: Nice addition ofadditionalHeadersto the request options.
This extension point helps make the API client more flexible for future customization.
107-107: Ensuringx-localeis set fromlocale
This effectively aligns with the localization strategy. Just double-check with your backend team that they accept this header for language negotiation.
131-131: Good call folding impersonation headers into a final header object.
This centralizes the header manipulation and keeps the call site clean.
133-137: Fail fast for missingAPI_ENDPOINT.
Throwing an error early here is preferable to silent failures or unclear error states later on.
139-139: Constructing the final URL withisAbsoluteUrlcheck.
Allows flexible usage of full remote endpoints or paths relative toAPI_ENDPOINT. Nicely done.
147-147: PassingfinalHeaderasadditionalHeaders
Keeps consistency with theRequestOptionsinterface and integrates impersonation logic seamlessly.
158-161: UsingRecord<string, string>instead of{}
This aligns with the previous lint guidance and clarifies the payload’s structure.
175-181:postApiAuthenticatedupdated to type payload and accept custom headers.
This ensures stricter typing for request payload and more flexibility for specialized headers.
189-189: ForwardingadditionalHeaders
Consistently merges the custom headers in the API call—maintains the improved design.
191-197:getApimethod signature improvements
Switching from generic{}toRecord<string, string>reflects better type safety for query params.Also applies to: 199-199
202-207:postApimethod simplified to typed payload
Ensures uniform method signatures and consistent use of typed objects—nice improvement.
212-217:putApimethod typed similarly for clarity and consistency
Good to see uniform usage ofRecord<string, string>for request payload types.
222-227:putApiAuthenticatedmethod matches the typed approach
This matches the same pattern for authenticated calls—keeps everything standard.
237-237:deleteApiAuthenticatedno longer accepts a payload
Removing the unused payload parameter for DELETE calls prevents confusion and aligns with typical REST usage.
- rename baseUrl -> apiBaseUrl
…equestConfig → options) to make it clearer and more concise.
…tricter payload rules
- updated getApiAuthenticated, getApi, and deleteApiAuthenticated to use stricter type constraints. -removed unnecessary default payload initialization in postApiAuthenticated, postApi, and putApi.
- removed the default version value
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/hooks/useApi.ts (2)
93-160: Consider optimizing error handling incallApi.While the logic is good, if the backend returns invalid JSON on an error response,
err instanceof APIErroris appropriate, yet additional guards or fallback message handling might be beneficial for unexpected payload formats.
161-321: Reduce duplication in similar API methods.All helper methods (
getApi,postApi, etc.) follow a very similar pattern. You could unify them by passing anauthRequiredflag and other method-specific parameters to a single function, reducing repetitive code.src/utils/apiRequests/apiClient.ts (1)
84-128: Handle unexpected JSON parsing errors gracefully.Returning
truefor a 204 is sensible, but consider handling the case where the response body isn’t valid JSON on non-2xx status codes. For example, an error page or HTML response could causeawait response.json()to fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/features/projectsV2/ProjectDetails/index.tsx(4 hunks)src/features/projectsV2/ProjectsContext.tsx(4 hunks)src/features/user/DonationReceipt/DonationReceiptWrapper.tsx(4 hunks)src/hooks/useApi.ts(1 hunks)src/utils/apiRequests/apiClient.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features/projectsV2/ProjectsContext.tsx
- src/features/user/DonationReceipt/DonationReceiptWrapper.tsx
- src/features/projectsV2/ProjectDetails/index.tsx
🧰 Additional context used
🧬 Code Definitions (1)
src/hooks/useApi.ts (1)
src/utils/apiRequests/apiClient.ts (1)
RequestOptions(4-38)
🪛 Biome (1.9.4)
src/hooks/useApi.ts
[error] 91-91: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/hooks/useApi.ts (1)
1-66: Great documentation coverage.These introductory doc comments provide thorough usage examples, dependencies, and helpful references, which make onboarding to this hook significantly easier.
src/utils/apiRequests/apiClient.ts (2)
4-38: Type definition aligns well with strict request semantics.The union type for method-specific payload enforcement is a clear way to ensure
POSTandPUTalways havedata. Nicely done!
50-53: Well-structured URL check.
isAbsoluteUrlis concise and properly tested against both HTTP and HTTPS patterns.
| type ApiConfig< | ||
| P extends Record<string, unknown>, | ||
| M extends HttpMethod = 'GET' | 'DELETE' | ||
| > = { | ||
| queryParams?: Record<string, string>; | ||
| impersonationData?: ImpersonationData; | ||
| additionalHeaders?: Record<string, string>; | ||
| version?: string; | ||
| } & (M extends 'POST' | 'PUT' ? { payload: P } : {}); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine the empty type in the union.
When specifying : {}, prefer an explicit shape or Record<string, never> to avoid confusion with banned {} (which means “any non-nullable value”). This helps keep types more precise and avoids linter warnings.
-type ApiConfig<
- P extends Record<string, unknown>,
- M extends HttpMethod = 'GET' | 'DELETE'
-> = {
- ...
-} & (M extends 'POST' | 'PUT' ? { payload: P } : {});
+type ApiConfig<
+ P extends Record<string, unknown>,
+ M extends HttpMethod = 'GET' | 'DELETE'
+> = {
+ ...
+} & (M extends 'POST' | 'PUT' ? { payload: P } : Record<string, never>);📝 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.
| type ApiConfig< | |
| P extends Record<string, unknown>, | |
| M extends HttpMethod = 'GET' | 'DELETE' | |
| > = { | |
| queryParams?: Record<string, string>; | |
| impersonationData?: ImpersonationData; | |
| additionalHeaders?: Record<string, string>; | |
| version?: string; | |
| } & (M extends 'POST' | 'PUT' ? { payload: P } : {}); | |
| type ApiConfig< | |
| P extends Record<string, unknown>, | |
| M extends HttpMethod = 'GET' | 'DELETE' | |
| > = { | |
| queryParams?: Record<string, string>; | |
| impersonationData?: ImpersonationData; | |
| additionalHeaders?: Record<string, string>; | |
| version?: string; | |
| } & (M extends 'POST' | 'PUT' ? { payload: P } : Record<string, never>); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Updated the type to Record<string, unknown>
There was a problem hiding this comment.
🧩 Analysis chain
@sunilsabatp Thanks for updating the type. I notice you chose Record<string, unknown> instead of the suggested Record<string, never>.
While both avoid the banned {} type, there's a subtle difference: Record<string, never> represents a truly empty object (cannot have properties), whereas Record<string, unknown> allows any string keys with unknown values.
The original intent was likely to ensure GET/DELETE methods don't have a payload property. If you want to strictly enforce this, Record<string, never> would be more appropriate. However, if you're intentionally allowing additional properties for flexibility, your choice of Record<string, unknown> works too.
🏁 Script executed:
#!/bin/bash
# Check the current implementation in the file
grep -A 10 "type ApiConfig" src/hooks/useApi.ts
Length of output: 403
@sunilsabatp I've confirmed your implementation using Record<string, unknown> in the conditional type. This change successfully addresses the linter warning about the banned {} type.
Just for your awareness: with Record<string, unknown>, GET/DELETE methods can technically still accept additional properties (including a payload) with values of any type. If you intended to strictly prohibit additional properties for those methods, Record<string, never> would have been more restrictive. However, your implementation works fine and might offer more flexibility if needed.
✏️ Learnings added
Learnt from: sunilsabatp
PR: Plant-for-the-Planet-org/planet-webapp#2455
File: src/hooks/useApi.ts:0-0
Timestamp: 2025-03-30T13:17:53.053Z
Learning: For conditional types in TypeScript that represent empty objects, `Record<string, never>` is preferred over `{}` to avoid linter warnings about banned types. `Record<string, never>` represents a truly empty object that cannot have properties, while `Record<string, unknown>` allows arbitrary properties with unknown values.
Learnt from: sunilsabatp
PR: Plant-for-the-Planet-org/planet-webapp#2455
File: src/hooks/useApi.ts:0-0
Timestamp: 2025-03-30T13:17:53.053Z
Learning: For conditional types in TypeScript that represent empty objects, `Record<string, never>` is preferred over `{}` to avoid linter warnings about banned types. `Record<string, never>` represents a truly empty object that cannot have properties, while `Record<string, unknown>` allows arbitrary properties with unknown values.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…adability. -Explicitly used Record<string, unknown> for ApiConfig, ensuring a more consistent structure. -Improved readability of requestOptions, making debugging easier.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/hooks/useApi.ts (4)
83-92: Refine the union type for non-POST/PUT methods.Currently, the type
(M extends 'POST' | 'PUT' ? { payload: P } : Record<string, unknown>)broadens the shape for GET/DELETE calls toRecord<string, unknown>. Consider opting for a stricter type (e.g.,Record<string, never>) to enforce that GET/DELETE calls cannot accidentally contain payloads, leading to more precise type checks.-type ApiConfig< - P extends Record<string, unknown>, - M extends HttpMethod = 'GET' | 'DELETE' -> = { - queryParams?: Record<string, string>; - ... -} & (M extends 'POST' | 'PUT' ? { payload: P } : Record<string, unknown>); +type ApiConfig< + P extends Record<string, unknown>, + M extends HttpMethod = 'GET' | 'DELETE' +> = { + queryParams?: Record<string, string>; + ... +} & (M extends 'POST' | 'PUT' ? { payload: P } : Record<string, never>);
93-148: Centralized error handling is commendable but consider additional logging or retry flow.The
callApifunction cleanly focuses on critical error cases (APIErrorandClientError). Consider adding optional hooks for retry logic or more detailed logging (e.g., request ID or correlation ID) to aid debugging, especially in production environments.
135-137: Consolidate data assignment logic for all methods.You conditionally include
datafor POST/PUT only. If you anticipate future expansions (e.g., allowing bodies on DELETE requests), you might unify or handle them consistently to reduce repeated conditional logic. For now, it’s acceptable, but keep this in mind if expansions occur.
311-320: Consider a more unified approach or an exposed object.Returning all methods in a single object is convenient, but consider grouping them (e.g., returning an object keyed by method type). This remains optional—your current design is perfectly usable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useApi.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/hooks/useApi.ts (1)
src/utils/apiRequests/apiClient.ts (1)
RequestOptions(4-38)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint on Ubuntu
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/hooks/useApi.ts (3)
1-66: Well-documented introduction and usage summary.The JSDoc comments covering the purpose, dependencies, and usage are thorough and will be helpful for onboarding and maintenance. Great work keeping the documentation succinct yet comprehensive.
197-238: Well-organized method wrappers for GET and POST.Splitting out
getApi,postApi, and their authenticated versions is clear and consistent with the rest of the file. This code is easy to navigate and code review. No issues observed here.
293-309:❓ Verification inconclusive
Validate “DELETE” payload usage.
Since
deleteApiAuthenticateddoes not accept a payload, ensure that any usage does not erroneously pass one. If required in the future, you could handle body content or remove it entirely from your type definitions.Please run a quick check to confirm no code tries to pass a payload to your DELETE method:
🏁 Script executed:
#!/bin/bash # Searching for any calls to deleteApiAuthenticated that provide a second argument with payload ast-grep --pattern $'deleteApiAuthenticated($_, { payload: $$$ })'Length of output: 67
🏁 Script executed:
#!/bin/bash # Re-run a search for any calls to deleteApiAuthenticated that include "payload" rg -n "deleteApiAuthenticated\\s*\\([^)]*payload"Length of output: 49
DELETE payload usage validated – no payloads found in deleteApiAuthenticated calls
Our searches (using both ast-grep and rg) did not reveal any instances where a payload property is passed to the DELETE method. This confirms that the current implementation is not being misused. Please manually verify across all client usages to ensure no inadvertent payloads are introduced in the future.
- Verified with an ast-grep search for calls with
{ payload: ... }and an rg search for any payload-related arguments.
Branch Name Validation Changes:
API Refactoring Work:
useServerApi()hook to fetch project and plant location dataContent-Type' to 'application/json' for 'POST', 'PUT', and 'DELETE'requestsBug Fixes:
'x-accept-versions'header is only added if a version is providedCode Cleanup:
NOTE : For future versioning, we could use an environment variable.