Conversation
kibertoad
requested review from
a team,
CarlosGamero and
dariacm
as code owners
WalkthroughBroadened the generic constraint for ResponseSchemasByStatusCode across route definition types and builder functions in packages/app/api-contracts/src/apiContracts.ts to also accept full Record<HttpStatusCode, z.Schema>, in addition to Partial and undefined. No runtime logic changed; only type signatures were updated. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/app/api-contracts/src/apiContracts.ts (1)
31-34: Optional: align CommonRouteDefinition’s default for consistencyWhile not changed in this PR, consider defaulting here to
Partial | undefinedtoo, so all exported types share the same relaxed default.- ResponseSchemasByStatusCode extends - | Partial<Record<HttpStatusCode, z.Schema>> - | undefined = undefined, + ResponseSchemasByStatusCode extends + | Partial<Record<HttpStatusCode, z.Schema>> + | undefined = Partial<Record<HttpStatusCode, z.Schema>> | undefined,Optional follow-up: you could also drop
| undefinedfrom the generic and rely on the?onresponseSchemasByStatusCodeto represent absence, reducing double-undefined unions.
📜 Review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/api-contracts/src/apiContracts.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/api-contracts/src/apiContracts.ts (1)
packages/app/api-contracts/src/HttpStatusCodes.ts (1)
HttpStatusCode(1-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (24.x)
- GitHub Check: build (20.x)
- GitHub Check: build (22.x)
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | ||
| | Record<HttpStatusCode, z.Schema> | ||
| | undefined, | ||
| > = CommonRouteDefinition< |
There was a problem hiding this comment.
Default now requires a full mapping; defeats “relax default” goal
Setting the default to Record<HttpStatusCode, z.Schema> | undefined narrows usage when generics are omitted—partials won’t type-check. Default to Partial<...> | undefined; Record<...> remains assignable to Partial<...> anyway.
- ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
- | Record<HttpStatusCode, z.Schema>
- | undefined,
+ ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
+ Partial<Record<HttpStatusCode, z.Schema>> | undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| | Record<HttpStatusCode, z.Schema> | |
| | undefined, | |
| > = CommonRouteDefinition< | |
| // … other generic parameters … | |
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| Partial<Record<HttpStatusCode, z.Schema>> | undefined, | |
| ) = CommonRouteDefinition< | |
| // … remaining parameters … |
🤖 Prompt for AI Agents
packages/app/api-contracts/src/apiContracts.ts around lines 68 to 71: the
generic default for ResponseSchemasByStatusCode was changed to
Record<HttpStatusCode, z.Schema> | undefined which forces a full mapping when
generics are omitted; revert the default to Partial<Record<HttpStatusCode,
z.Schema>> | undefined so callers can omit keys (a full Record still satisfies
Partial), i.e. update the generic default back to the Partial form.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | ||
| | Record<HttpStatusCode, z.Schema> | ||
| | undefined, | ||
| > = CommonRouteDefinition< |
There was a problem hiding this comment.
Same issue in GetRouteDefinition default
Defaulting to Record | undefined rejects partial maps by default. Use Partial | undefined.
- ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
- | Record<HttpStatusCode, z.Schema>
- | undefined,
+ ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
+ Partial<Record<HttpStatusCode, z.Schema>> | undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| | Record<HttpStatusCode, z.Schema> | |
| | undefined, | |
| > = CommonRouteDefinition< | |
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| Partial<Record<HttpStatusCode, z.Schema>> | undefined, |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/apiContracts.ts around lines 91 to 94, the
generic default for ResponseSchemasByStatusCode uses Record<HttpStatusCode,
z.Schema> | undefined which disallows partial maps; change the default to
Partial<Record<HttpStatusCode, z.Schema>> | undefined so callers can omit or
provide partial status-code maps, and apply the same change to the
GetRouteDefinition default mentioned in the comment.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | ||
| | Record<HttpStatusCode, z.Schema> | ||
| | undefined, | ||
| > = CommonRouteDefinition< |
There was a problem hiding this comment.
Same issue in DeleteRouteDefinition default
Align default with the relaxed intent so partials work out of the box.
- ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
- | Record<HttpStatusCode, z.Schema>
- | undefined,
+ ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
+ Partial<Record<HttpStatusCode, z.Schema>> | undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| | Record<HttpStatusCode, z.Schema> | |
| | undefined, | |
| > = CommonRouteDefinition< | |
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| Partial<Record<HttpStatusCode, z.Schema>> | undefined, |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/apiContracts.ts around lines 113 to 116, the
generic default for DeleteRouteDefinition uses a strict Record<HttpStatusCode,
z.Schema> | undefined which prevents partial response schema maps; change the
default generic to Partial<Record<HttpStatusCode, z.Schema>> | undefined so
partial response schema objects are accepted by default and align with the
relaxed intent.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | ||
| | Record<HttpStatusCode, z.Schema> | ||
| | undefined, | ||
| >( |
There was a problem hiding this comment.
Builder: buildPayloadRoute default also too strict
Mirror the fix here so callers can pass partial maps without specifying generics.
- ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
- | Record<HttpStatusCode, z.Schema>
- | undefined,
+ ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
+ Partial<Record<HttpStatusCode, z.Schema>> | undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| | Record<HttpStatusCode, z.Schema> | |
| | undefined, | |
| >( | |
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| Partial<Record<HttpStatusCode, z.Schema>> | undefined, |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/apiContracts.ts around lines 136 to 139, the
generic default for ResponseSchemasByStatusCode is too strict (requires a full
Record<HttpStatusCode, z.Schema>), preventing callers from passing partial maps
without specifying generics; change the default type to
Partial<Record<HttpStatusCode, z.Schema>> | undefined (i.e., use Partial instead
of Record) so callers can provide partial status-code-to-schema maps implicitly,
and ensure any related usages or overloads accept the updated union type.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | ||
| | Record<HttpStatusCode, z.Schema> | ||
| | undefined, | ||
| >( |
There was a problem hiding this comment.
Builder: buildGetRoute default too strict
Default should be Partial | undefined for the same reason.
- ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
- | Record<HttpStatusCode, z.Schema>
- | undefined,
+ ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
+ Partial<Record<HttpStatusCode, z.Schema>> | undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| | Record<HttpStatusCode, z.Schema> | |
| | undefined, | |
| >( | |
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| Partial<Record<HttpStatusCode, z.Schema>> | undefined, | |
| >( |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/apiContracts.ts around lines 186 to 189, the
generic default for ResponseSchemasByStatusCode in buildGetRoute is too strict
(currently uses Record<HttpStatusCode, z.Schema> | undefined); change the
default to Partial<Record<HttpStatusCode, z.Schema>> | undefined so callers can
omit some status codes; update the generic parameter default accordingly and
ensure any related type usages still accept the looser Partial | undefined form.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | ||
| | Record<HttpStatusCode, z.Schema> | ||
| | undefined, | ||
| >( |
There was a problem hiding this comment.
Builder: buildDeleteRoute default too strict
Apply the same correction here.
- ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
- | Record<HttpStatusCode, z.Schema>
- | undefined,
+ ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined =
+ Partial<Record<HttpStatusCode, z.Schema>> | undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| | Record<HttpStatusCode, z.Schema> | |
| | undefined, | |
| >( | |
| ResponseSchemasByStatusCode extends Partial<Record<HttpStatusCode, z.Schema>> | undefined = | |
| Partial<Record<HttpStatusCode, z.Schema>> | undefined, | |
| >( |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/apiContracts.ts around lines 236 to 239, the
generic parameter ResponseSchemasByStatusCode currently constrains to
Partial<Record<HttpStatusCode, z.Schema>> but its default is
Record<HttpStatusCode, z.Schema> | undefined which is too strict; change the
default to match the constraint (Partial<Record<HttpStatusCode, z.Schema>> |
undefined) so callers won’t be forced to supply every status code.
2 participants
Changes
Only absence of response map was accepted by default, which was too strict
Checklist
major,minor,patchorskip-releaseSummary by CodeRabbit