crueber/walhub

Watch

Fork 0

Merge strategy select on PR page is an unstyled native control — style with the system select idiom (both themes) #547

New issue

Closed

opened 2026-09-14 22:46:15 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 22:46:15 +00:00

Owner

What's requested

Style the merge-strategy <select> on the pull request page with the app's standard select idiom, correct in both light and dark themes.

Evidence (static, no local repro)

web/src/components/MergeBox.jsx:167 — the strategy select carries no class at all:

<label class="field">
  <span>Strategy</span>
  <select value={getStrategy()} onInput={(e) => setStrategy(e.target.value)} disabled={getMerging()}>

Observed on the live PR page:

The control renders as an unstyled native select — the closed control takes OS chrome (blue highlight on focus/interaction) instead of the app's bordered input look.
The dropdown panel is native: white background with OS blue option highlight even in dark theme, because nothing sets color-scheme or theme-aware option colors anywhere (grep color-scheme web/css/ web/src/ is empty).
The bare select sits in a label also styled with an undefined class (field has no rule in any stylesheet) and crowds the merge / update-branch button row below (MergeBox.jsx:173).

Architecture notes

The repo already has the canonical select idiom — .input in web/src/ui.css:130 (Tailwind component class: rounded border, bg-white/text-zinc-900 in light, dark:bg-zinc-900 dark:text-zinc-100 in dark, emerald focus ring) — used by sibling selects:

web/src/components/VisSelect.jsx:32 — class="input"
web/src/components/OwnerNameRow.jsx:52 — class="input h-9 truncate font-mono"
web/src/pages/Wal.jsx:242 — class="input inline-block w-44"

Note the competing legacy rule: web/css/base.css:74 styles bare select elements via element selector with the --bg/--fg/--border vars, which partially themes the closed control but leaves the popup native — that partial coverage is exactly why this one reads as "almost styled but wrong."

Fix shape

Apply the .input idiom (plus an explicit width, e.g. h-9/w-auto or a fixed width matching the row) to the strategy select, mirroring the sibling selects above.
Ensure the dropdown options render themed in dark mode — set color-scheme: dark on the dark theme root (and light in light) so the native popup panel and option highlight follow the theme, per the "system select idiom" ask; do NOT remove the base.css:74 element rule without checking other bare selects.
Also verify the field label class: either give it a rule or drop it for the idiom used on sibling forms, so the select, its "Strategy" caption, and the button row below have consistent spacing and no overlap.

Acceptance criteria

Strategy select uses the .input idiom (border, radius, padding, focus ring) matching VisSelect/Wal selects.
Closed control and dropdown panel both render correctly in light AND dark theme — no white panel / OS blue highlight in dark mode.
No overlap with the merge / update-branch button row at desktop and mobile widths.
The field label class is either styled or removed consistently (no undefined classes left on this form).
Sibling bare selects elsewhere are not visually regressed.

