Implement Breadcrumbs #1963
Implement Breadcrumbs #1963
Conversation
Summary by CodeRabbit
WalkthroughThis change refactors the BreadCrumbs component to be fully controlled via props, introduces a new PageLayout component for consistent page structure and breadcrumb rendering, and updates all relevant pages to use these components. Associated tests and mock data are updated to reflect the new breadcrumb logic and structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (3)
frontend/src/components/PageLayout.tsx (2)
22-35: Consider simplifying the breadcrumb generation logic.The function works correctly but could be more readable with reduced lodash usage.
Consider this cleaner implementation:
-function generateBreadcrumbs(pathname: string, excludeLast = false): BreadCrumbItem[] { - let segments = _.compact(_.split(pathname, '/')) - if (excludeLast) { - segments = _.dropRight(segments) - } - - return _.map(segments, (segment, index) => { - const path = '/' + _.join(_.slice(segments, 0, index + 1), '/') - return { - title: capitalize(segment), - path, - } - }) -} +function generateBreadcrumbs(pathname: string, excludeLast = false): BreadCrumbItem[] { + let segments = pathname.split('/').filter(Boolean) + if (excludeLast) { + segments = segments.slice(0, -1) + } + + return segments.map((segment, index) => ({ + title: capitalize(segment), + path: '/' + segments.slice(0, index + 1).join('/'), + })) +}
46-54: Simplify breadcrumb logic for better readability.The current logic is complex and could be more readable.
Consider this cleaner approach:
- if (customBreadcrumbs && customBreadcrumbs.length > 0) { - allBreadcrumbs = customBreadcrumbs - } else { - const isBreadCrumbItemsEmpty = _.isEmpty(breadcrumbItems) - const autoBreadcrumbs = generateBreadcrumbs(pathname, !isBreadCrumbItemsEmpty) - allBreadcrumbs = isBreadCrumbItemsEmpty - ? autoBreadcrumbs - : [...autoBreadcrumbs, { title: _.get(breadcrumbItems, 'title', ''), path: pathname }] - } + if (customBreadcrumbs?.length > 0) { + allBreadcrumbs = customBreadcrumbs + } else { + const autoBreadcrumbs = generateBreadcrumbs(pathname, !!breadcrumbItems) + allBreadcrumbs = breadcrumbItems + ? [...autoBreadcrumbs, { title: breadcrumbItems.title, path: pathname }] + : autoBreadcrumbs + }frontend/src/components/BreadCrumbs.tsx (1)
20-27: Consider more robust validation.The validation function works but could provide better debugging information.
Consider this enhanced version:
-function validateBreadcrumbItems(items: BreadCrumbItem[]): BreadCrumbItem[] { - return items.filter((item) => { - if (!item.title || !item.path) { - return false - } - return true - }) -} +function validateBreadcrumbItems(items: BreadCrumbItem[]): BreadCrumbItem[] { + return items.filter((item) => { + if (!item?.title?.trim() || !item?.path?.trim()) { + if (process.env.NODE_ENV === 'development') { + console.warn('Invalid breadcrumb item filtered out:', item) + } + return false + } + return true + }) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
frontend/__tests__/e2e/helpers/expects.ts(1 hunks)frontend/__tests__/unit/components/BreadCrumbs.test.tsx(1 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(2 hunks)frontend/__tests__/unit/data/mockRepositoryData.ts(1 hunks)frontend/__tests__/unit/pages/CommitteeDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/SnapshotDetails.test.tsx(8 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/CardDetailsPage.tsx(1 hunks)frontend/src/components/PageLayout.tsx(1 hunks)frontend/src/server/queries/organizationQueries.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)frontend/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/tests/unit/pages/CommitteeDetails.test.tsx
- frontend/src/app/layout.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: adithya-naik
PR: OWASP/Nest#1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: in next.js 13+ app router, userouter from 'next/navigation' does not provide aspath or query propert...
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/members/page.tsxfrontend/src/app/organizations/page.tsxfrontend/__tests__/unit/components/BreadCrumbs.test.tsxfrontend/src/components/PageLayout.tsxfrontend/src/components/BreadCrumbs.tsx
📚 Learning: in next.js 13+ app router, components with the 'use client' directive run entirely on the client sid...
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/members/page.tsxfrontend/src/app/organizations/page.tsxfrontend/src/components/PageLayout.tsx
📚 Learning: in the next.js frontend mentorship application, there are two distinct types for authentication-rela...
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/members/page.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/src/app/members/[memberKey]/page.tsx
📚 Learning: in the detailscard component (frontend/src/components/carddetailspage.tsx), there's a length check b...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/snapshots/page.tsxfrontend/src/app/organizations/[organizationKey]/page.tsx
📚 Learning: in the detailscard component (frontend/src/components/carddetailspage.tsx), there's a safety check t...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/snapshots/page.tsxfrontend/src/app/snapshots/[id]/page.tsx
📚 Learning: when testing react page components that use mocked form components, validation logic should be teste...
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/src/app/contribute/page.tsxfrontend/__tests__/unit/pages/ProjectDetails.test.tsxfrontend/src/app/snapshots/page.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/src/app/about/page.tsxfrontend/__tests__/unit/pages/SnapshotDetails.test.tsxfrontend/__tests__/e2e/helpers/expects.tsfrontend/src/app/snapshots/[id]/page.tsxfrontend/__tests__/unit/pages/RepositoryDetails.test.tsxfrontend/__tests__/unit/components/BreadCrumbs.test.tsx
📚 Learning: both programform (programcard.tsx) and moduleform (mainmodulecard.tsx) components already implement ...
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/app/contribute/page.tsx
📚 Learning: in the owasp nest project, exact mathematical accuracy in mock data is not required. mock data value...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1714
File: frontend/__tests__/unit/data/mockProjectsDashboardOverviewData.ts:12-14
Timestamp: 2025-07-08T16:25:39.325Z
Learning: In the OWASP Nest project, exact mathematical accuracy in mock data is not required. Mock data values can be simplified or rounded for testing purposes, as the focus is on testing functionality rather than precise calculations.
Applied to files:
frontend/__tests__/unit/data/mockProjectDetailsData.ts
📚 Learning: in the owasp nest project's metricspage component (frontend/src/app/projects/dashboard/metrics/page....
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/app/projects/dashboard/metrics/page.tsx:96-96
Timestamp: 2025-07-09T08:37:10.241Z
Learning: In the OWASP Nest project's MetricsPage component (frontend/src/app/projects/dashboard/metrics/page.tsx), the sorting dropdown intentionally uses selectionMode="multiple" to allow users to select multiple sorting criteria simultaneously. This enables complex sorting scenarios where users can sort by multiple fields in sequence.
Applied to files:
frontend/src/app/projects/page.tsx
🔇 Additional comments (59)
frontend/tsconfig.json (1)
24-28: Verify test runners map the new@testUtils/*alias.
ts-jest,jest, and the Next .jsjest.config.js(if any) must replicate this new path mapping throughmoduleNameMapper; otherwise unit tests that rely on@testUtils/*will break at runtime even though the TS compiler succeeds.frontend/src/app/projects/[projectKey]/page.tsx (2)
21-21: Import looks good.
Nothing else to flag here.
92-112: Review Comment: Breadcrumbs Are Already Auto-Generated
PageLayout’s implementation usesgenerateBreadcrumbs(pathname, excludeLast)to build all parent segments (e.g. “Projects”) and then appends your singlebreadcrumbItemsentry as the leaf. By passing<PageLayout breadcrumbItems={{ title: project.name }}>you already get “Projects › ”. No manual array is needed—ignore the suggested diff.
Likely an incorrect or invalid review comment.
frontend/__tests__/unit/data/mockProjectDetailsData.ts (2)
58-62: Type update may be required.
organizationnow includesname. Make sure theRepository/OrganizationTypeScript interfaces include this new field to avoidTS2339errors across the codebase.
72-75: LGTM – mock data stays aligned with backend schema.frontend/src/components/CardDetailsPage.tsx (1)
59-59: Light-mode background removed – risk of illegible content.With
bg-whitegone, the card relies on an ancestor (nowPageLayout) to supply a light background. IfPageLayoutdoes not, text will render on the default page background which is also white in many themes, so this is probably fine; just double-check visual contrast in both light and dark modes.frontend/src/app/chapters/[chapterKey]/page.tsx (2)
13-13: Import is fine.
64-76: Ignore manual breadcrumb suggestionThe
PageLayoutcomponent already generates the “Chapters” segment for you when you pass a singlebreadcrumbItemsobject. By supplying:<PageLayout breadcrumbItems={{ title: chapter.name }}> … </PageLayout>you’ll automatically get:
- “Chapters” → /chapters
[chapter.name]→ /current-pathNo changes are needed. If you need to override all breadcrumbs, use the
customBreadcrumbsprop instead.Likely an incorrect or invalid review comment.
frontend/src/server/queries/repositoryQueries.ts (1)
29-29: LGTM! Good addition for enhanced breadcrumb display.Adding the organization
namefield enables the breadcrumbs to display descriptive organization names rather than just login handles, which aligns perfectly with the PR objective to improve breadcrumb clarity.frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
91-92: Excellent improvement to test accessibility!Replacing generic
getByTextqueries with semanticgetByRole('heading')queries improves test robustness and aligns with accessibility best practices by targeting specific HTML elements rather than just text content.Also applies to: 271-272, 331-332
frontend/src/app/chapters/page.tsx (2)
12-12: Good addition of PageLayout import.The import supports the breadcrumb enhancement by bringing in the PageLayout component for consistent navigation structure.
80-107: Perfect implementation of PageLayout wrapper.Wrapping the SearchPageLayout with PageLayout follows the established pattern across the application for consistent breadcrumb navigation and page structure. The implementation preserves all existing functionality while adding the breadcrumb enhancement.
frontend/src/app/committees/[committeeKey]/page.tsx (2)
20-20: Good import addition for PageLayout component.Adding PageLayout import enables the breadcrumb enhancement for committee detail pages.
83-93: Excellent breadcrumb integration with dynamic committee name.The PageLayout wrapper with
breadcrumbItems={{ title: committee.name }}perfectly implements the PR objective to display descriptive titles in breadcrumbs. This will show the actual committee name instead of generic labels, enhancing navigation clarity.frontend/src/app/contribute/page.tsx (2)
12-12: Good import addition for PageLayout component.Adding PageLayout import enables consistent breadcrumb navigation for the contribute page.
74-88: Clean PageLayout wrapper implementation.Wrapping SearchPageLayout with PageLayout provides consistent breadcrumb navigation and page structure. The implementation follows the established pattern across search pages and maintains all existing functionality.
frontend/src/app/snapshots/page.tsx (2)
10-10: LGTM! Consistent PageLayout integration.The import of
PageLayoutaligns with the breadcrumb implementation objective across the application.
66-80: LGTM! Proper PageLayout wrapper implementation.The existing content is correctly wrapped in
PageLayoutwhile preserving all functionality. The early return for loading state (lines 61-63) is appropriately handled before the PageLayout wrapper, ensuring the loading spinner displays without breadcrumb navigation.frontend/__tests__/unit/data/mockRepositoryData.ts (1)
33-36: LGTM! Mock data enhancement for breadcrumb testing.The addition of the
organizationfield with bothloginandnameproperties supports the enhanced breadcrumb functionality that displays descriptive organization names instead of login handles, aligning with the PR objectives.frontend/src/app/organizations/page.tsx (2)
6-6: LGTM! Consistent import for PageLayout wrapper.The import follows the established pattern for adding breadcrumb navigation to search pages.
55-71: LGTM! Proper SearchPageLayout wrapper implementation.The existing
SearchPageLayoutis correctly wrapped inPageLayoutwhile preserving all props and functionality. This implementation is consistent with the breadcrumb enhancement pattern applied across other search pages.frontend/src/app/members/page.tsx (2)
6-6: LGTM! Consistent PageLayout import.The import aligns with the breadcrumb implementation pattern used across search pages.
55-71: LGTM! Proper SearchPageLayout integration.The
SearchPageLayoutis correctly wrapped inPageLayoutwhile maintaining all existing props and user card rendering functionality. This follows the established pattern for adding breadcrumb navigation to search pages.frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
81-81: ✅ Test assertion correctly updated to check for project URLThe
ProjectDetailsPagenow renders a “URL” entry displaying theproject.urlstring inside a<Link>. Verifying that the test checks forhttps://github.com/example-projectaligns perfectly with the component’sprojectDetailsconfiguration—no further changes needed.frontend/src/app/committees/page.tsx (2)
8-8: LGTM! Import aligns with breadcrumb enhancement pattern.The import of
PageLayoutis consistent with the broader refactoring to provide unified breadcrumb navigation across pages.
55-69: LGTM! Clean wrapper implementation preserves existing functionality.The
SearchPageLayoutis properly wrapped withPageLayoutwhile maintaining all existing props and behavior. This follows the established pattern for consistent breadcrumb navigation.frontend/src/app/projects/page.tsx (2)
10-10: LGTM! Consistent import pattern.The
PageLayoutimport follows the same pattern established across other pages for breadcrumb enhancement.
62-85: LGTM! Proper wrapper implementation with all props preserved.The
SearchPageLayoutis correctly wrapped withPageLayout, maintaining all existing functionality including sorting, pagination, and project filtering. The structure is consistent with other refactored pages.frontend/src/server/queries/organizationQueries.ts (1)
78-78: LGTM! Addition supports descriptive breadcrumb navigation.Adding the
namefield to the nested organization object enables displaying meaningful organization names in breadcrumbs instead of just login handles, which aligns with the PR objectives for more descriptive navigation.frontend/src/app/organizations/[organizationKey]/page.tsx (3)
18-18: LGTM! Import supports custom breadcrumb functionality.The
PageLayoutimport enables the implementation of custom breadcrumbs for organization details pages.
116-126: LGTM! Well-structured custom breadcrumbs implementation.The custom breadcrumbs correctly use the organization's name for display and login for the URL path, providing meaningful navigation hierarchy. The structure logically follows the URL pattern.
129-143: LGTM! Proper integration of PageLayout with custom breadcrumbs.The
DetailsCardis correctly wrapped withPageLayoutand receives the custom breadcrumbs, maintaining all existing functionality while adding enhanced navigation.frontend/src/app/members/[memberKey]/page.tsx (3)
25-25: LGTM! Import supports breadcrumb functionality.The
PageLayoutimport enables breadcrumb navigation for user details pages.
194-209: LGTM! Proper component wrapping with fallback breadcrumb title.The
DetailsCardis correctly wrapped withPageLayout, and the breadcrumb title appropriately falls back fromuser?.nametouser?.loginwhen the name is not available.
194-194: PageLayout prop interface supports both breadcrumbItems and customBreadcrumbs
Verified that PageLayoutProps infrontend/src/components/PageLayout.tsxdeclares:
breadcrumbItems?: crumbItemcustomBreadcrumbs?: customBreadcrumbItem[]Both usage patterns on member and organization pages are supported. No changes required.
frontend/src/app/snapshots/[id]/page.tsx (1)
19-19: LGTM! Clean breadcrumb implementation aligns with PR objectives.The addition of
PageLayoutimport and its usage with dynamic breadcrumb content successfully addresses the issue requirements. Usingsnapshot?.titlefor the breadcrumb display shows the actual page content rather than generic labels, which is exactly what was requested in issue #1425.Also applies to: 113-113
frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (2)
73-74: Improved semantic testing approach.Using
getByRole('heading')instead of plain text query is more semantic and robust for testing accessibility. This follows best practices for testing React components.
215-238: Comprehensive breadcrumb testing implementation.The test properly verifies:
- Presence of breadcrumb elements (Home, Organizations, organization name)
- Correct href attributes for navigation links
- Dynamic organization name display from mock data
This aligns with the PR objectives to test the new breadcrumb functionality showing actual content.
frontend/src/app/about/page.tsx (1)
32-32: Consistent PageLayout implementation.The About page correctly implements the new PageLayout wrapper pattern. Since no custom breadcrumbItems are provided, it will use URL-based breadcrumbs showing "Home > About", which is appropriate for this static page.
Also applies to: 103-103, 243-243
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (2)
110-128: Excellent custom breadcrumb implementation fulfills PR objectives.This breadcrumb construction perfectly addresses the issue requirements:
- Shows actual organization name (
repository.organization?.name) instead of generic labels- Provides robust fallback chain:
name→login→organizationKey- Creates logical navigation hierarchy: Organizations → [Org Name] → Repositories → [Repo Name]
- Dynamic breadcrumb paths based on actual data
This is exactly what was requested in issue #1425 for showing descriptive titles rather than generic labels.
131-148: Clean integration with PageLayout component.The
PageLayoutwrapper withcustomBreadcrumbsprop maintains the existingDetailsCardfunctionality while adding the enhanced breadcrumb navigation. The component structure remains clean and maintainable.frontend/__tests__/unit/pages/SnapshotDetails.test.tsx (3)
57-59: Proper React testing practices with act() wrapper.Wrapping render calls in
act()ensures proper handling of React state updates and effects during testing. This prevents potential race conditions and makes tests more reliable.Also applies to: 73-75, 88-90, 111-113, 132-134, 153-155, 180-182
92-94: Improved error assertion with waitFor.Adding
waitFor()before asserting the error message prevents race conditions and ensures the component has fully rendered the error state before testing.
158-159: Enhanced semantic testing approach.Using
getByRole('heading', { name: 'New Snapshot' })is more semantic and accessible than plain text queries, following React testing best practices.frontend/__tests__/e2e/helpers/expects.ts (3)
4-4: Improved breadcrumb selector specificity.The selector change from generic
[aria-label="breadcrumb"]tonav[role="navigation"][aria-label="breadcrumb"]is more specific and aligns with the semantic HTML structure of the refactored BreadCrumbs component.
6-6: Added appropriate timeout for e2e visibility checks.The 10-second timeout is reasonable for e2e tests where DOM elements may take time to render, especially for breadcrumb navigation that depends on data loading.
9-15: Robust breadcrumb verification with individual item checking.The logic to prepend "Home" when missing and check each breadcrumb individually with exact text matching is more reliable than bulk comparison. This handles edge cases where "Home" might not be explicitly provided in the test data.
frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (3)
71-78: Good test updates for new breadcrumb structure.The expectation that "Test Repo" appears twice (once in title, once in breadcrumb) correctly reflects the new breadcrumb implementation. The addition of repository statistics assertions improves test coverage.
235-272: Comprehensive breadcrumb rendering test.Excellent test coverage that verifies both breadcrumb text content and link href attributes. The test properly validates the organization name breadcrumb scenario and checks navigation paths.
274-310: Good fallback test for organization name edge case.This test properly handles the scenario where organization name is null, falling back to organization login. This ensures the breadcrumb system is robust and handles missing data gracefully.
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (4)
5-6: Well-designed test helper function.The
renderBreadCrumbshelper function provides a clean, consistent way to test the component with different configurations while maintaining good defaults.
62-83: Excellent accessibility testing coverage.The test thoroughly validates ARIA attributes, navigation role, and proper labeling. This ensures the breadcrumb component is accessible to screen readers and assistive technologies.
85-100: Good edge case handling for invalid breadcrumb items.This test ensures the component gracefully handles malformed data by filtering out items with missing titles or paths, preventing runtime errors.
122-133: Smart test for React key uniqueness.This test addresses a real-world scenario where breadcrumb items might have duplicate paths, ensuring the component generates unique keys to avoid React warnings.
frontend/src/components/BreadCrumbs.tsx (5)
10-18: Well-designed TypeScript interfaces.The interfaces clearly define the expected data structure for breadcrumb items and component props, with proper support for custom ARIA labels.
29-37: Good use of memo and early return pattern.The component is properly memoized for performance, and the early return for empty breadcrumbs prevents unnecessary rendering.
40-49: Excellent accessibility improvements.The component now uses semantic
<nav>element with proper ARIA attributes and hides decorative icons from screen readers.
58-66: Enhanced Home breadcrumb with accessibility.The Home breadcrumb now includes proper focus styles and ARIA labels for better accessibility and user experience.
68-92: Robust breadcrumb item rendering with accessibility.The rendering logic properly handles the last item as non-clickable with
aria-current="page", includes focus styles, and provides descriptive ARIA labels for each link.
Proposed change
Resolves #1425
Checklist
make check-testlocally; all checks and tests passed.