Skip to content

fix: stop the task-agent footer paying twice for vertical space#3604

Merged
matthiasn merged 3 commits into
mainfrom
fix/footer_vertical_rhythm
Jul 26, 2026
Merged

fix: stop the task-agent footer paying twice for vertical space#3604
matthiasn merged 3 commits into
mainfrom
fix/footer_vertical_rhythm

Conversation

@matthiasn

@matthiasn matthiasn commented Jul 26, 2026

Copy link
Copy Markdown
Owner

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) and step4 (12):

gap rendered declared
trigger row → switch row 33.5 16
switch row → model row 38.0 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. DesignSystemToggle owns its own InkWell wrapping 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

  • The whole settings row is the gesture. Tapping the "Automatic updates"
    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
    excludeFromSemantics so it does not publish a second, unlabelled button
    node beside the switch's own.
  • That row drops to step8, the minimum every other row in the footer
    already uses.
  • The stacked column and the gap before the identity region declare no
    vertical space at all.
    The row boxes supply the rhythm — space the boxes,
    not the text inside them.

Result

main this PR
trigger → switch (ink-to-ink) 33.5 13.5
switch → model row 38.0 22.0
card height, phone 466 430
card height, narrow scheduled 550 510
card height, German @ 1.3× 716 647
card height, desktop 386 366

The split-attribution state nets to zero: the attribution row's own step8 box
costs 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

main this PR
main now

Narrow, with a pending update

main this PR
main now

German at 320 px, 1.3× text scale

main this PR
main now

Version

Build number only: 0.9.1070+4258+4259. The version string is unchanged.

The 0.9.1070+4258 tag 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.
  • Screenshot matrix regenerated; every number in the tables above is measured
    from those PNGs rather than read off the tokens.

Summary by CodeRabbit

  • Bug Fixes
    • Reduced spacing in AI summary card controls, especially on smaller screens and with larger text.
    • Made the entire “Automatic updates” row tappable, including its label.
    • Improved accessibility behavior by preventing duplicate announcements and focus targets.
  • Documentation
    • Updated release notes and UI specifications to reflect the revised layout and interaction behavior.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

AI summary card controls

Layer / File(s) Summary
Compact automation layout
lib/features/agents/ui/task_agent_automation_row.dart, lib/features/agents/ui/task_agent_controls_footer.dart, knowledge/features/agents/ui-surfaces.md, CHANGELOG.md, flatpak/com.matthiasn.lotti.metainfo.xml, pubspec.yaml
Stacked controls and the footer no longer declare extra vertical gaps; sizing documentation, release notes, and the build version reflect the updated layout.
Full-row toggle and semantics
lib/features/agents/ui/task_agent_automation_row.dart, lib/features/agents/ui/task_agent_identity_region.dart, test/features/agents/ui/task_agent_automation_row_test.dart, knowledge/features/agents/ui-surfaces.md
The automatic-updates row toggles from label taps while excluding the wrapper from semantics and focus; tests validate sizing and focus traversal, and identity rows announce combined labels.

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
Loading

Possibly related PRs

Suggested reviewers: cyberk1ng

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: reducing redundant vertical spacing in the task-agent footer.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/footer_vertical_rhythm

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.15%. Comparing base (351ac3b) to head (f9b2f2f).

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           
Flag Coverage Δ
glados 14.08% <0.00%> (-0.01%) ⬇️
standard 98.90% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread lib/features/agents/ui/task_agent_automation_row.dart
Comment thread knowledge/features/agents/ui-surfaces.md
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.
@matthiasn
matthiasn force-pushed the fix/footer_vertical_rhythm branch from f3ceabf to 65006fd Compare July 26, 2026 13:57

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread knowledge/features/design_system/component-contracts.md Outdated
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
lib/features/agents/ui/task_agent_automation_row.dart (1)

655-687: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate 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

📥 Commits

Reviewing files that changed from the base of the PR and between 351ac3b and f9b2f2f.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • flatpak/com.matthiasn.lotti.metainfo.xml
  • knowledge/features/agents/ui-surfaces.md
  • lib/features/agents/ui/task_agent_automation_row.dart
  • lib/features/agents/ui/task_agent_controls_footer.dart
  • lib/features/agents/ui/task_agent_identity_region.dart
  • pubspec.yaml
  • test/features/agents/ui/task_agent_automation_row_test.dart

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +115 to +117
**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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fixed in the follow-up, since #3604 merged first: #3606

// 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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ''.

Fixed in the follow-up, since #3604 merged first: #3606

@matthiasn
matthiasn merged commit d54d4b8 into main Jul 26, 2026
33 of 34 checks passed
@matthiasn
matthiasn deleted the fix/footer_vertical_rhythm branch July 26, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant