Open
Conversation
CatchMe2
requested review from
a team,
CarlosGamero,
dariacm and
kibertoad
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lokalise/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces Sequence Diagram(s)sequenceDiagram
participant Client as Frontend Client
participant WretchLib as Wretch Instance
participant Server as HTTP Server
participant ContractResolver as Contract Resolver
participant BodyParser as Body Parser
Client->>ContractResolver: resolveResponseEntry(status, content-type)
ContractResolver-->>Client: ResponseKind (json/text/blob/sse/noContent)
Client->>WretchLib: send request (with pathParams, body, headers)
WretchLib->>Server: HTTP request
Server-->>WretchLib: HTTP response (status, headers, body stream)
alt SSE Response
Client->>BodyParser: parse SSE stream event-by-event
BodyParser-->>Client: AsyncIterable<typed events>
else JSON Response
Client->>BodyParser: validate against Zod schema
BodyParser-->>Client: typed object
else Text/Blob Response
Client->>BodyParser: read as string/Blob
BodyParser-->>Client: string or Blob
else No-Body Response
Client-->>Client: null
end
Client->>Client: normalize response headers
Client->>Client: wrap in Either{result, error} per captureAsError
Client-->>Client: Promise<Either>
sequenceDiagram
participant Client as Backend Client
participant UndiciClient as Undici Client
participant Server as HTTP Server
participant RetryInterceptor as Retry Interceptor
participant ContractResolver as Contract Resolver
participant BodyParser as Body Parser
Client->>Client: build request path from pathParams
Client->>RetryInterceptor: apply retry config via interceptor
RetryInterceptor->>UndiciClient: send HTTP request
UndiciClient->>Server: HTTP request
Server-->>UndiciClient: HTTP response (status, headers, body)
UndiciClient->>RetryInterceptor: check status against retry rules
alt Retry Condition Met
RetryInterceptor->>UndiciClient: retry request
else Request Succeeds or Max Retries Exceeded
RetryInterceptor-->>Client: response
end
Client->>ContractResolver: resolveResponseEntry(status, content-type, strictContentType)
ContractResolver-->>Client: ResponseKind or null
Client->>BodyParser: parse body per ResponseKind
alt SSE Streaming
BodyParser-->>Client: AsyncIterable<typed events>
else JSON/Text/Blob
BodyParser-->>Client: parsed body
end
Client->>Client: validate response headers if enabled
Client->>Client: construct Either{result, error} per captureAsError
Client-->>Client: Promise<Either>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
added 19 commits
April 3, 2026 15:01
CatchMe2
force-pushed
the
be-http-client-send-by-route-contract
branch
from
3da424f to
e02048e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
packages/app/frontend-http-client/src/sendByApiContract.ts (2)
106-119: Consider adding exhaustive check for future-proofing.The switch lacks a
defaultcase. While TypeScript enforces exhaustiveness at compile time, a runtime assertion would catch potential mismatches ifResponseKindis extended.♻️ Proposed defensive exhaustive check
case 'sse': return parseSseStreamWithSchema(response, resolvedEntry.schemaByEventName, signal) + default: { + const _exhaustiveCheck: never = resolvedEntry + throw new Error(`Unknown response kind: ${(_exhaustiveCheck as ResponseKind).kind}`) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/frontend-http-client/src/sendByApiContract.ts` around lines 106 - 119, The switch on resolvedEntry.kind in sendByApiContract.ts is missing a runtime fallback for new ResponseKind values; add a default branch after existing cases that throws an explicit Error (or calls a helper like assertUnreachable) including the unexpected resolvedEntry.kind value and contextual info (e.g., the endpoint or resolvedEntry) so mis-matches are detected at runtime; reference the switch over resolvedEntry.kind and functions/values like parseSseStreamWithSchema, validateResponse, and resolvedEntry.schema when locating where to add this defensive check.
135-135: The createdAbortControlleris immediately discarded.When no signal is provided, a new
AbortControlleris created but its reference is lost, meaning the signal can never actually abort. This is functionally correct (provides a non-null signal to fetch and SSE parsing), but consider adding a brief comment explaining the intent—or allowingundefinedsignal where supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/frontend-http-client/src/sendByApiContract.ts` at line 135, The current line in sendByApiContract.ts creates an AbortController but discards it (const signal = options.signal ?? new AbortController().signal), so add a short comment explaining the intent to create a throwaway signal when none is provided (or alternatively change the logic to preserve the controller if you need to call abort later); reference options.signal and the temporary AbortController in your comment or, if you prefer functional clarity, store the controller in a local variable (e.g., let fallbackController = options.signal ? undefined : new AbortController()) and use fallbackController.signal so the aborter is not implicitly lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/api-contracts/src/new/clientTypes.ts`:
- Around line 43-46: The type InferClientResponseHeaders currently intersects
InferSchemaOutput<TApiContract['responseHeaderSchema']> with Record<string,
string | undefined>, which forces parsed header types to also satisfy string and
yields never for transformed types; change the pattern to let parsed keys
override the raw map by building a base Record<string, string | undefined> then
using Omit<base, keyof Parsed> & Parsed where Parsed =
InferSchemaOutput<TApiContract['responseHeaderSchema']>, i.e., replace the
intersection with Omit<Record<string, string | undefined>, keyof Parsed> &
Parsed (referencing InferClientResponseHeaders, ApiContract,
responseHeaderSchema, and InferSchemaOutput), and update the clientTypes.spec.ts
expectations for headers that use non-string transforms.
In `@packages/app/api-contracts/src/new/defineApiContract.ts`:
- Around line 111-130: Change hasAnySuccessSseResponse to only enable streaming
when all present successful responses are unambiguously SSE: iterate
SUCCESSFUL_HTTP_STATUS_CODES and collect only the status codes that exist in
apiContract.responsesByStatusCode; for each existing response (referencing
hasAnySuccessSseResponse, isSseResponse, isAnyOfResponses and the
ApiContract.responsesByStatusCode shape) require that either
isSseResponse(value) is true or, if isAnyOfResponses(value), every response in
value.responses isSseResponse; return true only if at least one success status
is present and every present success response satisfies those SSE-only
conditions, otherwise return false.
In `@packages/app/api-contracts/src/new/inferTypes.ts`:
- Around line 87-108: ContractResponseMode currently treats any mix of SSE and
non-SSE across different status codes as 'dual', which forces streaming and
breaks InferSseClientResponse/InferNonSseClientResponse; change the
classification so 'dual' is returned only when a single success status code
contains both SSE and non‑SSE variants (i.e., an anyOf for the same status
code), otherwise use 'sse' or 'non-sse' based on whether all success status
codes are SSE or non‑SSE. Update the logic that computes ContractResponseMode
(and related helpers HasAnySseSuccessResponse / HasAnyNonSseSuccessResponse) to
inspect per-status-code unions (detect presence of both { _tag: "SseResponse" }
and non-SSE entries in the same status bucket) and adjust InferSseClientResponse
/ InferNonSseClientResponse to stop requiring streaming when the response is
discriminated by statusCode.
In `@packages/app/backend-http-client/docs/sendByApiContract.md`:
- Around line 44-45: The docs incorrectly state sendByApiContract always returns
an Either; update the documentation around sendByApiContract (and the same
wording at lines 199-203) to explicitly separate failures that remain inside the
Either envelope from failures that throw: describe that header/body validation
errors and other synchronous validation paths may throw and therefore require
callers to use try/catch, while content-type mismatches under the current strict
mode are surfaced as Either.error (and thus are handled via the Either result).
Reference the symbols sendByApiContract, Either.error, "header/body validation",
and "strict content-type mismatch" so readers know which code paths need
try/catch versus handling the Either.
In `@packages/app/backend-http-client/src/client/sendByApiContract.spec.ts`:
- Around line 564-567: The test expectation in sendByApiContract.spec.ts asserts
the old error text "Could not map response"; update the assertion to match the
current unmapped-response message emitted by sendByApiContract (e.g., assert
result.error.message contains the prefix "Failed to process API response" or
another stable substring) so the retry spec aligns with runtime; locate the
assertion near the expect(result.error).toMatchObject call and replace the
stringContaining check to match the new error prefix.
In `@packages/app/backend-http-client/src/client/sendByApiContract.ts`:
- Around line 103-109: The SSE parser in sendByApiContract.ts currently
overwrites `data` for each `data:` line and only splits blocks by '\n\n', so it
fails on CRLF framing and multi-line `data:` events; update the parsing loop to
first normalize CRLF to '\n' on the raw chunk, split blocks by '\n\n' (after
normalization), and for each block accumulate all `data:` lines into a single
string (joining multiple `data:` lines with '\n') rather than replacing `data`
each time; ensure trimming is applied to the final accumulated `data` and that
the same fix is applied to the identical logic around the 127-137 area (same
function/loop handling `event`/`data`).
- Around line 224-237: The code in sendByApiContract uses truthiness checks for
params.body which drops valid falsy JSON bodies (0, false, '', null) and omits
the content-type header; change the checks that currently use if (params.body)
and params.body ? JSON.stringify(params.body) : undefined to explicit undefined
checks (e.g., params.body !== undefined) so requestHeaders['content-type'] is
set and the request body is JSON.stringify(params.body) whenever params.body is
not undefined; update the request construction in the dispatcher.request call
and the header assignment to use this explicit check.
- Around line 79-93: The retry handler uses nullish coalescing and a falsy check
that accidentally treats valid falsy values (like 0) as missing; change the stub
construction and delay check in the retry callback to use explicit presence
checks: for statusCode and headers use conditional presence checks (e.g.,
"'statusCode' in err ? err.statusCode : 500" and similar for headers) instead of
("statusCode' in err && err.statusCode) ?? 500, and change the delay guard from
"if (!delay || delay === -1)" to an explicit undefined/null check (e.g., "if
(delay === undefined || delay === -1)" or "if (delay == null || delay === -1)")
so a zero delay returned by config.delayResolver is honored; update the retry
callback accordingly (references: config.delayResolver, the retry: handler,
stub, delay variable).
In `@packages/app/frontend-http-client/docs/send-by-api-contract.md`:
- Around line 63-84: The docs incorrectly state the default for the `signal`
option and the type of response headers in the `ResponseObject` return shape:
update the `sendByApiContract` docs so the `signal` default reflects "not
provided" (no AbortController created/used) instead of `new
AbortController().signal`, and change the `headers` type in the `result` shape
to allow undefined values (use `Record<string, string | undefined>`) to match
the exported response typing; update only the documentation text and the
returned `result` shape example that mentions `statusCode`, `headers`, and
`body` so it matches the actual behavior and types used by the
`sendByApiContract` implementation.
In `@packages/app/frontend-http-client/src/sendByApiContract.spec.ts`:
- Around line 144-146: The test's assertion expecting "Could not map response"
is stale; update the expectation in sendByApiContract.spec.ts (the assertion
around result.error) to match the new runtime message prefix "Failed to process
API response" instead of the removed wording, i.e. change the
expect.stringContaining argument used on result.error.message to assert the new
prefix so the test reflects the current error text returned by the client.
In `@packages/app/frontend-http-client/src/sendByApiContract.ts`:
- Around line 93-97: The JSON.parse call inside the async generator can throw on
malformed SSE data and kill the stream; wrap the parse + schema.parse logic in a
try/catch inside the generator (the code around parsed =
JSON.parse(sseEvent.data) and yield { event: sseEvent.event, data:
schema.parse(parsed) }) and on error log or emit a contextual error (include
sseEvent.event and the raw sseEvent.data) and continue the loop so subsequent
events are not lost.
---
Nitpick comments:
In `@packages/app/frontend-http-client/src/sendByApiContract.ts`:
- Around line 106-119: The switch on resolvedEntry.kind in sendByApiContract.ts
is missing a runtime fallback for new ResponseKind values; add a default branch
after existing cases that throws an explicit Error (or calls a helper like
assertUnreachable) including the unexpected resolvedEntry.kind value and
contextual info (e.g., the endpoint or resolvedEntry) so mis-matches are
detected at runtime; reference the switch over resolvedEntry.kind and
functions/values like parseSseStreamWithSchema, validateResponse, and
resolvedEntry.schema when locating where to add this defensive check.
- Line 135: The current line in sendByApiContract.ts creates an AbortController
but discards it (const signal = options.signal ?? new AbortController().signal),
so add a short comment explaining the intent to create a throwaway signal when
none is provided (or alternatively change the logic to preserve the controller
if you need to call abort later); reference options.signal and the temporary
AbortController in your comment or, if you prefer functional clarity, store the
controller in a local variable (e.g., let fallbackController = options.signal ?
undefined : new AbortController()) and use fallbackController.signal so the
aborter is not implicitly lost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79aae809-eae5-47b4-b1eb-e2e82add7f5c
📒 Files selected for processing (17)
packages/app/api-contracts/src/index.tspackages/app/api-contracts/src/new/clientTypes.spec.tspackages/app/api-contracts/src/new/clientTypes.tspackages/app/api-contracts/src/new/contractResponse.spec.tspackages/app/api-contracts/src/new/contractResponse.tspackages/app/api-contracts/src/new/defineApiContract.tspackages/app/api-contracts/src/new/inferTypes.tspackages/app/api-contracts/src/typeUtils.tspackages/app/backend-http-client/docs/sendByApiContract.mdpackages/app/backend-http-client/src/client/sendByApiContract.spec.tspackages/app/backend-http-client/src/client/sendByApiContract.tspackages/app/backend-http-client/src/index.tspackages/app/frontend-http-client/docs/send-by-api-contract.mdpackages/app/frontend-http-client/src/index.tspackages/app/frontend-http-client/src/sendByApiContract.spec.tspackages/app/frontend-http-client/src/sendByApiContract.tspackages/app/frontend-http-client/tsconfig.json
packages/app/backend-http-client/src/client/sendByApiContract.spec.ts
Outdated
Show resolved
Hide resolved
packages/app/backend-http-client/src/client/sendByApiContract.ts
Outdated
Show resolved
Hide resolved
added 19 commits
April 3, 2026 15:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Adds a contract-driven HTTP request function —
sendByApiContract— to bothfrontend-http-clientandbackend-http-client, alongside the supporting type infrastructure inapi-contracts.@lokalise/api-contractsclientTypes.ts(new):InferSseClientResponse<T, THeaders>andInferNonSseClientResponse<T, THeaders>— typed discriminated unions of{ statusCode, headers, body }derived from a contract'sresponsesByStatusCode.SSE bodies on success codes are
AsyncIterable; non-SSE bodies are inferred from the Zod schema / response kind.hasAnySuccessSseResponse:Utility that checks whether any 2xx entry in a contract is an SSE response (used to auto-detect streaming mode).
resolveContractResponse:Gains a
strictparameter (defaulttrue). In non-strict mode, a missing or mismatchedcontent-typefalls back to the entry's declared kind instead of returningnull.anyOfResponsesalways requires a content-type regardless ofstrict.sendByApiContract(both clients)A unified, contract-typed request function covering all HTTP methods (
GET,POST,PUT,PATCH,DELETE).Key options (all default to
true)captureAsErrortrue→ non-2xx responses go toEither.error;false→ all contract status codes go toEither.resultwith a narrowed type unionvalidateResponsestrictContentTypecontent-typedoesn't match the contract entrysignalAbortSignalfor mid-flight cancellationAdditional behavior
SSE streaming:
Contracts with
sseResponsedefault to streaming mode automatically.result.bodyis anAsyncIterableof schema-validated, typed events.Dual-mode (
anyOfResponses):Pass
streaming: true | falseexplicitly to select between SSE and JSON; the return type narrows accordingly.Apply one of following labels;
major,minor,patchorskip-releaseI've updated the documentation, or no changes were necessary
I've updated the tests, or no changes were necessary
AI Assistance Tracking
We're running a metric to understand where AI assists our engineering work. Please select exactly one of the options below:
Mark "Yes" if AI helped in any part of this work, for example: generating code, refactoring, debugging support,
explaining something, reviewing an idea, or suggesting an approach.