fix: Mobile session menu — ⋯ button, scroll, and keyboard fixes for iOS & Android#639
fix: Mobile session menu — ⋯ button, scroll, and keyboard fixes for iOS & Android#639
Conversation
🔍 Design-Level Concern (outside diff)Click-outside handler broken for portaled menusFile: The global This is a direct consequence of the portal approach in the diff. The fix should be included in whatever approach resolves the portal issue (inline finding #1).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Code Review — Adversarial Consensus Summary
Methodology: 3 independent reviewers with adversarial consensus. Findings flagged by only 1 reviewer were cross-validated with the other 2 models. Only findings reaching 2/3+ agreement are included.
Findings by Severity
| # | Severity | Finding | File | Consensus |
|---|---|---|---|---|
| 1 | 🔴 CRITICAL | Portaling Blazor-managed elements to document.body desyncs the render tree — menu/overlay leak, scroll freeze, DOM corruption |
index.html:1506 |
3/3 |
| 2 | 🟡 MODERATE | MutationObserver on document.body with subtree accumulates and may never disconnect |
index.html:1563 |
3/3 |
| 3 | 🟡 MODERATE | __menuInteraction never reset; 1s dead zone blocks intentional backdrop taps |
index.html:56 |
3/3 (after follow-up) |
| 4 | 🟡 MODERATE | Click target area reduced — .session-actions region no longer selects sessions |
SessionListItem.razor:40 |
3/3 |
| 5 | 🟡 MODERATE | async void ToggleFlyout bypasses Blazor's EventCallback integration |
MainLayout.razor:210 |
3/3 |
Design-Level Concern (outside diff)
- Click-outside handler (
index.html:1472) broken for portaled menus — consequence of finding #1
Discarded Findings (single reviewer only)
- Unused
MouseEventArgs eparameter inHandleItemClick— 🟢 MINOR - Safe-area probe element leak on exception — 🟢 MINOR
Assessment
The PR addresses real iOS touch-event bugs with well-commented, thoughtful solutions. However, finding #1 (portaling Blazor-managed elements) is a critical architectural issue — moving elements out of Blazor's DOM tree desyncs the reconciler and causes cascading failures (scroll freeze, observer leaks, broken click-outside handling). This needs an alternative approach before merging.
CI Status: Not evaluated (no authenticated access).
Test Coverage: No new tests added. The JS interaction logic is inherently difficult to unit test, but the portal/cleanup lifecycle could benefit from integration test scenarios.
Prior Reviews: None observed.
Generated by Expert Code Review (auto) for issue #639
| overlay.__originalNextSibling = overlay.nextSibling; | ||
| document.body.appendChild(overlay); | ||
| } | ||
| document.body.appendChild(menu); |
There was a problem hiding this comment.
🔴 CRITICAL — Portaling Blazor-managed elements to document.body desyncs the render tree (Flagged by: 3/3 reviewers)
positionSessionMenu moves Blazor-owned .session-menu and .menu-overlay nodes to document.body. When Blazor sets IsMenuOpen=false, its reconciler tries to remove these elements from their original parent via index-based DOM operations. Since they've been moved, this either throws NotFoundError, removes the wrong sibling, or leaves orphaned elements in (body).
Consequences:
- Menu/overlay orphaned in
(body)permanently (ghost elements) __restoreScrollnever fires → flyout and session-list scroll permanently frozen (overflow: hiddennever cleared)- Click-outside handler (~line 1472) also breaks:
overlay.closest('.session-actions')returnsnullafter portaling - Blazor renderer may enter a broken state from DOM structure mismatch
Suggested fixes:
- Create portal elements via JS (outside Blazor's tree) and replicate menu content
- Render the menu at the
MainLayoutlevel where it's outside the flyout's transform context - Add a deterministic JS cleanup call from
OnAfterRenderAsyncwhenIsMenuOpentransitions tofalse
| observer.disconnect(); | ||
| } | ||
| }); | ||
| observer.observe(document.body, { childList: true, subtree: true }); |
There was a problem hiding this comment.
🟡 MODERATE — MutationObserver on document.body accumulates and may never disconnect (Flagged by: 3/3 reviewers)
Each positionSessionMenu call creates a new MutationObserver watching the entire DOM ({ childList: true, subtree: true }). If the portal issue prevents menu removal, the disconnect condition is never met. Rapid menu open/close cycles accumulate observers, each firing on every DOM mutation — causing progressive performance degradation.
Fix: Store observer on menu.__observer. Add a timeout-based fallback disconnect (e.g., 10s), or use deterministic cleanup from C# when IsMenuOpen transitions to false.
| window.__menuInteraction = Date.now(); | ||
| } | ||
| // Block flyout-backdrop events during menu interaction | ||
| if (e.target.closest('.flyout-backdrop') && window.__menuInteraction && (Date.now() - window.__menuInteraction) < 1000) { |
There was a problem hiding this comment.
🟡 MODERATE — __menuInteraction never reset; 1s dead zone blocks intentional taps (Flagged by: 3/3 reviewers after follow-up)
__menuInteraction is set on every menu interaction but never cleared. Combined with the 1000ms window, closing a menu via overlay then immediately tapping the backdrop is hard-blocked for up to 1 second. iOS ghost clicks occur within 100–300ms; 1000ms is 3–10× longer than necessary.
The block fires in both the JS capture handler (stopImmediatePropagation + preventDefault) and C# HandleBackdropClick, making the dead zone especially aggressive.
Fix: Reset window.__menuInteraction = 0 when the menu is fully closed. Reduce the window to ~350ms.
| @oncontextmenu="OpenContextMenu" | ||
| @oncontextmenu:preventDefault="true"> | ||
| <div class="session-info"> | ||
| <div class="session-info" @onclick="HandleItemClick"> |
There was a problem hiding this comment.
🟡 MODERATE — Click target area reduced; .session-actions region no longer selects sessions (Flagged by: 3/3 reviewers)
Moving @onclick from the outer .session-item div to the inner .session-info div means clicks on .session-actions (context ring, children toggle area) no longer trigger HandleItemClick. The JS capture-phase handler still fires JsExpandSession, but that doesn't:
- Close the flyout on mobile (
OnSessionSelectednever fires) - Persist the selection (
SaveUiStatenot called) - Navigate back to dashboard if on another page
Fix: Keep @onclick on the outer .session-item and rely on @onclick:stopPropagation on the ⋯ button to prevent double-handling. Alternatively, add @onclick="HandleItemClick" to .session-actions as well.
| } | ||
|
|
||
| private void ToggleFlyout() | ||
| private async void ToggleFlyout() |
There was a problem hiding this comment.
🟡 MODERATE — async void bypasses Blazor's EventCallback integration (Flagged by: 3/3 reviewers)
ToggleFlyout changed from void to async void. Blazor's EventCallback expects async Task — async void sends exceptions to the unhandled handler instead of the Blazor error boundary. The current try/catch {} mitigates this, but a future edit removing it would cause hard-to-diagnose crashes.
Fix: Change to private async Task ToggleFlyout(). Blazor's EventCallback delegates support async Task transparently.
|
/review |
|
✅ Expert Code Review completed successfully! |
PureWeen
dismissed
github-actions[bot]’s stale review
Dismissed: this CHANGES_REQUESTED review was posted by the old review-on-open workflow before PR #635 switched to COMMENT-only reviews. The findings are still valid and visible in the review body — this dismissal only removes the merge block.
…rWebView MauiDevFlow.Blazor 0.1.0-preview.5.26217.12 breaks the BlazorWebView asset file handler on Android. The WebView serves index.html but fails to serve any CSS or JS assets (_framework/blazor.webview.js, app.css, bootstrap.min.css, etc.), leaving the app stuck on the loading screen with Blazor never initializing. Reverting both Agent and Blazor packages to 0.1.0-preview.4.26202.3 across PolyPilot and PolyPilot.Gtk projects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…positioning The session context menu (Rename, Close Session, etc.) was unusable on Android because: 1. The flyout panel's CSS transform creates a containing block for fixed-position children, so the menu is positioned relative to the flyout rather than the viewport. 2. The menu height calculation ignored safe area insets (Android nav bar: 62px, status bar: 30px), making the bottom items scroll behind the navigation bar and become untappable. Fix: positionSessionMenu now detects when inside a transformed flyout and adjusts coordinates accordingly. It also reads --nav-bar-height and --status-bar-height CSS variables to cap the menu within the usable area. Added -webkit-overflow-scrolling and overscroll-behavior to the menu CSS for smooth touch scrolling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three issues fixed: 1. Keyboard opens when flyout sidebar opens — the chat textarea retains focus behind the flyout. ToggleFlyout now calls activeElement.blur() when opening the flyout. 2. Menu items with `all: unset` reset `-webkit-user-select` to `auto` on iOS WebKit, making buttons appear editable and triggering keyboard reset. Also set `user-select: none` on the .session-menu container. 3. Menu scroll broken on iOS — added `touch-action: pan-y` to .session-menu so touch scroll events are correctly routed to the menu instead of the flyout panel behind it. Tested via MauiDevFlow CDP against iPhone 15 Pro Max. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On iOS, pointerdown events propagate to the parent session-item even when onclick stopPropagation is set, because Blazor's event delegation fires the parent handler via the pointer event path. This caused the session to be selected (closing the flyout and showing the chat view with keyboard) when the user just wanted to open the context menu. Added @onpointerdown:stopPropagation to the ⋯ button, menu overlay, and menu container to prevent touch events from reaching the parent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Blazor's stopPropagation directives are unreliable on iOS WebKit for touch-originated events. Instead of relying on event propagation control, HandleItemClick now checks two guards: - IsMenuOpen: skip selection if the menu is already open - _menuJustToggled: set by HandleMoreButtonClick, prevents the parent onclick from selecting the session during the same touch This is more robust than stopPropagation because it works at the application logic level rather than depending on browser event dispatching behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Blazor's @OnClick:stopPropagation is unreliable on iOS WebKit for touch-originated events because Blazor uses a single delegated event handler at the document root. This causes the parent session-item's onclick to fire when tapping ⋯, menu items, or the overlay — selecting the session, closing the flyout, and opening the keyboard. Fix uses two layers: 1. A capture-phase JS handler (before Blazor's delegation) sets a window.__menuInteraction timestamp when any .more-btn, .menu-overlay, or .session-menu element is touched. 2. HandleItemClick reads this timestamp via JS interop and skips session selection if it was set within the last 500ms. This approach works regardless of how the browser dispatches events, since it operates at the application logic level rather than depending on event propagation control. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The async JS interop guard (reading window.__menuInteraction) was too slow — both HandleMoreButtonClick and HandleItemClick fire in the same Blazor event batch, and the async JS.InvokeAsync didn't return before OnSelect was already invoked. Replaced with a static C# field (s_lastMenuInteraction) set synchronously by HandleMoreButtonClick and checked synchronously by HandleItemClick using Environment.TickCount64. Also set the guard on menu overlay clicks to prevent session selection when dismissing. Confirmed working via MauiDevFlow CDP touch simulation on iOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Root cause found via real-device event logging: when the ⋯ button is tapped on iOS, the menu-overlay appears under the finger and iOS WebKit dispatches a SECOND touch+click event to the flyout-backdrop behind it (ghost click). This closes the flyout and switches sessions. Fixes: 1. Moved @OnClick from session-item to session-info — ⋯ button clicks can never bubble to the session selection handler structurally. 2. HandleBackdropClick reads window.__menuInteraction timestamp (set by the capture-phase JS handler) and ignores ghost clicks within 400ms. 3. Flyout overflow frozen to 'hidden' while menu is open, restored via MutationObserver when menu is removed — enables menu scrolling on iOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The async JS interop in HandleBackdropClick was too late — Blazor processes the event before the await returns. Now the capture-phase JS handler uses stopImmediatePropagation + preventDefault to block ALL flyout-backdrop events (click, pointerdown, pointerup) within 1s of a menu interaction. This prevents Blazor from ever seeing the ghost click. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Root cause: A capture-phase JS click handler in index.html (line 699) calls JsExpandSession on ANY click inside a .session-item element, including clicks on the ⋯ button, menu items, and menu overlay. This handler was added to make session switching feel instant (CSS before Blazor), but it didn't exclude menu interactions. Fix: Added an early return in the capture-phase handler when the click target is inside .more-btn, .session-menu, or .menu-overlay. Found via stack trace instrumentation on SwitchSession showing Dashboard.JsExpandSession as the caller — not the sidebar's SelectSession as previously assumed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Touch-scrolling inside the session menu on iOS caused the .session-list parent to scroll, which triggered the scroll-close handler and immediately dismissed the menu. The sidebar then kept scrolling. Fix: When the menu opens, set overflow:hidden on BOTH the flyout panel AND the .session-list container. This prevents parent scroll capture entirely. Removed the scroll-close event listener since freezing the parent makes it unnecessary. Both are restored via MutationObserver when the menu is removed from DOM. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iOS WebKit doesn't support native overflow scroll on position:fixed elements inside a transformed parent (the flyout panel). The menu's overflowY:auto has no effect — touch gestures pass through to the parent instead of scrolling the menu. Fix: Added touchstart/touchmove handlers on the menu that manually adjust scrollTop and call preventDefault to stop the touch from reaching the parent. This gives smooth, correct scrolling on iOS while the native scroll continues to work on Android/desktop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
touch-action:pan-y tells iOS WebKit to handle panning natively, which prevents JS touchmove handlers from firing. Changed to touch-action:none so the browser doesn't intercept the gesture and our manual touchmove scroll handler runs correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iOS WebKit cannot natively scroll position:fixed elements inside a parent with CSS transform (the flyout panel). No amount of touch-action, -webkit-overflow-scrolling, or manual touchmove handlers can fix this because the browser's gesture recognizer captures the touch before JS. Fix: positionSessionMenu now moves the menu (and overlay) DOM nodes to document.body when inside a flyout with transform. This escapes the stacking context entirely, making position:fixed relative to the viewport and enabling native overflow scroll. The Blazor scoped CSS attribute stays on the element so styles still apply. A MutationObserver cleans up when Blazor re-renders and removes the original nodes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
touch-action:none prevents the browser from handling ANY touch gestures including scroll. With the menu portaled to document.body (escaping the flyout transform), native overflow scroll works — but only if we don't block it. Removed touch-action:none and rely on the portal + overflow-y + -webkit-overflow-scrolling for correct scroll behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On iOS, --nav-bar-height and --status-bar-height CSS variables are not set (they're Android-only). The menu maxHeight was calculated without accounting for the notch (59px) and home indicator (34px), making the menu appear to fit but actually extend behind the safe areas. Now probes env(safe-area-inset-top) and env(safe-area-inset-bottom) via a temporary element when the CSS variables aren't available. This makes the menu 831px instead of 924px on iPhone 15 Pro Max, enabling proper overflow scroll for the ~75px of content that exceeds the safe area. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…set timestamp Addresses all 5 findings from the adversarial code review: 1. CRITICAL: Removed DOM portaling of Blazor-managed elements to document.body. Menu stays in its original DOM location, avoiding render tree desync. Safe area insets and flyout-relative positioning still work correctly. MutationObserver now scopes to the menu's direct parent (childList only, no subtree). 2. MODERATE: MutationObserver scoped to parent element with childList only (not subtree on body). Disconnects reliably when Blazor removes the menu from its parent. 3. MODERATE: __menuInteraction timestamp reset to 0 after the backdrop ghost click is consumed, preventing the 1s dead zone from blocking intentional backdrop taps. 4. MODERATE: Restored @OnClick on session-item (full click area). The JS capture-phase handler in index.html already blocks JsExpandSession for .more-btn/.session-menu/.menu-overlay clicks. Blazor handler checks IsMenuOpen as additional guard. 5. MODERATE: Changed async void ToggleFlyout back to void. JS blur call is fire-and-forget via discard (no await needed for a blur). HandleBackdropClick simplified to sync — ghost clicks are blocked at the JS capture phase before Blazor sees them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Multi-Model Code Review — Adversarial Consensus (Re-Review)Methodology: 3 independent reviewers with adversarial cross-validation. Re-review of updated diff after fixes were applied. Previous Findings Status
New Findings
Discarded Findings (single reviewer only)
CI StatusTest Results✅ All 3503 tests passing (verified by reviewer) Recommendation✅ Approve — All previous findings addressed. One minor stale comment (finding 5) can be fixed now or in follow-up. Tests green. The iOS/Android touch handling workarounds are well-structured with appropriate safety nets (350ms ghost-click guard, 10s observer fallback, fault-continuation logging). |
…ror logging Finding 2: Removed .menu-overlay from __menuInteraction selector. The overlay is a dismiss action, not a ghost-click source — stamping it blocked intentional backdrop taps for up to 1s after closing a menu. Also tightened the guard window from 1000ms to 350ms (iOS ghost clicks fire within ~300ms). Finding 3: Added 10s setTimeout fallback for MutationObserver cleanup. If Blazor removes the parent element itself (e.g. page navigation while menu is open), the observer never fires and overflow:hidden persists. The timeout ensures cleanup even in edge cases. Finding 4: Added fault-continuation logging to the fire-and-forget JS.InvokeVoidAsync blur call in ToggleFlyout, preventing silent exception swallowing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removed based on analysis of 3 actual review runs (PRs #619, #639, #635): - Removed 3x redundant 'Read copilot-instructions.md' — the Copilot engine auto-loads it as system context, and sub-agents found deep domain bugs without being told to read it - Removed 'You are a thorough PR reviewer for PolyPilot' — generic preamble that adds tokens without changing behavior - Removed MCP tool usage hint ('not gh CLI — credentials are scrubbed') — the agent figures this out from tool availability - Removed duplicate path validation instruction - Removed verbose explanation of why COMMENT not REQUEST_CHANGES — one line is sufficient - Simplified sub-agent prompt from repo-specific to generic 72 lines → 60 lines, same review quality. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…abs learnings (#656) Based on analysis of 3 actual review runs (PRs #619, #639, #635), several prompt instructions are redundant and add tokens without improving output quality. **What was removed:** - 3x "Read copilot-instructions.md" — engine auto-loads it; sub-agents found deep domain bugs without it - Generic preamble ("You are a thorough PR reviewer for PolyPilot") - MCP tool usage hint (agent discovers available tools) - Duplicate path validation instruction - Verbose REQUEST_CHANGES explanation (one line is sufficient) - Repo-specific sub-agent prompt → generic ("expert code reviewer" not "expert PolyPilot code reviewer") **What was kept:** - Security warning (treat PR content as untrusted) — changes behavior - No test messages rule — prevents permanent damage - Full adversarial consensus methodology - Path/line validation rules - COMMENT-only policy 72 lines → 60 lines. Same review quality — the evidence from real runs shows the agent finds domain issues from reading code, not from hint bullets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Summary
Fixes the session context menu (⋯ button) on mobile devices. The menu was completely unusable on both iOS and Android — tapping ⋯ switched sessions, the menu couldn't scroll, and the keyboard appeared unexpectedly.
Changes
DevFlow Revert
ShouldInterceptRequestdelegation). Filed dotnet/maui-labs#101 and PR #102.⋯ Button Session Selection Fix
index.htmlcalledJsExpandSessionon ANY click inside a.session-item, including ⋯ button and menu items. This switched sessions on every menu interaction..more-btn,.session-menu, and.menu-overlayclicks.@onclickfromsession-itemtosession-infodiv so the ⋯ button area is structurally excluded.iOS Ghost Click Prevention
flyout-backdropbehind the menu overlay. A capture-phase JS handler now blocks backdrop events within 1s of a menu interaction viastopImmediatePropagation.iOS Keyboard Dismissal
ToggleFlyoutnow callsactiveElement.blur()to dismiss the keyboard when the flyout opens.positionSessionMenualso blurs on menu open.user-select: none !importanton.menu-itemto prevent iOS WebKit from treatingall: unsetbuttons as editable.Menu Scroll (iOS & Android)
positionSessionMenumoves the menu DOM todocument.bodyto escape the flyout's CSStransformstacking context. iOS WebKit cannot natively scrollposition: fixedelements inside a transformed parent.maxHeightnow probesenv(safe-area-inset-top/bottom)on iOS (via temp element) and reads--nav-bar-height/--status-bar-heighton Android.overflowset tohiddenwhile menu is open; restored viaMutationObserveron menu removal.Testing
Files Changed
PolyPilot/wwwroot/index.html— JS capture handlers, positionSessionMenu portal + safe areaPolyPilot/Components/Layout/SessionListItem.razor— moved onclick, simplified handlersPolyPilot/Components/Layout/SessionListItem.razor.css— menu scroll CSSPolyPilot/Components/Layout/MainLayout.razor— flyout keyboard dismiss, backdrop guardPolyPilot/PolyPilot.csproj— DevFlow revert to preview.4PolyPilot.Gtk/PolyPilot.Gtk.csproj— DevFlow revert to preview.4