chore: tighten server/client types ahead of pnpm migration - #668
chore: tighten server/client types ahead of pnpm migration#668serendipty01 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
a9cd452 to
9b92b09
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR appears to address TypeScript compatibility issues introduced by dependency/type changes (notably safe-stable-stringify, helmet, ts-pattern, and test tooling), plus a small fix to avoid invalid dates in property-based tests.
Changes:
- Add local type shims/casts for
safe-stable-stringify(multiple files) andhelmet(server API). - Adjust test utilities/types (Jest callback typing, Vitest jest-dom setup, client
tsconfigtypes) and fix fast-check date generation. - Work around a
ts-patternexhaustiveness typing regression by replacing.exhaustive()with.otherwise().
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/utils/encoding.ts | Add safe-stable-stringify import/cast shim to satisfy TS typing. |
| server/test/utils.ts | Tighten jestFn typing to match Jest it(...) signature. |
| server/test/arbitraries/ContentType.ts | Ensure generated dates are always valid (noInvalidDate). |
| server/services/orgAwareSignalExecutionService/signalExecutionService.ts | Add safe-stable-stringify import/cast shim. |
| server/services/networkingService/index.ts | Force request-body bytes to an ArrayBuffer for signing. |
| server/services/manualReviewToolService/modules/JobDecisioning.ts | Replace ts-pattern .exhaustive() with .otherwise() workaround. |
| server/services/itemInvestigationService/utils.ts | Add safe-stable-stringify import/cast shim. |
| server/rule_engine/RuleEvaluator.ts | Fix Opaque<> token to a stable unique tag string. |
| server/lib/cache/utils/wrapProducer.ts | Add safe-stable-stringify import/cast shim. |
| server/lib/cache/utils/utils.ts | Replace debug import with a minimal local implementation; add safe-stable-stringify shim. |
| server/graphql/customScalars/OpaqueScalarMixin.ts | Add @ts-expect-error for a jwt expiresIn typing mismatch. |
| server/condition_evaluator/getDerivedFieldValue.ts | Add safe-stable-stringify import/cast shim. |
| server/api.ts | Add helmet import/cast shim to satisfy TS typing. |
| client/tsconfig.json | Extend global types with jest + google.maps. |
| client/src/setupTests.ts | Add Testing Library jest-dom matchers for Vitest. |
| .with(['ALREADY_LOGGED', 'SUCCESS'], () => jobAlreadySubmittedError) | ||
| // Case 4, client retrying after failed job deletion; deletion failed again. | ||
| .with(['ALREADY_LOGGED', 'FAILED'], () => decisioningFailedError) | ||
| .exhaustive(), | ||
| // ts-pattern v5.9 + TS 5.9: .exhaustive() triggers NonExhaustiveError<[any]>; | ||
| // all four cases above are covered so this branch is dead code. | ||
| .otherwise(() => undefined), |
| serialize(value) { | ||
| // @ts-expect-error -- @types/jsonwebtoken@9 brands expiresIn as StringValue from ms; plain string is valid at runtime | ||
| return jwt.sign(value as T, jwtSigningKey, { | ||
| expiresIn: jwtExpiresIn, | ||
| }); |
| // Minimal debug-compatible logger — avoids phantom dep under pnpm strict mode. | ||
| // Behaviour is identical when DEBUG is unset (the common case in tests/prod). | ||
| const debug = (namespace: string) => { | ||
| const patterns = (process.env.DEBUG ?? '').split(',').map((p) => p.trim()); | ||
| const enabled = patterns.some( | ||
| (p) => p === '*' || p === namespace || (p.endsWith('*') && namespace.startsWith(p.slice(0, -1))), | ||
| ); | ||
| return (...args: unknown[]) => { if (enabled) console.debug(` ${namespace}`, ...args); }; | ||
| }; |
| import stringify_ from 'safe-stable-stringify'; | ||
| const stringify = stringify_ as unknown as (value: unknown) => string | undefined; |
| import helmet_, { type HelmetOptions } from 'helmet'; | ||
| const helmet = helmet_ as unknown as (options?: Readonly<HelmetOptions>) => (req: unknown, res: unknown, next: (err?: unknown) => void) => void; |
| "jsx": "react-jsx", | ||
| "target": "ESNext", | ||
| "types": ["vite/client", "vite-plugin-svgr/client"], | ||
| "types": ["vite/client", "vite-plugin-svgr/client", "jest", "google.maps"], |
Summary
fc.date()inContentType.tsarbitraries to usenoInvalidDate: true, preventingRangeError: Invalid time valuein property-based testsdebugdep inserver/lib/cache/utils/utils.tswith a minimal inline logger (avoids the transitive dep that pnpm strict mode won't hoist)as unknown astype assertions forsafe-stable-stringifydefault export across 5 files — the package's own types are imprecise and this workaround is needed under pnpm's stricternodeLinkerserver/api.tsto satisfy theHelmetOptionsbranded typeRuleEvaluationContextopaque brand fromOpaque<T, T>toOpaque<T, 'RuleEvaluationContext'>@ts-expect-erroronjwt.sign()call —@types/jsonwebtoken@9brandsexpiresInasStringValue; plainstringis valid at runtimeTextEncoder.encode().buffertoas ArrayBufferinnetworkingServicejest.ProvidesCallbacktype annotation inserver/test/utils.ts.exhaustive()to.otherwise(() => undefined)inJobDecisioning.ts(ts-pattern v5.9 + current TS emitsNonExhaustiveError<[any]>on the exhaustive call)@testing-library/jest-dom/vitestimport toclient/src/setupTests.tsjestandgoogle.mapstoclient/tsconfig.jsontypesWhy now
These changes are all valid on the current npm setup and are prerequisites for the upcoming pnpm migration PR. Landing them first keeps the pnpm PR a pure tooling diff (no source-code changes), making it easier to review.
Test plan
docker compose run --rm backend npm run lintdocker compose run --rm backend npm run builddocker compose run --rm client npm run lintdocker compose run --rm client npm run builddocker compose run --rm test