-
Notifications
You must be signed in to change notification settings - Fork 577
[MISC] Fix Dashboard icon always appearing highlighted #1760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Changed dashboard icon fill color from white to #90A4B7 to fix the issue where the dashboard icon was always appearing highlighted in the sidebar. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughReplaces global Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
8e61f12 to
19ce339
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/navigations/side-nav-bar/SideNavBar.jsx (2)
113-125: Pass pathname intogetActiveSettingsKeyinstead of reading globallocation.
Line 114 explicitly referencesglobalThis.location.pathname, which directly accesses the browser's global location object and bypasses router state (breaking in SSR/tests). Use React Router'suseLocation()hook inSettingsPopoverContentand pass the pathname as a parameter instead.🐛 Proposed fix
-const getActiveSettingsKey = () => { - const currentPath = globalThis.location.pathname; +const getActiveSettingsKey = (pathname) => { + const currentPath = pathname; if (currentPath.includes("/settings/platform")) return "platform"; if (currentPath.includes("/users")) return "users"; if (currentPath.includes("/settings/triad")) return "triad"; if (currentPath.includes("/settings/review")) return "review"; return "platform"; }; const SettingsPopoverContent = ({ orgName, navigate }) => { + const { pathname } = useLocation(); const settingsMenuItems = getSettingsMenuItems(orgName); - const currentActiveKey = getActiveSettingsKey(); + const currentActiveKey = getActiveSettingsKey(pathname);
159-174: Apply unconditional hook calls for bothuseUnstractSubscriptionPlanStoreanduseSelectedProductStore.Biome's
useHookAtTopLevelrule (enabled by default) flags both hooks as violations since they're called conditionally. The try-catch onuseUnstractSubscriptionPlanStoredoesn't prevent the Rules of Hooks violation—only catches runtime errors. Both hooks require the same fix pattern:🔧 Proposed fix (unconditional hook calls)
- try { - if (unstractSubscriptionPlanStore?.useUnstractSubscriptionPlanStore) { - unstractSubscriptionPlan = - unstractSubscriptionPlanStore?.useUnstractSubscriptionPlanStore( - (state) => state?.unstractSubscriptionPlan, - ); - } - } catch (error) { - // Do nothing - } + const useUnstractSubscriptionPlanStore = + unstractSubscriptionPlanStore?.useUnstractSubscriptionPlanStore ?? + (() => undefined); + unstractSubscriptionPlan = useUnstractSubscriptionPlanStore( + (state) => state?.unstractSubscriptionPlan, + ); - if (selectedProductStore?.useSelectedProductStore) { - selectedProduct = selectedProductStore.useSelectedProductStore( - (state) => state?.selectedProduct, - ); - } + const useSelectedProductStore = + selectedProductStore?.useSelectedProductStore ?? + (() => undefined); + selectedProduct = useSelectedProductStore( + (state) => state?.selectedProduct, + );
|
This reverts commit cb2aa68.



What
Why
brightness(0) invert(1)to properly highlight the icon only when activeHow
frontend/src/plugins/assets/dashboard-icon.svgfrom white to #90A4B7Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.