**What's requested** Style the merge-strategy `<select>` on the pull request page with the app's standard select idiom, correct in both light and dark themes. **Evidence (static, no local repro)** `web/src/components/MergeBox.jsx:167` — the strategy select carries no class at all: ```jsx <label class="field"> <span>Strategy</span> <select value={getStrategy()} onInput={(e) => setStrategy(e.target.value)} disabled={getMerging()}> ``` Observed on the live PR page: - The control renders as an unstyled native select — the closed control takes OS chrome (blue highlight on focus/interaction) instead of the app's bordered input look. - The dropdown panel is native: white background with OS blue option highlight even in dark theme, because nothing sets `color-scheme` or theme-aware option colors anywhere (`grep color-scheme web/css/ web/src/` is empty). - The bare select sits in a label also styled with an undefined class (`field` has no rule in any stylesheet) and crowds the merge / update-branch button row below (`MergeBox.jsx:173`). **Architecture notes** The repo already has the canonical select idiom — `.input` in `web/src/ui.css:130` (Tailwind component class: rounded border, `bg-white`/`text-zinc-900` in light, `dark:bg-zinc-900 dark:text-zinc-100` in dark, emerald focus ring) — used by sibling selects: - `web/src/components/VisSelect.jsx:32` — `class="input"` - `web/src/components/OwnerNameRow.jsx:52` — `class="input h-9 truncate font-mono"` - `web/src/pages/Wal.jsx:242` — `class="input inline-block w-44"` Note the competing legacy rule: `web/css/base.css:74` styles bare `select` elements via element selector with the `--bg`/`--fg`/`--border` vars, which partially themes the closed control but leaves the popup native — that partial coverage is exactly why this one reads as "almost styled but wrong." **Fix shape** - Apply the `.input` idiom (plus an explicit width, e.g. `h-9`/`w-auto` or a fixed width matching the row) to the strategy select, mirroring the sibling selects above. - Ensure the dropdown options render themed in dark mode — set `color-scheme: dark` on the dark theme root (and `light` in light) so the native popup panel and option highlight follow the theme, per the "system select idiom" ask; do NOT remove the base.css:74 element rule without checking other bare selects. - Also verify the `field` label class: either give it a rule or drop it for the idiom used on sibling forms, so the select, its "Strategy" caption, and the button row below have consistent spacing and no overlap. **Acceptance criteria** - [ ] Strategy select uses the `.input` idiom (border, radius, padding, focus ring) matching VisSelect/Wal selects. - [ ] Closed control and dropdown panel both render correctly in light AND dark theme — no white panel / OS blue highlight in dark mode. - [ ] No overlap with the merge / update-branch button row at desktop and mobile widths. - [ ] The `field` label class is either styled or removed consistently (no undefined classes left on this form). - [ ] Sibling bare selects elsewhere are not visually regressed.

crueber added the

bug

pull requests

labels

2026-09-14 22:46:22 +00:00

crueber added this to the v1 milestone

2026-09-14 22:46:22 +00:00

crueber referenced this issue from a commit

2026-09-15 01:12:59 +00:00

Fix #547: merge strategy select joins the system select idiom (12 web UI)

crueber referenced this issue from a pull request that will close it,

2026-09-15 01:13:04 +00:00

Fix #547: merge strategy select #552

crueber commented

2026-09-15 01:13:06 +00:00

Author

Owner

Fixed by #552 (branch fix/issue-547): MergeBox strategy select takes .input w-full in the #479 label shape; ui.css base layer declares color-scheme light/dark so the native dropdown follows the theme. Tests 1284 total / 1283 pass (1 pre-existing live-smoke failure, zero PR-caused); vite + esbuild green.

Fixed by #552 (branch fix/issue-547): MergeBox strategy select takes `.input w-full` in the #479 label shape; ui.css base layer declares color-scheme light/dark so the native dropdown follows the theme. Tests 1284 total / 1283 pass (1 pre-existing live-smoke failure, zero PR-caused); vite + esbuild green.

crueber referenced this issue from a commit

2026-09-15 01:26:58 +00:00

Fix #547: review — sweep walks web/src, not just components

crueber commented

2026-09-15 01:32:55 +00:00

Author

Owner

Review of PR #552 (fix/issue-547) — verified in scratch worktree, main untouched.

All 5 acceptance criteria hold:

.input idiom + width ✓ — MergeBox.jsx:167 select is now class="input w-full", byte-matching the same-page finish-review-verdict sibling (Pull.jsx:678). w-full fills the 16rem sidebar row (Pull.jsx:891 md:grid-cols-[1fr_16rem]); stacks full-width at ~390px.
Themed panel, both roots ✓ — ui.css:28-29 :root{color-scheme:light} + .dark{color-scheme:dark} in @layer base (F1 roots: .dark ships on <html>, light = class off). Compiled-CSS proof: dist bundle contains :root{...color-scheme:light}.dark{...color-scheme:dark} before @layer components. No appearance-none anywhere — native arrow/keyboard kept per OwnerNameRow precedent.
No overlap ✓ — label.grid.gap-1.mt-2 block + div.mt-2.flex.flex-wrap.gap-2 button row below in normal flow; wrap handles narrow. Reasoned only (no browser per review instructions — noted explicitly).
Field label fixed ✓ — undefined class="field" gone from this form; caption is text-sm font-medium, mirroring the #479 finish-review label (Pull.jsx:676).
Siblings unregressed ✓ — independent full-src check: all 24 real tags across web/src (components + pages) carry .input; comment-prose mentions only. base.css:74 untouched and unbundled (index.jsx imports ui.css only). PullNew.jsx field labels (4 sites) correctly stay a noted follow-up, not re-legitimized.
(6-8) No backend/SDK/API change (4 files: MergeBox.jsx, ui.css, new test, 12_web_ui.md amendment); no new deps (package.json untouched); docs entry accurate — counts/claims re-verified below.
One fix pushed (dc08fe8): the new test's sibling sweep walked only web/src/components while asserting 'anywhere in web/src' — SRC_DIR now points at web/src (68 jsx files, 6/6 green). Small test-only change; substance of the claim was already true.

