fix(cloud): stop bouncing working users to /cloud/survey mid-session#12301
Conversation
## 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates getSurveyCompletedStatus() to treat non-OK HTTP responses and network errors as survey-completed (return true), adds an inline comment documenting this policy, and adds Vitest tests covering OK, empty, error-status, and network-failure paths. ChangesSurvey Completion Status Error-Handling Policy
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🎭 Playwright: ✅ 1611 passed, 0 failed · 3 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
📦 Bundle: 5.38 MB gzip 🔴 +69 BDetailsSummary
Category Glance App Entry Points — 26.1 kB (baseline 26.1 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.24 MB (baseline 1.24 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 82.9 kB (baseline 82.9 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 2 unchanged Panels & Settings — 527 kB (baseline 527 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 14 unchanged User & Accounts — 17.8 kB (baseline 17.8 kB) • 🔴 +1 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed / 2 unchanged Editors & Dialogs — 112 kB (baseline 112 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 4 added / 4 removed UI Components — 58 kB (baseline 58 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 8 unchanged Data & Services — 3.16 MB (baseline 3.16 MB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 13 added / 13 removed / 4 unchanged Utilities & Hooks — 367 kB (baseline 367 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 13 added / 13 removed / 18 unchanged Vendor & Third-Party — 9.94 MB (baseline 9.94 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 9.23 MB (baseline 9.23 MB) • ⚪ 0 BBundles that do not match a named category
Status: 57 added / 57 removed / 86 unchanged ⚡ Performance Report
All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-05-18T22:11:29.981Z",
"gitSha": "f243e390baad71731ae9b3ea142c1f1e4a36571c",
"branch": "deep/fix-survey-gate-false-positives",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2030.4360000000088,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.661,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 365.359,
"heapDeltaBytes": 24307884,
"heapUsedBytes": 73484032,
"domNodes": 22,
"jsHeapTotalBytes": 15466496,
"scriptDurationMs": 24.651000000000003,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2035.8019999999897,
"styleRecalcs": 12,
"styleRecalcDurationMs": 11.531,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 489.762,
"heapDeltaBytes": -10404768,
"heapUsedBytes": 58030948,
"domNodes": -258,
"jsHeapTotalBytes": 19877888,
"scriptDurationMs": 26.817999999999994,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1896.016999999972,
"styleRecalcs": 74,
"styleRecalcDurationMs": 43.03999999999999,
"layouts": 12,
"layoutDurationMs": 3.92,
"taskDurationMs": 879.9,
"heapDeltaBytes": 1765920,
"heapUsedBytes": 49761208,
"domNodes": -261,
"jsHeapTotalBytes": 14544896,
"scriptDurationMs": 144.59199999999998,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2074.3780000000243,
"styleRecalcs": 81,
"styleRecalcDurationMs": 48.12000000000001,
"layouts": 12,
"layoutDurationMs": 3.9190000000000005,
"taskDurationMs": 1098.8829999999998,
"heapDeltaBytes": -13760780,
"heapUsedBytes": 54610256,
"domNodes": -260,
"jsHeapTotalBytes": 25382912,
"scriptDurationMs": 142.892,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1750.9769999999776,
"styleRecalcs": 32,
"styleRecalcDurationMs": 20.258,
"layouts": 6,
"layoutDurationMs": 0.7370000000000002,
"taskDurationMs": 351.126,
"heapDeltaBytes": 522600,
"heapUsedBytes": 49132184,
"domNodes": 79,
"jsHeapTotalBytes": 14942208,
"scriptDurationMs": 25.275,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1716.3560000000189,
"styleRecalcs": 30,
"styleRecalcDurationMs": 17.226,
"layouts": 6,
"layoutDurationMs": 0.5409999999999998,
"taskDurationMs": 325.519,
"heapDeltaBytes": 23069072,
"heapUsedBytes": 73319736,
"domNodes": 75,
"jsHeapTotalBytes": 24117248,
"scriptDurationMs": 19.196,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "dom-widget-clipping",
"durationMs": 589.5329999999603,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.2810000000000015,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 360.27299999999997,
"heapDeltaBytes": 9182620,
"heapUsedBytes": 57678104,
"domNodes": 12,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 62.114,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 546.5020000000322,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.21,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 340.25300000000004,
"heapDeltaBytes": -9279764,
"heapUsedBytes": 58738724,
"domNodes": 20,
"jsHeapTotalBytes": 19136512,
"scriptDurationMs": 62.586000000000006,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2032.9940000000306,
"styleRecalcs": 8,
"styleRecalcDurationMs": 9.489999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 589.062,
"heapDeltaBytes": -2002540,
"heapUsedBytes": 57634088,
"domNodes": -263,
"jsHeapTotalBytes": 5324800,
"scriptDurationMs": 102.04199999999999,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2067.877000000067,
"styleRecalcs": 10,
"styleRecalcDurationMs": 10.965,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 722.3459999999999,
"heapDeltaBytes": 45359348,
"heapUsedBytes": 102603068,
"domNodes": -258,
"jsHeapTotalBytes": 33579008,
"scriptDurationMs": 123.232,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2166.9899999999984,
"styleRecalcs": 69,
"styleRecalcDurationMs": 19.998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1267.1650000000002,
"heapDeltaBytes": -5880296,
"heapUsedBytes": 53989136,
"domNodes": -263,
"jsHeapTotalBytes": 6316032,
"scriptDurationMs": 455.897,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2169.563000000039,
"styleRecalcs": 69,
"styleRecalcDurationMs": 20.442,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1265.987,
"heapDeltaBytes": 50023284,
"heapUsedBytes": 109613260,
"domNodes": -268,
"jsHeapTotalBytes": 40599552,
"scriptDurationMs": 427.954,
"eventListeners": -159,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3246.8110000000365,
"styleRecalcs": 64,
"styleRecalcDurationMs": 18.687999999999995,
"layouts": 60,
"layoutDurationMs": 7.755999999999999,
"taskDurationMs": 1464.48,
"heapDeltaBytes": 45559164,
"heapUsedBytes": 106830112,
"domNodes": -269,
"jsHeapTotalBytes": 43016192,
"scriptDurationMs": 511.861,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3136.12999999998,
"styleRecalcs": 66,
"styleRecalcDurationMs": 18.922000000000004,
"layouts": 60,
"layoutDurationMs": 7.622,
"taskDurationMs": 1393.577,
"heapDeltaBytes": 7892560,
"heapUsedBytes": 69017684,
"domNodes": -265,
"jsHeapTotalBytes": 6316032,
"scriptDurationMs": 503.7010000000001,
"eventListeners": -125,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2079.7580000000266,
"styleRecalcs": 7,
"styleRecalcDurationMs": 8.41,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 740.0179999999998,
"heapDeltaBytes": 39660060,
"heapUsedBytes": 100854988,
"domNodes": -263,
"jsHeapTotalBytes": 32792576,
"scriptDurationMs": 116.727,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "minimap-idle",
"durationMs": 2016.845000000103,
"styleRecalcs": 7,
"styleRecalcDurationMs": 7.889999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 549.969,
"heapDeltaBytes": 10966732,
"heapUsedBytes": 71770784,
"domNodes": -265,
"jsHeapTotalBytes": 286720,
"scriptDurationMs": 96.78499999999998,
"eventListeners": -129,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 579.2059999999992,
"styleRecalcs": 47,
"styleRecalcDurationMs": 12.328000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 391.166,
"heapDeltaBytes": -9599252,
"heapUsedBytes": 59044988,
"domNodes": 20,
"jsHeapTotalBytes": 19398656,
"scriptDurationMs": 128.963,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.663333333333338,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 604.5950000000175,
"styleRecalcs": 48,
"styleRecalcDurationMs": 12.622000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 421.783,
"heapDeltaBytes": -13233588,
"heapUsedBytes": 55242244,
"domNodes": -237,
"jsHeapTotalBytes": 4411392,
"scriptDurationMs": 124.64800000000001,
"eventListeners": -127,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 1990.1249999999777,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.49,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 381.856,
"heapDeltaBytes": 1553688,
"heapUsedBytes": 67940628,
"domNodes": 22,
"jsHeapTotalBytes": 19488768,
"scriptDurationMs": 21.679,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-idle",
"durationMs": 1989.3680000000131,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.293999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 320.639,
"heapDeltaBytes": 23044944,
"heapUsedBytes": 71968796,
"domNodes": 18,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 11.347000000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1967.7760000000148,
"styleRecalcs": 85,
"styleRecalcDurationMs": 50.615,
"layouts": 16,
"layoutDurationMs": 5.3740000000000006,
"taskDurationMs": 950.216,
"heapDeltaBytes": 14495620,
"heapUsedBytes": 62931956,
"domNodes": 74,
"jsHeapTotalBytes": 15204352,
"scriptDurationMs": 110.03500000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1750.9129999999686,
"styleRecalcs": 77,
"styleRecalcDurationMs": 37.648999999999994,
"layouts": 16,
"layoutDurationMs": 4.5120000000000005,
"taskDurationMs": 770.8170000000001,
"heapDeltaBytes": 13196296,
"heapUsedBytes": 62458480,
"domNodes": -261,
"jsHeapTotalBytes": 19263488,
"scriptDurationMs": 102.87,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 932.7190000000201,
"styleRecalcs": 15,
"styleRecalcDurationMs": 26.528999999999996,
"layouts": 4,
"layoutDurationMs": 12.851999999999999,
"taskDurationMs": 752.6780000000002,
"heapDeltaBytes": 30310952,
"heapUsedBytes": 95723852,
"domNodes": 13513,
"jsHeapTotalBytes": 16777216,
"scriptDurationMs": 27.90100000000001,
"eventListeners": 2529,
"totalBlockingTimeMs": 160,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "viewport-pan-sweep",
"durationMs": 8174.472999999978,
"styleRecalcs": 249,
"styleRecalcDurationMs": 53.114,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3962.893,
"heapDeltaBytes": 13658208,
"heapUsedBytes": 71772864,
"domNodes": -264,
"jsHeapTotalBytes": 6782976,
"scriptDurationMs": 1286.689,
"eventListeners": -113,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8143.6089999999695,
"styleRecalcs": 251,
"styleRecalcDurationMs": 54.855000000000004,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3847.9780000000005,
"heapDeltaBytes": 21094088,
"heapUsedBytes": 84355128,
"domNodes": 20,
"jsHeapTotalBytes": 22077440,
"scriptDurationMs": 1322.906,
"eventListeners": 20,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "vue-large-graph-idle",
"durationMs": 12975.528999999995,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 12960.427,
"heapDeltaBytes": -36283292,
"heapUsedBytes": 170512384,
"domNodes": -8331,
"jsHeapTotalBytes": 21032960,
"scriptDurationMs": 626.986,
"eventListeners": -16464,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 15751.892999999996,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 15712.486000000003,
"heapDeltaBytes": 140982468,
"heapUsedBytes": 380855328,
"domNodes": -3306,
"jsHeapTotalBytes": 154374144,
"scriptDurationMs": 709.021,
"eventListeners": -16480,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 15115.257000000041,
"styleRecalcs": 71,
"styleRecalcDurationMs": 19.517000000000007,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 15091.710000000003,
"heapDeltaBytes": -25753736,
"heapUsedBytes": 169424116,
"domNodes": -8331,
"jsHeapTotalBytes": -1511424,
"scriptDurationMs": 896.7829999999999,
"eventListeners": -16460,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.333333333333332,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 14525.679999999966,
"styleRecalcs": 65,
"styleRecalcDurationMs": 17.533999999999995,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14495.566,
"heapDeltaBytes": -40934152,
"heapUsedBytes": 162153872,
"domNodes": -8331,
"jsHeapTotalBytes": 22605824,
"scriptDurationMs": 883.931,
"eventListeners": -16464,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 445.1930000000175,
"styleRecalcs": 17,
"styleRecalcDurationMs": 23.011999999999997,
"layouts": 5,
"layoutDurationMs": 1.44,
"taskDurationMs": 123.88800000000002,
"heapDeltaBytes": 5420368,
"heapUsedBytes": 57051204,
"domNodes": 168,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 28.178,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "workflow-execution",
"durationMs": 462.9520000000866,
"styleRecalcs": 17,
"styleRecalcDurationMs": 24.491000000000003,
"layouts": 4,
"layoutDurationMs": 1.139,
"taskDurationMs": 126.64600000000002,
"heapDeltaBytes": 5072956,
"heapUsedBytes": 56289964,
"domNodes": 157,
"jsHeapTotalBytes": 524288,
"scriptDurationMs": 24.985,
"eventListeners": 69,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.669999999999998,
"p95FrameDurationMs": 16.700000000000273
}
]
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/platform/cloud/onboarding/auth.ts (1)
144-166: ⚡ Quick winConsider extracting the error prefix to a constant to avoid silent breakage.
The error detection at line 149 relies on a string prefix that must match line 126 exactly. If someone edits one without the other, auth errors will be swallowed instead of re-thrown.
♻️ Proposed refactor
+const SURVEY_AUTH_ERROR_PREFIX = 'Survey status auth error:' + export async function getSurveyCompletedStatus(): Promise<boolean> { try { ... if (response.status === 401 || response.status === 403) { throw new Error( - `Survey status auth error: ${response.status} ${response.statusText}` + `${SURVEY_AUTH_ERROR_PREFIX} ${response.status} ${response.statusText}` ) } ... } catch (error) { if ( error instanceof Error && - error.message.startsWith('Survey status auth error:') + error.message.startsWith(SURVEY_AUTH_ERROR_PREFIX) ) { throw error } ... } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/cloud/onboarding/auth.ts` around lines 144 - 166, The string literal used to detect auth errors in the catch block (error.message.startsWith('Survey status auth error:')) is brittle; extract that prefix into a shared constant (e.g., SURVEY_STATUS_AUTH_ERROR_PREFIX) declared alongside the function that throws the error so both the throw site and the catch site reference the same symbol, then replace the literal in the catch with that constant and update the throw to use it as well to prevent silent breakage when the message changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/cloud/onboarding/auth.ts`:
- Around line 144-166: The string literal used to detect auth errors in the
catch block (error.message.startsWith('Survey status auth error:')) is brittle;
extract that prefix into a shared constant (e.g.,
SURVEY_STATUS_AUTH_ERROR_PREFIX) declared alongside the function that throws the
error so both the throw site and the catch site reference the same symbol, then
replace the literal in the catch with that constant and update the throw to use
it as well to prevent silent breakage when the message changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 213245c1-f1c0-4f37-b06b-46418ba8f819
📒 Files selected for processing (2)
src/platform/cloud/onboarding/auth.tssrc/router.ts
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #12301 +/- ##
===========================================
- Coverage 74.10% 59.69% -14.41%
===========================================
Files 1522 1412 -110
Lines 88026 72107 -15919
Branches 24660 20010 -4650
===========================================
- Hits 65230 43045 -22185
- Misses 21972 28589 +6617
+ Partials 824 473 -351
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1015 files with indirect coverage changes 🚀 New features to boost your workflow:
|
PR Review (cloud-leaning eyes)Overall AssessmentApprove — pure frontend change with no auth/middleware/API-contract drift. A couple of cloud-side observations worth filing as follow-ups, plus one nuance in the README trade-off table that's tighter than the actual backend contract. 1. Cloud backend contract — the 404 trade-off is narrower than the PR description suggestsThe PR description frames the 404 case as covering both "key absent for existing user" and "user has no settings yet" / brand-new user. Looking at the live handler ( // User row not found:
if database.IsNotFound(err) {
return ingest.GetSettingByKey401JSONResponse{Code: "UNAUTHORIZED", ...}
}
// DB error:
if err != nil {
return ingest.GetSettingByKey401JSONResponse{Code: "SERVER_ERROR", ...}
}
// Key not present in Settings JSON, and no default registered:
return ingest.GetSettingByKey404JSONResponse{Code: "NOT_FOUND", ...}So 404 only ever fires when the User row exists but the 2. The 401 path is wider than expected —
|
Review: fix(cloud): stop bouncing working users to /cloud/survey mid-sessionThe logic in Must Fix
// 200 non-empty → true
mockFetch({ ok: true, status: 200, json: () => ({ value: { q1: 'a' } }) })
expect(await getSurveyCompletedStatus()).toBe(true)
// 200 empty value → false
mockFetch({ ok: true, status: 200, json: () => ({ value: {} }) })
expect(await getSurveyCompletedStatus()).toBe(false)
// 404 → true (not a redirect)
mockFetch({ ok: false, status: 404 })
expect(await getSurveyCompletedStatus()).toBe(true)
// 5xx → true (not a redirect)
mockFetch({ ok: false, status: 503 })
expect(await getSurveyCompletedStatus()).toBe(true)
// 401 → throws (not swallowed as true)
mockFetch({ ok: false, status: 401 })
await expect(getSurveyCompletedStatus()).rejects.toThrow()
// network error → true
mockFetch(() => { throw new Error('Failed to fetch') })
expect(await getSurveyCompletedStatus()).toBe(true)The 401-throws path is the most dangerous one to ship untested — if the string-prefix catch in the
const SURVEY_AUTH_ERROR_PREFIX = 'Survey status auth error:'Use it at both the throw and the startsWith check. This is load-bearing for correctness (CodeRabbit flagged it too) and would be caught immediately by the 401 test above. Note on sessionStorage gateThe The fix itself is the right call. Ship it once |
|
Drive-by from a cloud lens. The auth/billing/contract surface is fine — no subscription/credits/usage touchpoints, the 5xx-as-completed tradeoff is documented and reasonable, and propagating 401/403 instead of masquerading it as ''no survey'' is strictly better. One real concern: sessionStorage is not cleared on logout, so the gate skip leaks across users in the same tab. Two ways out:
The second is more robust to logout paths we forget about. Smaller note: |
|
Drive-by review from cloud side — focused on data correctness / null safety / guard edges. The mid-session bounce fix is the right call; flagging a few edges that look load-bearing. Must Fix
Should Fix
Notes
|
Review: fix(cloud): stop bouncing working users to /cloud/survey mid-sessionOverall the policy change is sound and the trade-off is documented well. A few correctness/cleanliness issues before this can come out of draft. Must Fix
Should Fix
Already Exists
Nits
VerdictDirectionally correct and the documented trade-off is reasonable. Holding on the magic-string error matching, the unmocked behavior change in the two other callers, and the logout-cleanup gap before this is ready to merge. Tests for the five-branch resolution matrix are non-negotiable given this gates paying customers. |
Review: fix(cloud): stop bouncing working users to /cloud/survey mid-sessionPulling on this carefully because it touches the auth/onboarding hot path and the semantic of Issues (must fix)
Suggestions (non-blocking)
Nits
Test Coverage Assessment
VerdictDon't merge as-is. The auth.ts policy is sound, but the policy change wasn't applied consistently to the other two callers of |
Review: fix(cloud): stop bouncing working users to /cloud/survey mid-sessionThe intent here is correct and well-motivated. The 7,000 daily false-positive bounces are a real problem, the response-code triage table is the right mental model, and the sessionStorage gate addresses the background-reload trigger properly. A few issues worth resolving before merge, one of which is a user-facing correctness bug. Race Conditions[MUST FIX] The flag is written, then [MUST FIX] This is a concrete user-visible bug. Browser API Edge Cases[MUST FIX] The State Management in Callers[SHOULD FIX]
[SHOULD FIX] The Telemetry Correctness[INFORMATIONAL] The ordering is: evaluate, set flag, emit breadcrumb, call [INFORMATIONAL] The catch block uses Good
Summary of blockers before merge:
|
- 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/router.ts (1)
273-286: Add structured telemetry for fail-open survey checks.At Line 285, this path only logs to
console.error. Consider emitting a telemetry event/counter (non-PII) when the guard bypasses on error so you can monitor how often survey gating is skipped in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/router.ts` around lines 273 - 286, In the catch block that currently calls writeSurveyGateFlag() and console.error (the block referencing SurveyAuthError and writeSurveyGateFlag), emit a non-PII telemetry event or increment a counter whenever you execute the fail-open path (i.e., when error is not an instance of SurveyAuthError and you call writeSurveyGateFlag); implement this by calling the existing telemetry/metrics API in the codebase (e.g., telemetry.increment or telemetry.trackEvent) with a clear metric name like "survey_gate_bypassed" and include only non-identifying context (error type/category, environment, and boolean flags) before calling writeSurveyGateFlag and logging the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/composables/auth/useAuthActions.ts`:
- Around line 85-95: Wrap the call to authStore.logout() in a try/finally so
sessionStorage.removeItem(WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED) always
runs even if logout throws; move the existing sessionStorage removal into the
finally block (preserving the existing catch for storage access) and keep the
subsequent navigation via window.location.href unchanged so the survey-gate
cleanup executes on both success and failure of authStore.logout().
---
Nitpick comments:
In `@src/router.ts`:
- Around line 273-286: In the catch block that currently calls
writeSurveyGateFlag() and console.error (the block referencing SurveyAuthError
and writeSurveyGateFlag), emit a non-PII telemetry event or increment a counter
whenever you execute the fail-open path (i.e., when error is not an instance of
SurveyAuthError and you call writeSurveyGateFlag); implement this by calling the
existing telemetry/metrics API in the codebase (e.g., telemetry.increment or
telemetry.trackEvent) with a clear metric name like "survey_gate_bypassed" and
include only non-identifying context (error type/category, environment, and
boolean flags) before calling writeSurveyGateFlag and logging the error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8220c17d-b640-4dcf-b820-b2a11eaa75b7
📒 Files selected for processing (6)
src/composables/auth/useAuthActions.tssrc/platform/cloud/onboarding/CloudSurveyView.vuesrc/platform/cloud/onboarding/auth.test.tssrc/platform/cloud/onboarding/auth.tssrc/platform/workspace/workspaceConstants.tssrc/router.ts
✅ Files skipped from review due to trivial changes (1)
- src/platform/workspace/workspaceConstants.ts
|
|
||
| // 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. | ||
| } |
There was a problem hiding this comment.
Ensure survey-gate session cleanup runs even if logout throws.
If authStore.logout() fails, Lines 90-95 are never reached, so the stale gate key can survive in the same tab. Wrap logout with try/finally so removal always executes.
Suggested fix
- 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.
- }
+ try {
+ await authStore.logout()
+ } finally {
+ try {
+ sessionStorage.removeItem(WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED)
+ } catch {
+ // sessionStorage unavailable (sandboxed iframe / strict private mode)
+ }
+ }As per coding guidelines: "Implement proper error handling in all code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/composables/auth/useAuthActions.ts` around lines 85 - 95, Wrap the call
to authStore.logout() in a try/finally so
sessionStorage.removeItem(WORKSPACE_STORAGE_KEYS.SURVEY_GATE_EVALUATED) always
runs even if logout throws; move the existing sessionStorage removal into the
finally block (preserving the existing catch for storage access) and keep the
subsequent navigation via window.location.href unchanged so the survey-gate
cleanup executes on both success and failure of authStore.logout().
There was a problem hiding this comment.
Stale — useAuthActions.ts was reverted to origin/main in the simplify commit (6915d83). The PR no longer touches this file; gate-cleanup-on-logout was dropped along with the rest of the per-tab gate logic.
There was a problem hiding this comment.
🧩 Analysis chain
Thanks for the heads-up, @deepme987. Acknowledged — since useAuthActions.ts was reverted to origin/main and the per-tab gate logic was dropped entirely, this comment is no longer relevant. I'll resolve it now.
🏁 Script executed:
#!/bin/bash
# List unresolved review comments by coderabbitai on PR `#12301` to find the one to resolve
gh api graphql -f query='
{
repository(owner: "Comfy-Org", name: "ComfyUI_frontend") {
pullRequest(number: 12301) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}'
Length of output: 76
Got it, @deepme987 — the comment is stale and no longer applicable since the per-tab gate logic (including the SURVEY_GATE_EVALUATED sessionStorage cleanup on logout) was dropped in the simplify commit. I wasn't able to resolve the thread programmatically due to a missing auth token in the sandbox, but you or a maintainer can dismiss it directly in the GitHub UI.
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8248
File: src/platform/assets/schemas/assetSchema.ts:109-118
Timestamp: 2026-01-22T23:44:59.888Z
Learning: Do not export declarations (e.g., schemas, constants, types) unless they are actually used elsewhere in the codebase. Keep the public API surface minimal by exporting only items with concrete usage. This reduces coupling and keeps imports explicit. In src/platform/assets/schemas/assetSchema.ts, ensure only necessary exports are exposed; remove unused exports or switch to non-exported/internal where appropriate.
Learnt from: JemiloII
Repo: Comfy-Org/ComfyUI_frontend PR: 8282
File: src/composables/usePaste.test.ts:372-383
Timestamp: 2026-01-23T21:06:15.240Z
Learning: In paste/clipboard handling code, do not rely on ephemeral DataTransfer data across async boundaries. If an operation awaits async tasks (such as node creation) during a paste, clone the DataTransfer (e.g., using cloneDataTransfer) to persist files and string data before the paste event completes. This ensures clipboard contents remain available for subsequent processing.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8722
File: src/composables/maskeditor/useBrushDrawing.ts:5-5
Timestamp: 2026-02-08T03:19:12.009Z
Learning: Treat unresolved import warnings caused by pnpm workspace catalog mappings (pnpm-workspace.yaml) and unplugin-typegpu in vite.config.mts as false positives. Do not flag these in PR reviews; they are expected in sandboxes without node_modules and are not indicative of actual missing dependencies in the TS codebase.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/load3d.ts:427-430
Timestamp: 2026-02-19T02:06:23.468Z
Learning: In TypeScript, you can use typeof SomeClass in type annotation positions (e.g., param: typeof LGraphNode) even when SomeClass is imported with import type. This is a type query that only exists at compile time and is erased in runtime, so it is safe to combine with type-only imports. Apply this pattern to TS files broadly when you need a type that references the shape of a class or constructor function without importing the value at runtime.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/uploadAudio.ts:91-94
Timestamp: 2026-02-19T02:06:38.395Z
Learning: In TypeScript files, you can use a type annotation like 'nodeType: typeof MyClass' even if MyClass is imported via 'import type'. Both the type-only import and 'typeof' operate at the type level and are erased at compile time. This pattern is commonly used for constructor types (e.g., 'nodeType: typeof LGraphNode'). Apply this pattern across TypeScript files in the repository (src/**/*.ts) as appropriate, ensuring the imported symbol is a type-only import when possible for clarity and to avoid runtime imports.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8992
File: src/lib/litegraph/src/widgets/GradientSliderWidget.ts:18-18
Timestamp: 2026-02-20T21:08:19.814Z
Learning: When drawing with CanvasRenderingContext2D in TypeScript/JavaScript, wrap the drawing logic with ctx.save() at the start and ctx.restore() at the end to preserve and restore the canvas state. Do not manually destructure and restore individual properties (e.g., fillStyle, strokeStyle); rely on save/restore to manage state changes in a scoped manner. This should be applied to all TS files that perform canvas drawing.
Learnt from: dante01yoon
Repo: Comfy-Org/ComfyUI_frontend PR: 9075
File: src/scripts/api.featureFlags.test.ts:237-268
Timestamp: 2026-02-22T04:27:33.379Z
Learning: In Vite/Vitest, import.meta.env.DEV is true for any mode that is not 'production' (i.e., DEV is the opposite of PROD, and can be true in 'test', 'development', etc.). Do not assume DEV implies only 'development' mode. When reviewing code and tests, treat DEV as a non-production flag and verify environment-specific logic accordingly. Reference: https://vite.dev/guide/env-and-mode#modes
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 9427
File: src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuFilter.vue:33-33
Timestamp: 2026-03-06T00:53:28.835Z
Learning: When reviewing code, note that the enforce-canonical-classes (better-tailwindcss) rule may auto-fix Tailwind v3 !class-name syntax by converting it to v4 class-name! syntax. Do not treat these auto-fixed class-name! instances as newly introduced issues; the perceived change is in syntax placement, not in usage or intent. This guidance applies across all .vue and .ts files in the repository.
Learnt from: sonnybox
Repo: Comfy-Org/ComfyUI_frontend PR: 9446
File: src/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vue:45-45
Timestamp: 2026-03-06T01:55:00.013Z
Learning: Treat wrap-break-word as a valid Tailwind CSS utility for overflow-wrap: break-word in Tailwind v4+ projects. Do not flag this class as invalid in any Vue (.vue) or TypeScript (.ts/.tsx) files within the repository (e.g., Comfy-Org/ComfyUI_frontend) or other Tailwind v4+ projects. When reviewing, verify that the class is used to enable word breaking in long text content and reference the Tailwind docs: https://tailwindcss.com/docs/overflow-wrap.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 11420
File: src/composables/useCanvasScheduler.ts:20-20
Timestamp: 2026-04-19T22:12:36.981Z
Learning: In the ComfyUI frontend Vue/TypeScript codebase, when implementing singleton composables inside `src/composables/**`, prefer `createSharedComposable` from VueUse over module-scoped `let` variables. Module-scoped `let` can cause SSR cross-request state leakage, unpredictable HMR behavior, and requires manual lifecycle/cleanup management for watchers/computeds/effects. `createSharedComposable` ties the singleton to Vue lifecycle: it disposes watchers/computeds and effect-scope reactivity automatically when the last consumer unmounts, improving testability. Only use the module-scoped `let` pattern if the singleton must intentionally outlive all Vue component trees or if there is a clearly documented reason why `createSharedComposable` is inferior for that specific composable.
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 11531
File: src/composables/maskeditor/brushDrawingUtils.ts:14-14
Timestamp: 2026-04-22T04:06:01.353Z
Learning: In the Comfy-Org/ComfyUI_frontend codebase, `knip` is used to detect unused exports. When reviewing TypeScript/TSX code, avoid recommending or introducing exported types/functions/constants that have no concrete external consumers (e.g., they’re only used within the same module). Only recommend exporting when there is an actual external usage outside the module. If something might be needed externally later, keep it non-exported for now and defer exporting until an external consumer is added.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 12197
File: src/renderer/extensions/linearMode/AppInput.vue:23-23
Timestamp: 2026-05-14T21:10:45.382Z
Learning: In the ComfyUI_frontend repo, `WidgetEntityId` is a branded template-literal type (e.g., `${UUID}:${NodeId}:${string}`) and should be treated as structurally non-empty when valid—so an empty string is invalid and must be treated as “absent”. When handling optional `entityId?: WidgetEntityId` values (e.g., for Vue props or TS function parameters), guard missing/invalid IDs with a truthy check (e.g., `if (!entityId) return`) rather than `if (entityId === undefined)`. Rationale: `=== undefined` would allow empty-string values through into stores such as `appModeStore.selectedInputs`, creating rows that can’t resolve back to a widget.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/cloud/onboarding/auth.ts (1)
98-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate auth failures instead of failing open on 401/403.
This now returns
truefor all non-OK responses and all caught errors, so 401/403 are treated as “completed” instead of surfacing auth problems. That breaks the intended contract and can permanently bypass the survey gate after auth blips.Use explicit branching for auth statuses and rethrow in
catch(or use a typed sentinel error) so only ambiguous failures fail-open.Suggested fix
export async function getSurveyCompletedStatus(): Promise<boolean> { try { const response = await api.fetchApi(`/settings/${ONBOARDING_SURVEY_KEY}`, { method: 'GET', headers: { 'Content-Type': 'application/json' } }) if (!response.ok) { + if (response.status === 401 || response.status === 403) { + throw new Error(`Survey auth failure: ${response.status}`) + } 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 } const data = await response.json() return !isEmpty(data.value) } catch (error) { + if ( + error instanceof Error && + error.message.startsWith('Survey auth failure:') + ) { + 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}` }, level: 'warning' }) return true } }Also applies to: 116-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/cloud/onboarding/auth.ts` around lines 98 - 112, The current logic treats any non-OK HTTP response (and all caught errors) as "completed" which masks auth failures; change the non-ok handling so you explicitly check response.status and propagate auth errors: if response.status is 401 or 403, throw or rethrow an authentication error (do not return true), otherwise continue to add the Sentry breadcrumb and return true for ambiguous statuses; also update the corresponding catch block so it rethrows authentication-related errors (or a typed sentinel) while only swallowing/returning true for non-auth transient errors—use the same symbols shown (response.status, ONBOARDING_SURVEY_KEY, Sentry.addBreadcrumb) to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/cloud/onboarding/auth.test.ts`:
- Around line 40-74: Add two tests to src/platform/cloud/onboarding/auth.test.ts
that assert getSurveyCompletedStatus rejects when the backend returns 401 or
403: mimic the existing style using
fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 401 })) and
fetchApi.mockResolvedValueOnce(mockResponse({ ok: false, status: 403 })), and
use await expect(getSurveyCompletedStatus()).rejects.toThrow() (or similar
rejects matcher) to lock the auth-failure contract; reuse the mockResponse
helper and the same test structure as the other cases so the suite remains
consistent.
---
Outside diff comments:
In `@src/platform/cloud/onboarding/auth.ts`:
- Around line 98-112: The current logic treats any non-OK HTTP response (and all
caught errors) as "completed" which masks auth failures; change the non-ok
handling so you explicitly check response.status and propagate auth errors: if
response.status is 401 or 403, throw or rethrow an authentication error (do not
return true), otherwise continue to add the Sentry breadcrumb and return true
for ambiguous statuses; also update the corresponding catch block so it rethrows
authentication-related errors (or a typed sentinel) while only
swallowing/returning true for non-auth transient errors—use the same symbols
shown (response.status, ONBOARDING_SURVEY_KEY, Sentry.addBreadcrumb) to locate
the code to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29a9d844-2610-4ada-933f-08930b086782
📒 Files selected for processing (2)
src/platform/cloud/onboarding/auth.test.tssrc/platform/cloud/onboarding/auth.ts
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.
dosubot
Bot
added
the
size:S
DrJKL
added
needs-backport
DrJKL
left a comment
DrJKL
left a comment
There was a problem hiding this comment.
🙌 LGTM — thank you for protecting working users from the survey hammer
I need to start with full disclosure: I am professionally tired of being yanked out of a flow state to rate something, fill in another two-question onboarding survey, or confirm that yes, I still want to use the product I am actively using. Every single one of these is a paper cut, and at scale (the ~7k users/day in your Datadog screenshot) it is not a paper cut — it is a daily, recurring betrayal of the user's attention. So thank you, @deepme987, for fixing this. Sincerely.
What I like, beyond the user impact:
- Discipline in scoping. The simplification commit (
6915d833 refactor: simplify PR to the minimum policy change) is the kind of commit I wish I saw more often. You built a more elaborate fix — SurveyAuthError, sessionStorage gate, workspace key plumbing, CloudSurveyView edits — realized that tworeturn false→return trueplus comments and tests covers the whole failure mode, and threw the rest away. The PR is now ~10 lines of meaningful change for an outage-class fix. That is a real-deal YAGNI win. - The policy-locking tests are the right shape. You did not write change-detector tests; you wrote one test per resolution branch and added an explanatory comment specifically locking the 401/403 →
truedecision so a future refactor cannot quietly drift back to a throw branch. That comment is doing real work. - Comments that explain why. The replacement for the
// Not an error casecomment now tells the next reader the user-facing reason for the choice ("bouncing working customers to /cloud/survey on transient hiccups"). This is the standard the AGENTS.md guidance asks for. - The trade-off is named, not hidden. The PR description explicitly calls out the new-user-on-404 false negative and frames it against product's stated preference. That is the right level of honesty for a behavior change in a flow that touches paying customers.
Approving. One blocker-adjacent ask on the PR description (left as an inline note on the diff so it's adjacent to the code it describes), plus three minor polish comments inline that you can take or leave. None of them block the merge from my side — I would rather this ship and protect users today than block on stylistic notes.
Thank you again. This is a great fix.
| } | ||
| }) | ||
| return false | ||
| return true |
There was a problem hiding this comment.
The PR body's resolution table still says:
| 401 / 403 |
false→ bounced | throws (auth issue, not survey signal) |
…but the final commit (9dfb501c test(auth): lock 401/403 behavior under the ambiguous-response policy) and the new tests at L71–80 explicitly lock 401/403 → true (not throws). Every non-OK response now flows through this single return true; the function has no auth-error branch at all.
This is the only thing I would call out as actually important — reviewers / approvers / future spelunkers will read the PR body and sign off on a different contract than the one being shipped. Please update the table to show 401/403 → true and drop the "throws" row. (Bonus: the simplification story gets even stronger when the table reflects what actually shipped.)
There was a problem hiding this comment.
Updated the PR description — dropped the throws row, all non-OK / network now show true. Thanks for catching it.
| }) | ||
| if (!response.ok) { | ||
| // Not an error case - survey not completed is a valid state | ||
| // Ambiguous response (404/5xx/etc). Treat as completed to avoid |
There was a problem hiding this comment.
👍 Nit-positive: this comment is doing what the AGENTS.md guidance asks for — it explains the user-facing why (transient hiccups bouncing working customers), not the what. Future reader will know why this looks like a swallow and won't "helpfully" revert it. Keep this style.
| return true | ||
| } | ||
| const data = await response.json() | ||
| // Check if data exists and is not empty |
There was a problem hiding this comment.
Minor: this comment restates the code (!isEmpty(data.value)). Project guidance prefers dropping redundant comments. Non-blocking.
| // Check if data exists and is not empty | |
| return !isEmpty(data.value) |
| @@ -124,7 +126,7 @@ export async function getSurveyCompletedStatus(): Promise<boolean> { | |||
| }, | |||
| level: 'warning' | |||
There was a problem hiding this comment.
Minor consistency nit: the non-OK HTTP branch above breadcrumbs at level: 'info', but this catch branch logs at level: 'warning'. Under the new policy both are the same event class — "ambiguous response, deliberately resolved as completed." Picking one (probably 'warning' for both, so the suppression is visible in Sentry filters) would make incident triage easier next time someone digs through the breadcrumbs. Non-blocking.
There was a problem hiding this comment.
Bumped the breadcrumb to warning in 1e9a6f5 so both ambiguous-response paths log at the same level.
| // 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) | ||
| }) | ||
|
|
There was a problem hiding this comment.
👍 Nit-positive: this is the right way to write a regression test. The 401/403 cases carry an explanatory comment that pins the policy decision to a reason — auth layer handles re-auth on the next API call — so a future refactor that re-introduces a throw branch will trip a red test with a comment explaining why the test is red. Great use of test-as-spec.
| function mockResponse({ | ||
| ok, | ||
| status, | ||
| body | ||
| }: { | ||
| ok: boolean | ||
| status: number | ||
| body?: unknown | ||
| }): Response { | ||
| return { | ||
| ok, | ||
| status, | ||
| statusText: '', | ||
| json: async () => body |
There was a problem hiding this comment.
Minor: body?: unknown + json: async () => body + as unknown as Response is the kind of double-cast the project's type-safety guidance gently warns against. Acceptable for a test scaffold and I would not block on it, but a typed Partial<Response> helper (or body: { value: unknown } since that's all the SUT reads) would keep the test more honest about the shape it depends on. Non-blocking.
There was a problem hiding this comment.
Correction on myself — this codebase has @total-typescript/shoehorn already in dev deps and ~70 test files use it for exactly this. fromPartial<Response>(...) keeps the same partial-shape style without the as unknown as Response double cast (add import { fromPartial } from '@total-typescript/shoehorn' at the top):
| function mockResponse({ | |
| ok, | |
| status, | |
| body | |
| }: { | |
| ok: boolean | |
| status: number | |
| body?: unknown | |
| }): Response { | |
| return { | |
| ok, | |
| status, | |
| statusText: '', | |
| json: async () => body | |
| function mockResponse({ | |
| ok, | |
| status, | |
| body | |
| }: { | |
| ok: boolean | |
| status: number | |
| body?: unknown | |
| }): Response { | |
| return fromPartial<Response>({ | |
| ok, | |
| status, | |
| statusText: '', | |
| json: async () => body | |
| }) | |
| } |
Still non-blocking, just pointing at the local convention. Sorry for the noise.
There was a problem hiding this comment.
Switched to fromPartial in 1e9a6f5. Thanks for the pointer to the local convention.
- 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<Response>` 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).
|
@deepme987 Successfully backported to #12320 |
github-actions
Bot
removed
the
needs-backport
…ud/survey mid-session (#12320) Backport of #12301 to `cloud/1.44` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-12320-backport-cloud-1-44-fix-cloud-stop-bouncing-working-users-to-cloud-survey-mid-sessi-3646d73d3650815185cecaa2babdf3d1) by [Unito](https://www.unito.io) Co-authored-by: Deep Mehta <42841935+deepme987@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
5 participants
Summary
getSurveyCompletedStatus(auth.ts) now resolves ambiguous responses to "completed" instead of "not completed", so transient backend errors no longer bounce working users to/cloud/survey.valuetrue(completed)true(completed)valuefalse(not completed)false(not completed)false→ bouncedtrue(treat as completed)false→ bouncedtrue(treat as completed)false→ bouncedtrue(treat as completed)false→ bouncedtrue(treat as completed)Only a definitive
200with emptyvalueis treated as "not completed". Everything else fails open. The dedicated auth layer handles re-authentication on the next API call, so 401/403 doesn't need a separate branch here.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 hitting the
setting key onboarding_survey not foundpath on prod ingest. Withonboarding_survey_enabled: truein prod dynamic config and the catch-all!response.okreturningfalse, any mid-session reload tripped a redirect to/cloud/survey.User-validated requirement: rather miss showing the survey to a few users than show it duplicately or interrupt working customers.
Trade-off worth product review
A genuinely brand-new user whose
User.SettingsJSON is empty also returns 404 from/api/settings/onboarding_survey— the backend doesn't distinguish "key absent for existing user" from "user has no settings yet". With this change, that 404 is treated as "completed", so the survey gate does not fire on the strict 404 path. New users will still see the survey if signup pre-populates theonboarding_surveykey with an empty object (200with emptyvalue); if not, the survey is missed on initial signup.We picked this trade-off per the product call that false positives (bouncing paying customers) are strictly worse than false negatives (occasionally missing a new user).
The clean fix to recover the new-user signal is a backend change: return
200withvalue: nullwhen theUserrow exists but the key is absent — distinguishing "no survey saved" from "user not found". Out of scope for this PR; filing as follow-up if accepted.Test plan
/cloud/survey(gate still works for new sessions)/api/settings/onboarding_survey(DevTools blocking) — user stays on current page, no redirectUnit coverage in
auth.test.tslocks the resolution table above against drift (one test per branch, 8 total).Companion PRs
None — frontend only.
┆Issue is synchronized with this Notion page by Unito