From 7399dca4848cafac10cfea5fd7574831efd686d9 Mon Sep 17 00:00:00 2001 From: Sebastian Herrlinger Date: Fri, 24 Jul 2026 15:59:13 +0200 Subject: [PATCH] fix(cli): normalize canonical keybind duplicates --- packages/cli/src/config/migrate.ts | 4 +- packages/cli/test/config.test.ts | 67 ++++++------------- .../tui/test/cli/tui/diff-viewer.test.tsx | 17 ++--- packages/tui/test/mini/runtime.boot.test.ts | 25 ------- 4 files changed, 25 insertions(+), 88 deletions(-) diff --git a/packages/cli/src/config/migrate.ts b/packages/cli/src/config/migrate.ts index 38ef032eb5b..4617f709f18 100644 --- a/packages/cli/src/config/migrate.ts +++ b/packages/cli/src/config/migrate.ts @@ -42,9 +42,8 @@ export const run = Effect.fn("cli.config.migrate")(function* (input: { const updated = Object.keys(keybinds).reduce((text, name) => { const target = TuiKeybind.CommandMap[name as keyof typeof TuiKeybind.CommandMap] ?? - (LegacyKeybindTargets.has(name) ? name : undefined) + (name in Definitions || LegacyKeybindTargets.has(name) ? name : undefined) if (target === undefined) return text - if (target === name && target in Definitions) return text const properties = findKeybindProperties(text, name) if (!properties.length) return text const remove = !(target in Definitions) || (target !== name && target in keybinds) @@ -54,6 +53,7 @@ export const run = Effect.fn("cli.config.migrate")(function* (input: { return property === undefined ? text : removeProperty(text, property) }, text) if (remove) return updated + if (target === name) return updated const key = findKeybindProperties(updated, name)[0]?.children?.[0] if (key === undefined) return text return updated.slice(0, key.offset) + JSON.stringify(target) + updated.slice(key.offset + key.length) diff --git a/packages/cli/test/config.test.ts b/packages/cli/test/config.test.ts index 9c6cb7d49f3..d32e5392601 100644 --- a/packages/cli/test/config.test.ts +++ b/packages/cli/test/config.test.ts @@ -262,7 +262,7 @@ test("preserves the effective value when migrating duplicate legacy keybinds", a } }) -test("migrates the effective duplicate top-level keybinds", async () => { +test("migrates and updates the effective duplicate top-level keybinds", async () => { const directory = await Bun.$`mktemp -d`.text().then((value) => value.trim()) const file = path.join(directory, "cli.json") await Bun.write(file, `{"keybinds":{"session_delete":"first"},"keybinds":{"session_delete":"last"}}`) @@ -272,55 +272,20 @@ test("migrates the effective duplicate top-level keybinds", async () => { directory, Effect.gen(function* () { const service = yield* Config.Service - return yield* service.get() + expect((yield* service.get()).keybinds).toEqual({ "session.delete": "last" }) + return yield* service.update((draft) => { + draft.keybinds = { ...draft.keybinds, "session.delete": "changed" } + }) }), ) - expect(config.keybinds).toEqual({ "session.delete": "last" }) - expect(parse(await Bun.file(file).text()).keybinds).toEqual({ "session.delete": "last" }) + expect(config.keybinds).toEqual({ "session.delete": "changed" }) + expect(parse(await Bun.file(file).text()).keybinds).toEqual({ "session.delete": "changed" }) } finally { await Bun.$`rm -rf ${directory}` } }) -test("does not overwrite a concurrent config update during migration", async () => { - const directory = await Bun.$`mktemp -d`.text().then((value) => value.trim()) - const file = path.join(directory, "cli.json") - const initial = `{"keybinds":{"session_delete":"ctrl+d"}}` - await Bun.write(file, initial) - const gated = await Effect.runPromise(gateMigrationWrite(file, initial).pipe(Effect.provide(NodeFileSystem.layer))) - - try { - const config = await Effect.runPromise( - Effect.gen(function* () { - const service = yield* Config.Service - const reading = yield* service.get().pipe(Effect.forkChild({ startImmediately: true })) - expect(gated.state.writes).toBe(1) - const updating = yield* service - .update((draft) => { - draft.mouse = false - }) - .pipe(Effect.forkChild({ startImmediately: true })) - expect(gated.state.writes).toBe(1) - yield* gated.release.open - yield* Fiber.join(reading) - yield* Fiber.join(updating) - return yield* service.get() - }).pipe( - Effect.provide(Config.layer), - Effect.provide(Global.layerWith({ config: directory, state: directory })), - Effect.provideService(FileSystem.FileSystem, gated.fs), - ), - ) - - expect(config).toMatchObject({ keybinds: { "session.delete": "ctrl+d" }, mouse: false }) - expect(await Bun.file(file).json()).toMatchObject({ keybinds: { "session.delete": "ctrl+d" }, mouse: false }) - } finally { - gated.release.openUnsafe() - await Bun.$`rm -rf ${directory}` - } -}) - test("does not overwrite a concurrent update from another config layer", async () => { const directory = await Bun.$`mktemp -d`.text().then((value) => value.trim()) const file = path.join(directory, "cli.json") @@ -366,25 +331,31 @@ test("does not overwrite a concurrent update from another config layer", async ( } }) -test("updates the effective duplicate top-level keybinds", async () => { +test("updates effective duplicate canonical keybinds", async () => { const directory = await Bun.$`mktemp -d`.text().then((value) => value.trim()) const file = path.join(directory, "cli.json") - await Bun.write(file, `{"keybinds":{"session_delete":"first"},"keybinds":{"session_delete":"last"}}`) + await Bun.write( + file, + `{"keybinds":{"session.delete":"first","session.delete":"last","permission.mode":"off","permission.mode":"on"}}`, + ) try { const config = await run( directory, Effect.gen(function* () { const service = yield* Config.Service - yield* service.get() + expect((yield* service.get()).keybinds).toEqual({ "session.delete": "last", "permission.mode": "on" }) return yield* service.update((draft) => { - draft.keybinds = { ...draft.keybinds, "session.delete": "changed" } + draft.keybinds = { ...draft.keybinds, "session.delete": "changed", "permission.mode": "changed" } }) }), ) - expect(config.keybinds).toEqual({ "session.delete": "changed" }) - expect(parse(await Bun.file(file).text()).keybinds).toEqual({ "session.delete": "changed" }) + expect(config.keybinds).toEqual({ "session.delete": "changed", "permission.mode": "changed" }) + expect(parse(await Bun.file(file).text()).keybinds).toEqual({ + "session.delete": "changed", + "permission.mode": "changed", + }) } finally { await Bun.$`rm -rf ${directory}` } diff --git a/packages/tui/test/cli/tui/diff-viewer.test.tsx b/packages/tui/test/cli/tui/diff-viewer.test.tsx index db25212dbd3..f2a2365c657 100644 --- a/packages/tui/test/cli/tui/diff-viewer.test.tsx +++ b/packages/tui/test/cli/tui/diff-viewer.test.tsx @@ -243,7 +243,10 @@ async function renderDiffViewer( } const app = await testRender(() => , { width: 80, height: options.height ?? 20 }) - await waitForCommand(app, commands, "diff.close") + await app.waitFor(async () => { + await Bun.sleep(25) + return commands.has("diff.close") + }) await app.waitFor(() => vcsDiffInput !== undefined) return { app, @@ -344,15 +347,3 @@ test("branch diff source requests branch VCS diff", async () => { viewer.app.renderer.destroy() } }) - -async function waitForCommand( - app: Awaited>, - commands: Map, - command: string, -) { - for (let attempt = 0; attempt < 10; attempt++) { - await app.renderOnce() - if (commands.has(command)) return - await new Promise((resolve) => setTimeout(resolve, 25)) - } -} diff --git a/packages/tui/test/mini/runtime.boot.test.ts b/packages/tui/test/mini/runtime.boot.test.ts index f651847acc9..5a08ce02bf3 100644 --- a/packages/tui/test/mini/runtime.boot.test.ts +++ b/packages/tui/test/mini/runtime.boot.test.ts @@ -1,33 +1,14 @@ import { afterEach, describe, expect, mock, spyOn, test } from "bun:test" import { OpenCode } from "@opencode-ai/client/promise" -import type { Resolved } from "../../src/config" import { resolveMiniSettings, resolveModelInfo, resolveRunTuiConfig } from "../../src/mini/runtime.boot" import { catalogModel, catalogProvider } from "./fixture/catalog" import { createTuiResolvedConfig } from "../fixture/tui-runtime" -function config(input?: { - leader?: string - leaderTimeout?: number -}): Resolved { - return createTuiResolvedConfig({ - leader: input?.leaderTimeout === undefined ? undefined : { timeout: input.leaderTimeout }, - keybinds: input?.leader ? { leader: input.leader } : undefined, - }) -} - describe("run runtime boot", () => { afterEach(() => { mock.restore() }) - test("returns supplied resolved config", async () => { - const input = config({ leader: "ctrl+g" }) - - const result = await resolveRunTuiConfig(input) - - expect(result).toBe(input) - }) - test("falls back to default tui keymap config when config load fails", async () => { const result = await resolveRunTuiConfig(Promise.reject(new Error("boom"))) @@ -44,12 +25,6 @@ describe("run runtime boot", () => { expect(result.keybinds.get("prompt.queue")?.[0]?.key).toBe("alt+return") }) - test("preserves disabled leader from resolved tui config", async () => { - const result = await resolveRunTuiConfig(config({ leader: "none" })) - - expect(result.keybinds.get("leader")).toEqual([]) - }) - test("preserves shared config while resolving independent Mini defaults", async () => { const result = await resolveRunTuiConfig( createTuiResolvedConfig({