From c7abc90d15611051b016fe897def2692f9dd098c Mon Sep 17 00:00:00 2001 From: Deep Mehta Date: Fri, 15 May 2026 14:23:27 -0700 Subject: [PATCH 1/6] fix(cloud): stop bouncing working users to /cloud/survey mid-session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Two coupled changes to the onboarding-survey gate so that customers who are already in their workflow do not get yanked to /cloud/survey by transient errors or background reloads. 1. `getSurveyCompletedStatus` (auth.ts) now resolves ambiguous responses to "completed": - 200 with non-empty `value` → true (definitely completed) - 200 with empty `value` → false (definitely not completed) - 404 / 5xx / network → true (ambiguous → safer default) - 401 / 403 → throws (auth issue, not a survey signal) Previously every non-2xx response collapsed to `false`, meaning any transient 5xx, network blip, or token-refresh race during the `/api/settings/onboarding_survey` call redirected the user to the survey. The new resolution preserves the genuine "user has never saved a survey" signal (only fires on a 200 with an empty value) while refusing to treat a 5xx as a survey-state signal. 2. The router's survey gate (router.ts beforeEach) is now evaluated at most once per browser-tab session, using sessionStorage. sessionStorage persists across `window.location.reload()` in the same tab, so any background reload — token refresh recovery, GraphCanvas's 401 path, the cloud-remote-config 10-minute poll — can no longer re-bounce a working user. A new tab still gets a fresh check (matches the intent of showing the survey to first-time and existing-but-never-prompted users). The catch branch is also fixed: previously a thrown auth error bounced to /cloud-user-check, which calls the same endpoints and can loop. Now we mark the session as evaluated and let the user proceed; the auth layer handles re-auth on the next API call. ## Why User reports from team-plan customers: "I was working in a workflow, hit run, and then got logged out and redirected to a survey screen." Datadog shows ~7,000 distinct users/day hit the survey-not-found 404 path on prod ingest. The flag `onboarding_survey_enabled` is true in prod dynamic config; combined with the catch-all `!response.ok` returning `false`, any mid-session reload trips a redirect to /cloud/survey. The user's stated requirement: rather miss showing the survey to a few users than show it duplicately or interrupt working customers. ## Trade-off (worth Robin's review) A genuinely brand-new user whose User.Settings JSON is empty returns 404 from `/api/settings/onboarding_survey` (the backend doesn't distinguish "key absent" from "user not found"). With this change, that 404 is treated as "completed", so the survey gate does not fire for them on the strict 404 path. New users will still see the survey on the empty-200 path if signup pre-populates the Settings key with an empty object; if not, the survey is missed on initial signup. Acceptable per product requirement that false positives are worse than false negatives. If we want to recover that signal cleanly, the proper fix is a backend change to return 200 with `value: null` when the User row exists but the key is absent — distinguishing "no survey saved" from "user not found". Out of scope for this PR. ## Companion: Sentry breadcrumbs Both files emit Sentry breadcrumbs at decision points (gate redirect, ambiguous-response fallback) with `treated_as` and `initial_load` flags so the next incident report can be diagnosed from session replay rather than speculation. ## Test plan - [ ] Logged-in user with completed survey navigates around — no redirect - [ ] Logged-in user with no survey, fresh tab → redirected to /cloud/survey - [ ] Logged-in user with no survey, after submitting → no redirect on next nav - [ ] Simulate transient 5xx on `/api/settings/onboarding_survey` (DevTools throttling / blocking) → user stays on current page, no redirect - [ ] Trigger `window.location.reload()` mid-session → no redirect to survey - [ ] Sentry breadcrumbs show up on test events --- src/platform/cloud/onboarding/auth.ts | 75 ++++++++++++++++++++------- src/router.ts | 51 ++++++++++++++++-- 2 files changed, 103 insertions(+), 23 deletions(-) diff --git a/src/platform/cloud/onboarding/auth.ts b/src/platform/cloud/onboarding/auth.ts index 30a1d7878e5..4aca70dfc64 100644 --- a/src/platform/cloud/onboarding/auth.ts +++ b/src/platform/cloud/onboarding/auth.ts @@ -87,6 +87,22 @@ export async function getUserCloudStatus(): Promise { } } +/** + * Returns whether the user has completed the onboarding survey. + * + * Resolution rules (prefer false negatives over false positives — i.e. rather + * miss a survey prompt than redirect a working customer to /cloud/survey): + * - 200 with non-empty `value` → true (definitely completed) + * - 200 with empty `value` → false (definitely not completed) + * - 404 → true (key absent; could be a + * genuinely new user OR a customer whose Settings JSON pre-dates the + * survey. We can't distinguish on the wire and prefer the safer default. + * New users still get the survey via the onboarding signup flow itself.) + * - 401 / 403 → throws (auth issue, not a survey + * signal — propagate so the auth layer handles it) + * - 5xx / network error → true (backend hiccup — don't + * bounce the user on a transient blip) + */ export async function getSurveyCompletedStatus(): Promise { try { const response = await api.fetchApi(`/settings/${ONBOARDING_SURVEY_KEY}`, { @@ -95,24 +111,46 @@ export async function getSurveyCompletedStatus(): Promise { 'Content-Type': 'application/json' } }) - if (!response.ok) { - // Not an error case - survey not completed is a valid state - Sentry.addBreadcrumb({ - category: 'auth', - message: 'Survey status check returned non-ok response', - level: 'info', - data: { - status: response.status, - endpoint: `/settings/${ONBOARDING_SURVEY_KEY}` - } - }) - return false + + if (response.ok) { + const data = await response.json() + // 200: definitive signal. Empty value = never completed. + return !isEmpty(data.value) } - const data = await response.json() - // Check if data exists and is not empty - return !isEmpty(data.value) + + if (response.status === 401 || response.status === 403) { + // Auth failure - propagate so the auth layer can handle. Returning + // false here would let an expired token masquerade as "no survey" and + // bounce the user to /cloud/survey on every transient 401. + throw new Error( + `Survey status auth error: ${response.status} ${response.statusText}` + ) + } + + // 404 / 5xx / other: ambiguous. Treat as completed to avoid false + // positives. The Sentry breadcrumb is so we can audit if this fires + // often enough to suggest a backend regression. + Sentry.addBreadcrumb({ + category: 'auth', + message: 'Survey status check returned non-ok response', + level: 'info', + data: { + status: response.status, + endpoint: `/settings/${ONBOARDING_SURVEY_KEY}`, + treated_as: 'completed' + } + }) + return true } catch (error) { - // Network error - still capture it as it's not thrown from above + // Re-throw auth errors so callers can branch on them; everything else is + // treated as "completed" (network failure, parse failure, etc). + if ( + error instanceof Error && + error.message.startsWith('Survey status auth error:') + ) { + throw error + } + Sentry.captureException(error, { tags: { api_endpoint: '/settings/{key}', @@ -120,11 +158,12 @@ export async function getSurveyCompletedStatus(): Promise { }, extra: { route_template: '/settings/{key}', - route_actual: `/settings/${ONBOARDING_SURVEY_KEY}` + route_actual: `/settings/${ONBOARDING_SURVEY_KEY}`, + treated_as: 'completed' }, level: 'warning' }) - return false + return true } } diff --git a/src/router.ts b/src/router.ts index 7623786565a..33436b13f90 100644 --- a/src/router.ts +++ b/src/router.ts @@ -1,3 +1,4 @@ +import * as Sentry from '@sentry/vue' import { until } from '@vueuse/core' import { storeToRefs } from 'pinia' import { @@ -18,6 +19,12 @@ import LayoutDefault from '@/views/layouts/LayoutDefault.vue' import { installPreservedQueryTracker } from '@/platform/navigation/preservedQueryTracker' import { PRESERVED_QUERY_NAMESPACES } from '@/platform/navigation/preservedQueryNamespaces' +// Tab-scoped flag: once we've evaluated the onboarding-survey gate for a +// browser-tab session, don't re-evaluate it on subsequent navigations. +// sessionStorage survives `window.location.reload()` in the same tab but +// is fresh for new tabs — exactly the semantics we want. +const SURVEY_GATE_SESSION_KEY = 'comfy.survey_gate_evaluated_this_session' + const cloudOnboardingRoutes = isCloud ? (await import('./platform/cloud/onboarding/onboardingCloudRoutes')) .cloudOnboardingRoutes @@ -207,11 +214,22 @@ if (isCloud) { } // User is logged in - check if they need onboarding (when enabled) - // For root path, check actual user status to handle waitlisted users + // For root path, check actual user status to handle waitlisted users. + // + // The survey gate is intentionally gated once per browser-tab session. + // sessionStorage persists across `window.location.reload()` in the same + // tab, so background reloads (token refresh races, GraphCanvas's 401 + // recovery, the remote-config 10-minute poll, etc.) cannot re-bounce a + // working user mid-session. A fresh tab gets a fresh check, which + // matches the intent of showing the survey to first-time and existing- + // but-never-prompted users. if (!isDesktop && isLoggedIn && to.path === '/') { if (!flags.onboardingSurveyEnabled) { return next() } + if (sessionStorage.getItem(SURVEY_GATE_SESSION_KEY)) { + return next() + } // Import auth functions dynamically to avoid circular dependency const { getSurveyCompletedStatus } = await import('@/platform/cloud/onboarding/auth') @@ -219,14 +237,37 @@ if (isCloud) { // Check user's actual status const surveyCompleted = await getSurveyCompletedStatus() - // Survey is required for all users (when feature flag enabled) + // Mark this session as gate-evaluated regardless of outcome. + // If we redirect to /cloud/survey, the user fills it out and the + // next /cloud-user-check call sees a completed survey on its own + // server round-trip; this flag only suppresses re-evaluation of + // the *same* request on subsequent navigations within this tab. + sessionStorage.setItem(SURVEY_GATE_SESSION_KEY, '1') + + // Survey is required for all users (when feature flag enabled). + // getSurveyCompletedStatus returns true for ambiguous responses + // (404/5xx/network) so this branch only fires on a definitive + // "user has no survey saved" signal — see auth.ts for the policy. if (!surveyCompleted) { + Sentry.addBreadcrumb({ + category: 'navigation', + message: 'survey gate → /cloud/survey', + level: 'info', + data: { + from_path: _from.fullPath, + from_name: String(_from.name ?? ''), + initial_load: _from.name === undefined + } + }) return next({ name: 'cloud-survey' }) } } catch (error) { - console.error('Failed to check user status:', error) - // On error, redirect to user-check as fallback - return next({ name: 'cloud-user-check' }) + // Most likely an auth error from getSurveyCompletedStatus. + // Don't bounce to /cloud-user-check — that re-runs the same checks + // and can produce a redirect loop. Let the user proceed; the auth + // layer will handle re-authentication on the next API call. + sessionStorage.setItem(SURVEY_GATE_SESSION_KEY, '1') + console.error('Failed to check survey status:', error) } } From c7176161f073205f538256821946f2b76e3d7789 Mon Sep 17 00:00:00 2001 From: Deep Mehta Date: Fri, 15 May 2026 15:06:37 -0700 Subject: [PATCH 2/6] fix(cloud): address review feedback on survey-gate fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace string-prefix error sentinel with a typed SurveyAuthError class so router/CloudSurveyView can branch on `instanceof` instead of matching a hardcoded message. - Clear the session-scoped survey-gate flag on logout. Without this, a same-tab account switch (logout uses `window.location.href`, which preserves sessionStorage) silently skipped the gate for the next user. - Stop marking the gate as evaluated when the auth check itself throws SurveyAuthError. The previous catch branch swallowed 401/403 and set the flag, disabling the gate for the whole tab session on a single transient auth blip. Non-auth errors still mark the flag (those are expected to keep failing in the short term and we don't want to hammer the API). - Wrap sessionStorage reads/writes in try/catch to match the codebase convention for SecurityError in sandboxed iframes / private browsing. - Move the gate's sessionStorage key into WORKSPACE_STORAGE_KEYS to follow the existing `Comfy.Workspace.*` naming. - Drop CloudSurveyView's redundant `getSurveyCompletedStatus()` re-check in onMounted. The router gate is the source of truth; a redundant check would treat a transient 5xx during signup as "already completed" and bounce a new user away from the survey page — the same false- positive class this PR is meant to fix. - Remove the Sentry breadcrumb/captureException calls added inside the survey-gate code paths. Sentry is deprecated in this org per CLAUDE.md. Pre-existing Sentry usage in getUserCloudStatus and submitSurvey is untouched. - Add auth.test.ts covering the five resolution branches plus the SurveyAuthError shape. --- src/composables/auth/useAuthActions.ts | 13 ++ .../cloud/onboarding/CloudSurveyView.vue | 22 +-- src/platform/cloud/onboarding/auth.test.ts | 156 ++++++++++++++++++ src/platform/cloud/onboarding/auth.ts | 53 +++--- src/platform/workspace/workspaceConstants.ts | 1 + src/router.ts | 49 ++++-- 6 files changed, 228 insertions(+), 66 deletions(-) create mode 100644 src/platform/cloud/onboarding/auth.test.ts diff --git a/src/composables/auth/useAuthActions.ts b/src/composables/auth/useAuthActions.ts index d9c9c621891..c4fa0a4ac27 100644 --- a/src/composables/auth/useAuthActions.ts +++ b/src/composables/auth/useAuthActions.ts @@ -11,6 +11,7 @@ import { useTelemetry } from '@/platform/telemetry' import { useToastStore } from '@/platform/updates/common/toastStore' import { useWorkflowService } from '@/platform/workflow/core/services/workflowService' import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore' +import { WORKSPACE_STORAGE_KEYS } from '@/platform/workspace/workspaceConstants' import { useDialogService } from '@/services/dialogService' import { useAuthStore } from '@/stores/authStore' import type { BillingPortalTargetTier } from '@/stores/authStore' @@ -81,6 +82,18 @@ export const useAuthActions = () => { } await authStore.logout() + + // Clear the survey gate so the next user signing in on the same tab + // gets a fresh evaluation. `window.location.href` below preserves + // sessionStorage, so without this an account switch silently skips the + // survey gate for the new user. + try { + sessionStorage.removeItem(WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED) + } catch { + // sessionStorage unavailable (sandboxed iframe / strict private mode); + // the tab is being navigated away anyway. + } + toastStore.add({ severity: 'success', summary: t('auth.signOut.success'), diff --git a/src/platform/cloud/onboarding/CloudSurveyView.vue b/src/platform/cloud/onboarding/CloudSurveyView.vue index 56b50f33d35..b723cffd750 100644 --- a/src/platform/cloud/onboarding/CloudSurveyView.vue +++ b/src/platform/cloud/onboarding/CloudSurveyView.vue @@ -14,10 +14,7 @@ import { computed, onMounted, ref } from 'vue' import { useRouter } from 'vue-router' import { useFeatureFlags } from '@/composables/useFeatureFlags' -import { - getSurveyCompletedStatus, - submitSurvey -} from '@/platform/cloud/onboarding/auth' +import { submitSurvey } from '@/platform/cloud/onboarding/auth' import { isCloud } from '@/platform/distribution/types' import { remoteConfig } from '@/platform/remoteConfig/remoteConfig' import { useTelemetry } from '@/platform/telemetry' @@ -40,17 +37,12 @@ onMounted(async () => { await router.replace({ name: 'cloud-user-check' }) return } - try { - const surveyCompleted = await getSurveyCompletedStatus() - if (surveyCompleted) { - await router.replace({ name: 'cloud-user-check' }) - return - } - if (isCloud) { - useTelemetry()?.trackSurvey('opened') - } - } catch (error) { - console.error('Failed to check survey status:', error) + // Don't re-check survey status here. The router gate and UserCheckView + // are the source of truth — re-checking would treat a transient 5xx as + // "already completed" and bounce a new user away from /cloud/survey + // during signup, the same false-positive class this PR is meant to fix. + if (isCloud) { + useTelemetry()?.trackSurvey('opened') } }) diff --git a/src/platform/cloud/onboarding/auth.test.ts b/src/platform/cloud/onboarding/auth.test.ts new file mode 100644 index 00000000000..bbda14bbfc8 --- /dev/null +++ b/src/platform/cloud/onboarding/auth.test.ts @@ -0,0 +1,156 @@ +import { beforeEach, describe, expect, test, vi } from 'vitest' + +import { + SurveyAuthError, + getSurveyCompletedStatus +} from './auth' + +const fetchApi = vi.fn() + +vi.mock('@/scripts/api', () => ({ + api: { + fetchApi: (...args: unknown[]) => fetchApi(...args) + } +})) + +// `@sentry/vue` is still imported by other functions in auth.ts +// (getUserCloudStatus, submitSurvey). Mock it so test runs don't init the +// real SDK. +vi.mock('@sentry/vue', () => ({ + addBreadcrumb: vi.fn(), + captureException: vi.fn() +})) + +function mockResponse({ + ok, + status, + statusText = '', + body +}: { + ok: boolean + status: number + statusText?: string + body?: unknown +}): Response { + return { + ok, + status, + statusText, + json: async () => body + } as unknown as Response +} + +describe('getSurveyCompletedStatus', () => { + beforeEach(() => { + fetchApi.mockReset() + }) + + test('200 with non-empty value → true', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: true, status: 200, body: { value: { q1: 'a' } } }) + ) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + + test('200 with empty object value → false', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: true, status: 200, body: { value: {} } }) + ) + await expect(getSurveyCompletedStatus()).resolves.toBe(false) + }) + + test('200 with null value → false', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: true, status: 200, body: { value: null } }) + ) + await expect(getSurveyCompletedStatus()).resolves.toBe(false) + }) + + test('404 → true (treated as completed to avoid false-positive bounce)', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 404, statusText: 'Not Found' }) + ) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + + test('401 → throws SurveyAuthError with status', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 401, statusText: 'Unauthorized' }) + ) + await expect(getSurveyCompletedStatus()).rejects.toBeInstanceOf( + SurveyAuthError + ) + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 401, statusText: 'Unauthorized' }) + ) + await expect(getSurveyCompletedStatus()).rejects.toMatchObject({ + status: 401 + }) + }) + + test('403 → throws SurveyAuthError with status', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 403, statusText: 'Forbidden' }) + ) + await expect(getSurveyCompletedStatus()).rejects.toBeInstanceOf( + SurveyAuthError + ) + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 403, statusText: 'Forbidden' }) + ) + await expect(getSurveyCompletedStatus()).rejects.toMatchObject({ + status: 403 + }) + }) + + test('500 → true (transient backend hiccup, do not bounce)', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ + ok: false, + status: 500, + statusText: 'Internal Server Error' + }) + ) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + + test('503 → true (transient backend hiccup, do not bounce)', async () => { + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 503, statusText: 'Service Unavailable' }) + ) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + + test('network/fetch rejection → true (do not bounce)', async () => { + fetchApi.mockRejectedValueOnce(new TypeError('Network request failed')) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + + test('SurveyAuthError thrown by inner branch propagates through outer catch', async () => { + // Sanity check: the catch block must re-throw SurveyAuthError instead + // of swallowing it as "ambiguous" — that's the regression the typed + // error class is meant to prevent. + fetchApi.mockResolvedValueOnce( + mockResponse({ ok: false, status: 401, statusText: 'Unauthorized' }) + ) + await expect(getSurveyCompletedStatus()).rejects.toBeInstanceOf( + SurveyAuthError + ) + }) +}) + +describe('SurveyAuthError', () => { + test('is an Error instance', () => { + const err = new SurveyAuthError(401, 'Unauthorized') + expect(err).toBeInstanceOf(Error) + expect(err).toBeInstanceOf(SurveyAuthError) + }) + + test('carries status and a descriptive message', () => { + const err = new SurveyAuthError(403, 'Forbidden') + expect(err.status).toBe(403) + expect(err.message).toContain('403') + expect(err.message).toContain('Forbidden') + expect(err.name).toBe('SurveyAuthError') + }) +}) diff --git a/src/platform/cloud/onboarding/auth.ts b/src/platform/cloud/onboarding/auth.ts index 4aca70dfc64..54c5ea88e50 100644 --- a/src/platform/cloud/onboarding/auth.ts +++ b/src/platform/cloud/onboarding/auth.ts @@ -10,6 +10,20 @@ interface UserCloudStatus { const ONBOARDING_SURVEY_KEY = 'onboarding_survey' +/** + * Thrown when /settings/{key} returns 401/403. Callers can branch on + * `error instanceof SurveyAuthError` instead of pattern-matching error + * messages — see router.ts and CloudSurveyView.vue for the consumers. + */ +export class SurveyAuthError extends Error { + readonly status: number + constructor(status: number, statusText: string) { + super(`Survey status auth error: ${status} ${statusText}`) + this.name = 'SurveyAuthError' + this.status = status + } +} + /** * Helper function to capture API errors with Sentry */ @@ -122,47 +136,18 @@ export async function getSurveyCompletedStatus(): Promise { // Auth failure - propagate so the auth layer can handle. Returning // false here would let an expired token masquerade as "no survey" and // bounce the user to /cloud/survey on every transient 401. - throw new Error( - `Survey status auth error: ${response.status} ${response.statusText}` - ) + throw new SurveyAuthError(response.status, response.statusText) } // 404 / 5xx / other: ambiguous. Treat as completed to avoid false - // positives. The Sentry breadcrumb is so we can audit if this fires - // often enough to suggest a backend regression. - Sentry.addBreadcrumb({ - category: 'auth', - message: 'Survey status check returned non-ok response', - level: 'info', - data: { - status: response.status, - endpoint: `/settings/${ONBOARDING_SURVEY_KEY}`, - treated_as: 'completed' - } - }) + // positives — rather miss a survey than bounce a paying customer. return true } catch (error) { - // Re-throw auth errors so callers can branch on them; everything else is - // treated as "completed" (network failure, parse failure, etc). - if ( - error instanceof Error && - error.message.startsWith('Survey status auth error:') - ) { + // Re-throw auth errors so callers can branch on them; everything else + // (network failure, parse failure, etc) is treated as "completed". + if (error instanceof SurveyAuthError) { throw error } - - Sentry.captureException(error, { - tags: { - api_endpoint: '/settings/{key}', - error_type: 'network_error' - }, - extra: { - route_template: '/settings/{key}', - route_actual: `/settings/${ONBOARDING_SURVEY_KEY}`, - treated_as: 'completed' - }, - level: 'warning' - }) return true } } diff --git a/src/platform/workspace/workspaceConstants.ts b/src/platform/workspace/workspaceConstants.ts index b56c1644d86..447b584c545 100644 --- a/src/platform/workspace/workspaceConstants.ts +++ b/src/platform/workspace/workspaceConstants.ts @@ -3,6 +3,7 @@ export const WORKSPACE_STORAGE_KEYS = { CURRENT_WORKSPACE: 'Comfy.Workspace.Current', TOKEN: 'Comfy.Workspace.Token', EXPIRES_AT: 'Comfy.Workspace.ExpiresAt', + SURVEY_GATE_EVALUATED: 'Comfy.Workspace.SurveyGateEvaluated', // localStorage key (persists across browser sessions) LAST_WORKSPACE_ID: 'Comfy.Workspace.LastWorkspaceId' } as const diff --git a/src/router.ts b/src/router.ts index 33436b13f90..9ee31093970 100644 --- a/src/router.ts +++ b/src/router.ts @@ -1,4 +1,3 @@ -import * as Sentry from '@sentry/vue' import { until } from '@vueuse/core' import { storeToRefs } from 'pinia' import { @@ -11,6 +10,7 @@ import type { RouteLocationNormalized } from 'vue-router' import { useFeatureFlags } from '@/composables/useFeatureFlags' import { isCloud, isDesktop } from '@/platform/distribution/types' import { useTelemetry } from '@/platform/telemetry' +import { WORKSPACE_STORAGE_KEYS } from '@/platform/workspace/workspaceConstants' import { useDialogService } from '@/services/dialogService' import { useAuthStore } from '@/stores/authStore' import { useUserStore } from '@/stores/userStore' @@ -23,7 +23,26 @@ import { PRESERVED_QUERY_NAMESPACES } from '@/platform/navigation/preservedQuery // browser-tab session, don't re-evaluate it on subsequent navigations. // sessionStorage survives `window.location.reload()` in the same tab but // is fresh for new tabs — exactly the semantics we want. -const SURVEY_GATE_SESSION_KEY = 'comfy.survey_gate_evaluated_this_session' +const SURVEY_GATE_SESSION_KEY = WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED + +// sessionStorage can throw SecurityError in sandboxed iframes and some +// private-browsing contexts. Failing closed (re-evaluate the gate) is safe; +// failing open would block navigation entirely. +function readSurveyGateFlag(): boolean { + try { + return sessionStorage.getItem(SURVEY_GATE_SESSION_KEY) !== null + } catch { + return false + } +} + +function writeSurveyGateFlag(): void { + try { + sessionStorage.setItem(SURVEY_GATE_SESSION_KEY, '1') + } catch { + // Ignore — re-evaluating on the next nav is acceptable. + } +} const cloudOnboardingRoutes = isCloud ? (await import('./platform/cloud/onboarding/onboardingCloudRoutes')) @@ -227,11 +246,11 @@ if (isCloud) { if (!flags.onboardingSurveyEnabled) { return next() } - if (sessionStorage.getItem(SURVEY_GATE_SESSION_KEY)) { + if (readSurveyGateFlag()) { return next() } // Import auth functions dynamically to avoid circular dependency - const { getSurveyCompletedStatus } = + const { getSurveyCompletedStatus, SurveyAuthError } = await import('@/platform/cloud/onboarding/auth') try { // Check user's actual status @@ -242,31 +261,27 @@ if (isCloud) { // next /cloud-user-check call sees a completed survey on its own // server round-trip; this flag only suppresses re-evaluation of // the *same* request on subsequent navigations within this tab. - sessionStorage.setItem(SURVEY_GATE_SESSION_KEY, '1') + writeSurveyGateFlag() // Survey is required for all users (when feature flag enabled). // getSurveyCompletedStatus returns true for ambiguous responses // (404/5xx/network) so this branch only fires on a definitive // "user has no survey saved" signal — see auth.ts for the policy. if (!surveyCompleted) { - Sentry.addBreadcrumb({ - category: 'navigation', - message: 'survey gate → /cloud/survey', - level: 'info', - data: { - from_path: _from.fullPath, - from_name: String(_from.name ?? ''), - initial_load: _from.name === undefined - } - }) return next({ name: 'cloud-survey' }) } } catch (error) { - // Most likely an auth error from getSurveyCompletedStatus. // Don't bounce to /cloud-user-check — that re-runs the same checks // and can produce a redirect loop. Let the user proceed; the auth // layer will handle re-authentication on the next API call. - sessionStorage.setItem(SURVEY_GATE_SESSION_KEY, '1') + // + // Do NOT mark the gate as evaluated on a SurveyAuthError. Once + // re-auth succeeds, the next navigation will re-check and reach a + // definitive answer. Marking here would silently disable the gate + // for the whole tab session on a single transient 401. + if (!(error instanceof SurveyAuthError)) { + writeSurveyGateFlag() + } console.error('Failed to check survey status:', error) } } From e88a7ae9991139968b8fdb5860f1cd54d5ece585 Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Fri, 15 May 2026 22:10:14 +0000 Subject: [PATCH 3/6] [automated] Apply ESLint and Oxfmt fixes --- src/platform/cloud/onboarding/auth.test.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/platform/cloud/onboarding/auth.test.ts b/src/platform/cloud/onboarding/auth.test.ts index bbda14bbfc8..def47b3039e 100644 --- a/src/platform/cloud/onboarding/auth.test.ts +++ b/src/platform/cloud/onboarding/auth.test.ts @@ -1,9 +1,6 @@ import { beforeEach, describe, expect, test, vi } from 'vitest' -import { - SurveyAuthError, - getSurveyCompletedStatus -} from './auth' +import { SurveyAuthError, getSurveyCompletedStatus } from './auth' const fetchApi = vi.fn() @@ -116,7 +113,11 @@ describe('getSurveyCompletedStatus', () => { test('503 → true (transient backend hiccup, do not bounce)', async () => { fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 503, statusText: 'Service Unavailable' }) + mockResponse({ + ok: false, + status: 503, + statusText: 'Service Unavailable' + }) ) await expect(getSurveyCompletedStatus()).resolves.toBe(true) }) From 6915d833e68fef71b921ef6e9dbfe7a9439d2b6d Mon Sep 17 00:00:00 2001 From: Deep Mehta Date: Fri, 15 May 2026 15:15:54 -0700 Subject: [PATCH 4/6] refactor: simplify PR to the minimum policy change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the per-tab sessionStorage gate, SurveyAuthError class, logout-side cleanup, CloudSurveyView change, and workspace key — none of those are required to fix the mid-session bounce. With getSurveyCompletedStatus returning true on every ambiguous response (404 / 5xx / network), no transient error can produce a false "not completed" signal, so the existing single-evaluation router gate is correct as-is. Cumulative PR diff vs main is now: two `return false` -> `return true` in auth.ts (with comments updated), plus a new auth.test.ts covering the six resolution branches. --- src/composables/auth/useAuthActions.ts | 13 --- .../cloud/onboarding/CloudSurveyView.vue | 22 +++-- src/platform/cloud/onboarding/auth.test.ts | 98 ++----------------- src/platform/cloud/onboarding/auth.ts | 80 ++++++--------- src/platform/workspace/workspaceConstants.ts | 1 - src/router.ts | 68 ++----------- 6 files changed, 58 insertions(+), 224 deletions(-) diff --git a/src/composables/auth/useAuthActions.ts b/src/composables/auth/useAuthActions.ts index c4fa0a4ac27..d9c9c621891 100644 --- a/src/composables/auth/useAuthActions.ts +++ b/src/composables/auth/useAuthActions.ts @@ -11,7 +11,6 @@ import { useTelemetry } from '@/platform/telemetry' import { useToastStore } from '@/platform/updates/common/toastStore' import { useWorkflowService } from '@/platform/workflow/core/services/workflowService' import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore' -import { WORKSPACE_STORAGE_KEYS } from '@/platform/workspace/workspaceConstants' import { useDialogService } from '@/services/dialogService' import { useAuthStore } from '@/stores/authStore' import type { BillingPortalTargetTier } from '@/stores/authStore' @@ -82,18 +81,6 @@ export const useAuthActions = () => { } await authStore.logout() - - // Clear the survey gate so the next user signing in on the same tab - // gets a fresh evaluation. `window.location.href` below preserves - // sessionStorage, so without this an account switch silently skips the - // survey gate for the new user. - try { - sessionStorage.removeItem(WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED) - } catch { - // sessionStorage unavailable (sandboxed iframe / strict private mode); - // the tab is being navigated away anyway. - } - toastStore.add({ severity: 'success', summary: t('auth.signOut.success'), diff --git a/src/platform/cloud/onboarding/CloudSurveyView.vue b/src/platform/cloud/onboarding/CloudSurveyView.vue index b723cffd750..56b50f33d35 100644 --- a/src/platform/cloud/onboarding/CloudSurveyView.vue +++ b/src/platform/cloud/onboarding/CloudSurveyView.vue @@ -14,7 +14,10 @@ import { computed, onMounted, ref } from 'vue' import { useRouter } from 'vue-router' import { useFeatureFlags } from '@/composables/useFeatureFlags' -import { submitSurvey } from '@/platform/cloud/onboarding/auth' +import { + getSurveyCompletedStatus, + submitSurvey +} from '@/platform/cloud/onboarding/auth' import { isCloud } from '@/platform/distribution/types' import { remoteConfig } from '@/platform/remoteConfig/remoteConfig' import { useTelemetry } from '@/platform/telemetry' @@ -37,12 +40,17 @@ onMounted(async () => { await router.replace({ name: 'cloud-user-check' }) return } - // Don't re-check survey status here. The router gate and UserCheckView - // are the source of truth — re-checking would treat a transient 5xx as - // "already completed" and bounce a new user away from /cloud/survey - // during signup, the same false-positive class this PR is meant to fix. - if (isCloud) { - useTelemetry()?.trackSurvey('opened') + try { + const surveyCompleted = await getSurveyCompletedStatus() + if (surveyCompleted) { + await router.replace({ name: 'cloud-user-check' }) + return + } + if (isCloud) { + useTelemetry()?.trackSurvey('opened') + } + } catch (error) { + console.error('Failed to check survey status:', error) } }) diff --git a/src/platform/cloud/onboarding/auth.test.ts b/src/platform/cloud/onboarding/auth.test.ts index def47b3039e..21e40692b1d 100644 --- a/src/platform/cloud/onboarding/auth.test.ts +++ b/src/platform/cloud/onboarding/auth.test.ts @@ -1,6 +1,6 @@ import { beforeEach, describe, expect, test, vi } from 'vitest' -import { SurveyAuthError, getSurveyCompletedStatus } from './auth' +import { getSurveyCompletedStatus } from './auth' const fetchApi = vi.fn() @@ -10,9 +10,6 @@ vi.mock('@/scripts/api', () => ({ } })) -// `@sentry/vue` is still imported by other functions in auth.ts -// (getUserCloudStatus, submitSurvey). Mock it so test runs don't init the -// real SDK. vi.mock('@sentry/vue', () => ({ addBreadcrumb: vi.fn(), captureException: vi.fn() @@ -21,18 +18,16 @@ vi.mock('@sentry/vue', () => ({ function mockResponse({ ok, status, - statusText = '', body }: { ok: boolean status: number - statusText?: string body?: unknown }): Response { return { ok, status, - statusText, + statusText: '', json: async () => body } as unknown as Response } @@ -49,7 +44,7 @@ describe('getSurveyCompletedStatus', () => { await expect(getSurveyCompletedStatus()).resolves.toBe(true) }) - test('200 with empty object value → false', async () => { + test('200 with empty value → false (the only "not completed" signal)', async () => { fetchApi.mockResolvedValueOnce( mockResponse({ ok: true, status: 200, body: { value: {} } }) ) @@ -63,95 +58,18 @@ describe('getSurveyCompletedStatus', () => { await expect(getSurveyCompletedStatus()).resolves.toBe(false) }) - test('404 → true (treated as completed to avoid false-positive bounce)', async () => { - fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 404, statusText: 'Not Found' }) - ) + test('404 → true (do not bounce on missing key)', async () => { + fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 404 })) await expect(getSurveyCompletedStatus()).resolves.toBe(true) }) - test('401 → throws SurveyAuthError with status', async () => { - fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 401, statusText: 'Unauthorized' }) - ) - await expect(getSurveyCompletedStatus()).rejects.toBeInstanceOf( - SurveyAuthError - ) - fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 401, statusText: 'Unauthorized' }) - ) - await expect(getSurveyCompletedStatus()).rejects.toMatchObject({ - status: 401 - }) - }) - - test('403 → throws SurveyAuthError with status', async () => { - fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 403, statusText: 'Forbidden' }) - ) - await expect(getSurveyCompletedStatus()).rejects.toBeInstanceOf( - SurveyAuthError - ) - fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 403, statusText: 'Forbidden' }) - ) - await expect(getSurveyCompletedStatus()).rejects.toMatchObject({ - status: 403 - }) - }) - - test('500 → true (transient backend hiccup, do not bounce)', async () => { - fetchApi.mockResolvedValueOnce( - mockResponse({ - ok: false, - status: 500, - statusText: 'Internal Server Error' - }) - ) - await expect(getSurveyCompletedStatus()).resolves.toBe(true) - }) - - test('503 → true (transient backend hiccup, do not bounce)', async () => { - fetchApi.mockResolvedValueOnce( - mockResponse({ - ok: false, - status: 503, - statusText: 'Service Unavailable' - }) - ) + test('500 → true (do not bounce on transient backend error)', async () => { + fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 500 })) await expect(getSurveyCompletedStatus()).resolves.toBe(true) }) - test('network/fetch rejection → true (do not bounce)', async () => { + test('network rejection → true (do not bounce on network error)', async () => { fetchApi.mockRejectedValueOnce(new TypeError('Network request failed')) await expect(getSurveyCompletedStatus()).resolves.toBe(true) }) - - test('SurveyAuthError thrown by inner branch propagates through outer catch', async () => { - // Sanity check: the catch block must re-throw SurveyAuthError instead - // of swallowing it as "ambiguous" — that's the regression the typed - // error class is meant to prevent. - fetchApi.mockResolvedValueOnce( - mockResponse({ ok: false, status: 401, statusText: 'Unauthorized' }) - ) - await expect(getSurveyCompletedStatus()).rejects.toBeInstanceOf( - SurveyAuthError - ) - }) -}) - -describe('SurveyAuthError', () => { - test('is an Error instance', () => { - const err = new SurveyAuthError(401, 'Unauthorized') - expect(err).toBeInstanceOf(Error) - expect(err).toBeInstanceOf(SurveyAuthError) - }) - - test('carries status and a descriptive message', () => { - const err = new SurveyAuthError(403, 'Forbidden') - expect(err.status).toBe(403) - expect(err.message).toContain('403') - expect(err.message).toContain('Forbidden') - expect(err.name).toBe('SurveyAuthError') - }) }) diff --git a/src/platform/cloud/onboarding/auth.ts b/src/platform/cloud/onboarding/auth.ts index 54c5ea88e50..81361560010 100644 --- a/src/platform/cloud/onboarding/auth.ts +++ b/src/platform/cloud/onboarding/auth.ts @@ -10,20 +10,6 @@ interface UserCloudStatus { const ONBOARDING_SURVEY_KEY = 'onboarding_survey' -/** - * Thrown when /settings/{key} returns 401/403. Callers can branch on - * `error instanceof SurveyAuthError` instead of pattern-matching error - * messages — see router.ts and CloudSurveyView.vue for the consumers. - */ -export class SurveyAuthError extends Error { - readonly status: number - constructor(status: number, statusText: string) { - super(`Survey status auth error: ${status} ${statusText}`) - this.name = 'SurveyAuthError' - this.status = status - } -} - /** * Helper function to capture API errors with Sentry */ @@ -101,22 +87,6 @@ export async function getUserCloudStatus(): Promise { } } -/** - * Returns whether the user has completed the onboarding survey. - * - * Resolution rules (prefer false negatives over false positives — i.e. rather - * miss a survey prompt than redirect a working customer to /cloud/survey): - * - 200 with non-empty `value` → true (definitely completed) - * - 200 with empty `value` → false (definitely not completed) - * - 404 → true (key absent; could be a - * genuinely new user OR a customer whose Settings JSON pre-dates the - * survey. We can't distinguish on the wire and prefer the safer default. - * New users still get the survey via the onboarding signup flow itself.) - * - 401 / 403 → throws (auth issue, not a survey - * signal — propagate so the auth layer handles it) - * - 5xx / network error → true (backend hiccup — don't - * bounce the user on a transient blip) - */ export async function getSurveyCompletedStatus(): Promise { try { const response = await api.fetchApi(`/settings/${ONBOARDING_SURVEY_KEY}`, { @@ -125,29 +95,37 @@ export async function getSurveyCompletedStatus(): Promise { 'Content-Type': 'application/json' } }) - - if (response.ok) { - const data = await response.json() - // 200: definitive signal. Empty value = never completed. - return !isEmpty(data.value) - } - - if (response.status === 401 || response.status === 403) { - // Auth failure - propagate so the auth layer can handle. Returning - // false here would let an expired token masquerade as "no survey" and - // bounce the user to /cloud/survey on every transient 401. - throw new SurveyAuthError(response.status, response.statusText) + if (!response.ok) { + // Ambiguous response (404/5xx/etc). Treat as completed to avoid + // bouncing working customers to /cloud/survey on transient hiccups. + // Real "not completed" only comes from a 200 with empty value. + Sentry.addBreadcrumb({ + category: 'auth', + message: 'Survey status check returned non-ok response', + level: 'info', + data: { + status: response.status, + endpoint: `/settings/${ONBOARDING_SURVEY_KEY}` + } + }) + return true } - - // 404 / 5xx / other: ambiguous. Treat as completed to avoid false - // positives — rather miss a survey than bounce a paying customer. - return true + const data = await response.json() + // Check if data exists and is not empty + return !isEmpty(data.value) } catch (error) { - // Re-throw auth errors so callers can branch on them; everything else - // (network failure, parse failure, etc) is treated as "completed". - if (error instanceof SurveyAuthError) { - throw error - } + // Network/parse failure — same policy as ambiguous HTTP responses. + Sentry.captureException(error, { + tags: { + api_endpoint: '/settings/{key}', + error_type: 'network_error' + }, + extra: { + route_template: '/settings/{key}', + route_actual: `/settings/${ONBOARDING_SURVEY_KEY}` + }, + level: 'warning' + }) return true } } diff --git a/src/platform/workspace/workspaceConstants.ts b/src/platform/workspace/workspaceConstants.ts index 447b584c545..b56c1644d86 100644 --- a/src/platform/workspace/workspaceConstants.ts +++ b/src/platform/workspace/workspaceConstants.ts @@ -3,7 +3,6 @@ export const WORKSPACE_STORAGE_KEYS = { CURRENT_WORKSPACE: 'Comfy.Workspace.Current', TOKEN: 'Comfy.Workspace.Token', EXPIRES_AT: 'Comfy.Workspace.ExpiresAt', - SURVEY_GATE_EVALUATED: 'Comfy.Workspace.SurveyGateEvaluated', // localStorage key (persists across browser sessions) LAST_WORKSPACE_ID: 'Comfy.Workspace.LastWorkspaceId' } as const diff --git a/src/router.ts b/src/router.ts index 9ee31093970..7623786565a 100644 --- a/src/router.ts +++ b/src/router.ts @@ -10,7 +10,6 @@ import type { RouteLocationNormalized } from 'vue-router' import { useFeatureFlags } from '@/composables/useFeatureFlags' import { isCloud, isDesktop } from '@/platform/distribution/types' import { useTelemetry } from '@/platform/telemetry' -import { WORKSPACE_STORAGE_KEYS } from '@/platform/workspace/workspaceConstants' import { useDialogService } from '@/services/dialogService' import { useAuthStore } from '@/stores/authStore' import { useUserStore } from '@/stores/userStore' @@ -19,31 +18,6 @@ import LayoutDefault from '@/views/layouts/LayoutDefault.vue' import { installPreservedQueryTracker } from '@/platform/navigation/preservedQueryTracker' import { PRESERVED_QUERY_NAMESPACES } from '@/platform/navigation/preservedQueryNamespaces' -// Tab-scoped flag: once we've evaluated the onboarding-survey gate for a -// browser-tab session, don't re-evaluate it on subsequent navigations. -// sessionStorage survives `window.location.reload()` in the same tab but -// is fresh for new tabs — exactly the semantics we want. -const SURVEY_GATE_SESSION_KEY = WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED - -// sessionStorage can throw SecurityError in sandboxed iframes and some -// private-browsing contexts. Failing closed (re-evaluate the gate) is safe; -// failing open would block navigation entirely. -function readSurveyGateFlag(): boolean { - try { - return sessionStorage.getItem(SURVEY_GATE_SESSION_KEY) !== null - } catch { - return false - } -} - -function writeSurveyGateFlag(): void { - try { - sessionStorage.setItem(SURVEY_GATE_SESSION_KEY, '1') - } catch { - // Ignore — re-evaluating on the next nav is acceptable. - } -} - const cloudOnboardingRoutes = isCloud ? (await import('./platform/cloud/onboarding/onboardingCloudRoutes')) .cloudOnboardingRoutes @@ -233,56 +207,26 @@ if (isCloud) { } // User is logged in - check if they need onboarding (when enabled) - // For root path, check actual user status to handle waitlisted users. - // - // The survey gate is intentionally gated once per browser-tab session. - // sessionStorage persists across `window.location.reload()` in the same - // tab, so background reloads (token refresh races, GraphCanvas's 401 - // recovery, the remote-config 10-minute poll, etc.) cannot re-bounce a - // working user mid-session. A fresh tab gets a fresh check, which - // matches the intent of showing the survey to first-time and existing- - // but-never-prompted users. + // For root path, check actual user status to handle waitlisted users if (!isDesktop && isLoggedIn && to.path === '/') { if (!flags.onboardingSurveyEnabled) { return next() } - if (readSurveyGateFlag()) { - return next() - } // Import auth functions dynamically to avoid circular dependency - const { getSurveyCompletedStatus, SurveyAuthError } = + const { getSurveyCompletedStatus } = await import('@/platform/cloud/onboarding/auth') try { // Check user's actual status const surveyCompleted = await getSurveyCompletedStatus() - // Mark this session as gate-evaluated regardless of outcome. - // If we redirect to /cloud/survey, the user fills it out and the - // next /cloud-user-check call sees a completed survey on its own - // server round-trip; this flag only suppresses re-evaluation of - // the *same* request on subsequent navigations within this tab. - writeSurveyGateFlag() - - // Survey is required for all users (when feature flag enabled). - // getSurveyCompletedStatus returns true for ambiguous responses - // (404/5xx/network) so this branch only fires on a definitive - // "user has no survey saved" signal — see auth.ts for the policy. + // Survey is required for all users (when feature flag enabled) if (!surveyCompleted) { return next({ name: 'cloud-survey' }) } } catch (error) { - // Don't bounce to /cloud-user-check — that re-runs the same checks - // and can produce a redirect loop. Let the user proceed; the auth - // layer will handle re-authentication on the next API call. - // - // Do NOT mark the gate as evaluated on a SurveyAuthError. Once - // re-auth succeeds, the next navigation will re-check and reach a - // definitive answer. Marking here would silently disable the gate - // for the whole tab session on a single transient 401. - if (!(error instanceof SurveyAuthError)) { - writeSurveyGateFlag() - } - console.error('Failed to check survey status:', error) + console.error('Failed to check user status:', error) + // On error, redirect to user-check as fallback + return next({ name: 'cloud-user-check' }) } } From 9dfb501cb145a770a7536c10608e422785b8ec51 Mon Sep 17 00:00:00 2001 From: Deep Mehta Date: Fri, 15 May 2026 15:33:39 -0700 Subject: [PATCH 5/6] test(auth): lock 401/403 behavior under the ambiguous-response policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 401 and 403 are treated the same as any other non-OK response — return true (completed) — so a transient auth failure can't bounce a working user to /cloud/survey. The dedicated auth layer handles re-auth on the next API call. Add tests so the policy can't silently drift back to throwing on auth errors. --- src/platform/cloud/onboarding/auth.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/platform/cloud/onboarding/auth.test.ts b/src/platform/cloud/onboarding/auth.test.ts index 21e40692b1d..eff11d10386 100644 --- a/src/platform/cloud/onboarding/auth.test.ts +++ b/src/platform/cloud/onboarding/auth.test.ts @@ -68,6 +68,21 @@ describe('getSurveyCompletedStatus', () => { await expect(getSurveyCompletedStatus()).resolves.toBe(true) }) + // 401/403 fall under the same "ambiguous => treat as completed" policy. + // The dedicated auth layer handles re-authentication on the next API + // call; this function deliberately does not try to disambiguate auth + // failures from other non-OK responses. Locking with tests so the + // policy can't drift back to a "throw on auth error" branch. + test('401 → true (auth layer handles re-auth on next call)', async () => { + fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 401 })) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + + test('403 → true (auth layer handles re-auth on next call)', async () => { + fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 403 })) + await expect(getSurveyCompletedStatus()).resolves.toBe(true) + }) + test('network rejection → true (do not bounce on network error)', async () => { fetchApi.mockRejectedValueOnce(new TypeError('Network request failed')) await expect(getSurveyCompletedStatus()).resolves.toBe(true) From 1e9a6f56f06bb1eccd030e27880748c74aa6c2ca Mon Sep 17 00:00:00 2001 From: Deep Mehta Date: Mon, 18 May 2026 14:58:52 -0700 Subject: [PATCH 6/6] refactor(auth): address DrJKL review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bump the non-OK breadcrumb to `level: 'warning'` so both ambiguous- response paths (HTTP non-OK and network/parse failure) log at the same level. Both are the same event class — "deliberately resolved as completed" — and aligning the levels makes Sentry triage easier. - Drop the comment that restated `!isEmpty(data.value)`. - Use `fromPartial` from `@total-typescript/shoehorn` instead of the `as unknown as Response` double-cast in the test mock helper. Matches the existing repo convention (~150 call sites). --- src/platform/cloud/onboarding/auth.test.ts | 5 +++-- src/platform/cloud/onboarding/auth.ts | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/platform/cloud/onboarding/auth.test.ts b/src/platform/cloud/onboarding/auth.test.ts index eff11d10386..40d2cd1fcc8 100644 --- a/src/platform/cloud/onboarding/auth.test.ts +++ b/src/platform/cloud/onboarding/auth.test.ts @@ -1,3 +1,4 @@ +import { fromPartial } from '@total-typescript/shoehorn' import { beforeEach, describe, expect, test, vi } from 'vitest' import { getSurveyCompletedStatus } from './auth' @@ -24,12 +25,12 @@ function mockResponse({ status: number body?: unknown }): Response { - return { + return fromPartial({ ok, status, statusText: '', json: async () => body - } as unknown as Response + }) } describe('getSurveyCompletedStatus', () => { diff --git a/src/platform/cloud/onboarding/auth.ts b/src/platform/cloud/onboarding/auth.ts index 81361560010..31ef70d726f 100644 --- a/src/platform/cloud/onboarding/auth.ts +++ b/src/platform/cloud/onboarding/auth.ts @@ -102,7 +102,7 @@ export async function getSurveyCompletedStatus(): Promise { Sentry.addBreadcrumb({ category: 'auth', message: 'Survey status check returned non-ok response', - level: 'info', + level: 'warning', data: { status: response.status, endpoint: `/settings/${ONBOARDING_SURVEY_KEY}` @@ -111,7 +111,6 @@ export async function getSurveyCompletedStatus(): Promise { return true } const data = await response.json() - // Check if data exists and is not empty return !isEmpty(data.value) } catch (error) { // Network/parse failure — same policy as ambiguous HTTP responses.