-
Notifications
You must be signed in to change notification settings - Fork 673
fix: stop Canvas Navigation mode resetting to Custom on reload #14716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7a82ca3
a5b9500
7cc2224
b9ae3bb
fc3402f
0b59281
08e054c
866a49a
002f64d
41477df
c969f53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The three tests are well chosen and I verified the fixture actually supports them — Gaps I would like closed before this merges:
Minor: in "picking a preset never persists custom", There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Correctly so. With Added:
On the Six tests now, all passing, and thanks for verifying the fixture seeding independently. |
||
| 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| } | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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' | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Profiles that were already bitten do not self-heal — the PR description says otherwise.
But that is precisely the state the bug leaves people in. Cause 2 in the description writes 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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to get the profiles some DEET? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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: 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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:
Six E2E tests plus migration unit coverage, all green. |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.