Fix #1882: Added test for RecentIssues Component #2087
Conversation
Summary by CodeRabbit
WalkthroughAdds a comprehensive unit test suite for RecentIssues and removes the explicit empty-string fallback when constructing the repository navigation URL in RecentIssues.tsx. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/src/components/RecentIssues.tsx (2)
35-49: Harden navigation: also guard on organizationName and URL-encode path segments.If upstream data ever omits organizationName, clicking would navigate to /organizations/undefined/repositories/. Also, encoding segments avoids issues with spaces or special characters in names.
Apply this diff within the current block:
- {item?.repositoryName && ( + {item?.repositoryName && item?.organizationName && ( <div className="flex flex-1 items-center overflow-hidden"> <FontAwesomeIcon icon={faFolderOpen} className="mr-2 h-5 w-4" /> <button className="cursor-pointer overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" - onClick={() => - router.push( - `/organizations/${item.organizationName}/repositories/${item.repositoryName}` - ) - } + onClick={() => { + const org = encodeURIComponent(item.organizationName as string) + const repo = encodeURIComponent(item.repositoryName as string) + router.push(`/organizations/${org}/repositories/${repo}`) + }} > <TruncatedText text={item.repositoryName} /> </button> </div> )}
38-47: Prefer Next.js Link over button + router.push for navigation (a11y + UX).Links are semantically correct for navigation, enable open-in-new-tab, and can prefetch. Consider replacing the button with Link.
Example change inside this block:
- <button - className="cursor-pointer overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" - onClick={() => - router.push( - `/organizations/${item.organizationName}/repositories/${item.repositoryName}` - ) - } - > - <TruncatedText text={item.repositoryName} /> - </button> + <a + href={`/organizations/${encodeURIComponent(item.organizationName!)}/repositories/${encodeURIComponent(item.repositoryName!)}`} + className="cursor-pointer overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" + > + <TruncatedText text={item.repositoryName} /> + </a>If you go this route, you can also use Next.js
<Link>for prefetching:// at top-level imports import Link from 'next/link'<Link href={`/organizations/${encodeURIComponent(item.organizationName!)}/repositories/${encodeURIComponent(item.repositoryName!)}`} className="cursor-pointer overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" > <TruncatedText text={item.repositoryName} /> </Link>frontend/__tests__/unit/components/RecentIssues.test.tsx (6)
107-113: Prefer role-based queries and userEvent for ergonomic, semantic interactions.Querying by role improves resilience, and userEvent better simulates user behavior.
Apply this diff:
- it('renders repositoryName and navigates on click', () => { - render(<RecentIssues data={[baseIssue]} />) - const repoBtn = screen.getByText('repo') - expect(repoBtn).toBeInTheDocument() - fireEvent.click(repoBtn) - expect(mockPush).toHaveBeenCalledWith('/organizations/org/repositories/repo') - }) + it('renders repositoryName and navigates on click', async () => { + render(<RecentIssues data={[baseIssue]} />) + const repoBtn = screen.getByRole('button', { name: 'repo' }) + expect(repoBtn).toBeInTheDocument() + await userEvent.click(repoBtn) + expect(mockPush).toHaveBeenCalledWith('/organizations/org/repositories/repo') + })Add the import at the top:
import userEvent from '@testing-library/user-event'
115-120: Avoid deleting properties on strongly typed objects.Using delete on a typed object can fight TS’ structural typing and strictness. Prefer creating a new object without the key.
One approach:
- const issue = { ...baseIssue } - delete issue.repositoryName - render(<RecentIssues data={[issue]} />) + const { repositoryName: _omit, ...rest } = baseIssue + // If Issue requires repositoryName, cast only at callsite to keep the local object well-typed. + render(<RecentIssues data={[rest as unknown as Issue]} />)Alternatively, explicitly opt out for that line only:
const issue: any = { ...baseIssue } delete issue.repositoryName render(<RecentIssues data={[issue]} />)
161-165: Test name says “truncates” but the assertion doesn’t verify truncation.Right now it only checks the text is present. Consider asserting the truncation-related classes on the clickable element.
Apply this diff:
- it('renders with long repositoryName and truncates', () => { + it('renders with long repositoryName and applies truncation classes', () => { const issue = { ...baseIssue, repositoryName: 'a'.repeat(100) } render(<RecentIssues data={[issue]} />) - expect(screen.getByText('a'.repeat(100))).toBeInTheDocument() + const btn = screen.getByRole('button', { name: 'a'.repeat(100) }) + expect(btn).toHaveClass('overflow-hidden') + expect(btn).toHaveClass('text-ellipsis') + expect(btn).toHaveClass('whitespace-nowrap') })
149-152: DOM structure assertion is brittle; prefer targeted queries.closest('div') + toHaveClass('flex') couples to implementation details. Consider asserting on the known title container or heading role, or using getByRole('heading', { name: /Recent Issues/i }) and checking its container via a data-testid.
Example:
- expect(screen.getByText('Recent Issues').closest('div')).toHaveClass('flex') + const heading = screen.getByRole('heading', { name: /Recent Issues/i }) + expect(heading).toBeInTheDocument() + // If needed, add a data-testid to the title container for a stable assertion.
167-172: Consider adding a test for missing organizationName to avoid broken routes.The component currently doesn’t guard organizationName in the render condition, so a missing org could navigate to /organizations/undefined/.... Adding a test will prevent regressions.
Suggested test:
it('does not render repository button if organizationName is missing', () => { const issue: any = { ...baseIssue, organizationName: undefined } render(<RecentIssues data={[issue]} />) expect(screen.queryByRole('button', { name: 'repo' })).not.toBeInTheDocument() })If you adopt this behavior, update the component guard accordingly as suggested in the component review.
52-56: Minor: clear all mocks after each test for consistency.You already clear mockPush. Clearing all mocks avoids accidental leakage when more mocks get added.
Example:
afterEach(() => { jest.clearAllMocks() })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
frontend/__tests__/unit/components/RecentIssues.test.tsx(1 hunks)frontend/src/components/RecentIssues.tsx(1 hunks)
🔇 Additional comments (2)
frontend/src/components/RecentIssues.tsx (1)
41-43: Safe removal of repositoryName fallback — guarded by render condition (LGTM).Because the button is only rendered when item.repositoryName is truthy, dropping the || '' fallback is safe and makes the URL construction clearer.
frontend/__tests__/unit/components/RecentIssues.test.tsx (1)
1-188: Comprehensive, thoughtful coverage — great job.The suite meaningfully exercises rendering, conditional paths, interactions, and edge cases. Mocks are minimal and targeted; navigation assertions align with the component behavior.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 Could you run make check locally and address found issues? Thanks!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
frontend/__tests__/unit/components/RecentIssues.test.tsx (10)
52-56: Reset all mocks between tests to avoid cross-test leakageUsing mockPush.mockClear() only clears that one mock’s calls. Prefer clearing all mocks at the start of each test.
Apply this diff:
const mockPush = jest.fn() beforeEach(() => { - ;(useRouter as jest.Mock).mockReturnValue({ push: mockPush }) - mockPush.mockClear() + jest.clearAllMocks() + ;(useRouter as jest.Mock).mockReturnValue({ push: mockPush }) })
107-113: Prefer userEvent for click interactions; additionally assert role for robustnessfireEvent.click works but userEvent better simulates user behavior. Also consider asserting by role ('link' or 'button') rather than raw text to reduce brittleness.
Example refactor:
-import { render, screen, fireEvent } from '@testing-library/react' +import { render, screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event' @@ - const repoBtn = screen.getByText('repo') + const repoBtn = screen.getByRole(/link|button/, { name: 'repo' }) expect(repoBtn).toBeInTheDocument() - fireEvent.click(repoBtn) + const user = userEvent.setup() + await user.click(repoBtn) expect(mockPush).toHaveBeenCalledWith('/organizations/org/repositories/repo')Note: mark the test async and create a user instance.
115-120: Also assert no navigation occurs when repositoryName is missingStrengthen the negative case by verifying push is not called.
Apply this diff:
delete issue.repositoryName render(<RecentIssues data={[issue]} />) expect(screen.queryByText('repo')).not.toBeInTheDocument() + expect(mockPush).not.toHaveBeenCalled()
122-125: Make the date assertion locale-robustHardcoding "Mar" is locale-dependent and brittle. Compute the expected month/year from the test data instead.
Apply this diff:
- it('renders formatted date', () => { - render(<RecentIssues data={[baseIssue]} />) - expect(screen.getByText(/Mar \d{1,2}, 2024/)).toBeInTheDocument() - }) + it('renders formatted date', () => { + render(<RecentIssues data={[baseIssue]} />) + const created = new Date(baseIssue.createdAt * 1000) + const expMonth = created.toLocaleString('en-US', { month: 'short' }) + const expYear = created.getFullYear() + expect( + screen.getByText(new RegExp(`${expMonth} \\d{1,2}, ${expYear}`)) + ).toBeInTheDocument() + })
127-131: Test name mismatches the assertion; consider renaming or asserting labelsThe test named “renders label text” asserts the title. Either rename to “renders title text” or assert that the label “bug” is rendered too.
Apply this diff to rename and add a dedicated labels test:
- it('renders label text', () => { + it('renders title text', () => { render(<RecentIssues data={[baseIssue]} />) expect(screen.getByText('Issue Title')).toBeInTheDocument() }) + + it('renders labels', () => { + render(<RecentIssues data={[baseIssue]} />) + expect(screen.getByText('bug')).toBeInTheDocument() + })
149-152: DOM/className assertion is brittle; avoid structural couplingAsserting closest('div') has class 'flex' tightly couples tests to implementation details. Prefer roles/testids or behavior. Consider removing or switching to a stable testid if the component provides one.
154-160: Strengthen “renders multiple issues” assertionsThe current regex + count can pass in unintended scenarios. Assert each title explicitly.
Apply this diff:
render(<RecentIssues data={issues} />) expect(screen.getByText('Second Issue')).toBeInTheDocument() - expect(screen.getAllByText(/Issue Title|Second Issue/).length).toBeGreaterThan(1) + expect(screen.getByText('Issue Title')).toBeInTheDocument() + expect(screen.getByText('Second Issue')).toBeInTheDocument()
161-165: Test name contradicts assertion; clarify intentThe name says “truncates” but the assertion checks full text presence. If truncation is visual via CSS, it’s fine to assert text presence and rename the test for clarity.
Apply this diff:
- it('renders with long repositoryName and truncates', () => { + it('renders long repositoryName text (present in DOM; may be visually truncated via CSS)', () => { const issue = { ...baseIssue, repositoryName: 'a'.repeat(100) } render(<RecentIssues data={[issue]} />) expect(screen.getByText('a'.repeat(100))).toBeInTheDocument() })
174-177: “Missing props gracefully” test doesn’t validate behaviorThis ensures no crash, which is useful, but consider adding a concrete behavior assertion (e.g., empty state) to increase signal.
184-187: Duplicate coverage with earlier avatar testsThis duplicates the default avatar behavior already implied by the "shows avatar when showAvatar is true" plus the default props path. Consider removing to reduce runtime.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/__tests__/unit/components/RecentIssues.test.tsx(1 hunks)
🔇 Additional comments (6)
frontend/__tests__/unit/components/RecentIssues.test.tsx (6)
7-9: Router mock setup looks goodClean, focused mock for next/navigation's useRouter.
27-50: Solid Next/Image mockGood approach to sidestep Next's Image in JSDOM while preserving alt and basic style semantics.
144-147: Accessibility heading check is appropriateGood to assert by role for the main label.
167-172: Custom organization navigation LGTMGood coverage for org override and correct path formation.
179-182: Null data empty state LGTMCovers a valuable edge case.
58-83: No action needed – numeric timestamps in seconds are handled correctly
formatDatemultiplies numeric inputs by 1000 (treating them as Unix seconds), and theIssuetype definescreatedAt/updatedAtas numbers in seconds. The 10-digit timestamps in the tests align with this and will render reliably.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
These look good 👌🏼 Thanks!
* Added test for RecentIssues Component * Updated code format * update missing title test --------- Co-authored-by: Kate Golovanova <[email protected]>
2 participants
Resolves #1882
Description
Added Unit Tests for RecentIssues Component
Added comprehensive test suite with 18 test cases covering:
Checklist
make check-testlocally; all checks and tests passed.