Breadcrumb component implementation #1397
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces a new BreadCrumbs component to the frontend, which dynamically renders breadcrumb navigation based on the current URL path. The component is integrated into the main application layout, ensuring breadcrumbs appear site-wide. Supporting this, a suite of end-to-end tests and a unit test have been added to verify breadcrumb rendering on various pages and under different path scenarios. Additionally, a helper function is provided to facilitate breadcrumb assertions in Playwright-based tests. Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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
|
![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 (3)
frontend/src/components/BreadCrumb.tsx (2)
18-19: Consider revising the absolute positioning.The absolute positioning with multiple positioning constraints (
absolute bottom-2 left-0 top-1 mt-16) could cause layout conflicts or unexpected behavior across different viewport sizes. Consider using a simpler positioning approach.- <div className="absolute bottom-2 left-0 top-1 mt-16 w-full py-2"> + <div className="relative w-full py-2 mt-16">
20-33: Add accessibility attributes to improve navigation.The breadcrumb component would benefit from proper accessibility attributes to enhance usability for screen readers.
<Breadcrumbs separator={ <FontAwesomeIcon icon={faChevronRight} className="mx-1 text-xs text-gray-400 dark:text-gray-500" + aria-hidden="true" /> } className="text-gray-800 dark:text-gray-200" + aria-label="Breadcrumb navigation" itemClasses={{ base: 'transition-colors duration-200', item: 'text-sm font-medium', separator: 'flex items-center', }} >frontend/src/app/layout.tsx (1)
5-5: Ensure consistent component naming.There's a naming inconsistency: import uses
Breadcrumb(lowercase 'c') but the component and file are namedBreadCrumb(uppercase 'C').-import Breadcrumb from 'components/BreadCrumb' +import BreadCrumb from 'components/BreadCrumb'Or alternatively, rename the component file and export to match this import.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/BreadCrumb.test.tsx(1 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/components/BreadCrumb.tsx(1 hunks)
🔇 Additional comments (10)
frontend/src/components/BreadCrumb.tsx (3)
9-9: Well-implemented utility function.The
capitalizefunction smartly handles both capitalization and replacing hyphens with spaces, making URL segments more readable in the UI.
15-15: Good UX decision to hide breadcrumbs on homepage.The conditional return of
nullwhen on the root path is a good UX decision - breadcrumbs aren't needed when there's no navigation hierarchy to display.
40-61: Well-implemented breadcrumb segments with proper handling of last item.The implementation correctly constructs navigation links for each segment and properly handles the last segment as non-clickable. The code also builds the correct path URLs by joining previous segments.
frontend/src/app/layout.tsx (1)
45-45: Successful integration of breadcrumb into layout.The breadcrumb component is correctly positioned after the header and before the main content, which is the standard placement for breadcrumb navigation.
frontend/__tests__/unit/components/BreadCrumb.test.tsx (6)
6-8: Good mock implementation for usePathname.Correctly mocking the
usePathnamehook enables testing different path scenarios without depending on Next.js router internals.
11-13: Good practice clearing mocks between tests.Using
jest.clearAllMocks()after each test ensures test isolation and prevents potential test interference.
15-20: Comprehensive test for root path behavior.This test correctly verifies that the breadcrumb doesn't render on the root path, which aligns with the component implementation.
22-29: Good test for single segment path.The test correctly checks that both "Home" and the capitalized path segment are rendered, validating the basic breadcrumb functionality.
31-40: Comprehensive test for multiple segments.This test thoroughly validates the breadcrumb's ability to handle complex paths with multiple segments, checking that each segment is properly rendered.
42-50: Important test for non-clickable last segment.This test correctly verifies a key UX requirement - that the last breadcrumb segment is not a link, properly distinguishing the current page from navigation options.
There was a problem hiding this comment.
Could you add some e2e tests for breadcrumbs too?
There was a problem hiding this comment.
It should be plural BreadCrumbs?
frontend/src/app/layout.tsx
Outdated
| > | ||
| <Providers> | ||
| <Header /> | ||
| <BreadCrumb /> |
There was a problem hiding this comment.
| <BreadCrumb /> | |
| <BreadCrumbs /> |
| import Link from 'next/link' | ||
| import { usePathname } from 'next/navigation' | ||
|
|
||
| const capitalize = (str: string) => str.charAt(0).toUpperCase() + str.slice(1).replace(/-/g, ' ') |
There was a problem hiding this comment.
We may have capitalize function in utils. Please use or extend that one.
| const pathname = usePathname() | ||
| const segments = pathname.split('/').filter(Boolean) | ||
|
|
||
| if (pathname === '/') return null |
There was a problem hiding this comment.
We could extract this / to a separate constant.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/__tests__/e2e/pages/About.spec.ts (1)
88-92: Test looks good, but consider adding Home segment verificationThe test verifies the presence of the breadcrumb and the "About" segment, which is appropriate. For consistency with other page tests (like Projects and Users), consider also verifying the "Home" segment which should appear in the breadcrumb trail.
test('breadcrumb renders correct segments on /about', async ({ page }) => { const breadcrumb = page.locator('[aria-label="breadcrumb"]') await expect(breadcrumb).toBeVisible() + await expect(breadcrumb.getByText('Home')).toBeVisible() await expect(breadcrumb.getByText('About')).toBeVisible() })frontend/__tests__/e2e/pages/Users.spec.ts (1)
62-67: Test looks good, add page route to test name for consistencyThe breadcrumb test correctly verifies both the presence of the breadcrumb element and the expected text segments. For consistency with other page tests that specify the route in the test name (like "breadcrumb renders correct segments on /about"), consider updating the test name to include the route.
- test('breadcrumb renders correct segments', async ({ page }) => { + test('breadcrumb renders correct segments on /members', async ({ page }) => { const breadcrumb = page.locator('[aria-label="breadcrumb"]') await expect(breadcrumb).toBeVisible() await expect(breadcrumb.getByText('Home')).toBeVisible() await expect(breadcrumb.getByText('Members')).toBeVisible() })frontend/__tests__/e2e/pages/Chapters.spec.ts (1)
58-63: LGTM! Good breadcrumb test implementation.The test effectively verifies that the breadcrumb is visible and displays the correct segments.
Consider also testing the navigation functionality by verifying that the "Home" segment is a clickable link pointing to "/", similar to what's done in the Organizations.spec.ts file.
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (2)
16-16: Consider a cleaner approach to the mock implementation.The prefixed semicolon (
;(usePathname as jest.Mock)) is used to avoid automatic semicolon insertion issues, but it can look unusual and confusing to developers unfamiliar with this pattern.- ;(usePathname as jest.Mock).mockReturnValue('/') + (usePathname as jest.Mock).mockReturnValue('/')Or alternatively:
- ;(usePathname as jest.Mock).mockReturnValue('/') + const mockedUsePathname = usePathname as jest.Mock; + mockedUsePathname.mockReturnValue('/');
1-42: Consider adding tests for additional edge cases.While the current tests cover the core functionality well, consider adding tests for these edge cases:
- Paths with special characters or spaces (e.g.,
/products/women's-clothing)- Very long paths with many segments to ensure proper rendering
- Internationalization support if the application supports multiple languages
This would provide more comprehensive coverage for the component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/__tests__/e2e/pages/About.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Chapters.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Committees.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Organizations.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Projects.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Users.spec.ts(1 hunks)frontend/__tests__/unit/components/BreadCrumbs.test.tsx(1 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/components/BreadCrumbs.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/BreadCrumbs.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/layout.tsx
🔇 Additional comments (8)
frontend/__tests__/e2e/pages/Projects.spec.ts (1)
56-61: Test implementation looks goodThe breadcrumb test correctly verifies both the presence of the breadcrumb element and the expected text segments ("Home" and "Projects"). The test follows the same pattern as the other page tests, ensuring consistency.
frontend/__tests__/e2e/pages/Committees.spec.ts (1)
58-63: Test implementation looks goodThe breadcrumb test correctly verifies both the presence of the breadcrumb element and the expected text segments ("Home" and "Committees"). The test follows the same pattern as the other page tests, ensuring consistency.
frontend/__tests__/e2e/pages/Organizations.spec.ts (1)
44-51: Great implementation of breadcrumb test with link verification.The test effectively verifies the breadcrumb visibility, content, and most importantly, that the "Home" link correctly points to the root path. This is a comprehensive approach that ensures both the display and functionality of the breadcrumb navigation.
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (5)
1-9: Good setup for testing the BreadCrumbs component.The file correctly imports the necessary testing utilities and sets up the mock for Next.js's usePathname hook, which is essential for testing a component that depends on the current URL path.
10-14: Good practice using afterEach to clear mocks.Using jest.clearAllMocks() after each test prevents state leakage between tests and ensures that each test starts with a clean slate.
15-20: Great test for the root path edge case.This test correctly verifies that the breadcrumb doesn't render on the root path, which is important for user experience.
22-31: Comprehensive test for multi-segment paths.The test effectively verifies that all expected segments are rendered with proper capitalization, including Home and each path segment.
33-41: Good test for the last segment behavior.This test verifies that the last segment in the breadcrumb path is not a clickable link, which is the expected behavior for the current page.
| test('breadcrumb renders correct segments on /contribute', async ({ page }) => { | ||
| const breadcrumb = page.locator('[aria-label="breadcrumb"]') | ||
| await expect(breadcrumb).toBeVisible() | ||
| await expect(breadcrumb.getByText('Contribute')).toBeVisible() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test coverage can be improved for breadcrumb verification.
While the test verifies that the breadcrumb is visible and contains the 'Contribute' segment, it's missing some important checks:
- It doesn't verify the 'Home' segment that should also be present
- It doesn't verify that the 'Home' link points to the root path ('/')
Consider enhancing this test to align with the more comprehensive approach used in the Organizations.spec.ts:
test('breadcrumb renders correct segments on /contribute', async ({ page }) => {
const breadcrumb = page.locator('[aria-label="breadcrumb"]')
await expect(breadcrumb).toBeVisible()
+ await expect(breadcrumb.getByText('Home')).toBeVisible()
await expect(breadcrumb.getByText('Contribute')).toBeVisible()
+ const homeLink = breadcrumb.locator('a', { hasText: 'Home' })
+ await expect(homeLink).toHaveAttribute('href', '/')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('breadcrumb renders correct segments on /contribute', async ({ page }) => { | |
| const breadcrumb = page.locator('[aria-label="breadcrumb"]') | |
| await expect(breadcrumb).toBeVisible() | |
| await expect(breadcrumb.getByText('Contribute')).toBeVisible() | |
| }) | |
| test('breadcrumb renders correct segments on /contribute', async ({ page }) => { | |
| const breadcrumb = page.locator('[aria-label="breadcrumb"]') | |
| await expect(breadcrumb).toBeVisible() | |
| await expect(breadcrumb.getByText('Home')).toBeVisible() | |
| await expect(breadcrumb.getByText('Contribute')).toBeVisible() | |
| const homeLink = breadcrumb.locator('a', { hasText: 'Home' }) | |
| await expect(homeLink).toHaveAttribute('href', '/') | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/e2e/helpers/breadCrumbsHelper.ts (2)
3-9: Consider adding JSDoc comments for better documentation.Adding JSDoc comments would improve the documentation of this helper function, making it more maintainable and easier for other developers to understand its purpose and usage.
+/** + * Verifies that the breadcrumb navigation is visible and contains the specified items. + * @param page - The Playwright Page object + * @param items - Array of breadcrumb text items to check (defaults to ['Home']) + * @example + * // Check for Home > About breadcrumb + * await expectBreadcrumbVisible(page, ['Home', 'About']); + */ export async function expectBreadcrumbVisible(page: Page, items: string[] = ['Home']) { const breadcrumb = page.locator('[aria-label="breadcrumb"]') await expect(breadcrumb).toBeVisible() for (const item of items) { await expect(breadcrumb.getByText(item)).toBeVisible() } }
4-8: Consider checking breadcrumb items order.Currently, the function checks for the presence of each breadcrumb item, but not their order, which is important for breadcrumb navigation. You might want to enhance this helper to verify that items appear in the correct sequence.
export async function expectBreadcrumbVisible(page: Page, items: string[] = ['Home']) { const breadcrumb = page.locator('[aria-label="breadcrumb"]') await expect(breadcrumb).toBeVisible() + + // Get all breadcrumb items + const breadcrumbItems = breadcrumb.locator('li') + + // Verify item count matches expected + await expect(breadcrumbItems).toHaveCount(items.length) + + // Verify each item in order + for (let i = 0; i < items.length; i++) { + await expect(breadcrumbItems.nth(i)).toContainText(items[i]) + } + + // Also check individual items visibility as before for (const item of items) { await expect(breadcrumb.getByText(item)).toBeVisible() } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/__tests__/e2e/helpers/breadCrumbsHelper.ts(1 hunks)frontend/__tests__/e2e/pages/About.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Chapters.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Committees.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Contribute.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Organizations.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Projects.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Users.spec.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/tests/e2e/pages/Chapters.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/tests/e2e/pages/Committees.spec.ts
- frontend/tests/e2e/pages/Organizations.spec.ts
- frontend/tests/e2e/pages/Contribute.spec.ts
- frontend/tests/e2e/pages/Users.spec.ts
- frontend/tests/e2e/pages/About.spec.ts
- frontend/tests/e2e/pages/Projects.spec.ts
🔇 Additional comments (1)
frontend/__tests__/e2e/helpers/breadCrumbsHelper.ts (1)
1-9: Well-structured helper function with good accessibility practices!The implementation is clean, concise and follows good practices for Playwright testing. The use of aria-label for locating the breadcrumb is excellent for accessibility.
|
Hey @arkid15r |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/e2e/helpers/expects.ts (2)
9-11: Consider enhancing the breadcrumbs validationThe current implementation verifies each breadcrumb's presence but doesn't validate their order or relationships, which might be important for breadcrumb navigation.
Consider enhancing the function to also validate the order of breadcrumbs:
for (const breadcrumb of breadcrumbs) { await expect(breadcrumbsContainer.getByText(breadcrumb)).toBeVisible() } + +// Optional: Verify the breadcrumbs appear in the correct order +const allBreadcrumbTexts = await breadcrumbsContainer.locator('li').allTextContents(); +const visibleBreadcrumbs = allBreadcrumbTexts.filter(text => text.trim() !== ''); +await expect(visibleBreadcrumbs).toEqual(breadcrumbs);
6-7: Consider adding error messages to assertionsAdding custom error messages to the assertions would make test failures more informative and easier to debug.
- await expect(breadcrumbsContainer).toBeVisible() - await expect(breadcrumbsContainer).toHaveCount(1) + await expect(breadcrumbsContainer).toBeVisible({ timeout: 5000 }, 'Breadcrumbs container should be visible') + await expect(breadcrumbsContainer).toHaveCount(1, 'Should have exactly one breadcrumbs container')
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/__tests__/e2e/helpers/expects.ts(1 hunks)frontend/__tests__/e2e/pages/About.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Chapters.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Committees.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Contribute.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Organizations.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Projects.spec.ts(2 hunks)frontend/__tests__/e2e/pages/Users.spec.ts(2 hunks)frontend/src/components/BreadCrumbs.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/tests/e2e/pages/About.spec.ts
- frontend/tests/e2e/pages/Projects.spec.ts
- frontend/tests/e2e/pages/Committees.spec.ts
- frontend/tests/e2e/pages/Users.spec.ts
- frontend/tests/e2e/pages/Chapters.spec.ts
- frontend/tests/e2e/pages/Contribute.spec.ts
- frontend/tests/e2e/pages/Organizations.spec.ts
- frontend/src/components/BreadCrumbs.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
frontend/__tests__/e2e/helpers/expects.ts (2)
1-12: Well-structured helper function for breadcrumb testingThis helper function is well-designed with proper TypeScript types, reasonable defaults, and good use of Playwright's testing utilities. The function correctly checks for both the container visibility and the presence of each breadcrumb text.
4-4: Great usage of accessibility attributes for testingUsing
[aria-label="breadcrumb"]as a selector is an excellent approach, as it leverages accessibility attributes for testing purposes, ensuring both good test practices and accessibility compliance.
|
* breadcrumb component * updated code * added e2e * update code * updated code * Update code * Update tests --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
fixes: #1389
Screen.Recording.2025-04-18.at.5.09.41.PM.mp4
Screen.Recording.2025-04-18.at.4.31.48.PM.mp4
Screen.Recording.2025-04-18.at.4.29.34.PM.mp4