feat: configurable status colors get stronger non-color support - #143
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds WCAG contrast utilities and tests; emits theme-aware CSS vars and a ChangesContrast utilities & tests
Badge styling & wiring
Steppers: icons, ARIA, readable slider text
Pill components: status chips and role=status
Requirements table & small accessibility tweaks
Docs / testing guidelines / i18n
Sequence Diagram(s)(Skipped — changes are primarily library/util, component rendering, ARIA attributes and tests; no multi-actor sequential control flow requiring diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
+ Coverage 57.66% 58.23% +0.56%
==========================================
Files 289 290 +1
Lines 17472 17616 +144
Branches 6674 6607 -67
==========================================
+ Hits 10076 10258 +182
+ Misses 7286 7247 -39
- Partials 110 111 +1
🚀 New features to boost your workflow:
|
![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 (2)
components/RequirementsTable.tsx (1)
2395-2407:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
packageItemStatuscolor dot is missingaria-hidden="true"— identical pattern to theriskLevelfix in this PR.The read-only variant of the
packageItemStatuscell (when noonPackageItemStatusChangehandler is present) renders the exact same structure — a colored dot followed by a text label — but the dot is not hidden from assistive technology. Screen readers will announce a meaningless empty element or color rectangle.As per coding guidelines: "Mark decorative icons with
aria-hidden="true""🛡️ Proposed fix
<span + aria-hidden="true" className="inline-block w-2.5 h-2.5 rounded-full shrink-0" style={{ backgroundColor: statusColor }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/RequirementsTable.tsx` around lines 2395 - 2407, The colored status dot in the read-only rendering of packageItemStatus is missing aria-hidden and will be announced by screen readers; update the inline dot element (the span that sets style={{ backgroundColor: statusColor }}) used alongside statusLabel in the packageItemStatus rendering to include aria-hidden="true" so the decorative color circle is ignored by assistive tech; keep the rest of the structure (statusColor, statusLabel, and the surrounding inline-flex span) unchanged and apply this change where onPackageItemStatusChange is not present (the read-only branch).components/SuggestionStepper.tsx (1)
155-180:⚠️ Potential issue | 🟠 MajorHardcoded
text-whiteon all three active step colors violates WCAG 1.4.3 AA.The active slider uses
className="… text-white"against step background colors with insufficient contrast:
Step Background Contrast (white) WCAG AA (4.5:1) Draft #3b82f6~3.67:1 ❌ fails Review Requested #eab308~1.92:1 ❌ fails Resolved #22c55e~2.28:1 ❌ fails The coding guidelines explicitly state: "a yellow status would render white-on-yellow and fail 1.4.3." Use
pickReadableTextOn(activeColor)fromlib/color-contrast.tsto choose between#ffffffand#111827at runtime.🔧 Proposed fix
import { CheckCircle2, Eye, type LucideIcon, PenLine } from 'lucide-react' +import { pickReadableTextOn } from '@/lib/color-contrast' import { useTranslations } from 'next-intl'+ const activeTextColor = pickReadableTextOn(activeColor) return ( // biome-ignore ...<div - className="h-10 flex items-center justify-center text-white" + className="h-10 flex items-center justify-center" style={{ backgroundColor: activeColor, + color: activeTextColor, clipPath: sliderClipPath(targetIndex === 0),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/SuggestionStepper.tsx` around lines 155 - 180, The active step text is hardcoded to "text-white" which fails WCAG contrast; update SuggestionStepper to compute the readable text color at render using pickReadableTextOn(activeColor) (from lib/color-contrast.ts) and apply that value to the active slider's text instead of the "text-white" class; specifically, remove the fixed "text-white" on the div that uses activeColor and set the span (or container) style/class to use the returned color (either "#ffffff" or "#111827") so icons and t(STEPS[targetIndex].translationKey) have sufficient contrast for the STEPS/targetIndex active state.
🧹 Nitpick comments (3)
tests/unit/deviation-pill.test.tsx (1)
56-64: ⚡ Quick winStrengthen
getAllByTextassertions —>= 1won't catch regressions.
getAllByTextalready throws on zero matches, so>= 1provides no additional signal. SincestatusApprovedandstatusRejectedappear in two places (the header chip and the decision section), the assertion should require at least 2 occurrences. If the chip were accidentally removed the count would fall to 1, and the current guard would still pass silently.♻️ Proposed fix
- // statusApproved appears twice: once in the header chip, once in the decision heading - expect(screen.getAllByText('statusApproved').length).toBeGreaterThanOrEqual( - 1, - ) + // statusApproved appears in both the header chip and the decision heading + expect(screen.getAllByText('statusApproved').length).toBeGreaterThanOrEqual(2)- expect(screen.getAllByText('statusRejected').length).toBeGreaterThanOrEqual( - 1, - ) + expect(screen.getAllByText('statusRejected').length).toBeGreaterThanOrEqual(2)Also applies to: 78-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/deviation-pill.test.tsx` around lines 56 - 64, The current test in deviation-pill.test.tsx uses getAllByText('statusApproved').length to assert >=1 which is too weak; update the assertion for getAllByText('statusApproved') (and the analogous getAllByText('statusRejected') later) to require two occurrences (e.g., expect(...length).toBeGreaterThanOrEqual(2) or expect(...length).toHaveLength(2)) because the text must appear in both the header chip and the decision section; adjust the assertions around the calls to getAllByText('statusApproved') and the similar block at lines 78-86 to enforce at least 2 matches.tests/unit/suggestion-pill.test.tsx (1)
84-90: ⚡ Quick winExtend the
stepprop override tests to cover all three values.The test only exercises
step="review_requested". Sincestep="draft"andstep="resolved"each produce different (and possibly surprising) outcomes — in particularstep="resolved"on an unresolved suggestion currently renders the draft chip — adding those cases documents the actual behavior and would have caught the issue flagged incomponents/SuggestionPill.tsx.🧪 Suggested additional test cases
+ it('forces draft appearance when step="draft" overrides review_requested data', () => { + const reviewRequested = { ...baseSuggestion, isReviewRequested: 1 } + const { container } = render( + <SuggestionPill step="draft" suggestion={reviewRequested} />, + ) + expect(container.querySelector('.border-blue-200')).toBeTruthy() + expect(screen.getByText('stepDraft')).toBeInTheDocument() + }) + + it('renders resolved chip when step="resolved" is provided (even if not yet resolved)', () => { + const { container } = render( + <SuggestionPill step="resolved" suggestion={baseSuggestion} />, + ) + // Verify the resolved chip appears, not a draft fallback + expect(screen.getByText('statusResolved')).toBeInTheDocument() + expect(container.querySelector('.border-green-200')).toBeTruthy() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/suggestion-pill.test.tsx` around lines 84 - 90, Add two more assertions in the unit test for SuggestionPill to exercise all three step prop values: render SuggestionPill with step="draft", step="review_requested", and step="resolved" (using the existing baseSuggestion and render helper) and assert the expected DOM for each case—e.g., presence of the corresponding border class and text node (the test already checks .border-yellow-200 and 'statusPending' for review_requested; add equivalent expects for draft and resolved, noting that resolved currently renders the draft chip so assert that behavior). Locate usage of SuggestionPill and baseSuggestion in tests/unit/suggestion-pill.test.tsx and add the two new cases to the same it block or separate it blocks.lib/color-contrast.ts (1)
133-152: 💤 Low valueVerify the step count handles extreme color adjustments.
Both
clampForReadabilityandlightenForReadabilityreuseMAX_DARKEN_STEPS = 12. WithDARKEN_STEP = 0.05, the maximum lightness adjustment is 0.60 (60% of the L range). This should suffice for most DB-driven status colors, but very bright colors on white (or very dark on dark) may fall through to the fallback.The fallback behavior is correct, but consider whether 12 steps is adequate for all real-world status colors. The test suite exercises the fallback path, which is good.
💡 Optional: Rename constant for clarity
Since this constant is used for both darkening and lightening, a more neutral name would improve clarity:
-const MAX_DARKEN_STEPS = 12 +const MAX_ADJUSTMENT_STEPS = 12Then update references in both functions.
Also applies to: 160-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/color-contrast.ts` around lines 133 - 152, clampForReadability and lightenForReadability use MAX_DARKEN_STEPS with DARKEN_STEP which limits total lightness change to 0.60 and can let extreme colors fall back; update this by either increasing the step count or renaming and adjusting the constant to reflect total lightness range used by both functions (e.g., MAX_LIGHTNESS_STEPS) and/or reducing DARKEN_STEP so the product MAX_*_STEPS * DARKEN_STEP covers the full required range; change all references to the old constant (MAX_DARKEN_STEPS) in both clampForReadability and lightenForReadability and run tests to ensure the fallback path still behaves as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/SuggestionPill.tsx`:
- Around line 51-77: The statusChip logic incorrectly falls through to the draft
variant when a caller passes step="resolved" but isResolved is false; update the
conditional in SuggestionPill.tsx (statusChip, isResolved, effectiveStep) to
explicitly handle effectiveStep === 'resolved' before the final draft branch (or
alternatively restrict the step prop type to only 'draft' | 'review_requested');
ensure the new branch returns the intended resolved/dismissed visuals (Icon,
label, className) for the resolved step so previewing a resolved pill shows the
correct styling even when isResolved is false.
---
Outside diff comments:
In `@components/RequirementsTable.tsx`:
- Around line 2395-2407: The colored status dot in the read-only rendering of
packageItemStatus is missing aria-hidden and will be announced by screen
readers; update the inline dot element (the span that sets style={{
backgroundColor: statusColor }}) used alongside statusLabel in the
packageItemStatus rendering to include aria-hidden="true" so the decorative
color circle is ignored by assistive tech; keep the rest of the structure
(statusColor, statusLabel, and the surrounding inline-flex span) unchanged and
apply this change where onPackageItemStatusChange is not present (the read-only
branch).
In `@components/SuggestionStepper.tsx`:
- Around line 155-180: The active step text is hardcoded to "text-white" which
fails WCAG contrast; update SuggestionStepper to compute the readable text color
at render using pickReadableTextOn(activeColor) (from lib/color-contrast.ts) and
apply that value to the active slider's text instead of the "text-white" class;
specifically, remove the fixed "text-white" on the div that uses activeColor and
set the span (or container) style/class to use the returned color (either
"#ffffff" or "#111827") so icons and t(STEPS[targetIndex].translationKey) have
sufficient contrast for the STEPS/targetIndex active state.
---
Nitpick comments:
In `@lib/color-contrast.ts`:
- Around line 133-152: clampForReadability and lightenForReadability use
MAX_DARKEN_STEPS with DARKEN_STEP which limits total lightness change to 0.60
and can let extreme colors fall back; update this by either increasing the step
count or renaming and adjusting the constant to reflect total lightness range
used by both functions (e.g., MAX_LIGHTNESS_STEPS) and/or reducing DARKEN_STEP
so the product MAX_*_STEPS * DARKEN_STEP covers the full required range; change
all references to the old constant (MAX_DARKEN_STEPS) in both
clampForReadability and lightenForReadability and run tests to ensure the
fallback path still behaves as expected.
In `@tests/unit/deviation-pill.test.tsx`:
- Around line 56-64: The current test in deviation-pill.test.tsx uses
getAllByText('statusApproved').length to assert >=1 which is too weak; update
the assertion for getAllByText('statusApproved') (and the analogous
getAllByText('statusRejected') later) to require two occurrences (e.g.,
expect(...length).toBeGreaterThanOrEqual(2) or
expect(...length).toHaveLength(2)) because the text must appear in both the
header chip and the decision section; adjust the assertions around the calls to
getAllByText('statusApproved') and the similar block at lines 78-86 to enforce
at least 2 matches.
In `@tests/unit/suggestion-pill.test.tsx`:
- Around line 84-90: Add two more assertions in the unit test for SuggestionPill
to exercise all three step prop values: render SuggestionPill with step="draft",
step="review_requested", and step="resolved" (using the existing baseSuggestion
and render helper) and assert the expected DOM for each case—e.g., presence of
the corresponding border class and text node (the test already checks
.border-yellow-200 and 'statusPending' for review_requested; add equivalent
expects for draft and resolved, noting that resolved currently renders the draft
chip so assert that behavior). Locate usage of SuggestionPill and baseSuggestion
in tests/unit/suggestion-pill.test.tsx and add the two new cases to the same it
block or separate it blocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fcf0f9e-05b5-4104-8a42-ac317b2428f5
📒 Files selected for processing (20)
.github/instructions/tests.instructions.md.github/instructions/ui-ux.instructions.mdapp/globals.csscomponents/DeviationPill.tsxcomponents/DeviationStepper.tsxcomponents/RequirementsTable.tsxcomponents/StatusBadge.tsxcomponents/StatusStepper.tsxcomponents/SuggestionPill.tsxcomponents/SuggestionStepper.tsxlib/__tests__/color-contrast.test.tslib/color-contrast.tsmessages/en.jsonmessages/sv.jsontests/unit/deviation-pill.test.tsxtests/unit/deviation-stepper.test.tsxtests/unit/status-badge.test.tsxtests/unit/status-stepper.test.tsxtests/unit/suggestion-pill.test.tsxtests/unit/suggestion-stepper.test.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
components/SuggestionStepper.tsx (1)
56-64: ⚡ Quick winUse the standard
ComponentPropsinterface name for props.Please rename
SuggestionStepperPropstoComponentPropsand update the component signature accordingly for consistency with repo conventions.As per coding guidelines "
components/**/*.tsx: Define component props using an interface named ComponentProps".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/SuggestionStepper.tsx` around lines 56 - 64, Rename the props interface SuggestionStepperProps to ComponentProps and update the component signature to use ComponentProps; specifically change the interface name and the type annotation in the SuggestionStepper function parameter (the destructured props { currentStep, developerModeContext }: SuggestionStepperProps) to use ComponentProps instead, and ensure any other references to SuggestionStepperProps in this file are updated to ComponentProps for consistency with the components/**/*.tsx convention.tests/unit/deviation-pill.test.tsx (1)
39-40: ⚡ Quick winUse Testing Library role queries for the status assertion.
Prefer
screen.getByRole('status')here instead ofcontainer.querySelector('[role="status"]')to keep the test aligned with user-facing accessibility semantics.As per coding guidelines, “Test ARIA roles and attributes on status and state components by asserting
role... values usingscreen.getByRole()”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/deviation-pill.test.tsx` around lines 39 - 40, Replace the DOM query using container.querySelector('[role="status"]') with Testing Library's role query — call screen.getByRole('status') in the deviation-pill.test.tsx assertion so the test uses user-facing semantics; ensure you have the render call that sets up screen (or import screen from '@testing-library/react') and change the expect to assert the element returned by screen.getByRole('status').tests/unit/suggestion-pill.test.tsx (2)
25-26: ⚡ Quick winAdd a
beforeEach(() => vi.clearAllMocks())in thisdescribeblock.The suite currently skips mock clearing between tests; please add the standard
beforeEachhook for consistency and isolation.As per coding guidelines, “Structure tests with
describe()blocks containing abeforeEach(() => vi.clearAllMocks())hook andit()test cases.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/suggestion-pill.test.tsx` around lines 25 - 26, Add a beforeEach hook to clear mocks in the describe('SuggestionPill') test suite: inside the describe block (before the it() tests) add beforeEach(() => vi.clearAllMocks()) so vi.clearAllMocks() runs before each test, ensuring isolation between tests in this suite.
34-35: ⚡ Quick winPrefer
screen.getByRole('status')over selector-based role checks.Use a role query for this assertion instead of
container.querySelector('[role="status"]')to keep the test accessibility-first and resilient.As per coding guidelines, “Test ARIA roles and attributes on status and state components by asserting
role... values usingscreen.getByRole()”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/suggestion-pill.test.tsx` around lines 34 - 35, Replace the selector-based assertion that checks for a status role (container.querySelector('[role="status"]')) with an accessibility-first role query using screen.getByRole('status'); update the test in suggestion-pill.test.tsx to call screen.getByRole('status') and assert its presence (e.g., toBeInTheDocument()), and ensure the file imports or uses the testing-library's screen helper rather than relying on container.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/SuggestionStepper.tsx`:
- Around line 56-64: Rename the props interface SuggestionStepperProps to
ComponentProps and update the component signature to use ComponentProps;
specifically change the interface name and the type annotation in the
SuggestionStepper function parameter (the destructured props { currentStep,
developerModeContext }: SuggestionStepperProps) to use ComponentProps instead,
and ensure any other references to SuggestionStepperProps in this file are
updated to ComponentProps for consistency with the components/**/*.tsx
convention.
In `@tests/unit/deviation-pill.test.tsx`:
- Around line 39-40: Replace the DOM query using
container.querySelector('[role="status"]') with Testing Library's role query —
call screen.getByRole('status') in the deviation-pill.test.tsx assertion so the
test uses user-facing semantics; ensure you have the render call that sets up
screen (or import screen from '@testing-library/react') and change the expect to
assert the element returned by screen.getByRole('status').
In `@tests/unit/suggestion-pill.test.tsx`:
- Around line 25-26: Add a beforeEach hook to clear mocks in the
describe('SuggestionPill') test suite: inside the describe block (before the
it() tests) add beforeEach(() => vi.clearAllMocks()) so vi.clearAllMocks() runs
before each test, ensuring isolation between tests in this suite.
- Around line 34-35: Replace the selector-based assertion that checks for a
status role (container.querySelector('[role="status"]')) with an
accessibility-first role query using screen.getByRole('status'); update the test
in suggestion-pill.test.tsx to call screen.getByRole('status') and assert its
presence (e.g., toBeInTheDocument()), and ensure the file imports or uses the
testing-library's screen helper rather than relying on container.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b2f540f-9db0-408e-9dd6-e24427b6c2df
📒 Files selected for processing (7)
components/RequirementsTable.tsxcomponents/SuggestionPill.tsxcomponents/SuggestionStepper.tsxtests/unit/deviation-pill.test.tsxtests/unit/requirements-table.test.tsxtests/unit/suggestion-pill.test.tsxtests/unit/suggestion-stepper.test.tsx
✅ Files skipped from review due to trivial changes (2)
- tests/unit/requirements-table.test.tsx
- components/RequirementsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/suggestion-stepper.test.tsx
…; update tests to use screen.getByRole for status checks
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Description
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is