feat: deduplicate shared refs with a single component#292
Open
feat: deduplicate shared refs with a single component#292
Conversation
🦋 Changeset detectedLatest commit: f1b7818 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR consolidates multiple OpenAPIPath usages in docs into array-based paths, adds a remark-lint rule to forbid multiple OpenAPIPath components per file, refactors OpenAPIPath to accept multiple paths and render per-path bases, and removes UID context/uid-based anchor logic from several runtime components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenAPI MDX runtime rendering to deduplicate shared schema references when documenting multiple API paths, and enforces the “single OpenAPIPath per file” authoring pattern (IDP-1446).
Changes:
- Remove the
UidContext-based unique ID scheme and switch refs/headings to use stable schema-based anchors. - Extend
OpenAPIPathto acceptpath: string | string[]and collect/deduplicate referenced schemas across the provided paths. - Add a remark-lint rule to prevent multiple
OpenAPIPathcomponents in one MDX file, and update impacted docs to the new array form.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/doom/src/runtime/components/_context.ts | Removes UID context/provider used for unique anchor generation. |
| packages/doom/src/runtime/components/_RefLink.tsx | Simplifies ref links to target stable schema anchors (or COMMON_REFS). |
| packages/doom/src/runtime/components/_HeadingTitle.tsx | Adjusts slug extraction/id creation for headings (no UID prefix). |
| packages/doom/src/runtime/components/OpenAPIRef.tsx | Removes UID provider usage and the isCommonRef behavior. |
| packages/doom/src/runtime/components/OpenAPIPath.tsx | Adds multi-path support and ref collection/dedup across paths. |
| packages/doom/src/remarkrc.ts | Enables the new noMultiOpenAPIPaths lint rule. |
| packages/doom/src/remark-lint/site.ts | Renames the rule id to match the doom-lint:* naming scheme. |
| packages/doom/src/remark-lint/no-multi-open-api-paths.ts | Adds lint rule to enforce single OpenAPIPath usage per file. |
| packages/doom/src/remark-lint/index.ts | Exports the new lint rule. |
| docs/zh/apis/advanced_apis/log/search.mdx | Combines multiple OpenAPIPath usages into one with path={[...]}. |
| docs/zh/apis/advanced_apis/log/aggregation.mdx | Same as above. |
| docs/zh/apis/advanced_apis/gitops/applicationsets.mdx | Same as above (larger path list). |
| docs/zh/apis/advanced_apis/event/search.mdx | Same as above. |
| docs/en/apis/advanced_apis/log/search.mdx | Same as above (+ sourceSHA update). |
| docs/en/apis/advanced_apis/log/aggregation.mdx | Same as above (+ sourceSHA update). |
| docs/en/apis/advanced_apis/gitops/applicationsets.mdx | Same as above (+ sourceSHA update). |
| docs/en/apis/advanced_apis/event/search.mdx | Same as above (+ sourceSHA update). |
Comments suppressed due to low confidence (1)
packages/doom/src/runtime/components/_HeadingTitle.tsx:42
HeadingTitlecan computeidasundefined(e.g., whenslugis absent andslugger.slug(...)returns an empty string). In that case the anchor still renders withhref="#undefined", producing a broken/self-invented anchor. Consider only rendering the anchor whenidis truthy (or providing a fallback id).
<HeadingComponent id={id}>
<X.a className="rp-header-anchor" href={`#${id}`} aria-hidden>
#
</X.a>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
packages/doom/src/runtime/components/_HeadingTitle.tsx (1)
32-36: Simplified id construction aligns with single-component deduplication strategy.With multiple paths now consolidated into a single
OpenAPIPathcomponent (which creates oneBananaSluginstance), the slugger's internal deduplication handles collisions by appending-1,-2, etc. for duplicate headings within the same component instance.The single-element array pattern could be simplified to a direct expression, but this is a minor style preference:
♻️ Optional: Simplify the id computation
const id = useMemo( () => - [slug || slugger?.slug(slugFromChildren)].filter(Boolean).join('-') || - undefined, + slug || slugger?.slug(slugFromChildren) || undefined, [slug, slugger, slugFromChildren], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/runtime/components/_HeadingTitle.tsx` around lines 32 - 36, The id useMemo currently builds an array and filters to join which is unnecessary given the single-component deduplication; simplify the expression in the useMemo that computes id (referencing useMemo, id, slug, slugger, slugFromChildren) to directly choose slug || slugger?.slug(slugFromChildren) and return undefined when falsy, removing the intermediate array/filter/join pattern so the logic remains identical but clearer and relies on BananaSlug/OpenAPIPath slugger deduplication.packages/doom/src/runtime/components/OpenAPIPath.tsx (2)
174-182: Consider keepingOpenAPIPathBasePropsinternal.The
OpenAPIPathBasecomponent is not exported, so its props interface doesn't need to be exported either. This reduces the public API surface.♻️ Suggested change
-export interface OpenAPIPathBaseProps extends Omit< +interface OpenAPIPathBaseProps extends Omit< OpenAPIPathProps, 'openapiPath' > {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/runtime/components/OpenAPIPath.tsx` around lines 174 - 182, OpenAPIPathBaseProps is unnecessarily exported despite OpenAPIPathBase being internal; change the declaration of OpenAPIPathBaseProps from "export interface OpenAPIPathBaseProps" to a non-exported "interface OpenAPIPathBaseProps" and update any internal references in the same file (e.g., usages in OpenAPIPathBase, OpenAPIPath, or related helper types) to ensure they still resolve, removing it from the module's public API surface.
307-307: Variable shadowing: innerpathshadows the prop.The loop variable
pathshadows the destructured proppathfrom the function parameters. While functionally correct, this can be confusing and error-prone during future maintenance.♻️ Suggested change
- for (const path of paths) { - const pathItem = openapi.paths?.[path] + for (const pathKey of paths) { + const pathItem = openapi.paths?.[pathKey] if (pathItem) { - pathMap.set(path, { + pathMap.set(pathKey, { pathItem, openapi, }) if (!refMap.has(pathname)) { refMap.set(pathname, new Set()) } const refs = refMap.get(pathname)! - setRefsForPath(openapi, path, omitRoutePathRefs(page.routePath), refs) + setRefsForPath(openapi, pathKey, omitRoutePathRefs(page.routePath), refs) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/runtime/components/OpenAPIPath.tsx` at line 307, The loop variable `path` inside the for-loop shadows the component prop `path` in OpenAPIPath, which is confusing; rename the loop variable (for example to `apiPath`, `pathItem`, or `p`) in the loop `for (const ... of paths)` and update all references inside that loop body to the new name so the destructured prop `path` remains unshadowed (check usages within the loop and any nested callbacks in the OpenAPIPath component).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/doom/src/runtime/components/OpenAPIPath.tsx`:
- Line 326: The slugger instance is memoized with an empty dependency array so
BananaSlug (which tracks generated slugs) can retain state when the component's
path prop changes; update the useMemo call that creates slugger to include path
in its dependency array (i.e., recreate BananaSlug when path changes) so heading
IDs are regenerated without stale duplicate-count state—locate the useMemo
creating slugger and add path to its dependencies.
---
Nitpick comments:
In `@packages/doom/src/runtime/components/_HeadingTitle.tsx`:
- Around line 32-36: The id useMemo currently builds an array and filters to
join which is unnecessary given the single-component deduplication; simplify the
expression in the useMemo that computes id (referencing useMemo, id, slug,
slugger, slugFromChildren) to directly choose slug ||
slugger?.slug(slugFromChildren) and return undefined when falsy, removing the
intermediate array/filter/join pattern so the logic remains identical but
clearer and relies on BananaSlug/OpenAPIPath slugger deduplication.
In `@packages/doom/src/runtime/components/OpenAPIPath.tsx`:
- Around line 174-182: OpenAPIPathBaseProps is unnecessarily exported despite
OpenAPIPathBase being internal; change the declaration of OpenAPIPathBaseProps
from "export interface OpenAPIPathBaseProps" to a non-exported "interface
OpenAPIPathBaseProps" and update any internal references in the same file (e.g.,
usages in OpenAPIPathBase, OpenAPIPath, or related helper types) to ensure they
still resolve, removing it from the module's public API surface.
- Line 307: The loop variable `path` inside the for-loop shadows the component
prop `path` in OpenAPIPath, which is confusing; rename the loop variable (for
example to `apiPath`, `pathItem`, or `p`) in the loop `for (const ... of paths)`
and update all references inside that loop body to the new name so the
destructured prop `path` remains unshadowed (check usages within the loop and
any nested callbacks in the OpenAPIPath component).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bee29cf-ecfb-47a9-be8f-3a5bc7ad1bb5
📒 Files selected for processing (18)
.changeset/forty-brooms-pull.mddocs/en/apis/advanced_apis/event/search.mdxdocs/en/apis/advanced_apis/gitops/applicationsets.mdxdocs/en/apis/advanced_apis/log/aggregation.mdxdocs/en/apis/advanced_apis/log/search.mdxdocs/zh/apis/advanced_apis/event/search.mdxdocs/zh/apis/advanced_apis/gitops/applicationsets.mdxdocs/zh/apis/advanced_apis/log/aggregation.mdxdocs/zh/apis/advanced_apis/log/search.mdxpackages/doom/src/remark-lint/index.tspackages/doom/src/remark-lint/no-multi-open-api-paths.tspackages/doom/src/remark-lint/site.tspackages/doom/src/remarkrc.tspackages/doom/src/runtime/components/OpenAPIPath.tsxpackages/doom/src/runtime/components/OpenAPIRef.tsxpackages/doom/src/runtime/components/_HeadingTitle.tsxpackages/doom/src/runtime/components/_RefLink.tsxpackages/doom/src/runtime/components/_context.ts
💤 Files with no reviewable changes (1)
- packages/doom/src/runtime/components/_context.ts
| return { paths, pathMap, refMap } | ||
| }, [openapiPath, page.routePath, path]) | ||
|
|
||
| const slugger = useMemo(() => new BananaSlug(), []) |
There was a problem hiding this comment.
Slugger may retain stale state when path changes.
The slugger is memoized with an empty dependency array, so it persists across prop changes. BananaSlug tracks previously generated slugs to avoid duplicates. If path changes, the slugger retains its history, which could cause heading IDs to have unexpected numeric suffixes (e.g., heading-2 instead of heading).
Consider including path in the dependency array to reset the slugger when paths change:
🐛 Suggested fix
- const slugger = useMemo(() => new BananaSlug(), [])
+ const slugger = useMemo(() => new BananaSlug(), [path])📝 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.
Suggested change
| const slugger = useMemo(() => new BananaSlug(), []) | |
| const slugger = useMemo(() => new BananaSlug(), [path]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/doom/src/runtime/components/OpenAPIPath.tsx` at line 326, The
slugger instance is memoized with an empty dependency array so BananaSlug (which
tracks generated slugs) can retain state when the component's path prop changes;
update the useMemo call that creates slugger to include path in its dependency
array (i.e., recreate BananaSlug when path changes) so heading IDs are
regenerated without stale duplicate-count state—locate the useMemo creating
slugger and add path to its dependencies.
close IDP-1446
JounQin
force-pushed
the
feat/duplicate_refs
branch
from
231e9ad to
f1b7818
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/doom/src/runtime/components/OpenAPIPath.tsx (1)
307-307: Consider renaming loop variables to avoid shadowing thepathprop.The loop variable
pathshadows the component'spathprop in multiple places, which can reduce code clarity and cause confusion during maintenance.♻️ Suggested refactor
- for (const path of paths) { - const pathItem = openapi.paths?.[path] + for (const p of paths) { + const pathItem = openapi.paths?.[p] if (pathItem) { - pathMap.set(path, { + pathMap.set(p, { pathItem, openapi, }) if (!refMap.has(pathname)) { refMap.set(pathname, new Set()) } const refs = refMap.get(pathname)! - setRefsForPath(openapi, path, omitRoutePathRefs(page.routePath), refs) + setRefsForPath(openapi, p, omitRoutePathRefs(page.routePath), refs) } }And similarly in the render section:
- {paths.map((path) => { - const p = pathMap.get(path) + {paths.map((pathKey) => { + const resolved = pathMap.get(pathKey) return ( <OpenAPIPathBase - key={path} - path={path} - pathItem={p?.pathItem} - openapi={p?.openapi} + key={pathKey} + path={pathKey} + pathItem={resolved?.pathItem} + openapi={resolved?.openapi} pathPrefix={pathPrefix} slugger={slugger} /> ) })}Also applies to: 330-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/runtime/components/OpenAPIPath.tsx` at line 307, The component OpenAPIPath currently uses a loop variable named "path" which shadows the component prop "path" (e.g., the loop beginning with "for (const path of paths)" and the loops in the render section around the subsequent iteration), causing ambiguity; rename the loop variable to a non-conflicting identifier such as "pathKey", "pathItem", or "route" throughout the file (including all references in the loop bodies and render JSX) to eliminate shadowing and preserve the prop "path" name unmodified in OpenAPIPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/doom/src/runtime/components/OpenAPIPath.tsx`:
- Line 307: The component OpenAPIPath currently uses a loop variable named
"path" which shadows the component prop "path" (e.g., the loop beginning with
"for (const path of paths)" and the loops in the render section around the
subsequent iteration), causing ambiguity; rename the loop variable to a
non-conflicting identifier such as "pathKey", "pathItem", or "route" throughout
the file (including all references in the loop bodies and render JSX) to
eliminate shadowing and preserve the prop "path" name unmodified in OpenAPIPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 108b5272-c0c8-4046-83a8-e2618c5df595
📒 Files selected for processing (18)
.changeset/forty-brooms-pull.mddocs/en/apis/advanced_apis/event/search.mdxdocs/en/apis/advanced_apis/gitops/applicationsets.mdxdocs/en/apis/advanced_apis/log/aggregation.mdxdocs/en/apis/advanced_apis/log/search.mdxdocs/zh/apis/advanced_apis/event/search.mdxdocs/zh/apis/advanced_apis/gitops/applicationsets.mdxdocs/zh/apis/advanced_apis/log/aggregation.mdxdocs/zh/apis/advanced_apis/log/search.mdxpackages/doom/src/remark-lint/index.tspackages/doom/src/remark-lint/no-multi-open-api-paths.tspackages/doom/src/remark-lint/site.tspackages/doom/src/remarkrc.tspackages/doom/src/runtime/components/OpenAPIPath.tsxpackages/doom/src/runtime/components/OpenAPIRef.tsxpackages/doom/src/runtime/components/_HeadingTitle.tsxpackages/doom/src/runtime/components/_RefLink.tsxpackages/doom/src/runtime/components/_context.ts
💤 Files with no reviewable changes (1)
- packages/doom/src/runtime/components/_context.ts
✅ Files skipped from review due to trivial changes (9)
- .changeset/forty-brooms-pull.md
- docs/zh/apis/advanced_apis/event/search.mdx
- docs/zh/apis/advanced_apis/log/aggregation.mdx
- packages/doom/src/remark-lint/index.ts
- docs/zh/apis/advanced_apis/log/search.mdx
- docs/en/apis/advanced_apis/log/search.mdx
- packages/doom/src/remarkrc.ts
- docs/en/apis/advanced_apis/log/aggregation.mdx
- docs/en/apis/advanced_apis/event/search.mdx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/doom/src/runtime/components/_HeadingTitle.tsx
- docs/zh/apis/advanced_apis/gitops/applicationsets.mdx
- packages/doom/src/remark-lint/site.ts
- docs/en/apis/advanced_apis/gitops/applicationsets.mdx
- packages/doom/src/runtime/components/OpenAPIRef.tsx
- packages/doom/src/remark-lint/no-multi-open-api-paths.ts
- packages/doom/src/runtime/components/_RefLink.tsx
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
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.
close IDP-1446
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores