feat: improve status badge ui #2761
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces single-string status color with a structured StatusColorConfig ({ badge, dot }), adds Status props Changes
Sequence Diagram(s)(omitted) Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (4)src/components/Notifications/NotificationsStatusCell.tsx (1)
src/components/Notifications/NotificationSendHistory.tsx (4)
src/components/Notifications/Rules/notificationsRulesTableColumns.tsx (1)
src/components/Notifications/NotificationSendHistorySummary.tsx (1)
🔇 Additional comments (16)
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 |
yashmehrotra
force-pushed
the
design-sidebar
branch
from
f1713f3 to
36bcf9e
Compare
yashmehrotra
force-pushed
the
status-badge
branch
from
6174659 to
6203164
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/audit-report/components/View/ViewCard.tsx (1)
357-377: Health badge renders with empty children.The Badge for
healthtype has no text content—only classes are set butchildrenis empty. This appears to be an incomplete implementation or accidental removal.Consider adding the health value as badge content:
cellContent = ( <Badge variant={ healthValue === "healthy" ? "secondary" : healthValue === "warning" ? "default" : "destructive" } className={ healthValue === "healthy" ? "bg-green-100 text-green-800 hover:bg-green-100/80" : healthValue === "warning" ? "bg-yellow-100 text-yellow-800 hover:bg-yellow-100/80" : "" } - ></Badge> + > + {healthValue} + </Badge> );
🧹 Nitpick comments (1)
src/pages/audit-report/components/View/ViewCard.tsx (1)
463-522: Well-structured icon handling with appropriate status column detection.The logic correctly skips adding a redundant health dot for status columns since the
Statuscomponent already renders one. ThecloneElementapproach for injecting icons into existing Badge components is reasonable.One minor observation: the health icon colors here (
green-500,yellow-500,red-500) differ slightly from those ingetStatusColor(green-400,light-orange,red-400). If visual consistency across all status indicators is desired, consider extracting these color mappings to a shared location.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Status/index.tsx(2 hunks)src/pages/audit-report/components/DynamicDataTable.tsx(1 hunks)src/pages/audit-report/components/View/ViewCard.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/audit-report/components/View/ViewCard.tsx (2)
src/components/Status/index.tsx (1)
Status(44-70)src/components/ui/badge.tsx (1)
Badge(36-36)
src/pages/audit-report/components/DynamicDataTable.tsx (1)
src/components/Status/index.tsx (1)
Status(44-70)
🔇 Additional comments (3)
src/pages/audit-report/components/DynamicDataTable.tsx (1)
440-445: Clean integration with the updated Status component.Using
statusTextto pass the original cell content whilestatuscontrols the color is the correct approach. This removes the need for a wrapper span and leverages the Status component's Badge-based rendering.src/components/Status/index.tsx (1)
3-33: Good addition ofmixedstate handling.The priority order in
getStatusColoris sensible: explicit negative statuses take precedence, followed by unknown/warning, then themixedboolean flag, defaulting to green. Using the same color as warning (bg-light-orange) for mixed states provides visual consistency.src/pages/audit-report/components/View/ViewCard.tsx (1)
379-381: Consistent use of Status component for status columns.This aligns with the changes in
DynamicDataTable.tsxand properly delegates status rendering to the unifiedStatuscomponent.
yashmehrotra
force-pushed
the
status-badge
branch
from
6203164 to
a7f6e65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/audit-report/components/View/ViewCard.tsx (2)
471-522: Emptyifblock is a code smell—consider inverting the condition.The empty block at lines 472-474 is harder to read. Inverting the condition eliminates the empty branch and improves clarity.
🔎 Suggested refactor
- // Skip adding health icon for status columns since Status component already has the dot - if (isHealthIcon && column.type === "status") { - // Status component already handles the colored dot, skip adding another - } else { + // Skip adding health icon for status columns since Status component already has the dot + if (!(isHealthIcon && column.type === "status")) { const isBadge = React.isValidElement(cellContent) && cellContent.type === Badge; // ... rest of the logic - } + }
478-498: Minor style inconsistency: span vs. SVG for dots.The
Statuscomponent uses an SVG withfill-*classes for dots, while this code uses a<span>withbg-*classes. Both work, but using the same approach would improve visual consistency and maintainability.🔎 Optional: Use SVG for consistency with Status component
if (isHealthIcon) { const iconColor = iconStr === "healthy" - ? "bg-green-500" + ? "fill-green-500" : iconStr === "warning" - ? "bg-yellow-500" + ? "fill-yellow-500" : iconStr === "unhealthy" - ? "bg-red-500" - : "bg-gray-400"; + ? "fill-red-500" + : "fill-gray-400"; iconElement = ( - <span className={`h-2 w-2 rounded-full ${iconColor}`} /> + <svg + viewBox="0 0 6 6" + aria-hidden="true" + className={`size-2 ${iconColor}`} + > + <circle r={3} cx={3} cy={3} /> + </svg> );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Status/index.tsxsrc/pages/audit-report/components/DynamicDataTable.tsxsrc/pages/audit-report/components/View/ViewCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/audit-report/components/DynamicDataTable.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/audit-report/components/View/ViewCard.tsx (1)
src/components/Status/index.tsx (1)
Status(69-97)
🔇 Additional comments (6)
src/components/Status/index.tsx (2)
1-58: Well-structured color configuration approach.The
StatusColorConfigtype andgetStatusColorConfigfunction provide a clean, maintainable way to handle status colors with proper dark mode support. The separation of badge and dot colors allows for flexible styling.
83-96: Clean implementation with past feedback addressed.The
classNameis now correctly applied only to the outer<span>container (line 85), not the dot element. The SVG-based dot withfill-*classes is a good choice for consistent rendering.src/pages/audit-report/components/View/ViewCard.tsx (4)
23-23: Import added correctly.
357-377: Badge is rendered without any content.The
Badgecomponent has no children, resulting in an empty badge with just background color. If this is intentional (icon-only display), consider adding a comment. Otherwise, the health status text should be displayed.🔎 If content should be displayed, consider this fix
className={ healthValue === "healthy" ? "bg-green-100 text-green-800 hover:bg-green-100/80" : healthValue === "warning" ? "bg-yellow-100 text-yellow-800 hover:bg-yellow-100/80" : "" } - ></Badge> + > + <span className="capitalize">{healthValue}</span> + </Badge>Alternatively, consider using the
Statuscomponent here for consistency with the "status" type rendering.
379-381: Good use of the Status component.Clean integration with the refactored
Statuscomponent for consistent status badge rendering.
500-512:cloneElementapproach is functional but fragile.Injecting children via
React.cloneElementworks but is tightly coupled to Badge's internal structure. If Badge's implementation changes, this could break silently. This is acceptable for now but worth noting for future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/Badge/Badge.tsx (1)
47-53: Critical: Fix SVG syntax error.Line 49 has a malformed className string with a misplaced quote that terminates the template literal prematurely. The
fillandviewBoxattributes are incorrectly embedded in the className string rather than being separate SVG attributes.🔎 Proposed fix
{dot != null && ( <svg - className={`${svgClassName} text-blue-400" fill="${dot}" viewBox="0 0 8 8"`} + className={`${svgClassName} text-blue-400`} + fill={dot} + viewBox="0 0 8 8" > <circle cx={4} cy={4} r={3} /> </svg>
🧹 Nitpick comments (1)
src/components/Status/index.tsx (1)
67-82: The custom Tailwind color is properly configured; consider optimizing for direct lookup.The
bg-light-orangecolor is properly defined in the Tailwind configuration (#FBBE67). However,getDotBgClasscan be optimized from O(n) iteration to O(1) direct lookup:function getDotBgClass(dotClass: string): string { - for (const [fill, bg] of Object.entries(fillToBgMap)) { - if (dotClass.includes(fill)) { - return bg; - } - } - return "bg-gray-400"; + const match = dotClass.match(/fill-\w+-\d+/); + if (match) { + return fillToBgMap[match[0]] ?? "bg-gray-400"; + } + return "bg-gray-400"; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
pages/application.csssrc/components/Canary/CanaryStatsCard.tsxsrc/components/Canary/CanaryTableColumns.tsxsrc/components/Canary/Columns/index.tsxsrc/components/Canary/renderers.tsxsrc/components/Configs/ConfigSummary/Cells/ConfigSummaryHealthCells.tsxsrc/components/Configs/ConfigSummary/ConfigSummaryList.tsxsrc/components/Notifications/NotificationSendHistorySummary.tsxsrc/components/Notifications/NotificationsStatusCell.tsxsrc/components/Status/index.tsxsrc/components/StatusLine/StatusLine.tsxsrc/pages/audit-report/components/HealthBadge.tsxsrc/ui/Badge/Badge.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
src/components/Configs/ConfigSummary/Cells/ConfigSummaryHealthCells.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
src/components/Canary/CanaryStatsCard.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
src/pages/audit-report/components/HealthBadge.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
src/components/StatusLine/StatusLine.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
src/components/Canary/renderers.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
src/components/Canary/Columns/index.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
src/components/Notifications/NotificationsStatusCell.tsx (1)
src/components/Status/index.tsx (1)
Status(95-154)
🔇 Additional comments (20)
src/ui/Badge/Badge.tsx (1)
43-43: Removal of font-medium is intentional.The removal of
font-mediumfrom the badge styling aligns with the PR's objective to improve status badge UI consistency.pages/application.css (1)
41-43: LGTM!The removal of
font-mediumfrom the.status-badgeclass is consistent with the Badge component changes and aligns with the PR's UI standardization objectives.src/components/Configs/ConfigSummary/ConfigSummaryList.tsx (1)
290-290: Whitespace-only change, no functional impact.src/pages/audit-report/components/HealthBadge.tsx (1)
2-42: Well-structured refactor to Status component.The bifurcated rendering approach correctly handles both print and non-print views:
- Print view maintains minimal inline styling for PDF compatibility
- Standard view delegates to the centralized Status component
The color mappings are consistent across both paths.
src/components/Status/index.tsx (2)
1-65: Solid architectural refactor to centralized color configuration.The introduction of
StatusColorConfigand the refactoredgetStatusColorConfigfunction provides a clean, maintainable approach to managing status colors with support for both badge and dot variants, including dark mode.
94-153: Status component correctly implements three rendering modes.The component cleanly handles:
- Dot-only variant for simple indicators
- Count display with dot
- Full badge with optional text
The conditional logic is clear and the className application to the dot variant (line 116) is appropriate since it's a standalone element without a wrapper.
src/components/StatusLine/StatusLine.tsx (1)
34-71: Clean migration to Status component.The
colorToStatusmapping correctly translates existing color semantics to Status values, and both linked and inline rendering paths are handled consistently.src/components/Configs/ConfigSummary/Cells/ConfigSummaryHealthCells.tsx (3)
7-19: Good addition of deterministic health ordering.The
healthOrderarray andorderByHealthhelper ensure health statuses are consistently displayed in severity order (healthy → unhealthy → warning → unknown).
38-64: Effective refactor to Status-based rendering with navigation.The migration from Count/CountBar to Status components wrapped in Links improves both visual consistency and user experience by making health counts clickable filters.
83-101: Aggregate cell correctly uses same ordering without navigation.Consistent with the individual cell rendering, using the same
orderByHealthlogic while appropriately omitting links for aggregate rows.src/components/Canary/renderers.tsx (1)
20-25: Appropriate use of dot variant for Canary status.Adding
variant="dot"to both status branches provides a compact, consistent visual indicator for Canary health checks.src/components/Canary/Columns/index.tsx (2)
42-42: LGTM: Consistent adoption of dot variant.The addition of
variant="dot"aligns with the Status component's API and provides a more compact visual representation for the status column.
46-46: LGTM: Dot variant applied consistently.The
variant="dot"addition maintains consistency with the object-value branch above.src/components/Canary/CanaryTableColumns.tsx (1)
80-80: LGTM: Consistent dot variant usage.The addition of
variant="dot"to the Health column's Status component is consistent with similar changes across Canary components.src/components/Notifications/NotificationsStatusCell.tsx (2)
93-93: LGTM: Clean Status component integration.The rendering logic correctly uses the Status component with
statusandstatusTextprops from the configuration.
5-72: Status component properly supports all status values.The Status component's
getStatusColorConfigfunction (src/components/Status/index.tsx, lines 6-65) correctly handles all four status values used in the notification status object:
"healthy"→ Default case returns green colors (line 60)"unhealthy"→ Explicitly handled, returns red colors (line 17)"unknown"→ Explicitly handled, returns gray colors (line 29)"suppressed"→ Explicitly handled, returns gray colors (line 31)The refactoring is sound with no gaps in status value coverage.
src/components/Canary/CanaryStatsCard.tsx (3)
23-26: Review the failing count computation logic.The
failingCountcomputation subtractspassing.filteredorpassing.checksfrom the filtered or total checks length. Verify this logic is correct:
- When filtered:
filteredChecksLength - passing.filtered- When not filtered:
(checks?.length ?? 0) - passing.checksEnsure that
passing.filteredrepresents the count of passing checks within the filtered set, not a separate unrelated count.
29-47: LGTM: Clean Status component integration with tooltips.The refactoring from StatCard to Status components is well-executed. The tooltip integration provides good UX, and the Status components are used correctly with the
goodprop and customstatusText.
6-18: All consumers already updated with newpassingprop.Verification confirms that the only usage of
CanaryStatsCardsin the codebase (atsrc/pages/health.tsx:74) already provides the requiredpassingprop. No further updates needed.src/components/Notifications/NotificationSendHistorySummary.tsx (1)
109-133: The refactoring is clean and correct. The Status component explicitly handlesstatus="suppressed"(returns gray styling), so the conditional rendering at lines 125-129 works as intended. The change from CountBar to individual Status components improves clarity and flexibility.
Summary by CodeRabbit
Refactor
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.