fix: should only consider stable original config nav length#297
fix: should only consider stable original config nav length#297
Conversation
🦋 Changeset detectedLatest commit: 1db824b 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
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 47 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTagged dynamically injected nav entries with an internal Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 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: |
There was a problem hiding this comment.
Pull request overview
Adjusts VersionsNav’s “reset to original nav” behavior by tracking an initial nav length and using it when mutating configNav, aiming to only consider the stable original config nav length.
Changes:
- Replaces
originalConfigNavsnapshot with a persistedoriginLengthvalue. - Updates the effect that mutates
configNavto reset its length usingoriginLengthand updates the dependency list accordingly.
💡 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.
🧹 Nitpick comments (1)
packages/doom/src/theme/VersionsNav/index.tsx (1)
48-48: Potential stale baseline length whennavsource changes
originLengthat Line 48 is frozen from first render. Ifnavlater changes identity/length (e.g., locale/context switch without remount), Line 141 can apply an outdated length and produce incorrect truncation/expansion before appendingnavList.Proposed adjustment (track baseline per
configNavinstance)-import { useEffect, useMemo, useState } from 'react' +import { useEffect, useMemo, useRef, useState } from 'react' @@ - const [originLength] = useState(configNav.length) + const originLengthRef = useRef(configNav.length) + + useEffect(() => { + originLengthRef.current = configNav.length + }, [configNav]) @@ - configNav.length = originLength + configNav.length = originLengthRef.current configNav.push(...navList) forceRender() - }, [configNav, forceRender, navList, originLength]) + }, [configNav, forceRender, navList])Also applies to: 141-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/theme/VersionsNav/index.tsx` at line 48, originLength is captured once via useState and can become stale when configNav/nav changes; update the baseline whenever configNav identity changes by deriving originLength from configNav (e.g., compute originLength = configNav.length with useMemo or update the state inside a useEffect that listens to configNav) so that the truncation/expansion logic that uses originLength (see originLength, configNav, nav, navList) always uses the current baseline and avoids incorrect slicing at the block around lines referencing originLength (the truncation/append logic).
🤖 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/theme/VersionsNav/index.tsx`:
- Line 48: originLength is captured once via useState and can become stale when
configNav/nav changes; update the baseline whenever configNav identity changes
by deriving originLength from configNav (e.g., compute originLength =
configNav.length with useMemo or update the state inside a useEffect that
listens to configNav) so that the truncation/expansion logic that uses
originLength (see originLength, configNav, nav, navList) always uses the current
baseline and avoids incorrect slicing at the block around lines referencing
originLength (the truncation/append logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f30b118-9da3-450b-b3d0-f0d739dc1997
📒 Files selected for processing (1)
packages/doom/src/theme/VersionsNav/index.tsx
JounQin
force-pushed
the
fix/unstable_nav
branch
from
ee3a91b to
93cd8e7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: JounQin <admin@1stg.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit