From 65d25e3ce5e481e4e8a4b4aedb5516a8bf5e0f20 Mon Sep 17 00:00:00 2001
From: Richard R <me@richardr.dev>
Date: Sat, 23 May 2026 05:50:54 -0600
Subject: [PATCH] refactor(ui): centralize privacy gating logic for settings
 modal

Consolidate privacy acceptance checks into reusable hooks and callbacks within
SettingsModal. Replace scattered gating with a single entry point for opening
settings or changelog views, ensuring consistent enforcement. Update test
helpers to robustly dismiss onboarding and settings overlays by avoiding
state race conditions.
---
 src/components/SettingsModal.tsx | 54 +++++++++++++++++++++++++++-----
 tests/helpers.ts                 | 47 +++++++++++++--------------
 2 files changed, 71 insertions(+), 30 deletions(-)

diff --git a/src/components/SettingsModal.tsx b/src/components/SettingsModal.tsx
index ec1d72d..3a25446 100644
--- a/src/components/SettingsModal.tsx
+++ b/src/components/SettingsModal.tsx
@@ -208,9 +208,32 @@ export function SettingsModal({ className = '' }: { className?: string }) {
   const isSharedSelected = Boolean(selectedSharedProvider);
   const selectedProviderOption = ttsProviders.find((p) => p.id === localProviderRef) ?? ttsProviders[0];
 
-  const checkFirstVist = useCallback(async () => {
+  const closeSettingsForPrivacyGate = useCallback(() => {
+    setIsOpen(false);
+    setIsChangelogOpen(false);
+  }, []);
+
+  const canOpenSettings = useCallback(async () => {
+    if (!authEnabled) {
+      return true;
+    }
     const appConfig = await getAppConfig();
-    if (authEnabled && !appConfig?.privacyAccepted) {
+    return Boolean(appConfig?.privacyAccepted);
+  }, [authEnabled]);
+
+  const openSettings = useCallback(async (options?: { changelog?: boolean }) => {
+    const allowed = await canOpenSettings();
+    if (!allowed) {
+      closeSettingsForPrivacyGate();
+      return;
+    }
+    setIsOpen(true);
+    setIsChangelogOpen(Boolean(options?.changelog));
+  }, [canOpenSettings, closeSettingsForPrivacyGate]);
+
+  const checkFirstVist = useCallback(async () => {
+    const allowed = await canOpenSettings();
+    if (!allowed) {
       return;
     }
     const firstVisit = await getFirstVisit();
@@ -218,7 +241,7 @@ export function SettingsModal({ className = '' }: { className?: string }) {
       await setFirstVisit(true);
       setIsOpen(true);
     }
-  }, [authEnabled, setIsOpen]);
+  }, [canOpenSettings, setIsOpen]);
 
   useEffect(() => {
     checkFirstVist().catch((err) => {
@@ -250,6 +273,22 @@ export function SettingsModal({ className = '' }: { className?: string }) {
     };
   }, [authEnabled, checkFirstVist]);
 
+  useEffect(() => {
+    if (!isOpen) {
+      return;
+    }
+    let cancelled = false;
+    void (async () => {
+      const allowed = await canOpenSettings();
+      if (!allowed && !cancelled) {
+        closeSettingsForPrivacyGate();
+      }
+    })();
+    return () => {
+      cancelled = true;
+    };
+  }, [isOpen, canOpenSettings, closeSettingsForPrivacyGate]);
+
   useEffect(() => {
     return scheduleChangelogCheck({
       authEnabled,
@@ -260,13 +299,12 @@ export function SettingsModal({ className = '' }: { className?: string }) {
       inFlightRef: changelogVersionCheckInFlightRef,
       postCheck: async (currentVersion) => postChangelogVersionCheck(currentVersion),
       onShouldOpen: () => {
-        setIsOpen(true);
-        setIsChangelogOpen(true);
+        void openSettings({ changelog: true });
       },
       delayMs: 120,
       retryDelayMs: 400,
     });
-  }, [authEnabled, isSessionPending, session?.user?.id, runtimeConfig.appVersion]);
+  }, [authEnabled, isSessionPending, session?.user?.id, runtimeConfig.appVersion, openSettings]);
 
   useEffect(() => {
     if (!ttsModels.some(m => m.id === modelValue) && modelValue !== '') {
@@ -532,7 +570,9 @@ export function SettingsModal({ className = '' }: { className?: string }) {
   return (
     <>
       <Button
-        onClick={() => setIsOpen(true)}
+        onClick={() => {
+          void openSettings();
+        }}
         className={`inline-flex items-center py-1 px-2 rounded-md border border-offbase bg-base text-foreground text-xs hover:bg-offbase hover:text-accent transition-transform transition-colors duration-200 ease-out hover:scale-[1.08] ${className}`}
         aria-label="Settings"
         tabIndex={0}
diff --git a/tests/helpers.ts b/tests/helpers.ts
index d002522..c5b6da5 100644
--- a/tests/helpers.ts
+++ b/tests/helpers.ts
@@ -113,29 +113,6 @@ async function dismissOnboardingModals(page: Page): Promise<void> {
   let settledWithoutDialog = 0;
 
   for (let step = 0; step < maxSteps; step += 1) {
-    if (await settingsDialog.isVisible().catch(() => false)) {
-      const backToSettingsBtn = settingsDialog.getByRole('button', { name: /back to settings/i });
-      if (await backToSettingsBtn.isVisible().catch(() => false)) {
-        await expect(backToSettingsBtn).toBeEnabled({ timeout: 10000 });
-        await backToSettingsBtn.click();
-        await page.waitForTimeout(100);
-        settledWithoutDialog = 0;
-        continue;
-      }
-
-      const saveBtn = page.getByTestId('settings-save-button');
-      if (await saveBtn.isVisible().catch(() => false)) {
-        await expect(saveBtn).toBeEnabled({ timeout: 15000 });
-        await saveBtn.click();
-      } else {
-        await page.keyboard.press('Escape');
-      }
-      await settingsDialog.waitFor({ state: 'hidden', timeout: 15000 });
-      await page.waitForTimeout(100);
-      settledWithoutDialog = 0;
-      continue;
-    }
-
     if (await privacyDialog.isVisible().catch(() => false)) {
       const privacyAgree = page.getByTestId('privacy-agree-checkbox');
       if (await privacyAgree.isVisible().catch(() => false)) {
@@ -152,6 +129,30 @@ async function dismissOnboardingModals(page: Page): Promise<void> {
       continue;
     }
 
+    if (await settingsDialog.isVisible().catch(() => false)) {
+      const backToSettingsBtn = settingsDialog.getByRole('button', { name: /back to settings/i });
+      if (await backToSettingsBtn.isVisible().catch(() => false)) {
+        await expect(backToSettingsBtn).toBeEnabled({ timeout: 10000 });
+        await backToSettingsBtn.click();
+        await page.waitForTimeout(100);
+      }
+
+      // For test setup teardown, we only need to dismiss overlays.
+      // Avoid saving because Save can race with state changes and detach.
+      for (let attempt = 0; attempt < 3; attempt += 1) {
+        await page.keyboard.press('Escape');
+        const hidden = await settingsDialog.isHidden().catch(() => false);
+        if (hidden) {
+          break;
+        }
+        await page.waitForTimeout(100);
+      }
+      await settingsDialog.waitFor({ state: 'hidden', timeout: 15000 });
+      await page.waitForTimeout(100);
+      settledWithoutDialog = 0;
+      continue;
+    }
+
     if (await migrationDialog.isVisible().catch(() => false)) {
       const skipBtn = page.getByTestId('migration-skip-button');
       await expect(skipBtn).toBeEnabled({ timeout: 10000 });