improved breadcrumbs by introducing page layout component #1515
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change refactors the breadcrumb navigation system in the frontend. The BreadCrumbs component now receives explicit breadcrumb items via props instead of deriving them from the URL. A new PageLayout component is introduced to encapsulate breadcrumbs and main content. All relevant pages and tests are updated accordingly. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
Documentation and Community
|
![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 (8)
frontend/src/app/committees/page.tsx (1)
67-67: Consider using optional chaining for cleaner code.The static analysis tool correctly identifies an opportunity to use optional chaining for more concise code.
- {committees && committees.map(renderCommitteeCard)} + {committees?.map(renderCommitteeCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/contribute/page.tsx (1)
86-86: Consider using optional chaining for cleaner code.Similar to the committees page, this can be simplified with optional chaining.
- {issues && issues.map(renderContributeCard)} + {issues?.map(renderContributeCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/organizations/page.tsx (1)
68-68: Simplify conditional rendering with optional chaining.The static analysis tool correctly identifies that optional chaining would be cleaner here.
Apply this diff to use optional chaining:
- {organizations && organizations.map(renderOrganizationCard)} + {organizations?.map(renderOrganizationCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/members/page.tsx (1)
67-67: Consider using optional chaining for better defensive programming.The static analysis tool suggests using optional chaining, which would make the code more robust against potential undefined values.
Apply this diff to use optional chaining:
- {users && users.map((user) => <div key={user.key}>{renderUserCard(user)}</div>)} + {users?.map((user) => <div key={user.key}>{renderUserCard(user)}</div>)}🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/projects/page.tsx (1)
83-83: Consider using optional chaining for consistency and safety.The static analysis tool suggests using optional chaining, which would be more defensive and consistent with modern JavaScript practices.
Apply this diff to use optional chaining:
- {projects && projects.filter((project) => project.is_active).map(renderProjectCard)} + {projects?.filter((project) => project.is_active).map(renderProjectCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/chapters/page.tsx (1)
105-105: Use optional chaining for cleaner code.The static analysis tool correctly identified that the conditional rendering can be simplified using optional chaining.
Apply this diff to use optional chaining:
- {chapters && chapters.filter((chapter) => chapter.is_active).map(renderChapterCard)} + {chapters?.filter((chapter) => chapter.is_active).map(renderChapterCard)}🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/components/PageLayout.tsx (1)
4-7: Consider moving interface to a shared types file.While the interface definition is correct, consider moving
BreadcrumbItemto a shared types file (e.g.,types/breadcrumb.ts) for better reusability across components, especially since this interface will likely be used by consuming components.-interface BreadcrumbItem { - title: string - href: string -}Create
frontend/src/types/breadcrumb.ts:export interface BreadcrumbItem { title: string href: string }Then import it:
+import { BreadcrumbItem } from 'types/breadcrumb'frontend/src/components/BreadCrumbs.tsx (1)
9-16: Consider interface naming consistency.The interface
BCrumbItemcould be renamed toBreadcrumbItemfor consistency with the PageLayout component and better readability.-interface BCrumbItem { +interface BreadcrumbItem { title: string href: string } interface BreadCrumbsProps { - bcItems: BCrumbItem[] + bcItems: BreadcrumbItem[] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
frontend/__tests__/unit/components/BreadCrumbs.test.tsx(3 hunks)frontend/__tests__/unit/pages/CommitteeDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/SnapshotDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/about/page.tsx(2 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(2 hunks)frontend/src/app/chapters/page.tsx(2 hunks)frontend/src/app/committees/[committeeKey]/page.tsx(2 hunks)frontend/src/app/committees/page.tsx(2 hunks)frontend/src/app/contribute/page.tsx(2 hunks)frontend/src/app/layout.tsx(0 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/app/members/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(2 hunks)frontend/src/app/organizations/page.tsx(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/app/projects/page.tsx(2 hunks)frontend/src/app/snapshots/[id]/page.tsx(2 hunks)frontend/src/app/snapshots/page.tsx(2 hunks)frontend/src/components/BreadCrumbs.tsx(2 hunks)frontend/src/components/PageLayout.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/app/layout.tsx
🧰 Additional context used
🧬 Code Graph Analysis (14)
frontend/src/app/organizations/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
frontend/src/app/committees/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
frontend/src/app/contribute/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
frontend/src/app/snapshots/page.tsx (2)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)frontend/src/types/snapshot.ts (1)
Snapshots(21-26)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
frontend/src/app/members/page.tsx (2)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)frontend/src/types/user.ts (1)
user(3-18)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
frontend/src/app/about/page.tsx (2)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)frontend/src/utils/aboutData.ts (2)
aboutText(1-5)technologies(7-76)
frontend/src/components/PageLayout.tsx (1)
frontend/src/components/BreadCrumbs.tsx (1)
BreadCrumbs(18-68)
frontend/src/app/projects/[projectKey]/page.tsx (2)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)backend/apps/github/models/repository.py (1)
project(158-160)
frontend/src/app/snapshots/[id]/page.tsx (2)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/components/BreadCrumbs.tsx (1)
frontend/src/types/link.ts (1)
Link(1-6)
frontend/src/app/chapters/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(8-21)
🪛 Biome (1.9.4)
frontend/src/app/organizations/page.tsx
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/committees/page.tsx
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/contribute/page.tsx
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/members/page.tsx
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/projects/page.tsx
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/chapters/page.tsx
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (41)
frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
71-72: Excellent improvement to test accessibility and specificity.The change from
screen.getByText()toscreen.getByRole('heading', { name: '...' })is a great improvement that:
- Makes the test more semantically specific by targeting heading elements
- Follows accessibility testing best practices
- Reduces the risk of false positives from other elements containing the same text
- Aligns well with the PageLayout component refactoring
frontend/__tests__/unit/pages/CommitteeDetails.test.tsx (1)
54-55: Consistent improvement in test query specificity.The update to use
getByRole('heading', { name: '...' })maintains consistency with the other test file improvements and enhances the test's semantic accuracy by specifically targeting heading elements.frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
73-74: Good consistency in test query improvements.The change to use role-based heading queries maintains the consistent pattern across the test suite and improves the semantic specificity of the test assertions.
frontend/__tests__/unit/pages/UserDetails.test.tsx (3)
91-92: Excellent test improvement for accessibility and specificity.The change to use
getByRole('heading', { name: 'Test User' })improves the test by specifically targeting heading elements, following accessibility testing best practices and reducing false positive risks.
271-272: Consistent application of improved test queries.Good continuation of the heading-specific query pattern in the user summary section test.
331-332: Thorough application of test improvements.The consistent use of role-based queries throughout this test file demonstrates thorough attention to improving test specificity and accessibility practices.
frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
71-72: Excellent accessibility improvement in test assertion!The change from text-based query to role-based query for the heading is a great improvement. Using
getByRole('heading', { name: 'Test Project' })is more semantically accurate and follows accessibility testing best practices.frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (1)
74-75: Consistent accessibility improvements across test cases!Both test assertions have been properly updated to use role-based queries for headings. This maintains consistency with the accessibility testing improvements made across other page test files.
Also applies to: 149-150
frontend/src/app/committees/page.tsx (1)
8-8: Great implementation of the PageLayout wrapper pattern!The integration of PageLayout follows the consistent pattern established across the codebase. The breadcrumb configuration is appropriate for the committees page, and all existing SearchPageLayout functionality is preserved.
Also applies to: 55-69
frontend/src/app/contribute/page.tsx (1)
12-12: Excellent consistency with the PageLayout integration pattern!The implementation perfectly follows the established pattern from other pages in this refactor. The breadcrumb configuration is appropriate and the existing SearchPageLayout functionality is fully preserved.
Also applies to: 74-88
frontend/src/app/members/[memberKey]/page.tsx (1)
26-26: LGTM! Clean breadcrumb navigation implementation.The PageLayout integration is well-implemented with proper breadcrumb hierarchy (Members > Current Member). The fallback from
user?.nametouser?.loginprovides good user experience when the user's display name isn't available.Also applies to: 195-216
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
13-13: LGTM! Consistent breadcrumb implementation.The PageLayout integration follows the established pattern with appropriate breadcrumb hierarchy (Chapters > Current Chapter). The chapter data is properly validated before rendering due to the existing error handling.
Also applies to: 64-81
frontend/src/app/projects/[projectKey]/page.tsx (1)
21-21: LGTM! Follows established breadcrumb pattern.The PageLayout integration maintains consistency with other detail pages, using appropriate breadcrumb hierarchy (Projects > Current Project). The project data validation through existing error handling ensures breadcrumbs render safely.
Also applies to: 93-116
frontend/src/app/organizations/page.tsx (1)
6-6: LGTM! Appropriate breadcrumb for search page.The PageLayout integration is correctly implemented for a search page, with a single breadcrumb item for "Organizations" which is appropriate for a listing page.
Also applies to: 55-72
frontend/src/app/committees/[committeeKey]/page.tsx (2)
20-20: LGTM: Clean import addition.The PageLayout import is correctly added to support the breadcrumb refactoring.
83-98: LGTM: Proper breadcrumb implementation.The PageLayout integration is well-implemented with a logical breadcrumb hierarchy:
- "Committees" → "/committees" (parent listing)
- Committee name → current page
The DetailsCard props remain unchanged and are correctly nested within the PageLayout.
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (2)
20-20: LGTM: Appropriate import for breadcrumb refactoring.The PageLayout import is correctly added to support the new breadcrumb navigation pattern.
110-135: LGTM: Well-structured breadcrumb hierarchy.The PageLayout implementation provides a clear navigation path:
- "Organizations" → "/organizations"
- Organization name → "/organizations/{key}"
- "Repositories" → "/organizations/{key}/repositories"
- Repository name → current page
This creates an intuitive breadcrumb trail for users navigating deep into the organizational structure. The DetailsCard integration is seamless.
frontend/src/app/members/page.tsx (2)
6-6: LGTM: Correct import for breadcrumb integration.The PageLayout import is properly added to support the new breadcrumb navigation structure.
54-70: LGTM: Appropriate breadcrumb for top-level page.The PageLayout integration with a single "Members" breadcrumb item is appropriate for this top-level listing page. The SearchPageLayout wrapping preserves all existing functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/projects/page.tsx (2)
10-10: LGTM: Proper import for breadcrumb functionality.The PageLayout import is correctly added to support the breadcrumb navigation refactoring.
62-85: LGTM: Clean integration with preserved functionality.The PageLayout wrapper with a single "Projects" breadcrumb item is appropriate for this top-level listing page. All SearchPageLayout props are correctly preserved, including the sortChildren configuration.
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/app/organizations/[organizationKey]/page.tsx (2)
18-18: LGTM: Import statement is correct.The PageLayout import is properly added and aligns with the new breadcrumb navigation pattern.
117-136: LGTM: PageLayout implementation is well-structured.The breadcrumb navigation correctly shows the hierarchy from "Organizations" to the specific organization. The usage of
organization.namefor the title andorganizationKeyfor the href provides appropriate context and navigation.frontend/src/app/snapshots/page.tsx (2)
10-10: LGTM: Import statement is correct.The PageLayout import follows the established pattern for breadcrumb navigation enhancement.
66-80: LGTM: PageLayout integration is clean and consistent.The implementation correctly wraps the page content with appropriate breadcrumb navigation. The single breadcrumb item for "Snapshots" is suitable for this top-level page.
frontend/src/app/about/page.tsx (2)
31-31: LGTM: Import statement is correct.The PageLayout import is properly placed and consistent with the breadcrumb navigation enhancement.
102-240: LGTM: PageLayout integration maintains existing functionality.The implementation correctly wraps all page content with appropriate breadcrumb navigation. The single breadcrumb item for "About" provides clear page context while preserving all existing UI components and interactions.
frontend/src/app/chapters/page.tsx (2)
12-12: LGTM: Import statement is correct.The PageLayout import follows the established pattern for breadcrumb navigation enhancement.
80-107: LGTM: PageLayout integration is well-implemented.The PageLayout correctly wraps the SearchPageLayout with appropriate breadcrumb navigation for the Chapters page. The nested structure preserves all existing search functionality while adding consistent navigation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
frontend/src/components/PageLayout.tsx (1)
8-21: LGTM! Clean and well-structured component.The PageLayout component follows React best practices with:
- Clear TypeScript typing
- Simple, focused responsibility
- Proper component composition
- Semantic HTML structure with the
<main>elementfrontend/src/app/snapshots/[id]/page.tsx (3)
19-19: LGTM! Proper import of the new PageLayout component.
113-118: Excellent implementation of breadcrumb navigation.The breadcrumb implementation is well-structured with:
- Appropriate hierarchy: Snapshots → Current Snapshot
- Defensive programming with fallback title (
snapshot?.title || 'Snapshot Details')- Correct URL paths matching the navigation structure
119-201: LGTM! Content properly wrapped in PageLayout.The existing page content is correctly wrapped in the PageLayout component while preserving all functionality. The indentation changes are appropriate for the new structure.
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (4)
5-9: Improved test approach - explicit props over mocked navigation.The change from testing pathname-based behavior to explicit prop-based behavior makes the tests clearer and more maintainable. Testing with an empty array is more direct than mocking pathname.
11-26: LGTM! Comprehensive test with explicit breadcrumb items.The test properly verifies:
- Multiple breadcrumb segments render correctly
- Home link is always present
- All provided breadcrumb items are displayed
38-41: Correct assertion for non-clickable last segment.The change from checking
hrefattribute absence to verifying the element is not wrapped in an anchor tag is more accurate for the actual DOM structure.
43-78: Comprehensive test coverage maintained.All important functionality is properly tested:
- Correct href attributes on links
- Hover styles applied to clickable elements
- Last segment behavior (non-clickable)
frontend/src/components/BreadCrumbs.tsx (3)
6-6: LGTM! Appropriate use of lodash for empty array checking.
18-19: Excellent refactoring - simplified component responsibility.The component now has a single, clear responsibility: rendering breadcrumbs from provided data. The early return for empty arrays is appropriate and prevents unnecessary DOM rendering.
45-63: LGTM! Clean implementation using provided breadcrumb items.The refactored rendering logic:
- Correctly iterates over provided bcItems
- Uses item.href as key (unique identifier)
- Properly handles last item as non-clickable
- Uses item.title and item.href directly from props
This is much cleaner than the previous pathname parsing approach.
|
Hey @arkid15r ! I've looked at the nitpick comments by the code bot, and I don't think those changes should be considered for this PR. However, if changes are required, please let me know. |
|
| const homeRoute = '/' | ||
| const pathname = usePathname() | ||
| const segments = pathname.split(homeRoute).filter(Boolean) | ||
| interface BCrumbItem { |
There was a problem hiding this comment.
Can we come up with a more clear naming here? For example BreadCrumbItem - it does not seem to be conflicting with the imported component 🤔
| type="chapter" | ||
| /> | ||
| <PageLayout | ||
| bcItems={[ |
There was a problem hiding this comment.
Same here - can we update this for a clearer naming? If it's conflicting with anything we can do something like crumbItem.
|
|
||
| interface BreadcrumbItem { | ||
| title: string | ||
| href: string |
There was a problem hiding this comment.
I think path is more suitable here than href
There was a problem hiding this comment.
Also noticed how different this component looks like in different themes.
Light theme has some weird background color.
Can we get rid of it?
| /> | ||
| <PageLayout | ||
| bcItems={[ | ||
| { title: 'Organizations', href: '/organizations' }, |
There was a problem hiding this comment.
Can we utilize usePathname from next/navigation to dynamically get the path, UpperCase it and use here for title and a path itself for href(should be named path)? Instead of hardcoding this.
|
Thank you for the feedback, @kasya . I’ll make the updates asap!. |
Resolves #1425