Unify issues view between module issues page and Mentee page#3050
Unify issues view between module issues page and Mentee page#3050
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a reusable IssuesTable component and IssueRow type; replaces inline issue rendering in two pages with IssuesTable, adds client-side pagination and status filtering on the mentee page, extends GraphQL queries to include isMerged, and updates/expands related unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (7)
frontend/__tests__/unit/components/IssuesTable.test.tsx (2)
247-254: Consider removing or differentiating this duplicate test.This test case is functionally identical to the "calls onIssueClick when provided" test (lines 238-245). Both tests render the same component, click the same element, and verify the same callback invocation. To add value, consider either:
- Removing this test as redundant, or
- Differentiating by simulating a narrower viewport or verifying mobile-specific behavior
285-303: Tooltip tests don't verify actual tooltip behavior.These tests verify that buttons render for long and short titles but don't actually assert tooltip behavior (e.g., tooltip visibility,
isDisabledprop effect). The test names suggest tooltip verification but the assertions only confirm button presence.Consider adding assertions that verify the tooltip is disabled for short titles or visible for long titles, perhaps by querying for tooltip-related attributes or using a more detailed mock.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (2)
27-39: Consider extracting the inline type to a shared definition.This inline type closely mirrors parts of
IssueRowfromIssuesTable.tsx. While not blocking, extracting this to a shared type or reusing existing types would improve maintainability and reduce duplication.
68-78: Assignees are fetched but not mapped toIssueRow.The GraphQL query fetches
assigneesdata (line 37), but theuseMemomapping doesn't include them in the resultingIssueRow[]. While this works becauseshowAssignee={false}on line 218, including the assignees in the mapping would maintain data completeness and avoid issues if the display option changes in the future.🔎 Proposed fix
const menteeIssues: IssueRow[] = useMemo(() => { return menteeIssuesData.map((issue) => ({ objectID: issue.id, number: issue.number, title: issue.title, state: issue.state, isMerged: issue.isMerged, labels: issue.labels || [], url: issue.url, + assignees: issue.assignees || [], })) }, [menteeIssuesData])frontend/src/components/IssuesTable.tsx (2)
175-178: Display preference should favornameoverlogin.The current logic
login || nameprefers the GitHub login over the display name. Typically, the human-readable name is preferred for display with login as the fallback when name is empty.🔎 Proposed fix
<span className="max-w-[80px] truncate sm:max-w-[100px] md:max-w-[120px] lg:max-w-[150px]"> - {issue.assignees[0].login || issue.assignees[0].name} + {issue.assignees[0].name || issue.assignees[0].login} {issue.assignees.length > 1 && ` +${issue.assignees.length - 1}`} </span>
146-152: Consider using a unique key for label elements.Using
labelas the key could cause issues if duplicate labels exist in the array (though unlikely). Consider using the index or a combination of label and index for guaranteed uniqueness.🔎 Proposed fix
-{issue.labels.slice(0, maxVisibleLabels).map((label) => ( +{issue.labels.slice(0, maxVisibleLabels).map((label, index) => ( <span - key={label} + key={`${label}-${index}`} className="inline-flex items-center rounded-md bg-gray-100 px-2 py-0.5 text-xs text-gray-700 lg:rounded-lg lg:border lg:border-gray-400 lg:bg-transparent lg:hover:bg-gray-200 dark:bg-gray-800 dark:text-gray-300 dark:lg:border-gray-300 dark:lg:hover:bg-gray-700" > {label} </span> ))}frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx (1)
104-109: Type assertion may fail for non-element children.The
React.Children.mapcallback casts each child asReact.ReactElement, which could cause runtime issues if non-element children (like strings or nulls) are passed. Consider adding a type guard.🔎 Proposed fix
{React.Children.map(children, (child) => { + if (!React.isValidElement(child)) { + return child + } const itemKey = String(child.key ?? '') return React.cloneElement(child, { 'data-key': itemKey, onClick: () => handleItemClick(itemKey), } as Partial<unknown>) })}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/menteeQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (7)
frontend/__tests__/unit/components/IssuesTable.test.tsxfrontend/__tests__/unit/pages/IssuesPage.test.tsxfrontend/__tests__/unit/pages/MenteeProfilePage.test.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsxfrontend/src/components/IssuesTable.tsxfrontend/src/server/queries/menteeQueries.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
📚 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/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsxfrontend/__tests__/unit/pages/MenteeProfilePage.test.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 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/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/IssuesTable.test.tsxfrontend/__tests__/unit/pages/MenteeProfilePage.test.tsxfrontend/__tests__/unit/pages/IssuesPage.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/MenteeProfilePage.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/MenteeProfilePage.test.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/pages/MenteeProfilePage.test.tsxfrontend/__tests__/unit/pages/IssuesPage.test.tsx
🧬 Code graph analysis (3)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (1)
frontend/src/components/IssuesTable.tsx (1)
IssueRow(8-18)
frontend/__tests__/unit/components/IssuesTable.test.tsx (1)
frontend/src/components/IssuesTable.tsx (1)
IssueRow(8-18)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (2)
frontend/src/components/IssuesTable.tsx (1)
IssueRow(8-18)backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)
⏰ 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)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (16)
frontend/src/server/queries/menteeQueries.ts (1)
25-25: LGTM!The
isMergedfield addition aligns with theIssueRowtype definition in the newIssuesTablecomponent, enabling proper merged state display in the unified issues view.frontend/__tests__/unit/pages/IssuesPage.test.tsx (2)
92-92: LGTM!The assertion update correctly reflects the unified empty state rendering from the
IssuesTablecomponent, which now displays the message once rather than in multiple places.
261-262: LGTM!Using regex pattern
/\+1/withgetAllByTextappropriately handles the "+1" indicator that may appear in both desktop and mobile views rendered byIssuesTable.frontend/__tests__/unit/components/IssuesTable.test.tsx (1)
1-27: LGTM!Well-structured mocks for Next.js navigation and Image components. The mock setup appropriately isolates the component for unit testing.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (2)
114-119: LGTM!Using
window.openwithnoopener,noreferrerfor external GitHub URLs is appropriate and secure. This correctly differs from the IssuesPage which navigates to an internal detail page.
185-211: LGTM!The Select component implementation with proper
aria-label, selection handling, and page reset on filter change follows good UX and accessibility patterns.frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/page.tsx (3)
41-51: LGTM!Clean data transformation to
IssueRow[]with properuseMemomemoization. The mapping includes all relevant fields includingassigneesfor display in the table.
84-91: LGTM!Proper use of
useCallbackwith correct dependencies. This prevents unnecessary re-renders of theIssuesTablecomponent when the parent re-renders.
134-139: LGTM!The
IssuesTableintegration consolidates the previous inline rendering logic into a clean, reusable component with consistent props across both pages.frontend/src/components/IssuesTable.tsx (3)
8-18: LGTM!Well-defined and exported
IssueRowtype that provides a consistent contract for issue data across consuming components.
41-47: LGTM!Clear precedence logic:
onIssueClickcallback takes priority, falling back toissueUrlfor router navigation. This provides flexibility for different navigation behaviors across pages.
49-66: LGTM!Robust status badge implementation with proper handling of the merged state override and a sensible fallback to "Closed" for unknown states.
frontend/__tests__/unit/pages/MenteeProfilePage.test.tsx (4)
41-60: LGTM!Clean Tooltip mock that properly handles the
isDisabledprop to return children directly or wrap them with test attributes.
62-139: Well-crafted Select component mock.The mock properly simulates open/close state, selection changes, and item rendering, enabling robust interaction testing. This is a good pattern for testing components that use complex UI libraries.
228-240: LGTM!Adding explicit null checks with descriptive error messages improves test reliability and debugging when the DOM structure doesn't match expectations.
251-274: LGTM!Updated filter tests correctly use the new test IDs and verify filtering behavior through the custom Select mock. The assertions properly check for presence/absence of filtered issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (1)
108-113: Consider memoizing the click handler for performance.The implementation is correct and secure (good use of
noopener,noreferrer). However, since this function is passed to IssuesTable, consider wrapping it inuseCallbackto prevent unnecessary re-renders when the menteeIssues reference changes due to pagination or filtering.🔎 Proposed optimization
- const handleIssueClick = (issueNumber: number) => { + const handleIssueClick = useCallback((issueNumber: number) => { const issue = menteeIssues.find((i) => i.number === issueNumber) if (issue?.url) { window.open(issue.url, '_blank', 'noopener,noreferrer') } - } + }, [menteeIssues])frontend/__tests__/unit/components/IssuesTable.test.tsx (3)
112-120: Consider more specific assertions.While the test correctly verifies that issue rows are rendered, checking
length > 0is somewhat loose. Consider being more explicit about the expected count for more reliable tests.🔎 Example of more specific assertion
it('renders all issue rows', () => { render(<IssuesTable {...defaultProps} />) const issue1Buttons = screen.getAllByRole('button', { name: /Test Issue 1/i }) const issue2Buttons = screen.getAllByRole('button', { name: /Test Issue 2/i }) const issue3Buttons = screen.getAllByRole('button', { name: /Test Issue 3/i }) - expect(issue1Buttons.length).toBeGreaterThan(0) - expect(issue2Buttons.length).toBeGreaterThan(0) - expect(issue3Buttons.length).toBeGreaterThan(0) + // Each issue appears in both desktop and mobile views + expect(issue1Buttons).toHaveLength(2) + expect(issue2Buttons).toHaveLength(2) + expect(issue3Buttons).toHaveLength(2) })
185-190: Consider more robust DOM queries.The use of
closest('tr')?.querySelector('td:nth-child(3)')is fragile and depends on specific DOM structure. Consider using more semantic queries that are less likely to break with structural changes.🔎 Alternative approach
it('does not render labels section when labels array is empty', () => { render(<IssuesTable issues={[mockIssues[2]]} />) - const issueTitles = screen.getAllByText('Test Issue 3') - const labelsCell = issueTitles[0].closest('tr')?.querySelector('td:nth-child(3)') - expect(labelsCell?.textContent).toBe('') + // Verify no label badges are rendered for this issue + const table = screen.getByRole('table') + const rows = within(table).getAllByRole('row') + const issueRow = rows.find(row => row.textContent?.includes('Test Issue 3')) + const labelBadges = issueRow ? within(issueRow).queryAllByRole('status') : [] + expect(labelBadges).toHaveLength(0) })Note: This assumes labels use appropriate ARIA roles. If not, you might need to query by test-id or class.
230-235: Similar DOM query fragility concern.The same fragile querying pattern appears here with
querySelector('td:nth-child(4)'). Consider applying the same more semantic querying approach suggested for the labels tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/IssuesTable.test.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2000
File: backend/apps/mentorship/management/commands/sync_module_issues.py:109-145
Timestamp: 2025-08-17T11:55:55.990Z
Learning: In the OWASP/Nest mentorship system, tasks are designed to be assigned to only one assignee per issue, even if GitHub issues can have multiple assignees. The sync_module_issues command correctly uses issue.assignees.first() to create one task per issue for the first assignee only.
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/IssuesTable.test.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 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/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.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/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx
🧬 Code graph analysis (2)
frontend/__tests__/unit/components/IssuesTable.test.tsx (1)
frontend/src/components/IssuesTable.tsx (1)
IssueRow(8-18)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (3)
frontend/src/types/__generated__/menteeQueries.generated.ts (1)
GetModuleMenteeDetailsQuery(11-11)frontend/src/components/IssuesTable.tsx (1)
IssueRow(8-18)backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)
⏰ 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). (2)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (16)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/mentees/[menteeKey]/page.tsx (6)
4-21: LGTM! Well-organized imports and constants.The new imports for Select/SelectItem, IssuesTable, and Pagination are properly organized, and the ITEMS_PER_PAGE constant at module level is a good practice for configurability.
30-36: State management is well-typed and initialized correctly.The state declarations properly leverage TypeScript inference from the GraphQL types, ensuring type safety throughout the component.
86-106: LGTM! Clean filtering and pagination logic.The filtering logic correctly handles case-insensitive state comparison, and the pagination calculation is accurate. The status filter options with counts provide good user feedback.
179-205: Select component configuration looks good.The Select component is properly configured with single selection mode, appropriate styling, and the onSelectionChange handler correctly resets pagination when the filter changes—good UX decision.
210-223: IssuesTable and Pagination integration is well-implemented.The props passed to IssuesTable and Pagination are correct. Setting
showAssignee={false}makes sense in the mentee context, and the custom empty message provides good user feedback.
61-72: No action required. The GraphQL querygetMenteeModuleIssuesincludes all fields used in the data transformation, includingisMerged. The mapping is complete and correct.frontend/__tests__/unit/components/IssuesTable.test.tsx (10)
6-48: LGTM! Well-structured test mocks.The mocks for Next.js router, Image, and HeroUI Tooltip are appropriate for isolated component testing. The Tooltip mock correctly handles the
isDisabledprop and exposesdata-contentfor verification.
50-98: Comprehensive mock data covering key scenarios.The mock issues effectively cover different states (open, closed, merged), varying label and assignee counts, and all required IssueRow fields. This provides good test coverage foundation.
131-162: LGTM! Good coverage of status badge variations.The tests correctly verify Open, Closed, and Merged status badges, and the test for unknown states defaulting to Closed is a valuable edge case.
237-255: Excellent edge case coverage.The test for falling back to
loginwhennameis empty is particularly valuable and demonstrates thorough edge case consideration.
258-267: LGTM! Click handler test is clear and effective.The test correctly verifies that the
onIssueClickcallback is invoked with the expected issue number.
269-295: LGTM! Comprehensive empty state coverage.The tests effectively verify both default and custom empty messages across desktop and mobile views, ensuring good user experience in edge cases.
297-325: LGTM! Tooltip tests validate the 50-character threshold.The tests properly verify that tooltips appear for long titles and are disabled for short ones, ensuring good UX for title truncation.
327-358: LGTM! Good responsive behavior coverage.The tests verify that essential information (status, labels, assignees) is appropriately rendered in mobile view, with reasonable limits (3 labels) for smaller screens.
360-374: LGTM! Clean column count validation.The tests use semantic queries (
getAllByRole('columnheader')) and correctly verify the table structure adapts to theshowAssigneeprop.
376-399: LGTM! Important default props validation.Testing default prop values is crucial for API stability and preventing regressions. These tests provide good documentation of expected default behavior.
|
| import { useRouter } from 'next/navigation' | ||
| import type React from 'react' | ||
|
|
||
| export type IssueRow = { |
2 participants
Updated the Mentorship portal Mentee page to display assigned issues using the same table view as the module page. Introduced a reusable
IssuesTable.tsxcomponent shared across multiple views.Checklist
make check-testlocally and all tests passed