Repository navigation
fix(web): stop the customer picker reserving 240px of height inside dialogs, and keep the phone dialog footer side by side - #896
Merged
Conversation
…ialogs, and keep the phone dialog footer side by side .form-grid .named-picker carries flex: 0 1 15rem for the wrapping filter bars, where the basis is a width. Inside .dialog .form-grid the grid is a column, so the same basis became a 240px HEIGHT: a hole under the customer field on Sales at 1280 and above it at 390, where the phone label rule grew into the slack. The dialog column now resets the picker to flex: 0 0 auto and full width, and pins labels and stepper fields to their content height. The phone .dialog .dialog-foot rule stacked Cancel and the action full-width; the owner's chosen phone shape (#892, 2026-09-17) keeps them side by side and right-aligned, with the 44px target still coming from the padding rule.
Owner
Author
|
@coderabbitai review |
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Owner
Author
|
Before (merged #892 head, light) and after (this head), same scenario: New order with a customer picked.
|
Owner
Author
|
Codex review (gpt-5.6-sol, read-only, in place of CodeRabbit's rate-limited round): no CSS correctness defects. The selectors match the MUI Paper ( Ready to merge on green CI. A fresh CodeRabbit trigger is queued for when its window resets; it can land after the merge without changing anything here. |
This was referenced Sep 17, 2026
mforce
added a commit
that referenced
this pull request
Sep 17, 2026
CodeRabbit's one nitpick on 8a758f6 (no product findings): D3.4's phone action-button rule still named DialogActions as part of the "stacks below 900px" set, but the #832/#896 amendment further down the same doc never explicitly said it superseded D3.4 for the five CRUD screens' dialog footers. Named it as a second, explicit exception in D3.4 itself (parallel to the existing Daily Entry footer exception) rather than leaving readers to infer it from the #832 amendment alone. The stacking rule is unchanged for every other action row. Also folds in the mutants.ts comment fix that was staged and held for this push: phone-dialog-footer-stacked's comment called the geometry check "same-top-edge", which stopped matching reality once the actual assertion became a vertical-band intersection (top-edge equality had false-failed against Grades' real dialog footer). Verified: SchemaDocsTests (4/4, incl. PostgresImagePin) and the Application.Tests RealTree architecture guards (13/13) both green after the docs edit; web + harness typecheck clean.
mforce
added a commit
that referenced
this pull request
Sep 17, 2026
Closes #832 ## Why Customers, Products, Grades, Flocks and Users are the last five screens still hand-styled with `styles.css`'s `table.data`/`.inline-form`/`.dialog-foot` families. This moves each screen's table to a MUI `Table`/`TableContainer` and each dialog's form fields to `TextField`/`Select`/`Checkbox`/`FormControlLabel` in a `Stack`, per #822 pairs 9 and 11, and Flocks' bird-movement drill-down to pair 15's ruled region (a `Box` between two `Divider`s). It also picks up #896's mid-flight dialog-footer decision, moving the five screens' `.dialog-foot` divs to MUI `DialogActions`. Everything not named by pairs 9/11/15 — `.page-head`, `.muted`/`.error` paragraphs, `button.link`, `StatusBadge`, `BusyButton`, `NumberField` — is unchanged: those belong to #828/#831/#833 and are still shared with a dozen unconverted screens. ## Scope - `web/src/routes/{Customers,Products,Grades,Flocks,Users}Page.tsx`: table → `TableContainer`/`Table size="small"`/`TableHead`/`TableBody`/`TableRow`/`TableCell` (`td.num` → `align="right"`); dialog forms → `Stack` + `TextField`/`Select`/`Checkbox`/`FormControlLabel`; dialog footers → `DialogActions`. - `web/src/components/ProvenanceCell.tsx`: `td.nowrap`/`td.provenance-cell` → `sx` on a MUI `TableCell`, padding pinned to `table.data td`'s legacy values so the three still-unconverted callers (Sales, Expenses, History) render unchanged. - `web/src/routes/FlocksPage.tsx`: `.order-panel` drill-down → pair 15's ruled region. - `web/src/theme/FarmThemeProvider.tsx`: adds `MuiTableCell` (row-scale font/line-height, `tabular-nums`) and `MuiTableContainer` (`contain: "layout"` at phone width — a real #441 repro, see Verification); removes a stale `MuiDialogActions` phone override that stacked buttons, contradicting #896's row/right-aligned decision. - `web/src/styles.css`: deletes `.dialog .confirm-body` / `.dialog .confirm-body strong` — the one class family this PR's grep found orphaned. - `tools/simulation/ui/specs/phone.spec.ts`, `mutation-check.sh`, `src/mutants.ts`: repoint the `/customers`/`/flocks` overflow-walk locators at `role=table` (`table.data` no longer matches), add the Grades dialog footer to the action-row walk, and narrow `phone-table-overflow-unclipped`'s expected routes to `/sales`/`/history` (the only two its `table.data`-scoped CSS still reaches). - `docs/designs/822-mui-revamp.md`: amended for four things that shipped differently than the doc assumed (see the amendment note under the D8 table). ## Tradeoffs `ProvenanceCell`'s padding is hardcoded to `table.data td`'s exact legacy values rather than left to MUI's density context, specifically so Sales/Expenses/History (still plain `<table>`) render pixel-identical until #831 converts them — a deliberate compatibility bridge, not the final shape. Grade/unit/role pickers use `TextField select slotProps={{ select: { native: true } }}` rather than a full `Select`+`MenuItem` tree: it renders a real `<select>` (same interaction the app already had) dressed in MUI's outlined chrome, and needed zero test rewrites. ## Blast Radius Five routes' presentation layer, plus one shared component (`ProvenanceCell`) and one shared theme file, both already exercised by three other unconverted screens (Sales, Expenses, History) and confirmed unchanged for them (see Verification). No API, validation, or RBAC logic changed — `UsersPage.test.tsx`'s ~3,900-line RBAC matrix passes unmodified. Landing this un-blocks #831/#833, which now inherit a working `MuiTableCell`/`MuiTableContainer` theme pair instead of building it from scratch. ## Verification - `npm run typecheck`, `npm test -- --run` (3119/3119), `npm run test:coverage` (91.43/88.1/86.98/94.31 stmts/branch/func/lines against 89/80/85/92 floors), `npm run build`, `npm run verify:sw` — all clean. - Per-screen test files: CustomersPage (53), ProductsPage (42), GradesPage (28), FlocksPage (51), UsersPage (158) — all pass, same `it()` count before and after in every file (zero tests added or removed; only internal assertions rewritten). Two rewritten per file at most (the `td.provenance-cell` CSS-selector queries → `getByTitle`/`queryByTitle`, only in Grades/Flocks, the two screens that render `ProvenanceCell`). `ProvenanceCell.test.tsx`, `ExpensesPage.test.tsx`, `HistoryPage.test.tsx`, `SalesPage.test.tsx` needed the same one-line rewrite for the same reason. - `farmTheme.policy.test.ts` (G2): two new rows (`MuiTableCell`, `MuiTableContainer`), each run red first (mutated the value, confirmed the new assertion failed on the exact literal) then restored green. Two rows retired (the stale `MuiDialogActions` phone-stacking assertion and its `FarmThemeProvider.render.test.tsx` companion) as a named, deliberate coverage reduction — nothing app-owned is left to pin once the override itself was deleted. - `styles.conversion.test.ts` (G1) does not exist in this branch (#824 hasn't landed); ran the guards that do exist — `styles.harness-selectors.test.ts`, `styles.declared-tokens.test.ts`, `styles.test.ts`, `styles.elevation.test.ts`, `styles.csp.test.ts`, `styles.caps.test.ts`, `styles.grades.test.ts` — all green before and after the `.confirm-body` deletion. - CSS deletion evidence: `git grep -n "confirm-body" -- ':!web/src/styles.css'` returns only this PR's own explanatory comment; the rest of the classes touched (`table.data`, `.page-head`, `.inline-form`, `.dialog-foot`, `tr.inactive`, `.numfield-field`, `.named-picker-trigger`, `.hint`, `.check`, `.cell`, `td.nowrap`, `.order-panel`) are confirmed still consumed elsewhere by the same grep and were left in `styles.css`. - Playwright quick suite (`tools/simulation/ui`, `npm test`) against a stack rebuilt at this PR's head: 54 passed, 1 pre-existing skip. Found and fixed a real regression along the way: `phone.spec.ts`'s overflow walk (once repointed off `table.data`) showed `/flocks` genuinely overflowing 390px (`scrollWidth` 941px) — MUI's `TableContainer` scrolls a wide table within itself but doesn't stop a mobile browser's layout-viewport sizing from measuring its raw content width, the same #441 defect `table.data`'s own phone rule already carries `contain: layout` to close. Fixed with the `MuiTableContainer` theme override above; re-verified clean after. - `mutation-check.sh phone-table-overflow-unclipped phone-action-label-wrapped phone-entry-foot-stacked`: baseline GREEN, all 3 mutants KILLED (survived: 0), restore GREEN. Confirms `/sales` and `/history` still overflow under the width-scoped CSS mutant while `/customers`/`/flocks` correctly do not (their containment now comes from `MuiTableContainer`, not `table.data`). - Dialog captures confirmed the FlockPicker inside Users' flock-access dialog has no layout void at either width (a concern raised mid-review, tied to a `.form-grid`-scoped bug fixed in #896) — UsersPage wraps it in `.inline-form`/`Stack`, never `.form-grid`, so it was never exposed to that class of bug. Screenshots below: each of the five lists at 1280×800 and 390×844, light and dark, plus one open create dialog per screen at both widths in light, plus the Users flock-access dialog (the FlockPicker check) and the Flocks bird-ledger ruled region. Captured "after" only, from this head — a coordinator-directed final checklist narrowed the closing scope after several rounds of stack coordination and a runtime regression fix (see the decision log); a before/after diff against origin/main for these five specific tables/forms is straightforward to add on request. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Modernized customer, flock, grade, product, and user screens with consistent tables, forms, dialogs, and layouts. - Added audit-history links to provenance details, including records without provenance data. - Improved table readability with aligned numeric values, consistent spacing, and mobile layout containment. - Preserved validation, permissions, pagination, and existing workflows. - **Bug Fixes** - Prevented provenance details from wrapping excessively by truncating long summaries. - Improved mobile table overflow handling and dialog footer layout. - **Tests** - Expanded coverage for mobile layouts, provenance tooltips, table overflow, and form label behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
mforce
added a commit
that referenced
this pull request
Sep 17, 2026
…#898) Closes #826 ## What changed Converts `NamedEntityPicker`'s open-state combobox (`FlockPicker`/`CustomerPicker`'s shared engine) from a hand-rolled `role="combobox"`/`role="listbox"` pair to MUI `Autocomplete` (D2 pair 1, `docs/designs/822-mui-revamp.md`). `Autocomplete` is fully controlled (`open`, `inputValue`, `options`, `loading`, `value`), `filterOptions={(o) => o}` since the server filters, and `getOptionLabel`/`getOptionKey`/`isOptionEqualToValue` all key by id. **What stayed:** the async discovery engine — discovery generations, the offset cursor driven by `serverCount`, FR-009 eligibility, retention, the US2/US3 selection-transition state machine, the unavailable states — is untouched. The Load more button renders through a custom `slots.paper` component so it lands as a sibling of the `<ul role="listbox">`, never inside it (`AX assertion added to named-entity-picker.spec.ts` that every listbox child is `role="option"`). The stable `aria-live` region stays a hand-written, always-mounted node, independent of `Autocomplete`'s own popper lifecycle. `FlockPicker`/`CustomerPicker` keep their exact props — no caller needed an edit for the engine conversion itself. **Owner redesign (2026-09-17, mid-implementation):** the closed/committed state no longer renders the page-supplied `trigger` element's own markup (a `<button className="named-picker-trigger">` that, per the #896 review, read as "a plain dark box with no affordance that it reopens a search"). It now reads as the SAME outlined MUI field as the open search — a read-only `TextField` with the picker's `label` floating in the border and a `lucide-react` `ChevronDown` end adornment (checked against #864's own Daily Entry mockup, `docs/designs/864-visual-language/daily-entry-390.png`, which draws this exact affordance as a chevron, not the magnifier the owner's instruction also allowed) — with click, Enter and Space all opening the search. The `trigger` prop's `onClick`/`disabled` are extracted and reapplied to the new field; its DISPLAYED VALUE (the committed name or a per-screen placeholder — "No flocks yet", "All", a loading/unavailable fallback) still comes from the caller's trigger children, unchanged, since the engine's own `committedText` has no placeholder to fall back to. The open state's floating `<label>` also moved onto the `Autocomplete`'s `TextField` itself. This changed the picker's closed-state ACCESSIBLE NAME from a hand-built "`<label> <current value>`" `aria-labelledby` chain to the label alone (the value now lives on the field's own `.value`), which broke 69 tests across six page suites — none in the four picker test files — all fixed the same mechanical way (role `button`→`textbox`, name matcher from the value/placeholder to the plain label, with `.toHaveValue(...)` added where a query doubled as a display check). **Flagged for reviewer/owner attention, not resolved here:** - `DailyEntryPage.tsx`'s `"& .named-picker-trigger"` `sx` override (#830's underlined-select restyle for this specific row) had nothing left to target once the trigger stopped rendering that class — removed as dead code, but whether Daily Entry's flock control should keep that underlined language or fold into the new global outlined look is a design call this PR does not make unilaterally. See the "committed state, Daily Entry" capture below. - **A real truncation and popper-width defect, found by the capture set — fixed and re-verified.** Round 1: at 390px, a committed flock name rendered as just "C…" (English, not only `tl`) because MUI's default padding left almost no room for text in the narrow two-up phone row; fixed with `size="small"` (commit 56f70f7). Round 2 (coordinator review of the round-1 captures): the OPEN listbox popper inherited the anchor's ~110px width and wrapped every option across 3-4 lines, and the open-focus effect's select-all showed the TAIL of a long name ("se A") because a browser scrolls to the active end of an overflowing selection. Both fixed (commits ebb9c4d, d504ae4): `slotProps.popper` floors the popup at `min(20rem, 100vw - 32px)` with `placement="bottom-start"`, `renderOption` keeps each option single-line with an ellipsis, and the open-focus effect keeps select-all (dropping it would reopen #735's own "typing replaces" bug) but forces `scrollLeft = 0` so the field shows the HEAD of the name. Re-captured against a freshly rebuilt isolated stack and viewed directly before shipping — see the round-2 390 frames below. The picker's field/row WIDTH itself (the two-up Flock/Date layout) is untouched: that is #830's layout decision, not this slice's "replace the combobox" mandate. Six real divergences from the design doc's plan, each found by running the rewritten suite against the real installed `@mui/material@9.4.0`, are recorded in full in `docs/designs/822-mui-revamp.md`'s "Amendment on pair 1 (#826)": - `Autocomplete`'s own Escape handling calls `stopPropagation()`, which broke a picker nested in a Dialog (Sales' new-order customer, Escape used to close both exploration and the dialog in one press) — fixed by handling Escape in the picker's own root `onKeyDown`, before `Autocomplete`'s internal switch, via `defaultMuiPrevented`. - `useAutocomplete.js`'s `handleValue` bails out of calling `onChange` when the newly selected option is `===` the current controlled `value` (reference equality, not `isOptionEqualToValue`) — a picker whose committed entity and discovery window share the same object reference (FR-037's page-level default customer) silently no-oped on re-clicking the already-committed option. Fixed by passing a shallow clone as `value`. - `getOptionKey` needed explicit wiring by id — without it, two same-named rows (a real, tested scenario) collide as React list keys. - The listbox's own status slot mounts unconditionally whenever the popup is open (even with no text to show), and can't express "loading while retained rows are still shown" (Load more in flight) — nulled via `slots.status`, keeping the engine's own status/alert markup as the sole source, so `getByRole("status")` stays unambiguous. - `disablePortal` is required, not just elevation — without it the popper renders in a portal to `document.body`, defeating the engine's own outside-click `mousedown` listener. - Two more real bugs surfaced only by a real-browser Playwright run (jsdom's synchronous rendering never exposed them): ArrowDown at a mid-pagination boundary let `Autocomplete`'s own handling wrap the highlight to the first option against a stale, still-short options list (fixed alongside the Escape suppression); and `disableListWrap` stops the same wrap once discovery is fully exhausted, so the true last option holds still instead of teleporting back to the top. ## Tests | File | Lines before → after | Tests before → after | What changed | |---|---|---|---| | `NamedEntityPicker.tsx` (component) | 1,147 → 1,319 | — | engine untouched; render layer rebuilt on `Autocomplete` + the closed-state `TextField` redesign | | `NamedEntityPicker.test.tsx` | 1,424 → 1,420 | 52 → 52 | 2 rewritten: T023-3 (activedescendant format, not a literal id string), T023-12 (closed-state field mechanics, not a `<button>`) | | `namedEntityPicker.p1.test.tsx` | 241 → 241 | 5 → 5 | unchanged — pure open-state engine tests, no mechanism dependency | | `namedPickerUS3.recovery.test.tsx` | 138 → 138 | 3 → 3 | unchanged | | `NamedEntityPicker.openFocus.test.tsx` | 49 → 52 | 1 → 1 | 1 rewritten: click target is the new read-only field, not a `<button>` | | `DailyEntryPage.test.tsx` | +81/-… | 81/81 pass | 16 tests' picker-trigger queries fixed (role/name), plus `getByLabelText` exact-match loosened for the new MUI-painted required asterisk | | `ExpensesPage.test.tsx` | +5/-… | 71/71 pass | 1 shared helper (`pickAddFlock`) fixed | | `FeedPage.test.tsx` | +22/-… | 21/21 pass | 2 real query breaks fixed (+3 cascading order-dependent fallout), plus 3 `getByLabelText`+`toHaveTextContent` sites switched to `toHaveValue` (same underlying mechanism change) | | `SalesPage.test.tsx` | +44/-… | 231/231 pass | 11 tests' picker-trigger queries fixed across two `CustomerPicker` instances (dialog + page filter) | | `UsersPage.test.tsx` | +39/-… | 158/158 pass | 24 tests fixed via two shared helpers (`pickFlock`, `assignTrigger`) | | `WaterPage.test.tsx` | +68/-… | 30/30 pass | 10 tests fixed, dead `flockTriggerName` helper removed | Full web suite: `npm run typecheck` clean; `npx vitest run` 132 files / 3,116 tests green; `npx vitest run --coverage` 91.41 / 88.06 / 86.99 / 94.31 (statements/branches/functions/lines) against floors 89/80/85/92 — no re-baseline. `npm run build` + `npm run verify:sw` clean, precache 1,631.12 KiB (well under the 1,800 KiB D9 ceiling). ## Guards touched, and the mutation that proves each - `styles.elevation.test.ts`'s `SHADOW_ALLOWED` (removed `.named-picker-listbox`, now dead): added a throwaway `.mutant-test-selector { box-shadow: ... }` to `styles.css`, confirmed the guard goes red (`selectorsCastingShadow()` picks it up, equality fails), reverted, confirmed green again. - `styles.elevation.test.ts`'s radius `it.each` (removed `.named-picker-trigger`): list-membership change only, no new assertion logic to mutate; `input`'s own row still exercises the same `--r-input` token. - `styles.test.ts`'s `button.named-picker-trigger` link-bleed guard: retired (no successor — the concern doesn't apply to an MUI-themed field). - `styles.bare-elements.test.ts`'s non-vacuity floor: lowered from 150 to 120 to match the real post-deletion count (149, down from 181) — a floor, not a pin, per the guard's own comment. ## CSS deletion, with grep evidence Deleted (`web/src/styles.css`): `.named-picker-control` (+ its magnifier pseudo-elements, disabled/focus/aria-expanded input variants), `.named-picker-listbox`, `.named-picker-option` (+ `:last-child`/`:hover`/`.active`/`[aria-selected]`), `.named-picker-trigger` (base + `button`-qualified + `:hover`/`:disabled`), and the 900px media-query overrides for the listbox/option pair — ~216 lines total. Nothing renders any of these selectors anymore: `Autocomplete` owns the open-state markup, and the closed-state `trigger` element is read for its props only, never inserted into the DOM. Grepped the whole repo (`git grep -n "<class>" -- ':!web/src/styles.css'`, including `tools/simulation/ui/`) before deleting anything: - `.named-picker-listbox` / `.named-picker-option` / `.named-picker-control`: 0 hits outside `styles.css` — never referenced elsewhere. - `.named-picker-trigger`: still referenced by 11 caller `className` props (unchanged, now inert since the engine never renders the trigger element itself) plus historical design-doc/runbook prose — CSS-only deletion, no caller edits. Kept (still have a live render site, confirmed by the same grep): `.named-picker`, `.named-picker-label`, `.named-picker-committed`, the `.named-picker-meta`/`-status`/`-live`/`-loadmore`/`-clear` family, `.form-grid .named-picker` (+ #896's `.dialog .form-grid .named-picker` override), `.named-picker.disabled`. ## Script time (674's method) Production Docker builds (`deploy/docker-compose.yml`, isolated from the shared sim stack), 390×844 viewport, 6× CPU throttle, CDP `Performance.getMetrics()` deltas around a navigation to `/daily-entry`, median of 9, landmark = the flock picker's closed-state trigger visible: | build | wall | script | style recalc | layout | |---|---|---|---|---| | before, `91e3d65` (hand-rolled trigger) | 1450 ms | 831 ms | 39 ms | 72 ms | | after, this PR (MUI `Autocomplete`) | 1493 ms | 883 ms | 40 ms | 73 ms | Script time rises ~52 ms (+6%) for `Autocomplete` on Daily Entry — confirmed genuine by diffing bundle chunks (before's `NamedEntityPicker` chunk is 12.3 KB; after's is 69.8 KB, 5.7×). Wall time moves 43 ms; style-recalc/layout are flat within noise. Well inside the "light dose" territory 674 measured for the Dashboard's eleven components (+29 ms, +27%) — the `Autocomplete`-specific slope 674 flagged as unmeasured does not blow up script time on this screen. ## Captures 1:1, both widths, light theme, from isolated `deploy/docker-compose.yml` builds (before: `91e3d65`; after: this PR's head, after also includes the `size="small"`, popper-width and selection-head fixes; the 390 Daily-Entry-open and Sales-committed "after" frames were re-captured in round 2, superseding the round-1 versions inline above) — never the shared `cluckwork-sim` stack. 17 frames attached below: **Not a defect:** in the Sales toolbar capture, the Customer filter now renders as an outlined MUI field while the Status `<select>` beside it is still the old hand-rolled control — expected, since #831 has not yet converted `FilterBar` to MUI. - Daily Entry's flock picker, open with results / Load more visible (same frame — the fixture's first page already has `hasMore`): before/after × 1280/390. - Daily Entry's flock picker, closed, unavailable state: before/after × 1280/390. - Daily Entry's flock picker, open, `tl` locale: after-only, 390 (see the truncation finding above). - Sales "New order" dialog, customer committed (the trigger affordance): before/after × 1280/390. ## Design doc Amended `docs/designs/822-mui-revamp.md`'s D2 pair 1 with the "Amendment on pair 1 (#826)" section covering all six technical divergences plus the owner's presentation redesign, following the same pattern the pair-2 (#827) amendment already established.                  ## Codex review of #898 (2026-09-18) **First round (head 4a08662), six findings, all confirmed real and fixed** (`4a525f2`, `0f23fc0`): 1. `tools/simulation/ui/src/dom.ts`'s `commitNamedPicker` matched the discovery response with `encodeURIComponent` (`%20` for a space); the app builds the query string via `URLSearchParams` (`+` for a space) — every real call site passes a multi-word needle, so this never matched and hung until timeout. Fixed to build the expected fragment with the same serializer. 2. `NamedEntityPicker.tsx`'s Autocomplete `value` clone was recreated every render, giving `useAutocomplete.js`'s `syncHighlightedIndex` a new identity each time and retriggering its resync effect on renders unrelated to the committed value. Memoized by committed-entity id/name (`clonedSelectedValue`). 3. `highlightedIdRef` (FR-032's `atEnd` check) was set but never cleared on Escape/outside-click/typing/close — a retained discovery window reopened after reaching the true end falsely read "already at the end" on the first ArrowDown. Cleared on all four transitions. 4. The `disableListWrap` comment claimed the real-browser paging spec proved ArrowDown at the true final option does not wrap — the spec committed the moment its sentinel was highlighted, never testing that. Spec now pages past the sentinel to the true end and asserts one more ArrowDown leaves it there. 5. T023-12 asserted the closed field's static open contract but never pressed a key — added T023-12b (Enter/Space activate, no other key does). 6. The phone-stacking comment overclaimed the 1280 layout as pixel-identical to the old flat row — corrected to describe the actual (owner-approved) layout. The full quick Playwright suite (mandatory, all 55, both projects) was run against an isolated stack built at head, per instruction not to stop at the picker spec alone — it surfaced two MORE real defects beyond the six: `commitNamedPicker` hung a second, opposite way (a restricted worker's auto-prefilled picker made `fill(needle)` a no-op when the field already read `needle`), and `worker.spec.ts` asserted the closed trigger's bare flock name when it always renders `"{name} ({breed})"`. Both fixed (`0f23fc0`). Final suite result: 54 passed, 1 deliberately skipped, 0 failed. **Second round (head 0f23fc0), two items** (`bf5c81b`): - T023-7c only pinned the close/reopen `highlightedIdRef` clear. Added three siblings (T023-7e typing, T023-7f Escape, T023-7g outside-click), each using T023-7c's own id-collision technique — all four clear sites individually mutation-checked red-then-green. - Finding 2's "no jsdom test can pin it" claim was tested against a specific suggested construction (a deferred, manually-held extension request) — still mutation-checked vacuous (identical result memoized/unmemoized). Documented honestly rather than shipped as a guard that reads as safety without being one. **Third round (head bf5c81b, finding 2 follow-up), real-browser construction** (`d10d892`): built the same deferred-request construction in `named-entity-picker.spec.ts` against a real, isolated stack at head — commit a flock, page to the true end, hold the extension's own request via `page.route`, assert `aria-activedescendant` mid-flight and after landing. **Passed with the fix in place.** Mutation check: swapped `clonedSelectedValue` back to the inline unmemoized clone, rebuilt the app image, reran the identical spec — **passed again, unchanged.** Real React scheduling, not jsdom's, and still vacuous for this exact scenario (commit-then-page, one committed value, one extension) — this is the fourth failed attempt at an observable regression test for this mechanism, across two harnesses. Not shipped, per the standing rule against a vacuous guard; both real-browser results are recorded in `NamedEntityPicker.test.tsx`'s comment (now a four-attempt account) and cross-referenced from the spec file. The fix itself (`clonedSelectedValue`) is unchanged and still the correct response to the mechanism the first review identified by direct `useAutocomplete.js` reading — four failed attempts at catching it from the outside is a fact about the symptom's observability, not evidence the fix is unneeded. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Updates** * Named-entity pickers now use outlined, read-only text-field triggers with keyboard activation, clearer option handling, retry controls, and load-more support. * Improved accessibility with label-based names, textbox semantics, value reporting, and active-option tracking. * Daily Entry controls now use outlined fields and stack vertically on smaller screens. * Added localized open and close labels in English, Spanish, and Tagalog. * **Tests** * Expanded coverage for picker interactions, paging, accessibility, responsive layout, and recovery behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #674. Found by the owner on the merged #892 head (2026-09-17): the Sales "New order" dialog showed a hole under the customer field at 1280 and above it at 390, and the phone footer stacked its two buttons full-width.
Cause
.form-grid .named-pickercarriesflex: 0 1 15remfor the wrapping filter bars, where the basis is a width. Inside.dialog .form-gridthe grid is a column, so the same basis became a 240px height. Measured live on a stack atmainbefore the fix: the picker element was 240px tall at both widths; at 390 the phone rule.form-grid label { flex: 1 1 40% }then grew the label into that slack, which is why the hole moved above the field there. This predates #892 (the bottom-sheet before-frames on #892 show the same hole), but #892's centred content-height dialog made it the whole picture.Fix
.dialog .form-grid .named-picker { flex: 0 0 auto; width: 100% }, plus labels and stepper fields pinned to content height inside dialog columns at phone width..dialog .dialog-footrule goes from column/stretch to row/right-aligned, matching the phone shape the owner chose from the feat(web): convert Dialog and useConfirm to MUI Dialog #892 mockups. The 44px touch target still comes from the padding rule.Verification
npm run typecheck,npx vitest runEvery dialog that holds a
NamedEntityPickerinherits the fix (Sales new order, History adjust, and the Daily entry flock picker where it renders inside a dialog). #832 converts the CRUD dialogs on top of this.