Conversation
kibertoad
requested review from
a team,
CarlosGamero and
dariacm
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:
📝 WalkthroughWalkthroughThe changes migrate the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/app/universal-testing-utils/README.md (1)
418-422: Missing contract definition forsseContractWithQueryParamsin the example.The MSW SSE example uses
sseContractWithQueryParamsbut this contract is not defined in the preceding code block (onlysseContractandsseContractWithPathParamsare shown). Consider adding the contract definition or referencing an existing one for completeness.📝 Suggested addition after line 367
const sseContractWithQueryParams = buildSseContract({ method: 'get', requestQuerySchema: z.object({ yearFrom: z.coerce.number() }), pathResolver: () => '/events/stream', serverSentEventSchemas: { 'item.updated': z.object({ items: z.array(z.object({ id: z.string() })) }), completed: z.object({ totalCount: z.number() }), }, })🤖 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 418 - 422, The example references a missing SSE contract named sseContractWithQueryParams; add a corresponding contract definition (e.g., create sseContractWithQueryParams using buildSseContract) that includes requestQuerySchema for yearFrom, a pathResolver (e.g., '/events/stream'), and serverSentEventSchemas for the events used (e.g., 'completed' with totalCount) so the mswHelper.mockSseResponse example is self-contained; place this new const sseContractWithQueryParams near the other contracts (alongside sseContract and sseContractWithPathParams) in the README.packages/app/universal-testing-utils/src/MswHelper.ts (1)
315-324: Consider the silent fallthrough behavior when query params don't match.When the request's query params don't match the expected values, the handler returns
undefined, causing MSW to skip this handler. This is correct MSW behavior for conditional matching, but it can make debugging harder when tests fail - the request will appear unhandled without indicating that the query params didn't match.This is fine for the current implementation since it enables registering multiple handlers for the same path with different query params. Just noting that users debugging test failures may need to verify their query param values match exactly (including type coercion to strings).
For documentation clarity, consider adding a brief comment explaining the fallthrough behavior:
http[method](resolvedPath, ({ request }) => { const queryParams = params.queryParams if (queryParams) { const url = new URL(request.url) + // Return undefined to skip this handler if query params don't match + // (allows multiple handlers for same path with different query params) for (const [key, value] of Object.entries(queryParams as Record<string, unknown>)) { if (url.searchParams.get(key) !== String(value)) { return } } }🤖 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 315 - 324, The handler registered via http[method](resolvedPath, ({ request }) => { ... }) silently returns undefined when params.queryParams don't match, causing MSW to skip this handler and potentially make unhandled-request debugging confusing; add a brief inline comment near the params.queryParams check (referencing params.queryParams, new URL(request.url), and url.searchParams.get(key)) explaining that returning undefined is intentional to allow multiple handlers for the same path with different query params and that this fallthrough will make unmatched requests appear unhandled so callers should verify query param values (including string coercion).
🤖 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/opentelemetry-fastify-bootstrap/package.json`:
- Line 44: The peerDependency for "@fastify/otel" is pinned to "0.17.1" causing
inconsistency; update the peerDependencies entry for "@fastify/otel" to use a
caret range "^0.17.1" (matching the devDependencies entry) so patch updates are
allowed and aligns with other caret-ranged peerDependencies.
---
Nitpick comments:
In `@packages/app/universal-testing-utils/README.md`:
- Around line 418-422: The example references a missing SSE contract named
sseContractWithQueryParams; add a corresponding contract definition (e.g.,
create sseContractWithQueryParams using buildSseContract) that includes
requestQuerySchema for yearFrom, a pathResolver (e.g., '/events/stream'), and
serverSentEventSchemas for the events used (e.g., 'completed' with totalCount)
so the mswHelper.mockSseResponse example is self-contained; place this new const
sseContractWithQueryParams near the other contracts (alongside sseContract and
sseContractWithPathParams) in the README.
In `@packages/app/universal-testing-utils/src/MswHelper.ts`:
- Around line 315-324: The handler registered via http[method](resolvedPath, ({
request }) => { ... }) silently returns undefined when params.queryParams don't
match, causing MSW to skip this handler and potentially make unhandled-request
debugging confusing; add a brief inline comment near the params.queryParams
check (referencing params.queryParams, new URL(request.url), and
url.searchParams.get(key)) explaining that returning undefined is intentional to
allow multiple handlers for the same path with different query params and that
this fallthrough will make unmatched requests appear unhandled so callers should
verify query param values (including string coercion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 736bfb60-34ad-48b5-9d10-c752ca1ce65a
📒 Files selected for processing (5)
packages/app/opentelemetry-fastify-bootstrap/package.jsonpackages/app/universal-testing-utils/README.mdpackages/app/universal-testing-utils/src/MockttpHelper.spec.tspackages/app/universal-testing-utils/src/MswHelper.spec.tspackages/app/universal-testing-utils/src/MswHelper.ts
| "dependencies": {}, | ||
| "peerDependencies": { | ||
| "@fastify/otel": "^0.17.1", | ||
| "@fastify/otel": "0.17.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for available versions and security advisories for `@fastify/otel`
# Check available versions on npm
echo "=== Available versions of `@fastify/otel` ==="
npm view `@fastify/otel` versions --json | jq -r '.[]' | grep '^0\.17\.'
# Check latest version
echo -e "\n=== Latest version ==="
npm view `@fastify/otel` version
# Check for security advisories
echo -e "\n=== Security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@fastify/otel") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: lokalise/shared-ts-libs
Length of output: 345
🏁 Script executed:
cat -n packages/app/opentelemetry-fastify-bootstrap/package.json | sed -n '40,65p'Repository: lokalise/shared-ts-libs
Length of output: 1116
Revert @fastify/otel peerDependency to use caret range.
Line 44 pins the peer dependency to exactly 0.17.1 instead of allowing patch updates via ^0.17.1. This creates an inconsistency and deviates from best practice:
- Inconsistency: Line 59 (devDependencies) uses
^0.17.1while peerDependencies uses the exact version. - Outlier: All other peerDependencies use caret ranges (e.g.,
^0.68.0,^5.0.0). - Best practice violation: Exact version pinning in peer dependencies unnecessarily restricts consumer flexibility.
Revert to ^0.17.1 to align with the devDependency version and standard peer dependency practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app/opentelemetry-fastify-bootstrap/package.json` at line 44, The
peerDependency for "@fastify/otel" is pinned to "0.17.1" causing inconsistency;
update the peerDependencies entry for "@fastify/otel" to use a caret range
"^0.17.1" (matching the devDependencies entry) so patch updates are allowed and
aligns with other caret-ranged peerDependencies.
2 participants
Changes
convenient SSE testing with msw
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.