Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThreads configurable limits for example and security-schema combination generation through core helpers, JSON-schema flattening, documentation depiction, endpoint example registration, logical-container processing, diagnostics, and tests; adds header extraction for security configurations and documents the change in CHANGELOG v27.4.0. ChangesExample & Documentation Limits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
coverage: 100.0%. remained the same — limit-combinations-2 into master |
RobinTail
force-pushed
the
limit-combinations-2
branch
from
ea89779 to
83e0642
Compare
…to concatenations as well.
…sitive, now also with default.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two semantic mismatches between the CHANGELOG and the implementation, plus a footgun where limits.security: 0 silently drops the security field from generated operations. Public API surface is unaffected (none of the touched helpers are re-exported from src/index.ts).
Minor test gap: the top-level new Documentation({ limits: { ... } }) path has no end-to-end coverage, and Endpoint.getResponses({ maxExamples: N }) is only exercised with N = 0 (where the limit is a no-op on schemas without examples).
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Incremental review: the new commit adds maxExamples = defaultMaxCombinations default on getResponses and gates #ensureOutputExamples behind maxExamples > 0, addressing the prior "required params arg without default" nit. Nothing new to flag in this range. Prior findings on CHANGELOG wording, mergeExamples asymmetry at limit === 0, and processContainers(..., 0) producing empty security are unaddressed but still stand.
Claude Opus ｜ 𝕏
![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 (3)
express-zod-api/tests/result-helpers.spec.ts (1)
120-121: ⚡ Quick winAssert exact limited payloads, not just count
Line 120 currently verifies only the count. This can still pass if the limiter returns wrong/duplicated examples. Please assert the exact returned subset to make the limit behavior deterministic.
Suggested test tightening
- expect(pullResponseExamples(schema, 3)).toHaveLength(3); + expect(pullResponseExamples(schema, 3)).toEqual([ + { a: "one", b: 1, c: false }, + { a: "one", b: 2, c: false }, + { a: "two", b: 1, c: false }, + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@express-zod-api/tests/result-helpers.spec.ts` around lines 120 - 121, The test only checks the count from pullResponseExamples(schema, 3) which can hide duplicate/wrong items; update the assertion to verify the exact subset returned (e.g., compare to the expected first three examples) by replacing the toHaveLength check with an equality assertion (use toEqual) against the deterministic array you expect from the schema variable so the limiter behavior is strictly validated for pullResponseExamples.express-zod-api/tests/logical-container.spec.ts (1)
209-210: ⚡ Quick winStrengthen max-combination test with concrete expected output
Line 209 only checks array size. This won’t catch incorrect truncation logic (e.g., wrong branch preference). Please assert concrete combinations for the capped result.
Suggested assertion update
- ).toHaveLength(2); + ).toEqual([ + [1, 2, 5, 6], + [1, 2, 7, 8], + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@express-zod-api/tests/logical-container.spec.ts` around lines 209 - 210, The test currently only asserts ".toHaveLength(2)" which misses incorrect truncation or branch preference; replace the length-only assertion on the produced combinations with a concrete equality/assertion that lists the expected combination objects/strings (instead of just size) so the capped-result behavior is verified precisely—locate the assertion using the existing ".toHaveLength(2)" in the test and change it to assert the exact array of combinations (or use an order-insensitive matcher like toEqual with a sorted/normalized array or expect.arrayContaining) for the function under test.express-zod-api/src/json-schema-helpers.ts (1)
89-112: 💤 Low valueOptional: branches handle
limit <= 0asymmetrically.The optional branch short-circuits with
if (!(limit > 0)) return;, preserving any existingtarget.examples. The non-optional branch unconditionally callscombinations(...), which returns[]whenlimit <= 0, overwriting a previously populatedtarget.exampleswith an empty array. Consider hoisting the guard before the branch for consistent behavior.♻️ Symmetric guard
if (!entry.examples?.length) return; + if (!(limit > 0)) return; if (isOptional) { - if (!(limit > 0)) return; target.examples = R.concat(target.examples || [], entry.examples).slice( 0, limit, ); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@express-zod-api/src/json-schema-helpers.ts` around lines 89 - 112, In mergeExamples, the guard for limit > 0 is only applied in the optional branch which lets the non-optional branch overwrite target.examples with [] when limit <= 0; move the limit check to the top of mergeExamples (before branching on isOptional) and return early when limit <= 0 so both branches preserve existing target.examples; keep the rest of the logic involving target.examples, entry.examples, combinations and R.mergeDeepRight unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@express-zod-api/src/documentation.ts`:
- Around line 90-103: Add JSDoc for the new "limits" property on the
DocumentationParams type (used by the Documentation constructor) and for its
sub-properties "examples" and "security": include `@desc` describing purpose,
`@default` referencing defaultMaxCombinations, and `@example` showing a concrete
numeric example (e.g., 10) for both examples and security; ensure the top-level
limits JSDoc explains it groups per-feature combination limits and includes
`@default` and `@example` as well so the public API (Documentation constructor
params) has full documentation coverage.
---
Nitpick comments:
In `@express-zod-api/src/json-schema-helpers.ts`:
- Around line 89-112: In mergeExamples, the guard for limit > 0 is only applied
in the optional branch which lets the non-optional branch overwrite
target.examples with [] when limit <= 0; move the limit check to the top of
mergeExamples (before branching on isOptional) and return early when limit <= 0
so both branches preserve existing target.examples; keep the rest of the logic
involving target.examples, entry.examples, combinations and R.mergeDeepRight
unchanged.
In `@express-zod-api/tests/logical-container.spec.ts`:
- Around line 209-210: The test currently only asserts ".toHaveLength(2)" which
misses incorrect truncation or branch preference; replace the length-only
assertion on the produced combinations with a concrete equality/assertion that
lists the expected combination objects/strings (instead of just size) so the
capped-result behavior is verified precisely—locate the assertion using the
existing ".toHaveLength(2)" in the test and change it to assert the exact array
of combinations (or use an order-insensitive matcher like toEqual with a
sorted/normalized array or expect.arrayContaining) for the function under test.
In `@express-zod-api/tests/result-helpers.spec.ts`:
- Around line 120-121: The test only checks the count from
pullResponseExamples(schema, 3) which can hide duplicate/wrong items; update the
assertion to verify the exact subset returned (e.g., compare to the expected
first three examples) by replacing the toHaveLength check with an equality
assertion (use toEqual) against the deterministic array you expect from the
schema variable so the limiter behavior is strictly validated for
pullResponseExamples.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c388c653-9982-4e07-a342-df2fb36da197
📒 Files selected for processing (18)
CHANGELOG.mdexpress-zod-api/src/common-helpers.tsexpress-zod-api/src/diagnostics.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/documentation.tsexpress-zod-api/src/endpoint.tsexpress-zod-api/src/integration.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/logical-container.tsexpress-zod-api/src/result-helpers.tsexpress-zod-api/src/security.tsexpress-zod-api/tests/common-helpers.spec.tsexpress-zod-api/tests/documentation-helpers.spec.tsexpress-zod-api/tests/endpoint.spec.tsexpress-zod-api/tests/json-schema-helpers.spec.tsexpress-zod-api/tests/logical-container.spec.tsexpress-zod-api/tests/result-helpers.spec.tsexpress-zod-api/tests/security.spec.ts
There was a problem hiding this comment.
Important
The new commit addresses the prior processContainers(_, 0) footgun by clamping the per-step limit to Math.max(maxCombinations || 0, 1), so limits.security: 0 no longer wipes the documented security field. The fix itself is correct, but no test pins the new contract — a future revert of the early-exit removal would not be caught by CI.
Reviewed the following changes:
- Removed the
if (!(maxCombinations > 0)) return [];early exit inprocessContainersand clamped the innercombinations()limit to at least 1, solimits.security: 0produces a single combination instead of an emptysecurityarray - Tweaked the
limits.securityJSDoc to note the ≥1 contract and re-pointed@seeatMiddleware
Prior review feedback:
-
processContainers(_, 0)produced emptysecurityon documented operations — addressed via the clamp
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Replaced the
if (!(limit > 0)) return;early-exit inmergeExamples' optional branch with aMath.max(0, limit || 0)clamp onslice, so both branches now wipetarget.examplesto[]whenlimit <= 0(matching the non-optional branch'scombinations(..., 0) → []behavior).
Prior review feedback:
-
mergeExamplesasymmetric behavior atlimit === 0— addressed via the slice clamp (resolved by aligning the optional branch to also wipe, rather than the originally suggested early-return on the non-optional branch).
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Added a parameterised test for
Endpoint.getResponses("positive", { maxExamples })covering0,-1,1,2, andNaN, asserting the resultingexamplesarray length matchesmaxExamples > 0 ? maxExamples : 0.
Prior review feedback:
-
Endpoint.getResponses({ maxExamples: N })only exercised withN = 0— addressed by the new parameterised test.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@express-zod-api/tests/endpoint.spec.ts`:
- Around line 293-295: The parameterized test using test.each in the "should
respect the limit=%s of examples for positive variant" title should include the
index placeholder %# per the test naming guideline; update the test title string
passed to test.each to "should respect the limit=%s of examples for positive
variant (case %#)" or similar so it contains both %s and %#, leaving the
test.each([...]) and the test callback (maxExamples) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f83a1510-0bb5-4114-bb9c-c944ce2ee708
📒 Files selected for processing (1)
express-zod-api/tests/endpoint.spec.ts
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Parameterised the
processContainersmax-combinations test to cover0, -1, 1, 2, NaN, asserting the result length equalslimit > 1 ? limit : 1— pinning the prior commit'sMath.max(maxCombinations || 0, 1)clamp so a future revert would be caught by CI.
Prior review feedback:
- No test pinned the new
processContainersclamp contract — addressed by the parameterisedtest.eachinsecurity.spec.ts.
Claude Opus ｜ 𝕏
RobinTail
force-pushed
the
limit-combinations-2
branch
from
8a64edc to
29ea1e5
Compare
RobinTail
force-pushed
the
limit-combinations-2
branch
from
29ea1e5 to
57ca58f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@express-zod-api/src/documentation.ts`:
- Around line 214-219: The header extraction is using the full endpoint.security
tree (via getSecurityHeaders(endpoint.security)) even though securityRefs are
built from the truncated processContainers(endpoint.security, maxSecurity) and
limits.security should apply; update the flow so getSecurityHeaders is fed the
same truncated security representation (e.g., pass securityRefs or the output of
processContainers/limits.security) before calling depictRequestParams and any
other header extraction sites (like the similar block around the
depictRequestParams call at the other location), ensuring headers for discarded
OR-branches are not emitted as request parameters.
In `@express-zod-api/tests/logical-container.spec.ts`:
- Around line 200-201: The test title for the parameterized spec using
test.each([0, -1, 1, 2, NaN]) is missing the index placeholder; update the title
string in the test.each(...) call (the test name passed to test.each) to include
the %# placeholder in addition to %s so it prints both the value and the case
index (i.e., change the title from "should limit the maxCombinations=%s to at
least 1" to include "%#" alongside "%s" in the test.each invocation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1af5be6e-7bf0-4d00-b14d-a53b2f1fc31a
📒 Files selected for processing (4)
CHANGELOG.mdexpress-zod-api/src/documentation.tsexpress-zod-api/tests/endpoint.spec.tsexpress-zod-api/tests/logical-container.spec.ts
There was a problem hiding this comment.
Important
The new JSDoc block on DocumentationParams.limits wraps its directives in backticks (`@desc`, `@default`, `@example`) instead of using them as real JSDoc tags. Every other JSDoc block in this file (including the nested examples and security inside this same limits) uses the plain @desc / @default / @example form required by AGENTS.md. The backticked variant renders the tags as inline code text and makes them invisible to any tooling that consumes JSDoc directives.
Reviewed the following changes:
- Added JSDoc to the top-level
DocumentationParams.limitsproperty and expanded the nestedlimits.examplesdescription — addresses prior CodeRabbit feedback about missing JSDoc on the newlimitsoption.
Prior review feedback:
-
Documentationconstructorlimitsoption lacked JSDoc — addressed (modulo the directive-formatting issue flagged below).
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Unwrapped the
@desc,@default, and@exampledirectives onDocumentationParams.limitsso they're real JSDoc tags rather than inline-code text — the nestedlimits.examplesandlimits.securityblocks were already correct and unchanged.
Prior review feedback:
-
DocumentationParams.limitsJSDoc directives were backticked and invisible to JSDoc tooling — addressed by removing the backticks.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
♻️ Duplicate comments (2)
express-zod-api/src/documentation.ts (2)
214-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
securityHeadersis still derived from the full security tree whilesecurityRefsuses the truncated one.
securityHeadersis extracted from the completeendpoint.security(line 214), butsecurityRefsis built fromprocessContainers(endpoint.security, maxSecurity)(line 269), which truncates OR-branches tomaxSecurityentries. When the limit discards combinations, headers belonging to those dropped security schemes are still injected into the request parameters viadepictRequestParams, even though the corresponding schemes are no longer documented in the operation'ssecurityfield — creating an inconsistency visible to API consumers.The fix is to derive
securityHeadersfrom the same truncated representation used forsecurityRefs. One approach:📝 Proposed fix
- const securityHeaders = getSecurityHeaders(endpoint.security); + const truncatedSecurity = processContainers(endpoint.security, maxSecurity); + const securityHeaders = getSecurityHeaders(truncatedSecurity); const depictedParams = depictRequestParams({ ...commons, inputSources, isHeader, securityHeaders, request, ... }); ... const securityRefs = depictSecurityRefs( depictSecurity( - processContainers(endpoint.security, maxSecurity), + truncatedSecurity, inputSources, ), ... );Also applies to: 267-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@express-zod-api/src/documentation.ts` around lines 214 - 219, securityHeaders is currently derived from the full endpoint.security while securityRefs uses the truncated output of processContainers(endpoint.security, maxSecurity), causing injected headers that don't correspond to documented security combos; change the code to compute truncatedSecurity = processContainers(endpoint.security, maxSecurity) (or reuse the existing variable used to build securityRefs) and pass that truncatedSecurity into getSecurityHeaders and depictRequestParams instead of endpoint.security so getSecurityHeaders(...) and depictRequestParams(..., securityHeaders, ...) both reflect the same truncated security representation (update any references to securityHeaders, securityRefs, and the call sites of getSecurityHeaders and depictRequestParams to use the truncated value).
95-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
@exampledirective still missing onexamplesandsecurityinner properties.Both inner properties (
examplesandsecurity) have@desc,@default, and@seebut are missing@example, which is required by the coding guidelines for all publicly available entity properties. This was previously proposed (with@example 20/@example 10) and confirmed as addressed, but the current code doesn't include them.📝 Proposed fix
/** * `@desc` Caps the number of generated examples (request/response examples from examples of individual properties). * `@default` defaultMaxCombinations * `@see` defaultMaxCombinations + * `@example` 20 * */ examples?: number; /** * `@desc` Caps the number of security schemas combinations. Must be at least 1. * `@default` defaultMaxCombinations * `@see` Middleware + * `@example` 10 * */ security?: number;As per coding guidelines: "All properties of publicly available entities exposed via
index.tsmust have JSDoc documentation with@desc,@default, and@exampledirectives."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@express-zod-api/src/documentation.ts` around lines 95 - 108, Add missing `@example` JSDoc tags for the two inner properties under the limits option: add an `@example` value (e.g. "@example 20") to the examples property and an `@example` value (e.g. "@example 10") to the security property in the JSDoc above the limits block so that limits.examples and limits.security include `@desc`, `@default`, and `@example` as required by the public API guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@express-zod-api/src/documentation.ts`:
- Around line 214-219: securityHeaders is currently derived from the full
endpoint.security while securityRefs uses the truncated output of
processContainers(endpoint.security, maxSecurity), causing injected headers that
don't correspond to documented security combos; change the code to compute
truncatedSecurity = processContainers(endpoint.security, maxSecurity) (or reuse
the existing variable used to build securityRefs) and pass that
truncatedSecurity into getSecurityHeaders and depictRequestParams instead of
endpoint.security so getSecurityHeaders(...) and depictRequestParams(...,
securityHeaders, ...) both reflect the same truncated security representation
(update any references to securityHeaders, securityRefs, and the call sites of
getSecurityHeaders and depictRequestParams to use the truncated value).
- Around line 95-108: Add missing `@example` JSDoc tags for the two inner
properties under the limits option: add an `@example` value (e.g. "@example 20")
to the examples property and an `@example` value (e.g. "@example 10") to the
security property in the JSDoc above the limits block so that limits.examples
and limits.security include `@desc`, `@default`, and `@example` as required by the
public API guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 748b1341-f50d-4432-85b1-524f48003e1a
📒 Files selected for processing (1)
express-zod-api/src/documentation.ts
|
@pullfrog please review the whole idea again, evaluate the design and the messiness of the implementation |
There was a problem hiding this comment.
Important
Holistic re-review at your request. The core idea lands well — combinations(..., limit) is the right place for the cap, splitting getSecurityHeaders from processContainers is a genuine improvement, and the isStrict/options-bag refactor of flattenIO/mergeExamples/processAllOf is cleaner than the "coerce"|"throw" string it replaces. But three wrinkles are worth addressing before this ships.
Design
-
The headline feature is disarmed by default.
defaultMaxCombinations = Infinity+@todo set to 20 in v28means v27.4.0 ships the mechanism but none of the policy. Users who don't already know they have a combinatorial-explosion problem still won't, and the CHANGELOG's 64-combo example doesn't tell them what to set. Either pick a conservative default now (the v28 TODO is already admitting it's the right thing to do), or make the CHANGELOG prescriptive ("we recommend{ examples: 20, security: 10 }") so the feature has an on-ramp. -
maxExamplesis over-plumbed as a cross-cutting concern. It flows throughDocumentation→commons→OpenAPIContext.ctx→depictRequest/depictResponse/depictBody/depictRequestParams→flattenIO→mergeExamples/pullRequestExamples, and separately throughEndpoint.getResponses→#ensureOutputExamples→pullResponseExamples.OpenAPIContextalready exists to carry this kind of per-build value; every depicter that destructuresmaxExamplesinto its own signature is adding noise to functions that don't actually use it. Committing fully toctx(or closing overmaxExamplesat factory-construction time) would collapse several of these layers.
Messiness
-
Name drift between layers. The same number is called
examples/maxExamples/limit, and its sibling issecurity/maxSecurity/maxCombinations. The public shapelimits: { examples, security }is fine — it's the internal translation that proliferates. Picking one spine (e.g.maxExamples/maxSecurityat boundaries,limitonly inside the genericcombinationshelper) would be a 10-minute rename. -
Three NaN/zero guards for the same concept:
!(limit > 0) → []incombinations/pullRequestExamples/pullResponseExamples;Math.max(0, limit || 0)inmergeExamples;Math.max(maxCombinations || 0, 1)inprocessContainers. The security branch has different semantics on purpose (always emit ≥1 combo) but the idiom should be shared — e.g. a singleclampPositive(n, min)helper. -
Leftover TODOs in public JSDoc (
common-helpers.ts:112,documentation.ts:105). The most recent commit literally added one of them rather than closing it. -
Missing
@exampleonlimits.examples/limits.security. Already flagged by CodeRabbit upthread; still missing at the time of review.
Correctness — one subtle item worth fixing, one worth documenting
-
#ensureOutputExampleslostR.oncebut gained a sticky global-registry cache. First call wins permanently — twonew Documentation({ limits: { examples: N } })against the same endpoints with differentNnow silently reuse the firstN. The new endpoint test exercises a single call per factory and can't catch this. Either re-add instance-scoped memoization keyed onlimit, or don't mutateglobalRegistryat all and return the truncated list to the caller. -
processContainerssilently clampssecurity: 0to 1 while its own JSDoc asks users to "Must be at least 1." The clamp is defensible (an emptysecurityarray is almost always a documentation bug) but it violates the JSDoc's implied contract.
Inline comments below are scoped to the sharpest concrete issues; the rest of the list above is best handled as a tidy-up pass.
Claude Opus ｜ 𝕏
| /** @todo set to 20 in v28 to avoid too many combinations */ | ||
| export const defaultMaxCombinations = Infinity; |
There was a problem hiding this comment.
Shipping the limit feature with Infinity as the default means v27.4.0 users still hit the combinatorial explosion the CHANGELOG warns about unless they discover and configure limits. The @todo already concedes the right default is lower — consider picking it now (e.g. 20) and treating the cap-by-default as the breaking change it effectively is in v28, rather than landing a disarmed guardrail.
| /** | ||
| * @desc Caps the number of generated examples (request/response examples from examples of individual properties). | ||
| * @default defaultMaxCombinations | ||
| * @see defaultMaxCombinations | ||
| * */ | ||
| examples?: number; | ||
| /** | ||
| * @desc Caps the number of security schemas combinations. Must be at least 1. | ||
| * @default defaultMaxCombinations | ||
| * @todo decouple from defaultMaxCombinations, use higher but still fixed limit in v28 | ||
| * @see Middleware | ||
| * */ | ||
| security?: number; |
There was a problem hiding this comment.
Two issues on the nested properties: (1) a @todo directive is landing in public-API JSDoc, meaning the open design question ships to users; decide now whether security shares defaultMaxCombinations or gets its own constant. (2) AGENTS.md requires @example on all optional public-entity properties — both examples and security are missing it (the parent limits has one). CodeRabbit already flagged #2 upthread.
| /** | |
| * @desc Caps the number of generated examples (request/response examples from examples of individual properties). | |
| * @default defaultMaxCombinations | |
| * @see defaultMaxCombinations | |
| * */ | |
| examples?: number; | |
| /** | |
| * @desc Caps the number of security schemas combinations. Must be at least 1. | |
| * @default defaultMaxCombinations | |
| * @todo decouple from defaultMaxCombinations, use higher but still fixed limit in v28 | |
| * @see Middleware | |
| * */ | |
| security?: number; | |
| /** | |
| * @desc Caps the number of generated examples (request/response examples from examples of individual properties). | |
| * @default defaultMaxCombinations | |
| * @see defaultMaxCombinations | |
| * @example 20 | |
| * */ | |
| examples?: number; | |
| /** | |
| * @desc Caps the number of security schemas combinations. Values below 1 are clamped to 1. | |
| * @default defaultMaxCombinations | |
| * @see Middleware | |
| * @example 10 | |
| * */ | |
| security?: number; |
| R.map((opt) => (isSimple(opt) ? [opt] : opt.and), entry), | ||
| ([a, b]) => R.concat(a, b), | ||
| R.concat, | ||
| Math.max(maxCombinations || 0, 1), |
There was a problem hiding this comment.
Math.max(maxCombinations || 0, 1) silently coerces 0/negatives/NaN to 1, which contradicts the limits.security JSDoc saying "Must be at least 1." Either enforce the contract at the Documentation boundary (throw DocumentationError on invalid input), or weaken the JSDoc to say values below 1 are clamped. Right now readers are told one thing and given another.
| /** considered expensive operation, only required for generators */ | ||
| #ensureOutputExamples = R.once(() => { | ||
| #ensureOutputExamples(limit: number) { | ||
| if (globalRegistry.get(this.#def.outputSchema)?.examples?.length) return; // examples on output schema, or pull up: | ||
| if (!isSchema<z.core.$ZodObject>(this.#def.outputSchema, "object")) return; | ||
| const examples = pullResponseExamples(this.#def.outputSchema); | ||
| const examples = pullResponseExamples(this.#def.outputSchema, limit); | ||
| if (!examples.length) return; | ||
| const current = this.#def.outputSchema.meta(); | ||
| globalRegistry | ||
| .remove(this.#def.outputSchema) // reassign to avoid cloning | ||
| .add(this.#def.outputSchema, { ...current, examples }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Dropping R.once here traded closure-memoization for a sticky globalRegistry cache (the examples?.length check at line 96 short-circuits subsequent calls). The effect: the first limit value passed wins permanently for a given endpoint instance. If a consumer creates two Documentation generators with different limits.examples sharing endpoints, the second silently reuses the first's cap. The new test at endpoint.spec.ts:292-309 only exercises one call per factory. Consider re-adding instance-scoped memoization keyed on limit, or stop mutating globalRegistry and return the truncated examples to the caller so each consumer gets independent results.
| public abstract getResponses( | ||
| variant: ResponseVariant, | ||
| params: { maxExamples?: number }, | ||
| ): ReadonlyArray<NormalizedResponse>; |
There was a problem hiding this comment.
The options bag is required but its only field is optional, so every caller that doesn't want to cap must still write getResponses(variant, {}) or { maxExamples: N }. Four existing sites now carry that boilerplate. Making the bag itself optional would let indifferent callers drop it entirely.
| public abstract getResponses( | |
| variant: ResponseVariant, | |
| params: { maxExamples?: number }, | |
| ): ReadonlyArray<NormalizedResponse>; | |
| public abstract getResponses( | |
| variant: ResponseVariant, | |
| params?: { maxExamples?: number }, | |
| ): ReadonlyArray<NormalizedResponse>; |
|
I think I'd rather make a reasonably high limit in #3208 instead depending on some other factor |
1 participant
Instead of #3344
Summary by CodeRabbit
New Features
Tests