feat: Add archived badge for repositories (#2388) #2412
Conversation
Summary by CodeRabbit
WalkthroughExpose repository archived status from backend GraphQL, propagate it to frontend queries, add an ArchivedBadge UI component, surface isArchived in DetailsCard and repository lists, update types, and add unit tests for badge rendering and placements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 ignored due to path filters (5)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧬 Code graph analysis (1)frontend/__tests__/unit/components/RepositoriesCard.test.tsx (1)
🔇 Additional comments (6)
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 (2)
frontend/src/components/ArchivedBadge.tsx (2)
11-31: Remove leading whitespace in className.The component implementation is solid with good defaults, size variants, and dark mode support. However, there's a leading space in the className string at line 24 that should be removed for cleaner code.
Apply this diff to remove the leading whitespace:
<span - className={` ${sizeClasses[size]} ${className} inline-flex items-center gap-1.5 rounded-full border border-yellow-600 bg-yellow-50 font-medium text-yellow-800 dark:border-yellow-500 dark:bg-yellow-900/30 dark:text-yellow-400`} + className={`${sizeClasses[size]} ${className} inline-flex items-center gap-1.5 rounded-full border border-yellow-600 bg-yellow-50 font-medium text-yellow-800 dark:border-yellow-500 dark:bg-yellow-900/30 dark:text-yellow-400`} title="This repository has been archived and is read-only" >
25-26: Enhance accessibility with aria-label.While the
titleattribute provides a tooltip, consider adding anaria-labelfor better screen reader support.Apply this diff to add aria-label:
<span className={`${sizeClasses[size]} ${className} inline-flex items-center gap-1.5 rounded-full border border-yellow-600 bg-yellow-50 font-medium text-yellow-800 dark:border-yellow-500 dark:bg-yellow-900/30 dark:text-yellow-400`} title="This repository has been archived and is read-only" + aria-label="This repository has been archived and is read-only" >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/organizationQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (11)
backend/apps/github/api/internal/nodes/repository.py(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/server/queries/organizationQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/RepositoriesCard.tsx (1)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
🔇 Additional comments (15)
frontend/src/server/queries/organizationQueries.ts (1)
82-82: LGTM!The
isArchivedfield addition to the repositories query is correct and follows the established pattern. The camelCase naming is appropriate for GraphQL queries.frontend/src/server/queries/userQueries.ts (1)
58-58: LGTM!The
isArchivedfield addition is consistent with the pattern used in other query files and correctly positioned within the selection set.frontend/src/server/queries/repositoryQueries.ts (1)
12-12: LGTM!The
isArchivedfield addition to the main repository query is correct and maintains consistency with other query files.frontend/src/types/project.ts (1)
55-55: LGTM!The
isArchivedoptional boolean property is correctly typed and appropriately marked as optional to handle repositories that may not have this field.frontend/src/server/queries/projectQueries.ts (1)
63-63: LGTM!The
isArchivedfield addition maintains consistency across all query files in the PR.frontend/src/types/card.ts (1)
52-52: LGTM!The
isArchivedproperty is correctly added toDetailsCardPropswith appropriate typing and optional marking. The placement alongsideisActiveis logical.backend/apps/github/api/internal/nodes/repository.py (1)
30-30: LGTM!The
is_archivedfield is correctly exposed in the GraphQL schema. The snake_case naming (backend) correctly maps to camelCaseisArchived(frontend) through GraphQL conventions.frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
114-114: LGTM!The
isArchivedprop is correctly passed toDetailsCard, completing the data flow from the GraphQL query to the UI component. This aligns with the type definition inDetailsCardProps.frontend/src/components/RepositoriesCard.tsx (2)
6-6: LGTM!The import statement is correctly placed and follows the existing import organization pattern.
42-50: Verify text truncation alongside badge in RepositoriesCardOnly this component pairs
TruncatedTextwithArchivedBadge. On narrow screens, ensure the text still truncates before the badge (consider addingmin-w-0to the text wrapper if needed).frontend/src/components/CardDetailsPage.tsx (3)
20-20: LGTM!The import follows the existing alphabetical ordering of component imports.
54-54: LGTM!The default value of
falseis appropriate and ensures backward compatibility with existing usages that don't pass this prop.
106-113: Good implementation with proper type guard.The layout change correctly wraps both indicators in a flex container with
gap-2for consistent spacing. The conditional rendering ofArchivedBadgeis properly guarded withtype === 'repository', ensuring it only appears on repository pages as intended.frontend/src/components/ArchivedBadge.tsx (2)
1-3: LGTM!The imports are correct and include all necessary dependencies for the component.
5-9: LGTM!The interface is well-defined with appropriate optional props and a clear size union type.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Thanks for working on this @mrkeshav-05 !
I like the styling, but added a few comments.
Also, could you add relevant tests for new component and updated components?
Thanks!
| {!isActive && ( | ||
| <span className="justify-center rounded bg-red-200 px-2 py-1 text-sm text-red-800"> | ||
| Inactive | ||
| </span> | ||
| )} | ||
| {isArchived && type === 'repository' && <ArchivedBadge size="md" />} | ||
| </div> |
There was a problem hiding this comment.
I know original issue was only saying about the Archived repo badge, but this is something that caught my attention.
We have a very similar case of showing another "badge" for repos and projects for isActive state and it doesn't look anything close to what we now have in this new badge you've added.
Can we reuse the same component for all of these badges? With renaming it of course. Maybe something like StatusBadge but it's up to you.
Ideally, we'd make the component dynamic, passing in the property - 'Inactive', 'Archived' etc. This way we'll be able to reuse it in many other places and have a consistent look.
Can you then update the code above to use the same component instead of what we have now?
There was a problem hiding this comment.
Hey @kasya ,
Sure, I can definitely look into that!
However, I was wondering if we could raise a separate issue for this new requirement. I’ve already completed the code changes related to the original issue that was assigned to me.
For the new StatusBadge component, I’d like to plan the different approach since it involves making the badge dynamic and reusable.
If it works for you, you can assign me the new issue — I’ll be happy to take it up and work on it as soon as possible. 😊
| <MetricsScoreCircle | ||
| score={healthMetricsData[0].score} | ||
| clickable={true} | ||
| onClick={() => scrollToAnchor('issues-trend')} | ||
| /> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
When you end up updating that, can you also switch these two? I do think that Badge should go before/to the left of
the MetricsScoreCircle
There was a problem hiding this comment.
Should I only swap the positions of the Badge and MetricsScoreCircle components, or should I also update the Inactive button style to match the Archived badge style?
There was a problem hiding this comment.
@mrkeshav-05 the idea is to re-se the same component you created, with passing in the state as a prop. So yeah, it would be the same styling.. since it would be the same component.
There was a problem hiding this comment.
I created an issue - feel free to comment there to get assigned
mrkeshav-05
force-pushed
the
feature/archived-badge
branch
2 times, most recently
from
a1120a2 to
b1e32a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/organizationQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/github/api/internal/nodes/repository.py(1 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(2 hunks)frontend/__tests__/unit/components/ArchivedBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/components/RepositoriesCard.test.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/server/queries/organizationQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/src/components/ArchivedBadge.tsx
- frontend/src/server/queries/repositoryQueries.ts
- frontend/src/server/queries/userQueries.ts
- frontend/src/types/card.ts
- backend/apps/github/api/internal/nodes/repository.py
- frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx
- frontend/src/types/project.ts
- frontend/src/server/queries/organizationQueries.ts
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
backend/tests/apps/github/api/internal/nodes/repository_test.py (2)
backend/apps/github/api/internal/nodes/repository.py (1)
RepositoryNode(41-101)backend/tests/apps/github/api/internal/queries/repository_test.py (1)
mock_repository(15-19)
frontend/src/components/RepositoriesCard.tsx (1)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
frontend/__tests__/unit/components/RepositoriesCard.test.tsx (1)
frontend/src/types/project.ts (1)
RepositoryCardProps(52-63)
🔇 Additional comments (1)
backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
26-26: LGTM!Correctly adds
is_archivedto the expected fields set, ensuring the new field is verified by the meta configuration test.
backend/tests/apps/github/api/internal/nodes/repository_test.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/github/api/internal/nodes/repository_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (2)
1456-1467: Consider testing semantic structure over specific CSS classes.The tests at lines 1465 and 1478-1479 assert on specific Tailwind CSS classes (
.ml-4.flex.gap-2andpx-3, py-1, text-sm). While this verifies the current implementation, it couples tests to styling details. If the design changes while maintaining correct visual behavior, these tests will break unnecessarily.Consider alternatives that focus on semantic structure or visual behavior rather than exact class names, such as checking the badge's position relative to the title or verifying the badge is rendered as an inline element in the title area.
Also applies to: 1478-1479
1380-1493: Consider verifying icon and color scheme.The test suite comprehensively covers the badge's conditional rendering logic, which is excellent. However, per the PR objectives, the badge should include a FontAwesome archive icon and use a yellow/amber color scheme. The current tests verify the text "Archived" appears but don't confirm:
- The archive icon is rendered (the mocked
FontAwesomeIconrenders withdata-testid="icon-archive"which could be queried)- The badge uses the correct color classes (amber/yellow per issue #2388)
Adding assertions for these would ensure the visual requirements are met and prevent regressions.
Example for icon verification:
const badge = screen.getByText('Archived') const icon = badge.querySelector('[data-testid="icon-archive"]') expect(icon).toBeInTheDocument()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)
🔇 Additional comments (1)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
1469-1480: Past review concern has been resolved.The missing render call flagged in the previous review has been correctly added at line 1476. The test now properly renders the component before asserting on the badge classes.
|
@mrkeshav-05 could you resolve conflicts before I review this again? Thanks! |
mrkeshav-05
force-pushed
the
feature/archived-badge
branch
from
4ef13c9 to
51945cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ArchivedBadge.tsx (1)
28-28: Consider scaling icon size with badge size.The icon is currently hardcoded as
h-3 w-3across all badge sizes. For thelgsize badge, the icon might appear small relative to thetext-basetext size.Apply this diff to scale the icon with the badge size:
const ArchivedBadge: React.FC<ArchivedBadgeProps> = ({ className = '', showIcon = true, size = 'md', }) => { const sizeClasses = { sm: 'px-2 py-1 text-xs', md: 'text-sm px-3 py-1', lg: 'px-4 py-2 text-base', } + + const iconSizes = { + sm: 'h-3 w-3', + md: 'h-3.5 w-3.5', + lg: 'h-4 w-4', + } return ( <span className={`${sizeClasses[size]} ${className} inline-flex items-center gap-1.5 rounded-full border border-yellow-600 bg-yellow-50 font-medium text-yellow-800 dark:border-yellow-500 dark:bg-yellow-900/30 dark:text-yellow-400`} title="This repository has been archived and is read-only" aria-label="This repository has been archived and is read-only" > - {showIcon && <FontAwesomeIcon icon={faArchive} className="h-3 w-3" />} + {showIcon && <FontAwesomeIcon icon={faArchive} className={iconSizes[size]} />} Archived </span> ) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/organizationQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/github/api/internal/nodes/repository.py(1 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(2 hunks)frontend/__tests__/unit/components/ArchivedBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/components/RepositoriesCard.test.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/server/queries/organizationQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/apps/github/api/internal/nodes/repository.py
- frontend/src/server/queries/projectQueries.ts
- frontend/src/types/project.ts
- frontend/src/types/card.ts
- frontend/tests/unit/components/ArchivedBadge.test.tsx
- backend/tests/apps/github/api/internal/nodes/repository_test.py
- frontend/src/components/RepositoriesCard.tsx
- frontend/tests/unit/components/CardDetailsPage.test.tsx
- frontend/src/server/queries/userQueries.ts
- frontend/src/server/queries/organizationQueries.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
backend/apps/github/api/internal/queries/repository.py (1)
repository(14-35)
frontend/__tests__/unit/components/RepositoriesCard.test.tsx (1)
frontend/src/types/project.ts (1)
RepositoryCardProps(54-65)
frontend/src/components/CardDetailsPage.tsx (2)
frontend/src/utils/env.client.ts (1)
IS_PROJECT_HEALTH_ENABLED(7-8)frontend/src/utils/scrollToAnchor.ts (1)
scrollToAnchor(1-21)
🔇 Additional comments (6)
frontend/src/server/queries/repositoryQueries.ts (1)
12-12: LGTM!The addition of
isArchivedto the GraphQL query is correct and aligns with the backend schema changes.frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
114-114: LGTM!The
isArchivedprop is correctly passed from the repository data to theDetailsCardcomponent.frontend/src/components/ArchivedBadge.tsx (1)
25-26: LGTM!The accessibility attributes are well-implemented with clear, descriptive text for screen readers and tooltips.
frontend/__tests__/unit/components/RepositoriesCard.test.tsx (1)
180-321: LGTM!The test suite comprehensively covers the archived badge functionality, including edge cases like undefined/null values, mixed lists, layout preservation, and navigation. The tests verify both presence/absence of the badge and its visual properties.
frontend/src/components/CardDetailsPage.tsx (2)
100-114: LGTM!The header layout is correctly implemented with the
ArchivedBadgepositioned before theMetricsScoreCircle, addressing the previous review feedback. The unified flex container cleanly groups the Inactive status, archived badge, and health metrics.
56-56: LGTM!The
isArchivedprop is correctly added with a sensible default value offalse, ensuring backward compatibility with existing call sites.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
207-213: Remove redundant existence test (already covered by meta subset).
test_meta_configurationalready assertsis_archivedvia subset; this dedicated existence test duplicates coverage.Apply this diff:
- def test_is_archived_field_exists(self): - """Test that is_archived field is exposed in the GraphQL schema.""" - field_names = {field.name for field in RepositoryNode.__strawberry_definition__.fields} - assert "is_archived" in field_names, ( - "is_archived field should be exposed in RepositoryNode" - ) -frontend/src/components/CardDetailsPage.tsx (1)
100-106: Inconsistent badge styling approach.The inline styling for the Inactive badge differs from the component-based
ArchivedBadge, creating visual and maintenance inconsistencies. However, this concern has already been raised and the author has agreed to address it in a separate issue for a unifiedStatusBadgecomponent.
🧹 Nitpick comments (3)
frontend/src/types/card.ts (1)
54-55: Prop addition looks correct.
isArchived?: booleanaligns with usage in repository views.Consider documenting this as repository-only (or using a discriminated union on
type) to prevent accidental use on other entities.frontend/src/components/ArchivedBadge.tsx (1)
16-20: Improve a11y and visual consistency: hide decorative icon from SR; scale icon per size.Add
aria-hiddento the icon and map icon size to badge size.Apply this diff:
const sizeClasses = { - sm: 'px-2 py-1 text-xs', - md: 'text-sm px-3 py-1', - lg: 'px-4 py-2 text-base', + sm: 'px-2 py-1 text-xs', + md: 'px-3 py-1 text-sm', + lg: 'px-4 py-2 text-base', } + const iconSize = { + sm: 'h-3 w-3', + md: 'h-3.5 w-3.5', + lg: 'h-4 w-4', + }[size] ... - {showIcon && <FontAwesomeIcon icon={faArchive} className="h-3 w-3" />} + {showIcon && ( + <FontAwesomeIcon icon={faArchive} className={iconSize} aria-hidden="true" /> + )} ArchivedAlso applies to: 28-30
frontend/src/components/CardDetailsPage.tsx (1)
100-114: Consider restructuring the layout to prevent awkward spacing.The parent container uses
justify-betweenwith potentially three or more direct children (title, action buttons, and badges). When both action buttons (e.g., "Edit Module") and the badges/metrics are rendered, the spacing may appear unbalanced. Consider grouping the action buttons with the badges container so they're treated as a single right-aligned block.Example restructure:
<div className="flex w-full items-center justify-between"> <h1 className="text-4xl font-bold">{title}</h1> + <div className="flex items-center gap-3"> {type === 'program' && accessLevel === 'admin' && canUpdateStatus && ( <ProgramActions status={status} setStatus={setStatus} /> )} {type === 'module' && accessLevel === 'admin' && admins?.some( (admin) => admin.login === ((data as ExtendedSession)?.user?.login as string) ) && ( <button type="button" className="flex items-center justify-center gap-2 rounded-md border border-[#0D6EFD] bg-transparent px-2 py-2 text-nowrap text-[#0D6EFD] transition-all hover:bg-[#0D6EFD] hover:text-white dark:border-sky-600 dark:text-sky-600 dark:hover:bg-sky-100" onClick={() => { router.push(`${window.location.pathname}/edit`) }} > Edit Module </button> )} - <div className="flex items-center gap-3"> {!isActive && ( <span className="inline-flex items-center gap-1.5 rounded-full border border-red-600 bg-red-50 px-3 py-1 text-sm font-medium text-red-800 dark:border-red-500 dark:bg-red-900/30 dark:text-red-400"> Inactive </span> )} {isArchived && type === 'repository' && <ArchivedBadge size="md" />} {IS_PROJECT_HEALTH_ENABLED && type === 'project' && healthMetricsData?.length > 0 && ( <MetricsScoreCircle score={healthMetricsData[0].score} clickable={true} onClick={() => scrollToAnchor('issues-trend')} /> )} - </div> + </div> </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/organizationQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/github/api/internal/nodes/repository.py(1 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(2 hunks)frontend/__tests__/unit/components/ArchivedBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/components/RepositoriesCard.test.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/server/queries/organizationQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/components/RepositoriesCard.tsx
- frontend/src/types/project.ts
- frontend/src/server/queries/userQueries.ts
- frontend/src/server/queries/organizationQueries.ts
- backend/apps/github/api/internal/nodes/repository.py
- frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx
- frontend/tests/unit/components/RepositoriesCard.test.tsx
- frontend/tests/unit/components/ArchivedBadge.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
backend/apps/github/api/internal/nodes/repository.py (1)
RepositoryNode(41-101)
frontend/src/components/CardDetailsPage.tsx (2)
frontend/src/utils/env.client.ts (1)
IS_PROJECT_HEALTH_ENABLED(7-8)frontend/src/utils/scrollToAnchor.ts (1)
scrollToAnchor(1-21)
🔇 Additional comments (7)
backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
214-218: Good: Type assertion for is_archived.Asserting
booltype is appropriate and non-redundant.frontend/__tests__/unit/components/CardDetailsPage.test.tsx (2)
1488-1499: Good fix: render before asserting.Local render added; class assertion for medium size is now valid.
1501-1512: Drop nullisArchivedtest
In CardDetailsPage.test.tsx (lines 1501–1512), the test passesisArchived: nullagainst a prop typed asboolean(and the backend never returnsnull). Remove this null case (useundefinedif you need to cover “absent” state). EnablestrictNullChecksin yourtsconfig.jsonto catch null assignments at compile time.frontend/src/server/queries/repositoryQueries.ts (1)
12-12: AllisArchivedusages are present and types align.
GraphQL codegen, manual types, and component consumption correctly reflect the new field.frontend/src/server/queries/projectQueries.ts (1)
74-74: LGTM: Fetching archived flag for project repositories. Verified codegen ran;isArchivedis present in both type definitions and generated queries, enabling list-level badge rendering.frontend/src/components/CardDetailsPage.tsx (2)
20-20: LGTM!The import statement is correct and follows the existing import patterns in the file.
56-56: LGTM!The new
isArchivedprop with a default value offalseis consistent with the existingisActiveprop pattern and integrates well with the component's design.
|
While running the tests, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/ArchivedBadge.test.tsx (2)
10-61: Excellent coverage of rendering and size variants.The tests thoroughly validate default rendering, tooltip content, and all three size variants (sm, md, lg) with appropriate Tailwind class assertions.
Minor note: Lines 12-16 and 18-23 both verify that the badge renders and text is present—
toBeInTheDocument()andtoBeVisible()provide similar coverage here since there's no conditional visibility. You could optionally consolidate these into a single test.
63-91: Icon visibility tests are thorough.Tests correctly verify default icon visibility, hiding behavior when
showIcon={false}, and icon sizing classes.Minor note: Lines 64-69 and 71-76 test the same behavior since the icon is visible by default. You could optionally remove the explicit
showIcon={true}test to reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ArchivedBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/CardDetailsPage.test.tsx
🔇 Additional comments (3)
frontend/__tests__/unit/components/ArchivedBadge.test.tsx (3)
1-8: LGTM! Clean mock setup.The FontAwesomeIcon mock is appropriate for unit testing, isolating the badge's behavior while still allowing icon-related assertions via the data-testid.
93-148: Styling and className tests are solid.Comprehensive validation of base classes, light/dark mode color schemes, font weight, and custom className handling. The tests correctly verify that custom classes are additive (line 139).
176-204: Edge case coverage is comprehensive.Tests validate combined props, re-render stability, and absence of interactive elements—all important scenarios.
The re-render consistency test (lines 186-195) compares className strings directly. While this works for deterministic class ordering, it could theoretically break if Tailwind or the component's class concatenation logic changes order. That said, for this simple component with deterministic class generation, the current approach is acceptable.
mrkeshav-05
force-pushed
the
feature/archived-badge
branch
from
27b73ee to
92e477b
Compare
|
@mrkeshav-05 please resolve conflicts and update branch for another round of review! I will create a separate task for updating this component with proposed changes (to make it dynamic). Thanks! |
mrkeshav-05
force-pushed
the
feature/archived-badge
branch
from
92e477b to
ec79930
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ArchivedBadge.test.tsx (1)
168-173: Consider using testid instead of HTML structure assertion.The
toContainHTML('i')assertion depends on the mock's implementation detail of rendering an<i>tag. If the mock or actual FontAwesome implementation changes (e.g., to use<svg>directly), this test could break unnecessarily.Since icon presence is already validated using the
archive-icontestid in other tests (lines 67, 74, etc.), this test is somewhat redundant. Consider either:
- Removing this test entirely, or
- Refactoring to use the testid pattern consistently
Apply this diff to use the testid pattern:
it('has appropriate semantic structure with icon', () => { render(<ArchivedBadge />) const badge = screen.getByText('Archived') - expect(badge).toContainHTML('i') + const icon = screen.getByTestId('archive-icon') + expect(badge).toContainElement(icon) })Or simply remove the redundant test:
- it('has appropriate semantic structure with icon', () => { - render(<ArchivedBadge />) - - const badge = screen.getByText('Archived') - expect(badge).toContainHTML('i') - })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/organizationQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/github/api/internal/nodes/repository.py(1 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(2 hunks)frontend/__tests__/unit/components/ArchivedBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(2 hunks)frontend/__tests__/unit/components/RepositoriesCard.test.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/server/queries/organizationQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/repositoryQueries.ts(1 hunks)frontend/src/server/queries/userQueries.ts(1 hunks)frontend/src/types/card.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/server/queries/organizationQueries.ts
- frontend/src/server/queries/userQueries.ts
- frontend/src/components/RepositoriesCard.tsx
- frontend/src/types/project.ts
- frontend/src/server/queries/repositoryQueries.ts
- backend/apps/github/api/internal/nodes/repository.py
- backend/tests/apps/github/api/internal/nodes/repository_test.py
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/RepositoriesCard.test.tsx (1)
frontend/src/types/project.ts (1)
RepositoryCardProps(54-65)
🔇 Additional comments (7)
frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx (1)
114-114: LGTM! Clean integration of archived status.The
isArchivedprop is correctly wired from the GraphQL query data to theDetailsCardcomponent, enabling the archived badge display on repository detail pages.frontend/src/server/queries/projectQueries.ts (1)
74-74: LGTM! GraphQL query updated correctly.The
isArchivedfield addition to the repositories selection is correct and aligns with the backend schema changes.frontend/__tests__/unit/components/RepositoriesCard.test.tsx (1)
180-321: LGTM! Comprehensive test coverage for archived badge.The test suite thoroughly covers the archived badge functionality including:
- Display logic for archived/non-archived repositories
- Mixed repository lists
- Badge styling and sizing
- Layout integration
- Persistence across UI interactions
- Navigation behavior
The edge cases (null/undefined handling) are properly tested.
frontend/src/types/card.ts (1)
55-55: LGTM! Type definition is correct.The optional
isArchivedproperty is appropriately added toDetailsCardPropswith the correct type.frontend/src/components/ArchivedBadge.tsx (1)
1-34: LGTM! Well-implemented reusable component.The
ArchivedBadgecomponent is well-designed with:
- Clear size variants (sm, md, lg)
- Conditional icon rendering
- Proper accessibility attributes (title and aria-label)
- Appropriate yellow/amber color scheme for archived status
- Dark mode support
The implementation follows React best practices and is ready for use across the application.
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
1400-1514: LGTM! Comprehensive test coverage with past issue resolved.The "Archived Badge Functionality" test suite thoroughly validates:
- Display logic for archived repositories
- Type restrictions (repository-only)
- Badge interactions with inactive state
- Layout and positioning
- Size rendering
- Edge cases (null/undefined handling)
The previous review concern about the missing render call in the size test (lines 1490-1501) has been properly addressed.
frontend/__tests__/unit/components/ArchivedBadge.test.tsx (1)
11-167: LGTM! Excellent test coverage for ArchivedBadge component.The test suite is comprehensive and well-organized, covering:
- Basic rendering and text display
- All size variants with proper defaulting
- Icon visibility controls
- Styling classes for light/dark modes
- Custom className handling
- Accessibility attributes
- Edge cases and multiple prop combinations
The tests follow good practices and provide strong confidence in the component's behavior.
mrkeshav-05
force-pushed
the
feature/archived-badge
branch
from
0fd2db3 to
06fff7c
Compare
…ArchivedBadge component
…dDetailsPage and RepositoriesCard components
…positoryNode, CardDetailsPage, and RepositoriesCard components
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
mrkeshav-05
force-pushed
the
feature/archived-badge
branch
from
06fff7c to
95a7c70
Compare
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
This works and looks great! Thanks!
3 participants
Description
This PR implements an "Archived" badge for archived repositories, making it clear to users which repositories are no longer actively maintained.
Fixed #2388
Changes Made
Backend Changes
apps/github/api/internal/nodes/repository.py: Addedis_archivedfield to the GraphQLRepositoryNodeschemaFrontend Changes
GraphQL Queries
repositoryQueries.ts: AddedisArchivedfield toGetRepositoryDataqueryorganizationQueries.ts: AddedisArchivedfield to repositories list queryprojectQueries.ts: AddedisArchivedfield to repositories list queryuserQueries.ts: AddedisArchivedfield to top contributed repositories queryNew Component
ArchivedBadge.tsx: Created a reusable badge component with:Updated Components
CardDetailsPage.tsx:isArchivedprop to component interfaceRepositoriesCard.tsx:Generated Types
pnpm graphql-codegento regenerate TypeScript typesisArchived: booleanChecklist
make check-testlocally; all checks and tests passed.isArchivedfield in GraphQL schema🔗 References