refactor: update breadcrumbs component to support dynamic title rende… #2573
Conversation
Summary by CodeRabbitRelease Notes
WalkthroughAdds a breadcrumb system (hook, renderer, pathname-driven wrapper), a PageLayout component that renders breadcrumbs, wraps multiple detail pages with PageLayout, updates tests, adjusts repository GraphQL query to include organization.name, and updates the custom dictionary. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-07-13T11:29:25.245ZApplied to files:
📚 Learning: 2025-07-13T07:31:06.511ZApplied to files:
🧬 Code graph analysis (2)frontend/src/components/RepositoryCard.tsx (1)
frontend/src/components/BreadCrumbs.tsx (1)
⏰ 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). (4)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/hooks/useBreadcrumbs.ts (1)
26-27: Consider making route pattern detection more maintainable.The nested repository route detection uses a hardcoded check with magic values. Consider extracting this as a constant or pattern matcher for better maintainability.
+const NESTED_REPO_PATTERN = { + segments: 4, + positions: { organization: 0, repositories: 2 }, + values: { organization: 'organizations', repositories: 'repositories' } +} + const isNestedRepoRoute = - segments.length === 4 && segments[0] === 'organizations' && segments[2] === 'repositories' + segments.length === NESTED_REPO_PATTERN.segments && + segments[0] === NESTED_REPO_PATTERN.values.organization && + segments[2] === NESTED_REPO_PATTERN.values.repositoriesfrontend/src/components/BreadCrumbsWrapper.tsx (1)
19-78: LGTM! Solid implementation with good edge case handling.The BreadCrumbsWrapper component correctly:
- ✅ Returns null for home page and PageLayout-managed routes
- ✅ Generates breadcrumb items from path segments with proper formatting (
upperFirst+ dash replacement)- ✅ Builds progressive paths for each breadcrumb link
- ✅ Renders with consistent styling using HeroUI and FontAwesome
The logic aligns well with the
useBreadcrumbsfallback behavior and has comprehensive test coverage.Optional future refactor: The rendering logic (lines 36-77) is nearly identical to
BreadCrumbRenderer(lines 10-53 infrontend/src/components/BreadCrumbs.tsx). Consider extracting the common rendering logic to reduce duplication:// Shared base component function BreadcrumbsBase({ items }: { items: BreadCrumbItem[] }) { return ( <div className="mt-16 w-full pt-4"> {/* ... shared rendering logic ... */} </div> ) } // BreadCrumbsWrapper would then: export default function BreadCrumbsWrapper() { // ... route detection and item generation ... return <BreadcrumbsBase items={items} /> }This would centralize the styling and structure, making future updates easier.
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (1)
37-43: Update the last-item test to assert current-page semantics.Once the renderer stops using
isDisabled, this case should verify the breadcrumb exposesaria-current="page"(and rename the test accordingly) rather than asserting it’s disabled. That keeps the test aligned with the accessibility contract we want. (heroui.com)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (20)
cspell/custom-dict.txt(3 hunks)frontend/__tests__/unit/components/BreadCrumbs.test.tsx(1 hunks)frontend/__tests__/unit/components/BreadCrumbsWrapper.test.tsx(1 hunks)frontend/__tests__/unit/components/PageLayout.test.tsx(1 hunks)frontend/__tests__/unit/hooks/useBreadcrumbs.test.ts(1 hunks)frontend/__tests__/unit/pages/ChapterDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/CommitteeDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(2 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(2 hunks)frontend/src/app/committees/[committeeKey]/page.tsx(2 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(3 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/components/BreadCrumbs.tsx(2 hunks)frontend/src/components/BreadCrumbsWrapper.tsx(1 hunks)frontend/src/components/PageLayout.tsx(1 hunks)frontend/src/hooks/useBreadcrumbs.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/app/committees/[committeeKey]/page.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/components/PageLayout.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/CommitteeDetails.test.tsxfrontend/__tests__/unit/pages/ChapterDetails.test.tsxfrontend/__tests__/unit/components/BreadCrumbsWrapper.test.tsxfrontend/__tests__/unit/components/BreadCrumbs.test.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsxfrontend/__tests__/unit/pages/CommitteeDetails.test.tsxfrontend/__tests__/unit/pages/ChapterDetails.test.tsxfrontend/src/components/BreadCrumbs.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/components/BreadCrumbsWrapper.tsxfrontend/__tests__/unit/pages/CommitteeDetails.test.tsxfrontend/__tests__/unit/pages/ChapterDetails.test.tsxfrontend/src/components/BreadCrumbs.tsxfrontend/src/components/PageLayout.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/server/queries/repositoryQueries.ts
🧬 Code graph analysis (13)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
frontend/__tests__/unit/components/PageLayout.test.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
frontend/src/app/projects/[projectKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
frontend/__tests__/unit/hooks/useBreadcrumbs.test.ts (1)
frontend/src/hooks/useBreadcrumbs.ts (1)
useBreadcrumbs(18-64)
frontend/src/app/members/[memberKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
frontend/__tests__/unit/components/BreadCrumbsWrapper.test.tsx (1)
frontend/src/components/BreadCrumbsWrapper.tsx (1)
BreadCrumbsWrapper(19-78)
frontend/src/app/layout.tsx (1)
frontend/src/components/BreadCrumbsWrapper.tsx (1)
BreadCrumbsWrapper(19-78)
frontend/src/components/BreadCrumbs.tsx (1)
frontend/src/hooks/useBreadcrumbs.ts (1)
BreadCrumbItem(4-7)
frontend/src/components/PageLayout.tsx (2)
frontend/src/hooks/useBreadcrumbs.ts (1)
useBreadcrumbs(18-64)frontend/src/components/BreadCrumbs.tsx (1)
BreadCrumbRenderer(11-54)
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (1)
frontend/src/components/BreadCrumbs.tsx (1)
BreadCrumbRenderer(11-54)
frontend/src/app/organizations/[organizationKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
🔇 Additional comments (18)
cspell/custom-dict.txt (1)
9-9: Verify the necessity of bothCyclonedxandcyclonedxcasing variants.All four new dictionary entries are correctly placed in alphabetical order. However, the addition of both
Cyclonedx(line 9, capitalized) andcyclonedx(line 58, lowercase) appears redundant if both are intended to match the same tool name. Most spell-checkers handle case-insensitive matching, so a single entry may be sufficient.Additionally, the standard spelling is
CycloneDX(mixed case), notCyclonedx. If this tool is referenced in the breadcrumb-related code, consider whether the dictionary entries match the actual casing used there.Please confirm:
- Whether both casing variants are necessary for the spell-checker configuration in this project.
- Whether these words are actually used in the breadcrumb component changes, tests, or documentation, and if so, in what casing.
Also applies to: 17-17, 48-48, 58-58
frontend/__tests__/unit/components/PageLayout.test.tsx (1)
1-137: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of the PageLayout component functionality, including:
- Children rendering
- Breadcrumb positioning in the DOM
- Various breadcrumbData field types (projectName, memberName, chapterName, committeeName, orgName/repoName)
- Edge cases (undefined breadcrumbData, mismatched field types)
The use of
compareDocumentPositionto verify breadcrumb placement is a solid approach.frontend/src/app/layout.tsx (1)
9-9: LGTM! Clean component swap.The replacement of BreadCrumbs with BreadCrumbsWrapper aligns with the broader breadcrumb refactoring described in the PR objectives.
Also applies to: 77-77
frontend/src/server/queries/repositoryQueries.ts (1)
34-34: LGTM! Supports breadcrumb display names.Adding
organization.nameto the query enables breadcrumbs to display the full organization name instead of relying on the slug-formattedloginfield.frontend/src/app/projects/[projectKey]/page.tsx (1)
21-21: LGTM! Consistent PageLayout integration.The wrapping of DetailsCard with PageLayout and the passing of
breadcrumbData={{ projectName: project.name }}follows the established pattern and enables proper breadcrumb rendering with the actual project name.Also applies to: 93-113
frontend/__tests__/unit/pages/CommitteeDetails.test.tsx (1)
25-25: LGTM! Improved test robustness.The changes adapt the test for the PageLayout integration:
- Mocking
usePathnamesupports the breadcrumb generation logic- Using
getByRole('heading', { name: 'Test Committee' })is more semantically correct and resilient than text-based queriesAlso applies to: 56-56
frontend/src/app/members/[memberKey]/page.tsx (1)
29-29: LGTM! Proper fallback handling.The PageLayout integration is clean and includes a sensible fallback:
memberName: user?.name || user?.loginensures the breadcrumb displays either the user's full name or their login if no name is set.Also applies to: 235-249
frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
31-31: LGTM! Consistent test improvements.The changes mirror the pattern in CommitteeDetails.test.tsx:
usePathnamemock enables breadcrumb generation- Role-based heading query is more robust than text matching
Also applies to: 74-74
frontend/src/hooks/useBreadcrumbs.ts (1)
50-58: The current breadcrumbData logic is correct and intentional.The OR-chaining behavior in lines 50-58 is explicitly tested and expected. The test "handles partial breadcrumbData (uses first matching field)" at line 212 in
useBreadcrumbs.test.tsvalidates that when a breadcrumbData field is provided, the first truthy value from the OR-chain is used regardless of route type. The same scenario is tested inPageLayout.test.tsxat line 125 under the title "handles partial breadcrumbData (wrong field for route type)," confirming this is intentional design.Additionally, the codebase design prevents the problematic scenario from occurring: every page passes only the appropriate field for its route type (e.g.,
projectNamefor project routes,memberNamefor member routes). No instances exist where multiple conflicting fields are passed simultaneously.frontend/src/app/committees/[committeeKey]/page.tsx (1)
84-94: LGTM!The PageLayout integration is clean and consistent with the breadcrumb system. The
committeeNameis correctly passed from the GraphQL-fetched data, and the early return at lines 48-55 ensurescommitteeis not null.frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
31-31: LGTM!Adding the
usePathnamemock is necessary for the PageLayout integration in ChapterDetailsPage, as theuseBreadcrumbshook (used by PageLayout) depends onusePathnameto generate breadcrumb items. The mocked path/chapters/test-chaptercorrectly matches thechapterKeyfrom the existinguseParamsmock.frontend/__tests__/unit/components/BreadCrumbsWrapper.test.tsx (1)
1-148: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of BreadCrumbsWrapper functionality:
- ✅ Route detection logic (hiding on home and detail pages)
- ✅ Breadcrumb rendering on list pages
- ✅ Auto-generation logic (capitalization, dash replacement)
- ✅ Progressive path building for links
- ✅ Edge cases (trailing slashes, special characters)
The test structure is clear with well-organized describe blocks, and the assertions properly validate both visibility and rendering behavior.
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
113-145: PageLayout integration looks good.The PageLayout wrapper is correctly implemented with appropriate fallback chains for organization and repository names. The optional chaining at lines 116-118 properly handles cases where
repository.organizationmight be null.frontend/src/app/chapters/[chapterKey]/page.tsx (1)
64-77: LGTM!The PageLayout integration is clean and follows the established pattern. The
chapterNameis correctly passed from the GraphQL-fetched chapter data.frontend/src/components/PageLayout.tsx (1)
1-28: LGTM! Clean component design.The PageLayout component follows React best practices:
- ✅ Proper separation of concerns (delegates breadcrumb logic to
useBreadcrumbshook)- ✅ Client component correctly marked with
'use client'directive (required for Next.js 15 app router withusePathname)- ✅ Fragment wrapper avoids unnecessary DOM nesting
- ✅ Composition pattern with
childrenprop for flexible page contentThe interface design is pragmatic, allowing optional breadcrumb data fields that are resolved by the
useBreadcrumbshook based on the current route.frontend/src/components/BreadCrumbsWrapper.tsx (1)
10-17: Route pattern matching looks correct.The
ROUTES_WITH_PAGE_LAYOUTpatterns properly identify detail pages that use PageLayout component and should not show path-based breadcrumbs. The regex patterns correctly match:
- Single-segment detail pages:
/members/[key],/projects/[key], etc.- Nested repository pages:
/organizations/[key]/repositories/[key]This prevents double breadcrumb rendering on pages that have entity-specific breadcrumbs from PageLayout.
frontend/src/app/organizations/[organizationKey]/page.tsx (1)
117-131: Verify TypeScript satisfaction and type safety across state/useEffect.The code logic is correct at runtime: the guard at line 54 prevents reaching the return statement unless
graphQLData?.organizationexists, which triggerssetOrganization()in the useEffect. However, TypeScript may not recognize this connection becauseorganizationis a separate state variable initialized asnull(line 21), and the guard checksgraphQLData?.organizationrather than directly narrowing theorganizationstate variable.The type-safety concern raised in the original review is valid—accessing
organization.name,organization.description, andorganization.statsat lines 117-131 could trigger TypeScript errors depending on the configuration and how state narrowing is handled.Manual verification needed: Confirm whether TypeScript reports any errors for
organizationproperty access at lines 67-70, 79-91, and 115-131 in a proper build environment with TypeScript configured for the project.frontend/__tests__/unit/hooks/useBreadcrumbs.test.ts (1)
14-241: Great coverage of breadcrumb scenarios.The suite hits every branch of the hook, including nested repo routes, fallbacks, and trailing-slash handling. That gives strong confidence in the refactor.
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
…rmatting - Replace isDisabled with aria-current="page" for proper accessibility - Use consistent split/map/join capitalization for repository names
…rmatting - Replace isDisabled with aria-current="page" for proper accessibility - Use consistent split/map/join capitalization for repository names
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx
🧬 Code graph analysis (1)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
frontend/src/components/PageLayout.tsx (1)
PageLayout(19-28)
🔇 Additional comments (3)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (3)
10-10: LGTM!The import is correctly placed and used for formatting breadcrumb names.
20-20: LGTM!The PageLayout import is correctly placed and consistent with the breadcrumb refactoring across other detail pages.
130-146: LGTM!The DetailsCard props are correctly preserved, and wrapping it in PageLayout successfully adds breadcrumb functionality without affecting the existing detail page content.
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/BreadCrumbs.tsx (1)
11-57: Consider adding a guard for empty items.While the parent components should ensure
itemsis never empty, a defensive check would make the component more resilient:export default function BreadCrumbRenderer({ items }: BreadCrumbRendererProps) { + if (items.length === 0) { + return null + } + return ( <div className="mt-16 w-full pt-4">This is optional since the
useBreadcrumbshook should always provide at least a Home item according to the PR objectives.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/BreadCrumbs.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/components/BreadCrumbs.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/components/BreadCrumbs.tsx
🧬 Code graph analysis (1)
frontend/src/components/BreadCrumbs.tsx (1)
frontend/src/hooks/useBreadcrumbs.ts (1)
BreadCrumbItem(4-7)
🔇 Additional comments (2)
frontend/src/components/BreadCrumbs.tsx (2)
1-9: LGTM! Clean imports and interface definition.The type-only import on line 4 follows TypeScript best practices, and the rename of
BreadcrumbItemtoHeroUIBreadcrumbItemon line 3 properly avoids naming collisions with the customBreadCrumbItemtype. The interface is focused and well-typed.
30-52: Excellent refactor—previous accessibility issue properly resolved.The component now correctly implements breadcrumb semantics:
- Last item uses
aria-current="page"(line 38) instead of the previousaria-disabled="true"- No
isDisabledprop onHeroUIBreadcrumbItem(line 34)- Clean separation of concerns with logic moved to the
useBreadcrumbshook- Key using
item.pathis appropriate since breadcrumb paths are unique by natureThe refactor transforms this into a focused, pure renderer component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
34-144: Consider adding a breadcrumb rendering test.While the breadcrumb system is tested elsewhere, you might consider adding a test in this suite to verify that the chapter name appears in the breadcrumbs when rendered through PageLayout. This would provide integration-level coverage for the breadcrumb feature in the chapter details context.
Example test structure:
test('renders chapter name in breadcrumbs', async () => { render(<ChapterDetailsPage />) await waitFor(() => { // Verify breadcrumb contains chapter name from mockChapterDetailsData expect(screen.getByText(mockChapterDetailsData.chapter.name)).toBeInTheDocument() }) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/pages/ChapterDetails.test.tsx(1 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/members/[memberKey]/page.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/__tests__/unit/pages/ChapterDetails.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/ChapterDetails.test.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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/__tests__/unit/pages/ChapterDetails.test.tsx
🔇 Additional comments (1)
frontend/__tests__/unit/pages/ChapterDetails.test.tsx (1)
31-31: LGTM! Mock addition supports breadcrumb integration.The
usePathnamemock is correctly added to support the new breadcrumb system that PageLayout uses when wrapping ChapterDetailsPage. The returned path '/chapters/test-chapter' is consistent with the test data'schapterKey.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Thank you @Kunal1522 !
It works as expected for most part.. but I do have some concerns for the code. Left a few comments below ⬇️
| const isNestedRepoRoute = | ||
| segments.length === 4 && segments[0] === 'organizations' && segments[2] === 'repositories' |
There was a problem hiding this comment.
Where is this 4 coming from? What if we have more then 4 nested items? 🤔
We want to make it as dynamic as possible
|
|
||
| return ( | ||
| <BreadcrumbItem key={href} isDisabled={isLast}> | ||
| <HeroUIBreadcrumbItem key={item.path}> |
There was a problem hiding this comment.
HeroUIBreadcrumbItem doesn't use isDisabled prop, but BreadCrumbsWrapper.tsxon line 58 does, causing inconsistent behavior. 🤔
Think on adding isDisabled={isLast} to match BreadCrumbsWrapper
| path: string | ||
| } | ||
|
|
||
| interface BreadcrumbData { |
There was a problem hiding this comment.
BreadcrumbData interface is duplicated in two files, violating DRY.
It's added here and in PageLayout.tsx.
Also, we are trying to get away from using interface and want to use type instead. Please move this into types/ folder and use where needed to avoid code duplication
| if (isNestedRepoRoute) { | ||
| breadcrumbs.push({ title: 'Organizations', path: '/organizations' }) | ||
|
|
||
| const orgTitle = breadcrumbData?.orgName || upperFirst(segments[1]).replaceAll('-', ' ') |
There was a problem hiding this comment.
I can see you use the same logic several times. Can we move this into a function instead? Lines 32, 38, 48.
| const ROUTES_WITH_PAGE_LAYOUT = [ | ||
| /^\/members\/[^/]+$/, | ||
| /^\/projects\/[^/]+$/, | ||
| /^\/chapters\/[^/]+$/, | ||
| /^\/committees\/[^/]+$/, | ||
| /^\/organizations\/[^/]+$/, | ||
| /^\/organizations\/[^/]+\/repositories\/[^/]+$/, | ||
| ] |
There was a problem hiding this comment.
I'm also not super happy with these hardcoded values. If we ever change our project structure - this might fail. Is there a better way to find a match? 🤔
| import Link from 'next/link' | ||
| import { usePathname } from 'next/navigation' | ||
|
|
||
| const ROUTES_WITH_PAGE_LAYOUT = [ |
There was a problem hiding this comment.
I also think this route detection logic is somewhat duplicated: regex patterns in here vs string matching in useBreadcrumbs 🤔
| }) | ||
|
|
||
| test('returns null for member detail pages', () => { | ||
| ;(usePathname as jest.Mock).mockReturnValue('/members/bjornkimminich') |
There was a problem hiding this comment.
Can we use users/project names that we already added to cspell to not add new items? Or just some fake ones, that would be even better.
|
|
@Kunal1522 I pushed some changes to address issues I found while reviewing your PR. The one I noticed was not connected to your changes, but other 2 were from running Could you also address 5 newly added issues found by SonarQube? Here's the list |
|
@Kunal1522 Hi! Are you planning to address the comments in this PR? The deadline for this issue passed a while ago (Nov 8th). If the updates aren’t made within the next day, I’ll need to reassign the issue to someone else. Thanks! |
|
Hi, I’m sorry for the late reply. I have my end-term exams going on right now, so I won’t be able to contribute until 25th November. However I went through the PR comments and completed all the requested changes, including the SonarQube ones. Before committing, the only part I wanted to improve was making the breadcrumbs more dynamic and easier to extend. Right now, the only path that appears in the breadcrumb but does not actually exist is: organisations/[slug]/repositories. I had to hard-code it, but similar cases may be added in the future. One solution I found is to run a script at build time that walks through the app directory and stores all existing routes in a JSON file. This JSON can be checked whenever needed. This would help in two ways: validating multi-level slugged routes,(so i would not need regex pattern matching ) To make things more extensible, I also moved the configuration to a types file. So if more pages are added later, we can just update the types, and everything will adjust automatically. Please assign this issue to someone else if they volunteer otherwise i will fix this after 26th November. |
2 participants
Tasks
/repositoriesto appear in breadcrumb as the page does not really existBreadCrumbs.tsx,BreadCrumbsWrapper.tsxResolves #1425
Proposed change
Display actual entity names in breadcrumbs instead of URL slugs.
Currently, breadcrumbs show URL-formatted text like "Bjornkimminich" instead of the real name "Björn Kimminich" from the database. This PR fixes that by having detail pages pass their GraphQL data to the breadcrumb system through a simple interface.
Pages now pass entity names like this:
The breadcrumb checks which field is set and displays that name. If no data is provided, it falls back to formatting the URL slug nicely.
Also refactored breadcrumb logic into reusable components (
BreadCrumbs,BreadCrumbsWrapper,useBreadcrumbshook) and added 72 tests.Detailed PR
Problem
Breadcrumbs were showing URL slugs instead of actual entity names from the database.
Solution
Created Reusable Breadcrumb System
Three main pieces:
BreadCrumbs.tsx- Pure UI component that renders breadcrumb items.BreadCrumbsWrapper.tsx- Renders breadcrumbs. It takes an array of items and displays them nicely with proper styling and navigation links.useBreadcrumbshook - Contains all the logic for building breadcrumbs. It handles both simple routes (/projects/zap) and nested routes (/organizations/x/repositories/y).Pages Pass Real Names via Props
Each detail page already fetches GraphQL data. Now they pass the entity name to
PageLayout:The interface accepts different entity types:
The hook checks which field is present and uses it. If none is provided, it falls back to formatting the URL slug.
Special Handling for Nested Routes
Repository routes (
/organizations/[org]/repositories/[repo]) get special detection. The breadcrumb displays actual org and repo names, and skips the "Repositories" segment since that page doesn't exist.Testing
Added 72 tests covering rendering, auto-generation, real name display, fallbacks, and edge cases. All pass.
BEFORE
AFTER
BEFORE
AFTER
Checklist
make check-testlocally; all checks and tests passed.