Skip to content

fix(desktop): align Permission Center metadata rows - #4725

Merged
Astro-Han merged 1 commit into
apache:mainfrom
liuxiaocs7:fix/permission-center-metadata-alignment
Sep 4, 2026
Merged

fix(desktop): align Permission Center metadata rows#4725
Astro-Han merged 1 commit into
apache:mainfrom
liuxiaocs7:fix/permission-center-metadata-alignment

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Sep 4, 2026

Copy link
Copy Markdown
Member

Summary

Align each Permission Center metadata label with its value on the same horizontal text baseline and restore the primary values to the list's 14px body size. The multi-column Astryx layout omitted the baseline alignment used by its single-column variant, while this call site additionally reduced values such as “Configured” to 12px. Labels remain medium-weight and secondary-colored; values remain regular-weight and primary-colored; helper text remains 12px.

Fixes #4723

Verification

  • npm run lint
  • npm run format:check
  • npm --workspace @maka/desktop run typecheck
  • npx playwright test e2e/permission-center-metadata-layout.spec.ts --config e2e/playwright.config.ts --workers=1 (1 passed)

Before

Blue solid lines show the label text baseline; red dashed lines show the value text baseline. They are 4 px apart, and the primary value text is 12px beside a 14px label.

Permission Center metadata rows before baseline alignment

After

The same guides overlap after the fix: the labels and values now share one horizontal baseline, and both use the 14px body size.

Permission Center metadata rows after baseline and type-size alignment

Typography comparison

This same-state crop makes the size correction explicit. Only primary values change from 12px to 14px; helper text remains 12px.

Permission Center metadata typography before and after

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex diagnosed the layout and typography issues, implemented the scoped fixes and focused Electron E2E coverage, verified the rendered text baselines and font sizes, and prepared this pull request.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suite pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 4, 2026
@liuxiaocs7
liuxiaocs7 force-pushed the fix/permission-center-metadata-alignment branch 2 times, most recently from ef02bd7 to 7cadd76 Compare September 4, 2026 06:43
@liuxiaocs7 liuxiaocs7 changed the title fix(desktop): align Permission Center metadata columns fix(desktop): align Permission Center metadata rows Sep 4, 2026
@liuxiaocs7
liuxiaocs7 force-pushed the fix/permission-center-metadata-alignment branch 2 times, most recently from 4e2e807 to 8c0d05c Compare September 4, 2026 08:02
@liuxiaocs7
liuxiaocs7 force-pushed the fix/permission-center-metadata-alignment branch from 8c0d05c to 3021744 Compare September 4, 2026 08:18

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Diagnosis confirmed in the compiled component rather than from the screenshots. MetadataList's style table carries the baseline rule on the single-column variant only:

gridSingle: { k1xSpc: "xrvj5dj", kumcoG: "x1pmbctz", kOIVth: "xpec5dj", kGNEyG: "x1pha0wt" }
gridMulti:  { k1xSpc: "xrvj5dj", kumcoG: "x189bvgu", kOIVth: "x18g69wz" }

.x1pha0wt is align-items: baseline; gridMulti has no kGNEyG key at all. So this is an inconsistency in the component, not a call-site mistake.

I tried to break the fix and could not. Measuring first-line baselines with range.getClientRects()[0].bottom on a real window, across viewports 1490 / 1100 / 900 / 760 / 700 and zoom 1–3 — labels wrapping to five lines, values to ten — every pair in the layers list comes out at 0.00 px. <dd>'s VStack takes its baseline from the first flex item, which is what you want. Dark theme is identical to light; the change only touches font-size and align-items, so it never reaches a theme token. And there is no overflow at 12px→14px: dd.scrollWidth - clientWidth and dl.scrollWidth - clientWidth are both 0 across the whole range.

Ablation both ways: dropping the CSS rule alone fails the baseline assertion, reverting size="sm" alone fails the font-size assertion. Both halves carry weight.

Also confirmed this is the only multi-column side-label call site in the repo — bot-chat-detail.tsx:282 passes no label and falls to gridStackedMulti, mcp-page.tsx:900 and scheduled-task-inspector.tsx:134 are columns="single" and already baseline-aligned.

Four notes, none blocking.

[P3] The test's measurement changes meaning with the viewport

permission-center-metadata-layout.spec.ts:33-36 uses range.getBoundingClientRect().bottom, which is the union of all line boxes — equal to the baseline only while the text happens to occupy one line. At viewport 1490 with zoom 2, 「操作审批」's value wraps to two lines and that assertion computes a delta of 40 while the real first-line delta is 0; at 700 px with zoom 3 the union delta reaches ±360 with first-line deltas still flat at 0.

So page.setViewportSize({ width: 1490, height: 900 }) is an unstated load-bearing condition, and the failure direction is false red — a CJK font swap in CI or a one-character fixture edit is enough. range.getClientRects()[0].bottom holds at 0 everywhere I tried, including 700 px × zoom 3.

[P3] A 1 px residue sitting exactly on the tolerance

After the fix, the 所需系统权限 rows still differ by exactly 1.00 px (it was −1.0 px before — sign flipped, magnitude unchanged), against a toBeLessThanOrEqual(1) tolerance. The cause is rows.css:129, .settingsStatus { display: inline-flex } — a synthesised baseline, not a text one; injecting display: inline at runtime takes the delta to 0.

Not introduced here and not blocking, but two things follow: the body's "labels and values now share one horizontal baseline" is not quite true of that list, and any change to the status row's gap, dot size or font turns the assertion red with no real regression. Giving .settingsStatus align-items: baseline in this context would close it.

[P3] The new class may not be needed

.settingsCapabilityDetail already wraps both lists, and .settingsCapabilityDetail dl matches exactly and only these two MetadataLists — guidance and audit render ul via List. That would drop the two settingsCapabilityMetadata additions and match the .settingsCapabilityGroup .astryx-collapsible-trigger pattern already in the same file. The argument the other way (a named hook states intent, and gives the test a stable selector) is also fine — just say which one you meant.

[P3] Adjacent, pre-existing

The comment at permission-center-page.tsx:485-487 explains width: 92, but getGridTemplateColumns() returns repeat(${columns}, auto 1fr) whenever columns is a number > 1, so the label.width branch is unreachable and 92 is dead. Measured tracks are 70px 341px 56px 341px — the label columns are not even equal widths. Since this PR is editing those lines anyway, dropping width: 92 or noting that it has no effect would be more honest than leaving it. A separate issue is fine too.

On the choice of a call-site override

Reaching through MetadataList to its <dl> does depend on Astryx's internal structure, so I checked whether patches/ was the better home. It is not: patches/README.md says to prefer product code using the published API and to patch only for bugs that cannot be worked around at the call site, and the existing @astryxdesign+core+0.5.2.patch entry justifies itself with "No product override can reach this." This one is reachable. The override is the right home.

What I would still like visible is whether this was reported upstream — a line in the comment pointing at the report makes the override removable later instead of permanent, and stops the next multi-column side-label call site from copying it.

On placement

The spec asserts text baselines and computed font sizes, and adds a permissionCenterWindow fixture. Nothing in it needs Electron — it needs a layout engine — which is exactly the Storybook + Chromium case in #4761 P2; session-workbar.stories.tsx already compares getBoundingClientRect() in a play function. The fixture addition is the part worth avoiding, since e2e/fixtures.ts is what that tracker shrinks. Not a blocker, and I would rather have this covered in E2E than not covered.

Approving.

@Astro-Han
Astro-Han merged commit 3dc920f into apache:main Sep 4, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(desktop): align Permission Center metadata label/value rows

2 participants