Conversation
- Add isPhoneDevice() utility that blocks phones but allows tablets - Create useDevice hook with breakpoint detection (mobile/tablet/desktop) - Create useLongPress hook for touch context menu support - Export new hooks from packages/ui
- Auto-collapse sidebar when on tablet breakpoint - Disable split view (right container) on tablets - Force all opens to left container on tablet - Add touch-friendly resize handles (12px width) - Add tablet layout tokens to spacing config
Update touch targets to 44px minimum for tablet/touch devices. Add touch-friendly resize handles and editor action buttons. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Integrate react-dnd-touch-backend with automatic detection. Uses 200ms delay to prevent accidental drags during scroll. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clarify that tablets are supported, only phones are blocked. Update messaging to reflect tablet support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds tablet responsive support to enable the application to work on tablets while still blocking phones. The changes introduce device detection hooks, tablet-optimized layouts with single-pane views, touch-friendly UI elements, and touch-based drag-and-drop support.
Key changes include:
- Device detection system to differentiate phones from tablets using viewport breakpoints (640px/1024px)
- Touch-optimized UI with 44px minimum touch targets and larger resize handles (12px vs 3px)
- Touch backend integration for drag-and-drop operations with 200ms delay to prevent accidental drags
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/hooks/use-device.tsx | New hook using useSyncExternalStore for responsive device detection based on viewport width and touch capability |
| packages/ui/src/hooks/use-long-press.tsx | New hook for handling long press interactions on touch devices |
| packages/ui/src/hooks/use-layout-state.tsx | Updated to force single-pane layout on tablets by always using left container |
| packages/ui/src/lib/dnd-backend.ts | Modified to dynamically select TouchBackend for touch devices vs HTML5Backend for desktop |
| packages/ui/src/lib/spacing.ts | Added tablet-specific spacing constants for touch targets and icon bar width |
| packages/ui/src/components/layouts/layout.tsx | Updated with touch-friendly resize handles and auto-collapse sidebar behavior for tablets |
| packages/ui/src/components/layouts/sidebars/sidebar-icon-bar.tsx | Increased button sizes to 44px (size-11) on tablets/touch devices for accessibility |
| packages/ui/src/components/layouts/sidebars/right-sidebar.tsx | Updated icon bar width and button sizes for touch devices |
| packages/ui/src/components/databases/tables/table-view-field-header.tsx | Enlarged resize handles (24px) and made them visible on touch devices |
| packages/ui/src/editor/menus/action-menu.tsx | Increased icon sizes and padding for touch interactions |
| apps/web/src/lib/utils.ts | Refactored device detection to distinguish phones from tablets, allowing tablets through |
| apps/web/src/main.tsx | Updated to use isPhoneDevice instead of isMobileDevice for more accurate blocking |
| apps/web/src/components/mobile-not-supported.tsx | Updated messaging to clarify only phones are blocked, not tablets |
| packages/ui/package.json | Added react-dnd-touch-backend dependency for touch-based drag-and-drop |
| packages/ui/src/hooks/index.ts | Exported new useDevice and useLongPress hooks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isTablet && !sidebarMetadata.collapsed) { | ||
| handleSidebarToggle(); | ||
| } | ||
| }, [isTablet]); |
There was a problem hiding this comment.
The useEffect has a missing dependency on handleSidebarToggle. When handleSidebarToggle changes, this effect won't re-run, which could lead to stale closures. The effect should either include handleSidebarToggle in its dependency array, or should be structured to avoid the dependency (e.g., by checking sidebar state and calling toggle conditionally).
| }, [isTablet]); | |
| }, [isTablet, sidebarMetadata.collapsed, handleSidebarToggle]); |
| export const HTML5Backend = (...args: unknown[]) => { | ||
| if (isTouch()) { | ||
| // @ts-expect-error - TouchBackend args | ||
| return TouchBackend(args[0], args[1], { delayTouchStart: 200 }); |
There was a problem hiding this comment.
The TouchBackend is not being wrapped with the ProseMirror event filtering logic that the HTML5Backend receives via wrapBackendListeners. This means drag-and-drop operations on touch devices won't properly ignore ProseMirror editor events, potentially causing the same issues the HTML5Backend wrapper was designed to fix. Consider wrapping the TouchBackend instance with the same listener filtering.
| return TouchBackend(args[0], args[1], { delayTouchStart: 200 }); | |
| return wrapBackendListeners(TouchBackend(args[0], args[1], { delayTouchStart: 200 })); |
| export const HTML5Backend = (...args: unknown[]) => { | ||
| if (isTouch()) { | ||
| // @ts-expect-error - TouchBackend args | ||
| return TouchBackend(args[0], args[1], { delayTouchStart: 200 }); |
There was a problem hiding this comment.
The TouchBackend instantiation appears incorrect. The TouchBackend constructor from react-dnd-touch-backend expects the manager as the first argument and options as the second argument. This code is passing args[0], args[1], and then options as a third argument, which doesn't match the expected signature. The correct pattern should be: new TouchBackend(args[0], { ...args[1], delayTouchStart: 200 }) where args[1] should be merged with the new options.
| return TouchBackend(args[0], args[1], { delayTouchStart: 200 }); | |
| return new TouchBackend(args[0], { | |
| ...(args[1] as object), | |
| delayTouchStart: 200, | |
| }); |
| onTouchEnd: () => { | ||
| if (timeoutRef.current) { | ||
| window.clearTimeout(timeoutRef.current); | ||
| } | ||
| setLongPressed(false); | ||
| }, |
There was a problem hiding this comment.
The onTouchEnd handler doesn't call onClick when appropriate, unlike the onMouseUp handler which calls clear(e, true) to trigger the onClick callback if the long press wasn't activated. This creates an inconsistent behavior where touch users cannot trigger regular clicks, only long presses. The handler should check if the press was not long and call onClick if provided.
| function subscribe(callback: () => void): () => void { | ||
| const handleResize = () => { | ||
| cachedDeviceInfo = null; | ||
| callback(); | ||
| }; | ||
| window.addEventListener('resize', handleResize); | ||
| return () => window.removeEventListener('resize', handleResize); |
There was a problem hiding this comment.
Both useDevice and useWindowSize hooks subscribe to the window resize event independently. This creates redundant event listeners that could impact performance, especially during rapid window resizing. Consider consolidating these into a single hook or having useDevice leverage useWindowSize to avoid duplicate resize listeners.
| function subscribe(callback: () => void): () => void { | |
| const handleResize = () => { | |
| cachedDeviceInfo = null; | |
| callback(); | |
| }; | |
| window.addEventListener('resize', handleResize); | |
| return () => window.removeEventListener('resize', handleResize); | |
| const subscribers = new Set<() => void>(); | |
| let isListeningToResize = false; | |
| function handleResize() { | |
| cachedDeviceInfo = null; | |
| subscribers.forEach((callback) => { | |
| callback(); | |
| }); | |
| } | |
| function subscribe(callback: () => void): () => void { | |
| subscribers.add(callback); | |
| if (typeof window !== 'undefined' && !isListeningToResize) { | |
| window.addEventListener('resize', handleResize); | |
| isListeningToResize = true; | |
| } | |
| return () => { | |
| subscribers.delete(callback); | |
| if (typeof window !== 'undefined' && isListeningToResize && subscribers.size === 0) { | |
| window.removeEventListener('resize', handleResize); | |
| isListeningToResize = false; | |
| } | |
| }; |
| const phoneDeviceRegex = /iPhone|iPod|Opera Mini|IEMobile|WPDesktop/i; | ||
|
|
||
| export const isMobileDevice = (): boolean => { | ||
| return mobileDeviceRegex.test(navigator.userAgent); | ||
| // Android phone detection (excludes tablets) | ||
| const isAndroidPhone = (): boolean => { | ||
| const ua = navigator.userAgent; | ||
| // Android tablets typically have "Tablet" or larger screen identifiers | ||
| // Android phones have "Mobile" in the user agent | ||
| return /Android/i.test(ua) && /Mobile/i.test(ua); | ||
| }; | ||
|
|
||
| export const isPhoneDevice = (): boolean => { | ||
| return phoneDeviceRegex.test(navigator.userAgent) || isAndroidPhone(); |
There was a problem hiding this comment.
The phone detection logic removed "iPad" from the regex, but modern iPads (iPadOS 13+) report their user agent as "Macintosh" when requesting desktop sites, making them indistinguishable from actual desktops via user agent alone. While this change allows tablets through, it may inadvertently allow iPads that could benefit from tablet-specific UX. Consider adding detection for iPads using feature detection (e.g., checking for touch support combined with large screen size) or relying solely on the viewport-based breakpoint detection in the useDevice hook.
| export function useLongPress({ | ||
| onLongPress, | ||
| onClick, | ||
| delay = 500, | ||
| }: UseLongPressOptions) { | ||
| const [longPressed, setLongPressed] = useState(false); | ||
| const timeoutRef = useRef<number | null>(null); | ||
| const targetRef = useRef<EventTarget | null>(null); | ||
|
|
||
| const start = useCallback( | ||
| (event: React.TouchEvent | React.MouseEvent) => { | ||
| targetRef.current = event.target; | ||
| timeoutRef.current = window.setTimeout(() => { | ||
| setLongPressed(true); | ||
| onLongPress(event); | ||
| }, delay); | ||
| }, | ||
| [onLongPress, delay] | ||
| ); | ||
|
|
||
| const clear = useCallback( | ||
| (event: React.MouseEvent, shouldClick: boolean = false) => { | ||
| if (timeoutRef.current) { | ||
| window.clearTimeout(timeoutRef.current); | ||
| } | ||
| if (shouldClick && !longPressed && onClick) { | ||
| onClick(event); | ||
| } | ||
| setLongPressed(false); | ||
| }, | ||
| [onClick, longPressed] | ||
| ); | ||
|
|
||
| const handlers = { | ||
| onMouseDown: start, | ||
| onMouseUp: (e: React.MouseEvent) => clear(e, true), | ||
| onMouseLeave: (e: React.MouseEvent) => clear(e, false), | ||
| onTouchStart: start, | ||
| onTouchEnd: () => { | ||
| if (timeoutRef.current) { | ||
| window.clearTimeout(timeoutRef.current); | ||
| } | ||
| setLongPressed(false); | ||
| }, | ||
| }; | ||
|
|
||
| return handlers; | ||
| } |
There was a problem hiding this comment.
The useLongPress hook doesn't include a cleanup effect to clear pending timeouts when the component unmounts. If the component unmounts while a long press is in progress, the timeout will still fire, potentially causing a setState on an unmounted component. Add a useEffect cleanup to clear timeoutRef.current on unmount.
Greptile SummaryThis PR adds comprehensive tablet support while maintaining the phone block. The implementation uses a new Key changes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant useDevice
participant Layout
participant DnD as DnD Backend
User->>Browser: Load app on tablet
Browser->>useDevice: Check window.innerWidth & touch
useDevice-->>Browser: isTablet=true, isTouch=true
Browser->>Layout: Render with device info
Layout->>Layout: Auto-collapse sidebar (useEffect)
Layout->>Layout: Hide right container
Layout->>Layout: Apply 44px touch targets
User->>Browser: Attempt drag operation
Browser->>DnD: Detect touch event
DnD->>DnD: Wait 200ms (delayTouchStart)
DnD-->>Browser: Enable drag-and-drop
User->>Browser: Resize window
Browser->>useDevice: Update device detection
useDevice-->>Layout: New device state
Layout->>Layout: Re-apply responsive rules
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (isTablet && !sidebarMetadata.collapsed) { | ||
| handleSidebarToggle(); | ||
| } | ||
| }, [isTablet]); |
There was a problem hiding this comment.
logic: missing handleSidebarToggle in dependency array will cause stale closure
| useEffect(() => { | |
| if (isTablet && !sidebarMetadata.collapsed) { | |
| handleSidebarToggle(); | |
| } | |
| }, [isTablet]); | |
| useEffect(() => { | |
| if (isTablet && !sidebarMetadata.collapsed) { | |
| handleSidebarToggle(); | |
| } | |
| }, [isTablet, handleSidebarToggle, sidebarMetadata.collapsed]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/layouts/layout.tsx
Line: 97:101
Comment:
**logic:** missing `handleSidebarToggle` in dependency array will cause stale closure
```suggestion
useEffect(() => {
if (isTablet && !sidebarMetadata.collapsed) {
handleSidebarToggle();
}
}, [isTablet, handleSidebarToggle, sidebarMetadata.collapsed]);
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Changes
useDevicehook with breakpoint detection (mobile/tablet/desktop)Test plan
🤖 Generated with Claude Code