Repository navigation
refactor(web): pass TopBar centerWidth="wide" in the shell header - #2599
Conversation
The header keeps its own grid override for a centre that holds a search and the Reflect action. TopBar now owns that layout through centerWidth="wide" (adea-ai/ui#446, released in 0.139.0, present in the pinned 0.140.0), so the local .m7-application-header grid rule and its class are removed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
Coordinating review outcome: PENDING qualification on exact a642c09. Personally read the full two-file patch, caller and pinned UI v0.140.0 TopBar implementation; centerWidth=wide is genuinely supported and no broad dependency/runtime change is present. Before approval, qualify the actual caller at320/390px with200%text/browserzoom and the macOS inset where supported. The new7rem center minimum combines with the caller's clipped leading section and trailing minimum; source arithmetic suggests the navigation control could be squeezed/clipped under enlarged text. This is a potential regression to test, not a claimed observed failure. Preserve keyboard access to nav/search/Reflect/trailing controls and return measurements/screenshots. Existingowner has been contacted through projectlead. Do not weaken thresholds or reintroduce an unreviewed local grid fork.
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
Review outcome: CHANGES REQUESTED for this exact head (recorded as COMMENT because the connected account is the PR author). The complete two-file patch was personally reviewed. Follow-up qualification from the existing implementation owner reproduces the suspected narrow-layout regression: at 200% text sizing, the 390px header exposes only about 7px of the 56px navigation toggle; at 320px it disappears and Actions extends beyond the bar. The main baseline keeps navigation visible. The new shared minmax(7rem, 1fr) center minimum interacts with the caller's clipped leading section and trailing minimum. Preserve usable navigation/actions under constrained width and enlarged text, preferably with a shared TopBar regression fixture matching this caller, then publish the repair and rerun exact-head qualification. Search collapsing at 200% occurs on both baseline and candidate and is not attributed to this patch. Narrow web behavior must not be mislabeled as a native macOS width failure (native minimum width is 980px). Keep draft and auto-merge off pending repair and review.
The Mod+K hint chip in the search field's trailing addon competes with the spinner and the clear control for width at 320px and 200% text. Mobile has no keyboard shortcut to hint at, so hide the chip when the navigation is in its mobile layout and keep it on wide windows. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
|
Pushed Checks run at this head, in the
Earlier measurements (throwaway build with pinned UI 0.140.0 plus the #465 TopBar change): the search field is 98px wide at 320px with 200% text and 168px at 390px with 200% text. Those were not re-measured on this commit. State is unchanged: this PR stays draft, held, auto-merge OFF. It is not released, pinned, or merged, and #465's release and pin still gate that. This work is separate from #465. Generated by Claude Code |
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
Review outcome: PENDING dependency qualification at 8b0597d. I personally reviewed both changed files since the previously reviewed a642c09 head. Hiding only the keyboard hint on mobile preserves the loading spinner and desktop hint, and the regression covers wide/mobile mounts. This accepts the bounded consumer repair, not full PR approval: the shared TopBar fix in adea-ai/ui#465 still needs qualified release/pin before the original enlarged-text clipping finding can be closed. Run real-header narrow-width/200% text and keyboard checks against that actual pinned package, including resize behavior; draft-light CI is not passing full qualification. Update the PR description to distinguish this new repair and the still-blocked dependency from the older validation. Keep draft and auto-merge off; #2568 remains open.
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
Personal review outcome: PENDING shared-package qualification at exact head dfd9402. I read the complete current header/test/style patch. The bounded wide-header and mobile shortcut-hint repair remains understandable, but merging main does not satisfy the outstanding consumer qualification: UI #465 is merged, while the release/published package and this branch's pin must be verified before narrow-width, 200% text, keyboard and resize acceptance can close. Keep draft/auto-merge off and preserve the existing owner. The PR body still describes #465 as unmerged and should be reconciled with current dependency state; it is not evidence that this head has been qualified. #2568 stays open.
0.142.4 is the first published @adea-ai/ui release that contains the top-bar fix for narrow widths and enlarged text (adea-ai/ui#465), and it declares @adea-ai/themes ^0.10.10, so the themes pin moves with it to keep a single themes copy. The shared NOTICE headings follow the installed packages; the published notice texts are unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
PENDING at 8747da4. Personally reviewed the complete current patch, including UI0.142.4/themes0.10.10 pins, lockfile and notices; earlier header/hint source review remains applicable. Existing owner reports layout qualification at320/390/1100 and100%/200% text, resize10/10 and Ctrl+K4/4, but also a Tab-order issue at200% text. Request exact reproduction/expected behavior and bounded correction before final approval; do not dismiss it based on successful Ctrl+K. Exact-head frozen-install evidence and factual PR-body update are outstanding. Full hosted CI remains draft-gated; #2568 stays open.
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
SOURCE APPROVED at 8747da4 after personal complete-patch review and exact-head owner qualification. Published UI0.142.4/themes0.10.10 pins and registry integrity match; clean frozen install, lint/format/types,78files/722tests and build pass. Narrow/enlarged-rem layout clipping is resolved in the measured Chromium141 cases. The remaining two-row Tab traversal is documented: all controls remain reachable and visible, DOM order remains leading→search/Reflect→trailing, without a focus trap; I treat that visual/DOM ordering mismatch as a bounded upstream follow-up rather than a blocker for this clipping repair. This is not a claim of real browser zoom, Firefox/WebKit or desktop inset qualification. Preserve #2568 and the upstream follow-up. Full applicable hosted checks/protections still required before merge; this personal review does not replace any required independent approval.
When the top bar wraps to a second row, the System status details opened under the status button, over the bar's second row. That hid the search field and, with the taller bar, put the popover's action buttons over the Document tab trigger at 320px, which fails the "Capture and audit" target-size check. The popover now anchors to the bar's bottom edge when the bar has wrapped; a single-row bar keeps the trigger anchor. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
Exact-head personal review outcome: PENDING qualification at fb90604. I reviewed the complete nine-file patch, including the published UI/theme lock changes and the new systemStatusAnchorRect helper/tests. The wrapped header now extends the System status anchor to the bar bottom, addressing the prior second-row overlap; the three geometry cases preserve the single-row fallback. No blocking source defect identified. The previous approval at 8747da4 does not apply to this changed head. Await the exact-head hosted Chromium audit and detailed open-popover resize/enlarged-text qualification before final approval. Owner reports 725 tests and local Chromium141 audit success; Chromium153 local download was blocked and WebKit remains untested. Do not treat those unavailable qualifications as passes. Auto-merge remains off.
0xPlayerOne
left a comment
0xPlayerOne
left a comment
There was a problem hiding this comment.
Exact-head review outcome: SOURCE APPROVED at fb90604. Complete nine-file patch personally reviewed; earlier pending qualification is updated with new evidence: the exact-head hosted Chromium capture/audit and substantive validation/desktop checks now completed without failures. The anchor repair preserves horizontal trigger alignment and clears the wrapped second row; geometry tests and owner local qualification support the behavior. No blocking source defect found. Local Chromium153 reproduction was unavailable and WebKit was not run; neither is represented as passed. Open-popover ancestor-only resize coverage remains a nonblocking follow-up. This does not close the larger #2568 scope or substitute for any required GitHub approval/protection.
#2609) #2599 recorded the header's two-row layout with the keyboard order and the visual order disagreeing: the search and Reflect sat in the bar's centre track, which the shared TopBar moves to a second row below 19rem while the DOM keeps the centre before the trailing group, so the focus dropped to the second row and returned to the first. #2602 left it open as follow-up 1 of 3, owned by @adea-ai/ui and pending a product decision. The header no longer has that structure: #2605 adopted the shared ShellTopBar and moved the search and Reflect into the trailing group, so the bar's DOM order is the leading group and then the trailing group, and wrapping does not reorder it. Measured on the production renderer at 320, 390, 768, 1024 and 1440 px at 100% and 200% text, the order the keyboard walks and the order the bar draws are the same order in every case, including the two-row bar at 320 px with 200% text. What this adds is the coverage that keeps them the same: - scripts/capture-m7-visuals.mjs walks the header's keyboard order at 320 px with 200% text, where the bar wraps, and fails when the order the keyboard walks is not the order the bar draws, when a keyboard stop falls outside the viewport, or when the bar stops wrapping, which would leave the check testing nothing. The wrapped bar is read only after it stops moving: a text-size change reflows the bar and the shell re-reads its own measurements afterwards. - apps/web/src/components/m7/M7ApplicationHeader.test.tsx pins the header's controls to the order the bar draws them, and the search to the trailing group rather than the shared centre slot, which the bar moves to a second row while the DOM keeps it before the trailing group. - docs/verification/issue-2568-topbar-keyboard-order records the finding, the measurement, the negative control and the limits. The check was run against a deliberate reproduction of the finding's cause (the trailing group moved to the front of the DOM, and the rows ordered so the eye still reads the leading group first) and it reported the disagreement it is there to catch. Observed, not changed: on the two-row bar at 320 px with 200% text the trailing group is drawn below the bar's own box and overlaps the page content. That is a layout finding of its own and outside this scope; the keyboard order is unaffected. Checks: bun run format:check, bun run lint, bun run --cwd apps/web typecheck (139 and 21 files, no type errors), bun run test (80 JavaScript test files, passed), cargo check --all-targets, and scripts/capture-m7-visuals.mjs against the production renderer (nord, 320 px, 23 screenshots). Part of #2568. Co-authored-by: Andrew Mahoney-Fernandes <amf@AMF-MB-PRO.local>
Summary
The shell header (
M7ApplicationHeader) kept a local grid override because its centre holds a search field and the Reflect action, which the stockTopBarcentre track was too narrow for.TopBarnow provides that layout ascenterWidth="wide"(adea-ai/ui#446, released in 0.139.0). The header passes the prop, and the local.m7-application-headergrid rule and its class are removed.Part of #2568. Scope is the header only. The four per-source number inputs in
SourceSettingsWorkflow.tsxwere moved toNumberFieldin #2600 (merged). #2586 source-card styling remains deferred.Status of this PR (read first)
This PR is not merged and auto-merge is OFF (none is set). It was marked ready for review by its reviewer at 2026-10-19T00:23Z; that was not done by the owner, and it is still gated by the required hosted checks and independent approval. The reviewer recorded a personal SOURCE APPROVED at head
8747da4, which does not replace those. The packages it pins are published:@adea-ai/ui0.142.4 is released and on npm. Head8747da4pinned them and was qualified against them (receipts below), with one keyboard finding that the reviewer treats as a bounded upstream follow-up. The hosted check "Capture and audit (chromium)" then failed at8747da4; headfb90604repairs it (section below) and awaits fresh review.centerWidth="wide"header change (commita642c09)8b0597d)M7ApplicationShell.tsxplus a regression test inApp.test.tsx. The reviewer accepted it as a bounded consumer repair; that is not full PR approval.@adea-ai/ui0.142.4 (Release Please PR adea-ai/ui#466 merged 23:31:22Z asccc0ceb; GitHub release v0.142.4 published 23:31:40Z; npm published 23:42:32Z, latest, integritysha512-1wB9LNx84gFMoWHiq8wC3a6UXj09MO8MiC5Z5TqpEqRmctvsdixaY0rIQa1kAeEwFgpq5MgiBasATcWcSXz0ww==).8747da4)@adea-ai/ui0.142.4 (was 0.142.0) in the root andapps/webpackage.json, and@adea-ai/themes0.10.10 (was 0.10.6) inapps/web/package.json.bun.lockresolves exactly one copy of each, and both integrity hashes equal the npm registry's.@adea-ai/themes ^0.10.10, which 0.10.10 satisfies. Compared with 0.10.6: the same 34 theme ids (none removed or added) and identical package export names; palette values were not diffed. Cortana imports themes only through@adea-ai/ui. The full suite (78 files), typecheck and build pass on this pair. NOTICE headings follow the installed versions; the published notice texts are unchanged.7d3d786, UI 0.140.0): Tab order matches the single-row visual order, but the search field is 0 px wide at 200% text (390, 320 and 1100 px), which is what the two-row layout fixes. Cause: for a wide bar narrower than 19rem,TopBarmoves its centre torow-start-2while the trailing group stays on row 1, and DOM order is leading, centre, trailing. That is UI package CSS, so this PR does not change it and adds no override; the fix belongs in@adea-ai/ui.| Hosted check "Capture and audit (chromium)" at
8747da4| Failed, run 39180589331, check-run 117832293843: axetarget-sizeon#tabs-cl-19-trigger-documentat nord/320 with System status open (partially obscured, 108 x 11.7 px; 18 px closest offset). Cause: the two-row header is 88 px instead of 48 px, so the popover anchored under the status button opens over the bar's second row (hiding search) and its action button lands on the tab trigger. Repaired infb90604(below). The check is unchanged. |Evidence (commands, raw results, screenshots, scripts):
/mnt/project-files/cortana/pr2599-evidence/qualification-ui-0.142.4/EVIDENCE.md.Qualification at
8747da4(Bun 1.4.3, pinned@adea-ai/ui0.142.4)Re-run on the exact committed head with a clean tree:
bun install --frozen-lockfile: "Checked 365 installs across 542 packages (no changes)".bun run lint,bun run format:check,bun run --cwd apps/web typecheck: pass.bun run test: 78 JavaScript test files, 722 passes, 0 failures. An earlier run before the NOTICE headings were updated failedscripts/shared-notice-contract.test.mjs.bun run --cwd apps/web build: passes. Budgets: initial JS 518,278 / 525,000 (headroom 1.3%), complete JS 897,128 / 940,000, CSS 245,051 / 285,000.?demo; "200% text" ishtml { font-size: 200% }):/favicon.ico404 per load, unrelated to the pin.Popover-anchor repair at
fb90604systemStatusAnchorRect(newsystemStatusAnchor.ts, used byM7ApplicationHeader) anchors the System status popover to the bar's bottom edge when the bar has wrapped to a second row, and keeps the trigger anchor for a single-row bar (desktop placement unchanged). Three unit tests insystemStatusAnchor.test.ts.bun install --frozen-lockfile, lint, format check, typecheck: pass.bun run test: 79 files, 725 passes, 0 failures. Build passes (initial JS 518,495 / 525,000).scripts/capture-m7-visuals.mjs, full default matrix (23 dark themes x 320/768/1024/1440/1920 px): passes (405 screenshots), on a local Chromium 141. The same script fails at8747da4and passes on main7d3d786at nord/320.Shortcut-hint repair at
8b0597dWhat changed: the
Mod+Khint chip in the search field's trailing addon is hidden when navigation is in its mobile layout, and still shown on wide windows. The loading spinner behaves as before. Mobile has no keyboard shortcut to hint at, and the chip competed with the spinner for width at 320px and 200% text. A regression test covers hidden at 320px and shown at 1440px; it fails without the change.Older validation (at
a642c09,centerWidth="wide"change only)7d3d786): lint and format pass; typecheck passes; 78 JavaScript test files passed; build passes with budgets holding.?demo): widths 1600 to 390 had no page or header horizontal overflow, the search field and Reflect stayed visible, tabbing never landed on an offscreen control, and Ctrl+K focused the search field at every width. That pass did not cover 200% text.Impact and rollout
Review notes
minmax(0, 1fr)for the centre below 1280px;TopBarwide usesminmax(7rem, 1fr)so the side groups scroll before the centre collapses. That is the intended UI behaviour, and the measured widths above reflect it.@adea-ai/ui.Related issue
Part of #2568 (stays open).
https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
Generated by Claude Code