diff --git a/src/main/keybindings/keybinding-file.test.ts b/src/main/keybindings/keybinding-file.test.ts index 5d88511ebb0..c2cc32c7074 100644 --- a/src/main/keybindings/keybinding-file.test.ts +++ b/src/main/keybindings/keybinding-file.test.ts @@ -247,15 +247,46 @@ describe('keybinding-file', () => { expect(snapshot.diagnostics).toEqual([]) }) - it('skips the seed when the user has already customized any swapped action', () => { + it('preserves a customized action while still pinning the un-customized ones', () => { + // A partially-customized existing user: they rebound one action. The other + // three must still land on their pre-swap defaults, not the new ones. writeKeybindingOverride(filePath, 'darwin', 'tab.nextSameType', ['Mod+Alt+K']) const result = seedLegacyTabSwitchBindings(filePath, 'darwin', LEGACY_TAB_SWITCH_BINDINGS) - expect(result.seeded).toBe(false) - // The user's choice is untouched and the other actions stay on their defaults. + expect(result.seeded).toBe(true) expect(readKeybindingFile(filePath, 'darwin').overrides).toEqual({ - 'tab.nextSameType': ['Mod+Alt+K'] + 'tab.nextSameType': ['Mod+Alt+K'], + 'tab.previousSameType': ['Mod+Shift+BracketLeft'], + 'tab.nextAllTypes': ['Mod+Alt+BracketRight'], + 'tab.previousAllTypes': ['Mod+Alt+BracketLeft'] + }) + }) + + it('is a no-op once every swapped action already resolves on this platform', () => { + // Seeding writes the whole document at once, so it clears the per-write + // conflict guard that a naive one-action-at-a-time write would trip. + const first = seedLegacyTabSwitchBindings(filePath, 'darwin', LEGACY_TAB_SWITCH_BINDINGS) + expect(first.seeded).toBe(true) + expect(first.snapshot.diagnostics).toEqual([]) + + const second = seedLegacyTabSwitchBindings(filePath, 'darwin', LEGACY_TAB_SWITCH_BINDINGS) + expect(second.seeded).toBe(false) + }) + + it('does not let a foreign-platform override block the active-platform pin', () => { + // Only linux is customized; a darwin launch must still pin darwin so the + // active platform keeps the pre-swap behavior. + writeKeybindingOverride(filePath, 'linux', 'tab.nextAllTypes', ['Mod+Alt+K']) + + const result = seedLegacyTabSwitchBindings(filePath, 'darwin', LEGACY_TAB_SWITCH_BINDINGS) + + expect(result.seeded).toBe(true) + expect(readKeybindingFile(filePath, 'darwin').platformOverrides.darwin).toMatchObject({ + 'tab.nextAllTypes': ['Mod+Alt+BracketRight'] + }) + expect(readKeybindingFile(filePath, 'linux').platformOverrides.linux).toEqual({ + 'tab.nextAllTypes': ['Mod+Alt+K'] }) }) diff --git a/src/main/keybindings/keybinding-file.ts b/src/main/keybindings/keybinding-file.ts index ed9726fb1fc..c1c5a779c2f 100644 --- a/src/main/keybindings/keybinding-file.ts +++ b/src/main/keybindings/keybinding-file.ts @@ -322,9 +322,15 @@ export function migrateLegacyKeybindings( * Pin the pre-swap tab-switch chords for a pre-existing install so upgrading * users keep the shortcuts they learned. Writes into the active-platform * section (mirroring `writeKeybindingOverride`) so the seeded values stay - * resettable from Settings. No-op when the user has already customized any of - * the swapped actions — that is custom territory, and a half-swapped pin would - * be worse than leaving their choices alone. + * resettable from Settings. + * + * Pins per action, not all-or-nothing: an action is seeded only when this + * platform has no effective override for it yet. That way a user who rebound + * just one of the swapped actions keeps that choice AND keeps the pre-swap + * default on the other three — an existing user's behavior is never altered, + * whether they customized none, some, or all of them. Because every pin equals + * the action's old default, the seeded set reproduces exactly today's effective + * config and introduces no new conflicts. */ export function seedLegacyTabSwitchBindings( path: string, @@ -334,14 +340,14 @@ export function seedLegacyTabSwitchBindings( const keybindingPlatform = getKeybindingPlatform(platform) const actionIds = Object.keys(legacyBindings) as KeybindingActionId[] const current = readKeybindingFile(path, platform) - const alreadyCustomized = actionIds.some( - (actionId) => - Object.prototype.hasOwnProperty.call(current.commonOverrides, actionId) || - PLATFORM_KEYS.some((os) => - Object.prototype.hasOwnProperty.call(current.platformOverrides[os] ?? {}, actionId) - ) + // `current.overrides` is the effective override map for this platform (common + // + this platform's section). Skip any action that already resolves to a + // user-set binding here; a foreign-platform-only override must not block the + // pin, or this platform would drift to the new default. + const toSeed = actionIds.filter( + (actionId) => !Object.prototype.hasOwnProperty.call(current.overrides, actionId) ) - if (alreadyCustomized) { + if (toSeed.length === 0) { return { seeded: false, snapshot: current } } @@ -359,7 +365,7 @@ export function seedLegacyTabSwitchBindings( const activePlatform = isJsonObject(platforms[keybindingPlatform]) ? { ...(platforms[keybindingPlatform] as JsonObject) } : {} - for (const actionId of actionIds) { + for (const actionId of toSeed) { const normalized = normalizeKeybindingArrayForAction(actionId, legacyBindings[actionId] ?? []) if (Array.isArray(normalized)) { activePlatform[actionId] = normalized diff --git a/src/main/keybindings/keybinding-service.test.ts b/src/main/keybindings/keybinding-service.test.ts new file mode 100644 index 00000000000..2d18190fba8 --- /dev/null +++ b/src/main/keybindings/keybinding-service.test.ts @@ -0,0 +1,227 @@ +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + getEffectiveKeybindingsForAction, + keybindingMatchesAction, + LEGACY_TAB_SWITCH_BINDINGS, + type KeybindingActionId, + type KeybindingInput +} from '../../shared/keybindings' +import { getUserKeybindingsPath, writeKeybindingOverride } from './keybinding-file' +import { KeybindingService } from './keybinding-service' + +// End-to-end coverage of the tab-switch convention swap: constructs a real +// KeybindingService (which runs the seed) and asserts the effective bindings AND +// real keystroke matching for both cohorts. The load-bearing claim under test is +// that a pre-existing install's behavior is byte-identical to what it was before +// the swap, on every platform and every customization state. + +const PLATFORMS: NodeJS.Platform[] = ['darwin', 'linux', 'win32'] +const SWAPPED_ACTIONS = Object.keys(LEGACY_TAB_SWITCH_BINDINGS) as KeybindingActionId[] + +function makeCohort(pending: boolean): { + controller: { isPending: () => boolean; markSeeded: () => void } + seeded: () => boolean +} { + let stillPending = pending + let didSeed = false + return { + controller: { + isPending: () => stillPending, + markSeeded: () => { + didSeed = true + stillPending = false + } + }, + seeded: () => didSeed + } +} + +// A physical bracket press. Mod is Cmd on macOS and Ctrl elsewhere; the matcher +// resolves brackets by `code`, so this stays layout-independent. +function bracketPress(opts: { + code: 'BracketLeft' | 'BracketRight' + alt: boolean + shift: boolean + platform: NodeJS.Platform +}): KeybindingInput { + const isMac = opts.platform === 'darwin' + return { + code: opts.code, + key: opts.code === 'BracketRight' ? ']' : '[', + meta: isMac, + control: !isMac, + alt: opts.alt, + shift: opts.shift + } +} + +describe('KeybindingService tab-switch cohort seeding', () => { + let home: string + + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'orca-kb-service-')) + }) + + afterEach(() => { + rmSync(home, { recursive: true, force: true }) + }) + + it.each(PLATFORMS)( + 'fresh install (%s) adopts the swapped defaults and writes no file', + (platform) => { + const cohort = makeCohort(false) + const service = new KeybindingService({ + homePath: home, + platform, + legacyTabSwitchSeed: cohort.controller + }) + + expect(cohort.seeded()).toBe(false) + // Nothing written: a fresh install rides on the registry defaults. + expect(existsSync(getUserKeybindingsPath(home))).toBe(false) + + const overrides = service.getOverrides() + expect(getEffectiveKeybindingsForAction('tab.nextAllTypes', platform, overrides)).toEqual([ + 'Mod+Shift+BracketRight' + ]) + expect(getEffectiveKeybindingsForAction('tab.nextSameType', platform, overrides)).toEqual([ + 'Mod+Alt+BracketRight' + ]) + + // Shift+bracket now drives the broad all-types cycle. + expect( + keybindingMatchesAction( + 'tab.nextAllTypes', + bracketPress({ code: 'BracketRight', alt: false, shift: true, platform }), + platform, + overrides + ) + ).toBe(true) + expect( + keybindingMatchesAction( + 'tab.nextSameType', + bracketPress({ code: 'BracketRight', alt: false, shift: true, platform }), + platform, + overrides + ) + ).toBe(false) + } + ) + + it.each(PLATFORMS)( + 'existing install (%s) keeps the pre-swap chords exactly, on a clean profile', + (platform) => { + const cohort = makeCohort(true) + const service = new KeybindingService({ + homePath: home, + platform, + legacyTabSwitchSeed: cohort.controller + }) + + expect(cohort.seeded()).toBe(true) + const overrides = service.getOverrides() + + // Effective bindings for all four actions equal the pre-swap defaults. + for (const actionId of SWAPPED_ACTIONS) { + expect(getEffectiveKeybindingsForAction(actionId, platform, overrides)).toEqual( + LEGACY_TAB_SWITCH_BINDINGS[actionId] + ) + } + + // Behavior is unchanged: Mod+Shift+bracket still cycles same-type, and + // Mod+Alt+bracket still cycles all-types — the inverse of a fresh install. + const shiftRight = bracketPress({ code: 'BracketRight', alt: false, shift: true, platform }) + expect(keybindingMatchesAction('tab.nextSameType', shiftRight, platform, overrides)).toBe( + true + ) + expect(keybindingMatchesAction('tab.nextAllTypes', shiftRight, platform, overrides)).toBe( + false + ) + + const altLeft = bracketPress({ code: 'BracketLeft', alt: true, shift: false, platform }) + expect(keybindingMatchesAction('tab.previousAllTypes', altLeft, platform, overrides)).toBe( + true + ) + expect(keybindingMatchesAction('tab.previousSameType', altLeft, platform, overrides)).toBe( + false + ) + } + ) + + it('existing install keeps a rebound action AND pins the pre-swap default on the rest', () => { + const platform: NodeJS.Platform = 'darwin' + // The user had rebound just one action before upgrading. + writeKeybindingOverride(getUserKeybindingsPath(home), platform, 'tab.nextSameType', ['Mod+K']) + + const cohort = makeCohort(true) + const service = new KeybindingService({ + homePath: home, + platform, + legacyTabSwitchSeed: cohort.controller + }) + const overrides = service.getOverrides() + + // Their custom binding is untouched... + expect(getEffectiveKeybindingsForAction('tab.nextSameType', platform, overrides)).toEqual([ + 'Mod+K' + ]) + // ...and the other three still resolve to their pre-swap defaults, not the + // new ones — no silent behavior change for a partially-customized user. + expect(getEffectiveKeybindingsForAction('tab.previousSameType', platform, overrides)).toEqual([ + 'Mod+Shift+BracketLeft' + ]) + expect(getEffectiveKeybindingsForAction('tab.nextAllTypes', platform, overrides)).toEqual([ + 'Mod+Alt+BracketRight' + ]) + expect(getEffectiveKeybindingsForAction('tab.previousAllTypes', platform, overrides)).toEqual([ + 'Mod+Alt+BracketLeft' + ]) + }) + + it('is idempotent: a second launch after the seed changes nothing', () => { + const platform: NodeJS.Platform = 'linux' + const cohort = makeCohort(true) + const first = new KeybindingService({ + homePath: home, + platform, + legacyTabSwitchSeed: cohort.controller + }) + const afterFirst = first.getOverrides() + + // Same cohort controller: markSeeded flipped it to not-pending, mirroring the + // persisted flag flipping to 'done'. + const second = new KeybindingService({ + homePath: home, + platform, + legacyTabSwitchSeed: cohort.controller + }) + expect(second.getOverrides()).toEqual(afterFirst) + for (const actionId of SWAPPED_ACTIONS) { + expect(getEffectiveKeybindingsForAction(actionId, platform, second.getOverrides())).toEqual( + LEGACY_TAB_SWITCH_BINDINGS[actionId] + ) + } + }) + + it('leaves the cohort pending when the seed write fails (retries next launch)', () => { + // homePath is a regular file, so creating `/.orca/` throws. The service + // must swallow the error and NOT mark the one-shot done. + const fileHome = join(home, 'not-a-dir') + writeFileSync(fileHome, 'x', 'utf8') + const cohort = makeCohort(true) + + expect( + () => + new KeybindingService({ + homePath: fileHome, + platform: 'darwin', + legacyTabSwitchSeed: cohort.controller + }) + ).not.toThrow() + expect(cohort.seeded()).toBe(false) + expect(cohort.controller.isPending()).toBe(true) + }) +})