Internal requests bypassing rate limits#1609
Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
WalkthroughAdds server-side request signing utilities and schema/accessors, an SSR fetch wrapper that injects internal auth headers and overrides global fetch during server render, refactors API URL/error handling with a retry-capable GET, and updates CI workflow and env placeholders for SSR secrets. (39 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Layout as app/layout.tsx
participant SSRFetch as ssrFetch (enhancedFetch)
participant Env as getServerEnvOrThrow
participant Signer as server-signature.helpers
participant Native as original fetch
participant API as API Server
Layout->>SSRFetch: import/initialize (server)
SSRFetch->>SSRFetch: isApiRequest(url)?
alt API request & server env present
SSRFetch->>Env: getServerEnvOrThrow()
Env-->>SSRFetch: {SSR_CLIENT_ID, SSR_CLIENT_SECRET}
SSRFetch->>Signer: generateClientSignature(...) & generateWafSignature(...)
Signer-->>SSRFetch: signature, wafSignature, timestamp
SSRFetch->>SSRFetch: merge headers + x-6529-internal-*
SSRFetch->>Native: fetch(url, { headers: augmented, ... })
else non-API or missing creds
SSRFetch->>Native: fetch(url, init)
end
Native->>API: HTTP request
API-->>Native: Response
Native-->>SSRFetch: Response
SSRFetch-->>Layout: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)lib/fetch/ssrFetch.ts (3)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
next.config.mjs (1)
201-209: Remove SSR_CLIENT_ID and SSR_CLIENT_SECRET from publicEnvSchema.These secrets must not be part of the public environment schema. They violate the documented design principle which states only
NEXT_PUBLIC_prefixed or explicitly exposed variables should be public. Since no separate server-only schema exists, remove these frompublicEnvSchemainconfig/env.schema.ts(lines 49–50). If these values are needed at build time, handle them separately outside the public schema validation.
🧹 Nitpick comments (2)
helpers/server.app.helpers.ts (1)
22-49: Use server-only env for secrets and avoid duplicating internal header logicThe runtime behavior of
getClientIdHeaderslooks correct (server-only guard, safe fallback to{}), but there are two concerns:
Secret source is
publicEnv
clientSecretis read frompublicEnv.SSR_CLIENT_SECRET, which is currently part of the public env schema. Even if this helper is server-only, usingpublicEnvfor secrets is a smell and ties you to an unsafe exposure model.Once
SSR_CLIENT_SECRETis moved to a server-only schema (see comment inconfig/env.schema.ts), update this helper to read from that server env instead, e.g.:import { serverEnv } from "@/config/env"; // or equivalent const clientId = serverEnv.SSR_CLIENT_ID; const clientSecret = serverEnv.SSR_CLIENT_SECRET;Duplicated header/signature construction
The logic here is effectively duplicated inside
getHeadersinservices/api/common-api.ts(samex-6529-internal-*headers and signature generation). This duplication invites drift if the signing scheme ever changes.Consider centralizing this logic:
- Keep
getClientIdHeaders(method, path)as the single implementation.- In
common-api.ts, callgetClientIdHeaders(method, path)and merge its result intobaseHeadersinstead of reimplementing the signing logic there.This keeps the signing format consistent across all internal callers and simplifies future changes.
services/api/common-api.ts (1)
73-190: Retry helper is well-structured; consider mild validation of optionsThe new
RetryOptionsandcommonApiFetchWithRetrylogic are generally robust:
- Wraps
commonApiFetchin a retry loop with configurablemaxRetries,initialDelayMs,backoffFactor, andjitter.- Properly respects
AbortSignal:
- Throws immediately if already aborted.
- Rethrows if aborted during a request or during the delay (via
Promise.race).- Uses exponential backoff plus jitter to avoid thundering herd behavior.
- Logs retries with useful context.
If you want to tighten it further (optional):
- Clamp
jitterto a non-negative value (e.g.,Math.max(0, jitterFactor)) so a misconfigured negative jitter doesn’t accidentally shrink delay to near-zero.- Optionally guard against
backoffFactor <= 0(which could freeze or invert backoff), defaulting to2in that case.These are defensive tweaks; the current implementation is otherwise sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env.sample(1 hunks).github/workflows/build-upload-deploy-prod.yml(1 hunks)config/env.schema.ts(1 hunks)helpers/server-signature.helpers.ts(1 hunks)helpers/server.app.helpers.ts(2 hunks)next.config.mjs(1 hunks)services/api/common-api.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
helpers/server.app.helpers.ts (2)
next.config.mjs (2)
publicEnv(209-209)publicEnv(269-269)helpers/server-signature.helpers.ts (1)
generateClientSignature(3-25)
next.config.mjs (1)
config/env.ts (1)
publicEnv(7-7)
services/api/common-api.ts (2)
services/auth/auth.utils.ts (2)
getStagingAuth(58-60)getAuthJwt(62-67)helpers/server-signature.helpers.ts (1)
generateClientSignature(3-25)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
services/api/common-api.ts (2)
41-71: Path-aware signing integration forcommonApiFetchlooks consistentThe changes in
commonApiFetch:
- Construct
pathas/api/${endpoint}andurlas${publicEnv.API_ENDPOINT}${path}.- When
paramsexist, build a query string and append it to bothurlandpath.- Pass
pathand"GET"intogetHeadersso the signature includes the same path the server sees.This is coherent and keeps
pathandurlaligned (including query params), which is important for signature verification. Aside from the broader concerns about where signing is implemented and where secrets come from, this specific path/signature wiring is solid.
192-336: Header/path updates across POST/DELETE/PUT/FORM are consistent with the signing modelFor
commonApiPost,commonApiPostWithoutBodyAndResponse,commonApiDelete,commonApiDeleteWithBody,commonApiPut, andcommonApiPostForm:
- Each constructs
path = /api/${endpoint}and (where applicable) appends the query string to bothurlandpath.- Each passes the correct HTTP method string (
"POST","DELETE","PUT") and the computedpathintogetHeaders.This keeps the signature inputs (
method,path) consistent with the actual request and mirrors the GET implementation. Once the earlier concerns about secret sourcing and server-only placement of signing logic are addressed, these call sites look good..env.sample (1)
46-49: Sample vars look good; ensure realSSR_CLIENT_SECRETstays server-onlyThe added sample entries clearly document the purpose of
SSR_CLIENT_ID/SSR_CLIENT_SECRETfor HMAC-based SSR request signing, which is helpful for setup.Just align their actual usage with the security model:
- Keep real
SSR_CLIENT_SECRETvalues only in server-side configuration (CI secrets, server env).- Avoid exposing
SSR_CLIENT_SECRETvia any public runtime env or client bundles (see comments onconfig/env.schema.tsandcommon-api.ts).The sample placeholders themselves are fine.
helpers/server-signature.helpers.ts (1)
1-25: Backend verification code not found; manual verification requiredThe client-side implementation is cryptographically sound and consistently calls
generateClientSignature(clientId, clientSecret, method, path)in bothservices/api/common-api.tsandhelpers/server.app.helpers.ts, properly transmitting the signature in headers.However, no backend verification logic was found in this codebase. Since the original review comment correctly flagged canonicalization matching as critical (the backend must reconstruct the exact payload
${clientId}\n${ts}\n${method}\n${path}), you must manually confirm:
- The backend verification code uses the identical payload format and HMAC-SHA256 verification.
- Query string handling, path leading slashes, and HTTP method casing are consistent between client and backend.
If the backend is in a separate repository, coordinate with that team to ensure alignment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/api/common-api.ts (1)
168-168: Replace Node-specificNodeJS.Timeouttype for browser compatibility.Line 168 uses
NodeJS.Timeout, which is a Node.js-specific type. In browser environments,setTimeoutreturns anumber, not aNodeJS.Timeoutobject. Sincecommon-api.tsis imported from client components (as confirmed in past reviews), this can cause type errors.Apply this diff:
- let timeoutId: NodeJS.Timeout | undefined = undefined; + let timeoutId: ReturnType<typeof setTimeout> | undefined = undefined;This uses
ReturnType<typeof setTimeout>, which correctly resolves tonumberin browser andNodeJS.Timeoutin Node environments.
🧹 Nitpick comments (5)
services/api/common-api.ts (3)
18-38: Remove unusedpathreturn value.The
pathvariable is computed and returned but never used by any caller (all callers only destructureurl). This wastes computation building query strings twice.Apply this diff to simplify:
const buildUrlAndPath = ( endpoint: string, params?: Record<string, string>, transformParams?: (params: Record<string, string>) => Record<string, string> -): { url: string; path: string } => { - let path = `/api/${endpoint}`; - let url = `${publicEnv.API_ENDPOINT}${path}`; +): string => { + let url = `${publicEnv.API_ENDPOINT}/api/${endpoint}`; if (params) { const queryParams = new URLSearchParams(); const processedParams = transformParams ? transformParams(params) : params; Object.entries(processedParams).forEach(([key, value]) => { queryParams.set(key, value); }); const queryString = queryParams.toString(); url += `?${queryString}`; - path += `?${queryString}`; } - return { url, path }; + return url; };Then update all callers from
const { url } = buildUrlAndPath(...)toconst url = buildUrlAndPath(...).
40-46: Make error handling more robust for non-JSON responses.If the response body is not valid JSON,
res.json()will throw, causing an unhandled error instead of falling back tores.statusText.Apply this diff:
const handleApiError = (res: Response): Promise<never> => { - return res.json().then((body: any) => { - return Promise.reject( - body?.error ?? res.statusText ?? "Something went wrong" - ); - }); + return res.json() + .then((body: { error?: string }) => { + return Promise.reject( + body?.error ?? res.statusText ?? "Something went wrong" + ); + }) + .catch(() => { + return Promise.reject(res.statusText || "Something went wrong"); + }); };
143-146: Use the existingsecurity-loggerutility from the codebase for retry logging.The codebase has an established logging utility at
src/utils/security-logger.tswithlogError()function that is already used elsewhere (e.g.,WalletErrorBoundary.tsx,Auth.tsx). Replace the directconsole.warn()call on line 143 with this utility for consistency and to benefit from the built-in security sanitization and environment-aware logging.Example:
logError('api-retry', error)or create a dedicated logging callback inretryOptionsthat can be configured per caller.helpers/server-signature.helpers.ts (1)
1-25: Signature helper looks sound; consider marking the module as server-onlyThe HMAC construction, payload shape, and timestamp handling all look correct for a simple client-id / timestamp / method / path signature. The runtime guard against
globalThis.windowalso ensures it is not executed in the browser.To reduce the chance of accidental client-side imports (which could end up bundling secrets) you might consider explicitly marking this helper as server-only (e.g., via a small wrapper module or a
server-onlyimport at the top, depending on your existing pattern).app/layout.tsx (1)
3-3: Side-effect import is fine; keepRootLayoutserver-onlyImporting
"@/lib/fetch/ssrFetch"here is a reasonable way to ensure the SSR fetch wrapper is registered early.One thing to watch: if
app/layout.tsxis ever converted into a client component ("use client"), this side-effect import will try to bundle a server-only module (Node crypto, server env), which will fail. Worth documenting that this layout must remain a server component while using this pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/layout.tsx(1 hunks)helpers/server-signature.helpers.ts(1 hunks)lib/fetch/ssrFetch.ts(1 hunks)services/api/common-api.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/fetch/ssrFetch.ts (1)
helpers/server-signature.helpers.ts (1)
generateClientSignature(3-25)
services/api/common-api.ts (1)
next.config.mjs (2)
publicEnv(209-209)publicEnv(269-269)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
services/api/common-api.ts (2)
197-221: LGTM!The refactor correctly uses
buildUrlAndPathfor URL construction andhandleApiErrorfor consistent error handling.
223-326: LGTM!All remaining API helpers (
commonApiPostWithoutBodyAndResponse,commonApiDelete,commonApiDeleteWithBody,commonApiPut,commonApiPostForm) correctly adopt thebuildUrlAndPathandhandleApiErrorpattern, maintaining consistency across the module.lib/fetch/ssrFetch.ts (1)
45-50: The original review comment's concern is invalid—SSR_CLIENT_SECRET is not exposed to browser bundles.The variable
SSR_CLIENT_SECRETis included innext.config.mjsline 256 in theenvobject without theNEXT_PUBLIC_prefix. Per Next.js standard behavior, environment variables in theenvconfig object are only inlined into browser bundles if they have theNEXT_PUBLIC_prefix. SinceSSR_CLIENT_SECRETlacks this prefix, it will not be included in client-side bundles.In
config/env.ts, the client-side code attempts to load fromprocess.env.PUBLIC_RUNTIME, which will beundefinedin the browser, causing the schema parser to receive an empty object. The schema requiresSSR_CLIENT_SECRETas a non-empty string, so any client component attempting to importpublicEnvwould fail validation—but this is a separate reliability issue, not a security leak.The
ssrFetchmodule is imported atapp/layout.tsxline 3 without a "use client" directive, making it server-only. The module does not expose the secret to client code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build-upload-deploy-prod.yml (1)
39-53: LGTM! Secrets correctly excluded from build step.The build environment no longer includes
SSR_CLIENT_IDandSSR_CLIENT_SECRET, which correctly prevents these server-side secrets from being embedded in the client bundle. This addresses the critical security concern from the previous review.
🧹 Nitpick comments (2)
config/serverEnv.ts (2)
3-5: Consider deferring the client-side check to runtime.The top-level throw executes during module evaluation. If this module is accidentally imported on the client side (e.g., via a shared import in Next.js), it will crash immediately. Consider moving this check into the
getServerEnvfunction where it will only execute when actually called.Apply this diff to defer the check:
-if (typeof globalThis.window !== "undefined") { - throw new TypeError("serverEnv can only be accessed on the server side"); -} - const getServerEnv = (): ServerEnv | null => { + if (typeof globalThis.window !== "undefined") { + throw new TypeError("serverEnv can only be accessed on the server side"); + } + if (typeof process === "undefined" || !process.env) { return null; }
17-20: Consider logging validation errors for easier debugging.When validation fails, the function silently returns
nullwithout logging the specific validation errors. This can make troubleshooting configuration issues more difficult.Apply this diff to add error logging:
const parsed = serverEnvSchema.safeParse(raw); if (!parsed.success) { + console.error("Server environment validation failed:", parsed.error.format()); return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-upload-deploy-prod.yml(1 hunks)config/serverEnv.schema.ts(1 hunks)config/serverEnv.ts(1 hunks)lib/fetch/ssrFetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/fetch/ssrFetch.ts
🧰 Additional context used
🧬 Code graph analysis (1)
config/serverEnv.ts (1)
config/serverEnv.schema.ts (2)
ServerEnv(8-8)serverEnvSchema(3-6)
🔇 Additional comments (3)
config/serverEnv.schema.ts (1)
1-9: LGTM! Clean schema definition.The Zod schema properly validates the required SSR credentials with clear error messages and appropriate constraints. The type inference setup is correct.
config/serverEnv.ts (1)
25-35: LGTM! Clear error handling and caching.The
getServerEnvOrThrowfunction provides a clean API with a descriptive error message, and the cachedserverEnvexport is appropriate for this use case..github/workflows/build-upload-deploy-prod.yml (1)
83-91: AWS CLI syntax is correct.The AWS CLI supports space-separated multiple entries after a single parameter flag using shorthand syntax, which is what the code uses. AWS Elastic Beanstalk's
update-environmentcommand accepts--option-settingsin both JSON file format and shorthand inline format with comma-separated key-value pairs. The code's approach of passing multiple option-settings as space-separated entries after a single--option-settingsflag conforms to AWS CLI shorthand syntax conventions and is valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
config/serverEnv.ts (1)
3-35: Improve diagnostics when server env validation failsThe overall pattern looks good (server-only guard + Zod validation), but
getServerEnvcurrently swallows Zod parse errors and just returnsnull, so callers only see the generic message fromgetServerEnvOrThrow. That makes it harder to debug misconfigured environments (e.g., empty values vs missing vars).Consider either:
- Including the Zod error message in the thrown
Error, or- Logging/
console.error‑ing validation issues insidegetServerEnvbefore returningnull.This will make SSR misconfigurations much easier to track down in staging/production while keeping the public surface the same.
eslint.config.mjs (1)
84-92: ESLint ignore forserverEnvis consistent with existing env handlingAdding
config/serverEnv.tsto the ignore list mirrors howconfig/env.tsis treated and avoids conflict with theprocess.envrestriction rule. That’s reasonable for these small server-only config helpers. If this file grows more complex over time, consider switching to a more targeted override instead of a full ignore.helpers/server-signature.helpers.ts (1)
1-25: Signature helper looks solid; consider tightening method/timestamp semanticsThe HMAC construction and server-only guard look good and match the intended use for internal signing.
To reduce subtle signature mismatches between client and verifier, consider:
- Normalizing
methodinside the helper (e.g.,method = method.toUpperCase().trim()), and/or- Making the timestamp intent explicit (e.g., rename to
timestampSecondsor validate that an override is in seconds, not ms).These are minor, but they make the API harder to misuse.
services/api/common-api.ts (5)
18-38: URL builder with transform hook looks good; consider endpoint normalization / path usage
buildUrlAndPathnicely centralizes URL construction and thetransformParamshook is a clean way to keep query param transformations out of the generic helper.Two minor follow-ups to consider:
- Normalize
endpointto avoid double slashes inpath/url(e.g.,const normalized = endpoint.replace(/^\/+/, "");).- Since
pathisn’t used in this module yet, either document that it’s intended for internal signing (SSR fetch) or defer adding it until it’s actually consumed.Both are small polish items; behavior today is fine.
40-67: Make error handling resilient to non‑JSON responses / empty bodies
handleApiErrorandexecuteApiRequestassume all responses (including error responses and void-returning endpoints) are valid JSON. If an upstream returns HTML/text (e.g., a proxy 502 page) or a 204/empty body,res.json()will throw aSyntaxError, and callers will see that rather than the more useful status text.Consider:
- Wrapping
res.json()in a try/catch and falling back tores.text()orres.statusText, and/or- Special-casing
res.status === 204to resolve toundefinedwhen the generic type isvoid.This keeps the shared helpers robust across a wider set of API behaviors without changing the call sites.
75-85: Param transform for nic→cic is now localized; consider documenting the behaviorMoving the nic→cic mapping into
commonApiFetchviatransformParamsis an improvement over having it embedded in the generic URL helper.Since this still applies to all callers of the generic GET helper, it would help future readers if you briefly document why
"nic"must be rewritten to"cic"(e.g., protocol/versioning requirement) so it doesn’t look arbitrary to consumers ofcommonApiFetch.
99-216: Retry loop is well-structured; minor simplifications possible around abort handlingThe retry logic correctly:
- Honors
maxRetries(as “extra attempts” after the initial call),- Uses exponential backoff with jitter, and
- Respects
AbortSignalboth before each attempt and while sleeping.A couple of small cleanups you might consider:
- Use
ReturnType<typeof setTimeout>instead ofNodeJS.TimeoutfortimeoutIdto keep types portable between Node and browser environments.- You can simplify the abort-delay race by reusing a single timeout for both the delay and listener cleanup, or by extracting the abort‑aware sleep into a small helper to reduce cognitive load.
Functionally this looks solid; these are just maintainability tweaks.
243-250: POST without body/response now reuses shared executor
commonApiPostWithoutBodyAndResponsecorrectly builds the URL and delegates toexecuteApiRequest<void>with an empty body and JSON content-type; this keeps error handling uniform. Just ensure the backing API always tolerates an empty JSON body for this endpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build-upload-deploy-prod.yml(1 hunks)config/serverEnv.ts(1 hunks)eslint.config.mjs(1 hunks)helpers/server-signature.helpers.ts(1 hunks)lib/fetch/ssrFetch.ts(1 hunks)services/api/common-api.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/fetch/ssrFetch.ts
- .github/workflows/build-upload-deploy-prod.yml
🧰 Additional context used
🧬 Code graph analysis (2)
services/api/common-api.ts (1)
next.config.mjs (2)
publicEnv(209-209)publicEnv(267-267)
config/serverEnv.ts (1)
config/serverEnv.schema.ts (2)
ServerEnv(8-8)serverEnvSchema(3-6)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
services/api/common-api.ts (5)
225-236: POST helper refactor viabuildUrlAndPath/executeApiRequestRefactoring
commonApiPostto use the shared URL builder andexecuteApiRequestimproves consistency and keeps header/body wiring centralized. No behavioral issues spotted.
257-263: DELETE helper refactor is straightforward
commonApiDeletenow usesbuildUrlAndPathandexecuteApiRequest<void>with the appropriate headers (no JSON content-type). This matches expectations for a bodyless DELETE.
276-286: DELETE with body correctly uses shared executor
commonApiDeleteWithBody’s refactor is consistent with POST/PUT: URL viabuildUrlAndPath, JSON body viaJSON.stringify, and shared error handling viaexecuteApiRequest. Looks good.
295-305: PUT helper aligned with shared request pipeline
commonApiPutnow mirrors the POST helper’s structure withbuildUrlAndPath+executeApiRequest, which keeps the API surface consistent and reduces duplication.
313-320: Form POST helper correctly avoids JSON content-type
commonApiPostFormnow builds the URL viabuildUrlAndPathand delegates toexecuteApiRequest, while correctly passingcontentType=falseso the browser can set the multipart/form-data boundary on theFormDatabody.
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
helpers/server-signature.helpers.ts (2)
27-37: Add JSDoc documentation for the WAF signature.The function logic is correct and straightforward. However, adding JSDoc comments would help clarify:
- What is the WAF signature used for?
- How does it differ from the client signature?
- What are the expected inputs and outputs?
Example JSDoc:
/** * Generates a WAF (Web Application Firewall) signature for server-side requests. * This signature authenticates the client identity to the WAF layer. * * @param clientId - The client identifier * @param secret - The shared secret for signing * @returns The hex-encoded HMAC-SHA256 signature * @throws {TypeError} If called in a browser context */ export function generateWafSignature(clientId: string, secret: string): string { // ... existing implementation }
10-14: Consider extracting the server-side guard to reduce duplication.Both functions share identical server-side validation logic. Extracting this to a shared helper would improve maintainability and ensure consistent error messages.
Example refactor:
function assertServerSide(functionName: string): void { if (globalThis.window !== undefined) { throw new TypeError(`${functionName} can only be used on the server side`); } } export function generateClientSignature( clientId: string, secret: string, method: string, path: string, timestamp?: number ): { clientId: string; timestamp: number; signature: string } { assertServerSide("generateClientSignature"); // ... rest of implementation } export function generateWafSignature(clientId: string, secret: string): string { assertServerSide("generateWafSignature"); // ... rest of implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helpers/server-signature.helpers.ts(1 hunks)lib/fetch/ssrFetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/fetch/ssrFetch.ts
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/fetch/ssrFetch.ts (1)
24-32: Consider caching the API origin for minor optimization.The function creates
new URL(publicEnv.API_ENDPOINT)twice (lines 27 and 28). Since the API endpoint is static, you could cachenew URL(publicEnv.API_ENDPOINT).originat module scope to avoid repeated parsing.Example refactor:
+const API_ORIGIN = new URL(publicEnv.API_ENDPOINT).origin; + const isApiRequest = (url: string | URL): boolean => { const urlString = typeof url === "string" ? url : url.toString(); try { const parsedUrl = new URL(urlString, publicEnv.API_ENDPOINT); - return parsedUrl.origin === new URL(publicEnv.API_ENDPOINT).origin; + return parsedUrl.origin === API_ORIGIN; } catch { return false; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/serverEnv.schema.ts(1 hunks)config/serverEnv.ts(1 hunks)lib/fetch/ssrFetch.ts(1 hunks)proxy.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- proxy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- config/serverEnv.ts
🧰 Additional context used
🧬 Code graph analysis (1)
lib/fetch/ssrFetch.ts (3)
next.config.mjs (2)
publicEnv(209-209)publicEnv(267-267)config/serverEnv.ts (1)
getServerEnvOrThrow(26-34)helpers/server-signature.helpers.ts (2)
generateClientSignature(3-25)generateWafSignature(27-37)
🔇 Additional comments (8)
config/serverEnv.schema.ts (1)
1-10: LGTM!The Zod schema is well-structured with appropriate validation rules. The required fields use
.min(1)to ensure non-empty strings, andSTAGING_API_KEY_SERVERis correctly marked as optional for staging environments.lib/fetch/ssrFetch.ts (7)
8-22: Previous concern addressed: defensive fetch guard implemented.The
getOriginalFetch()function properly validatesglobalThis.fetchbefore capture, addressing the prior issue whereoriginalFetchwas captured without guards. The implementation checks for bothundefinedand non-function cases with clear error messages.
34-42: LGTM!The path extraction correctly combines
pathnameandsearchparameters, and handles malformed URLs gracefully by returning an empty string, which triggers fallback to the original fetch.
78-82: Previous concern addressed: method derivation now handles Request inputs.The method is now correctly derived from
init?.method, falling back toinput.methodwheninputis aRequest, and finally defaulting to"GET". This fixes the signature mismatch issue raised in the previous review.
98-104: Previous concern addressed: headers now properly merged from Request and init.The header merging now correctly seeds from
input.headerswheninputis aRequest(line 98), then overlaysinit?.headers(lines 100-104), preventing the loss of original headers likeContent-Type. This fixes the header-dropping issue from the previous review.
48-76: LGTM! Robust fallback strategy.The implementation correctly falls back to
originalFetchin multiple scenarios:
- Browser context (lines 48-50)
- Non-API requests (lines 61-63)
- Missing SSR credentials (lines 74-75)
The silent fallback when credentials are unavailable (line 75) enables graceful degradation, allowing the app to function even if SSR credentials are not configured.
89-121: LGTM! Signature generation and header injection implemented correctly.The implementation:
- Generates client signature with the correct method and path (lines 89-94)
- Generates WAF signature (line 96)
- Sets all required internal headers (lines 106-112)
- Conditionally adds staging API key (lines 114-116)
- Preserves the original
input(including body for POST/PUT) while augmenting headers (lines 118-121)
124-126: LGTM! Global fetch replacement appropriately guarded.The global fetch is only replaced in SSR contexts (when
windowis undefined), ensuring client-side code continues to use the native fetch implementation. The module's import inapp/layout.tsxactivates this patch at the application entry point.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/serverEnv.ts (2)
3-5: Consider using theserver-onlypackage for better DX.While the client-side guard is functional, it throws immediately when the module is imported on the client, which can cause cryptic errors during development if accidentally imported in a client component.
Consider using the
server-onlypackage instead:+import "server-only"; + -if (globalThis.window !== undefined) { - throw new TypeError("serverEnv can only be accessed on the server side"); -}The
server-onlypackage provides better error messages and is the recommended approach in Next.js for server-only modules.
7-23: Simplify the environment check.The check
!process.envon line 8 is redundant becauseprocess.envis always truthy whenprocessexists.Apply this diff:
- if (typeof process === "undefined" || !process.env) { + if (typeof process === "undefined") { return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/serverEnv.schema.ts(1 hunks)config/serverEnv.ts(1 hunks)lib/fetch/ssrFetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/fetch/ssrFetch.ts
🧰 Additional context used
🧬 Code graph analysis (1)
config/serverEnv.ts (1)
config/serverEnv.schema.ts (2)
ServerEnv(8-8)serverEnvSchema(3-6)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
config/serverEnv.ts (2)
25-33: LGTM!The implementation is clean and appropriate. The error message clearly indicates which environment variables are required.
35-35: LGTM!The module-level export provides a convenient cached value. The nullable type is appropriate for cases where environment variables might not be set (e.g., during development or testing).
config/serverEnv.schema.ts (1)
1-8: Schema implementation is solid; Zod version is confirmed.The schema definition is clean, uses appropriate validation with
.min(1)for non-empty strings, and includes helpful custom error messages. Zod version 3.25.76 exists and is the version declared in the repository. The codebase is using an older major version (3.x vs. the current 4.1.12), but this appears intentional and does not affect the quality of the schema implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helpers/server-signature.helpers.ts (1)
30-76: Signature helper looks solid; consider trimming method and validating timestampThe implementation matches the documented scheme and addresses prior concerns (server-only guard, non-empty string validation, uppercase method, clear return shape).
Two small robustness tweaks you might consider:
- Use the trimmed method in the payload
Right now you validate withmethod.trim().lengthbut callmethod.toUpperCase(). If a caller accidentally passes" get"or"GET ", it passes validation but produces a non-canonical method string in the signature payload. Using the trimmed value avoids hard‑to‑spot mismatches:- if (typeof method !== "string" || method.trim().length === 0) { + if (typeof method !== "string" || method.trim().length === 0) { throw new TypeError( "generateClientSignature: method must be a non-empty string" ); } @@ - const normalizedMethod = method.toUpperCase(); + const normalizedMethod = method.trim().toUpperCase();
- Optionally, validate the
timestampshape
To fail fast on misuse (e.g., accidentally passingDate.now()in ms), you could assert that a providedtimestampis an integer Unix timestamp in seconds:if (typeof path !== "string" || path.trim().length === 0) { throw new TypeError( "generateClientSignature: path must be a non-empty string" ); } + + if (timestamp !== undefined && !Number.isInteger(timestamp)) { + throw new TypeError( + "generateClientSignature: timestamp must be an integer Unix timestamp in seconds" + ); + }These are defensive tweaks; the current logic is otherwise in good shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helpers/server-signature.helpers.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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
helpers/server-signature.helpers.ts (1)
3-29: Docstring clearly captures the signature contractThe JSDoc here is thorough (scheme, parameters, replay window, path expectations) and will make it much easier to keep client/server implementations aligned. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
services/api/common-api.ts (4)
18-38: URL builder and nic→cic transform placement look goodCentralizing URL construction in
buildUrlAndPathand keeping thenic→cicmapping in thecommonApiFetchcall-site (viatransformParams) is a cleaner separation than hard-coding the transform inside the generic helper.If you find
transformParamsis only ever used for this single mapping, you could optionally inline that logic intocommonApiFetchto keepbuildUrlAndPathfully generic, but the current shape is still reasonable and readable.Also applies to: 88-98
40-46: Consider hardeningexecuteApiRequestagainst non‑JSON error bodies and JSON parse failuresThe shared execution path is a nice consolidation, but there are a couple of edge cases worth calling out:
handleApiErrorassumesres.json()always succeeds for non‑OK responses. A plain‑text or empty error body will causeres.json()to throw, and the caller will see a low‑signal parse error instead of the status text or a structured message.- For successful responses,
executeApiRequest:
- Returns
undefinedfor non‑JSON content types (which is fine), but- For JSON content types, it unconditionally calls
res.json(), which will again throw on malformed/empty JSON.If you want slightly more robust behavior, you could wrap the JSON parsing in a
try/catchand fall back tores.statusText(for errors) orundefined(for successes) when parsing fails:const safeParseJson = async <T>(res: Response): Promise<T | undefined> => { try { return (await res.json()) as T; } catch { return undefined; } };Then use this helper from both
handleApiErrorandexecuteApiRequest.Also applies to: 48-80
82-110: Consider routingcommonApiFetchthroughexecuteApiRequestfor consistent behavior
commonApiFetchnow shares URL building and the nic→cic param transform, but still does its ownfetch+handleApiError/res.json()instead of delegating toexecuteApiRequest.That means:
commonApiFetchalways attempts to parse JSON (no 204/no‑content handling, no content‑type check).- Other helpers (
POST,PUT,DELETE, form) use the more defensive behavior inexecuteApiRequest.For consistency and to avoid two subtly different code paths, you might want to rewrite this to:
const { url } = buildUrlAndPath(/* ... */); return executeApiRequest<T>( url, "GET", getHeaders(param.headers, false), undefined, param.signal );This keeps all HTTP verbs on the same response- and error-handling semantics.
141-229: Verify retry behavior for 4xx status codes and assess logging impactBoth review concerns are valid and worth addressing:
Retrying 4xx errors is not HTTP best practice. The code currently retries all errors thrown by
commonApiFetch, which includes 4xx responses processed byhandleApiError. Since 4xx errors (400, 403, 404) indicate client-side issues that won't change on retry, they should not be retried. Only transient failures (network errors, 5xx) typically warrant retries. However, changing this would be a breaking change if callers currently depend on this behavior—all three production usages inwave-messages-utils.ts(lines 88, 357, 456) wrap the call in try-catch and don't differentiate error types.
console.warnin production is problematic. The current logging will produce excessive noise in production environments, especially with highermaxRetriesor high-traffic endpoints. Gating behindNODE_ENVor using a centralized logging/telemetry mechanism is the correct approach.Consider:
- Adding error type differentiation (catch/filter 4xx vs transient failures) if it aligns with your API design.
- Replacing
console.warnwith conditional logging or a centralized logging service.- Updating callers' error handling if retry semantics change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/services/common-api.more.test.ts(1 hunks)helpers/server-signature.helpers.ts(1 hunks)services/api/common-api.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- helpers/server-signature.helpers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
services/api/common-api.ts (2)
next.config.mjs (2)
publicEnv(209-209)publicEnv(267-267)__mocks__/@/services/api/common-api.ts (4)
commonApiFetch(11-11)commonApiPostWithoutBodyAndResponse(14-16)commonApiDelete(17-17)commonApiPostForm(20-20)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
services/api/common-api.ts (1)
238-249: Refactored verb helpers correctly delegate to shared executorThe refactor of
commonApiPost,commonApiPostWithoutBodyAndResponse,commonApiDelete,commonApiDeleteWithBody,commonApiPut, andcommonApiPostFormto:
- Use
buildUrlAndPathfor URL construction, and- Delegate the actual HTTP work to
executeApiRequestis a clean de-duplication of the previous fetch logic and keeps content-type and error handling centralized. The
contentTypeflag ingetHeadersis also applied consistently (JSON for body-bearing calls, omitted for DELETE w/o body and form posts).No functional issues stand out here; behavior looks aligned and easier to maintain.
Also applies to: 256-263, 270-276, 289-299, 308-318, 326-333
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/api/common-api.ts (1)
255-272: Inconsistent error handling: usehandleApiError.
commonApiDeletewraps errors inErrorobjects (line 269), while other methods reject with strings viahandleApiError. This inconsistency forces callers to handle different error types.Compare:
- This method:
Promise.reject(new Error(...))handleApiError:Promise.reject(string)commonApiPostWithoutBodyAndResponse(line 251): useshandleApiErrorApply this diff to align error handling:
const { url } = buildUrlAndPath(param.endpoint); const res = await fetch(url, { method: "DELETE", headers: getHeaders(param.headers), }); if (!res.ok) { - const body: any = await res.json(); - return Promise.reject( - new Error(body?.error ?? res.statusText ?? "Something went wrong") - ); + return handleApiError(res); } };
🧹 Nitpick comments (3)
__tests__/services/common-api.more.test.ts (1)
44-44: Minor inconsistency: prefer consistent mock target.Line 44 uses
globalThis.fetchwhile other tests in this file useglobal.fetch(lines 21, 70, 91). Both work, but consistency aids readability.Apply this diff for consistency:
- (globalThis.fetch as jest.Mock).mockResolvedValue({ + (global.fetch as jest.Mock).mockResolvedValue({services/api/common-api.ts (2)
18-38: Consider removing unusedpathreturn value.The
buildUrlAndPathhelper returns bothurlandpath, but all call sites (lines 74, 224, 242, 259, 284, 303, 321) only destructureurl. Thepathvalue appears unused.If
pathis truly unused, simplify the return type:-): { url: string; path: string } => { - let path = `/api/${endpoint}`; +): string => { let url = `${publicEnv.API_ENDPOINT}${path}`; if (params) { const queryParams = new URLSearchParams(); const processedParams = transformParams ? transformParams(params) : params; Object.entries(processedParams).forEach(([key, value]) => { queryParams.set(key, value); }); const queryString = queryParams.toString(); url += `?${queryString}`; - path += `?${queryString}`; } - return { url, path }; + return url;Then update all call sites from
const { url } = buildUrlAndPath(...)toconst url = buildUrlAndPath(...).
74-96: Consider usingexecuteApiRequestfor consistency.
commonApiFetchmanually implements the fetch-then-parse pattern (lines 86-95) thatexecuteApiRequestprovides. Other methods (commonApiPost,commonApiPut, etc.) delegate toexecuteApiRequest.Apply this diff to align with the pattern established elsewhere:
const { url } = buildUrlAndPath( param.endpoint, param.params as Record<string, string> | undefined, (params) => { const transformed: Record<string, string> = {}; Object.entries(params).forEach(([key, value]) => { transformed[key] = value === "nic" ? "cic" : value; }); return transformed; } ); - const res = await fetch(url, { - headers: getHeaders(param.headers, false), - signal: param.signal, - }); - - if (!res.ok) { - return handleApiError(res); - } - - return res.json(); + return executeApiRequest<T>( + url, + "GET", + getHeaders(param.headers, false), + undefined, + param.signal + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__tests__/services/common-api.more.test.ts(4 hunks)services/api/common-api.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/api/common-api.ts (1)
next.config.mjs (2)
publicEnv(209-209)publicEnv(267-267)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
__tests__/services/common-api.more.test.ts (3)
21-26: LGTM! Mock properly structured forexecuteApiRequest.The mock now includes all required Response fields (
status,headers,json) thatexecuteApiRequestexpects.
70-74: LGTM! Correctly structured mock for void response.The mock appropriately omits the
jsonmethod sincecommonApiDeletereturnsPromise<void>and doesn't parse the response body.
91-96: LGTM! Mock properly includes required fields.The mock correctly provides
status,headers, andjsonfor theexecuteApiRequestpathway.services/api/common-api.ts (3)
47-66: LGTM! Helper correctly scoped to JSON responses.
executeApiRequestalways parses responses as JSON (line 65), which is appropriate since it's only called by methods that expect JSON responses (commonApiPost,commonApiDeleteWithBody,commonApiPut,commonApiPostForm). Methods with void responses don't use this helper.
127-215: LGTM! Robust retry implementation with proper abort handling.The retry logic correctly implements:
- Exponential backoff with jitter
- Proper AbortSignal handling throughout (pre-attempt, during delay, post-delay)
- Cleanup of event listeners to prevent leaks
Note: Line 163 uses
console.warnfor retry logging. Consider a configurable logging strategy for production environments.
217-329: LGTM! Methods consistently use new helper pattern.The refactored methods (
commonApiPost,commonApiPostWithoutBodyAndResponse,commonApiDeleteWithBody,commonApiPut,commonApiPostForm) correctly delegate tobuildUrlAndPathfor URL construction and eitherexecuteApiRequest(for JSON responses) orhandleApiError(for void responses).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/services/common-api.postNoBody.test.ts (1)
16-29: Relax strict fetch options assertion to account for extra fields likesignal
executeApiRequestalways passes asignalproperty tofetch(it will beundefinedwhen not provided). The current assertion:expect(globalThis.fetch).toHaveBeenCalledWith( "https://api.test.6529.io/api/e", { method: "POST", headers: { "Content-Type": "application/json", "x-6529-auth": "s" }, body: "", } );will fail because the actual options object includes extra properties (e.g.,
signal). Consider matching only the fields you care about:- expect(globalThis.fetch).toHaveBeenCalledWith( - "https://api.test.6529.io/api/e", - { - method: "POST", - headers: { "Content-Type": "application/json", "x-6529-auth": "s" }, - body: "", - } - ); + expect(globalThis.fetch).toHaveBeenCalledWith( + "https://api.test.6529.io/api/e", + expect.objectContaining({ + method: "POST", + headers: { "Content-Type": "application/json", "x-6529-auth": "s" }, + body: "", + }), + );This keeps the test robust against harmless additions to the options object.
🧹 Nitpick comments (3)
lib/fetch/ssrFetch.ts (2)
105-118: Consider logging app header retrieval failures.While setting internal signature headers (lines 105-111) is correct, the silent catch on lines 113-118 means failures in
getAppCommonHeaders()are invisible. If the cookies API or auth retrieval fails, the request proceeds without app-level auth headers, which might cause unexpected behavior.Consider logging errors for observability:
try { const appHeaders = await getAppCommonHeaders(); Object.entries(appHeaders).forEach(([key, value]) => { enhancedHeaders.set(key, value); }); - } catch {} + } catch (error) { + console.warn("[SSR Fetch] Failed to retrieve app headers:", error); + }
120-121: Consider using a proper logger or making console.log conditional.The
console.logoutputs header keys on every SSR fetch, which could be noisy in production logs. Consider using a structured logger with configurable log levels or guarding this behind a debug flag.Example:
- const headerKeys = Array.from(enhancedHeaders.keys()); - console.log(`[SSR Fetch] Request headers: ${headerKeys.join(", ")}`); + if (process.env.DEBUG_SSR_FETCH) { + const headerKeys = Array.from(enhancedHeaders.keys()); + console.log(`[SSR Fetch] Request headers: ${headerKeys.join(", ")}`); + }services/api/common-api.ts (1)
18-37: Consider tighteningbuildUrlparam/transform typings
buildUrltakesparams?: Record<string, string>and an optionaltransformParams, but callers pass a genericUthen cast toRecord<string, string>. This weakens type-safety for query params.You could make
buildUrlgeneric to preserve types end-to-end:-const buildUrl = ( - endpoint: string, - params?: Record<string, string>, - transformParams?: (params: Record<string, string>) => Record<string, string> -): string => { +const buildUrl = <P extends Record<string, string> = Record<string, string>>( + endpoint: string, + params?: P, + transformParams?: (params: P) => P +): string => { let path = `/api/${endpoint}`; let url = `${publicEnv.API_ENDPOINT}${path}`; - if (params) { + if (params) { const queryParams = new URLSearchParams(); - const processedParams = transformParams ? transformParams(params) : params; + const processedParams = transformParams ? transformParams(params) : params; Object.entries(processedParams).forEach(([key, value]) => { queryParams.set(key, value); });Then callers can pass their concrete param type without unsafe casting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/services/common-api.postNoBody.test.ts(1 hunks)__tests__/services/common-api.test.ts(2 hunks)lib/fetch/ssrFetch.ts(1 hunks)services/api/common-api.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
__tests__/services/common-api.postNoBody.test.ts (1)
services/api/common-api.ts (1)
commonApiPostWithoutBodyAndResponse(254-268)
__tests__/services/common-api.test.ts (1)
services/api/common-api.ts (2)
commonApiFetch(87-112)commonApiPost(233-252)
services/api/common-api.ts (1)
next.config.mjs (2)
publicEnv(209-209)publicEnv(267-267)
lib/fetch/ssrFetch.ts (3)
config/serverEnv.ts (1)
getServerEnvOrThrow(25-33)helpers/server-signature.helpers.ts (2)
generateClientSignature(30-77)generateWafSignature(79-101)helpers/server.app.helpers.ts (1)
getAppCommonHeaders(5-18)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
lib/fetch/ssrFetch.ts (5)
9-21: LGTM! Defensive guards properly implemented.The
getOriginalFetchfunction correctly validates thatfetchis defined and is a function before returning it, with clear error messages for both failure modes. This addresses the previous review concern about undefinedoriginalFetch.
25-43: LGTM! URL parsing logic is sound.Both helper functions correctly handle absolute and relative URLs using the
URLconstructor's base parameter, with appropriate error handling and safe defaults.
49-64: LGTM! Early returns and URL extraction are correct.The function correctly bails out for browser contexts and non-API requests, and handles all three
RequestInfoinput types appropriately.
77-103: LGTM! Method derivation and header merging correctly implemented.The code properly derives the HTTP method from
init, falls back toinput.methodwheninputis aRequest, and defaults to"GET". Header merging now correctly seeds from the originalRequestheaders (if present) and overlaysinit.headers, preserving the original request's headers while allowing overrides. This correctly addresses the previous review feedback.
123-133: LGTM! Final fetch call, global override, and export are correct.The function correctly invokes
originalFetchwith the enhanced headers, and the globalfetchoverride is properly conditioned to SSR-only contexts. The export provides a clean public API.services/api/common-api.ts (3)
61-85: CentralizedexecuteApiRequesthelper looks solidConsolidating fetch +
okcheck + optional JSON parsing intoexecuteApiRequestremoves duplication and ensures consistent error handling and 204/no-body handling viaparseJson = false. The call sites (GET/POST/DELETE/PUT/form) now share a single behavior surface, which is a good maintainability win.
87-112: Good localization of nic→cic mapping tocommonApiFetchonlyMoving the
nic→cictransform into a dedicatedtransformParamscallback forcommonApiFetchkeeps that domain-specific behavior out of the generic URL builder and other verbs. This addresses earlier concerns about mixing business rules into generic helpers while preserving the behavior where it’s needed.
233-341: Refactored POST/DELETE/PUT/form helpers are consistent with the new core helpersThe wrappers now all go through
buildUrlandexecuteApiRequest, withparseJsoncorrectly disabled forcommonApiPostWithoutBodyAndResponseandcommonApiDeleteand enabled where a response body is expected. Header handling remains consistent viagetHeaders. This refactor reduces duplication without changing observable behavior.__tests__/services/common-api.postNoBody.test.ts (1)
32-42: Error expectation matches composed HTTP error string (oncehandleApiErroris adjusted)With
handleApiErrorrejecting with a composed string ("HTTP 400 x: err"), this test’srejects.toBe("HTTP 400 x: err")correctly validates the new error format and properly asserts status + statusText + server error. No further changes needed after aligning the implementation.__tests__/services/common-api.test.ts (2)
43-54: GET error test correctly asserts new HTTP-aware error formatThis test now expects
"HTTP 400 Bad: err", which matches the composed error string fromhandleApiError(status + statusText + server-provided error). The use ofexpect.objectContaining({ headers: {}, signal: undefined })is compatible withexecuteApiRequestalways passing asignalfield.
96-108: POST error test expectations are aligned with centralized error handlingThe POST error path now checks for
"HTTP 400 B: err", which is consistent with the new standardized error message format. As long ashandleApiErrorrejects with the composed string (per earlier suggestion), this test will correctly capture both HTTP metadata and the backend error.
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/layout/sidebar/WebSidebarHeader.tsx (1)
4-5: SVG logo swap looks good; consider exposing toggle state for a11yThe move to
/6529.svgand the import/formatting tweaks look fine and keep behavior unchanged. Since this button is a sidebar toggle driven bycollapsed, you might consider also reflecting its state to assistive tech, e.g.:- <button + <button type="button" onClick={onToggle} onMouseDown={(event) => event.preventDefault()} className={`desktop-hover:hover:tw-border-iron-600 ...`} - aria-label="Toggle right sidebar"> + aria-label="Toggle right sidebar" + aria-pressed={collapsed}>This is optional but would make the control clearer to screen-reader users.
Also applies to: 17-25, 38-42
components/layout/SmallScreenLayout.tsx (2)
16-38: Header ref wiring is sound; cleanup effect can be simplifiedThe header registration via
headerWrapperReflooks good and correctly updates bothregisterRefandsetHeaderRefon mount/unmount (React calls the callback withnullon unmount). Given that, the explicit cleanup effect:useEffect(() => { return () => { registerRef("header", null); setHeaderRef(null); }; }, [registerRef, setHeaderRef]);is redundant — the callback ref already sets both to
nullwhen the node is detached. You can drop this effect to reduce duplication and dependency noise, e.g.:- useEffect(() => { - return () => { - registerRef("header", null); - setHeaderRef(null); - }; - }, [registerRef, setHeaderRef]);This is purely a cleanup/clarity win; behavior is fine as-is.
40-47: Menu/open state and scroll behavior: optional scroll‑locking improvementThe
activeTab-driven scroll reset and the overflow toggle on the outer container look reasonable:
activeTabchange →scrollTop = 0viarequestAnimationFrame.- Overflow is
"hidden"only whenactiveTab === "feed".Given that the sidebar acts as a mobile offcanvas controlled by
isMenuOpen, you might want to also lock background scrolling while the menu is open. For example:- <div - ref={containerRef} - className={`tw-bg-black ${ - activeTab === "feed" ? "tw-overflow-hidden" : "tw-overflow-auto" - }`}> + <div + ref={containerRef} + className={`tw-bg-black ${ + isMenuOpen || activeTab === "feed" + ? "tw-overflow-hidden" + : "tw-overflow-auto" + }`}>This would prevent the underlying content from scrolling when the sidebar menu is open, which typically feels better on small screens.
Also applies to: 59-63, 71-79, 82-85
components/layout/SmallScreenHeader.tsx (1)
4-7: Header props and SVG swap look good; consider richer ARIA on menu buttonThe new
isMenuOpenprop and its use inaria-labelis a nice accessibility improvement, and the switch to/6529.svgfor the logo is fine.To make the menu state even clearer to assistive technologies, you could also expose it via
aria-expanded(and optionallyaria-controlspointing at the sidebar/offcanvas element):- <button - onClick={onMenuToggle} - className="tw-flex tw-items-center tw-justify-center tw-rounded-lg tw-h-10 tw-w-10 tw-border-0 tw-text-iron-300 ..." - aria-label={isMenuOpen ? "Close menu" : "Open menu"}> + <button + onClick={onMenuToggle} + className="tw-flex tw-items-center tw-justify-center tw-rounded-lg tw-h-10 tw-w-10 tw-border-0 tw-text-iron-300 ..." + aria-label={isMenuOpen ? "Close menu" : "Open menu"} + aria-expanded={isMenuOpen} + // aria-controls="mobile-sidebar" // if you give the sidebar an id + >Not required, but a small win for screen-reader users.
Also applies to: 8-11, 13-16, 26-27, 34-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
__tests__/services/common-api.postNoBody.test.ts(1 hunks)__tests__/services/common-api.test.ts(5 hunks)components/layout/SmallScreenHeader.tsx(3 hunks)components/layout/SmallScreenLayout.tsx(2 hunks)components/layout/WebLayout.tsx(1 hunks)components/layout/sidebar/WebSidebarHeader.tsx(3 hunks)lib/fetch/ssrFetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/fetch/ssrFetch.ts
🧰 Additional context used
🧬 Code graph analysis (3)
__tests__/services/common-api.test.ts (2)
services/api/common-api.ts (2)
commonApiFetch(87-112)commonApiPost(233-252)__mocks__/@/services/api/common-api.ts (2)
commonApiFetch(11-11)commonApiPost(13-13)
__tests__/services/common-api.postNoBody.test.ts (1)
services/api/common-api.ts (1)
commonApiPostWithoutBodyAndResponse(254-268)
components/layout/SmallScreenLayout.tsx (2)
components/layout/SmallScreenHeader.tsx (1)
SmallScreenHeader(13-44)constants/sidebar.ts (1)
SIDEBAR_WIDTHS(1-5)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
__tests__/services/common-api.postNoBody.test.ts (1)
35-41: LGTM! Test correctly aligned with new error handling.The test updates properly reflect the enhanced error formatting that now includes HTTP status and statusText. The mock response includes all necessary fields (
status,statusText,error), and the expected error message follows the standardized "HTTP {status} {statusText}: {error}" format.__tests__/services/common-api.test.ts (2)
48-55: LGTM! Error test correctly updated for new format.The test now properly validates that
commonApiFetchthrows formatted errors including HTTP status and statusText. The mock response includes all necessary fields, and the expected error message follows the "HTTP {status} {statusText}: {error}" pattern.
103-110: LGTM! Error test consistently updated.The
commonApiPosterror test follows the same pattern ascommonApiFetch, correctly expecting the formatted error message "HTTP {status} {statusText}: {error}". The mock response is complete and the test properly validates the new error handling behavior.components/layout/WebLayout.tsx (1)
71-75: Web layout composition underSidebarProviderlooks correctRendering
<WebLayoutContent>directly inside<SidebarProvider>withisSmallandchildrenkeeps the sidebar context correctly wired and simplifies the layout tree. No issues from a state or SSR/CSR standpoint given this is a"use client"component.
|
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores
UI
Tests