Final verification (scratch worktree @ dc08fe8, node_modules symlinked from main):
- node --test web/test/unit/merge-strategy-select-547.test.js: 6/6 pass
- full node --test web/test/unit/*.test.js: 1284 total / 1283 pass / 1 fail — the 1 failure is the live-server smoke subtest (smoke.test.js /setup 403), confirmed pre-existing by running smoke.test.js on pristine origin/main (2 pass / 1 fail, same subtest)
- vite build green; esbuild bundle green; color-scheme light+dark present in dist CSS
MERGE RECOMMENDATION: ready to merge.

Review of PR #552 (fix/issue-547) — verified in scratch worktree, main untouched. All 5 acceptance criteria hold: 1. .input idiom + width ✓ — MergeBox.jsx:167 select is now class="input w-full", byte-matching the same-page finish-review-verdict sibling (Pull.jsx:678). w-full fills the 16rem sidebar row (Pull.jsx:891 md:grid-cols-[1fr_16rem]); stacks full-width at ~390px. 2. Themed panel, both roots ✓ — ui.css:28-29 :root{color-scheme:light} + .dark{color-scheme:dark} in @layer base (F1 roots: .dark ships on <html>, light = class off). Compiled-CSS proof: dist bundle contains :root{...color-scheme:light}.dark{...color-scheme:dark} before @layer components. No appearance-none anywhere — native arrow/keyboard kept per OwnerNameRow precedent. 3. No overlap ✓ — label.grid.gap-1.mt-2 block + div.mt-2.flex.flex-wrap.gap-2 button row below in normal flow; wrap handles narrow. Reasoned only (no browser per review instructions — noted explicitly). 4. Field label fixed ✓ — undefined class="field" gone from this form; caption is text-sm font-medium, mirroring the #479 finish-review label (Pull.jsx:676). 5. Siblings unregressed ✓ — independent full-src check: all 24 real <select> tags across web/src (components + pages) carry .input; comment-prose mentions only. base.css:74 untouched and unbundled (index.jsx imports ui.css only). PullNew.jsx field labels (4 sites) correctly stay a noted follow-up, not re-legitimized. (6-8) No backend/SDK/API change (4 files: MergeBox.jsx, ui.css, new test, 12_web_ui.md amendment); no new deps (package.json untouched); docs entry accurate — counts/claims re-verified below. One fix pushed (dc08fe8): the new test's sibling sweep walked only web/src/components while asserting 'anywhere in web/src' — SRC_DIR now points at web/src (68 jsx files, 6/6 green). Small test-only change; substance of the claim was already true. Final verification (scratch worktree @ dc08fe8, node_modules symlinked from main): - node --test web/test/unit/merge-strategy-select-547.test.js: 6/6 pass - full node --test web/test/unit/*.test.js: 1284 total / 1283 pass / 1 fail — the 1 failure is the live-server smoke subtest (smoke.test.js /setup 403), confirmed pre-existing by running smoke.test.js on pristine origin/main (2 pass / 1 fail, same subtest) - vite build green; esbuild bundle green; color-scheme light+dark present in dist CSS MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-15 01:34:26 +00:00

crueber referenced this issue from a commit

2026-09-15 01:34:26 +00:00

Merge pull request 'Fix #547: merge strategy select' (#552) from fix/issue-547 into main

crueber commented

2026-09-15 01:34:26 +00:00

Author

Owner

Fixed by PR #552 (review clean + one test-scope fix by reviewer; all 8 checks pass, themed panel verified in compiled CSS), merged. Closing.