fix: unstable slug due to children changes #58
Conversation
🦋 Changeset detectedLatest commit: 500fb27 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 or files that can be reviewed per hour. Please wait 14 minutes and 14 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 ignored due to path filters (1)
📒 Files selected for processing (15)
WalkthroughA changeset file was added to document a patch update for the Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant HeadingTitle
ParentComponent->>HeadingTitle: Render with children
HeadingTitle->>HeadingTitle: useMemo to extract slug from children
HeadingTitle->>HeadingTitle: useMemo to compute id using slug
HeadingTitle-->>ParentComponent: Rendered heading with stable id
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an unstable slug issue by refactoring the memoization of the slug used to generate element IDs in the HeadingTitle component.
- Introduces a memoized value for slug generated from children content.
- Updates the dependency arrays on useMemo hooks to rely on memoized children slug.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/runtime/components/_HeadingTitle.tsx | Refactors slug generation logic using memoized children content. |
| .changeset/heavy-crews-glow.md | Adds a changeset entry documenting the slug fix. |
commit: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
JounQin
force-pushed
the
fix/unstable_slug
branch
from
89205c8 to
6e5333b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.changeset/heavy-crews-glow.md(1 hunks)eslint.config.js(2 hunks)package.json(1 hunks)src/global/SiteOverrides/index.tsx(1 hunks)src/global/VersionsNav/index.tsx(1 hunks)src/runtime/components/ExternalSiteLink.tsx(1 hunks)src/runtime/components/K8sCrd.tsx(4 hunks)src/runtime/components/K8sPermissionTable.tsx(1 hunks)src/runtime/components/Mermaid.tsx(1 hunks)src/runtime/components/OpenAPIPath.tsx(1 hunks)src/runtime/components/OpenAPIRef.tsx(2 hunks)src/runtime/components/Overview.tsx(4 hunks)src/runtime/components/_ExternalSiteBase.tsx(1 hunks)src/runtime/components/_HeadingTitle.tsx(1 hunks)src/runtime/hooks/useIsPrint.ts(1 hunks)src/runtime/hooks/useSiteOverrides.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/runtime/components/_ExternalSiteBase.tsx (1)
src/runtime/hooks/useTranslation.ts (1)
useLang(7-7)
🪛 ESLint
src/runtime/components/Overview.tsx
[error] 14-14: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
🔇 Additional comments (28)
src/runtime/hooks/useSiteOverrides.ts (1)
128-128: LGTM! Correctly fixes the dependency array.The useEffect now properly depends on
acpSiteandsiteData.base, which are both used in thefetchSiteOverridescall. This ensures the effect re-runs when these values change, maintaining correct reactivity.src/runtime/hooks/useIsPrint.ts (1)
35-35: LGTM! Correctly includes all effect dependencies.Adding
onChangeto the dependency array is correct since the effect uses it for event listener registration. This ensures proper cleanup and re-registration if the callback reference changes.src/global/SiteOverrides/index.tsx (1)
74-81: LGTM! Comprehensive dependency array update.The expanded dependency array correctly includes all variables used in the effect's logic for computing and setting the document title. This ensures the title updates reactively when any relevant page data changes.
src/runtime/components/_HeadingTitle.tsx (2)
26-32: Excellent refactor! Centralizes slug computation and fixes instability.Extracting the string content extraction from
childreninto a separateuseMemohook is a smart optimization. This ensures the slug only recalculates when the actual string content changes, not when thechildrenarray reference changes but contains the same content.
35-38: LGTM! Proper use of memoized value and correct dependencies.Using
slugFromChildreninstead of computing inline eliminates redundant computation and improves stability. The dependency array correctly includes all referenced values:uid,slug,slugger, andslugFromChildren..changeset/heavy-crews-glow.md (1)
1-6: Changeset properly documents the fix.The changeset follows the correct format and accurately describes the patch fix for unstable slug issues.
src/runtime/components/Mermaid.tsx (1)
35-35: Correct dependency array fix.Adding
idto the dependency array is necessary since it's used within the effect to generate the unique mermaid identifier. This ensures the diagram re-renders when the ID changes.src/global/VersionsNav/index.tsx (1)
109-109: Proper dependency array fix for version fetching.Adding
versionandversionsBaseto the dependency array is correct since both values are used within the effect. This ensures versions are re-fetched when these inputs change.package.json (1)
121-121:Details
✅ Verification successful
Good addition of React hooks ESLint plugin.
Adding
eslint-plugin-react-hooksis appropriate given the dependency array fixes in this PR. This will help prevent similar React hooks issues in the future.Verify this is the latest stable version:
🌐 Web query:
What is the latest stable version of eslint-plugin-react-hooks?💡 Result:
The latest stable version of
eslint-plugin-react-hooksas of June 4, 2025, is 5.2.0. This version was published on February 28, 2025[1][6]. While there is a 6.0.0 release candidate (RC) available, version 5.2.0 is the most recent stable release[6][7].Citations:
- 1: https://www.npmjs.com/package/eslint-plugin-react-hooks
- 2: https://github.com/facebook/react/blob/master/packages/eslint-plugin-react-hooks/CHANGELOG.md
- 3: https://classic.yarnpkg.com/en/package/eslint-plugin-react-hooks
- 4: https://forum.devtalk.com/t/react-eslint-plugin-react-hooks-5-0-0-released/184262
- 5: https://react.dev/blog/2025/04/21/react-compiler-rc
- 6: https://security.snyk.io/package/npm/eslint-plugin-react-hooks
- 7: https://react.dev/learn/react-compiler
- 8: https://classic.yarnpkg.com/en/package/eslint-plugin-react
- 9: https://biomejs.dev/linter/rules-sources/
- 10: https://version.cs.vt.edu/atink/Brave-Souls/-/tree/loginpage/client/node_modules/eslint-plugin-react
✔️ Approved: eslint-plugin-react-hooks v5.2.0 is up-to-date
The addition of
eslint-plugin-react-hooks(v5.2.0) aligns perfectly with the dependency-array fixes in this PR. Version 5.2.0 is confirmed as the latest stable release as of June 4, 2025, so no further changes are needed.
- package.json (line 121)
src/runtime/components/OpenAPIPath.tsx (1)
200-200: Excellent dependency array fix!Adding
[openapiPath_, page.routePath, path]to the dependency array ensures the memoized OpenAPI path data recalculates correctly when these inputs change. This fixes potential stale closure issues where the component might not update when props change.src/runtime/components/ExternalSiteLink.tsx (1)
34-37: Correct dependency array fix!Adding
[name]to the dependency array ensures thesitelookup recalculates when thenameprop changes. This prevents stale closures where the site would remain the same even if a different name prop was passed.src/runtime/components/_ExternalSiteBase.tsx (2)
97-100: Correct dependency array fix for site lookup.Adding
[name]ensures the site lookup recalculates when the name prop changes, preventing stale memoization.
106-106: Comprehensive dependency array for displayName.The dependency array
[lang, name, site?.displayName]correctly captures all inputs that affect the displayName computation, ensuring proper reactivity when language, name, or site display names change.eslint.config.js (2)
5-6: LGTM: Proper React hooks plugin integrationThe import changes correctly switch to named imports and add React hooks support. This aligns well with the PR's focus on improving React hooks usage patterns.
16-16: LGTM: React hooks linting configurationAdding the recommended React hooks configuration will help enforce best practices for hooks usage throughout the codebase, which is beneficial given the memoization improvements in this PR.
src/runtime/components/K8sCrd.tsx (3)
75-86: LGTM: Improved JSX formatting for consistent spacingThe explicit spacing fragments ensure consistent visual spacing between elements, improving the component's rendering reliability.
184-184: LGTM: Critical dependency array fixThe useMemo dependency array was previously empty but should include
crdPathandnamesince the memoized computation depends on these props. This fix ensures the component properly recalculates when these props change, which aligns with the PR's goal of fixing unstable behavior.
195-195: LGTM: Consistent formatting improvementThe formatting change maintains consistency with the pattern established in the K8sCrdSchemaPart component.
src/runtime/components/OpenAPIRef.tsx (3)
176-176: LGTM: Essential dependency array correctionThe useMemo dependency array was previously empty but should include
openapiPath_andschemasince the memoized computation searches for schemas based on these values. This fix ensures proper recalculation when props change.
179-179: LGTM: Improved refs calculation logicAdding the
openapicheck ensures refs are only calculated when the OpenAPI document is available, preventing potential errors.
188-191: LGTM: Better error handling flowMoving the error handling after the refs calculation ensures all memoized values are computed before early returns, improving the component's logic flow.
src/runtime/components/Overview.tsx (7)
14-14: LGTM: Necessary React hooks importsThe addition of
useCallbackanduseMemoimports supports the performance optimizations in this component.Note: The static analysis error about unresolvable React import appears to be a false positive since React hooks are being used successfully throughout the file.
🧰 Tools
🪛 ESLint
[error] 14-14: Unable to resolve path to module 'react'.
(import-x/no-unresolved)
32-42: LGTM: Function extraction for performanceMoving
getChildLinkoutside the component prevents it from being recreated on every render, which is a good performance optimization since it's a pure function.
60-67: LGTM: Proper memoization of filter functionThe
subFilterfunction is correctly memoized withroutePathas a dependency, preventing unnecessary re-computations when other props change.
71-74: LGTM: Memoized modules computationThe
overviewModulescomputation is properly memoized with correct dependencies (pagesandsubFilter), improving performance by avoiding redundant filtering operations.
89-126: LGTM: Complex function properly memoizedThe
normalizeSidebarItemfunction is correctly memoized with all relevant dependencies (overviewModules,props.overviewHeaders,routePath). This prevents expensive re-computations while ensuring correctness when dependencies change.
133-184: LGTM: Well-structured memoized group processingThe
getGroupfunction is properly memoized with correct dependencies (frontmatter,normalizeSidebarItem,subFilter). The complex group processing logic is preserved while gaining performance benefits.
186-201: LGTM: Optimized default groups computationThe
defaultGroupsmemoization with fallback logic for nested sidebar groups is well-implemented. The dependencies (getGroup,overviewSidebarGroups) correctly trigger recalculation when needed.
JounQin
force-pushed
the
fix/unstable_slug
branch
from
6e5333b to
500fb27
Compare
Summary by CodeRabbit