fix: use hack way to reuse builtin nav #231
Conversation
🦋 Changeset detectedLatest commit: 61c0544 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 |
|
Warning Rate limit exceeded@JounQin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors theme class names to rp- BEM, removes several VersionsNav subcomponents, adds a ForceRenderContext and new Link component with PDF/download handling, adjusts PDF path generation to be root-relative, and consolidates version navigation construction into VersionsNav. Changes
Sequence Diagram(s)mermaid Note over Layout,Context: Initialization Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Signed-off-by: JounQin <admin@1stg.me>
commit: |
There was a problem hiding this comment.
Pull request overview
This PR refactors the navigation system to reuse the built-in navigation components instead of maintaining custom implementations. The changes eliminate custom navigation components (NavMenuGroup, NavMenuSingleItem, SvgWrapper, Down) and replace them with a hack that manipulates the configuration navigation array directly.
Key changes:
- Removes custom navigation menu components and replaces them with direct manipulation of the configuration nav array
- Introduces a ForceRenderContext to trigger re-renders when navigation items change
- Creates a custom Link component wrapper to handle PDF downloads and absolute URLs
- Updates CSS class names from custom prefixes to built-in rspress classes
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/doom/styles/link.module.scss | Removes custom link styles that are now handled by built-in classes |
| packages/doom/src/theme/index.ts | Exports the new Link component wrapper |
| packages/doom/src/theme/VersionsNav/index.tsx | Refactored to manipulate nav array directly instead of rendering custom components |
| packages/doom/src/theme/VersionsNav/context.tsx | Adds ForceRenderContext for triggering nav re-renders |
| packages/doom/src/theme/VersionsNav/SvgWrapper.tsx | Deleted - no longer needed with built-in nav |
| packages/doom/src/theme/VersionsNav/NavMenuSingleItem.tsx | Deleted - replaced by built-in nav components |
| packages/doom/src/theme/VersionsNav/NavMenuGroup.tsx | Deleted - replaced by built-in nav components |
| packages/doom/src/theme/VersionsNav/Down.tsx | Deleted - no longer needed |
| packages/doom/src/theme/Link.tsx | New wrapper component for Link with PDF download handling |
| packages/doom/src/theme/Layout.tsx | Integrates ForceRenderContext and updates link rendering |
| packages/doom/src/shared/helpers.ts | Adds leading slash to PDF filename |
| packages/doom/src/runtime/components/ExternalSiteLink.tsx | Updates CSS class from custom to built-in |
| packages/doom/src/runtime/components/Directive.tsx | Updates CSS classes from custom to built-in |
| .changeset/twelve-spiders-listen.md | Adds changeset entry |
💡 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: 2
🧹 Nitpick comments (3)
packages/doom/src/theme/VersionsNav/context.tsx (1)
1-8: Consider adding null check in useForceRender hook.The
usehook is a React 19 feature for reading context. The context is initialized withnull!, which will throw at runtime ifuseForceRenderis called without a provider.🔎 Suggested defensive implementation
-export const useForceRender = () => use(ForceRenderContext).setValue +export const useForceRender = () => { + const context = use(ForceRenderContext) + if (!context) { + throw new Error('useForceRender must be used within a ForceRenderContext provider') + } + return context.setValue +}packages/doom/src/theme/Layout.tsx (1)
132-136: Consider using getPdfName helper for consistency.The pdfLink construction duplicates the logic from
getPdfNameinshared/helpers.ts. Using the helper would ensure consistency and reduce duplication.🔎 Suggested refactor
Import the helper at the top:
+import { getPdfName } from '../shared/helpers.ts'Then refactor the pdfLink computation:
const pdfLink = useMemo( () => - found && `/${found.exportItem.name ?? found.sidebar.text}-${lang}.pdf`, + found && getPdfName(lang, found.exportItem.name, found.sidebar.text), [found, lang], )Note: Verify that
getPdfNamesignature matches the expected arguments (the second parameter isuserBasein the helper).packages/doom/src/theme/VersionsNav/index.tsx (1)
137-142: Mutating shared state directly is fragile.This pattern modifies
configNavin place, which may affect the source object fromuseLocaleSiteData(). Since this is acknowledged as a "hack" (line 50 comment, PR title), consider:
- Documenting why this approach was chosen over alternatives.
- Adding a comment explaining when this hack can be removed (e.g., when rspress exposes a proper API).
The mutation could cause subtle bugs if other components also reference the same nav array.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.changeset/twelve-spiders-listen.mdpackages/doom/src/runtime/components/Directive.tsxpackages/doom/src/runtime/components/ExternalSiteLink.tsxpackages/doom/src/shared/helpers.tspackages/doom/src/theme/Layout.tsxpackages/doom/src/theme/Link.tsxpackages/doom/src/theme/VersionsNav/Down.tsxpackages/doom/src/theme/VersionsNav/NavMenuGroup.tsxpackages/doom/src/theme/VersionsNav/NavMenuSingleItem.tsxpackages/doom/src/theme/VersionsNav/SvgWrapper.tsxpackages/doom/src/theme/VersionsNav/context.tsxpackages/doom/src/theme/VersionsNav/index.tsxpackages/doom/src/theme/index.tspackages/doom/styles/link.module.scss
💤 Files with no reviewable changes (5)
- packages/doom/src/theme/VersionsNav/Down.tsx
- packages/doom/src/theme/VersionsNav/NavMenuSingleItem.tsx
- packages/doom/src/theme/VersionsNav/NavMenuGroup.tsx
- packages/doom/styles/link.module.scss
- packages/doom/src/theme/VersionsNav/SvgWrapper.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx,json,yml,yaml,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier according to the project's Prettier configuration
Files:
packages/doom/src/theme/index.tspackages/doom/src/theme/Link.tsxpackages/doom/src/theme/VersionsNav/context.tsxpackages/doom/src/runtime/components/Directive.tsxpackages/doom/src/theme/VersionsNav/index.tsxpackages/doom/src/shared/helpers.tspackages/doom/src/theme/Layout.tsxpackages/doom/src/runtime/components/ExternalSiteLink.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode for type safety
Files:
packages/doom/src/theme/index.tspackages/doom/src/theme/Link.tsxpackages/doom/src/theme/VersionsNav/context.tsxpackages/doom/src/runtime/components/Directive.tsxpackages/doom/src/theme/VersionsNav/index.tsxpackages/doom/src/shared/helpers.tspackages/doom/src/theme/Layout.tsxpackages/doom/src/runtime/components/ExternalSiteLink.tsx
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Follow ESLint rules defined in eslint.config.js
Files:
packages/doom/src/theme/index.tspackages/doom/src/theme/Link.tsxpackages/doom/src/theme/VersionsNav/context.tsxpackages/doom/src/runtime/components/Directive.tsxpackages/doom/src/theme/VersionsNav/index.tsxpackages/doom/src/shared/helpers.tspackages/doom/src/theme/Layout.tsxpackages/doom/src/runtime/components/ExternalSiteLink.tsx
🧠 Learnings (4)
📚 Learning: 2025-05-26T08:59:41.491Z
Learnt from: JounQin
Repo: alauda/doom PR: 30
File: src/runtime/components/K8sCrd.tsx:5-5
Timestamp: 2025-05-26T08:59:41.491Z
Learning: In rspress/core v2.0.0-beta.7, the '/theme' export is available in the package exports field as `"./theme": { "default": "./theme.ts" }`, so imports like `import { Badge, Button } from 'rspress/core/theme'` are valid even if ESLint shows resolution errors.
Applied to files:
packages/doom/src/theme/index.tspackages/doom/src/theme/VersionsNav/index.tsx
📚 Learning: 2025-05-26T09:09:21.339Z
Learnt from: JounQin
Repo: alauda/doom PR: 30
File: src/runtime/components/K8sCrd.tsx:5-5
Timestamp: 2025-05-26T09:09:21.339Z
Learning: The alauda/doom project uses yarn v4 for dependency management, which handles package exports and module resolution correctly, so imports like `rspress/core/theme` work without any ESLint errors.
Applied to files:
packages/doom/src/theme/index.tspackages/doom/src/theme/VersionsNav/index.tsx.changeset/twelve-spiders-listen.md
📚 Learning: 2025-05-29T16:25:28.086Z
Learnt from: JounQin
Repo: alauda/doom PR: 40
File: src/plugins/sitemap/index.ts:7-7
Timestamp: 2025-05-29T16:25:28.086Z
Learning: In rspress/shared v2.0.0-beta.8, the '/logger' export is available in the package exports field, so imports like `import { logger } from 'rspress/shared/logger'` are valid even if ESLint shows resolution errors. This is used throughout the codebase in files like src/cli/translate.ts, src/cli/load-config.ts, src/utils/git.ts, and src/plugins/sitemap/index.ts.
Applied to files:
packages/doom/src/theme/index.tspackages/doom/src/theme/VersionsNav/index.tsx
📚 Learning: 2025-06-09T03:10:41.010Z
Learnt from: JounQin
Repo: alauda/doom PR: 75
File: src/cli/load-config.ts:4-7
Timestamp: 2025-06-09T03:10:41.010Z
Learning: The alauda/doom project uses yarn v4 as the package manager, not npm. Always reference yarn commands when suggesting package management operations.
Applied to files:
.changeset/twelve-spiders-listen.md
🧬 Code graph analysis (1)
packages/doom/src/theme/Layout.tsx (5)
packages/doom/src/theme/VersionsNav/context.tsx (1)
ForceRenderContext(3-6)packages/doom/src/theme/VersionsNav/index.tsx (1)
VersionsNav(147-151)packages/doom/src/runtime/components/_X.tsx (1)
X(3-3)packages/doom/src/theme/Link.tsx (1)
Link(6-19)packages/doom/src/theme/index.ts (1)
Link(6-6)
🪛 GitHub Actions: CI
packages/doom/src/theme/Link.tsx
[error] 16-16: "typecov" failed: type coverage rate 99.99% is lower than the target 100%.
🔇 Additional comments (13)
.changeset/twelve-spiders-listen.md (1)
1-5: LGTM!The changeset format is correct for a patch-level fix. The description aligns with the PR's navigation refactoring objectives.
packages/doom/src/runtime/components/ExternalSiteLink.tsx (1)
78-78: LGTM!The migration from CSS module class to the global
rp-linkclass is consistent with the broader theme refactoring in this PR.packages/doom/src/runtime/components/Directive.tsx (1)
22-44: LGTM!The CSS class refactoring follows a clean BEM convention (
rp-callout,rp-callout__*,rp-callout--*), improving consistency and maintainability.packages/doom/src/theme/index.ts (1)
6-6: LGTM!The new
Linkexport extends the theme's public API surface appropriately.packages/doom/src/theme/Layout.tsx (3)
138-141: LGTM!The force-render mechanism is well-implemented using a boolean toggle state pattern.
144-160: LGTM!The
ForceRenderContextprovider is correctly initialized with a memoized value object, and the render order (VersionsNav before OriginalLayout) allows navigation updates to trigger layout re-renders.
151-159: Unable to proceed: No review comment has been provided. Please provide the review comment that needs to be rewritten within<review_comment>tags, along with any relevant context such as the file path, code snippet, or codebase details needed for verification.packages/doom/src/shared/helpers.ts (1)
14-15: Verify that all consumers ofgetPdfNameexpect a root-relative path with leading slash.The function now returns paths with a leading slash (e.g.,
/-${lang}.pdf). Check that all call sites are compatible with this format, particularly anywhere the result is used for URL construction or link generation.packages/doom/src/theme/Link.tsx (2)
7-12: [Your rewritten review comment text here]
[Exactly ONE classification tag]
14-17: Remove suggested fix; the download attribute does not accept boolean values.The HTML
downloadattribute is string-based, not boolean. Settingdownload="true"ordownload="false"treats the string as a suggested filename, not as a download flag. The TypeScript DOM type isstring | undefined, notboolean | undefined.The type coverage issue likely stems from passing boolean values to a string-typed property. To trigger download behavior, use an empty string:
download={props.href?.endsWith('.pdf') ? "" : undefined}ordownload={props.href?.endsWith('.pdf') ? "filename" : undefined}.The suggested fix with
|| undefinedwill not resolve the type issue because it still passes a booleantrueto a string property.Likely an incorrect or invalid review comment.
packages/doom/src/theme/VersionsNav/index.tsx (3)
1-23: LGTM!Imports are correctly structured. Based on learnings, the rspress package exports are valid even if ESLint shows resolution errors.
25-36: LGTM!Constants are well-structured. The conditional localhost addition for non-production environments is appropriate for development workflows.
144-153: LGTM!Returning
nullafter config mutation and wrapping withNoSSRis appropriate since this component's purpose is side-effect-based (injecting nav items) rather than rendering UI directly.
JounQin
force-pushed
the
fix/versions_nav
branch
from
0a45e44 to
abd356c
Compare
JounQin
force-pushed
the
fix/versions_nav
branch
from
abd356c to
61c0544
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.