Skip to content

fix(desktop): prevent WorkHub result overlap - #3645

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-workhub-result-overlap
Aug 23, 2026
Merged

fix(desktop): prevent WorkHub result overlap#3645
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-workhub-result-overlap

Conversation

@ARE404

@ARE404 ARE404 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Prevents two-line WorkHub Session metadata from overflowing into the returned result text.

  • Lets the submitted and clarification target buttons grow beyond the Astryx medium control fixed height while retaining the same minimum height.
  • Adds a real Electron layout regression test that measures the rendered Session metadata and result geometry.

Refs #3492

Root cause

The Astryx medium Button sets a fixed 32px height. WorkHub adds 10px vertical padding and renders Session name plus project name inside that control, so the second line overflowed outside the button and occupied the same vertical space as .workhub-result.

Verification

  • npm run build — passed
  • npm run lint — passed (2,650 files)
  • npm run format:check — passed (1,599 files)
  • npm --workspace @maka/desktop run typecheck — passed
  • npx playwright test workhub-layout.spec.ts --config e2e/playwright.config.ts — 1/1 passed
  • git diff --check — passed

The regression failed before the CSS fix because the button did not contain the project metadata. After the fix, the real Electron layout reports the metadata inside the button with no vertical overlap against the result.

UI evidence

  • Attach a fixed-state screenshot or short recording before marking ready for review.

AI use

Select exactly one:

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

Tool(s) and scope: Codex diagnosed the rendered geometry, implemented the CSS fix, and added the Electron regression test. The affected commit includes a Generated-by: Codex trailer.

Checklist

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

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Allow WorkHub target buttons to grow beyond the fixed medium control height so Session metadata stays clear of the result text. Add an Electron layout regression test.

Generated-by: Codex
@ARE404
ARE404 marked this pull request as ready for review August 23, 2026 17:16

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

Reviewed at exact head 04a00a20044854379ca054d79ca426c93404703a. The fix mechanism was verified by mutation rather than by reading, and it holds: the fixed height comes from the Astryx Button component (@astryxdesign/core/src/Button/Button.tsx sets a fixed height per size and defaults to md), so height: auto is what actually releases the box and lets two lines of content grow it. Removing both declarations turns the new spec RED on buttonContainsProject; restoring height: auto alone turns it GREEN.

That same experiment is what makes the second declaration a problem — see the inline note.

Blocking on machine state, not on the code: exact-head test is completed/failure. The failure is not the UI spec — it is Astryx surface inventory, where the committed generated doc says 204 files and the generator wants 206. Run npm run astryx:surface-inventory:write and commit the regenerated output. Nothing here can be approved until that is terminal green.

justify-content: space-between;
width: 100%;
height: auto;
min-height: var(--h-control-md, 32px);

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.

[P3] This declaration is redundant for the bug being fixed, and it hardcodes a value that does not match its own token.

The mutation run isolates the cause: with height: auto alone the new spec passes 1/1. min-height therefore contributes nothing to the overlap fix — it only asserts a floor that the content already exceeds in the failing case.

It also introduces a magic number. --h-control-md resolves to var(--size-element-sm), which is 28px, not the 32px written as the fallback. The fallback is unreachable while the token is defined, so this is latent rather than live — but it is a second, wrong copy of a managed value, and if the token ever went missing the layout would silently take a size that exists nowhere in the design system.

Suggest deleting the whole min-height line. That removes the hardcoded number entirely and leaves the fix as the one declaration that is actually doing the work. If a floor is genuinely wanted for empty-content cases, that is a separate change and should carry its own justification and use the token without a divergent fallback.

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

Freshness review on the new exact head 6082ad353a1ae852f975cd6fb2413c972b1fea63.

The delta from the previous head is exactly the two requested changes and nothing else: the redundant min-height line is gone from workhub.css, and the Astryx inventory totals move 204 → 206. No other file, no incidental edits.

My earlier [P3] is closed. height: auto alone is the fix — it releases the fixed height that @astryxdesign/core's Button sizeStyles applies via --size-element-sm; the min-height was a second, and wrong, copy of that value (--h-control-md resolves to 28px, not the 32px fallback that was written).

On the check set: only test appears on this head, and that is correct rather than incomplete. dependency-audit.yml and windows-sandbox-w0.yml are both path-filtered, and this PR touches only apps/desktop/src/renderer/styles/workhub.css and docs/astryx-surface-file-inventory.md, which match neither filter. So test being terminal success is the complete required set here.

One cross-PR note for whoever merges: #3640 edits docs/astryx-surface-file-inventory.md too, but it is the same 204 → 206 change. From the shared merge base the two are conflict-free and both land on 206, so merge order does not matter.

No P0–P2 outstanding. Approving; I am not merging — that call belongs to a human.

@Astro-Han
Astro-Han merged commit 5525be3 into apache:main Aug 23, 2026
1 check passed
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.

2 participants