fix(android): keep the server switcher centred over the chat column, clear of header controls - #3589
fix(android): keep the server switcher centred over the chat column, clear of header controls#3589btli wants to merge 39 commits into
Conversation
64c4987 to
3820073
Compare
|
Withdrawing — this isn't the right fix for the picker overlap; a different approach is coming. |
|
This is the different approach promised when the PR was withdrawn — suggesting reopen. Pushed 3 commits:
Verification: web 4,807 green; Android 55/55 on this branch's base. Recommended merge order — this PR is one of seven fixed and verified together; the fully integrated reference (all cross-PR conflicts resolved, 5,003 web tests + 273/273 Android tests green) is Android track (suggest landing first — security fixes, and the later two resolve against it):
Web track (file-disjoint from the Android track; order within it matters): |
c65fb81 to
ce1ff7c
Compare
On a tablet or unfolded foldable the right Workspace rail rendered under the status bar, leaving its tab icons un-tappable, and ran under the gesture-nav bar at the bottom. The rail is `md:m-2` and only renders at md+, but every native-shell inset rule lives inside `@media (width < 48rem)` — so none of them reach it — and it is absent from the shared panel selector lists on both the CSS and Android-injected sides. Add the safe-area margins outside the width gate, and mirror the same declaration into the Android injected sheet so shells pointed at an older web build get the fix too. Uses --omnigent-safe-*, not --omnigent-inset-*: the latter folds in the native bottom-bar footprint and would double-count. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The panel docks as a full-height rail at md+ but was missing from both inset selector lists, so it had no safe-area padding at any width. It is reachable only in debug mode, which is why it went unnoticed. Pre-existing gap, adjacent to the Workspace rail fix rather than part of it. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Signed-off-by: Bryan Li <bryan.li@gmail.com>
A media-gated override appearing later in index.css with the same selector previously slipped past the ancestry check, which only inspected the first matching rule. Assert over every rule that sets the Workspace rail's margins so a later override at md+ fails the suite instead of silently winning the cascade. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The safe-area assertion read only the first matching rule, so a second top-level rule could reintroduce the composite inset vars — double counting the native bottom bar — while the suite stayed green. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Only the longhand properties were matched, so a shorthand `margin` declaration set both edges without ever being checked for an enclosing at-rule. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Matching the exact selector text let an equivalently-spelled one through, and checking only the first rule let a later override zero the margins without naming a banned variable. Match on the rail's aria-label and require the margin rule to be unique. Signed-off-by: Bryan Li <bryan.li@gmail.com>
margin-block and its start/end forms set the same edges as the rule being guarded, so an override written that way was never matched. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The rule scan keyed on one literal spelling, so the same selector written with single quotes, no quotes, or extra spacing introduced a margin the uniqueness and safe-area checks never saw. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The scan matched any spelling of the rail's attribute selector, but the ancestry check then re-found one exact literal and skipped the rule when it differed — two sources of truth for the same question, and a rule whose nesting was never actually checked. Signed-off-by: Bryan Li <bryan.li@gmail.com>
…tors The ancestry walk built named at-rule records that no assertion ever read — every use compared them to an empty list. Brace depth answers the same question in a third of the lines and is stricter: it also rejects the rule being nested inside a plain rule, which the ancestor form let through. Also stops `scroll-margin-top` and friends registering as margin overrides. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The rail now pads content clear of the OS bars on all four edges instead of shrinking with top/bottom margins, so its background and divider stay flush. Lateral insets are folded into --omnigent-safe-left and -right and the published insets include the display cutout, which systemBars() alone omits. The injected fallback sheet mirrors the same rules. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
The injected sheet is interpolated into a JS template literal, so a backtick or dollar-brace in the CSS breaks the script at runtime while the style-tag tests keep passing. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
7201b6d to
18b9608
Compare
|
This PR is part of a stacked train rebased onto current main so the whole set merges conflict-free in order. Full order: #3985 → #4065 → #4060 → #4057 (web/Sidebar train) and #3587 → #3589 → #3800 (android train). The two trains are independent of each other. This PR is 2nd in the android train, stacked on #3587. Until its predecessors merge, this PR's diff temporarily includes their commits; it collapses to a clean, self-contained diff automatically as each predecessor lands. |
The chat header's right edge clears the inline workspace rail via --workspace-panel-offset. The +16 covered the rail's old outer margins; the rail is flush now, so the offset is the rail width itself. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
…bars Scoping the injected panel inset rule to phone widths also stripped the conversations sidebar's insets at md+, where it is a pinned column at the left screen edge — unfolded, its header rendered under the status bar. Pad its exposed edges (top/bottom/left) at md+ in index.css and mirror the rule in the injected fallback sheet. Android-only: iOS md+ keeps its current layout. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The switcher is a native view stacked above the web view, so wherever it sits it swallows taps. Centred on the whole window it lands on the conversations rail's search/settings/collapse controls whenever the rail's width, the window's width and the hostname's length line up — and always at the rail's maximum width, where those controls end just left of centre. The web layer now publishes the chat column's extent and native centres the pill inside it, so no overlap is possible by construction. Hiding the pill while the rail is open was rejected: the rail is open by default at md+, and the pill is the only way into the server menu. Published as fractions of the viewport, so a band applied after a rotation or fold cannot push the pill off-screen, and cleared on every navigation so a band cannot outlive the page that sent it. With no message the pill keeps its whole-window centring, leaving older web builds as they were. Also caps the pill's width to the iOS ceiling so a long hostname cannot sweep it across the page. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Relative START gravity was paired with an absolute leftMargin. Under an RTL locale START resolves to RIGHT, and FrameLayout positions a right-gravity child from the right edge using rightMargin — the computed leftMargin is ignored entirely and the pill snaps flush to the physical right edge, over the workspace rail's tabs. The band is derived from getBoundingClientRect, whose coordinates are physical and do not mirror, so the margin it produces needs absolute gravity. Resolve the fraction against the pill's parent too, which is the view its margin is measured within. Also makes the frame-coalescing test meaningful: it flushed no frame before dispatching, so the mount frame satisfied the assertion and the test passed with both listeners removed. Signed-off-by: Bryan Li <bryan.li@gmail.com>
onConfigurationChanged received the new Configuration but re-read resources.configuration instead, which has not necessarily picked up the change yet. A light/dark switch could therefore leave the status and navigation bar icons at the previous polarity — unreadable against the new background until something else triggered a re-apply. Thread the supplied config through, keeping the resources value as the default for the initial call. Fixes a long-standing failure in the existing polarity test, which had been passing a config that was being ignored. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Three problems, all of which let the pill go back to swallowing taps meant for the rails. The pill was centred on the published band but only ever clamped to the screen, so a band narrower than the pill let it spill out both sides. A 768dp-wide window reaches that: the conversation rail's default 320 plus the workspace rail's 420 floor leave the chat column around 28px. Clamp the position to the band and bound the pill's width by it, letting a narrow band override the minimum tap target — covering a rail's controls is worse than a small pill. A collapsed chat column also froze the band. The publisher skips a zero-width column, but a ResizeObserver watches size and not position, so widening the conversation rail behind a push panel never republished and the stale band drifted onto its controls. Observe the content region too and fall back to it, which is always clear of that rail. Finally the publisher gave up for good when the bridge arrived late: WebViews without document-start injection install the facade after React mounts, and the effect had already returned without observers. Test for the shell when publishing rather than when subscribing. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Keeping the pill inside its band was allowed to override the minimum tap target, which inverts the problem it solved: a chat column a few pixels wide produced a pill of the same size, and that pill is the only way into the server menu. A one-pixel column also slipped past the zero-width check that hands over to the content region. Require a usably wide column before publishing it, and hold the pill at the recovery floor regardless of the band. Where a band is narrower than that floor the pill now overflows it slightly, which is the better trade: a small overlap still leaves every control reachable, an invisible pill does not. The layout assertions cover the pill's resolved bounds after a real traversal, since an unchanged frame skips onLayout and leaves stale positions behind. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Nothing covered the exact width at which the publisher hands over to the content region, so both the threshold value and its comparison could drift unnoticed. Signed-off-by: Bryan Li <bryan.li@gmail.com>
Anchoring the pill at the band's left edge when it cannot fit had no guard against the parent's width, so a narrow band hard against the right edge pushed part of the pill outside the parent and the framework clipped it — leaving a target smaller than the floor the pill is held at. A fold reaches this without any bad input: the published fractions are retained across the configuration change, and a band that was comfortably wide before resolves to a sliver at the new width until the page republishes. Clamp into the parent so such a band overflows to the left instead, and leave a band that already fits untouched. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The pill was repositioned from its own layout callback and from band updates, and neither fires when a retained Activity is resized without the published band changing: a left-gravity child keeps its bounds, so its callback stays silent, and an identical band was discarded before it could trigger anything. Expanding a folded window therefore left the pill sized and placed for the old width, over whatever now sits beneath it. Watch the parent for layout changes as well, and stop treating an equal band as nothing to do — positioning already skips writing when neither the gravity nor the margin actually changes. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The field was declared as a plain View while only ever holding a TextView, so every place needing the text or width bounds re-tested the type and silently did nothing if the cast failed — a branch that could never run but still had to be read as if it could. Also records why the frame scheduler keeps a separate pending flag: collapsing the two looks obvious and breaks re-entrant callbacks, since the handle is only assigned after the callback may already have run. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The accessible description has to match the middle-ellipsized text, and
that pairing was spelled out at both the construction and reconnect
sites; the dp width conversion was likewise written once for setup and
again for the runtime path. Fold each into the one place that owns it,
so construction now goes through the same bounds helper a published band
does.
Both bridge callbacks had `= {}` defaults no caller used, which would
turn a forgotten argument into a silent no-op instead of a compile
error. Also corrects the floor comment: it bounds the pill's width, not
its touch target, whose height is still whatever the text measures.
Signed-off-by: Bryan Li <bryan.li@gmail.com>
…rent CI's ktlint (which runs for real there, unlike the local no-op wrapper) rejects the long nine-underscore lambda lines. One named listener reads better than two exploded lambdas and registers identically on both views. Signed-off-by: Bryan Li <bryan.li@gmail.com>
The pill now positions inside the chat column band minus a 48dp control reserve at each edge, hides entirely (web-driven setServerSwitcherHidden or an unusable band) instead of borrowing an adjacent surface, and the web hook clamps the published fractions to the viewport and hides on overlay/collapse instead of falling back to the content region. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
The hardcoded pixel expectations assumed the label always measures past the width cap; under Robolectric it doesn't, so anchor the asserts to button.width via serverSwitcherLeftMargin with the 48px reserve, and add a third layout pass where the container-driven cap change makes the shared listener reposition once more. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
…band The symmetric 0.4-0.6 case centres to 448 with or without the 48px reserve, so it can't catch a dropped reserve; a 0.4-0.56 band anchors at the reserved edge (448) but centres to 428 without it. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
… overlay The band hook ran on iOS too, where its publish() ends in setServerSwitcherHidden(false) and fought the iOS frontmost-surface visibility owner (ChatPage / useNativeServerSwitcherForMainSurface) on every resize or observer tick. Gate it to the Android shell, where it is now the sole visibility owner. On Android the obscured signal was a render-time 'sidebarOpen && isMobileViewport()' snapshot that never re-evaluated on viewport changes and missed the full-screen drawers (files / agents / shells / tasks / logs / file viewer) and the maximized rail. Replace it with the same frontmost-surface tracking iOS uses — generalized with a shell predicate and a native-ready re-check for the late-attaching Android bridge — so any surface covering the chat column hides the switcher and a band republish can never re-show it over an overlay. Also drop the dead onNativeInsets subscription (Android never emits insets to the web facade), the dead column guard under the observer, the single-call-site serverSwitcherTopMarginUpdate wrapper, and the doubled band/containerWidth guard; share one dp->px conversion for the control reserve so the fit test and margin math can't drift. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
… e2e Publish narrow bands as-is — the shell's control reserve + minimum pill width already decide "too small to fit", so the 64px web threshold was a second copy of that policy that could drift. Gate the hook's listeners behind shell readiness so plain browsers don't carry a dead ResizeObserver, and drop the two-flag rAF bookkeeping for the sibling hook's cancel-and-replace pattern. Add a Playwright test driving the real chain: bridge -> band publication -> drawer open hides the pill -> dismiss re-shows it. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
Both hooks re-check the shell after the bridge's page-finished fallback attaches; one useShellReady helper keeps the readiness rule from drifting between placement and visibility. Co-authored-by: Isaac Signed-off-by: Bryan Li <bryan.li@gmail.com>
18b9608 to
1f92ccf
Compare
Signed-off-by: Bryan Li <bryan.li@gmail.com>
Signed-off-by: Bryan Li <bryan.li@gmail.com>
|
Closed. If you want to pick this back up, comment |
|
Closing this in favour of the pattern that landed in #4551. Recording why, plus an audit of where each shell's picker actually stands on Why the band approach is supersededThe mechanism proposed here — the web publishing a free horizontal band as viewport fractions, native centring the pill inside it minus a 48dp control reserve — is not in the tree. A current-tree search for #4551 (merged as That is the better trade. The band design's whole cost centre was keeping two coordinate systems in sync across rotation, fold, rail drags, and old-bundle compatibility. Docking deletes the coupling rather than validating it. Where each shell stands todayDesktop (Electron) — fixed by #4551.
Android — still exactly the state this issue describes. #4551 touched zero Android files.
iOS — same shape as Android, but with a visibility escape hatch.
What following #4551 means for mobileThe desktop fix isn't directly portable as-is — two gaps:
Net: the resolution is (1) port the picker IPC to the native bridges, (2) render the sidebar picker on mobile, and (3) then hide the native pill — the objection to hiding it evaporates once a web-side picker exists as the recovery path, and hiding needs only the boolean Leaving this closed — say the word if you'd rather it stay open as the tracking issue for the mobile half, or I can file a fresh issue scoped to the three steps above. |
Related issue
Closes #3586
Summary
The Android server-picker pill is a native
TextViewstacked above the WebView, so wherever it sits it swallows touches. Laid outGravity.TOP or CENTER_HORIZONTALit centres on the whole window with no knowledge of the web layout beneath it, and lands on the conversations rail's Search / Settings / Collapse controls.With rail width
S, window widthWand half-pill-widthp, the controls spanx ∈ [S-88, S-8]and the pillx ∈ [W/2-p, W/2+p], so they collide iffW/2 - p + 8 < S < W/2 + p + 88:10.0.2.2:8000(p≈50dp)342 < S < 906omnigent.joyful.house(p≈77dp)315 < S < 933S = W/2the controls end atW/2-8Vertically they always overlap, so horizontal position is the only variable — hence "only when the resolution is just right".
The fix: the web publishes the free horizontal band and native centres the pill inside it.
<main>, between the two rails).or the content region when that column is too narrow to host a usable pill(revised: a too-narrow or obscured column now hides the pill instead of borrowing the content region — see Revision below).Why not hide the pill while the rail is open (the obvious cheaper fix): the rail is open by default at ≥768px, and
MainActivity.ktdocuments the pill as the always-available recovery path — the only entry toshowServerSwitcherMenu. Hiding it would remove the recovery control by default on every tablet. Two independent reviewers ranked re-centring first and hiding last for that reason. (Revised: blanket hiding is still rejected for that reason, but the follow-up commits hide it in the narrow cases where showing it is worse — see Revision below.)Also included: the pill's width is capped to the iOS ceiling with middle-ellipsis (the full host stays in the content description and the menu), and a separate, independently droppable commit fixes a pre-existing bug where
onConfigurationChangeddiscarded theConfigurationit was handed, so status/nav bar icon polarity could lag a light/dark switch.ELI5: the picker is a native button floating over the web page. It sat in the middle of the screen — which is where the sidebar's buttons are once the sidebar is wide enough — and being a native button it ate the taps. Now the page tells it where the free space is.
Revision (Aug 5) — adversarial-review follow-ups
Three commits (
fed7a2cf,5f77e9d5,b7ae1a6c) extend the band mechanism after review found the pill could still cover the chat header's own find / settings / collapse controls (the band cleared the rails, but the header overlay lives inside the chat column):setServerSwitcherHidden) instead of borrowing an adjacent surface whose own controls it would then cover. Whole-window centring for old bundles is unchanged.Test Plan
./gradlew testDebugUnitTest --rerun-tasks— 47 tests, 0 failures. Covers the band arithmetic (centring, inward rounding, containment, clamping at both parent edges, pill wider than band, pill wider than container), the bridge dispatch validation (missing / wrong-type / non-finite / out-of-range / reversed fractions), navigation-start reset, RTL placement, width bounds, and the parent-resize and equal-band repositioning paths.npx vitest run src/shell/AppShell.test.tsx src/lib/nativeBridge.test.ts— 141 passed. Covers band source selection and its 63/64px boundary, the off-shell no-op, republication on observed resizes, animation-frame coalescing per event source, observer cleanup, and publication after a late bridge install.Demo
Recorded on an Android emulator at 800×1280 @ 160dpi (an 800dp-wide viewport, i.e. the md+ layout with the conversations rail docked and open by default), connected to a local server.
mainAPK)Before: at the default rail width the window-centred pill just misses the rail's Search / Settings / Collapse controls; dragging the rail wider slides them underneath it, and tapping Settings opens the native server-switcher menu instead — the tap never reaches the web UI. After: the pill centres over the chat column, visibly re-centres live while the rail is dragged wider, and the same tap opens Settings. Full-resolution MP4s on the asset branch.
The "after" capture also exercises the real web→native round trip: the served bundle publishes the band and the native pill repositions from it, while the "before" APK against the same bundle ignores the band — confirming old-APK compatibility in the other direction.
Type of change
Test coverage
Coverage notes
The geometry, the bridge validation and the web publisher are unit-tested, and every fix is mutation-checked as described above.
The web↔native round trip is now also exercised live on an emulator (see Demo): the served bundle publishes the band, the native pill repositions from it, and the touch handoff before/after is observed directly. Not yet verified on physical foldable hardware (fold/unfold configuration changes in particular).
Two deliberate scope decisions, called out so a reviewer can overrule them:
Adversarial review of this change also surfaced four pre-existing defects in the Android shell, none of them regressions from this PR and none touched by it — filed separately rather than folded in: #3730 (renderer death terminates the app), #3731 (the pill's touch target is ~27dp tall), #3732 (a Display-size change leaves the pill's metrics at the old density), #3733 (the hardcoded 25px switcher height desyncs the scroll-fade at large font sizes). #3731 and #3733 are worth reading alongside this PR since they concern the same view: this change floors the pill's width at 48dp, which is a different axis from #3731's height problem, and adds
isSingleLine, which improves #3733's constant without making it correct.Changelog
The Android server picker no longer covers the conversation rail's search, settings and collapse buttons