Open
Conversation
|
coverage: 100.0%. remained the same — limit-combinations into master |
Contributor
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThreads an optional ChangesCombination + Example Bounding
Sequence Diagram(s)sequenceDiagram
participant Docs as "Documentation" rect rgba(63,81,181,0.5)
participant Endpoint as "Endpoint" rect rgba(3,169,244,0.5)
participant JSON as "json-schema-helpers" rect rgba(0,150,136,0.5)
participant Comb as "combinations" rect rgba(255,193,7,0.5)
Docs->>Endpoint: getResponses(variant, { maxCombinations })
Endpoint->>Endpoint: ensureOutputExamples(limit = maxCombinations)
Endpoint->>JSON: pullResponseExamples(..., limit)
JSON->>Comb: combinations(left, right, merge(a, b), limit)
Comb-->>JSON: merged (possibly truncated) examples
JSON-->>Endpoint: examples
Endpoint-->>Docs: responses with bounded examples
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
RobinTail
force-pushed
the
limit-combinations
branch
from
94693b9 to
120fa0d
Compare
…security container processing.
RobinTail
added a commit
that referenced
this pull request
Apr 24, 2026
…chema` (#3345) Performance regression since v24.0.0 - the `outputSchema` getter was calling the expensive `pullResponseExamples()` operation on first access (R.once protected), even though examples are only needed for OpenAPI documentation generation (getResponses). This fixes the issue where simply accessing the output schema for runtime validation was unnecessarily triggering example generation. first introduced by #2666 merged by #2546 found this during my work on #3344 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved request performance by deferring output schema example processing; examples are now generated only when needed for documentation/OpenAPI workflows, not during initial request handling. * **Documentation** * Added changelog entry v27.2.4 documenting the change in example processing behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
R.once on #ensureOutputExamples silently drops the limit parameter from all callers after the first. In development mode, Diagnostics wins the race with maxCombinations: 0, preventing Documentation from ever generating output examples.
Task list (4/4 completed)
- Read full diff and understand the feature
- Trace data flow and check boundaries for each area of change
- Self-critique drafted comments
- Submit review
Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@express-zod-api/src/documentation-helpers.ts`:
- Around line 129-133: depictIntersection reads maxCombinations from the
depiction context but depictResponse and depictRequest build ctx without that
field, causing intersections to run unbounded; update the context construction
in depictResponse and depictRequest to include maxCombinations (propagate the
value from the function arguments/config/options into the ctx passed to
depictIntersection and flattenIO) so that flattenIO receives the intended
maxCombinations limit.
In `@express-zod-api/src/documentation.ts`:
- Around line 89-94: The JSDoc for the public constructor option maxCombinations
is missing an `@example`; update the comment block for the maxCombinations
property in documentation.ts to include an `@example` directive showing typical
usage (e.g., setting maxCombinations: 20 or Infinity) while keeping the existing
`@desc` and `@default` lines so the property has `@desc`, `@default`, and `@example` as
required by the public API documentation guidelines.
In `@express-zod-api/src/endpoint.ts`:
- Around line 94-98: Remove the R.once memoization wrapper from the private
methods that generate examples so they can be re-run with different limits:
specifically un-wrap the definitions of `#ensureOutputExamples` and the analogous
`#ensureInputExamples` (the ones currently defined as R.once((limit?: number) => {
... })). Leave the existing guard checks (e.g., the globalRegistry lookup and
isSchema check inside `#ensureOutputExamples`) intact so redundant work is still
avoided, but allow the function body to execute on each call with the provided
limit parameter instead of being permanently memoized.
🪄 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: 99c13e7d-1c60-4cf9-8e92-1770c8851336
📒 Files selected for processing (14)
express-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/tests/common-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.ts
Owner
Author
|
the direct limit of the security schemes may be really confusing. it should only apply to the Cartesian product (multiplication), early exits should be removed too. |
Contributor
There was a problem hiding this comment.
Important
Stale %s placeholder in a test name after converting from test.each to a plain test.
Reviewed the following changes:
- Narrowed
limitsemantics incombinations()to apply only to the cartesian product of two non-empty arrays — the concatenation passthrough path (left.concat(right)) is no longer truncated - Removed redundant early-return guards from
mergeExamples,pullRequestExamples, andpullResponseExamples— these are unnecessary sincecombinations()already handles0, negative, andNaNlimits correctly via the loop conditionresult.length < limit - Updated JSDoc on
maxCombinationsto"disables combinations, but keeps concatenations", clarifying the intent that0suppresses cross-products without suppressing passthrough concatenation - Removed edge-case tests for non-positive/
NaNlimits inmergeExamples,pullRequestExamples,pullResponseExamples, and simplified theprocessContainerslimit test to a single assertion withlimit = 2
Prior review feedback:
-
R.onceon#ensureOutputExamplessilently drops thelimitparameter — addressed in prior commits - Propagate
maxCombinationsinto depictionctxfordepictResponseanddepictRequest— addressed in prior commits
Claude Opus ｜ 𝕏
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/tests/logical-container.spec.ts (1)
127-137: Consider asserting the returned combinations, not just their count.
toHaveLength(2)confirms truncation occurred but does not verify which combinations survived or that ordering is preserved (i.e., that the cap stops generation deterministically rather than discarding arbitrary results). Given the existing tests in this file already assert the full Cartesian output, asserting the exact truncated prefix would make this test more robust against regressions in the truncation order.♻️ Suggested stronger assertion
test("should control the maximum combinations", () => { expect( processContainers( [ { or: [{ and: [1, 2] }, { and: [3, 4] }] }, { or: [{ and: [5, 6] }, { and: [7, 8] }] }, ], 2, ), - ).toHaveLength(2); + ).toEqual([ + [1, 2, 5, 6], + [1, 2, 7, 8], + ]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/tests/logical-container.spec.ts` around lines 127 - 137, The test "should control the maximum combinations" only checks length; update it to assert the actual returned combinations from processContainers (not just toHaveLength) to ensure deterministic truncation and ordering. Replace the existing length assertion with an equality assertion against the expected truncated prefix (call processContainers with the same inputs and max=2 and assert the returned array equals the first two combinations in the deterministic Cartesian order produced by processContainers) so the test verifies which combinations survive and that ordering is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@express-zod-api/tests/logical-container.spec.ts`:
- Around line 127-137: The test "should control the maximum combinations" only
checks length; update it to assert the actual returned combinations from
processContainers (not just toHaveLength) to ensure deterministic truncation and
ordering. Replace the existing length assertion with an equality assertion
against the expected truncated prefix (call processContainers with the same
inputs and max=2 and assert the returned array equals the first two combinations
in the deterministic Cartesian order produced by processContainers) so the test
verifies which combinations survive and that ordering is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e62ed6c2-3d0b-4383-adb3-becc1d3f5695
📒 Files selected for processing (1)
express-zod-api/tests/logical-container.spec.ts
Contributor
There was a problem hiding this comment.
Important
JSDoc indentation on the new maxCombinations property is misaligned with the rest of the interface.
Reviewed changes:
- Added CHANGELOG entry for
v27.3.0documenting themaxCombinationsfeature with a code example - Rewrote the
maxCombinationsJSDoc inDocumentationParams— clearer wording distinguishing "product combinations" from "concatenations"
Claude Opus ｜ 𝕏
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@express-zod-api/src/documentation.ts`:
- Around line 89-95: The JSDoc block starting with "Limits cartesian product
when generating examples by combining each property's own examples." is
mis-indented and failing Prettier; fix it by adding 2-space indentation to each
line of the comment block so each line aligns with the surrounding code style
(prefix each line with two spaces), preserving all text and annotations (e.g.,
`@desc`, `@example`, `@default`, `@todo`).
🪄 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: 18c023d7-21a1-4f4b-8f1b-5e1b780f8d87
📒 Files selected for processing (2)
CHANGELOG.mdexpress-zod-api/src/documentation.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
RobinTail
force-pushed
the
limit-combinations
branch
from
639cc12 to
c969f22
Compare
Contributor
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Removed early-exit guards (
if (!(limit > 0)) return []) frommergeExamples,processContainers,pullRequestExamples, andpullResponseExamples— the limit is now enforced solely by thecombinations()loop condition (result.length < limit), which naturally handles0,-1, andNaN - Changed
combinations()to returnleft.concat(right)without.slice(0, limit)when one side is empty, making the limit apply exclusively to the Cartesian product of two non-empty arrays — JSDoc updated to reflect this - Rearranged
pickHeadersto checkisLogicalAnd/isLogicalOrfirst and fall through to the simple leaf case, removing the redundantisSimpleguard and fixing coverage of the terminalreturn [] - Simplified tests: removed edge-case parameterized tests for
0/-1/NaNfrom higher-level functions (now covered bycombinations()itself), added explicit test for passthrough-without-limit behavior - Updated JSDoc for
maxCombinationsto clarify it limits Cartesian product only ("disables product combinations, keeps concatenations") - Added CHANGELOG entry for v27.3.0 with a clear example of the combinatorial explosion this feature addresses
Claude Opus ｜ 𝕏
Contributor
|
No code changes needed — the PR author already fixed the JSDoc indentation on Task list (3/3 completed)
|
Contributor
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Removed early-exit guards from
mergeExamples,pullRequestExamples,pullResponseExamples, andprocessContainers— thelimitis now enforced solely by the loop condition incombinations(), and only for actual Cartesian products (both arrays non-empty) - Changed
combinations()passthrough path (left.concat(right)) to no longer truncate bylimit, with updated JSDoc clarifying the narrowed scope: "only applies to Cartesian product of non-empty arrays" - Removed the
.slice(0, maxCombinations)truncation frommergeExamples's optional/concat branch, consistent with the design that limit targets cross-products only - Rearranged
pickHeadersto checkisLogicalAnd/isLogicalOrfirst and fall through to the simplecontainer.type === "header"case, fixing coverage for the terminalreturn [] - Added CHANGELOG entry for v27.3.0 with a clear description and worked example of the combinatorial explosion the feature addresses
- Refined
maxCombinationsJSDoc: more specific wording on what "applies to" means, updated@exampleto say "disables product combinations, keeps concatenations" - Cleaned up tests: removed edge-case parameterized tests for removed early-exit behavior, added
"should not apply the limit when returns original array"test, simplifiedprocessContainerslimit test to singlelimit=2case
Claude Opus ｜ 𝕏
RobinTail
changed the title
Contributor
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Moved
pickHeadersandpickSecurityHeadersfromlogical-container.tstosecurity.tsand renamed the export togetSecurityHeaders— colocating the helper with theSecuritytypes it operates on and removing theSecurityimport fromlogical-container.ts - Updated the
documentation.tsimport accordingly
Claude Opus ｜ 𝕏
Contributor
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Added
security.spec.tswith unit tests forgetSecurityHeaders— covers flat security arrays (filtering to header-type only), empty input, single-level AND/OR containers, and nested AND/OR containers usingtest.eachwith%#placeholders
Claude Opus ｜ 𝕏
RobinTail
force-pushed
the
limit-combinations
branch
from
9727140 to
61ef57a
Compare
Owner
Author
|
Swagger validator is 502/504 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
API Changes
Tests
Changelog