refactor(frontend): improve dialog component and settings navigation

chriswritescode-dev · chriswritescode-dev · commit 93fd21908774 · 2026-05-03T10:43:30.000-04:00
diff --git a/frontend/src/components/settings/SettingsDialog.tsx b/frontend/src/components/settings/SettingsDialog.tsx
@@ -1,4 +1,4 @@
-import { useState, useRef, useEffect, useCallback } from 'react'
+import { useState, useEffect, useCallback } from 'react'
 import { GeneralSettings } from '@/components/settings/GeneralSettings'
 import { GitSettings } from '@/components/settings/GitSettings'
 import { KeyboardShortcuts } from '@/components/settings/KeyboardShortcuts'
@@ -12,7 +12,6 @@ import { Dialog, DialogContent } from '@/components/ui/dialog'
 import { Tabs, TabsContent, TabsList, TabsTrigger } from '@/components/ui/tabs'
 import { Settings2, Keyboard, Code, ChevronLeft, Key, GitBranch, User, Volume2, Bell, X } from 'lucide-react'
 import { Button } from '@/components/ui/button'
-import { useSwipeBack } from '@/hooks/useMobile'
 import { useSettingsDialog } from '@/hooks/useSettingsDialog'
 
 type SettingsView = 'menu' | 'general' | 'git' | 'shortcuts' | 'opencode' | 'providers' | 'account' | 'voice' | 'notifications'
