Refactor: Replace ArchivedBadge with Flexible StatusBadge Component#2444
Refactor: Replace ArchivedBadge with Flexible StatusBadge Component#2444kasya merged 8 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughReplaces ArchivedBadge with a configurable StatusBadge, updates CardDetailsPage and RepositoriesCard to use StatusBadge, removes ArchivedBadge and its tests, and adds comprehensive StatusBadge tests plus a small test selector update in RepositoriesCard tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
|
Test: This PR must be linked to an issue. |
mrkeshav-05
changed the title
![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 (3)
frontend/src/components/StatusBadge.tsx (3)
21-29: ExportStatusBadgePropsinterface for external use.The interface should be exported to allow consumers to type-check props when wrapping or using this component in TypeScript.
Apply this diff:
-interface StatusBadgeProps { +export interface StatusBadgeProps { status: StatusType className?: string showIcon?: boolean size?: 'sm' | 'md' | 'lg' customText?: string customIcon?: IconDefinition customTooltip?: string }
80-80: Consider refactoring className concatenation for better readability.The long template literal makes the className assembly hard to read and maintain. Consider using an array with
.join(' ')or a utility likeclsxfor cleaner code.Example refactor:
+ const classNames = [ + sizeClasses[size], + className, + 'inline-flex items-center gap-1.5 rounded-full border', + config.borderColor, + config.bgColor, + config.textColor, + config.darkBorderColor, + config.darkBgColor, + config.darkTextColor, + 'font-medium', + ].join(' ') + return ( <span - className={`${sizeClasses[size]} ${className} inline-flex items-center gap-1.5 rounded-full border ${config.borderColor} ${config.bgColor} ${config.textColor} ${config.darkBorderColor} ${config.darkBgColor} ${config.darkTextColor} font-medium`} + className={classNames} title={displayTooltip} aria-label={config.ariaLabel} >
70-82: Consider makingaria-labelconsistent with custom tooltip.When
customTooltipis provided, the visible tooltip changes butaria-labelremains unchanged. This creates an inconsistency between what sighted users and screen reader users experience.Consider one of these approaches:
- Derive aria-label from the tooltip:
const displayTooltip = customTooltip ?? config.tooltip + const displayAriaLabel = customTooltip ?? config.ariaLabel return ( <span className={`...`} title={displayTooltip} - aria-label={config.ariaLabel} + aria-label={displayAriaLabel} >
- Add a
customAriaLabelprop for explicit control:customText?: string customIcon?: IconDefinition customTooltip?: string + customAriaLabel?: string
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/__tests__/unit/components/ArchivedBadge.test.tsx(0 hunks)frontend/__tests__/unit/components/RepositoriesCard.test.tsx(1 hunks)frontend/__tests__/unit/components/StatusBadge.test.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(0 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/components/StatusBadge.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/tests/unit/components/ArchivedBadge.test.tsx
- frontend/src/components/ArchivedBadge.tsx
🔇 Additional comments (4)
frontend/src/components/StatusBadge.tsx (1)
58-88: LGTM! Well-structured reusable component.The component provides a clean, type-safe API with good defaults and extensibility through custom props. The separation of configuration from rendering logic is excellent, and the dark mode support is properly implemented.
frontend/src/components/RepositoriesCard.tsx (1)
8-8: LGTM! Clean migration to StatusBadge.The import and usage have been correctly updated to use the new
StatusBadgecomponent with appropriate props (status="archived",size="sm",showIcon={false}), maintaining the same visual appearance as before.Also applies to: 51-51
frontend/src/components/CardDetailsPage.tsx (1)
35-35: LGTM! Consistent migration to StatusBadge.Both inactive and archived statuses now use the new
StatusBadgecomponent with appropriate props. Thesize="md"is suitable for the detail page context.Also applies to: 101-102
frontend/__tests__/unit/components/StatusBadge.test.tsx (1)
1-332: Excellent test coverage!The test suite is comprehensive and well-organized, covering rendering, status types, size variants, icon display, custom overrides, styling, accessibility, edge cases, and integration scenarios. The tests are well-named and focused.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/components/StatusBadge.tsx (4)
21-29: Consider exporting StatusBadgeProps for better reusability.The interface is not currently exported, which prevents other components from referencing these prop types (e.g., for wrapper components or HOCs). If external code needs to work with StatusBadge props, consider adding the
exportkeyword.-interface StatusBadgeProps { +export interface StatusBadgeProps {
72-76: Move sizeClasses outside the component for better performance.The
sizeClassesobject is static but currently recreated on every render. Moving it outside the component body will improve performance.+const sizeClasses = { + sm: 'px-2 py-1 text-xs', + md: 'text-sm px-3 py-1', + lg: 'px-4 py-2 text-base', +} + const StatusBadge: React.FC<StatusBadgeProps> = ({ status, className = '', showIcon = true, size = 'md', customText, customIcon, customTooltip, }) => { const config = statusConfig[status] const displayText = customText ?? config.text const displayIcon = customIcon ?? config.icon const displayTooltip = customTooltip ?? config.tooltip - - const sizeClasses = { - sm: 'px-2 py-1 text-xs', - md: 'text-sm px-3 py-1', - lg: 'px-4 py-2 text-base', - }
82-82: Consider allowing customization of aria-label for consistency.The component allows
customTooltip(line 81) but always usesconfig.ariaLabel(line 82). If a consumer provides a custom tooltip, the aria-label and tooltip will be inconsistent. Consider adding acustomAriaLabelprop or deriving aria-label fromdisplayTooltipwhen customized.Example approach:
interface StatusBadgeProps { status: StatusType className?: string showIcon?: boolean size?: 'sm' | 'md' | 'lg' customText?: string customIcon?: IconDefinition customTooltip?: string + customAriaLabel?: string }Then use:
const displayAriaLabel = customAriaLabel ?? config.ariaLabel
85-85: Consider scaling icon size with badge size.The icon size is hardcoded as
h-3 w-3regardless of the badge size variant. For better visual consistency, consider scaling the icon based on thesizeprop (e.g., sm: h-2.5 w-2.5, md: h-3 w-3, lg: h-4 w-4).+const iconSizeClasses = { + sm: 'h-2.5 w-2.5', + md: 'h-3 w-3', + lg: 'h-4 w-4', +} + const StatusBadge: React.FC<StatusBadgeProps> = ({ ... }) => { ... return ( <span ...> {showIcon && ( - <FontAwesomeIcon icon={displayIcon} className="h-3 w-3" data-testid="status-icon" /> + <FontAwesomeIcon icon={displayIcon} className={iconSizeClasses[size]} data-testid="status-icon" /> )} {displayText} </span> ) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/StatusBadge.test.tsx(1 hunks)frontend/src/components/StatusBadge.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/StatusBadge.test.tsx
🔇 Additional comments (3)
frontend/src/components/StatusBadge.tsx (3)
1-4: LGTM!Imports are well-structured and follow best practices by importing individual icons for optimal bundle size.
31-56: LGTM!The statusConfig object is well-structured with comprehensive theming for both light and dark modes, proper accessibility labels, and type-safe exhaustiveness via
Record<StatusType, StatusConfig>.
92-92: LGTM!
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@mrkeshav-05 This looks great! Thanks for working on it!
Left a couple of tiny requests, but overall - good work! 👍🏼
…itoriesCard components
…ate icon rendering
mrkeshav-05
force-pushed
the
feature/status-badge
branch
from
4b08414 to
0e5eab9
Compare
mrkeshav-05
force-pushed
the
feature/status-badge
branch
from
0e5eab9 to
5e35953
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/components/RepositoriesCard.tsx (2)
25-27: Avoid array index as React key.Use a stable identifier to prevent stale child identity when toggling “Show more”.
- return <RepositoryItem key={index} details={repository} /> + return <RepositoryItem key={repository.key} details={repository} />
43-48: Prefer semantic links over buttons for navigation.Use Next.js Link for accessibility, semantics, and better prefetch.
Add import:
+import Link from 'next/link'Update markup:
- <button - onClick={handleClick} - className="min-w-0 flex-1 text-start font-semibold text-blue-400 hover:underline" - > - <TruncatedText text={details?.name} /> - </button> + <Link + href={`/organizations/${details.organization.login}/repositories/${details.key}`} + className="min-w-0 flex-1 text-start font-semibold text-blue-400 hover:underline" + > + <TruncatedText text={details?.name} /> + </Link>frontend/__tests__/unit/components/StatusBadge.test.tsx (2)
274-281: Reduce brittleness in “renders consistently across multiple instances”.Comparing full className strings is order-sensitive. Prefer asserting required classes via toHaveClass.
- const firstRender = screen.getByText('Archived').className - - rerender(<StatusBadge status="archived" />) - const secondRender = screen.getByText('Archived').className - expect(firstRender).toBe(secondRender) + const base = ['inline-flex', 'items-center', 'rounded-full', 'border', 'font-medium'] + const first = screen.getByText('Archived') + base.forEach(c => expect(first).toHaveClass(c)) + rerender(<StatusBadge status="archived" />) + const second = screen.getByText('Archived') + base.forEach(c => expect(second).toHaveClass(c))
309-320: Avoid counting raw elements.Span counts can change with harmless markup tweaks. Assert on the rendered statuses instead.
- expect(container.querySelectorAll('span').length).toBe(2) + expect(screen.getByText('Archived')).toBeInTheDocument() + expect(screen.getByText('Inactive')).toBeInTheDocument()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/__tests__/unit/components/ArchivedBadge.test.tsx(0 hunks)frontend/__tests__/unit/components/RepositoriesCard.test.tsx(1 hunks)frontend/__tests__/unit/components/StatusBadge.test.tsx(1 hunks)frontend/src/components/ArchivedBadge.tsx(0 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/RepositoriesCard.tsx(2 hunks)frontend/src/components/StatusBadge.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/src/components/ArchivedBadge.tsx
- frontend/tests/unit/components/ArchivedBadge.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/tests/unit/components/RepositoriesCard.test.tsx
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/components/StatusBadge.tsx
🔇 Additional comments (3)
frontend/src/components/RepositoriesCard.tsx (2)
8-8: Good migration to StatusBadge.Import and usage align with the new reusable component; no issues.
51-51: Confirm icon visibility choice for archived badge.You’re hiding the icon (showIcon={false}). Verify this matches the new UX across pages (CardDetailsPage, etc.) for consistency.
frontend/__tests__/unit/components/StatusBadge.test.tsx (1)
1-3: No action required — jest-dom is properly configured globally.Verification confirms jest.setup.ts imports jest-dom at the entry point, and jest.config.ts correctly references it via
setupFilesAfterEnv: ['<rootDir>/jest.setup.ts']. All jest-dom matchers (toBeInTheDocument, toHaveClass, toHaveAttribute, toBeVisible, etc.) used in StatusBadge.test.tsx will work correctly through the global setup.
There was a problem hiding this comment.
Great work! Thanks for working on this! 👍🏼
One suggestion for any future PRs - could you please not force-push updates when changes were requested? This makes it not possible for me as a reviewer to see any changes from last review. I can only see the whole thing, but not incremental changes.
Regular push works just fine in most cases 👌🏼
2 participants
Proposed change
This PR refactors the
ArchivedBadgecomponent into a more flexible and reusableStatusBadgecomponent that supports multiple status types. This eliminates code duplication and provides a consistent, maintainable solution for displaying status indicators across the application.Fixed #2418
Status Configurations:
bg-yellow-50with archive iconbg-red-50with ban iconChecklist
make check-testlocally; all checks and tests passed.