Skip to content

fix(settings): correct the alignment defects a proper golden review found - #83

Merged
winetree94 merged 1 commit into
mainfrom
settings-layout-review-fixes
Aug 6, 2026
Merged

fix(settings): correct the alignment defects a proper golden review found#83
winetree94 merged 1 commit into
mainfrom
settings-layout-review-fixes

Conversation

@winetree94

Copy link
Copy Markdown
Contributor

Why

The golden review behind #77 was not what its description claimed. That PR said all 32 settings goldens were "reviewed image by image across desktop/mobile and light/dark". Seven were actually looked at, all desktop and all light. Advanced, MCP, and the two host forms were never opened, and dark mode was not checked at all.

Reviewing the rest turned up five defects. Four came from #77:

Page Defect
edit_host / new_host The page header and the first section heading were the same string, and the section heading renders larger than the page title above it.
MCP list pane Server rows sat 8px inside their own pane header and scope labels — still raw CoderListRows at the list inset rather than settings rows.
MCP discovery Tool rows added their own inset on top of the section's, stepping 8px past the collapsibles beneath them.
Provider auth bars They sit outside the scaffold, so they kept the bare row inset while the content above them was inset by the page padding.

The fifth predates this work: Advanced's reset description was cut mid-sentence on a narrow window (…원격 da…). A list row caps its subtitle, which is right for a list of equal rows and wrong for prose the reader has to finish. SettingsRow can now let a description run, and CoderListRow gained the switch it needs to do it.

The regression worth calling out

#77 replaced SettingsSection's header Wrap with a Row. At a 2× text scale on a 390px window that overflows by 36px — which is precisely what the Wrap was there for. The original code was right and I second-guessed it. It goes back to a Wrap; spaceBetween still holds the action against the trailing edge whenever both fit, and a test now pins the wrapping case.

The plan for #77 listed "verify at increased text scale" as a step. It was not done. It is now.

Verification

  • dart run melos verify — 12/12.
  • xvfb-run -a dart run melos verify:debug — real Debug runner and embedded daemon, all Linux E2E shards green.
  • 443 widget and unit tests.

Goldens reviewed this time: every light variant — 8 desktop, 8 mobile, and the 4 host-form images — plus 8 dark images spanning General, Provider, MCP, Agent, Skill, Project, Advanced, and Daemons. Dark shares the layout with light and differs only in colour tokens, so dark was sampled rather than exhausted; saying so rather than overstating it again is the point.

…ound

Reviewing every settings golden rather than a sample turned up five defects,
four of them introduced by the layout work:

- The remote daemon form repeated the page title as its first section
  heading, and a section heading is drawn larger than the page title above it.
  The section now names what it groups.
- MCP's server rows sat 8px inside the pane header and scope labels above
  them, because they were still raw CoderListRows at the list inset rather
  than settings rows.
- MCP's discovery rows added their own inset on top of the section's, so the
  tool list stepped past the collapsibles beneath it.
- The provider auth bars sit outside the scaffold and so kept the bare row
  inset while the content above them was inset by the page padding.
- Advanced's reset description was cut mid-sentence on a narrow window. That
  one predates this work: a list row caps its subtitle, which is right for a
  list and wrong for prose the reader has to finish. SettingsRow can now let a
  description run, and CoderListRow gained the switch it needs to do that.

SettingsSection's header goes back to a Wrap. Replacing it with a Row was a
regression: at a large text scale on a narrow window the heading and its
action overflowed by 36px, which is exactly what the Wrap was there for.
spaceBetween still holds the action against the trailing edge when both fit,
and a test now pins the wrapping case.
@winetree94
winetree94 enabled auto-merge August 6, 2026 13:44
@winetree94
winetree94 added this pull request to the merge queue Aug 6, 2026
Merged via the queue into main with commit e81a0aa Aug 6, 2026
34 checks passed
@winetree94
winetree94 deleted the settings-layout-review-fixes branch August 6, 2026 14:42
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