Conversation
kibertoad
requested review from
a team,
CarlosGamero and
dariacm
as code owners
| Params extends z.ZodTypeAny = z.ZodTypeAny, | ||
| Query extends z.ZodTypeAny = z.ZodTypeAny, | ||
| RequestHeaders extends z.ZodTypeAny = z.ZodTypeAny, | ||
| Params extends z.ZodTypeAny | undefined = undefined, |
There was a problem hiding this comment.
this was inconsistent with simple REST contracts for no good reason, which made using them harder
|
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:
📝 WalkthroughWalkthroughThe PR introduces optional schema parameters across SSE and Dual-mode contract definitions, allowing Params, Query, and RequestHeaders to default to Sequence DiagramsequenceDiagram
participant Client as Test Client
participant Helper as MockttpHelper/<br/>MswHelper
participant Contract as SSE Contract<br/>Definition
participant Server as Mock Server
participant Stream as Text/Event<br/>Stream Response
Client->>Helper: mockSseResponse(contract, {<br/>pathParams?, events })
Helper->>Contract: Extract requestPathParamsSchema<br/>and HTTP method
Helper->>Helper: Resolve path using pathParams<br/>or pathResolver()
Helper->>Server: Register handler for<br/>resolved path & method
Client->>Server: Send HTTP request<br/>(GET/POST/PATCH/PUT)
Server->>Helper: Invoke registered handler
Helper->>Helper: formatSseResponse(events)<br/>converts to text/event-stream format
Helper->>Stream: Return response with<br/>content-type: text/event-stream
Stream->>Client: Stream SSE events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/api-contracts/src/sse/sseContractBuilders.types.spec.ts (1)
137-147:⚠️ Potential issue | 🟡 MinorFix lint error: unused variable
contract.The CI pipeline reports an unused variable lint error at line 138. The variable appears to be used in the type assertions below, but the linter is flagging it. This could be because
expectTypeOfonly uses the variable at the type level, not at runtime.Consider using the
_prefix convention or restructuring to satisfy the linter:🔧 Proposed fix
it('omitted schemas accept undefined', () => { - const contract = buildSseContract({ + const _contract = buildSseContract({ method: 'get' as const, pathResolver: () => '/api/stream', serverSentEventSchemas: { message: z.object({ text: z.string() }) }, }) - expectTypeOf<undefined>().toMatchTypeOf<typeof contract.requestPathParamsSchema>() - expectTypeOf<undefined>().toMatchTypeOf<typeof contract.requestQuerySchema>() - expectTypeOf<undefined>().toMatchTypeOf<typeof contract.requestHeaderSchema>() + expectTypeOf<undefined>().toMatchTypeOf<typeof _contract.requestPathParamsSchema>() + expectTypeOf<undefined>().toMatchTypeOf<typeof _contract.requestQuerySchema>() + expectTypeOf<undefined>().toMatchTypeOf<typeof _contract.requestHeaderSchema>() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/api-contracts/src/sse/sseContractBuilders.types.spec.ts` around lines 137 - 147, The variable contract created by buildSseContract is only used in type-level assertions so the linter flags it as unused; rename it to a deliberately-unused-prefixed identifier (e.g. _contract) or otherwise reference it in a no-op runtime expression so the linter is satisfied—update the declaration of contract in the it block (where buildSseContract is called) accordingly and keep the existing expectTypeOf checks that reference that identifier (e.g. _contract) so buildSseContract, contract/_contract, and expectTypeOf usages remain aligned.
🧹 Nitpick comments (4)
packages/app/universal-testing-utils/README.md (1)
336-443: Consider showingqueryParamsin the new SSE docs too.
packages/app/universal-testing-utils/test/testContracts.tsnow includessseGetContractWithQueryParams, but this section only demonstrates no-param and path-param calls. One shortmockSseResponse(..., { queryParams: ... })example would make that capability easier to discover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/universal-testing-utils/README.md` around lines 336 - 443, The docs for SSE mock support are missing an example demonstrating queryParams usage; add a concise example near the other mockSseResponse snippets showing mockSseResponse called with a contract that expects query params (referencing sseGetContractWithQueryParams from test/testContracts.ts) and pass a queryParams object (e.g., { page: '1' }) in the options for MockttpHelper and MswHelper calls so readers can discover query param support alongside pathParams and responseCode; keep the example minimal and consistent with the existing event array and formatSseResponse usage.packages/app/universal-testing-utils/src/MockttpHelper.ts (2)
40-42: Consider adding trailing newline for SSE spec compliance.The SSE specification recommends ending the stream with a blank line. The current implementation joins events with
\nbut the final event only has one trailing newline, not two (which would create the blank line separator).♻️ Optional fix for stricter SSE compliance
export function formatSseResponse(events: { event: string; data: unknown }[]): string { - return events.map((e) => `event: ${e.event}\ndata: ${JSON.stringify(e.data)}\n`).join('\n') + return events.map((e) => `event: ${e.event}\ndata: ${JSON.stringify(e.data)}\n\n`).join('') }This produces
event: foo\ndata: {...}\n\nper event, which matches the SSE spec where events are separated by blank lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/universal-testing-utils/src/MockttpHelper.ts` around lines 40 - 42, The formatSseResponse function currently joins events with single newlines which leaves the final event without the required blank-line separator; update formatSseResponse so each serialized event ends with an extra newline (e.g., make each event string end with "\n\n" or append a final "\n" after join) so the output yields a blank line after the last event to comply with the SSE spec.
199-201: Path resolution should match contract's pathResolver signature.Same issue as in MswHelper: when
requestPathParamsSchemais undefined, the contract'spathResolveris typed as() => string. Calling it with{} as anyworks but is semantically incorrect.♻️ Proposed fix
const path = contract.requestPathParamsSchema ? contract.pathResolver(pathParams) - : contract.pathResolver({} as any) + : (contract.pathResolver as () => string)()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/universal-testing-utils/src/MockttpHelper.ts` around lines 199 - 201, The path resolution currently calls contract.pathResolver({} as any) when contract.requestPathParamsSchema is falsy, which mismatches the resolver's () => string signature; update the logic in MockttpHelper.ts to call contract.pathResolver(pathParams) when contract.requestPathParamsSchema is present and call contract.pathResolver() with no arguments when it is absent, ensuring you reference the pathResolver and requestPathParamsSchema checks around the pathParams variable to satisfy the correct function signature.packages/app/universal-testing-utils/src/MswHelper.ts (1)
290-292: Path resolution should match contract's pathResolver signature.When
requestPathParamsSchemais undefined, the contract'spathResolveris typed as() => string(parameterless). Calling it with{} as anyworks but is semantically incorrect.♻️ Proposed fix for cleaner path resolution
const path = contract.requestPathParamsSchema ? contract.pathResolver(pathParams) - : contract.pathResolver({} as any) + : (contract.pathResolver as () => string)()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/universal-testing-utils/src/MswHelper.ts` around lines 290 - 292, The current call always invokes contract.pathResolver with an argument (using {} as any when requestPathParamsSchema is absent), which violates the resolver's typed signature; update the logic so that when contract.requestPathParamsSchema is present you call contract.pathResolver(pathParams), otherwise call contract.pathResolver() with no arguments, keeping the use of contract.requestPathParamsSchema, contract.pathResolver and pathParams to locate and fix the expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/app/api-contracts/src/sse/sseContractBuilders.types.spec.ts`:
- Around line 137-147: The variable contract created by buildSseContract is only
used in type-level assertions so the linter flags it as unused; rename it to a
deliberately-unused-prefixed identifier (e.g. _contract) or otherwise reference
it in a no-op runtime expression so the linter is satisfied—update the
declaration of contract in the it block (where buildSseContract is called)
accordingly and keep the existing expectTypeOf checks that reference that
identifier (e.g. _contract) so buildSseContract, contract/_contract, and
expectTypeOf usages remain aligned.
---
Nitpick comments:
In `@packages/app/universal-testing-utils/README.md`:
- Around line 336-443: The docs for SSE mock support are missing an example
demonstrating queryParams usage; add a concise example near the other
mockSseResponse snippets showing mockSseResponse called with a contract that
expects query params (referencing sseGetContractWithQueryParams from
test/testContracts.ts) and pass a queryParams object (e.g., { page: '1' }) in
the options for MockttpHelper and MswHelper calls so readers can discover query
param support alongside pathParams and responseCode; keep the example minimal
and consistent with the existing event array and formatSseResponse usage.
In `@packages/app/universal-testing-utils/src/MockttpHelper.ts`:
- Around line 40-42: The formatSseResponse function currently joins events with
single newlines which leaves the final event without the required blank-line
separator; update formatSseResponse so each serialized event ends with an extra
newline (e.g., make each event string end with "\n\n" or append a final "\n"
after join) so the output yields a blank line after the last event to comply
with the SSE spec.
- Around line 199-201: The path resolution currently calls
contract.pathResolver({} as any) when contract.requestPathParamsSchema is falsy,
which mismatches the resolver's () => string signature; update the logic in
MockttpHelper.ts to call contract.pathResolver(pathParams) when
contract.requestPathParamsSchema is present and call contract.pathResolver()
with no arguments when it is absent, ensuring you reference the pathResolver and
requestPathParamsSchema checks around the pathParams variable to satisfy the
correct function signature.
In `@packages/app/universal-testing-utils/src/MswHelper.ts`:
- Around line 290-292: The current call always invokes contract.pathResolver
with an argument (using {} as any when requestPathParamsSchema is absent), which
violates the resolver's typed signature; update the logic so that when
contract.requestPathParamsSchema is present you call
contract.pathResolver(pathParams), otherwise call contract.pathResolver() with
no arguments, keeping the use of contract.requestPathParamsSchema,
contract.pathResolver and pathParams to locate and fix the expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a19ddb4-ce57-40ce-8a02-f0c9239746a7
📒 Files selected for processing (14)
packages/app/api-contracts/src/contractBuilder.types.spec.tspackages/app/api-contracts/src/sse/dualModeContracts.tspackages/app/api-contracts/src/sse/sseContractBuilders.spec.tspackages/app/api-contracts/src/sse/sseContractBuilders.tspackages/app/api-contracts/src/sse/sseContractBuilders.types.spec.tspackages/app/api-contracts/src/sse/sseContracts.tspackages/app/universal-testing-utils/README.mdpackages/app/universal-testing-utils/package.jsonpackages/app/universal-testing-utils/src/MockttpHelper.spec.tspackages/app/universal-testing-utils/src/MockttpHelper.tspackages/app/universal-testing-utils/src/MswHelper.spec.tspackages/app/universal-testing-utils/src/MswHelper.tspackages/app/universal-testing-utils/src/index.tspackages/app/universal-testing-utils/test/testContracts.ts
💤 Files with no reviewable changes (1)
- packages/app/api-contracts/src/contractBuilder.types.spec.ts
leonaves
left a comment
leonaves
left a comment
There was a problem hiding this comment.
Looks good :) can definitely make use of this!
2 participants
Changes
Simplify client-side SSE testing
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.