Merge remote-tracking branch 'origin/benjcooley/matrix-detection-proof' into benjcooley/matrix-detection-proof

Author: benjcooley

browser_tests/fixtures/helpers/SettingsHelper.ts

@@ -20,4 +20,18 @@ export class SettingsHelper {
       return await window.app!.extensionManager.setting.get(id)
     }, settingId)) as T
   }
+
+  /**
+   * Reads the value the server holds, which {@link getSetting} cannot: that
+   * reports the in-memory store, where a value may have been derived at load
+   * rather than persisted. Returns `undefined` for a setting never written.
+   */
+  async getPersistedSetting<T = unknown>(
+    settingId: string
+  ): Promise<T | undefined> {
+    return (await this.page.evaluate(async (id) => {
+      const persisted = await window.app!.api.getSettings()
+      return persisted[id as keyof typeof persisted]
+    }, settingId)) as T | undefined
+  }
 }

browser_tests/tests/canvasSettings.spec.ts

@@ -188,6 +188,138 @@ test.describe('Canvas settings', { tag: '@canvas' }, () => {
     })
   })
 
+  test.describe('Comfy.Canvas.NavigationMode', () => {
+    test('picking a preset never persists custom', async ({ comfyPage }) => {
+      const modeWrites: unknown[] = []
+      comfyPage.page.on('request', (request) => {
+        if (
+          request.method() === 'POST' &&
+          request.url().endsWith('/api/settings/Comfy.Canvas.NavigationMode')
+        ) {
+          modeWrites.push(request.postDataJSON())
+        }
+      })
+
+      await comfyPage.settings.setSetting(
+        'Comfy.Canvas.NavigationMode',
+        'standard'
+      )
+      await comfyPage.workflow.reloadAndWaitForApp()
+
+      // A preset also writes the two overrides it implies; those writes must
+      // not read back as the user hand-picking an override.
+      expect(modeWrites).toContain('standard')
+      expect(modeWrites).not.toContain('custom')
+      expect(
+        await comfyPage.settings.getSetting('Comfy.Canvas.NavigationMode')
+      ).toBe('standard')
+    })
+
+    test('picking custom leaves the overrides untouched', async ({
+      comfyPage
+    }) => {
+      // Arrive on the standard pair first, so both overrides differ from their
+      // defaults and a handler that rewrote them would be caught.
+      await comfyPage.settings.setSetting(
+        'Comfy.Canvas.NavigationMode',
+        'standard'
+      )
+
+      await comfyPage.settings.setSetting(
+        'Comfy.Canvas.NavigationMode',
+        'custom'
+      )
+      await comfyPage.workflow.reloadAndWaitForApp()
+
+      expect(
+        await comfyPage.settings.getSetting('Comfy.Canvas.NavigationMode')
+      ).toBe('custom')
+      expect(
+        await comfyPage.settings.getSetting(
+          'Comfy.Canvas.LeftMouseClickBehavior'
+        )
+      ).toBe('select')
+      expect(
+        await comfyPage.settings.getSetting('Comfy.Canvas.MouseWheelScroll')
+      ).toBe('panning')
+    })
+
+    // 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', () => {
+      test.use({
+        initialSettings: { 'Comfy.Canvas.NavigationMode': 'standard' }
+      })
+
+      test('keeps the stored preset through load', async ({ comfyPage }) => {
+        expect(
+          await comfyPage.settings.getSetting('Comfy.Canvas.NavigationMode')
+        ).toBe('standard')
+      })
+
+      // Reads the server, not the store: the migration is idempotent, so an
+      // in-memory read passes whether or not the write ever landed.
+      test('persists the stored preset to the overrides', async ({
+        comfyPage
+      }) => {
+        expect(
+          await comfyPage.settings.getPersistedSetting(
+            'Comfy.Canvas.LeftMouseClickBehavior'
+          )
+        ).toBe('select')
+        expect(
+          await comfyPage.settings.getPersistedSetting(
+            'Comfy.Canvas.MouseWheelScroll'
+          )
+        ).toBe('panning')
+      })
+    })
+
+    // Every profile that already loaded a 1.27.4+ build has the mode demoted to
+    // 'custom' with the overrides never written. The original choice is
+    // unrecoverable, so this pins the no-op as deliberate.
+    test.describe('already demoted to custom', () => {
+      test.use({
+        initialSettings: { 'Comfy.Canvas.NavigationMode': 'custom' }
+      })
+
+      test('is left as custom', async ({ comfyPage }) => {
+        expect(
+          await comfyPage.settings.getSetting('Comfy.Canvas.NavigationMode')
+        ).toBe('custom')
+      })
+    })
+
+    test.describe('stored with only one override', () => {
+      test.use({
+        initialSettings: {
+          'Comfy.Canvas.NavigationMode': 'standard',
+          'Comfy.Canvas.MouseWheelScroll': 'zoom'
+        }
+      })
+
+      test('fills the gap without overwriting the stored override', async ({
+        comfyPage
+      }) => {
+        expect(
+          await comfyPage.settings.getSetting(
+            'Comfy.Canvas.LeftMouseClickBehavior'
+          )
+        ).toBe('select')
+        expect(
+          await comfyPage.settings.getSetting('Comfy.Canvas.MouseWheelScroll')
+        ).toBe('zoom')
+
+        // select + zoom is no preset, so demoting the mode is correct here.
+        // Overwriting the stored 'zoom' to match the mode instead would
+        // discard a real preference, which is the bug this all started as.
+        expect(
+          await comfyPage.settings.getSetting('Comfy.Canvas.NavigationMode')
+        ).toBe('custom')
+      })
+    })
+  })
+
   test.describe('Comfy.Canvas.LeftMouseClickBehavior', () => {
     test('override to panning makes empty left-drag pan the canvas', async ({
       comfyPage

src/platform/settings/constants/canvasNavigation.test.ts

@@ -0,0 +1,49 @@
+import { describe, expect, it } from 'vitest'
+
+import { CANVAS_NAVIGATION_PRESETS } from '@/platform/settings/constants/canvasNavigation'
+import { CORE_SETTINGS } from '@/platform/settings/constants/coreSettings'
+import type { SettingParams } from '@/platform/settings/types'
+
+const NAV = 'Comfy.Canvas.NavigationMode'
+const LEFT = 'Comfy.Canvas.LeftMouseClickBehavior'
+const WHEEL = 'Comfy.Canvas.MouseWheelScroll'
+
+const settingById = (id: string) => CORE_SETTINGS.find((s) => s.id === id)
+
+const resolveDefaultValue = (setting: SettingParams | undefined): unknown => {
+  const { defaultValue } = setting ?? {}
+  return typeof defaultValue === 'function'
+    ? (defaultValue as () => unknown)()
+    : defaultValue
+}
+
+const presetForMode = (mode: unknown) =>
+  typeof mode === 'string' ? CANVAS_NAVIGATION_PRESETS[mode] : undefined
+
+const overrideDefaults = () => ({
+  [LEFT]: resolveDefaultValue(settingById(LEFT)),
+  [WHEEL]: resolveDefaultValue(settingById(WHEEL))
+})
+
+describe('CANVAS_NAVIGATION_PRESETS', () => {
+  /**
+   * The override defaults have to describe whichever Navigation Mode a fresh
+   * profile resolves to. If they disagree, that profile loads with a mode no
+   * preset matches and the override handlers demote it to 'custom' on first
+   * load — the bug this pairing exists to prevent. Asserted as a relationship
+   * rather than against fixed values so changing a default is what trips it.
+   */
+  it('agrees with the default Navigation Mode', () => {
+    const defaultMode = resolveDefaultValue(settingById(NAV))
+
+    expect(presetForMode(defaultMode)).toEqual(overrideDefaults())
+  })
+
+  it('agrees with every install-versioned Navigation Mode default', () => {
+    const versioned = settingById(NAV)?.defaultsByInstallVersion ?? {}
+
+    for (const mode of Object.values(versioned)) {
+      expect(presetForMode(mode)).toEqual(overrideDefaults())
+    }
+  })
+})

src/platform/settings/constants/canvasNavigation.ts

@@ -0,0 +1,22 @@
+import type { Settings } from '@/schemas/apiSchema'
+
+/**
+ * The Left Mouse Click Behavior and Mouse Wheel Scroll values each Navigation
+ * Mode preset stands for.
+ *
+ * `custom` is deliberately absent: it means "the overrides are whatever the
+ * user set", so there is no pair to apply or to restore from it.
+ */
+export const CANVAS_NAVIGATION_PRESETS: Record<
+  string,
+  Partial<Settings> | undefined
+> = {
+  standard: {
+    'Comfy.Canvas.LeftMouseClickBehavior': 'select',
+    'Comfy.Canvas.MouseWheelScroll': 'panning'
+  },
+  legacy: {
+    'Comfy.Canvas.LeftMouseClickBehavior': 'panning',
+    'Comfy.Canvas.MouseWheelScroll': 'zoom'
+  }
+}

src/platform/settings/constants/coreSettings.ts

@@ -5,6 +5,7 @@ import {
 } from '@/locales/localeConfig'
 import { isCloud, isDesktop, isNightly } from '@/platform/distribution/types'
 import { TOUR_SEEN_SETTING } from '@/platform/onboarding/onboardingTours'
+import { CANVAS_NAVIGATION_PRESETS } from '@/platform/settings/constants/canvasNavigation'
 import { useSettingStore } from '@/platform/settings/settingStore'
 import type { SettingParams } from '@/platform/settings/types'
 import type { ColorPalettes } from '@/schemas/colorPaletteSchema'
@@ -188,22 +189,11 @@ export const CORE_SETTINGS: SettingParams[] = [
       '1.25.0': 'legacy'
     },
     onChange: async (val: unknown, old?: unknown) => {
-      const newValue = val as string
-      const oldValue = old as string | undefined
-      if (!oldValue) return
-      const settingStore = useSettingStore()
+      if (!old || typeof val !== 'string') return
+      const preset = CANVAS_NAVIGATION_PRESETS[val]
+      if (!preset) return
 
-      if (newValue === 'standard') {
-        await settingStore.setMany({
-          'Comfy.Canvas.LeftMouseClickBehavior': 'select',
-          'Comfy.Canvas.MouseWheelScroll': 'panning'
-        })
-      } else if (newValue === 'legacy') {
-        await settingStore.setMany({
-          'Comfy.Canvas.LeftMouseClickBehavior': 'panning',
-          'Comfy.Canvas.MouseWheelScroll': 'zoom'
-        })
-      }
+      await useSettingStore().setMany(preset)
     }
   },
   {