@@ -21,7 +20,6 @@ export function SettingsDialog() {
   const { isOpen, close, activeTab, setActiveTab } = useSettingsDialog()
   const [mobileView, setMobileView] = useState<SettingsView>('menu')
   const [sectionHistory, setSectionHistory] = useState<SettingsView[]>([])
-  const contentRef = useRef<HTMLDivElement>(null)
 
   const pushSectionHistory = useCallback((view: SettingsView) => {
     if (view === 'menu') return
@@ -54,16 +52,6 @@ export function SettingsDialog() {
     setMobileView('menu')
   }, [mobileView, sectionHistory, close, setActiveTab])
 
-  const { bind: bindSwipe, swipeStyles } = useSwipeBack(close, {
-    enabled: isOpen,
-    canBack: () => mobileView !== 'menu',
-    onBack: handleSettingsBack,
-  })
-
-  useEffect(() => {
-    return bindSwipe(contentRef.current)
-  }, [bindSwipe])
-
   useEffect(() => {
     if (!isOpen) {
       setMobileView('menu')
@@ -105,13 +93,13 @@ export function SettingsDialog() {
   }
 
    return (
-     <Dialog open={isOpen} modal={false}>
-       <DialogContent 
-         ref={contentRef}
-         className="inset-0 w-full h-full max-w-none max-h-none p-0 rounded-none bg-gradient-to-br from-background via-background to-background border-border overflow-hidden !flex !flex-col"
-         style={swipeStyles}
-         fullscreen
-       >
+      <Dialog open={isOpen} modal={false} onOpenChange={(open) => !open && close()}>
+        <DialogContent 
+          className="inset-0 w-full h-full max-w-none max-h-none p-0 rounded-none bg-gradient-to-br from-background via-background to-background border-border overflow-hidden !flex !flex-col"
+          fullscreen
+          canSwipeBack={() => mobileView !== 'menu'}
+          onSwipeBack={handleSettingsBack}
+        >
          <div className="hidden sm:flex sm:flex-col sm:h-full">
            <div className="sticky top-0 z-10 bg-gradient-to-b from-background via-background to-transparent border-b border-border backdrop-blur-sm px-6 py-4 flex-shrink-0 flex items-center justify-between">
              <h2 className="text-2xl font-semibold bg-gradient-to-r from-foreground to-muted-foreground bg-clip-text text-transparent">
diff --git a/frontend/src/components/ui/dialog.test.tsx b/frontend/src/components/ui/dialog.test.tsx
@@ -1,4 +1,4 @@
-import { describe, it, expect, vi } from "vitest";
+import { describe, it, expect, vi, beforeEach } from "vitest";
 import { render, screen } from "@testing-library/react";
 import {
   Dialog,
@@ -7,7 +7,30 @@ import {
   DialogTitle,
 } from "./dialog";
 
+function withMobileViewport(fn: () => void) {
+  const originalWidth = window.innerWidth
+  Object.defineProperty(window, 'innerWidth', {
+    writable: true,
+    configurable: true,
+    value: 375,
+  })
+  fn()
+  Object.defineProperty(window, 'innerWidth', {
+    writable: true,
+    configurable: true,
+    value: originalWidth,
+  })
+}
+
 describe("DialogContent", () => {
+  beforeEach(() => {
+    Object.defineProperty(window, 'innerWidth', {
+      writable: true,
+      configurable: true,
+      value: 375,
+    })
+  })
+
   it("applies safe-area padding when fullscreen prop is true", () => {
     render(
       <Dialog open>
@@ -162,64 +185,225 @@ describe("DialogContent", () => {
     expect(content).toHaveStyle({ paddingTop: "env(safe-area-inset-top, 0px)" });
   });
 
-  it('renders hidden close trigger when mobileSwipeToClose is enabled', () => {
-    const onOpenChange = vi.fn();
-    render(
-      <Dialog open onOpenChange={onOpenChange}>
-        <DialogContent mobileFullscreen mobileSwipeToClose>
-          <DialogHeader>
-            <DialogTitle>Swipe Dialog</DialogTitle>
-          </DialogHeader>
-        </DialogContent>
-      </Dialog>
-    );
-    const closeTrigger = document.querySelector('[data-swipe-close-trigger]');
-    expect(closeTrigger).toBeInTheDocument();
+  it('renders hidden close trigger by default on mobile', () => {
+    withMobileViewport(() => {
+      const onOpenChange = vi.fn();
+      render(
+        <Dialog open onOpenChange={onOpenChange}>
+          <DialogContent>
+            <DialogHeader>
+              <DialogTitle>Swipe Dialog</DialogTitle>
+            </DialogHeader>
+          </DialogContent>
+        </Dialog>
+      );
+      const closeTrigger = document.querySelector('[data-swipe-close-trigger]');
+      expect(closeTrigger).toBeInTheDocument();
+    });
+  });
+
+  it('does not render hidden close trigger when mobileSwipeToClose is false', () => {
+    withMobileViewport(() => {
+      render(
+        <Dialog open>
+          <DialogContent mobileSwipeToClose={false}>
+            <DialogHeader>
+              <DialogTitle>Swipe Dialog</DialogTitle>
+            </DialogHeader>
+          </DialogContent>
+        </Dialog>
+      );
+      const closeTrigger = document.querySelector('[data-swipe-close-trigger]');
+      expect(closeTrigger).not.toBeInTheDocument();
+    });
   });
 
   it('closes dialog when hidden close trigger is activated', () => {
-    const onOpenChange = vi.fn();
-    render(
-      <Dialog open onOpenChange={onOpenChange}>
-        <DialogContent mobileFullscreen mobileSwipeToClose>
-          <DialogHeader>
-            <DialogTitle>Swipe Dialog</DialogTitle>
-          </DialogHeader>
-        </DialogContent>
-      </Dialog>
-    );
-    
-    const closeTrigger = document.querySelector('[data-swipe-close-trigger]') as HTMLButtonElement | null;
-    expect(closeTrigger).toBeInTheDocument();
-    
-    if (closeTrigger) {
-      closeTrigger.click();
+    withMobileViewport(() => {
+      const onOpenChange = vi.fn();
+      render(
+        <Dialog open onOpenChange={onOpenChange}>
+          <DialogContent>
+            <DialogHeader>
+              <DialogTitle>Swipe Dialog</DialogTitle>
+            </DialogHeader>
+          </DialogContent>
+        </Dialog>
+      );
+      
+      const closeTrigger = document.querySelector('[data-swipe-close-trigger]') as HTMLButtonElement | null;
+      expect(closeTrigger).toBeInTheDocument();
+      
+      if (closeTrigger) {
+        closeTrigger.click();
+        expect(onOpenChange).toHaveBeenCalledWith(false);
+      }
+    });
+  });
+
+  it('calls onSwipeBack when canSwipeBack is true and swipe completes', () => {
+    withMobileViewport(() => {
+      const mockOnSwipeBack = vi.fn();
+      const onOpenChange = vi.fn();
+      render(
+        <Dialog open onOpenChange={onOpenChange}>
+          <DialogContent
+            canSwipeBack={() => true}
+            onSwipeBack={mockOnSwipeBack}
+            data-testid="swipe-dialog"
+          >
+            Content
+          </DialogContent>
+        </Dialog>
+      );
+      
+      const content = screen.getByTestId('swipe-dialog');
+
+      content.dispatchEvent(new TouchEvent('touchstart', {
+        touches: [{ clientX: 10, clientY: 100 }] as any,
+      }));
+      content.dispatchEvent(new TouchEvent('touchmove', {
+        touches: [{ clientX: 100, clientY: 100 }] as any,
+      }));
+      content.dispatchEvent(new TouchEvent('touchend', {
+        changedTouches: [{ clientX: 100, clientY: 100 }] as any,
+      }));
+      
+      expect(mockOnSwipeBack).toHaveBeenCalled();
+      expect(onOpenChange).not.toHaveBeenCalled();
+    });
+  });
+
+  it('attempts close when canSwipeBack is false and swipe completes', () => {
+    withMobileViewport(() => {
+      const mockOnSwipeBack = vi.fn();
+      const onOpenChange = vi.fn();
+      render(
+        <Dialog open onOpenChange={onOpenChange}>
+          <DialogContent
+            canSwipeBack={() => false}
+            onSwipeBack={mockOnSwipeBack}
+            data-testid="swipe-dialog"
+          >
+            Content
+          </DialogContent>
+        </Dialog>
+      );
+      
+      const content = screen.getByTestId('swipe-dialog');
+      content.dispatchEvent(new TouchEvent('touchstart', {
+        touches: [{ clientX: 10, clientY: 100 }] as any,
+      }));
+      content.dispatchEvent(new TouchEvent('touchmove', {
+        touches: [{ clientX: 100, clientY: 100 }] as any,
+      }));
+      content.dispatchEvent(new TouchEvent('touchend', {
+        changedTouches: [{ clientX: 100, clientY: 100 }] as any,
+      }));
+      
+      expect(mockOnSwipeBack).not.toHaveBeenCalled();
       expect(onOpenChange).toHaveBeenCalledWith(false);
-    }
+    });
+  });
+
+  it('applies safe-area style and hidden trigger for mobileFullscreen', () => {
+    withMobileViewport(() => {
+      render(
+        <Dialog open>
+          <DialogContent mobileFullscreen data-testid="dialog-content">
+            Content
+          </DialogContent>
+        </Dialog>
+      );
+      const content = screen.getByTestId("dialog-content");
+      expect(content).toHaveStyle({ paddingTop: "env(safe-area-inset-top, 0px)" });
+      expect(document.querySelector('[data-swipe-close-trigger]')).toBeInTheDocument();
+    });
+  });
+
+  it('does not apply transform styles to non-fullscreen dialogs', () => {
+    withMobileViewport(() => {
+      render(
+        <Dialog open>
+          <DialogContent data-testid="dialog-content">
+            Content
+          </DialogContent>
+        </Dialog>
+      );
+      const content = screen.getByTestId("dialog-content");
+      const style = content.getAttribute("style") || "";
+      expect(style).not.toMatch(/transform/);
+    });
+  });
+
+  it('applies swipe transform to fullscreen dialogs during touchmove', () => {
+    withMobileViewport(() => {
+      render(
+        <Dialog open>
+          <DialogContent fullscreen data-testid="dialog-content">
+            Content
+          </DialogContent>
+        </Dialog>
+      );
+      const content = screen.getByTestId("dialog-content");
+      content.dispatchEvent(new TouchEvent('touchstart', {
+        touches: [{ clientX: 10, clientY: 100 }] as any,
+      }));
+      content.dispatchEvent(new TouchEvent('touchmove', {
+        touches: [{ clientX: 50, clientY: 100 }] as any,
+      }));
+      
+      const style = content.getAttribute("style") || "";
+      expect(style).toMatch(/transform/);
+    });
+  });
+
+  it('does not apply swipe transform to non-fullscreen dialogs during touchmove', () => {
+    withMobileViewport(() => {
+      render(
+        <Dialog open>
+          <DialogContent data-testid="dialog-content">
+            Content
+          </DialogContent>
+        </Dialog>
+      );
+      const content = screen.getByTestId("dialog-content");
+      content.dispatchEvent(new TouchEvent('touchstart', {
+        touches: [{ clientX: 10, clientY: 100 }] as any,
+      }));
+      content.dispatchEvent(new TouchEvent('touchmove', {
+        touches: [{ clientX: 50, clientY: 100 }] as any,
+      }));
+      
+      const style = content.getAttribute("style") || "";
+      expect(style).not.toMatch(/transform/);
+    });
   });
 
   it('binds swipe handler when mobileSwipeToClose and mobileFullscreen are enabled', () => {
-    const onOpenChange = vi.fn();
-    render(
-      <Dialog open onOpenChange={onOpenChange}>
-        <DialogContent mobileFullscreen mobileSwipeToClose data-testid="swipe-dialog">
-          <DialogHeader>
-            <DialogTitle>Swipe Dialog</DialogTitle>
-          </DialogHeader>
-        </DialogContent>
-      </Dialog>
-    );
-    
-    const content = screen.getByTestId('swipe-dialog');
-    const closeTrigger = document.querySelector('[data-swipe-close-trigger]') as HTMLButtonElement | null;
-    
-    expect(content).toBeInTheDocument();
-    expect(closeTrigger).toBeInTheDocument();
-    
-    const clickSpy = vi.spyOn(closeTrigger as HTMLButtonElement, 'click');
-    closeTrigger?.click();
-    
-    expect(clickSpy).toHaveBeenCalled();
-    expect(onOpenChange).toHaveBeenCalledWith(false);
+    withMobileViewport(() => {
+      const onOpenChange = vi.fn();
+      render(
+        <Dialog open onOpenChange={onOpenChange}>
+          <DialogContent mobileFullscreen mobileSwipeToClose data-testid="swipe-dialog">
+            <DialogHeader>
+              <DialogTitle>Swipe Dialog</DialogTitle>
+            </DialogHeader>
+          </DialogContent>
+        </Dialog>
+      );
+      
+      const content = screen.getByTestId('swipe-dialog');
+      const closeTrigger = document.querySelector('[data-swipe-close-trigger]') as HTMLButtonElement | null;
+      
+      expect(content).toBeInTheDocument();
+      expect(closeTrigger).toBeInTheDocument();
+      
+      const clickSpy = vi.spyOn(closeTrigger as HTMLButtonElement, 'click');
+      closeTrigger?.click();
+      
+      expect(clickSpy).toHaveBeenCalled();
+      expect(onOpenChange).toHaveBeenCalledWith(false);
+    });
   });
 });
diff --git a/frontend/src/components/ui/dialog.tsx b/frontend/src/components/ui/dialog.tsx
