Skip to content

fix(ui): paint the phone top safe-area with the header surface - #1393

Merged
BigSimmo merged 10 commits into
mainfrom
claude/white-element-positioning-t607pk
Jul 30, 2026
Merged

fix(ui): paint the phone top safe-area with the header surface#1393
BigSimmo merged 10 commits into
mainfrom
claude/white-element-positioning-t607pk

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • On phones, the white header did not reach the top of the page on any mode except answer — a page-coloured band sat across the status-bar inset above the white bar.
  • Cause: the chrome-safe-area-top spacer in master-search-header.tsx painted bg-[color:var(--background)] (#f1f4f8, the page canvas) while the bar immediately below it paints var(--surface) (#fcfdfe). Answer mode never showed the seam because it is the only mode on the overlay hide strategy: it renders no spacer and pads the inset inside the header itself (pt-[max(0.5rem,var(--safe-area-top))]), so the bar's surface paints straight through to the viewport top. Collapse-strategy modes need the separate spacer so the inset can be released on scroll, and that spacer simply carried the wrong colour.
  • Fix: paint the spacer var(--surface) so it reads as the top of the header at every width and in every colour fallback. Kept opaque deliberately, so the sm+ pinned inset still hides scrolled content.
  • Also documents the rule as part of invariant 6 in docs/search-chrome-behaviour.md and adds a contract assertion so the paint cannot drift back.

Measured in Chromium at 440×956 with the inset injected exactly as tests/ui-phone-scroll.spec.ts does (:root{--safe-area-top:62px !important}), sampling the top-left pixel. The "before" column was reproduced in place on the same element, and it matches the reporter's screenshots pixel-for-pixel:

Route Before After
/?mode=answer rgb(252,253,254) rgb(252,253,254) (unchanged)
/?mode=documents rgb(241,244,248) rgb(252,253,254)
/services rgb(241,244,248) rgb(252,253,254)
/services?q=Community&run=1 rgb(241,244,248) rgb(252,253,254)

The main regression risk was that a white band would be worse than a grey one if it ever failed to collapse, so the release contract (invariant 6) was checked explicitly. On scroll-hide the spacer still collapses to height 0 — with the collapse wrapper (universal-header-collapse) reporting data-scroll-hidden="true"; the spacer itself carries only aria-hidden and its data-testid — and the top pixel returns to page content: rgb(241,244,248) light, rgb(6,7,8) dark. Dark mode paints rgb(16,19,21), matching the dark --surface. Forced-colors is unaffected: --background and --surface both resolve to Canvas.

Verification

  • npm run verify:phone-chrome -- --files src/components/clinical-dashboard/master-search-header.tsx,tests/header-scroll-hide-contract.test.ts,docs/search-chrome-behaviour.md — the prescribed gate for phone-chrome work.

Static/unit contracts:

Test Files  9 passed (9)
     Tests  101 passed (101)

Focused phone/PWA browser journeys:

13 passed (34.7s)
  • npm run verify:ui (run as the gate's escalation stage): 331 passed, 2 failed (11.4m).

The two failures are pre-existing in this container and independent of this change. Established by A/B — the same two specs run twice in the same tree and build pipeline, toggling only the one changed token:

Arm Spacer token Result
A var(--surface) (this PR) 2 failed
B var(--background) (pre-change baseline) 2 failed

Identical in both arms, so the change is not the cause:

  • tests/ui-pwa.spec.ts:143 — manifest/icons/service-worker headers/Chromium installability.
  • tests/ui-smoke.spec.ts:3614 — fails at line 3670, expect(pdfScroller.locator("canvas")).toBeVisible(); the pdf.js canvas never paints.

Neither asserts anything a background-color can reach, and Tailwind bg-* sets no layout property (the element was already opaque, so occlusion is unchanged too). Hosted Advisory UI is green on this head, which is consistent with these being local-environment artefacts.

  • npm run verify:pr-local

Environment note for anyone reproducing: the gate's browser stages need PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=/opt/pw-browsers/chromium. On the default path all 13 focused journeys failed in 1–3 ms with browserType.launch: Executable doesn't exist at /opt/pw-browsers/chromium_headless_shell-1234/... — this container ships headless-shell build 1194 against Playwright 1.62.0's expected 1234. That substituted binary is also the most likely source of the two pdf.js/installability failures above.

Risk and rollout

  • Risk: low. One design-token value on one aria-hidden spacer; no geometry, timing, ownership, or layout change, and no change to the hide/reveal release contract.
  • Rollback: revert this commit — the spacer returns to var(--background).
  • Provider or production effects: None.

Notes

  • Verified in both colour schemes. Not certified on physical Safari or a cold-launched Home Screen PWA — per docs/phone-chrome-physical-acceptance.md, local Chromium cannot certify iOS paint. The change moves the visible band from the page colour to the bar colour and leaves the release-to-zero behaviour untouched, so it does not touch the system-owned-pixel concern in invariant 23, but a physical check is still the honest final word.

Generated by Claude Code

The `chrome-safe-area-top` spacer painted `var(--background)` while the bar
directly below it paints `var(--surface)`, so every collapse-strategy mode
drew a page-coloured status-bar band above the white header on phones.

Answer mode never showed the seam because it is the only mode on the overlay
hide strategy: it renders no spacer and pads the inset inside the header
itself, so the bar's surface paints straight through to the viewport top.
Collapse-strategy modes need the separate spacer so the inset can be released
on scroll, and that spacer simply had the wrong colour.

Measured at 440x956 with the inset injected as tests/ui-phone-scroll.spec.ts
does, top-edge pixel before -> after:

  /?mode=answer                    rgb(252,253,254) -> unchanged
  /?mode=documents                 rgb(241,244,248) -> rgb(252,253,254)
  /services                        rgb(241,244,248) -> rgb(252,253,254)
  /services?q=Community&run=1      rgb(241,244,248) -> rgb(252,253,254)

Kept opaque so the sm+ pinned inset still hides scrolled content, and the
release contract is unchanged: on scroll-hide the spacer still collapses to
height 0 with data-scroll-hidden="true" and the top pixel returns to page
content (light rgb(241,244,248), dark rgb(6,7,8)). Dark mode paints
rgb(16,19,21), matching the dark `--surface`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MYeZh7qEEXc36AX4JuDjzF
@supabase

supabase Bot commented Jul 30, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 7 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8daa9c19-f402-4199-83fe-c42ef077ecaf

📥 Commits

Reviewing files that changed from the base of the PR and between aa4a6cf and 2a1f221.

📒 Files selected for processing (1)
  • docs/branch-review-ledger.md
📝 Walkthrough

Walkthrough

The phone header’s chromeSafeAreaTop spacer now paints --surface instead of --background. Documentation and a contract test record the updated behavior, and the review ledger adds three dated records.

Changes

Phone chrome safe-area contract

Layer / File(s) Summary
Safe-area spacer paint contract
src/components/clinical-dashboard/master-search-header.tsx, docs/search-chrome-behaviour.md, tests/header-scroll-hide-contract.test.ts
The collapse spacer uses --surface; documentation and tests specify and verify the associated hide/reveal behavior.
Review ledger records
docs/branch-review-ledger.md
Three dated records document approval, review findings, and bugbot status for the revision.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: codex

Suggested reviewers: claude, cursoragent

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately describes the main UI change: painting the phone safe-area with the header surface.
Description check ✅ Passed The description follows the template well, covering summary, verification, risk/rollback, and notes with specific details.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@BigSimmo
BigSimmo marked this pull request as ready for review July 30, 2026 02:35
@BigSimmo
BigSimmo enabled auto-merge (squash) July 30, 2026 02:36
@BigSimmo

Copy link
Copy Markdown
Owner Author

@cursoragent review PR and run bugbot

@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

PR #1393 review — no P0/P1

Reviewed db341491 (product tip) on claude/white-element-positioning-t607pk. Ledger was previously NOT REVIEWED; recorded and pushed.

Findings

No high-confidence P0–P2 defects.

Severity Finding Disposition
Spacer bg --background--surface matches phone opaque .edge-glass-header / .universal-header (globals.css ~1535–1538) Correct fix
Overlay answer path never mounts chrome-safe-area-top (hideStrategy === "collapse" gate); inset stays in header pt-[max(0.5rem,…)] Unaffected
Scroll-hide still uses max-sm:h-0; only paint token changed Release contract intact
P3 PR text says spacer collapses with data-scroll-hidden="true" — that attr is on the collapse/overlay host, not the spacer Wording only; behaviour fine
residual At sm+, bar is translucent color-mix(…surface 72%) while spacer is opaque --surface Pre-existing; this PR improves vs page-coloured band

Bugbot

Severity Location Finding Disposition Evidence
No Bugbot findings on head no findings reviewThreads=[], 0 review comments; only cursor[bot] “Taking a look!” ack
Cursor suite Stuck queued, 0 check-runs left open / blocked Suite 82712209672 (old) / 82713985418 (after ledger push); rerequest 403

Local Bugbot-equivalent + frontend-ui review: same verdict — no P0/P1.

Checks

  • npm run test -- tests/header-scroll-hide-contract.test.ts27 passed
  • Hosted: Static/Build/Unit/Advisory/PR policy/CircleCI green; Production UI was still pending on the product tip
  • Not run: verify:phone-chrome browser journeys, verify:ui, physical Safari/PWA (PR already notes this)

Verdict

Safe to merge from a defect standpoint once required CI settles. Hosted Bugbot never produced a report in this environment — local triage covers it. Ledger commit: aa4a6cf7 (docs only).

Open in Web Open in Cursor 

cursoragent and others added 2 commits July 30, 2026 02:47
Ledger append for claude/white-element-positioning-t607pk at
db34149 after local review and
Bugbot triage (no hosted cursor[bot] findings; suite stuck queued).

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
@BigSimmo

Copy link
Copy Markdown
Owner Author

@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers.

@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Result

No product defect to fix. The spacer paint change is sound. The CI “failures” were Production UI cancelled by repeated Merge branch 'main' pushes, which makes PR required fail with production-ui result was cancelled.

Root causes

Issue Root cause Fix needed?
PR required failed on older heads Production UI cancelled when a newer commit superseded the run No code fix — let one head finish
GitHub DIRTY / CONFLICTING Stale-behind main (merge-tree was clean) Synced; now MERGEABLE
Unresolved review comments None (reviewThreads=0) N/A
Bugbot findings None; Cursor suite stays queued (rerequest 403) Local Bugbot-equivalent: clean
Local Playwright (3 fails) Env flake — identical on --surface and --background A/B Not this PR

Proof the product is fine: head e7a27bbf went fully green — Production UI pass + PR required pass.

Changes made

  • Synced main when merge-tree was clean (no real conflict)
  • Ledger only: 2a1f2217 records the babysit/churn finding
  • No change to master-search-header.tsx / product logic

Validation

  • npm run verify:cheap — 4451 passed
  • format:check — pass
  • Phone contracts — 82 passed
  • Hosted green once at e7a27bbf
  • A/B Playwright — same 3 fails both tokens

Remaining blocker

Stop merging main into this PR for ~20 minutes so Production UI can finish on 2a1f2217.

Current state: MERGEABLE, BLOCKED only while CI runs. CI is restarting on the ledger tip. Further main merges will cancel it again and recreate the false PR required failure.

No review threads to resolve.

Open in Web Open in Cursor 

Copy link
Copy Markdown
Owner Author

PR required is failing on branch-sync churn, not on a defect

All three PR required failures have the same shape — production-ui cancelled, every other job success:

CHANGES_RESULT: success   STATIC_RESULT: success   SAFETY_RESULT: success
COVERAGE_RESULT: success  BUILD_RESULT: success    CONTAINER_RESULT: skipped
UI_RESULT: cancelled      DB_RESULT: skipped
##[error]production-ui result was cancelled

ci.yml uses concurrency: group: ${{ github.workflow }}-${{ github.ref }} with cancel-in-progress: true, so any synchronize event kills the in-flight run. Production UI needs ~13 min, and this branch is being re-synced from main at about that cadence:

Time (UTC+8) Event Effect on Production UI
10:58:58 7b34ecf merge main → branch cancelled run on aa4a6cf
11:11:18 bfd764b (#1392) lands on main
11:11:43 e7a27bb merge main → branch cancelled run on 7b34ecf

Earlier, aa4a6cf (the review-ledger push) cancelled the run on db34149 the same way. The job has not yet been allowed to finish once.

This is not fixable by changing code in this PR, and pushing a commit to "fix CI" actively makes it worse — each push is another synchronize that cancels the job again. It is the anti-churn case described in AGENTS.md → "Open PR branch sync (anti-churn)".

What actually clears it: let the branch sit un-synced long enough for production-ui to run to completion (~15 min), and auto-merge takes it. If main keeps advancing faster than that, hold / skip-branch-sync on this PR stops the auto-sync so the job can finish.

For anyone auditing the diff itself: it is a single design-token value (--background--surface) on one aria-hidden spacer, plus a doc line and a contract assertion. verify:phone-chrome static/unit contracts are 101/101 and its focused phone/PWA journeys 13/13. The two verify:ui failures noted in the description were A/B-proved independent of the change — identical 2-failed results with the token set both ways — and hosted Advisory UI is green on this head.


Generated by Claude Code

@BigSimmo

Copy link
Copy Markdown
Owner Author

@cursoragent fix merge conflict

@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

No merge conflict to fix.

GitHub reports mergeable: true / MERGEABLE. Local git merge-tree origin/main…HEAD is clean, and the branch is 0 behind main.

BLOCKED is only from CI still running (Production UI / Static / Unit / Build pending) — not a content conflict. Pushing or merging main again would cancel Production UI and recreate the false PR required failure you already diagnosed.

Leave the branch alone until that run finishes (~15 min).

Open in Web Open in Cursor 

cursoragent and others added 6 commits July 30, 2026 03:30
Hosted CI green on e7a27bb (Production UI + PR required). Prior
PR-required failures were Production UI cancelled by superseding
main merges, not product defects. No Bugbot findings; no unresolved
review threads. Local A/B confirmed Playwright env flakes unrelated
to the spacer paint token.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Sync one commit from main (#1394) so the PR is not stale. git
merge-tree was clean; GitHub DIRTY was behind-by-1 staleness.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Product change is sound (hosted green at e7a27bb). Remaining merge
blocker is repeated main merges cancelling Production UI before
PR required can pass. No product code change in this commit.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
@BigSimmo
BigSimmo disabled auto-merge July 30, 2026 04:06
@BigSimmo
BigSimmo merged commit b73299a into main Jul 30, 2026
19 checks passed
@BigSimmo
BigSimmo deleted the claude/white-element-positioning-t607pk branch July 30, 2026 04:11
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.

3 participants