Fix: Links of sidebar_menu now working at view_profile_section #3086
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ProfileDropdown/ProfileDropdown.tsx (1)
106-106: Ensure fallback navigation logic for members is intentional
WhenuserRoleis not'User', the code navigates to/member/${orgId || ''}. IforgIdis empty, this might cause a route mismatch. Consider redirecting to a fully qualified and valid path whenorgIdisn’t present.-: navigate(`/member/${orgId || ''}`) +: navigate(orgId ? `/member/${orgId}` : '/defaultMemberPath')src/components/ProfileDropdown/ProfileDropdown.spec.tsx (1)
242-242: Consider using an in-memory router for testing.While
window.history.pushState()works here, you might findMemoryRouterorcreateMemoryHistory(fromhistory) more aligned with typical React testing patterns. This approach isolates navigation for each test, preventing potential side effects in a shared window history.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ProfileDropdown/ProfileDropdown.spec.tsx(3 hunks)src/components/ProfileDropdown/ProfileDropdown.tsx(3 hunks)
🔇 Additional comments (5)
src/components/ProfileDropdown/ProfileDropdown.tsx (2)
4-4: Use of useParams is aligned with React Router best practices
The import of the useParams hook is a commendable approach to extracting route parameters directly from the URL, aligning well with recommended usage patterns.
40-40: Verify that orgId will always be available or handled gracefully
If orgId in the URL is omitted, useParams() returns undefined. The fallback to an empty string might result in navigation issues (e.g., navigating to /member/). Confirm the presence of a default route or guard this scenario more explicitly.
✅ Verification successful
Let me verify the usage of orgId with additional checks.
Let me check how the MemberDetail component handles the orgId parameter.
Let me check how the ProfileDropdown component handles navigation and if there are any fallbacks.
The orgId parameter is handled gracefully with appropriate fallbacks
The codebase shows that:
- The ProfileDropdown component handles missing
orgIdby using the nullish coalescing operator (orgId || ''), preventing navigation to an invalid URL. - There's a default route
/memberwithout parameters that will catch any invalid member routes. - The navigation is role-dependent, with users being redirected to
/user/settingsinstead of the member route.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the /member/ route is defined or gracefully handled in the codebase.
# Searching for route definitions containing '/member'
rg -A 3 $'/member'
Length of output: 5061
Script:
#!/bin/bash
# Check how MemberDetail component handles the orgId parameter
ast-grep --pattern 'function MemberDetail() {
$$$
}'
# Check for any route guards or redirects
rg -A 3 "Navigate|Redirect" src/
Length of output: 56327
Script:
#!/bin/bash
# Check MemberDetail component implementation
rg -A 10 "function MemberDetail" src/screens/MemberDetail/MemberDetail.tsx
# Check if there are any guards or error handling for orgId in MemberDetail
rg -A 5 "orgId|currentUrl" src/screens/MemberDetail/MemberDetail.tsx
Length of output: 1366
Script:
#!/bin/bash
# Check ProfileDropdown navigation logic
rg -B 5 -A 5 "navigate.*member.*orgId" src/components/ProfileDropdown/ProfileDropdown.tsx
# Check if there are any default routes or fallbacks for member route
rg -A 3 "path=\"/member\"" src/App.tsx
Length of output: 674
src/components/ProfileDropdown/ProfileDropdown.spec.tsx (3)
4-4: Use of Route and Routes is appropriate.
Importing Route and Routes from react-router-dom for test scenarios is a recommended approach and ensures that the component is properly tested in a realistic routing context.
250-252: Route definition for /:orgId is sufficient but verify broader coverage.
Your inclusion of the <Route path="/:orgId" element={<ProfileDropdown />} /> covers the updated logic. However, consider edge cases and error routes (e.g., invalid or missing orgId) by adding tests to ensure robust handling.
[approve]
Would you like me to draft a shell script to locate and check all route definitions in the codebase to confirm coverage for possible error states?
266-266: Ensure that /member/:orgId route is present in production code.
The test expects navigation to '/member/321'. Please confirm that the main application indeed has a corresponding route for '/member/:orgId'. If not, the test might pass locally but encounter an undefined route in production.
✅ Verification successful
Route /member/:orgId is properly defined and used throughout the application
The route /member/:orgId is correctly defined in src/App.tsx:
<Route path="/member/:orgId" element={<MemberDetail />} />This route is consistently used across multiple components:
OrganizationPeople: Links to member profilesProfileDropdown: Navigation to member profilesAddMember: Member profile navigationManageTag: View profile functionalityLeaderboard: User profile navigationEventAttendance: Member profile links
The test's expectation of navigation to /member/321 aligns with the production code's routing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for route definitions matching '/member/{orgId}'
rg -A 5 $'/member/'
Length of output: 6878
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3086 +/- ##
=====================================================
+ Coverage 26.46% 88.94% +62.47%
=====================================================
Files 300 321 +21
Lines 7568 8410 +842
Branches 1652 1837 +185
=====================================================
+ Hits 2003 7480 +5477
+ Misses 5434 688 -4746
- Partials 131 242 +111 ☔ View full report in Codecov by Sentry. |
palisadoes
merged commit e0da67b
into
PalisadoesFoundation:develop-postgres
…adoesFoundation#3086) * fix: Links of sidebar_menu now working at view_profile_section * Added Test when no organisation is selected
PratapRathi
deleted the
fix-links-of-sidebar-menu-not-working-at-view-profile-page
branch
What kind of change does this PR introduce?
This PR will fix the issue, when at view_profile page left-side menu links are not working, also separate test is also added for the same so that it will not break again.
Issue Number:
Fixes #3000
Did you add tests for your changes?
Yes
Snapshots/Videos:
Screen.Recording.2024-12-30.at.18.59.47.mov
If relevant, did you update the documentation?
N/A
Summary
Does this PR introduce a breaking change?
No
Other information
When we click on the
view profilelink it was routing page to/member/:userIdbut now it is fixed to/member/:orgId. Also whenorgIdis not available then route to/member/.Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests