fix(theme): hoist schema normalization out of render path#1527
Draft
sserrata wants to merge 17 commits into
Draft
fix(theme): hoist schema normalization out of render path#1527sserrata wants to merge 17 commits into
sserrata wants to merge 17 commits into
Conversation
Three O(subtree) recursive walkers added between v4.7.1 and v5.1.0 ran on every SchemaNode render, turning O(N) total work into O(N²) and hanging the browser on deeply recursive specs like Komga's (~17K nodes): - `findDiscriminator` (#1303) searched the entire subtree from every SchemaNode; in 4.7.1 this was `schema.discriminator` (O(1)). - `findProperty` (#1303) searched the entire subtree from every DiscriminatorNode; in 4.7.1 this was `schema.properties?.[name]` (O(1)). - `stripConflictingAdditionalProps` (#1463) deep-walked every value inside `mergeAllOf`, compounding with `foldSiblingsIntoBranches` (#1457) which called `mergeAllOf` per oneOf/anyOf branch. Fix: introduce `normalizeSchema` (new `Schema/normalize.ts`) which does a single O(N) pass via `useMemo` — merging allOf, stripping conflicting additionalProperties, and folding siblings into oneOf/anyOf branches. Revert SchemaNode to 4.7.1's O(1) discriminator check (normalization promotes discriminators from allOf members to top level) and DiscriminatorNode to O(1) property lookup. Add WeakMap cache to the build-time `stripConflictingAdditionalProps` in createSchema.ts. Closes #1525 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Size Change: +31.3 kB (+1.34%) Total Size: 2.36 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Visit the preview URL for this PR (updated for commit abf6870): https://docusaurus-openapi-36b86--pr1527-9bwck37p.web.app (expires Thu, 09 Jul 2026 23:08:25 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: bf293780ee827f578864d92193b8c2866acd459f |
Minimal OpenAPI spec modeled after Komga's search-filter schema: a SearchCondition with a discriminator on `operator` whose AND/OR/NOT branches recurse back into SearchCondition. After $ref dereferencing this produces the same recursive oneOf + allOf + discriminator shape that caused the browser hang. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
normalizeSchema was calling mergeAllOf on allOf arrays containing circular-reference string markers (e.g. `allOf: ["circular(Title)"]`) produced by loadAndResolveSpec's JSON.stringify roundtrip. allof-merge doesn't handle string members and would discard the marker, breaking SchemaEdge's existing circular reference rendering. Skip the merge when any allOf member is a string. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Six endpoints testing circular $ref patterns: - Self-referencing tree node (items position) - Self-referencing category (property + items positions) - Mutual recursion between Person and Address - Circular ref via additionalProperties - Circular ref inside allOf composition - Deeply nested circular ref through intermediate schema Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers allOf merging, sibling folding, additionalProperties stripping, circular reference marker preservation, recursive normalization, and identity caching. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the separate WeakSet (normalizedSet) + per-call WeakMap with a single module-level WeakMap that serves both purposes. Self-register each result via cache.set(result, result) so re-normalization is an identity no-op. Avoids stale entries persisting across unrelated renders. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The renderer only handled circular string markers (e.g. "circular(TreeNode)") when they appeared inside allOf. Markers in property, items, and additionalProperties positions were silently dropped. Add early-return handlers in SchemaEdge (property position), Items (array items position), and AdditionalProperties for circular string markers. Also add a SchemaEdge route for arrays whose items is a circular string so they reach SchemaNodeDetails -> Items. Update circularRef.yaml fixture to include inline schema snippets in endpoint descriptions, matching the convention of other test specs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
foldSiblingsIntoBranches returned only the branch key, dropping parent metadata like title, description, and discriminator. Destructure out the sibling keys (properties, required, type) that get merged into branches and spread the remaining metadata onto the result. Fixed in both normalize.ts and the inline copy in Schema/index.tsx. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Remove the inline copies of stripConflictingAdditionalProps, mergeAllOf, and foldSiblingsIntoBranches from Schema/index.tsx and import them from Schema/normalize.ts instead. The plugin-side copy in createSchema.ts remains separate since the plugin and theme are different npm packages. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove dead findProperty and findDiscriminator from Schema/index.tsx - Wire cached getDiscriminator into SchemaNode as fallback for discriminators that allof-merge doesn't promote to top level - Fix foldSiblingsIntoBranches to use METADATA_KEYS allowlist instead of hardcoded destructuring, preventing non-metadata keys from leaking - Rename mergeAllOf parameter from allOf to schema for clarity - Trim excessive multi-line comments across normalize.ts, index.tsx, and createSchema.ts - Add tests for foldSiblingsIntoBranches and metadata/extension handling Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… scope
- Extract isCircularMarker() predicate to replace 4 scattered typeof
checks with a stricter guard that validates the "circular(" prefix
- Remove the `&& working.properties` narrowing on the sibling folding
gate so normalizeSchema folds all sibling keys (matching the original
foldSiblingsIntoBranches behavior)
- Add tests for discriminator property defined only in oneOf branches
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Discriminator - Wrap normalizeSchema with a clean public signature, moving the WeakMap cache parameter into a private normalizeSchemaImpl function - Collapse three identical oneOf/anyOf/allOf loops in getDiscriminator into a single loop over BRANCH_KEYS - Document foldSiblingsIntoBranches call chain in JSDoc Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
foldSiblingsIntoBranches was merging the parent discriminator (and other
metadata) into each oneOf/anyOf branch via allOf-merge. The merged
branches then carried their own discriminator, causing the inner
SchemaNode to render a SECOND DiscriminatorNode inside each tab with an
empty mapping ("Possible values: []"). Shared properties at the top
level (e.g. CommonProps.id, CommonProps.createdAt in
nested-discriminator-in-all-of) also went missing because folding
stripped them from the top-level schema, and DiscriminatorNode's
PropertyDiscriminator only renders shared props from schema.properties
at the top level.
Fix: skip foldSiblingsIntoBranches entirely when the schema has a
discriminator — DiscriminatorNode already handles top-level shared
properties via PropertyDiscriminator. Additionally exclude METADATA_KEYS
from the siblings set even for non-discriminator schemas, so metadata
never leaks into folded branches.
Affected visual regressions (7 of 15 diffs):
- discriminator-with-shared-properties-without-mapping
- discriminator-with-nested-schemas-without-mapping
- nested-discriminator-in-all-of
- deeply-nested-discriminator-in-all-of-chains
- doubly-nested-discriminators-issue
- all-of-with-multiple-one-of-constraints
- any-of-with-nested-one-of-and-properties-at-same-level
Update affected tests to assert the new correct behavior: when a
discriminator is present, top-level properties stay at the top level
and branches retain their own properties without sibling pollution.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nators Eager allOf merge in normalizeSchema caused cartesian product expansion for allOf with multiple oneOf members (e.g. allof-multiple-oneof: 2 x 3 = 6 cartesian branches instead of two separate tab strips). Eager foldSiblingsIntoBranches caused double-folding when SchemaNode's renderChildren ran fold again — for any-of-with-nested-oneOf the cascade stripped top-level properties and lost the second anyOf group. Both eager operations were added to promote the discriminator property to the top level so DiscriminatorNode's O(1) schema.properties lookup would work. Replace this with a more surgical fix in DiscriminatorNode: 1. Eagerly merge top-level allOf at the start of DiscriminatorNode so shared properties contributed by allOf members (e.g. CommonProps in nested-discriminator-in-all-of) are accessible to PropertyDiscriminator's top-level rendering. 2. Move discriminatorProperty calculation AFTER the merge loop. The loop populates schema.properties[discriminator.propertyName] from branches, but the previous code captured the empty pre-loop value, causing missing "string" type annotations on discriminator headers in doubly-nested cases. normalizeSchema is now an identity pass-through. The perf win for #1525 still comes from getDiscriminator's WeakMap cache replacing the O(subtree) findDiscriminator walk, plus the WeakMap cache in stripConflictingAdditionalProps preventing redundant strip walks. SchemaNode's existing allOf/renderChildren paths handle merging and folding conditionally, matching pre-PR behavior for visual rendering. Visual regressions fixed (3 of the 4 schema diffs): - doubly-nested-discriminators-issue (missing type annotations) - all-of-with-multiple-one-of-constraints (cartesian expansion) - any-of-with-nested-one-of-and-properties-at-same-level (missing fields) The 4th diff (discriminator-with-all-of-and-mapping) is an improvement — preview now renders sharedProp that prod was missing. Rewrite the test suite to assert the new behavior: normalizeSchema is identity, mergeAllOf and foldSiblingsIntoBranches are tested directly, getDiscriminator covers all branch types plus cycle handling. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…is nested
Match prod parity: SchemaNode in main only calls mergeAllOf when the
discriminator is nested inside allOf (`schema.allOf && !schema.discriminator
&& resolvedDiscriminator`). When the discriminator is at the raw top level,
prod passes the unmerged schema to DiscriminatorNode, so shared properties
from other allOf members never appear at the top level.
Previous fix eagerly merged whenever `rawSchema.allOf` existed, which
pulled sharedProp from BaseAllOfMapping's second allOf member into the
top-level rendering (discriminator-with-all-of-and-mapping). While
technically an improvement (prod had a bug), the user wants prod parity.
Condition the merge on `!rawSchema.discriminator`:
- Discriminator at top level (BaseAllOfMapping): skip eager merge, schema
stays as raw with just `properties: {type}` → matches prod
- Discriminator nested (NestedDiscriminatorBase, DoublyNestedBase): do
eager merge, so `properties: {id, name, createdAt}` from allOf members
are accessible → matches prod's `workingSchema = mergeAllOf(schema)`
When discriminator is at top level AND schema still has allOf, do a
temporary merge just to extract discriminatedSchemas (oneOf/anyOf) without
replacing schema — matches prod's original DiscriminatorNode behavior.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previous attempts to fix doubly-nested-discriminators-issue by eagerly merging allOf inside DiscriminatorNode still produced visual diffs vs prod. Root cause: the eager merge in DiscriminatorNode created a different object identity/mutation timing than prod's approach of merging in SchemaNode before passing workingSchema down. Revert to main's exact structure: - SchemaNode: `workingSchema = mergeAllOf(schema)` when discriminator is nested in allOf (matches main line 1194-1211) - DiscriminatorNode: no internal merge, uses schema directly and looks up discriminatorProperty via recursive walk (matches main line 447) To preserve the #1525 perf fix, replace the O(subtree) findProperty walk with a new cached `findPropertyDeep` (WeakMap keyed by object + property name, sentinel-based cycle detection). Same O(1) amortized guarantee as getDiscriminator. Both replace main's per-render recursive walks that caused the O(N^2) render cost. Add tests for findPropertyDeep: top-level lookup, oneOf/anyOf/allOf recursion, missing property, primitive input, cycle handling. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…le mutation reads DiscriminatorNode's merge loop deletes the discriminator property from each subschema.properties to prevent duplicate rendering inside tabs. The WeakMap cache in stripConflictingAdditionalProps was returning the pre-mutation deep clone on subsequent mergeAllOf calls, causing the deleted properties to reappear as an extra "type" row inside nested tab content (visible in doubly-nested-discriminators-issue between timestamp and id). Removing the cache matches prod's per-call behavior. Perf impact is minimal — stripConflictingAdditionalProps is only called inside mergeAllOf, which is invoked at most a handful of times per rendered schema (once per DiscriminatorNode, once per SchemaEdge.allOf branch), and the O(subtree) walk cost is dominated by allof-merge itself, not the strip pass. The cache was originally added as part of the perf fix, but the real perf wins for #1525 come from getDiscriminator's cache (replacing the per-render findDiscriminator walk) — not from caching mergeAllOf. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
Fixes #1525 — browser hangs on deeply recursive OpenAPI specs (e.g. Komga's ~17K-node search-filter schema) after upgrading to v5.1.0.
Root cause: Three O(subtree) recursive walkers added between v4.7.1 and v5.1.0 ran on every
SchemaNoderender, turning O(N) total work into O(N²):findDiscriminator(fix(theme): handle nested discriminators in allOf structures #1303) — searched entire subtree per SchemaNode; wasschema.discriminator(O(1)) in 4.7.1findProperty(fix(theme): handle nested discriminators in allOf structures #1303) — searched entire subtree per DiscriminatorNode; wasschema.properties?.[name](O(1)) in 4.7.1stripConflictingAdditionalProps(fix: render allOf schemas with additionalProperties:false on every member (#1119) #1463) — deep-walked every value insidemergeAllOf, compounding withfoldSiblingsIntoBranches(fix: dedupe properties when allOf override redefines nested array items (#1218) #1457)Fix:
Schema/normalize.tswithnormalizeSchema()— single O(N) pass viauseMemothat merges allOf, strips conflicting additionalProperties, and folds siblings into oneOf/anyOf branchesschema.discriminatorcheck (normalization promotes discriminators from allOf members to top level)schema.properties?.[name]lookupstripConflictingAdditionalPropsincreateSchema.tsTest plan
gotson/komga-website@docusaurus_3.10) — both previously-hanging pages ("List series", "List series groups") now render within ~3 secondsisCircularMarkerguard clauses handle circular markers in property, items, additionalProperties, and SchemaEdge positionscircularRef.yaml(6 endpoints: self-ref, mutual recursion, additionalProperties, allOf composition, indirect recursion) andrecursiveFilter.yaml(Komga-shaped discriminated recursive filter)🤖 Generated with Claude Code