fix: light-theme alert contrast — add an AA-safe ink step to the alert ramp#3579
Merged
Conversation
…t ramp
The light alert ramp was anchored for vibrancy, not legibility. Measured
against the surfaces it actually lands on:
light default on level01 / level02
warning 2.38 / 2.15 below the 3:1 non-text floor
info 2.84 / 2.57 below
success 2.96 / 2.68 below
error 5.07 / 4.58 passing
Three of the four families missed WCAG SC 1.4.11 in light theme, so any
feature using one as its only chromatic mark stopped carrying meaning
there — the task-relationship-links card's blocked glyph, the header's
blocked-by chip border, the blocker picker. Dark theme measured clean for
those three but error's default sits at 4.25:1 on cards, under AA for text.
Nothing in the build checked the palette, which is how this shipped and
stayed. Two changes at the token source, both using values already in the
ramp — no new hexes:
* alert.<family>.ink — the least-extreme step of the same ramp that
clears 4.5:1, resolved per brightness. Light: hover for error, pressed
for the rest. Dark: hover for error, default for the rest, so dark
warning/success/info text is unchanged. "Least-extreme" rather than a
uniform pressed keeps error text a red instead of a maroon.
* light default moved onto the family's hover value for warning, info and
success, so fills, dots, borders and glyphs clear 3:1. This collapses
default and hover in light for those three; nothing binds either today,
and a future Figma pass should re-derive a distinct light hover.
Consumers split by role: fills, dots, borders and chart series keep
default; anything painting the tone as text moves to ink. Four widgets that
hand-rolled exactly this with Theme.of(context).brightness branches
(insights_delta_chip, impact_model_table, onboarding_api_key_panel x2)
collapse to a single token read.
design_tokens_test now pins both floors across four families, two
brightnesses and two surfaces, plus the invariant that ink is never weaker
than default. level03 is excluded deliberately: it is a mid-grey chip fill
and no step of the dark error ramp can reach AA against it.
The blocked-by chip and section header needed no change — both already put
amber only on a border and a glyph and keep their labels neutral, which is
non-text and now clears its floor. The tasks README claimed an amber label;
corrected.
Closes lotti3-mq9.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe alert design tokens now include contrast-focused ChangesAlert token contract and design-system consumers
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3579 +/- ##
=======================================
Coverage 99.14% 99.15%
=======================================
Files 1783 1783
Lines 130627 130629 +2
=======================================
+ Hits 129515 129520 +5
+ Misses 1112 1109 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/features/insights/ui/widgets/insights_delta_chip_test.dart (1)
103-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale contrast rationale.
The comment immediately above still says light uses
pressedand dark useshover, but both themes now usealert.success.ink. Update it to describe the token-levelinkcontract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/insights/ui/widgets/insights_delta_chip_test.dart` around lines 103 - 114, Update the comment immediately above the light and dark icon color assertions to describe the shared token-level alert.success.ink contract, replacing the stale pressed-versus-hover rationale; leave the test expectations unchanged.lib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dart (1)
336-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSeparate alert text colors from decorative surface colors.
Both sites apply
*.inkto text and to fills/borders. Use*.inkonly for alert-toned text, and the corresponding*.defaultColorfor backgrounds and borders.
lib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dart#L336-L350: splitdangerinto an ink color for the heading and a default color for the container fill and border.lib/features/daily_os_next/ui/pages/refine_page_widgets.dart#L192-L200: split the message text color from the warning/error decoration color.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dart` around lines 336 - 350, Separate alert text and decoration colors at both affected sites: in lib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dart#L336-L350, update the error result modal’s color variables so heading text uses tokens.colors.alert.error.ink while the Container background and border use tokens.colors.alert.error.defaultColor; in lib/features/daily_os_next/ui/pages/refine_page_widgets.dart#L192-L200, apply the same split to the warning/error message text versus its decorative fill and border, preserving the existing styling and opacity values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/agents/ui/ai_summary_card/proposal_row_part.dart`:
- Line 834: Preserve the alert token role split: in proposal_row_part.dart at
lines 834-834, separate the reject badge fill from its label color, using error
defaultColor for the fill and ink for text; in agent_pending_wakes_page.dart at
lines 83-83 and 141-141, use success defaultColor for the running dot and badge
background, and success ink for elapsed text; in diff_row.dart at lines 128-128,
use info defaultColor for the moved badge background and info ink for its
foregrounds.
In `@lib/features/agents/ui/listing/widgets/agent_list_row.dart`:
- Around line 245-247: Separate foreground and fill color resolution across
_toneColors and the listed consumers: in
lib/features/agents/ui/listing/widgets/agent_list_row.dart#L245-L247, keep alert
ink tokens for captions/labels and use corresponding default tokens for fills;
in lib/features/ai_consumption/ui/widgets/impact_model_table.dart#L90-L93, use
alert.error.defaultColor for the badge background and alert.error.ink for its
caption; in lib/features/daily_os_next/ui/widgets/capacity_donut.dart#L88-L88,
use separate error default and ink values for the ring and center/eyebrow text.
In `@lib/features/daily_os_next/ui/widgets/parsed_card.dart`:
- Around line 89-94: Separate alert surface and foreground token usage across
_KindBadge and the related chips/badges: use each alert token’s defaultColor for
translucent backgrounds, while retaining ink for text and icon foregrounds.
Apply this in lib/features/daily_os_next/ui/widgets/parsed_card.dart at lines
89-94 and 160-161, and lib/features/daily_os_next/ui/widgets/pending_card.dart
at lines 98-108.
In `@lib/features/design_system/README.md`:
- Around line 109-113: Complete the alert-role table in the README by finishing
the hover/pressed obligation with an explicit reference to the control’s
required contrast, and clarify glyph ownership: standalone glyphs remain under
defaultColor, while glyphs paired with alert-toned text use ink. Keep the
existing contrast requirements and role names unchanged.
In `@lib/features/tasks/ui/header/desktop_task_header_meta.dart`:
- Around line 295-304: The urgent pill currently uses one color for both its
tinted fill and label; update the DsPill construction in
lib/features/tasks/ui/header/desktop_task_header_meta.dart lines 295-304 to use
defaultColor for the fill/icons and ink for labelColor. Update the corresponding
assertions in test/features/tasks/ui/header/desktop_task_header_test.dart lines
1258-1265 to verify defaultColor as the pill background and ink as labelColor.
---
Outside diff comments:
In
`@lib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dart`:
- Around line 336-350: Separate alert text and decoration colors at both
affected sites: in
lib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dart#L336-L350,
update the error result modal’s color variables so heading text uses
tokens.colors.alert.error.ink while the Container background and border use
tokens.colors.alert.error.defaultColor; in
lib/features/daily_os_next/ui/pages/refine_page_widgets.dart#L192-L200, apply
the same split to the warning/error message text versus its decorative fill and
border, preserving the existing styling and opacity values.
In `@test/features/insights/ui/widgets/insights_delta_chip_test.dart`:
- Around line 103-114: Update the comment immediately above the light and dark
icon color assertions to describe the shared token-level alert.success.ink
contract, replacing the stale pressed-versus-hover rationale; leave the test
expectations unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8314960c-9567-4682-9e45-1f3035eb427a
⛔ Files ignored due to path filters (1)
lib/features/design_system/theme/generated/design_tokens.g.dartis excluded by!**/generated/**
📒 Files selected for processing (67)
CHANGELOG.mdassets/design_system/tokens.jsonflatpak/com.matthiasn.lotti.metainfo.xmllib/features/agents/ui/ai_summary_card/proposal_row_part.dartlib/features/agents/ui/listing/agent_listing_shell.dartlib/features/agents/ui/listing/widgets/active_filters_row.dartlib/features/agents/ui/listing/widgets/agent_list_row.dartlib/features/agents/ui/pending_wakes/agent_pending_wakes_page.dartlib/features/agents/ui/sidebar_wake_queue.dartlib/features/agents/ui/task_agent_identity_region.dartlib/features/ai/ui/settings/provider/ai_provider_connection_section.dartlib/features/ai/ui/settings/provider/ai_provider_detail_widgets.dartlib/features/ai/ui/settings/widgets/ftue/ai_provider_setup_result_modal.dartlib/features/ai/ui/settings/widgets/v2/ai_profile_card.dartlib/features/ai/ui/settings/widgets/v2/ai_provider_card.dartlib/features/ai_consumption/ui/widgets/impact_model_table.dartlib/features/daily_os_next/ui/pages/capture_page.dartlib/features/daily_os_next/ui/pages/daily_os_next_root.dartlib/features/daily_os_next/ui/pages/refine_page_widgets.dartlib/features/daily_os_next/ui/widgets/agenda_view.dartlib/features/daily_os_next/ui/widgets/capacity_donut.dartlib/features/daily_os_next/ui/widgets/day_agent_status_chip.dartlib/features/daily_os_next/ui/widgets/day_timeline_widgets.dartlib/features/daily_os_next/ui/widgets/diff_row.dartlib/features/daily_os_next/ui/widgets/knowledge_panel.dartlib/features/daily_os_next/ui/widgets/link_badge.dartlib/features/daily_os_next/ui/widgets/parsed_card.dartlib/features/daily_os_next/ui/widgets/pending_card.dartlib/features/design_system/README.mdlib/features/design_system/components/badges/design_system_badge.dartlib/features/design_system/components/context_menus/design_system_context_menu.dartlib/features/design_system/components/file_uploads/design_system_file_upload_item.dartlib/features/design_system/components/inputs/design_system_text_input.dartlib/features/design_system/components/task_filters/design_system_filter_modal.dartlib/features/design_system/components/task_list_items/design_system_task_list_item.dartlib/features/design_system/components/textareas/design_system_textarea.dartlib/features/insights/ui/widgets/insights_delta_chip.dartlib/features/journal/ui/widgets/list_cards/journal_card.dartlib/features/knowledge_graph_poc/ui/task_knowledge_graph_page.dartlib/features/labels/ui/pages/label_details_page.dartlib/features/onboarding/ui/widgets/onboarding_api_key_panel.dartlib/features/projects/ui/widgets/project_selection_modal_content.dartlib/features/settings_v2/ui/tree/settings_tree_row.dartlib/features/sync/ui/backfill_settings_stats.dartlib/features/sync/ui/pages/conflicts/conflict_detail_route.dartlib/features/sync/ui/widgets/conflicts/conflict_resolution_view.dartlib/features/sync/ui/widgets/outbox/outbox_summary_header.dartlib/features/tasks/README.mdlib/features/tasks/ui/header/desktop_task_header_meta.dartlib/features/tasks/ui/widgets/task_showcase_palette.dartlib/pages/create/create_measurement_dialog.dartlib/widgets/charts/habits/habit_completion_rate_chart.dartlib/widgets/settings/settings_delete_row.darttest/features/ai/ui/settings/widgets/v2/ai_card_action_menu_test.darttest/features/daily_os_next/ui/widgets/capacity_donut_test.darttest/features/daily_os_next/ui/widgets/day_timeline_test.darttest/features/daily_os_next/ui/widgets/link_badge_test.darttest/features/daily_os_next/ui/widgets/parsed_card_test.darttest/features/design_system/components/badges/design_system_badge_test.darttest/features/design_system/components/context_menus/design_system_context_menu_test.darttest/features/design_system/components/task_list_items/design_system_task_list_item_test.darttest/features/design_system/theme/design_tokens_test.darttest/features/insights/ui/widgets/insights_delta_chip_test.darttest/features/journal/ui/widgets/list_cards/journal_card_test.darttest/features/tasks/ui/header/desktop_task_header_test.darttest/features/tasks/ui/widgets/task_showcase_palette_test.darttest/test_utils/wcag_contrast.dart
| final messages = context.messages; | ||
| final accept = kind == ProposalResolveKind.accept; | ||
| final color = accept ? ai.accent : tokens.colors.alert.error.defaultColor; | ||
| final color = accept ? ai.accent : tokens.colors.alert.error.ink; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the alert token role split across consumers.
These changes route ink into fills or dots as well as text. Use defaultColor for surfaces/fills/dots and ink for text foregrounds.
lib/features/agents/ui/ai_summary_card/proposal_row_part.dart#L834-L834: split reject badge fill and label colors.lib/features/agents/ui/pending_wakes/agent_pending_wakes_page.dart#L83-L83: use successdefaultColorfor the running dot.lib/features/agents/ui/pending_wakes/agent_pending_wakes_page.dart#L141-L141: use successdefaultColorfor the badge background andinkfor elapsed text.lib/features/daily_os_next/ui/widgets/diff_row.dart#L128-L128: use infodefaultColorfor the moved badge background andinkfor its foregrounds.
📍 Affects 3 files
lib/features/agents/ui/ai_summary_card/proposal_row_part.dart#L834-L834(this comment)lib/features/agents/ui/pending_wakes/agent_pending_wakes_page.dart#L83-L83lib/features/agents/ui/pending_wakes/agent_pending_wakes_page.dart#L141-L141lib/features/daily_os_next/ui/widgets/diff_row.dart#L128-L128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/agents/ui/ai_summary_card/proposal_row_part.dart` at line 834,
Preserve the alert token role split: in proposal_row_part.dart at lines 834-834,
separate the reject badge fill from its label color, using error defaultColor
for the fill and ink for text; in agent_pending_wakes_page.dart at lines 83-83
and 141-141, use success defaultColor for the running dot and badge background,
and success ink for elapsed text; in diff_row.dart at lines 128-128, use info
defaultColor for the moved badge background and info ink for its foregrounds.
Comment on lines
+245
to
+247
| AgentListPillTone.warning => colors.alert.warning.ink, | ||
| AgentListPillTone.error => colors.alert.error.ink, | ||
| AgentListPillTone.info => colors.alert.info.ink, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate alert text tokens from alert fill tokens across these consumers.
The new ink token is correctly intended for text, but these sites also use the same value to derive fills. Keep ink for captions/labels and use the corresponding default token for surfaces, ring fills, and translucent backgrounds.
lib/features/agents/ui/listing/widgets/agent_list_row.dart#L245-L247: split_toneColorsinto separate foreground and fill resolution.lib/features/ai_consumption/ui/widgets/impact_model_table.dart#L90-L93: usealert.error.defaultColorfor the badge background andalert.error.inkfor its caption.lib/features/daily_os_next/ui/widgets/capacity_donut.dart#L88-L88: use separate error fill and error ink values for the ring versus center/eyebrow text.
Based on the PR objective, default remains the fill token and ink is the text token.
📍 Affects 3 files
lib/features/agents/ui/listing/widgets/agent_list_row.dart#L245-L247(this comment)lib/features/ai_consumption/ui/widgets/impact_model_table.dart#L90-L93lib/features/daily_os_next/ui/widgets/capacity_donut.dart#L88-L88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/agents/ui/listing/widgets/agent_list_row.dart` around lines 245
- 247, Separate foreground and fill color resolution across _toneColors and the
listed consumers: in
lib/features/agents/ui/listing/widgets/agent_list_row.dart#L245-L247, keep alert
ink tokens for captions/labels and use corresponding default tokens for fills;
in lib/features/ai_consumption/ui/widgets/impact_model_table.dart#L90-L93, use
alert.error.defaultColor for the badge background and alert.error.ink for its
caption; in lib/features/daily_os_next/ui/widgets/capacity_donut.dart#L88-L88,
use separate error default and ink values for the ring and center/eyebrow text.
Comment on lines
+89
to
+94
| tokens.colors.alert.info.ink, | ||
| context.messages.dailyOsNextReconcileBadgeMatched, | ||
| Icons.link_rounded, | ||
| ), | ||
| ParsedItemKind.update => ( | ||
| tokens.colors.alert.success.defaultColor, | ||
| tokens.colors.alert.success.ink, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate alert surface and foreground token roles.
These changed colors are reused for translucent fills and for icon/text foregrounds. Per the new alert-token contract, use defaultColor for fills and ink for text/icons:
lib/features/daily_os_next/ui/widgets/parsed_card.dart#L89-L94: split_KindBadgesurface and foreground colors.lib/features/daily_os_next/ui/widgets/parsed_card.dart#L160-L161: usedefaultColorfor the matched-task chip background.lib/features/daily_os_next/ui/widgets/pending_card.dart#L98-L108: usedefaultColorfor the state-badge background.
📍 Affects 2 files
lib/features/daily_os_next/ui/widgets/parsed_card.dart#L89-L94(this comment)lib/features/daily_os_next/ui/widgets/parsed_card.dart#L160-L161lib/features/daily_os_next/ui/widgets/pending_card.dart#L98-L108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/daily_os_next/ui/widgets/parsed_card.dart` around lines 89 - 94,
Separate alert surface and foreground token usage across _KindBadge and the
related chips/badges: use each alert token’s defaultColor for translucent
backgrounds, while retaining ink for text and icon foregrounds. Apply this in
lib/features/daily_os_next/ui/widgets/parsed_card.dart at lines 89-94 and
160-161, and lib/features/daily_os_next/ui/widgets/pending_card.dart at lines
98-108.
Comment on lines
+109
to
+113
| | step | role | obligation | | ||
| | --- | --- | --- | | ||
| | `defaultColor` | fills, dots, borders, glyphs, chart series | ≥ 3:1 on `background.level01`/`level02` (WCAG SC 1.4.11) | | ||
| | `hover` / `pressed` | interaction states of a control already using the tone | inherits the control's | | ||
| | `ink` | **any alert-toned text**, and the glyph paired with it | ≥ 4.5:1 on `background.level01`/`level02` (SC 1.4.3) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete and disambiguate the alert-role table.
The hover/pressed obligation ends at “inherits the control’s”, and glyphs appears under both defaultColor and ink without distinguishing standalone glyphs from glyphs paired with alert text. This can send new consumers to the wrong token.
Suggested wording
-| `defaultColor` | fills, dots, borders, glyphs, chart series | ≥ 3:1 on `background.level01`/`level02` (WCAG SC 1.4.11) |
-| `hover` / `pressed` | interaction states of a control already using the tone | inherits the control's |
-| `ink` | **any alert-toned text**, and the glyph paired with it | ≥ 4.5:1 on `background.level01`/`level02` (SC 1.4.3) |
+| `defaultColor` | fills, dots, borders, standalone glyphs, chart series | ≥ 3:1 on `background.level01`/`level02` (WCAG SC 1.4.11) |
+| `hover` / `pressed` | interaction states of a control already using the tone | inherits the control's interaction-state contrast requirements |
+| `ink` | **any alert-toned text**, and glyphs paired with that text | ≥ 4.5:1 on `background.level01`/`level02` (SC 1.4.3) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | step | role | obligation | | |
| | --- | --- | --- | | |
| | `defaultColor` | fills, dots, borders, glyphs, chart series | ≥ 3:1 on `background.level01`/`level02` (WCAG SC 1.4.11) | | |
| | `hover` / `pressed` | interaction states of a control already using the tone | inherits the control's | | |
| | `ink` | **any alert-toned text**, and the glyph paired with it | ≥ 4.5:1 on `background.level01`/`level02` (SC 1.4.3) | | |
| | step | role | obligation | | |
| | --- | --- | --- | | |
| | `defaultColor` | fills, dots, borders, standalone glyphs, chart series | ≥ 3:1 on `background.level01`/`level02` (WCAG SC 1.4.11) | | |
| | `hover` / `pressed` | interaction states of a control already using the tone | inherits the control's interaction-state contrast requirements | | |
| | `ink` | **any alert-toned text**, and glyphs paired with that text | ≥ 4.5:1 on `background.level01`/`level02` (SC 1.4.3) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/design_system/README.md` around lines 109 - 113, Complete the
alert-role table in the README by finishing the hover/pressed obligation with an
explicit reference to the control’s required contrast, and clarify glyph
ownership: standalone glyphs remain under defaultColor, while glyphs paired with
alert-toned text use ink. Keep the existing contrast requirements and role names
unchanged.
Source: Coding guidelines
Comment on lines
+295
to
+304
| // Ink, because an urgent chip leaves `labelColor` null and the tinted pill | ||
| // then paints its label in this colour — the fill strengths only clear the | ||
| // non-text floor. | ||
| final accent = switch (dueDate.urgency) { | ||
| DesktopTaskHeaderDueUrgency.overdue => TaskShowcasePalette.error(context), | ||
| DesktopTaskHeaderDueUrgency.today => TaskShowcasePalette.warning(context), | ||
| DesktopTaskHeaderDueUrgency.overdue => TaskShowcasePalette.errorInk( | ||
| context, | ||
| ), | ||
| DesktopTaskHeaderDueUrgency.today => TaskShowcasePalette.warningInk( | ||
| context, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate the urgent pill’s fill and text token roles.
The current implementation makes the text AA-safe by passing ink through DsPill.color, but that property also drives the tinted background. Use defaultColor for fills/icons and ink for the label.
lib/features/tasks/ui/header/desktop_task_header_meta.dart#L295-L304: pass separate fill and label colors toDsPill.test/features/tasks/ui/header/desktop_task_header_test.dart#L1258-L1265: assertdefaultColorfor the pill background andinkforlabelColor.
📍 Affects 2 files
lib/features/tasks/ui/header/desktop_task_header_meta.dart#L295-L304(this comment)test/features/tasks/ui/header/desktop_task_header_test.dart#L1258-L1265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/features/tasks/ui/header/desktop_task_header_meta.dart` around lines 295
- 304, The urgent pill currently uses one color for both its tinted fill and
label; update the DsPill construction in
lib/features/tasks/ui/header/desktop_task_header_meta.dart lines 295-304 to use
defaultColor for the fill/icons and ink for labelColor. Update the corresponding
assertions in test/features/tasks/ui/header/desktop_task_header_test.dart lines
1258-1265 to verify defaultColor as the pill background and ink as labelColor.
matthiasn
added a commit
that referenced
this pull request
Jul 25, 2026
Fifth review round. Both findings land on the previous round's privacy fix. Privacy never reached the database. `createDbEntity` rebuilds the entity with `private: linked?.meta.private` — with no `linkedId` that is an explicit null into copyWith, which *erases* the privacy the caller set rather than leaving it alone. So the previous round's fix threaded `private` all the way down to createMetadata and then had it wiped one call later. Now falls back to the entity's own value; a linked parent still wins when it has an opinion. That test passed anyway, which is the more useful lesson: it asserted on the in-memory Task returned to the caller, not on the stored row. Re-reading from JournalDb makes it fail against the old code, which is now verified. The erasure was never specific to this feature. Any create without a `linkedId` lost its privacy, so `createMeasurementEntryImpl` (which takes `required bool private`) and `createHabitCompletionEntryImpl` (which passes `habitDefinition?.private`) were both affected: a private measurement, or a completion of a private habit, created unlinked, was stored public. Hence its own CHANGELOG entry rather than being folded into the task-links work. The glados property test failed on the fix because it encoded the bug as an invariant — `expectedPrivate => hasLinkedEntity ? linkedPrivate : null`. The expectation is corrected to "a linked parent's privacy wins; without one the entity keeps what it was built with", with the reasoning inline so it does not get re-asserted. Required-link failures were masked. Hoisting the applied verdict past `createLink` meant a link write that threw still reported success, so a caller would navigate to a task whose link does not exist. `applied` is now set only after the link lands: ancillary badge and geolocation failures still preserve success, the requested link does not. CHANGELOG entries moved to 0.9.1069, the version current since main's bump; the entries from #3579 and #3583 stay under 0.9.1068 where they shipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the light-theme alert palette failing WCAG contrast, at the token source.
The measurement
The bug was filed against
alert.warning. Recomputing every step against thesurfaces it actually lands on showed three of the four families miss the floor:
defaultbackground.level01background.level02warning#FA8C05info#1CA3E3success#59A66Berror#CC3633All three sit under the 3:1 floor for graphical objects and UI-component state
(SC 1.4.11). Dark theme measures clean for those three, but
error.defaultis 4.25:1 on
level02— under AA for text, which the original report did notcatch.
Consequence beyond one feature:
warningis a semantic colour. Where afeature uses it as its only chromatic mark, that meaning stops carrying in
light theme while the same surface measures clean in dark.
Nothing in the build checked the palette, which is how this shipped and stayed.
The fix
Two changes in
assets/design_system/tokens.json, both drawn from valuesalready in the ramp — no invented hexes:
1. A new
inkstep per family. The least-extreme step of the same rampthat clears 4.5:1, resolved per brightness:
inkinkerror#A32B29(hover)#E08684(hover)success#366340(pressed)#7AB889(default)warning#965402(pressed)#FBA336(default)info#116288(pressed)#4AB6E8(default)"Least-extreme that passes" rather than a uniform
pressed: it keeps errortext a red instead of a maroon, and leaves dark warning/success/info text
pixel-identical to today.
2. Light
defaultmoved onto the family'shovervalue for warning, infoand success, so fills, dots, borders and glyphs clear 3:1. This collapses
defaultandhoverin light for those three — nothing binds either today,and a future Figma pass should re-derive a distinct light
hover. Noted in thedesign-system README.
Consumers
Split by role, not swept wholesale:
defaultColor— they owe 3:1,which the darkened light ramp now delivers, and nothing about them changes in
dark theme
ink— priority labels,status chips, connection results, ledger figures, destructive menu items,
outlined badge labels
DesignSystemBadgeis thereference: fill and border on the accent, label and glyph on the ink.
Four widgets already hand-rolled exactly this with
Theme.of(context).brightnessbranches —insights_delta_chip,impact_model_table,onboarding_api_key_panel(×2). Each collapses to asingle token read, and their comments explaining the manual pick are replaced
by a pointer at the token.
The guard
design_tokens_test.dartnow pins the contract: 4 families × 2 brightnesses ×2 surfaces, asserting
ink≥ 4.5:1,default≥ 3:1, and thatinkis neverweaker than
default— so moving text onto it can never lose contrast.background.level03is excluded deliberately: it is a mid-grey chip and panelfill, and no step of the dark error ramp can reach AA against it (pure white
tops out at 7.7:1 there).
Verified the guard bites rather than passing vacuously: reverting light
warning.defaultto#FA8C05fails exactly the two cases it should, and thedark
error.inkmiss was caught by the test during development, not byinspection.
Notes
section header — needed no widget change. Both already put amber on a
border and a ⊘ glyph only and keep their labels at
text.highEmphasis/text.mediumEmphasis; those are non-text and now clear their floor. Thetasks README claimed an amber label; corrected to match the code.
text.onInteractiveAlertonthe accent fill, which is 3.64:1 on the new amber — still under AA for text.
Closing that needs the ramp darkened much further than the non-text floor
requires, so it belongs in its own change.
fvm flutter analyzeclean. ~850 tests across the affected surfaces green.Summary by CodeRabbit
Style
Documentation