From c4b6c131bca07719eb327ee8699cb5c59f0e0092 Mon Sep 17 00:00:00 2001
From: Waleed Latif <walif6@gmail.com>
Date: Sun, 23 Aug 2026 18:55:48 -0700
Subject: [PATCH] fix(settings): report a failed settings write instead of
 reporting success
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The PATCH catch answered `{ success: true }` with 200, so a failed upsert was
indistinguishable from a saved one.

`useUpdateGeneralSetting` is optimistic: `onMutate` writes the new value into
the cache and calls `syncThemeToNextThemes`, and `onError` restores the previous
settings. `requestJson` only throws on a non-2xx, so `onError` could never run —
the rollback and its theme re-sync were unreachable code. A user toggling a
consent-shaped setting (telemetry, email opt-out) saw it applied and it was not
saved, until a later refetch quietly reverted it.

The catch now returns 500, which is what the mutation was already written to
handle.

Left alone deliberately: GET still falls back to `defaultUserSettings` on error.
Failing it would take the settings page down on a transient read, and the value
of changing it is a separate judgement from this one.

Covered by a route test that drives the failure through the real handler.
Verified it fails when the 200 is put back.
---
 .../app/api/users/me/settings/route.test.ts   | 48 +++++++++++++++++++
 apps/sim/app/api/users/me/settings/route.ts   |  6 ++-
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 apps/sim/app/api/users/me/settings/route.test.ts

diff --git a/apps/sim/app/api/users/me/settings/route.test.ts b/apps/sim/app/api/users/me/settings/route.test.ts
new file mode 100644
index 00000000000..f11f71ff96f
--- /dev/null
+++ b/apps/sim/app/api/users/me/settings/route.test.ts
@@ -0,0 +1,48 @@
+/**
+ * @vitest-environment node
+ */
+import { createMockRequest, dbChainMockFns, resetDbChainMock } from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockGetSession } = vi.hoisted(() => ({
+  mockGetSession: vi.fn(),
+}))
+
+vi.mock('@/lib/auth', () => ({
+  auth: { api: { getSession: vi.fn() } },
+  getSession: mockGetSession,
+}))
+
+import { PATCH } from '@/app/api/users/me/settings/route'
+
+describe('PATCH /api/users/me/settings', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+    mockGetSession.mockResolvedValue({ user: { id: 'user-1' } })
+  })
+
+  it('reports success when the write lands', async () => {
+    const response = await PATCH(createMockRequest('PATCH', { theme: 'dark' }))
+
+    expect(response.status).toBe(200)
+    expect(await response.json()).toEqual({ success: true })
+  })
+
+  /**
+   * The regression this guards: the catch answered `{ success: true }` with 200, so
+   * `useUpdateGeneralSetting`'s optimistic rollback in `onError` could never run —
+   * a failed write showed as applied until the next refetch, including for
+   * consent-shaped settings the user believes they changed.
+   */
+  it('reports failure when the write throws', async () => {
+    dbChainMockFns.insert.mockImplementationOnce(() => {
+      throw new Error('connection terminated unexpectedly')
+    })
+
+    const response = await PATCH(createMockRequest('PATCH', { theme: 'dark' }))
+
+    expect(response.status).toBe(500)
+    expect(await response.json()).not.toMatchObject({ success: true })
+  })
+})
diff --git a/apps/sim/app/api/users/me/settings/route.ts b/apps/sim/app/api/users/me/settings/route.ts
index 24ccacceb62..69e06dd689c 100644
--- a/apps/sim/app/api/users/me/settings/route.ts
+++ b/apps/sim/app/api/users/me/settings/route.ts
@@ -74,6 +74,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest) => {
     return NextResponse.json({ success: true }, { status: 200 })
   } catch (error: any) {
     logger.error(`[${requestId}] Settings update error`, error)
-    return NextResponse.json({ success: true }, { status: 200 })
+    /* The client mutation is optimistic: it writes the new value into the cache in
+       `onMutate` and restores it in `onError`. Answering 200 here left that rollback
+       unreachable, so a failed write showed as applied until the next refetch —
+       including for consent-shaped settings the user believes they changed. */
+    return NextResponse.json({ error: 'Failed to update settings' }, { status: 500 })
   }
 })