Conversation
| }) | ||
|
|
||
| it('silently ignores unknown event names', async () => { | ||
| await mockServer.forGet('/events/stream').thenCallback(() => ({ |
There was a problem hiding this comment.
@leonaves would providing some test server implementation for FE testing inside opinionated-machine for cases like this be helpful? to endure mocking is fully accurate with how the real thing works
There was a problem hiding this comment.
that sounds good, maybe could we make it part of a follow up?
There was a problem hiding this comment.
I can build it already :D.
but yeah, migrating to it can be a follow-up
| /* v8 ignore stop */ | ||
| const reader = response.body.getReader() | ||
|
|
||
| for await (const { event, data } of parseSseStream(reader, abortController.signal)) { |
There was a problem hiding this comment.
do I inderstand correctly that this iteration is supposed to keep running indefinitely until the user closes the page/navigates away?
There was a problem hiding this comment.
You can also close manually with connection.close() with the returned connection object.
| if (state.currentData) { | ||
| state.currentData += '\n' | ||
| } | ||
| state.currentData += line.slice(5).trim() |
There was a problem hiding this comment.
are we sure we don't want to use classes for managing state that has logic?
| buffer += decoder.decode(value, { stream: true }) | ||
| const lines = buffer.split('\n') | ||
| // Keep the last element — it may be a partial line (split always returns at least 1 element) | ||
| /* v8 ignore start */ |
There was a problem hiding this comment.
why is the branch unreachable?
There was a problem hiding this comment.
it's just the part after ??, it's needed for typescript but split never actually returns no elements, so that's never executed, breaking coverage a bit.
|
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 adds a new SSE (Server-Sent Events) client module to the frontend HTTP client package. The implementation includes a typed SSE connection mechanism driven by contract definitions, with utilities for parsing SSE streams, parameter validation, header resolution, and event callback dispatching. A new test suite with 649 lines provides comprehensive coverage for connection handling, parameter validation, event parsing, error scenarios, and callback invocation. The module exports are re-exposed through the package's main entry point. Sequence DiagramsequenceDiagram
actor Client
participant connectSse as connectSseByContract
participant Validation as Parameter Validation
participant Wretch as HTTP Client (Wretch)
participant Stream as SSE Stream
participant Parser as parseSseStream
participant Contracts as Contract Schemas
participant Callbacks as User Callbacks
Client->>connectSse: connectSseByContract(wretch, contract, params, callbacks)
connectSse->>Validation: Validate pathParams, queryParams, body
Validation-->>connectSse: Validation result
alt Validation fails
connectSse->>Callbacks: onError(ValidationError)
else Validation succeeds
connectSse->>Wretch: GET/POST with headers, params
Wretch-->>Stream: text/event-stream response
connectSse->>Parser: parseSseStream(reader, abortSignal)
loop Until stream ends or aborts
Parser->>Stream: Read SSE data chunks
Stream-->>Parser: Raw bytes
Parser->>Parser: Decode and parse SSE format
Parser-->>connectSse: SseEvent {event, data}
connectSse->>Contracts: Validate event against schema
alt Validation succeeds
Contracts-->>connectSse: Parsed event data
connectSse->>Callbacks: onEvent[eventType](data)
else Validation fails
connectSse->>Callbacks: onError(ValidationError)
end
end
connectSse->>Callbacks: onOpen() [on connection established]
end
Client->>connectSse: close()
connectSse->>Stream: Abort stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/app/frontend-http-client/src/sse.spec.ts (2)
550-582: Header resolution test doesn't verify headers were sent.The test verifies that providing a
headersfunction doesn't break the connection, but it doesn't assert that the custom header (x-custom: test-value) was actually sent to the server. The mock callback receives the request but ignores it.Consider adding an assertion:
Proposed enhancement
- await mockServer.forGet('/events/stream').thenCallback((_req) => ({ + await mockServer.forGet('/events/stream').thenCallback((req) => { + expect(req.headers['x-custom']).toBe('test-value') + return { statusCode: 200, headers: { 'Content-Type': 'text/event-stream' }, body: sseResponse([{ event: 'done', data: JSON.stringify({ total: 0 }) }]), - })) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/frontend-http-client/src/sse.spec.ts` around lines 550 - 582, The test should assert the custom header was actually sent: modify the mockServer.forGet('/events/stream') callback to inspect the incoming request (the _req argument) and either store or directly assert that _req.headers['x-custom'] (or _req.getHeader('x-custom') depending on the mock API) equals 'test-value'; keep the rest of the test using headersFn, headerContract, connectSseByContract and onDone as-is, and perform the header assertion before closing the connection.
584-616: Same applies to async header resolution test.Similar to the sync header test, this test should verify the async-resolved header was actually included in the request.
Proposed enhancement
- await mockServer.forGet('/events/stream').thenCallback(() => ({ + await mockServer.forGet('/events/stream').thenCallback((req) => { + expect(req.headers['x-custom']).toBe('async-value') + return { statusCode: 200, headers: { 'Content-Type': 'text/event-stream' }, body: sseResponse([{ event: 'done', data: JSON.stringify({ total: 0 }) }]), - })) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/frontend-http-client/src/sse.spec.ts` around lines 584 - 616, The test currently doesn't assert that the async-resolved header is actually sent; update the mockServer callback used in mockServer.forGet('/events/stream') to capture/inspect the incoming request headers (the request param provided to thenCallback) and assert that request.headers['x-custom'] === 'async-value' (or equivalent) before returning the SSE response; keep using asyncHeadersFn, headerContract, connectSseByContract and onDone as-is but add this header assertion to verify async header resolution.
🤖 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/frontend-http-client/src/sse.spec.ts`:
- Around line 483-516: The test constructs a multi-line SSE JSON payload that,
when joined with '\n', produces invalid JSON and causes handleSseEvent to hit
callbacks.onError instead of invoking the expected handler; update the test in
sse.spec.ts so the SSE split occurs at a valid JSON token boundary (e.g., split
after the '[' or after a complete key/value) so the joined data string is valid
JSON, or alternatively change the test to send multi-line plain text (not JSON)
or add an onError mock and assert it is called; locate the construction of
multiLineData / firstHalf / secondHalf and the connectSseByContract call to
apply the fix.
In `@packages/app/frontend-http-client/src/sse.ts`:
- Around line 226-241: The parseRequestBody result is validated but its
transformed value isn't applied, so downstream code uses the original
sseParams.body; update sseParams.body with the validated result before calling
runSseConnection. Specifically, after calling parseRequestBody(...) and after
the isFailure(body) check, set sseParams.body = body.result (or the appropriate
success value from parseRequestBody) so runSseConnection(wretch, contract,
sseParams, ...) sends the validated/transformed body; keep the existing error
handling using callbacks.onError and return behavior unchanged.
---
Nitpick comments:
In `@packages/app/frontend-http-client/src/sse.spec.ts`:
- Around line 550-582: The test should assert the custom header was actually
sent: modify the mockServer.forGet('/events/stream') callback to inspect the
incoming request (the _req argument) and either store or directly assert that
_req.headers['x-custom'] (or _req.getHeader('x-custom') depending on the mock
API) equals 'test-value'; keep the rest of the test using headersFn,
headerContract, connectSseByContract and onDone as-is, and perform the header
assertion before closing the connection.
- Around line 584-616: The test currently doesn't assert that the async-resolved
header is actually sent; update the mockServer callback used in
mockServer.forGet('/events/stream') to capture/inspect the incoming request
headers (the request param provided to thenCallback) and assert that
request.headers['x-custom'] === 'async-value' (or equivalent) before returning
the SSE response; keep using asyncHeadersFn, headerContract,
connectSseByContract and onDone as-is but add this header assertion to verify
async header resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8042fc77-46e1-4264-9b8a-21f0b3bd62e1
📒 Files selected for processing (4)
packages/app/frontend-http-client/src/index.tspackages/app/frontend-http-client/src/sse.spec.tspackages/app/frontend-http-client/src/sse.tspackages/app/frontend-http-client/src/utils/sseUtils.ts
| handler?.(result.data) | ||
| } catch (err) { | ||
| /* v8 ignore start */ | ||
| const message = err instanceof Error ? err.message : String(err) |
There was a problem hiding this comment.
instanceof Error checks are unreliable in JS, this is the recommended check:
export function isError(maybeError: unknown): maybeError is Error {
return (
maybeError instanceof Error || Object.prototype.toString.call(maybeError) === '[object Error]'
)
}
|
|
||
| /* v8 ignore start */ | ||
| if (!response.body) { | ||
| throw new Error('Response body is null') |
There was a problem hiding this comment.
since we are going to immediately catch the error anyway, I wonder if we should provide more direct escape hatch for just passing this to callbacks directly and returning
There was a problem hiding this comment.
I don't really mind, seems much the same to me, this way means we call the callback the same place whether its this error or one from another function in this block, but maybe is less obvious to follow. I think it's quite a small block though. I'll change if you strongly prefer but if it's down to subjectivity I'd say might as well leave it.
There was a problem hiding this comment.
no strong opinion, good to keep as is!
2 participants
Changes
Adds
connectSseByContractto@lokalise/frontend-http-client— a new function for opening SSE connections using SSE and dual-mode contracts. As we migrate from websockets to SSE, this gives frontend consumers a typed, contract-driven way to connect to streaming endpoints using the same wretch instance and auth middleware they already use for sync requests.buildSseContract(both SSE-only and dual-mode)onEvent), plusonOpenandonErrorlifecycle hooksSseConnectionhandle withclose()to abort the streamNew exports
connectSseByContract— the main functionSseConnection,SseCallbacks,SseRouteRequestParams— supporting typesUsage example
Using
getWorkspacePortalRequestsContract(a dual-mode contract):Checklist
major,minor,patchorskip-releaseAI 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.