fix: stop the task-agent footer paying twice for vertical space#3604
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe AI summary card controls now use tighter stacked spacing, a step8-based automatic-updates row, full-row tapping with preserved switch focus semantics, explicit identity announcements, updated tests, and refreshed release metadata. ChangesAI summary card controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AutomationRow
participant Switch
User->>AutomationRow: Tap "Automatic updates" row
AutomationRow->>Switch: Call onChanged(!value)
Switch-->>AutomationRow: Render updated value
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 #3604 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 1785 1785
Lines 131174 131175 +1
=======================================
+ Hits 130062 130063 +1
Misses 1112 1112
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: f3ceabfc3c
ℹ️ 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".
The settings band was a third of the card on a phone, and the space around the automatic-updates row was the worst of it: 33.5 logical px above the switch and 38 below, measured ink-to-ink, against declared gaps of 16 and 12. Two causes, both invisible in the source. The switch sat in a `ConstrainedBox(minWidth: step9, minHeight: step9)` whose comment claimed it "keeps a full-size interaction target for anyone who cannot land on 24px". It did not. `DesignSystemToggle` owns its own `InkWell` wrapping the 40x24 track, and that outer box is not a gesture detector — so it reserved 48px of column height while the only thing a finger could actually hit stayed the 24px track. It bought airiness and paid for nothing. And every row in the band is a touch-target box taller than its own ink, each contributing ~10px of air above and below the visible text. The declared gaps sat on top of that, so the band paid for the same space twice. So: the whole settings row is the gesture now — tapping the label toggles the setting, which is a target the width of the band rather than a 40x24 track — and it drops to the `step8` minimum every other row in the footer already uses. The row publishes `excludeFromSemantics` so it does not add a second unlabelled button node beside the switch's own. The stacked column and the gap before the identity region declare no vertical space at all; the row boxes supply the rhythm. Measured, ink-to-ink: trigger row to switch row 33.5 -> 13.5, switch row to model row 38.0 -> 22.0. Card height: 466 -> 430 on a phone, 716 -> 647 in German at 320px and 1.3x, 386 -> 366 on desktop. The split-attribution state nets to zero, because the attribution row's own step8 box costs back what the gap removal saves — the trade that keeps the card's bottom margin from moving. Also two more review findings on #3602: The report row's tooltip entered semantics. Excluding it alone would have made that row worse — its visible route sheds whole segments, so the children and the tooltip carry different strings, and excluding only the tooltip leaves the *shortened* route as the sole announcement. It now publishes one node with the untruncated attribution, the shape `DesignSystemInlineAction` uses a row above. And both concepts carried a `generated.at` of 14:00:00Z against a 13:04:50Z commit, claiming a snapshot from the future.
Build only — 0.9.1070+4258 -> +4259; the version string is unchanged. The 4258 tag contains the footer redesign but not #3602, so the over-spaced settings band actually shipped. That makes this a fix to a released state rather than a defect introduced and resolved inside an unreleased version, so it carries a CHANGELOG entry and the matching metainfo line.
f3ceabf to
65006fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65006fd057
ℹ️ 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".
Review findings on #3604. Making the whole settings row tappable gave it a focus stop of its own beside the switch's, so Tab landed twice on one setting and both stops toggled it. `excludeFromSemantics` suppresses the semantic node and does nothing whatever to traversal. The wrapper is pointer-only now via `canRequestFocus: false`, and a test counts focusable InkWells in the row — it reports 2 without the flag. The concept also stated the old layout contract in two places the new prose contradicted: the flow diagram's stack node still promised "a larger gap above" the schedule, and the automation section still claimed "the toggle keeps a full `spacing.step9` interaction slot around its 40x24 track" — the very claim this branch exists to delete, since that slot was inert. Both now describe the full-row target on the shared `step8` minimum. The one remaining step9 mention narrates the earlier revision as history, not as a contract. And `component-contracts.md` is dropped from this branch entirely. Its only change here was a `generated.at` rewound from 14:00:00Z to 13:56:33Z — bulk collateral from #3602 that rode along in the cherry-pick — which would have claimed an unchanged document predates its own revision. `ui-surfaces.md` keeps its bump because its prose does change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/agents/ui/task_agent_automation_row.dart (1)
655-687: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate comment block left in by mistake.
Lines 666-670 and 672-676 are byte-for-byte identical ("...and it must not add a second focus stop either..."). Looks like a copy-paste artifact from drafting; the second copy should be removed.
✏️ Proposed fix
excludeFromSemantics: true, // ...and it must not add a second *focus* stop either. Excluding // semantics does nothing to focus traversal, so without this Tab lands // twice on one setting — once on this wrapper, once on the switch — // and both stops toggle it. The switch keeps the keyboard; the row is // pointer-only. canRequestFocus: false, - // ...and it must not add a second *focus* stop either. Excluding - // semantics does nothing to focus traversal, so without this Tab lands - // twice on one setting — once on this wrapper, once on the switch — - // and both stops toggle it. The switch keeps the keyboard; the row is - // pointer-only. // One row box, on the same `step8` minimum as every other row in this // band. It is 8px shorter than the slot it replaces and, unlike that // slot, all of it is tappable. child: ConstrainedBox(🤖 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/task_agent_automation_row.dart` around lines 655 - 687, Remove the duplicated focus-stop comment immediately before the ConstrainedBox in the Material/InkWell block, keeping one copy of the explanation and all surrounding behavior unchanged.
🤖 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.
Nitpick comments:
In `@lib/features/agents/ui/task_agent_automation_row.dart`:
- Around line 655-687: Remove the duplicated focus-stop comment immediately
before the ConstrainedBox in the Material/InkWell block, keeping one copy of the
explanation and all surrounding behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 997f062b-6683-4adf-82c6-2f8e9c204799
📒 Files selected for processing (8)
CHANGELOG.mdflatpak/com.matthiasn.lotti.metainfo.xmlknowledge/features/agents/ui-surfaces.mdlib/features/agents/ui/task_agent_automation_row.dartlib/features/agents/ui/task_agent_controls_footer.dartlib/features/agents/ui/task_agent_identity_region.dartpubspec.yamltest/features/agents/ui/task_agent_automation_row_test.dart
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9b2f2f048
ℹ️ 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".
| **Vertically the band declares no gaps at all, and that is the contract.** Every | ||
| row here is a touch-target box taller than the ink inside it — all of them on | ||
| one `spacing.step8` minimum — so each already contributes ~10 logical px of air |
There was a problem hiding this comment.
Stop claiming every stacked row is step8
Fresh evidence in the reviewed tree is that the corrected concept now introduces a different false invariant: when automation is enabled without a pending countdown, _ScheduleCluster returns a plain Row containing _ScheduleLabel, whose SizedBox constrains only width and has no step8 minimum height. Therefore the schedule row does not self-space as this paragraph and the updated diagram claim; either give that row the documented minimum or describe the zero-gap layout without asserting that every row is a step8 box.
AGENTS.md reference: AGENTS.md:L95-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — I introduced a false invariant while fixing another one. Verified:
_ScheduleLabel returns SizedBox(width: spec.reservedWidth, …), width only,
no minimum height, so the schedule line is not a step8 box and does not
self-space.
Rather than inflate that row to match the prose — which would add height back
to the state this whole branch exists to shrink — the concept now says what is
actually true: the interactive rows (trigger, switch row, identity rows) are
step8 target boxes that space themselves, and the schedule line is a bare
text row that needs no target of its own and is spaced by the boxes above and
below it. The diagram node is reworded to match.
| // already publishes the accessible control — button, toggled state and | ||
| // label — so this row must not add a second, unlabelled button node | ||
| // beside it. | ||
| excludeFromSemantics: true, |
There was a problem hiding this comment.
Expose the enlarged row as the accessible toggle
Under TalkBack or VoiceOver touch exploration, excludeFromSemantics removes the outer row's tap action while the only actionable semantic node remains the inner DesignSystemToggle, whose InkWell is still just the 40×24 track; the visible label remains non-actionable semantics. Consequently, touching the newly tappable wording does not expose the advertised full-row activation target to screen-reader users. Publish one toggled tap semantic over the row and suppress the inner control's duplicate semantics/focus instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this was the important one — the enlarged target never reached
assistive tech at all.
I verified before fixing: with the row's semantics excluded and the label
inert, the semantics node over the row had an empty label and no tap
action, so touch exploration only ever found the switch's own 40×24 track. The
enlarged target was pointer-only in practice while the docs advertised a
full-row control.
Fixed with MergeSemantics — the shape SwitchListTile uses — so the row
publishes one node carrying the switch's toggled state and tap action over the
union of their rects. The visible label is wrapped in ExcludeSemantics
because the toggle already publishes that exact string and the merged node
would otherwise announce it twice. The pointer wrapper keeps
canRequestFocus: false from your earlier comment, so the switch still owns
the keyboard and there is still exactly one focus stop.
A regression test pins it: it asserts the merged node's label, toggled state,
tap action and that its rect spans the row. Against the unmerged tree that test
reports label: ''.
Follow-up to #3602. That PR merged while this commit was still in flight, so
the vertical-spacing fix never made it in.
The band was a third of the card on a phone
Measured ink-to-ink on the committed screenshot matrix, against gaps the code
declared as
step5(16) andstep4(12):Two causes, neither visible in the source.
The switch's touch target was not a touch target. It sat in a
ConstrainedBox(minWidth: step9, minHeight: step9)whose comment claimed it"keeps a full-size interaction target for anyone who cannot land on 24px". It
did not.
DesignSystemToggleowns its ownInkWellwrapping the 40×24 track,and that outer box is not a gesture detector — so it reserved 48 logical px of
column height while the only thing a finger could hit stayed the 24px track. It
bought airiness and paid for nothing.
And every row pays for its own air already. Each row in this band is a
target box taller than its ink, contributing ~10px above and below the visible
text. The declared gaps stacked on top, so the band paid for the same space
twice.
What changed
wording toggles the switch — a target the width of the band, rather than a
40×24 track that was under the 48 minimum in its short dimension. It carries
excludeFromSemanticsso it does not publish a second, unlabelled buttonnode beside the switch's own.
step8, the minimum every other row in the footeralready uses.
vertical space at all. The row boxes supply the rhythm — space the boxes,
not the text inside them.
Result
The split-attribution state nets to zero: the attribution row's own
step8boxcosts back what the gap removal saves. That is the trade that keeps the card's
bottom margin from moving depending on whether the "This report …" line exists.
Phone
Narrow, with a pending update
German at 320 px, 1.3× text scale
Version
Build number only:
0.9.1070+4258→+4259. The version string is unchanged.The
0.9.1070+4258tag contains the footer redesign (#3591, #3597) but not#3602, so the over-spaced band actually shipped. This is therefore a fix to a
released state rather than a defect introduced and resolved inside an
unreleased version, so it carries a CHANGELOG entry and the matching flatpak
metainfo line.
Verification
dart analyze lib test— no issues.make okf_check— 88 concepts, 0 errors, 0 warnings.flutter test test/features/agents/ test/features/design_system/—6166 passed, 2 skipped, 0 failed.
from those PNGs rather than read off the tokens.
Summary by CodeRabbit