fix: stop Canvas Navigation mode resetting to Custom on reload - #14716
Conversation
|
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:
📝 WalkthroughWalkthroughCanvas navigation presets are centralized. The setting store migrates missing navigation overrides and updates values before asynchronous change handlers run. Unit and browser tests cover preset persistence, legacy settings, custom modes, migration, and notification order. ChangesCanvas navigation settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to This change preserves the selected canvas navigation mode across reloads and keeps its dependent controls aligned. A bounded type-safety concern remains around preset lookup and default resolution, warranting owner follow-up but not blocking merge. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BrowserTest
participant SettingStore
participant CanvasNavigationMode
participant CanvasSettings
BrowserTest->>SettingStore: select navigation mode
SettingStore->>CanvasNavigationMode: invoke onChange
CanvasNavigationMode->>CanvasSettings: apply matching preset
CanvasSettings-->>SettingStore: persist navigation settings
SettingStore-->>BrowserTest: retain selected mode
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
🎭 Playwright: ✅ 1812 passed, 0 failed · 3 flaky📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 8.85 MB gzip 🔴 +1.44 kBDetailsSummary
Category Glance App Entry Points — 3.71 kB (baseline 3.71 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.37 MB (baseline 1.37 MB) • 🟢 -227 BGraph editor runtime, canvas, workflow orchestration
Status: 2 added / 2 removed / 1 unchanged Views & Navigation — 124 kB (baseline 124 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 13 added / 13 removed / 4 unchanged Panels & Settings — 565 kB (baseline 565 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 16 unchanged User & Accounts — 27.7 kB (baseline 27.7 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed / 5 unchanged Editors & Dialogs — 125 kB (baseline 125 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 7 added / 7 removed / 1 unchanged UI Components — 67.1 kB (baseline 67.1 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed / 8 unchanged Data & Services — 3.52 MB (baseline 3.52 MB) • 🔴 +3.52 kBStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 550 kB (baseline 550 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 18 added / 18 removed / 20 unchanged Vendor & Third-Party — 16.8 MB (baseline 16.8 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 18 unchanged Other — 14.2 MB (baseline 14.2 MB) • 🔴 +1 BBundles that do not match a named category
Status: 68 added / 68 removed / 218 unchanged ⚡ Performance Report
Show regressions
All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-08-14T02:17:36.374Z",
"gitSha": "787b67db593ec3df63bff09fd124be72638969f4",
"branch": "glary/fix-canvas-navigation-mode-reset",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2008.5180000000094,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.882999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 469.65099999999995,
"heapDeltaBytes": 5151708,
"heapUsedBytes": 69706156,
"domNodes": 20,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 7.2940000000000005,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2028.1719999999837,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.821999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 467.00699999999995,
"heapDeltaBytes": 5136308,
"heapUsedBytes": 69834344,
"domNodes": 20,
"jsHeapTotalBytes": 24903680,
"scriptDurationMs": 7.771999999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2219.251000000014,
"styleRecalcs": 79,
"styleRecalcDurationMs": 51.772,
"layouts": 12,
"layoutDurationMs": 6.976000000000001,
"taskDurationMs": 1154.3400000000001,
"heapDeltaBytes": -14662280,
"heapUsedBytes": 49810124,
"domNodes": -288,
"jsHeapTotalBytes": 23040000,
"scriptDurationMs": 123.616,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1867.2220000000266,
"styleRecalcs": 77,
"styleRecalcDurationMs": 40.42900000000001,
"layouts": 12,
"layoutDurationMs": 3.9259999999999997,
"taskDurationMs": 892.244,
"heapDeltaBytes": -663240,
"heapUsedBytes": 63738048,
"domNodes": 59,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 110.196,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1707.6789999999846,
"styleRecalcs": 30,
"styleRecalcDurationMs": 17.291,
"layouts": 6,
"layoutDurationMs": 0.565,
"taskDurationMs": 393.896,
"heapDeltaBytes": 8103792,
"heapUsedBytes": 72761208,
"domNodes": 77,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 9.772,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1754.122999999936,
"styleRecalcs": 31,
"styleRecalcDurationMs": 18.852,
"layouts": 6,
"layoutDurationMs": 0.7910000000000003,
"taskDurationMs": 384.16900000000004,
"heapDeltaBytes": 8019784,
"heapUsedBytes": 72487644,
"domNodes": 78,
"jsHeapTotalBytes": 24903680,
"scriptDurationMs": 10.56,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 571.0020000000213,
"styleRecalcs": 11,
"styleRecalcDurationMs": 6.926999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 362.772,
"heapDeltaBytes": -11388988,
"heapUsedBytes": 53161056,
"domNodes": 18,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 51.216,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 583.1269999999904,
"styleRecalcs": 11,
"styleRecalcDurationMs": 8.009999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 375.81,
"heapDeltaBytes": -11137140,
"heapUsedBytes": 53272956,
"domNodes": 18,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 52.532999999999994,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-idle",
"durationMs": 2040.5890000000113,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.641000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 607.157,
"heapDeltaBytes": 12680724,
"heapUsedBytes": 72685832,
"domNodes": -282,
"jsHeapTotalBytes": 3244032,
"scriptDurationMs": 14.316999999999997,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-idle",
"durationMs": 2041.532000000018,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.169999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 611.39,
"heapDeltaBytes": 13208232,
"heapUsedBytes": 73499804,
"domNodes": -283,
"jsHeapTotalBytes": 3244032,
"scriptDurationMs": 12.686000000000003,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2163.1419999999935,
"styleRecalcs": 68,
"styleRecalcDurationMs": 13.405000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1198.185,
"heapDeltaBytes": 1448104,
"heapUsedBytes": 63328004,
"domNodes": -284,
"jsHeapTotalBytes": 3960832,
"scriptDurationMs": 319.256,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "large-graph-pan",
"durationMs": 2131.7850000000362,
"styleRecalcs": 68,
"styleRecalcDurationMs": 13.566000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1221.671,
"heapDeltaBytes": 1126988,
"heapUsedBytes": 62426492,
"domNodes": -286,
"jsHeapTotalBytes": 3960832,
"scriptDurationMs": 314.83700000000005,
"eventListeners": -179,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3130.3210000000377,
"styleRecalcs": 66,
"styleRecalcDurationMs": 14.516000000000002,
"layouts": 60,
"layoutDurationMs": 7.533,
"taskDurationMs": 1300.286,
"heapDeltaBytes": 16880156,
"heapUsedBytes": 78995116,
"domNodes": 14,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 353.03,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3199.2010000000164,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.254,
"layouts": 60,
"layoutDurationMs": 7.8740000000000006,
"taskDurationMs": 1395.7410000000002,
"heapDeltaBytes": -3951008,
"heapUsedBytes": 58342920,
"domNodes": -287,
"jsHeapTotalBytes": 3768320,
"scriptDurationMs": 365.769,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2031.829000000016,
"styleRecalcs": 7,
"styleRecalcDurationMs": 5.711000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 608.081,
"heapDeltaBytes": 12723912,
"heapUsedBytes": 73635780,
"domNodes": -283,
"jsHeapTotalBytes": 2981888,
"scriptDurationMs": 14.411,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2009.439000000043,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.613999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 614.771,
"heapDeltaBytes": 12697104,
"heapUsedBytes": 73472464,
"domNodes": -283,
"jsHeapTotalBytes": 3244032,
"scriptDurationMs": 14.227,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 606.9239999999922,
"styleRecalcs": 47,
"styleRecalcDurationMs": 10.615000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 409.6190000000001,
"heapDeltaBytes": -10546300,
"heapUsedBytes": 54263392,
"domNodes": 20,
"jsHeapTotalBytes": 25427968,
"scriptDurationMs": 118.24200000000002,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 599.072000000092,
"styleRecalcs": 48,
"styleRecalcDurationMs": 11.075999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 404.29099999999994,
"heapDeltaBytes": -10295744,
"heapUsedBytes": 54196952,
"domNodes": 22,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 117.163,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666682,
"p95FrameDurationMs": 16.700000000000273
},
{
"name": "subgraph-idle",
"durationMs": 2006.4050000000293,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.728000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 454.133,
"heapDeltaBytes": 5279736,
"heapUsedBytes": 69986516,
"domNodes": 18,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 6.929000000000001,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-idle",
"durationMs": 1992.400000000032,
"styleRecalcs": 10,
"styleRecalcDurationMs": 8.174000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 463.01599999999996,
"heapDeltaBytes": 4953160,
"heapUsedBytes": 69836880,
"domNodes": 20,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 7.631999999999998,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1710.5770000000007,
"styleRecalcs": 75,
"styleRecalcDurationMs": 38.096000000000004,
"layouts": 16,
"layoutDurationMs": 4.732,
"taskDurationMs": 830.4730000000001,
"heapDeltaBytes": -3822836,
"heapUsedBytes": 60789048,
"domNodes": 61,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 84.029,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1727.5070000000028,
"styleRecalcs": 77,
"styleRecalcDurationMs": 38.175999999999995,
"layouts": 16,
"layoutDurationMs": 4.791,
"taskDurationMs": 798.3439999999999,
"heapDeltaBytes": -4008284,
"heapUsedBytes": 60624396,
"domNodes": 63,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 80.11,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-transition-enter",
"durationMs": 1399.9520000000416,
"styleRecalcs": 19,
"styleRecalcDurationMs": 27.89699999999999,
"layouts": 14,
"layoutDurationMs": 11.356000000000002,
"taskDurationMs": 926.9419999999999,
"heapDeltaBytes": -9781728,
"heapUsedBytes": 76901448,
"domNodes": 13673,
"jsHeapTotalBytes": 14680064,
"scriptDurationMs": 17.479000000000003,
"eventListeners": 2375,
"totalBlockingTimeMs": 134,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8194.165000000055,
"styleRecalcs": 251,
"styleRecalcDurationMs": 37.795,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4149.669,
"heapDeltaBytes": 15979696,
"heapUsedBytes": 76340708,
"domNodes": -282,
"jsHeapTotalBytes": 3960832,
"scriptDurationMs": 961.098,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8206.309000000034,
"styleRecalcs": 249,
"styleRecalcDurationMs": 37.677,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4331.975,
"heapDeltaBytes": 13135824,
"heapUsedBytes": 73674336,
"domNodes": -284,
"jsHeapTotalBytes": 3960832,
"scriptDurationMs": 998.5530000000001,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 16103.492000000017,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14980.948,
"heapDeltaBytes": -30470340,
"heapUsedBytes": 167012836,
"domNodes": -8312,
"jsHeapTotalBytes": -10424320,
"scriptDurationMs": 105.21799999999999,
"eventListeners": -16391,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-idle",
"durationMs": 15888.735999999994,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 14824.452000000001,
"heapDeltaBytes": -43981308,
"heapUsedBytes": 168097008,
"domNodes": -8312,
"jsHeapTotalBytes": -9641984,
"scriptDurationMs": 92.86500000000001,
"eventListeners": -16389,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 18890.048999999977,
"styleRecalcs": 164,
"styleRecalcDurationMs": 18.33700000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 18376.653000000002,
"heapDeltaBytes": -27692788,
"heapUsedBytes": 184026896,
"domNodes": -8312,
"jsHeapTotalBytes": -11546624,
"scriptDurationMs": 397.656,
"eventListeners": -16385,
"totalBlockingTimeMs": 5,
"frameDurationMs": 17.780000000000047,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 18248.175999999945,
"styleRecalcs": 158,
"styleRecalcDurationMs": 18.228000000000023,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17881.362,
"heapDeltaBytes": -28091244,
"heapUsedBytes": 182659632,
"domNodes": -8316,
"jsHeapTotalBytes": -9449472,
"scriptDurationMs": 378.428,
"eventListeners": -16385,
"totalBlockingTimeMs": 60,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 475.45800000000327,
"styleRecalcs": 15,
"styleRecalcDurationMs": 21.720000000000002,
"layouts": 4,
"layoutDurationMs": 1.356,
"taskDurationMs": 121.13199999999999,
"heapDeltaBytes": 4998544,
"heapUsedBytes": 68645780,
"domNodes": 124,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 7.735,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 461.5280000000439,
"styleRecalcs": 14,
"styleRecalcDurationMs": 18.623,
"layouts": 3,
"layoutDurationMs": 0.76,
"taskDurationMs": 109.185,
"heapDeltaBytes": 4930248,
"heapUsedBytes": 68645620,
"domNodes": 125,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 6.791000000000002,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66666666666665,
"p95FrameDurationMs": 16.800000000000182
}
]
} |
|
Thanks for the review. No changes needed — nothing actionable was raised, so this is unchanged from the approved state. For anyone picking this up: the two seat values are inert. Nothing outside the generated types reads Follow-up filed as FE-1507 so this cannot merge red again: |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #14716 +/- ##
==========================================
- Coverage 82.11% 78.90% -3.22%
==========================================
Files 1882 1883 +1
Lines 120655 114379 -6276
Branches 36514 33471 -3043
==========================================
- Hits 99079 90249 -8830
- Misses 21096 23626 +2530
- Partials 480 504 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 521 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Navigation Mode is stored independently of the Left Mouse Click Behavior and Mouse Wheel Scroll overrides it implies, and the three were kept in sync by cross-writes in onChange. Two paths lost the mode the user picked: - applySettingLocally fired onChange before committing the new value, so the overrides written by a preset read back the mode it replaced, decided they no longer matched it, and demoted it to 'custom'. Commit the value first so the cascade observes the mode it is applying. - A mode stored before the overrides shipped in 1.27.4 is the only value on record, so the overrides loaded as their defaults, which describe 'legacy'. The override handlers then rewrote any other stored mode to 'custom' on the first load after upgrading. Treat a stored preset as authoritative instead: on registration it now supplies the overrides that were never stored. The second point is why fixing only the write order is not enough, and why the override handlers are not simply muted during registration - that would keep the dropdown reading Standard while the canvas still panned and zoomed like Legacy.
|
The duplicate-key finding is correct, but deduping is the wrong fix — this PR is now redundant and should be closed.
I verified on current The breakage this PR existed to repair is gone, fixed upstream. Deduping would leave a PR whose only content is changes Recommend: close this without merging. I've rebased the dependent fix (#14716) directly onto One correction to my earlier comment on this PR: I said nothing outside the generated types reads |
6a40902 to
b1861ab
Compare
3a4bfb6 to
7a82ca3
Compare
|
Rebased onto Context for the change of base: this was originally stacked on #14714, which existed to repair Re-verified on the new base (
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Thanks — no changes needed, nothing actionable was raised, so the branch is unchanged from the approved state ( State for the human reviewers:
Left as a draft pending human review. The one judgement call worth a reviewer's eye is in |
dosubot
Bot
added
the
size:M
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
| ([id]) => !settingStore.exists(id as keyof Settings) | ||
| ) | ||
| ) | ||
| if (Object.keys(unset).length) await settingStore.setMany(unset) |
There was a problem hiding this comment.
This migration belongs in load(), not in an onChange registration replay.
settingStore.ts already has exactly this pattern — migrateZoomThresholdToFontSize(), invoked from load() before any setting is registered:
settingValues.value = await retry(() => api.getSettings(), {...})
await migrateZoomThresholdToFontSize()and load() even asserts that nothing is registered yet. Its shape is identical to what this branch does: "old key is stored, new key is not → derive the new one and persist it."
Doing it here instead buys three problems that the load() hook does not have:
-
Un-awaited network write with no error handling.
onChange()insettingStore.tscallssetting.onChange(newValue, oldValue)and discards the returned promise. Soawait settingStore.setMany(unset)→api.storeSettings→fetchApiruns detached. If the backend is unreachable, read-only, or 4xx-on-write, this is an unhandled promise rejection fired during app bootstrap, on every load, for exactly the affected profiles. Before this PR the registration replay returned atif (!oldValue) returnbefore touching the network, soaddSettingnever made a request. -
Silent dependence on
CORE_SETTINGSarray order. This only works becauseComfy.Canvas.NavigationModeis registered beforeLeftMouseClickBehaviorandMouseWheelScroll. Reorder the array (alphabetise, move the canvas block, register an override from an extension earlier) and the demotion comes straight back: the override replays first, readsNavigationMode=standardagainst its own defaultpanning, and writescustom. Nothing in the file states this invariant. -
settingsByIdis not yet populated for the keys being written.setMany→applySettingLocally('Comfy.Canvas.LeftMouseClickBehavior', ...)runs whilesettingsById.value[key]is stillundefined, soonChange(undefined, ...)no-ops andsettingChangedEventreturnsundefined. It happens to produce the right result, but it means these writes skipdispatchChange(extensions never see them) and skip telemetry, unlike every other settings write. That is load-bearing accidental behaviour.
Hoisting this into a migrateCanvasNavigationOverrides() called from load() fixes all three: it is awaited, it runs before registration so order is irrelevant, and onChange goes back to meaning "the user changed something" (if (!oldValue) return). It also becomes unit-testable without Playwright.
There was a problem hiding this comment.
Agreed on all three, and fixed in a5b9500 — the repair now lives in migrateCanvasNavigationOverrides(), called from load() right after migrateZoomThresholdToFontSize().
You were right that all three problems were real, and (1) and (3) I had not spotted:
- Detached write. Confirmed —
onChange()discarded the promise, so the healingPOST /settingsran unsupervised on every load for exactly the affected profiles. Fromload()it is awaited inside theuseAsyncStateblock, so a failure surfaces throughsettingsError, whichGraphCanvasalready handles. - Order dependence. This was load-bearing and undocumented, which is the part I am least comfortable having shipped. Running before registration makes it structurally irrelevant rather than relying on
CORE_SETTINGSordering. settingsByIdunpopulated. I had noticed theonChangeno-op and reasoned "harmless because we want no reconciliation" — I missed that it also silently skippeddispatchChangeand telemetry. Accidental correctness, as you say. The migration now writessettingValuesdirectly and callsapi.storeSettingsonce, which is honest about being a migration rather than pretending to be a normal settings write.
NavigationMode.onChange is back to if (!old) return plus the cascade, so it means "the user changed something" again. The preset pairs moved to constants/canvasNavigation.ts so the migration and the handler share one definition without a settingStore ↔ coreSettings cycle.
| const oldValue = old as string | undefined | ||
| if (!oldValue) return | ||
| const preset = CANVAS_NAVIGATION_PRESETS[val as string] | ||
| if (!preset) return |
There was a problem hiding this comment.
Profiles that were already bitten do not self-heal — the PR description says otherwise.
CANVAS_NAVIGATION_PRESETS has no custom entry, so if (!preset) return short-circuits for anyone whose stored mode is already custom.
But that is precisely the state the bug leaves people in. Cause 2 in the description writes custom to the server (settingStore.set('Comfy.Canvas.NavigationMode', 'custom') in LeftMouseClickBehavior.onChange → api.storeSetting). So every user who has already loaded a 1.27.4+ build now has NavigationMode: "custom" persisted and the two overrides still absent — the demotion writes only the mode, never the overrides. After this PR those profiles load with mode custom, overrides falling back to panning/zoom, dropdown reading Custom, and useCanvasInteractions' isStandardNavMode still false. Nothing recovers them.
That is the population that filed FE-1503. The original choice is genuinely unrecoverable — the demotion overwrote the only record of it — so I am not asking for a fix here, but:
- the claim "affected profiles self-heal on next load" holds only for profiles that have not yet loaded a 1.27.4+ build, which is a shrinking set. Please correct it in the description and in whatever goes into the release notes / the Linear issue, because as written it reads as "the reported bug is repaired for the reporters," and it is not — they have to re-pick the mode by hand (which will now stick).
- worth adding the
{ 'Comfy.Canvas.NavigationMode': 'custom' }+ no-overrides profile to the test matrix, to pin that it is a deliberate no-op rather than an untested gap.
There was a problem hiding this comment.
Do we need to get the profiles some DEET?
There was a problem hiding this comment.
Ha — taking that as a real question: do the already-bitten profiles get repaired, or do they stay itchy?
Partly, and I overstated it in the PR description. I probed every persisted shape against the real store and CORE_SETTINGS:
| Persisted before load | After load | Repaired? |
|---|---|---|
{nav: standard} — pre-1.27.4 |
standard + select/panning |
✅ yes, this PR |
{nav: custom, left: select, wheel: panning} |
custom + select/panning |
❌ no — label still lies |
{nav: custom} |
custom + panning/zoom |
❌ no — choice is gone |
{nav: custom, left: select, wheel: zoom} — real Custom |
untouched | ✅ correctly left alone |
So: this PR stops the bite, and it heals anyone who hadn't been bitten yet (row 1 — a preset stored before 1.27.4, which is the population that gets corrupted on their next load). It does not heal anyone already corrupted. Row 2 keeps reading Custom even though its overrides are exactly the Standard pair, which is precisely the state in the original report's screenshot. Row 3 is worse and unrecoverable — there is no record left that they ever chose Standard.
Recovery for rows 2 and 3 is one manual re-pick of the mode. That does work cleanly post-fix — a single write, since the overrides already match:
E: after user re-picks Standard
writes: [["Comfy.Canvas.NavigationMode","standard"]]
effective: {"nav":"standard","left":"select","wheel":"panning"}
I'd correct the PR description's "self-heals affected profiles on next load" to "self-heals profiles that had not yet been corrupted."
Could we DEET row 2 automatically? Yes — normalise custom to the matching preset at load when the two overrides exactly equal a preset pair. I deliberately didn't, because row 2 is byte-identical to someone who deliberately picked Custom and happened to set select+panning; there's no way to tell them apart. And it isn't a no-op: useCanvasInteractions gates on NavigationMode === 'standard' exactly, so flipping the label would switch wheel forwarding on for those users. That's a behaviour change for real Custom users, which felt like it deserved its own PR and its own decision rather than riding along in a bugfix. Row 4 is the case that has to keep working, and it does.
Happy to add that normalisation here if you'd rather have the retroactive fix — it's small, and it's arguably the right semantics given custom is really an output state (the dropdown offering it as an input is itself a bit odd). Just say the word. The longer-term version is the follow-up in the description: derive NavigationMode from the two overrides instead of storing a third key, which makes rows 2 and 3 impossible rather than repairable.
There was a problem hiding this comment.
You are right, and I had reached the same conclusion independently just before your review landed — a reviewer asked whether the profiles needed "some DEET", so I probed every persisted shape against the real store. Results agreed with your reading exactly:
| Persisted before load | After load | Repaired? |
|---|---|---|
{nav: standard} — pre-1.27.4 |
standard + select/panning |
✅ |
{nav: custom, left: select, wheel: panning} |
custom + select/panning |
❌ label still lies |
{nav: custom} |
custom + panning/zoom |
❌ choice is gone |
{nav: custom, left: select, wheel: zoom} — real Custom |
untouched | ✅ correctly |
So the description's claim is wrong as written and I have corrected it: it holds only for profiles that have not yet loaded a 1.27.4+ build. Your framing that this is a shrinking set is the important part — it is worse than "some users are unaffected", because the reporters of FE-1503 are by definition in the already-demoted group. They have to re-pick by hand, which does now stick.
Both requests done:
- Test added.
already demoted to customseeds{nav: custom}with the overrides absent and asserts it stayscustom, so the no-op is pinned as deliberate rather than looking like an untested gap. I also added the mixed-pair profile, which is the case that proves the no-op is not just laziness. - Correcting the claim beyond this PR. I cannot edit the description with my tooling, so I am taking the correction to the team for the description, the release notes and the FE-1503 comment thread — agreed that "the reported bug is repaired for the reporters" is the wrong thing to let stand anywhere, since it is exactly what a triager would read and close on.
Agreed on not fixing it here. Auto-repairing row 2 would mean inferring the mode from a matching override pair, and row 2 is byte-identical to someone who deliberately picked Custom and happened to set select+panning; it is not cosmetic either, since useCanvasInteractions gates on NavigationMode === 'standard' exactly, so it would flip wheel forwarding on for real Custom users. Row 3 is unrecoverable regardless. The follow-up in the description — deriving the mode from the two overrides instead of storing a third key — makes rows 2 and 3 unreachable rather than repairable, which is the actual fix.
There was a problem hiding this comment.
Follow-up to my earlier answer: the repair has moved since, and it now heals slightly more than it did.
@benjcooley's review pointed out that doing it from the onChange registration replay was the wrong hook — detached un-awaited write, silent dependence on CORE_SETTINGS ordering, and writes that skipped dispatchChange/telemetry. It now runs as migrateCanvasNavigationOverrides() from load(), next to the existing zoom-threshold migration.
The DEET table is unchanged in substance — already-demoted profiles still do not self-heal, and I have corrected that claim in the description thread — but the repair is now awaited and order-independent, and the previously-untested cases are pinned:
{nav: custom}+ no overrides → stayscustom(deliberate no-op, now covered){nav: standard, wheel: zoom}→ fillsleft: select, keepszoom, demotes tocustom, becauseselect+zoomis genuinely not a preset. That one failed when I first wrote it — my expectation was wrong, not the code.
Six E2E tests plus migration unit coverage, all green.
| // Registration replay. A preset stored before the overrides shipped in | ||
| // 1.27.4 is the only record of the choice, so it has to supply the ones | ||
| // still missing instead of being overruled by their defaults. | ||
| if (!settingStore.exists('Comfy.Canvas.NavigationMode')) return |
There was a problem hiding this comment.
Keying on exists() rather than the effective value re-arms the bug the moment the default flips.
This guard means "only heal when the mode was explicitly stored." That is safe today only because the effective default of NavigationMode (defaultValue: 'legacy', defaultsByInstallVersion: { '1.25.0': 'legacy' }) happens to agree with the two override defaults (panning / zoom = the legacy pair).
Change either side of that coincidence — ship defaultsByInstallVersion: { '1.28.0': 'standard' }, or flip defaultValue to 'standard' (an obvious near-term product move given the option is literally labelled "Standard (New)") — and a fresh profile has effective mode standard with override defaults describing legacy. This branch returns early because nothing is stored, then LeftMouseClickBehavior's replay sees panning vs standard, mismatches, and writes NavigationMode = "custom". Every new user, first load, no interaction: the exact bug this PR is fixing.
Deriving from the effective value (settingStore.get) instead of the stored one would close this permanently and simplify the branch — a fresh profile would just materialise the overrides implied by its default mode. If you would rather keep the exists() scoping, please at minimum add a comment stating the invariant (override defaults must describe the default NavigationMode) and a unit test that asserts CANVAS_NAVIGATION_PRESETS[defaultNavigationMode] equals the two overrides' defaultValues, so the next person to touch the default trips a test rather than shipping this to every new install.
There was a problem hiding this comment.
Good catch — this was a latent trap and I have taken the test option.
The exists() guard is gone entirely now that the repair runs from load(), but your underlying point survives the move: the migration reads the stored mode, so a fresh profile with no stored mode still resolves its effective mode from defaultValue / defaultsByInstallVersion while the overrides resolve from theirs. Flip either side and a brand-new install lands on a mode no preset matches, and the override replay demotes it — every new user, first load, exactly as you describe.
Rather than leave that as a comment, constants/canvasNavigation.test.ts now pins it:
it('agrees with the default Navigation Mode', () => {
const defaultMode = settingById(NAV)?.defaultValue as string
expect(CANVAS_NAVIGATION_PRESETS[defaultMode]).toEqual(overrideDefaults())
})
it('agrees with every install-versioned Navigation Mode default', () => { ... })It reads the values out of CORE_SETTINGS rather than hardcoding them, so it asserts the relationship and not the current defaults — shipping defaultsByInstallVersion: { '1.28.0': 'standard' } fails the second test instead of shipping the bug. I covered the versioned map too since that is the likelier vehicle for the change.
I did consider deriving from settingStore.get as you suggested, which would make a fresh profile materialise its default mode's overrides and close it permanently. I went with the invariant test instead because deriving means writing settings for every new install on first load purely to restate defaults, and exists() is used elsewhere to mean "the user set this manually" — I would rather not blur that. Happy to switch if you would prefer the structural fix over the guard rail.
| onChange(settingsById.value[key], newValue, oldValue) | ||
| const typedNewValue = newValue as Settings[K] | ||
| settingValues.value[key] = typedNewValue | ||
| onChange(settingsById.value[key], newValue, oldValue) |
There was a problem hiding this comment.
The reorder itself is right, and the new unit test pins it well. Two things worth recording while this line is being touched:
1. The remaining two writes are still concurrent, so the fix is probabilistic, not deterministic.
onChange() (line 40-53) calls setting.onChange(newValue, oldValue) without awaiting it. So when the user picks a preset:
set()→applySettingLocally→onChange→NavigationMode.onChange→setMany(preset)→POST /settingsis issued and the promise is dropped on the floor;- control returns to
set(), which immediately issuesPOST /settings/Comfy.Canvas.NavigationMode.
Both are in flight against a backend that does a non-atomic read-modify-write of one JSON file with an await request.json() between the read and the write — as the description itself notes. So "leaving one write per key" is accurate about count but the two are racing. If the mode write wins the read, the overrides are lost (harmless — the registration branch re-supplies them next load). If the overrides write wins, the mode is lost, and on the next load LeftMouseClickBehavior replays against the stale mode and demotes it to custom: the original symptom, at lower probability.
Awaiting the handler here would serialise them and make the fix deterministic without depending on the backend follow-up:
await setting.onChange(newValue, oldValue)Notably, this PR is what makes awaiting safe. Under the old ordering, a handler that wrote back to the same key would recurse forever, because newValue === oldValue compared against a value that had not been committed yet. Committing first makes that guard actually guard. Worth calling out as a second benefit of the change — and worth doing, since the whole PR hinges on the cascade landing intact.
2. Rollback semantics changed. If a synchronous onChange handler throws, the value is now already in settingValues but api.storeSetting is never reached, so local and server diverge until the next load. Previously neither the commit nor the write happened. Low severity — every current handler is either async (rejection is swallowed anyway) or non-throwing — but it is a real change to the contract and the new docblock does not mention it.
There was a problem hiding this comment.
This is the best catch in the review and I have taken it — onChange is now awaited.
You are right that "one write per key" described the count while leaving them racing, and that the losing interleaving reproduces the original symptom. Implemented as:
const handled = setting?.onChange?.(newValue, oldValue)
if (setting) app.ui.settings.dispatchChange(setting.id, newValue, oldValue)
await handledStarted before dispatchChange and awaited after it, so extension listeners still fire at the same point in the sequence — awaiting inline first would have delayed every listener behind the handler, which is a change I did not want to smuggle in alongside this. applySettingLocally becomes async and set/setMany await it; addSetting keeps it detached via void, which is correct now that the registration replay no longer writes anything.
Your observation that this PR is what makes awaiting safe is the part I had not connected, and it is now in the commit message. Under the old ordering a handler writing back to its own key compared newValue against an uncommitted oldValue, so the guard did not guard and awaiting would have recursed. Committing first is the precondition.
Pinned with a unit test so the ordering cannot silently regress:
expect(order).toEqual(['onChange', 'storeSetting'])
On rollback semantics: fair, and I had reached the same conclusion from the other direction — dispatchChange goes through EventTarget.dispatchEvent, which never propagates listener exceptions to the caller, and every current handler is async or non-throwing, so there is no reachable trigger. Worth noting setMany could already leave earlier keys locally mutated on a sync throw before this PR, so the contract was never all-or-nothing. I have not added rollback for a path with no trigger, but the divergence is now called out in the docblock rather than left implicit.
| import { LinkReleaseTriggerAction } from '@/types/searchBoxTypes' | ||
| import { breakpointsTailwind } from '@vueuse/core' | ||
|
|
||
| const CANVAS_NAVIGATION_PRESETS: Record<string, Partial<Settings>> = { |
There was a problem hiding this comment.
Two type nits, both of which cost the map the safety it looks like it has.
Record<string, Partial<Settings>> makes the runtime guard invisible to TS. With strict: true but no noUncheckedIndexedAccess, CANVAS_NAVIGATION_PRESETS[val as string] is typed Partial<Settings> — never undefined. So if (!preset) return on line 204, which is load-bearing (it is the entire handling of custom and of any unknown value), reads to the compiler as dead code. Prefer:
const CANVAS_NAVIGATION_PRESETS: Record<string, Partial<Settings> | undefined> = { ... }or key it on the literal union and look up through a helper, so the narrowing is real.
Object.fromEntries(Object.entries(preset)…) erases to Record<string, any>. Object.entries on Partial<Settings> falls through to the entries(o: {}): [string, any][] overload, so unset is { [k: string]: any } and the setMany(unset) call site accepts anything. The Partial<Settings> annotation on the map buys nothing past this line — misspell 'Comfy.Canvas.MouseWheelScrol' in the preset and it still compiles and still writes a junk key to the user's settings file. A typed accumulate keeps it honest:
const unset: Partial<Settings> = {}
for (const id of Object.keys(preset) as (keyof Settings)[]) {
if (!settingStore.exists(id)) Object.assign(unset, { [id]: preset[id] })
}There was a problem hiding this comment.
Both correct, both fixed.
Invisible guard. The map is now Record<string, Partial<Settings> | undefined> in constants/canvasNavigation.ts, so if (!preset) return narrows for real instead of reading as dead code to the compiler. That guard carries the entire handling of custom and of any unknown value, so having TS treat it as unreachable was the worst place to lose type information.
Object.fromEntries erasure. Replaced with the typed accumulate you suggested:
const unset: Partial<Settings> = {}
for (const id of Object.keys(preset) as (keyof Settings)[]) {
if (settingValues.value[id] === undefined) {
Object.assign(unset, { [id]: preset[id] })
}
}Your misspelling test is the convincing argument — 'Comfy.Canvas.MouseWheelScrol' previously compiled and would have written a junk key into the user's settings file, which is the sort of thing that survives forever because nothing ever reads it. The annotation on the map now actually reaches the call site.
Both the map and the migration also gained a note that custom is deliberately absent, since its absence is what makes the no-op work and is otherwise an inviting thing to "complete".
|
|
||
| // A mode stored before the overrides shipped in 1.27.4 is the only value on | ||
| // record, so they load as their defaults — which describe a different mode. | ||
| test.describe('stored without the override settings', () => { |
There was a problem hiding this comment.
The three tests are well chosen and I verified the fixture actually supports them — comfyPageFixture's default setupSettings block seeds neither Comfy.Canvas.LeftMouseClickBehavior nor Comfy.Canvas.MouseWheelScroll, so test.use({ initialSettings: { 'Comfy.Canvas.NavigationMode': 'standard' } }) really does reproduce a pre-1.27.4 profile rather than quietly inheriting the overrides. Good.
Gaps I would like closed before this merges:
- The already-demoted profile is untested.
{ 'Comfy.Canvas.NavigationMode': 'custom' }with the overrides absent is the state every user who has already loaded 1.27.4+ is in, and this PR deliberately no-ops on it (see my comment on line 204). Right now that is indistinguishable from an oversight. - The partial case is untested. The
.filter(([id]) => !exists(id))only ever runs with both overrides missing. A profile withNavigationMode: 'standard'and, say, onlyMouseWheelScrollstored exercises the filter for real — and it is the case where "supply only what is missing" could plausibly do the wrong thing. customshort-circuit is untested. Nothing pins that picking Custom does not cascade.- "applies the stored preset to the overrides" only proves the in-memory store.
comfyPage.settings.getSettingresolves toextensionManager.setting.get, which readssettingValues. Since the healingsetManyis fired from an un-awaited handler, the assertion passes even if thePOST /settingsthat persists the healed overrides never lands. AreloadAndWaitForApp()before the assertions would turn this into a persistence test — which is what the fix actually claims.
Minor: in "picking a preset never persists custom", request.url().endsWith('/api/settings/Comfy.Canvas.NavigationMode') will silently match nothing if a query string is ever appended, and the test would then pass vacuously. .includes() on the path, or asserting modeWrites.length > 0 alongside the not.toContain, makes the test fail loudly instead of quietly.
There was a problem hiding this comment.
All four gaps closed, and the partial case earned its keep immediately — it failed, and the bug was in my expectation, not the code.
I wrote it asserting the mode stays standard, and got:
Expected: "standard"
Received: "custom"
Correctly so. With {nav: standard, wheel: zoom} the migration fills only left: 'select', and select + zoom is no preset, so the replay demotes to custom — the honest label for a genuinely mixed pair. The alternative would be overwriting the stored zoom to match the mode, which discards an explicit preference and is the exact failure mode this PR exists to fix. So the test now pins gap-filling-without-overwriting, with a comment explaining why custom is the right answer there — an assertion of toBe('custom') in this PR needs the reasoning attached or someone will "fix" it.
Added:
already demoted to custom—{nav: custom}, overrides absent. Pins the no-op as deliberate.stored with only one override— the case above.picking custom leaves the overrides untouched— pins that Custom does not cascade.- Reload in the override test, renamed to
persists the stored preset to the overrides. You were right that it only proved the in-memory store; with the repair now awaited insideload()it would have been meaningfully better anyway, but the reload makes it test what the fix claims.
On the endsWith nit: added expect(modeWrites).toContain('standard') alongside the not.toContain, so the URL matcher silently matching nothing fails loudly instead of passing vacuously. I kept endsWith rather than includes because storeSetting builds the path with encodeURIComponent(id) and no query string, so a substring match would be looser without being more correct — the positive assertion is what actually guards it.
Six tests now, all passing, and thanks for verifying the fixture seeding independently.
benjcooley
left a comment
benjcooley
left a comment
There was a problem hiding this comment.
Careful diagnosis and a genuinely good root-cause writeup — the two-cause analysis is correct, and I confirmed both against the code. The applySettingLocally reorder is the right call and the unit test pins the contract properly. But I don't think the second half of the fix is in the right place, and one claim in the description doesn't hold.
Blocking
1. The migration is implemented in an onChange registration replay instead of the load() migration hook the codebase already has for this. (coreSettings.ts L215-222)
settingStore.ts already does this exact shape of thing in migrateZoomThresholdToFontSize(), awaited from load() before any setting is registered — "old key stored, new key isn't → derive it and persist." Putting it in onChange instead buys three problems that hook doesn't have:
- Un-awaited network write, unhandled rejection.
onChange()discards the promise fromsetting.onChange(...), sosetMany→POST /settingsruns detached with nocatch. Backend unreachable or write-rejected → unhandled rejection during bootstrap, every load, for exactly the affected profiles. Before this PR, registration replay bailed atif (!oldValue) returnand never touched the network. - Silent dependence on
CORE_SETTINGSarray order. It only works becauseNavigationModeis registered before the two overrides. Reorder the array and the demotion returns immediately. Nothing states the invariant. - The writes land while
settingsByIdhas no entry for those keys, so they skipdispatchChangeand skip telemetry unlike every other settings write. Right answer by accident.
A migrateCanvasNavigationOverrides() called from load() is awaited, order-independent, unit-testable without Playwright, and lets onChange go back to meaning "the user changed something."
2. exists() re-arms the bug the moment the default flips. (coreSettings.ts L215)
Gating on stored rather than effective mode is safe only because NavigationMode's default (legacy) happens to agree with the override defaults (panning/zoom). Ship defaultValue: 'standard' or a newer defaultsByInstallVersion entry — an obvious near-term move for an option labelled "Standard (New)" — and every fresh profile gets demoted to custom on first load with no interaction. Derive from settingStore.get, or add the invariant test.
Please correct the description
"Affected profiles self-heal on next load" is not true for the profiles that reported this. The cause-2 demotion persists custom to the server and writes only the mode, never the overrides. So anyone who has already loaded a 1.27.4+ build is sitting on NavigationMode: "custom" with the overrides absent — and CANVAS_NAVIGATION_PRESETS has no custom entry, so this PR no-ops on them. Their original choice is genuinely unrecoverable (the demotion overwrote the only record), so I'm not asking for code here — but the description, release notes, and FE-1503 shouldn't say the reporters are fixed. They have to re-pick the mode by hand, which will now stick. Please also add that profile to the test matrix so the no-op is pinned as deliberate.
Non-blocking
- The two remaining writes still race.
onChangeisn't awaited, soPOST /settings(overrides) andPOST /settings/NavigationModeare in flight simultaneously against the non-atomic backend you documented. "One write per key" is true about count, but if the overrides write wins the read, the mode is lost and the next load demotes tocustom— the original symptom, at lower probability.await setting.onChange(...)serialises them, and this PR is what makes awaiting safe: committing before the handler runs is what makes thenewValue === oldValueguard actually guard against re-entrancy. Worth claiming as a second benefit. - Rollback semantics changed — a synchronous handler that throws now leaves the value committed locally with no server write. Low severity, but the new docblock should say so.
- Type nits —
Record<string, Partial<Settings>>makes the load-bearingif (!preset) returninvisible to TS;Object.fromEntries(Object.entries(...))erasesunsettoRecord<string, any>, so a typo'd preset key compiles and writes junk to the settings file. - Test gaps — the
customprofile, the partial-override case (the.filternever runs with only one missing), thecustomshort-circuit; and "applies the stored preset to the overrides" only reads the in-memory store, so it passes even if the healing POST never lands. AreloadAndWaitForApp()would make it the persistence test it claims to be.
What I liked
The settingStore.test.ts case is the right test — it pins the ordering as a store-wide contract rather than a canvas quirk, which is where the bug actually lived. Catching that muting the handlers would fix the label but not the behaviour, and saying so in the description, is exactly right. And the follow-up about deriving NavigationMode from the two overrides is the real fix — this cluster has three keys for two bits of state, and every bug in it traces to that.
|
|
||
| onChange(settingsById.value[key], newValue, oldValue) | ||
| const typedNewValue = newValue as Settings[K] | ||
| settingValues.value[key] = typedNewValue |
There was a problem hiding this comment.
Two things a full sweep of the blast radius turned up — one argues for the change, one is an unlisted breaking change.
In favour: this makes set/setMany consistent with addSetting, which already commits first.
// addSetting, further down this file
settingsById.value[setting.id] = setting
if (settingValues.value[setting.id] !== undefined) { ... }
onChange(setting, get(setting.id), undefined) // ← already post-commitRegistration has always fired onChange against a committed value; only set/setMany did it backwards. That is a stronger framing than "fixes the cascade" and belongs in the docblock — it is why the old ordering was a bug rather than a choice. Worth noting too that const oldValue = get(key) is still computed before the swap, so the (newValue, oldValue) arguments and the newValue === oldValue short-circuit are byte-for-byte unchanged; only re-entrant reads move.
Against: this silently changes behaviour for third-party extensions, and the PR does not mention it.
onChange() also fires app.ui.settings.dispatchChange(...), which this file explicitly documents as a compat shim:
// Backward compatibility with old settings dialog.
// Some extensions still listens event emitted by the old settings dialog.
Any extension listening on '<id>.change' that re-reads app.ui.settings.getSettingValue(id) inside the handler — rather than using event.detail.value — now observes the new value where it previously observed the old one. There is an in-repo instance of exactly that shape at src/scripts/ui/components/button.ts:133-145:
const settingUpdated = () => {
this.hidden =
app.ui.settings.getSettingValue(visibilitySetting.id) !== visibilitySetting.showValue
}
app.ui.settings.addEventListener(visibilitySetting.id + '.change', settingUpdated)That one is a fix (it was reading stale and rendering one change behind, though nothing in-repo currently passes visibilitySetting, so the practical impact is custom-node code). But it demonstrates the pattern is real and out there. detail.oldValue consumers are unaffected.
I do not think this should block — the new behaviour is the correct one and the old one was indefensible. But a store-wide ordering change on a documented extension compat surface deserves a line in the PR description and a changelog note, rather than arriving as a side effect of a canvas-settings bugfix.
There was a problem hiding this comment.
The addSetting framing is much better than mine and I have taken it into the docblock — registration always fired onChange post-commit, so set/setMany were the inconsistent pair, not the other way round. That reframes the old ordering as a straightforward inconsistency rather than a defensible design, which is a more useful thing for the next reader to find. Your note that const oldValue = get(key) is still computed before the swap is worth having on the record too: the (newValue, oldValue) arguments and the short-circuit are byte-identical, only re-entrant reads moved.
On the extension compat surface — you are right that it deserved calling out rather than arriving as a side effect, and thank you for actually finding the in-repo instance at button.ts:133-145. That is a much more concrete basis than my own reasoning, which had stopped at "dispatchEvent cannot throw into the caller" and had not considered listeners that re-read rather than using event.detail.
Two things I did as a result:
- Kept
dispatchChangefiring at its original point in the sequence, before the handler is awaited (see my reply on thesettingStore.tsthread). So listeners fire at the same time relative to the value commit as they did before this PR — the only change for them is which value a re-read observes, not when they run. - Left the behaviour as-is, since as you say the new reading is the correct one and
button.tswas rendering one change behind.
I cannot edit the PR description with my tooling, so I will get the ordering change and its extension-visible effect written into the description and flagged for the changelog rather than leaving it implicit — along with the correction on self-healing from the other thread. Flagging both to the team now so they land in the release notes and not just in this conversation.
Addresses review on #14716. Healing a pre-1.27.4 profile from the mode's own onChange registration replay had three problems the load() hook does not: the write was detached and unhandled because onChange() discarded the returned promise, it silently depended on CORE_SETTINGS registering the mode before the two overrides, and it wrote keys whose settingsById entries did not exist yet, so those writes skipped dispatchChange and telemetry. migrateCanvasNavigationOverrides() now runs from load() alongside the existing zoom-threshold migration, before any setting is registered, so onChange goes back to meaning "the user changed something". The preset pairs move to constants/canvasNavigation.ts so both call sites share one definition without a cycle. Also await the handler in onChange() so a cascading write completes before the caller writes its own key. The two were racing against the backend's non-atomic read-modify-write, which left the fix probabilistic; committing the value first is what makes awaiting safe, since the newValue === oldValue guard now compares against committed state. Tests: migration unit coverage for the stored-preset, partial-override, custom and unstored profiles; an invariant test that the override defaults describe whichever mode a fresh profile resolves to, so flipping that default trips a test instead of demoting every new install; and E2E cover for the already-demoted profile, the partial profile, the custom no-op, plus a reload so the override assertions test persistence.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
That note is the auto-generated "GitHub couldn't provide an incremental comparison, running a full review instead" message — it carries no findings, so there's nothing to action from it directly. It fired because @DrJKL merged Rather than just say "no-op", I re-verified the branch from scratch against the merged state, since that merge pulled in a lot (vite/vitest/vue bumps, the image compositor feature, ~55k lines). The merge did not touch this PR's changes. Verification on the merged head
The 5 failures are unrelated to this PR — I checked out End-to-end re-checkI also reproduced the original bug and the fix by hand against a local backend, seeding a pre-1.27.4 profile (mode stored, both overrides absent) and loading the page with no user interaction: On clean On this branch it holds, and the overrides it implies are materialised: Same seeded profile, same settings pane, both screenshots below. One correction to the PR descriptionThe Verification section has two broken image placeholders ( No code changes in this round — the branch was already correct and CodeRabbit's standing review on it is Screenshots |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/settings/constants/canvasNavigation.test.ts`:
- Around line 25-37: Update the two Navigation Mode tests around
settingById(NAV) to narrow defaultValue and each defaultsByInstallVersion value
to a string before indexing CANVAS_NAVIGATION_PRESETS, resolving callable
defaults using the same behavior as the setting store. Remove both as string
assertions and preserve the existing preset comparisons.
In `@src/platform/settings/constants/coreSettings.ts`:
- Around line 191-196: Update the onChange callback to verify that val has type
string before indexing CANVAS_NAVIGATION_PRESETS; return early when it is not a
string, while preserving the existing old-value and missing-preset checks.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 75484cff-f64f-485b-a060-00739421875f
📒 Files selected for processing (7)
browser_tests/tests/canvasSettings.spec.tssrc/platform/settings/constants/canvasNavigation.test.tssrc/platform/settings/constants/canvasNavigation.tssrc/platform/settings/constants/coreSettings.tssrc/platform/settings/settingStore.test.tssrc/platform/settings/settingStore.tssrc/platform/settings/types.ts
Replace the 'as string' assertions on the unknown onChange value and on the Navigation Mode defaults with typeof narrowing, and resolve callable defaults the way the setting store does.
github-actions
Bot
added
the
risk:R2
| onChange(settingsById.value[key], newValue, oldValue) | ||
| const typedNewValue = newValue as Settings[K] | ||
| settingValues.value[key] = typedNewValue | ||
| await onChange(settingsById.value[key], newValue, oldValue) |
There was a problem hiding this comment.
Awaiting the cascade was my suggestion and the ordering comment in onChange() is exactly right — but it has a consequence worth handling explicitly.
set() is now:
const applied = await applySettingLocally(key, value) // ← awaits onChange, can reject
if (applied === undefined) return
await api.storeSetting(key, applied.newValue) // ← skipped on rejectionA rejecting handler now aborts the parent write. Before, the promise was dropped and api.storeSetting ran regardless. Concretely: pick Navigation Mode standard, have the cascaded storeSettings fail, and the mode is applied in memory but never persisted — silently reverting on next load. In setMany it is worse: keys applied earlier in the batch are already committed in memory when a later handler throws, and the single api.storeSettings for the whole batch never fires.
The sharper edge is third-party: SettingParams.onChange is public API via ComfyExtension.settings. One custom node with a throwing handler now blocks the user's own setting from ever being saved. That is a bad failure mode to hand to extension authors.
You can keep the serialisation you need — which is the whole point of the await — without the coupling:
await Promise.resolve(handled).catch((e) => {
console.error(`[settings] onChange handler for ${setting?.id} failed`, e)
})Ordering is preserved (the write still waits for the cascade to finish), but a broken handler degrades to a logged error instead of silently discarding the user's change.
| }) => { | ||
| // Reload first so this asserts what reached the server, not just what | ||
| // the migration put in the in-memory store. | ||
| await comfyPage.workflow.reloadAndWaitForApp() |
There was a problem hiding this comment.
This addresses my earlier note, but the reload does not actually buy what the comment claims:
// Reload first so this asserts what reached the server, not just what
// the migration put in the in-memory store.
await comfyPage.workflow.reloadAndWaitForApp()Comfy.Canvas.NavigationMode: 'standard' is still on the server after the reload — it is the seeded value, and nothing in this flow removes it. So on the second load migrateCanvasNavigationOverrides runs again, finds the overrides missing (if the POST failed), and re-derives select/panning into settingValues from scratch. getSetting then returns the right answer whether or not storeSettings ever succeeded. The migration's idempotence is exactly what makes the assertion blind to persistence.
So the test is fine as a "the repair is stable across reloads" test, but the comment asserts a guarantee it does not provide — and given the uncaught-write issue I flagged in settingStore.ts, this is the test that would otherwise have caught it. Either read the server directly:
const settings = await comfyPage.request.get('/api/settings').then(r => r.json())
expect(settings['Comfy.Canvas.LeftMouseClickBehavior']).toBe('select')or assert the POST /api/settings fired with the expected body (the pattern you already use in "picking a preset never persists custom"). Failing that, please reword the comment so it does not claim server-side coverage.
| defaultValue: TValue | (() => TValue) | ||
| defaultsByInstallVersion?: Record<`${number}.${number}.${number}`, TValue> | ||
| onChange?(newValue: TValue, oldValue?: TValue): void | ||
| onChange?(newValue: TValue, oldValue?: TValue): void | Promise<void> |
There was a problem hiding this comment.
Correct and necessary now that the result is awaited — but note this is a source-breaking change on published API (SettingParams reaches extension authors through ComfyExtension.settings), and the PR does not mention it.
TS's void-returning-function assignability exemption — which lets you assign () => T to a slot typed () => void for any T — applies only when the target return type is exactly void. void | Promise<void> is a union, so the exemption is gone. Any TS extension written as an expression-bodied arrow that happens to return something now fails to compile:
onChange: (v) => this.values.push(v) // push returns number — was fine, now errors
onChange: (v) => (this.enabled = v) // assignment expression — sameThis already bit inside this PR: the handler in settingStore.test.ts had to be rewritten from an expression body to a block body to satisfy the new signature. That is the exact diff every affected extension author will have to make, without the benefit of seeing this PR.
Low severity in practice — most custom nodes ship JS, not TS — so I am not blocking on it. But it deserves a line in the description and a changelog entry, since silent compile breaks in third-party code are the kind of thing that gets reported as "the frontend broke my extension" three releases later. If you would rather avoid the break entirely, keeping the field typed void and doing await Promise.resolve(setting.onChange(...)) at the call site gets you the same runtime serialisation with no signature change.
| if (!Object.keys(unset).length) return | ||
|
|
||
| Object.assign(settingValues.value, unset) | ||
| await api.storeSettings(unset) |
There was a problem hiding this comment.
Blocking: an uncaught write here stops the app from booting.
migrateCanvasNavigationOverrides is awaited inside the useAsyncState loader, so a rejecting api.storeSettings lands in settingStore.error. Downstream, GraphCanvas.vue:512-521:
await until(() => isSettingsReady.value || !!settingsError.value).toBe(true)
if (settingsError.value) {
if (settingsError.value instanceof UnauthorizedError) { ...reload...; return }
throw settingsError.value // ← boot aborts here
}
CORE_SETTINGS.forEach(settingStore.addSetting) // ← never runs(settingsError is storeToRefs(settingStore).error, GraphCanvas.vue:228.) So one transient 5xx or offline POST turns a cosmetic settings repair into an app that will not start. api.getSettings immediately above is wrapped in retry(...) precisely because this loader is boot-critical; this new write has neither a retry nor a catch.
I know migrateZoomThresholdToFontSize has the same uncaught shape, so this is a pre-existing hazard class rather than one you invented. It is still worth blocking on, because this PR changes the exposure by orders of magnitude: that migration only fires for profiles carrying the deprecated zoom-threshold key, a set that only shrinks, whereas this one fires for every profile with a stored preset and missing overrides — by your own analysis, everyone who picked a mode between 1.25.0 and 1.27.4. A rare failure mode becomes a common one.
The fix is small and self-correcting. Object.assign(settingValues.value, unset) has already run so the session is correct either way, and the server still lacks the overrides so the next load simply retries:
try {
await api.storeSettings(unset)
} catch (e) {
console.warn('Failed to persist canvas navigation overrides; retrying next load', e)
}Worth giving migrateZoomThresholdToFontSize the same treatment while you are in here.
benjcooley
left a comment
benjcooley
left a comment
There was a problem hiding this comment.
Re-reviewed at 002f64d. You addressed all three of my blocking points properly, and not superficially:
- the repair moved into
migrateCanvasNavigationOverrides()inload(), awaited, ahead of registration — so theCORE_SETTINGSordering dependency and thesettingsById-not-yet-populated weirdness are both gone; onChangeis awaited, with a comment explaining why the handler starts beforedispatchChangebut is awaited after it — that is a subtle ordering constraint and documenting it was the right call;- the presets moved to
canvasNavigation.tswith real unit tests,Partial<Settings> | undefinedrestores the narrowing soif (!preset) returnis honest, and theunsetaccumulation is now typed; - the
customno-op is explicitly documented and tested rather than left implicit, and the description now says affected profiles need to re-pick.
I traced the state machine again at this head — standard↔legacy↔custom, preset picks, partial profiles, registration replay — and found no remaining path where the mode is spuriously demoted. The fix is correct. CI is green (29 pass).
One thing stops me approving.
Blocking
The migration's api.storeSettings is uncaught inside the boot-critical loader (settingStore.ts:341). It rejects → settingStore.error is set → GraphCanvas.vue:521 does throw settingsError.value → CORE_SETTINGS.forEach(settingStore.addSetting) never runs. A transient POST failure turns a cosmetic settings repair into an app that will not start.
migrateZoomThresholdToFontSize has the same shape, so the hazard predates you — but this PR takes it from "fires for profiles with a deprecated zoom key" to "fires for everyone who picked a mode between 1.25.0 and 1.27.4," which is the whole population this PR exists to serve. A try/catch with a warn is enough: the in-memory assign has already happened, and the server still lacks the overrides, so the next load retries by itself.
Non-blocking, but please look
settingStore.ts:151— awaiting the cascade (my suggestion) means a rejecting handler now aborts the parent write, so the setting is applied in memory and never persisted. Worse forsetMany, where earlier keys in the batch are already committed. And sinceonChangeis public API viaComfyExtension.settings, one custom node with a throwing handler can block the user's own setting from saving.await Promise.resolve(handled).catch(logIt)keeps the serialisation you need without the coupling.canvasSettings.spec.ts:265— the reload does not make this a persistence test.NavigationModeis still stored server-side, so the second load just re-runs the migration and re-derivesselect/panningin memory; the assertion passes whether or not the POST landed. The migration's idempotence is what blinds it. Notably this is the test that would otherwise have caught the blocker above. Read/api/settingsdirectly, or reword the comment.types.ts:43—void→void | Promise<void>drops TS's void-return assignability exemption on a published interface, soonChange: (v) => arr.push(v)stops compiling for TS extension authors. It already bit inside this PR: the test handler insettingStore.test.tshad to be rewritten to a block body. Low real-world impact, but it wants a changelog line.
Fix the boot path and I'm happy to approve — the rest is polish and I'd take it as follow-up. Nice iteration on this one; the reasoning in the description is genuinely better than most bugfix PRs I read.
The canvas navigation override migration runs inside the boot-critical settings loader, so a rejected write surfaced as settingStore.error, which GraphCanvas rethrows before registering any core setting. A transient POST failure turned a cosmetic repair into an app that would not start, for every profile that picked a mode between 1.25.0 and 1.27.4. Both migrations now log and continue; the value is already applied in memory and the server still holds the un-migrated state, so the next load retries. Awaiting onChange also meant a failing handler aborted the caller's own write. onChange is extension-facing public API, so one custom node could stop the user's setting from ever being saved. Handler failures are now logged and isolated, covering synchronous throws as well as rejections. The persistence e2e test read the in-memory store, which the migration re-derives on every load, so it passed whether or not the write landed. It now reads the server through a new getPersistedSetting helper.
Awaiting onChange means a slow third-party handler lets a later change to the same key land first. The stalled call then resumed and persisted its own stale value, leaving the server disagreeing with the store and the setting reverting on reload — the failure this branch set out to fix. Each apply now takes a per-key ticket and skips its write and telemetry if a newer change claimed the key while its handler ran. Handler faults log at warn rather than error, matching wrapListener: RUM collects console.error, so reporting third-party faults there would relocate the noise the isolation exists to remove.
|
|
||
| // Handlers are awaited, so a slow one lets a later change to this key land | ||
| // first. That change owns the value now; persisting ours would revert it. | ||
| if (latestWrite.get(key) !== write) return undefined |
There was a problem hiding this comment.
Non-blocking — this is the right guard, but the title claim ("drop settings writes a newer change has superseded") is stronger than what it delivers, and it's worth writing that down.
The counter closes the handler-await window: a slow onChange can no longer let a later change land first and then have the stale value written on top. Verified against the cascade — set(nav,'standard') bumps nav to 1, the cascade bumps left/wheel independently, and nothing demotes.
What it does not close is the network window. Both writes can pass the guard and still reorder in flight:
set(key,'A') counter=1 guard passes (no concurrent set yet) POST A ──── slow ────┐
set(key,'B') counter=2 guard passes POST B ── fast ──┐ │
B A ← server ends on A
Memory holds B, server holds A. Re-checking the counter immediately before api.storeSetting doesn't help — A already passed. Closing it properly means serialising per key (chain each key's write on the previous one's promise) or having the backend accept a version.
This is pre-existing — on main applySettingLocally was sync, so both writes were issued the same way — and it's the same hazard as the non-atomic app_settings.py read-modify-write you already list under Follow-ups. I'd just extend that bullet to say the client can reorder too, so nobody reads this guard as "concurrent writes to one key are now safe."
| // Store the migrated setting | ||
| await api.storeSetting(newKey, clampedFontSize) | ||
| await api.storeSetting(oldKey, undefined) | ||
| await persistMigration(async () => { |
There was a problem hiding this comment.
Non-blocking, and I'd take it as a follow-up: the "retrying on next load" contract in persistMigration's docblock doesn't hold for this caller, because these are two writes, not one.
If storeSetting(newKey, …) succeeds and storeSetting(oldKey, undefined) fails, the server ends up holding both keys. Next load re-enters with newKey !== undefined, so the guard on line 401 skips the migration — and the stale LowQualityRenderingZoomThreshold is never cleaned up. No retry, just a permanent orphan.
Harmless in effect (nothing reads the old key once the new one exists) but it means the failure is silent and terminal rather than deferred. The nav migration doesn't have this shape — one storeSettings call, all-or-nothing, so its comment is accurate there.
Cheapest fix if you want it: hoist the delete into its own persistMigration and drop the newKey === undefined half of the guard, or just accept the orphan and narrow the comment to "the nav migration retries; this one may leave the old key behind."
benjcooley
left a comment
benjcooley
left a comment
There was a problem hiding this comment.
Approving — the blocker is fixed, and fixed properly
Re-reviewed at c969f53. Both new commits land my outstanding findings, and I verified each against the code rather than taking the commit messages at face value.
What I checked
1. Boot-critical uncaught write — fixed, and the pre-existing case with it.
persistMigration() wraps both migrations. I'd flagged only migrateCanvasNavigationOverrides; you also wrapped migrateZoomThresholdToFontSize, which had the identical shape on main and would have taken the app down the same way. That was the right call — it wasn't your regression, but it was live. The unit tests assert the thing that actually matters (store.error undefined, isReady true), not just that a warning was logged.
2. onChange rejection discarding the user's own write — fixed.
callHandler catches both a rejected promise and a synchronous throw, and the it.for covers both. Importantly, it swallows only the handler leg — api.storeSetting still rejects out of set(), so scripts/ui/settings.ts:87's errorSaveSetting toast still fires on a real save failure. I swept every SettingParams.onChange in the repo (8 handlers, 6 distinct functions across coreSettings.ts, electronAdapter.ts, slotDefaults.ts): none validates its input, none throws deliberately, none is a veto-by-throw. Nothing regresses. warn over error matches ComfyApi.wrapListener — checked, the precedent is real.
3. The reload test — fixed, and the fix is better than what I asked for.
getPersistedSetting goes through api.getSettings(), which I confirmed is an uncached GET /settings. That is a genuine server assertion; the reload never was. Comment now says what the test does.
4. defaultValue flip re-arming the bug (my round-1 finding) — canvasNavigation.test.ts is a better answer than the one I proposed. Asserting the relationship between NavigationMode's default and the override defaults, including defaultsByInstallVersion, means whoever flips the default to standard trips a red test instead of shipping the regression. That's the finding turned into a permanent guard.
State machine
Re-traced end to end at head. The demotion paths are closed for the right reason, not by suppression:
- Preset cascade —
set(nav,'standard')commitsnavbefore awaiting, soleft/wheelhandlers read'standard'and both match. One write per key, no'custom'. - Two mismatched overrides at registration —
LEFT's replay commitsnav='custom'synchronously before its firstawait, soWHEEL's replay reads'custom'and no-ops. No double write, and it holds because of the commit-before-onChangereorder, not by luck. - Partial profile (
{nav: standard, wheel: zoom}) — fillsleft, keepszoom, demotes tocustom. Correct:select+zoomgenuinely isn't a preset, and you found that one yourself when the test failed. custom— no preset, no cascade, overrides untouched.
CI green including the test job and all 16 Playwright shards.
Non-blocking, none of it gating
- Two inline notes: the supersede counter closes the handler-await window but not the network-reorder one (worth folding into your existing
app_settings.pyfollow-up bullet so nobody reads it as "concurrent same-key writes are safe now"), and the zoom migration's two-write shape can orphan the old key on partial failure. - The description has drifted from the code and it feeds release notes. Two spots: "On the registration replay, a stored preset now supplies the overrides" — it runs from
load()now, per your own fix; and "affected profiles self-heal on next load" — you agreed in this thread that already-demoted profiles do not, and said you'd corrected it, but the body still says it. Users who hit FE-1503 still have to re-pick the mode by hand; support should know that. SettingParams.onChangewidening tovoid | Promise<void>is still a source break for TS extension authors — the() => Tvoid-exemption disappears once the target isn't exactlyvoid, which is why you had to rewrite your own test handler to a block body. It needs a changelog line. Not this PR's job to fix, but it shouldn't ship unannounced.
Good change. The root cause is genuinely fixed rather than papered over, and the follow-ups section is honest about what's left.
5 participants
PR Created by the Glary-Bot Agent
Fixes FE-1503.
Problem
Pick a Navigation Mode, refresh, and the dropdown reads Custom — while the Left Mouse Click Behavior / Mouse Wheel Scroll radios still show the preset you chose.
Comfy.Canvas.NavigationModeis stored independently of the two overrides it implies, and the three were kept in sync by cross-writes inonChange. Two separate paths destroyed the stored mode.1. The preset cascade read a stale mode.
applySettingLocallyfiredonChangebefore committing the new value:So selecting
standardcascaded intosetMany, andLeftMouseClickBehavior.onChangereadNavigationModeas the value it was replacing (legacy), concludedselectno longer matched it, and wroteNavigationMode = 'custom'. Captured from one dropdown click:Three concurrent writes, two to the same key. ComfyUI's
app_settings.pyhas anawait request.json()between its file read and write, so these whole-file read-modify-writes are not atomic and the last one to land wins.2. A mode stored before 1.27.4 was overruled by the override defaults.
addSettingreplaysonChangefor every setting at registration. The overrides shipped in 1.27.4;NavigationModeshipped in 1.25.0. Anyone who chose a mode in between has only the mode on record, so the overrides load as their defaults —panning/zoom, which describelegacy. The override handlers saw the mismatch and rewrote the mode tocustomon the first load after upgrading, with no user interaction. Deterministic, and it explains "has been there for quite some time" on both Windows and Mac.Once
customis stored the handlers no-op, so it never recovers. This also isn't purely cosmetic:useCanvasInteractionsgates onNavigationMode === 'standard'exactly, so the standard-mode wheel path silently turns off.Fix
onChangeso a cascade observes the mode it is applying. This removes the spurious write entirely, leaving one write per key.CANVAS_NAVIGATION_PRESETSmap rather than being restated per branch.Muting the override handlers during registration was the smaller change and I started there, but review caught that it only fixes the label: the dropdown would read Standard while the canvas still panned and zoomed like Legacy. Making the preset supply its missing overrides fixes both, and affected profiles self-heal on next load.
Tests
Written first, and each fails on
main:mainReceived array: ["custom", "standard"]Expected "standard",Received "custom"Expected "select",Received "panning"The third is the one that would have caught the label-only fix. The unit test in
settingStore.test.tspins the ordering contract directly (['default','default']vs['default','newvalue']without the fix), since it governs every setting, not just this cluster.Verification
Reproduced and confirmed end to end against a local ComfyUI backend. Seeded a pre-1.27.4 profile (
NavigationMode: 'standard', overrides absent) and loaded the page with no user interaction:Before —
comfy.settings.jsonsilently rewritten tocustom; dropdown reads Custom while the radios still show the Standard pair:After — mode holds, and the overrides materialise to
select/panningso label and behaviour agree:settingStoreunit test: fail onmain, pass herecanvasSettings.spec.ts: 5 pre-existing failures in this sandbox (headless canvas drag + screenshot baselines) — identical set on cleanmain; baseline 8 passed → 10 passed herepnpm test:unit: 3 pre-existing failing files (previewAny,onboardingCloudRoutes,GraphView), confirmed failing on cleanmain, none in the settings domainpnpm typecheck,typecheck:browser,lint,format:check,knip— all cleanFollow-ups (not in scope here)
Comfy.Canvas.NavigationModewould be better derived from the two overrides than stored as a third key — one source of truth, no cross-setting cascade. That removes this bug class rather than this instance.POST /settingsandPOST /settings/{id}in ComfyUI'sapp/app_settings.pydo non-atomic read-modify-write of one JSON file with anawaitbetween read and write, so any concurrent setting writes can lose updates.typecheck:browserunconditional in CI.Screenshots