generated a Release compoenent which is removing the code duplicacy a… #2179
Conversation
…nd used a single compoenent in recentRelease and new release on snapshot page
Summary by CodeRabbit
WalkthroughAdds a reusable Release component, updates RecentReleases and snapshots page to render releases via that component, extends the snapshot GraphQL selection and test fixtures with organization/repository/author fields, and adds unit tests for the Release component covering rendering and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ 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: 2
🧹 Nitpick comments (12)
frontend/src/app/snapshots/[id]/page.tsx (1)
168-173: Use a stable key; drop redundant prop
- Prefer a deterministic key over index/tagName to avoid reconciliation issues.
- showAvatar defaults to true; no need to pass explicitly.
- <Release - key={`${release.tagName}-${index}`} - release={release} - showAvatar={true} - index={index} - /> + <Release + key={`release-${release.organizationName ?? ''}-${release.repositoryName ?? ''}-${release.tagName ?? index}`} + release={release} + index={index} + />If release.id is available in the type, prefer key={release.id}.
frontend/src/components/RecentReleases.tsx (1)
33-33: Avoid index as key; include stable identityUse a composite of org/repo/tag for stability; keep passing index for tooltip IDs.
- <Release key={index} release={item} showAvatar={showAvatar} index={index} /> + <Release + key={`release-${item.organizationName ?? ''}-${item.repositoryName ?? ''}-${item.tagName ?? index}`} + release={item} + showAvatar={showAvatar} + index={index} + />frontend/__tests__/unit/components/Release.test.tsx (4)
125-126: Tighten the date assertionAvoid allowing 5-digit years; assert a 4-digit year and fixed month tokens.
- expect(screen.getByText(/\w{3} \d{1,2}, \d{4,5}/)).toBeInTheDocument() // Date format + expect( + screen.getByText(/\b(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\b \d{1,2}, \d{4}$/) + ).toBeInTheDocument()
144-149: Type-safety for author absenceRelease.author is non-optional in the type; this test sets it to undefined. Either make author optional in types/release.ts (preferred) or cast in the test to satisfy TS.
Option A (prefer): make author?: User | null in types/release.ts.
Option B (test-only cast):
- const releaseWithoutAuthor = { ...mockReleases[0], author: undefined } + const releaseWithoutAuthor = ({ ...mockReleases[0], author: undefined } as unknown) as ReleaseType
166-169: Query buttons by accessible namegetByRole('button') can become ambiguous as tests/components grow. Query by name to target the correct control.
Example:
- const repoButton = screen.getByRole('button') + const repoButton = screen.getByRole('button', { name: /our-awesome-project/i })Repeat similarly for other occurrences using the repository name in each case.
Also applies to: 174-180, 186-188, 243-245, 252-254, 261-263
191-195: Reduce brittleness of tooltip selectionRelying on closest('div') + id is fragile. Prefer a data-testid exposed by the component and assert on that.
Test change:
- const tooltip = screen.getByAltText('Test User').closest('div') - expect(tooltip).toHaveAttribute('id', 'avatar-tooltip-0') + const tooltip = screen.getByTestId('avatar-tooltip-0') + expect(tooltip).toBeInTheDocument()Component change (Release.tsx) to support this:
<Tooltip id={`avatar-tooltip-${index}`} data-testid={`avatar-tooltip-${index}`} ...> ... </Tooltip>frontend/src/components/Release.tsx (6)
31-53: Avoid “dead” avatar links; add fallbacks for missing avatarUrl and better a11y.
- Rendering a Link to "#" when login is absent is misleading.
- Next/Image will error if avatarUrl is empty/null; add a safe fallback.
- Add an aria-label for SR users; use empty alt when non-clickable.
Apply:
- <Link - className="shrink-0 text-blue-400 hover:underline" - href={release.author.login ? `/members/${release.author.login}` : '#'} - > - <Image - alt={release.author.name || release.author.login} - className="mr-2 h-6 w-6 rounded-full" - height={24} - src={release.author.avatarUrl} - width={24} - /> - </Link> + {release.author.login ? ( + <Link + className="shrink-0" + href={`/members/${release.author.login}`} + aria-label={`View ${release.author.name || release.author.login}'s profile`} + > + <Image + alt={release.author.name || release.author.login || 'Author avatar'} + className="mr-2 h-6 w-6 rounded-full" + height={24} + src={release.author.avatarUrl || '/images/avatar-placeholder.png'} + width={24} + /> + </Link> + ) : ( + <span className="shrink-0"> + <Image + alt="" + className="mr-2 h-6 w-6 rounded-full" + height={24} + src={release.author.avatarUrl || '/images/avatar-placeholder.png'} + width={24} + /> + </span> + )}
33-37: Drop the hardcoded tooltip id to avoid duplicates across lists.Using index defaults to 0 can create repeated ids. Tooltip doesn’t require a stable id here.
Apply:
- id={`avatar-tooltip-${index}`}
55-63: Use a plain anchor for external GitHub links (Next Link is for internal routes).Improves semantics and avoids unnecessary client-side routing.
Apply:
- <Link - className="text-blue-400 hover:underline" - href={`https://github.com/${release.organizationName}/${release.repositoryName}/releases/tag/${release.tagName}`} - target="_blank" - rel="noopener noreferrer" - > + <a + className="text-blue-400 hover:underline" + href={`https://github.com/${release.organizationName}/${release.repositoryName}/releases/tag/${release.tagName}`} + target="_blank" + rel="noopener noreferrer" + > <TruncatedText text={release.name || release.tagName} /> - </Link> + </a>
66-69: Harden date rendering against bad inputs.formatDate throws on invalid dates. Wrap with a safe helper or guard to avoid runtime errors if data is malformed.
Add a safe helper (in utils/dateFormatter.ts):
export const tryFormatDate = (input?: number | string | null) => { try { return input ? formatDate(input as any) : '' } catch { return '-' } }Then update here:
- <span>{formatDate(release.publishedAt)}</span> + <span>{tryFormatDate(release.publishedAt) || '-'}</span>Please confirm GraphQL guarantees publishedAt to be a valid ISO string for all releases; if not, the safe helper is recommended.
71-83: Prefer declarative navigation via Link for the repo pill; improves a11y and prefetch.Switching from router.push to Link enables middle-click/new-tab and prefetch. Render a non-interactive span when org/repo are missing.
Apply:
-import { useRouter } from 'next/navigation' +import Link from 'next/link' @@ - const router = useRouter() @@ - <button - className="cursor-pointer overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" - disabled={!release.organizationName || !release.repositoryName} - onClick={() => { - const org = release.organizationName || '' - const repo = release.repositoryName || '' - if (!org || !repo) return - router.push(`/organizations/${org}/repositories/${repo}`) - }} - > - <TruncatedText text={release.repositoryName} /> - </button> + {release.organizationName && release.repositoryName ? ( + <Link + className="overflow-hidden text-ellipsis whitespace-nowrap text-gray-600 hover:underline dark:text-gray-400" + href={`/organizations/${release.organizationName}/repositories/${release.repositoryName}`} + prefetch + aria-label={`Open ${release.repositoryName} repository`} + > + <TruncatedText text={release.repositoryName} /> + </Link> + ) : ( + <span + className="overflow-hidden text-ellipsis whitespace-nowrap text-gray-400" + aria-disabled="true" + > + <TruncatedText text={release.repositoryName || 'Unknown repository'} /> + </span> + )}Also applies to: 6-6, 25-25
12-17: Avoid exposing and defaulting an “index” prop solely for DOM ids.Index defaults to 0 can repeat; prefer a stable identifier (release.id) or no id at all.
Would you like me to remove index from the public API and migrate Tooltip to be id-less?
Also applies to: 35-35
📜 Review details
Configuration used: Path: .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 ignored due to path filters (1)
frontend/src/types/__generated__/snapshotQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (6)
frontend/__tests__/unit/components/Release.test.tsx(1 hunks)frontend/__tests__/unit/data/mockSnapshotData.ts(1 hunks)frontend/src/app/snapshots/[id]/page.tsx(2 hunks)frontend/src/components/RecentReleases.tsx(2 hunks)frontend/src/components/Release.tsx(1 hunks)frontend/src/server/queries/snapshotQueries.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/app/snapshots/[id]/page.tsx (1)
frontend/src/types/release.ts (1)
Release(3-13)
frontend/src/components/Release.tsx (2)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/components/RecentReleases.tsx (1)
frontend/src/types/release.ts (1)
Release(3-13)
⏰ 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). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (9)
frontend/src/app/snapshots/[id]/page.tsx (2)
19-19: Good move importing the reusable componentImporting and using the shared Release component here reduces duplication.
163-165: Heading tweaks look fineDark-mode color added; consistent with other sections.
frontend/src/components/RecentReleases.tsx (2)
9-9: Prop typing update LGTMSwitch to ReleaseType[] aligns this component with the new shared type.
1-1: Imports look correctIcons, types, and component import changes are consistent with the refactor.
Also applies to: 3-3, 5-5
frontend/__tests__/unit/data/mockSnapshotData.ts (1)
17-24: Add release.id and standardize date types in mocks
- Add
idto eachnewReleasesentry infrontend/__tests__/unit/data/mockSnapshotData.tsto mirror your GraphQL selection set and support stable UI keys.- Ensure all date fields (
publishedAt,updatedAt, etc.) use a single consistent type (e.g. ISO string) across mocks and components.tagName: 'v0.9.2', projectName: 'test-project-1', + id: 'release-1', organizationName: 'owasp', repositoryName: 'test-project-1',frontend/__tests__/unit/components/Release.test.tsx (2)
224-237: Good multi-item coverageVerifies unique tooltip IDs across instances; helpful regression guard.
1-114: Overall test suite qualityBroad coverage (rendering, links, navigation, disabled states, fallbacks). Once the typing and selector nits above are addressed, this looks solid.
Also applies to: 115-286
frontend/src/components/Release.tsx (2)
12-17: Nice, focused prop surface for a reusable card.The props are minimal and cover the reuse scenarios well. Good defaulting of optional props.
44-49: No action required: avatars.githubusercontent.com is already whitelisted in Next/Image remotePatterns.
|
@kasya Done with the PR as I said , please review this |
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Piyushrathoree thanks for working on this issue!
The changes look good 👍🏼
Please resolve Code Rabbit suggestions before requesting a PR review next time 👌🏼 If the suggestion is not relevant - just mark it as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/server/queries/snapshotQueries.ts (1)
11-25: Missing isPreRelease in selection; align with Release props and past feedback.To keep props consistent across pages using , add
isPreRelease. Also double-checkpublishedAttyping (GraphQL usually returns an ISO string) against your TS type for Release.Apply this minimal change:
newReleases { id name organizationName projectName publishedAt repositoryName tagName + isPreRelease author { avatarUrl id login name } }Optional: extract a fragment to prevent drift across queries:
// near the top of this file export const RELEASE_CARD_FIELDS = gql` fragment ReleaseCardFields on Release { id name organizationName projectName publishedAt repositoryName tagName isPreRelease author { avatarUrl id login name } } `; // then use in GET_SNAPSHOT_DETAILS newReleases { ...ReleaseCardFields }To verify impact and typing, run:
#!/usr/bin/env bash # Check Release props and isPreRelease usage rg -n --glob 'frontend/**' 'isPreRelease|<Release|ReleaseProps' # Inspect Release type and publishedAt typing rg -nP --glob 'frontend/**' -C3 'export\s+(interface|type)\s+Release\b|publishedAt'
🧹 Nitpick comments (2)
frontend/src/server/queries/snapshotQueries.ts (2)
17-17: repositoryName added — consider also fetching a canonical repo URL (optional).If the UI builds links, prefer a backend-provided
repositoryUrl/htmlUrl(if available in your schema) to avoid client-side string composition and drift.
19-24: Author block looks good; ensure null-safe handling in .Some releases may have a missing/nullable author. Verify the component gracefully renders without avatar/name when
authoris null. If you link to profiles, consider addingauthorUrl(if exposed) to avoid crafting URLs on the client.
📜 Review details
Configuration used: Path: .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 ignored due to path filters (1)
frontend/src/types/__generated__/snapshotQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (1)
frontend/src/server/queries/snapshotQueries.ts(1 hunks)
⏰ 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 e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (1)
frontend/src/server/queries/snapshotQueries.ts (1)
14-15: Good addition: organizationName and projectName fetched for Release.This aligns the Snapshots query with the new component props and avoids ad-hoc derivations.
Piyushrathoree
deleted the
task/update-release-component-snapshot-page
branch
#2179) * generated a Release compoenent which is removing the code duplicacy and used a single compoenent in recentRelease and new release on snapshot page * Apply order --------- Co-authored-by: Kate Golovanova <[email protected]>
OWASP#2179) * generated a Release compoenent which is removing the code duplicacy and used a single compoenent in recentRelease and new release on snapshot page * Apply order --------- Co-authored-by: Kate Golovanova <[email protected]>
2 participants
Proposed change
Resolves #2040
generated a Release component which is removing the code duplicacy and used a single component in recentRelease component and new release component on snapshot page.
Checklist
make check-testlocally; all checks and tests passed.