Conversation
Summary by CodeRabbit
WalkthroughIntroduced an internal AuthorAvatar component in ItemCardList that centralizes avatar rendering: renders a linked Image when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/ItemCardList.tsx`:
- Around line 64-80: The current conditional in ItemCardList allows rendering
the Link when item.author.name exists but item.author.login is missing, causing
a broken href; change the condition to require item?.author?.login before
rendering the Link (e.g., render Link + Image only when item?.author?.login is
truthy), and otherwise render a non-link avatar/fallback (use Image or FaUser)
with the same alt/text fallback (item.author.name || item.author.login) so no
/members/undefined is produced; update usages of Link, Image and FaUser
accordingly.
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ItemCardList.test.tsx (1)
718-758: Assert the fallback icon is rendered (not just avatar absence).The test names say “shows fallback icon,” but the assertions only verify that the avatar image is missing. Consider asserting the icon’s presence by adding a
data-testidoraria-labelto the icon in the component.✅ Suggested test assertion + small component hook
- const avatarImage = screen.queryByTestId('avatar-image') - expect(avatarImage).not.toBeInTheDocument() + const avatarImage = screen.queryByTestId('avatar-image') + expect(avatarImage).not.toBeInTheDocument() + expect(screen.getByTestId('avatar-fallback-icon')).toBeInTheDocument()- <FaUser className="mr-2 h-6 w-6 text-gray-400 dark:text-gray-500" /> + <FaUser + className="mr-2 h-6 w-6 text-gray-400 dark:text-gray-500" + data-testid="avatar-fallback-icon" + aria-label="Unknown author" + />
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/ItemCardList.tsx">
<violation number="1" location="frontend/src/components/ItemCardList.tsx:31">
P1: Missing check for `avatarUrl` before rendering the Image. If author has `name` or `login` but no `avatarUrl` (possible with GitHub Copilot or other agents), the Image component will receive undefined/empty src, causing a Next.js Image error. Consider checking `author.avatarUrl` exists before rendering the Image, or fall back to the FaUser icon.</violation>
</file>
<file name="frontend/__tests__/unit/components/RecentIssues.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/RecentIssues.test.tsx:210">
P2: This test is titled “shows fallback icon…” but only asserts the avatar image is absent. It doesn’t verify the fallback icon is rendered, so the test would still pass if nothing is shown. Add an assertion for the fallback icon within the avatar tooltip.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| render(<RecentIssues data={[issueWithEmptyAuthor]} />) | ||
| expect(screen.getByAltText("Author's avatar")).toBeInTheDocument() | ||
| const avatarImage = screen.queryByAltText("Author's avatar") |
There was a problem hiding this comment.
P2: This test is titled “shows fallback icon…” but only asserts the avatar image is absent. It doesn’t verify the fallback icon is rendered, so the test would still pass if nothing is shown. Add an assertion for the fallback icon within the avatar tooltip.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/__tests__/unit/components/RecentIssues.test.tsx, line 210:
<comment>This test is titled “shows fallback icon…” but only asserts the avatar image is absent. It doesn’t verify the fallback icon is rendered, so the test would still pass if nothing is shown. Add an assertion for the fallback icon within the avatar tooltip.</comment>
<file context>
@@ -207,6 +207,7 @@ describe('<RecentIssues />', () => {
}
render(<RecentIssues data={[issueWithEmptyAuthor]} />)
- expect(screen.getByAltText("Author's avatar")).toBeInTheDocument()
+ const avatarImage = screen.queryByAltText("Author's avatar")
+ expect(avatarImage).not.toBeInTheDocument()
})
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/ItemCardList.tsx`:
- Around line 22-52: In AuthorAvatar, guard against a missing/empty avatarUrl
before rendering next/image: update the hasAuthorInfo logic (or add a new
condition) to verify author?.avatarUrl is truthy and only create/use
imageElement when avatarUrl exists; if avatarUrl is falsy, return the FaUser
fallback (wrapped in the same Link when author.login exists, or in a div
otherwise) so next/image is never called with an undefined/empty src (affecting
the AuthorAvatar component and the imageElement usage).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3742 +/- ##
=======================================
Coverage 87.70% 87.70%
=======================================
Files 462 462
Lines 14277 14287 +10
Branches 1902 1905 +3
=======================================
+ Hits 12521 12530 +9
Misses 1344 1344
- Partials 412 413 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 thanks for updating this! 👍🏼
2 participants
Proposed change
This fixes the UI break in the Recent Pull Requests card so it renders correctly even when the PR is opened by GitHub Copilot or other agent.
Resolves #3733
Checklist
make check-testlocally: all warnings addressed, tests passed