-
Notifications
You must be signed in to change notification settings - Fork 575
UN-3003: Addition to navbar #1659
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
Summary by CodeRabbit
WalkthroughAdds conditional rendering of a Custom Plans menu item in the navigation bar. The item appears when the application is Unstract, the user is staff, and the organization is not open source, including a SettingOutlined icon and navigation link to the admin custom-plans page. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 |
|
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 (1)
frontend/src/components/navigations/top-nav-bar/TopNavBar.jsx (1)
398-408: Add missing dependencies to the useMemo hook.The
itemsuseMemo is missing several dependencies that are used within its callback:isStaff,isOpenSource,isLoggedIn,logout, andsessionDetails. This will cause the menu to use stale values when these dependencies change, preventing the Custom Plans menu item from appearing/disappearing correctly when user permissions or organization context changes.Apply this diff to include the missing dependencies:
}, [ isUnstract, isSimpleLayout, reviewerStatus, approverStatus, allOrganization, cascadeOptions, orgName, orgId, shouldDisableRouting, + isStaff, + isOpenSource, + isLoggedIn, + logout, + sessionDetails, ]);
🧹 Nitpick comments (1)
frontend/src/components/navigations/top-nav-bar/TopNavBar.jsx (1)
167-167: Consider extracting the magic string to a constant.The hardcoded
"mock_org"string could be extracted to a named constant at the module level for better maintainability and clarity.Apply this diff to extract the constant:
+const OPEN_SOURCE_ORG_NAME = "mock_org"; + let TrialDaysInfo;Then update line 167:
- const isOpenSource = orgName === "mock_org"; + const isOpenSource = orgName === OPEN_SOURCE_ORG_NAME;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/navigations/top-nav-bar/TopNavBar.jsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
frontend/src/components/navigations/top-nav-bar/TopNavBar.jsx (3)
19-19: LGTM!The import of
SettingOutlinedis correctly added and properly used in the Custom Plans menu item.
355-369: AI summary inconsistency: No duplication detected.The AI summary claims this Custom Plans menu item is duplicated in two locations with the same key, but only one instance is visible in the provided code at lines 355-369. The implementation appears correct with no duplication.
355-369: Route verification confirms implementation is correct.The route
admin/custom-plansis properly defined atfrontend/src/routes/useMainAppRoutes.js:122and maps toUnstractAdministrationPage. The navigation target in the menu item correctly references this route.
jaseemjaskp
left a comment
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.
LGTM



What
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.