fix(desktop): centre the request-header remove button on its field - #3346
fix(desktop): centre the request-header remove button on its field#3346shaokeyibb wants to merge 1 commit into
Conversation
The trash button at the end of each custom request header row sat 4px below the centre of the inputs beside it, and was the only thing making the row 40px tall. The row is `align-items: start` on purpose, so the cell centres its 28px sm IconButton inside its own declared height. That height was a bare 2.5rem. 40px is not what the inputs are -- Astryx renders TextInput at --size-element-md = 32px -- and 40 is off maka's control ruler anyway (--h-control-2xl, the toolbar tier). Pin the cell to --h-control-lg, which IS --size-element-md, so the two cannot drift. Covered by a new e2e contract spec asserting the button's centre against the field's. scripts/audit-alignment.mjs cannot cover it: it clusters controls by parentElement, and this row wraps every cell in its own div, so the inputs and the button never share a cluster. Fixes apache#3345 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes a vertical misalignment of the custom request-header row’s remove (trash) button by aligning it to the actual input control height, and adds an e2e “contract” test to prevent regressions.
Changes:
- Adjust
.requestHeaderRemoveto use the shared control-height token so the icon centers on the field. - Add a Playwright e2e spec that measures button-vs-field centerline drift.
- Introduce a dedicated e2e window fixture to reach the request headers editor surface reliably.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/desktop/src/renderer/styles/settings/rows.css | Aligns the remove-button cell height to the same control height as TextInput. |
| apps/desktop/e2e/request-header-row-contract.spec.ts | Adds an e2e geometry assertion to pin the alignment behavior. |
| apps/desktop/e2e/fixtures.ts | Adds a new window fixture for navigating to the relevant settings surface. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* The remove control centres on the FIELD, not on the cell. The row is | ||
| `align-items: start` because the value input can grow an inline error under | ||
| itself and the trash must not ride down with it — so this cell states the | ||
| height it centres against, the same move `.settingsCapabilityGroup`'s | ||
| chevron makes against its title line. That height is the input's own box: | ||
| `--h-control-lg` IS Astryx's `--size-element-md`, which is what TextInput | ||
| renders at (measured 32px). The bare 2.5rem here was 40px — off the control | ||
| ruler entirely, per the `--h-control-*` scale's own note that 40 is the | ||
| toolbar tier — and centring the 28px `sm` IconButton in that 40px box | ||
| parked the icon 4px below the field's centre line. */ |
| // The input's own bordered wrapper is the box the eye reads as "the | ||
| // field"; the bare <input> inside it is shorter than the control. | ||
| const field = row.querySelector<HTMLInputElement>('.requestHeaderName input')!.parentElement!; |
| // Sub-pixel, not zero: a fractional device pixel ratio can land a centre on | ||
| // .5. Four pixels — the regression this pins — is not that. | ||
| expect(Math.abs(geometry.drift)).toBeLessThanOrEqual(0.5); | ||
| // The mechanism, named separately so a failure says which half broke: the | ||
| // cell must not declare a taller box than the field it centres against. | ||
| expect(geometry.cellHeight).toBeLessThanOrEqual(geometry.fieldHeight); |
| requestHeaderRowWindow: async ({}, use) => { | ||
| await withE2eWindow({ | ||
| seed: false, | ||
| readinessSelector: '.settingsSurface', | ||
| e2eFixtureScenario: 'settings-models', | ||
| locale: 'zh', | ||
| showWindow: true, | ||
| }, use); | ||
| }, |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Automated Codex review and focused verification on exact head 6ca2f6c53f9b4b7b0464ddc49e3c3a7c7f7b2b03.
Current main still declares .requestHeaderRemove { min-height: 2.5rem; } (40 px) beside 32 px inputs, so the reported 4 px centreline drift is real. Replacing that literal with the existing 32 px control token is the smallest coherent fix and preserves the row's intentional align-items: start behavior.
I ran the previously-unrun supported-host check on macOS Apple Silicon:
- Desktop workspace dependencies and Desktop build: passed
e2e/request-header-row-contract.spec.ts: 1/1 passed (4.6 s)
That removes the PR's stated uncertainty about the geometry test. The existing comments about the long CSS rationale, parentElement selector, and 0.5 px tolerance are maintainability/flakiness improvements, not evidence that the production fix is wrong; they can be simplified without changing the behavior.
Required conclusions:
- Optimal for the actual problem: yes.
- Production code to delete: none identified beyond optionally shortening the explanatory comment.
- Tests to delete/replace: none identified; the focused E2E now has real red/green value.
- Deeper refactor: no.
- Ready to merge: no; the required hosted
testcheck has not reported and independent human review is missing. - Residual risk/gaps: hosted CI and human visual judgment remain; the supplied before/after images satisfy the repository's evidence requirement.
This is a user-visible UI change and requires independent human review under CONTRIBUTING.md. No security, licensing, release, governance, or public-contract effect was identified.
Summary
The trash button at the end of each custom-request-header row sat 4px below the
centre of the inputs beside it.
The row is
align-items: starton purpose — the value input can grow an inlineerror underneath it and the trash must not ride down with it — so the cell
centres its 28px
smIconButton inside its own declared height. That height wasa bare
min-height: 2.5rem. 40px is not what the inputs are: Astryx rendersTextInputat--size-element-md= 32px, and 40 is off maka's control ruleranyway (
--h-control-2xl, the toolbar tier). Pinning the cell to--h-control-lg— which is--size-element-md, so the two cannot drift —lands the icon on the field's centre line and keeps the row 32px like its fields.
Fixes #3345
Verification
Measured the rendered boxes in Storybook, on both surfaces that render
RequestHeadersEditor:Product/Settings/Providers → AddProvider→ advanced request settings: 0Product/Settings/Providers → ConnectionDetailPage→ 自定义请求头 → 编辑: 0row to 48px and the trash still measures 0 against the field line.
npm run lint,npm run format:check, andnpm --workspace @maka/desktop run typecheck(all four projects) pass.e2e/, so the repo'stypecheckdoes notcover the new spec. I checked it separately with a throwaway project over
e2e/— the spec is clean.npm run buildand the Playwright e2e suite — Windows is not asupported build target here. Repo-wide
npm run typecheckfails inpackages/clion an unbuilt@maka/evalin my tree; that is a build-stateissue, unrelated to a CSS file.
Before / after
Storybook renders of
RequestHeadersEditor, before and after.Review focus
The new spec is unrun — that is why this is a draft.
e2e/request-header-row-contract.spec.tsasserts the remove button's centre against the field's, on a new
requestHeaderRowWindowfixture over the existingsettings-modelsscenario.It typechecks and lints, but Windows is not a supported build target, so I have
never seen it go red-then-green. Please treat the spec as a proposal and run it
once on a supported host before trusting it; I would rather say that than check
a box I did not earn. Mark this ready for review, or tell me to drop the spec
and I will reduce the PR to the one-line CSS change.
scripts/audit-alignment.mjsshould have caught this and structurallycannot. It already checks centreline drift at a 1.5px threshold, but it
clusters controls by
parentElement, and this row wraps every cell in its owndiv — so the inputs and the trash button never share a cluster to be compared
in. Any grid row built that way is invisible to it. Its fixtures also only see
a surface as it first renders, and this editor is three clicks deep. Widening
the auditor is a bigger change than this PR should carry, so I left it alone
and noted it in #3345 instead; happy to open a follow-up.
AI use
Select exactly one:
Tool(s) and scope: Claude Code (Claude Opus 5) — reproduced and measured the
defect, traced the root cause, audited the rest of the renderer for the same
pattern, wrote the CSS change and its comment, and drafted the e2e spec. I reviewed and verified the
result and I am the contributor of record.
Checklist
run it locally; see Review focus
Does this PR entail a change in behavior?