fix: regroup the task-agent footer by question and shed route segments#3597
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (35)
📝 WalkthroughWalkthroughThe task-agent summary card now uses separated automation control clusters, responsive stacking, tiered model identity rendering, updated “Skip once” and setup tooltip localizations, expanded UI tests, and version 0.9.1070 release metadata. ChangesTask-agent summary card UI
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3597 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 1784 1784
Lines 131005 131044 +39
=======================================
+ Hits 129899 129938 +39
Misses 1106 1106
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:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c880634825
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Design-panel round 2 scored the merged footer 6.17/10 (experts) and 6.0/10 (personas), up from 4.08/3.8. This applies its consensus must-fixes. Grouping. "When does it update" was split across the full card width with the manual trigger wedged between the halves, which made "Automatic updates" read as a caption for the button. The row now presents two groups: state and its remedy lead (freshness word + trigger), and the switch keeps its own readout (countdown + Skip) on the trailing rail, separated by a real step6 gap rather than leftover `spaceBetween` slack. Stacked, the schedule sits tight against the switch it belongs to, with the larger gap between the two questions — evenly spaced rows let the countdown read as the manual run's ETA. Emphasis. The trigger was `outlined`, which is literally the same button as `Confirm all` (proposals_section_part.dart:187) — the card's hero action. It drops to the design system's text tier, so exactly one outlined action remains and it belongs to the proposals. Truncation. Routes are structured, so they now shed whole segments instead of characters: "Qwen 3.5 Plus · Alibaba · via Melious.ai" → "Qwen 3.5 Plus · Melious.ai" → "Qwen 3.5 Plus". An ellipsis ate the serving provider — the one fact the row exists to disclose — and left the connective "via" behind. The fixed "This report" label never gives ground; only its route does. Alignment. `DesignSystemButton` pays its own step3 content inset, so a button box on the leading edge put its glyph 8px inside it — invisible until the row stacked, then a broken column, and a direct contradiction of the footer's own docstring. The row negates that inset, and a test now asserts the column at 320px in German rather than a comment claiming it. The switch also moves to the trailing edge of its 48px slot: centring a 40px track left it short of the rail the TTS button and the proposal cards terminate on. Also: the countdown joins the freshness word in the state register so it stops reading as the same class of information as the static model route; the attribution line's vertical padding is top-only, so the card's bottom margin no longer depends on whether provenance happens to differ; the model row's tooltip names its action instead of repeating a visible label; and the schedule says nothing at all while a run is in flight, where "Updates when this task changes" beside a spinner described a state the card was not in. Copy: "Skip" becomes "Skip once" — the persona panel could not tell whether it skipped one run or disabled automation. New `taskAgentChangeSetupTooltip`. Both hand-authored across all 11 locales.
Design-panel rounds 3 and 4 (6.5/6.1 then 6.58/6.4) kept returning to the same two complaints, so this addresses them at the source rather than in the widget. `DesignSystemButton` gains a `dense` size whose label is `typography.styles.others.caption`. The footer trigger was `small`, which is 14/600 — the same type as `Confirm all` one hairline above it — so a settings action was tied with the card's hero action on weight even after it stopped being `outlined`. Additive: no existing call site changes. Accent now means exactly one thing in the band, "this starts work". It was on the trigger, on `Skip once`, and on the fresh-state check glyph at once, so two opposite actions and a passive status shared one colour on one line. Only the trigger keeps it; Skip carries its affordance through the underline. The attribution line's ink was also backwards: the fixed "This report" label was brighter than the route it labels. Swapped, so emphasis points at the payload. Docs: the agents README described three question-groups, the former status/trigger/switch stacking, `Skip`, and identity rows that "truncate with the full route in the tooltip" — all superseded by the two-group layout, `Skip once`, segment shedding and the action-only setup tooltip. Rewritten, including the fit-decision diagram, which now shows the two extra stacking tiers. The design-system README documents the new size.
c880634 to
2d73e60
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d73e609fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DesignSystemButtonSize.dense => _ButtonSizeSpec( | ||
| labelStyle: tokens.typography.styles.others.caption, | ||
| iconSize: tokens.typography.lineHeight.caption, | ||
| horizontalPadding: tokens.spacing.step2, | ||
| verticalPadding: tokens.spacing.step2, |
There was a problem hiding this comment.
Preserve a full-size touch target for dense buttons
When dense is used by the new taskAgentWakeButton, the 16 px caption line plus two 4 px vertical insets makes the button's InkWell only 24 px high. DesignSystemButton adds no minimum hit-area constraint, so touch users must land on a target half the 48 px size used by the adjacent toggle; keep the dense visual treatment while providing a full-size interaction target.
Useful? React with 👍 / 👎.
| size: DesignSystemButtonSize.dense, | ||
| // Tertiary, not outlined: this is a settings-zone action and must read | ||
| // one tier below "Confirm all", the only thing on the card that changes | ||
| // the user's own task. It stays labelled — an icon-only glyph beside an | ||
| // automation switch is exactly the ambiguity the worded Skip refuses. | ||
| variant: DesignSystemButtonVariant.tertiary, |
There was a problem hiding this comment.
Keep the dense tertiary label at AA contrast
In the light theme, combining the new 12 px caption size with the tertiary foreground paints Update now in interactive.enabled (#1F7963) over the composited footer background at only about 4.1:1 contrast, below the 4.5:1 requirement for normal text. This makes the footer's always-present manual action difficult to read for low-vision users; use a foreground or variant that retains the quieter hierarchy without dropping below the normal-text threshold.
Useful? React with 👍 / 👎.
| Text( | ||
| label, | ||
| maxLines: 1, | ||
| style: caption.copyWith(color: ai.faintMeta), |
There was a problem hiding this comment.
Use a readable token for the report attribution label
In the light theme, ai.faintMeta composites to roughly 2.6:1 against the footer wash while this is 12 px caption text. Because “This report” distinguishes the historical report route from the current setup, it is meaningful text rather than decorative metadata, and low-vision users can no longer reliably tell which model identity the second row describes; keep this label on a token that meets normal-text contrast.
Useful? React with 👍 / 👎.
Follow-up to #3591, driven by the design panel's round-2 verdict.
Scores
Both panels are deliberately grumpy (10 = ship-grade, 8 = only polish left).
Every round re-rates freshly generated screenshots; no score is carried over.
What changed, and why
Grouping. "When does it update" was split across the full card width with
the manual trigger wedged between the halves, which made "Automatic updates"
read as a caption for the button. The row now presents two groups: state and
its remedy lead (freshness word + trigger), and the switch keeps its own
readout (countdown + Skip) on the trailing rail, separated by a real
step6gap rather than leftover
spaceBetweenslack. Stacked, the schedule sitstight against the switch it belongs to with the larger gap between the two
questions — evenly spaced rows let the countdown read as the manual run's ETA.
Emphasis. The trigger was
outlined, which is literally the same buttonas
Confirm all(proposals_section_part.dart:187) — the card's hero action.It drops to the design system's text tier, so exactly one outlined action
remains and it belongs to the proposals.
Truncation. Routes are structured, so they shed whole segments instead of
characters:
Qwen 3.5 Plus · Alibaba · via Melious.ai→Qwen 3.5 Plus · Melious.ai→Qwen 3.5 Plus. An ellipsis ate the serving provider — the onefact the row exists to disclose — and left the connective "via" behind
(
This rep… GLM 5.2 · Z.ai · via OpenRou…). The fixed "This report" labelnever gives ground; only its route does.
Alignment.
DesignSystemButtonpays its ownstep3content inset, so abutton box on the leading edge put its glyph 8px inside it — invisible until
the row stacked, then a broken column, and a direct contradiction of the
footer's own docstring. The row negates that inset, and a test asserts the
column at 320px in German rather than a comment claiming it. The switch also
moves to the trailing edge of its 48px slot: centring a 40px track left it
short of the rail the TTS button and proposal cards terminate on.
Also: the countdown joins the freshness word in the state register, so it
stops reading as the same class of information as the static model route; the
attribution line's vertical padding is top-only, so the card's bottom margin
no longer depends on whether provenance happens to differ; the model row's
tooltip names its action instead of repeating a visible label; and the
schedule says nothing while a run is in flight, where "Updates when this task
changes" beside a spinner described a state the card was not in.
Copy. "Skip" → "Skip once" — the persona panel could not tell whether it
skipped one run or disabled automation. Plus a new
taskAgentChangeSetupTooltip. Both hand-authored across all 11 locales.One recommendation I did not follow
The panel wants
aiCard.footerWashchanged at the token source (it argues ablack multiply cannot separate a band on a near-black card). That is a Figma
-exported token consumed by generated code, so it is not mine to edit —
flagging it rather than working around it in the widget.
Testing
Analyzer clean (zero warnings, zero infos), formatter clean, 2212 tests
green across
test/features/agents/ui/andtest/features/tasks/ui/. Newcoverage for the segment ladder, the stacked leading column, and the
running-state schedule suppression.
Version bumped to 0.9.1070+4256 at the maintainer's request.
Summary by CodeRabbit
New Features
Bug Fixes