Fix LogViewer mobile scrolling via touch fallback#164
Conversation
🤖 My Senior Dev — Analysis Complete👤 For @khaliqgant📁 Expert in View your contributor analytics → 📊 1 file reviewed • 1 high risk • 4 need attention 🚨 High Risk:
🚀 Open Interactive Review →The full interface unlocks features not available in GitHub:
💬 Chat here: 📖 View all 12 personas & slash commandsYou can interact with me by mentioning In PR comments or on any line of code:
Slash commands:
AI Personas (mention to get their perspective):
For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews. |
There was a problem hiding this comment.
Pull request overview
This PR fixes mobile touch scrolling in the LogViewer component by implementing a touchmove-based fallback for coarse pointer devices. The changes ensure proper scroll behavior on mobile devices by binding touch handlers directly to the xterm viewport and preventing default scrolling when the viewport has scrollable content.
Changes:
- Added terminal readiness state to ensure viewport exists before binding touch handlers
- Modified touch event listeners to attach directly to viewport instead of container
- Added preventDefault logic to prevent default scrolling when viewport can scroll
- Extended pointer-events CSS override to helper textarea
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| container.addEventListener('touchstart', handleTouchStart, { passive: true }); | ||
| container.addEventListener('touchmove', handleTouchMove, { passive: true }); | ||
| viewport.addEventListener('touchstart', handleTouchStart, { passive: true }); | ||
| viewport.addEventListener('touchmove', handleTouchMove, { passive: false }); |
There was a problem hiding this comment.
The touchmove event listener is registered with passive: false, which is necessary for preventDefault() to work. However, the preventDefault() call inside handleTouchMove is conditional. If viewport.scrollHeight > viewport.clientHeight is false, the event will not be prevented, but the browser will still treat it as non-passive, which can impact scroll performance. Consider restructuring to only register as non-passive when necessary, or ensure the conditional logic is correct to avoid unnecessary performance impact.
| @@ -377,6 +384,7 @@ export function XTermLogViewer({ | |||
| max-height: 100%; | |||
| touch-action: pan-y !important; | |||
| overscroll-behavior: contain; | |||
There was a problem hiding this comment.
The addition of pointer-events: auto to the helper textarea lacks a comment explaining its purpose. Given that this is part of a mobile scrolling fix, it would be helpful to document why this override is necessary and how it relates to the touch event handling changes.
| overscroll-behavior: contain; | |
| overscroll-behavior: contain; | |
| /* Allow the viewport to receive pointer events (selection, wheel/mouse) by default. | |
| On coarse pointers (mobile Safari), child elements have pointer-events disabled | |
| in the media query below so vertical pan gestures can reach this scrollable area. */ |
Summary\n- add touchmove-based scroll fallback on xterm viewport for coarse pointers\n- prevent default scrolling when viewport can scroll\n- ensure viewport exists before binding touch handlers\n- extend pointer-events override to helper textarea\n\n## Testing\n- not run