diff --git a/docs/architecture.md b/docs/architecture.md index 04b8216..ac500e9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -71,7 +71,8 @@ The codebase is split so the implementation reads through file boundaries and na - editor lifecycle stays in the controller, not in the reducers - persisted ask settings are versioned and normalized before use - invalid persisted keymaps fall back to default keymaps for the current session without discarding valid behaviour settings -- settings modal edits a local draft and only persists on explicit save +- settings modal edits a local draft and only persists modal changes on explicit save +- when the ask config file is missing, the first ask use writes a default persisted config snapshot - live config updates can affect an in-progress ask flow immediately ## Documentation rule diff --git a/docs/configuration.md b/docs/configuration.md index da890af..f266ad9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -12,7 +12,9 @@ When changing pi-ask settings: Default path: -`~/.pi/agent/extensions/eko24ive-pi-ask.json` +`~/.pi/agent/eko24ive-pi-ask.json` + +If the file does not exist yet, pi-ask creates it with the current default settings the first time the ask flow is used. ## Config shape @@ -21,7 +23,8 @@ Default path: "schemaVersion": 1, "behaviour": { "autoSubmitWhenAnsweredWithoutNotes": false, - "confirmDismissWhenDirty": false, + "confirmDismissWhenDirty": true, + "doublePressReviewShortcuts": true, "showFooterHints": true }, "keymaps": { @@ -46,11 +49,18 @@ Default path: ### `behaviour.confirmDismissWhenDirty` - type: boolean -- default: `false` +- default: `true` - effect: when enabled, discarding a dirty ask flow requires a second cancel/dismiss action - the warning stays visible until the user changes tabs in the ask flow - dirty means there are saved answers/notes or unsaved editor draft text +### `behaviour.doublePressReviewShortcuts` + +- type: boolean +- default: `true` +- effect: when enabled, review-tab number shortcuts (`1`, `2`, `3`) only trigger after pressing the same key twice +- the review screen shows an inline hint and keeps the pending shortcut armed until another review shortcut is pressed or the user leaves the tab + ### `behaviour.showFooterHints` - type: boolean @@ -142,6 +152,7 @@ These are intentionally not configurable: - `tab`, `shift+tab`, `left`, `right` move between tabs - `up`, `down` move between options/actions - `1..9` triggers option/review shortcuts +- when `behaviour.doublePressReviewShortcuts` is enabled, review-tab shortcuts `1`, `2`, and `3` require the same key twice - `@` remains file-reference affordance in editors ## Invalid keymaps behavior @@ -169,6 +180,7 @@ Invalid keymaps include: "behaviour": { "autoSubmitWhenAnsweredWithoutNotes": true, "confirmDismissWhenDirty": true, + "doublePressReviewShortcuts": true, "showFooterHints": false }, "keymaps": { diff --git a/src/config/store.ts b/src/config/store.ts index 379a805..c8b980b 100644 --- a/src/config/store.ts +++ b/src/config/store.ts @@ -28,9 +28,15 @@ export class AskConfigStore { private notice?: AskConfigNotice; private readonly listeners = new Set<(config: AskConfig) => void>(); private readonly configPath: string; + private readonly legacyConfigPath?: string; - constructor(configPath = getAskConfigPath()) { + constructor( + configPath = getAskConfigPath(), + legacyConfigPath = getLegacyAskConfigPath() + ) { this.configPath = configPath; + this.legacyConfigPath = + legacyConfigPath === configPath ? undefined : legacyConfigPath; } subscribe(onChange: (config: AskConfig) => void): () => void { @@ -80,12 +86,15 @@ export class AskConfigStore { } private async loadFromDisk(): Promise { + await this.migrateLegacyConfigIfNeeded(); let content: string; try { content = await readFile(this.configPath, "utf-8"); } catch (error) { if (isMissingFileError(error)) { - return { config: normalizeAskConfig(DEFAULT_ASK_CONFIG) }; + const config = normalizeAskConfig(DEFAULT_ASK_CONFIG); + await this.save(config); + return { config }; } throw error; } @@ -137,6 +146,20 @@ export class AskConfigStore { }, }; } + + private async migrateLegacyConfigIfNeeded(): Promise { + if (!this.legacyConfigPath) { + return; + } + if (await pathExists(this.configPath)) { + return; + } + if (!(await pathExists(this.legacyConfigPath))) { + return; + } + await mkdir(dirname(this.configPath), { recursive: true }); + await rename(this.legacyConfigPath, this.configPath); + } } let askConfigStore: AskConfigStore | undefined; @@ -147,6 +170,10 @@ export function getAskConfigStore(): AskConfigStore { } export function getAskConfigPath(): string { + return join(getAgentDir(), "eko24ive-pi-ask.json"); +} + +export function getLegacyAskConfigPath(): string { return join(getAgentDir(), "extensions", "eko24ive-pi-ask.json"); } @@ -155,6 +182,18 @@ function createBackupPath(path: string, date: Date): string { return path.replace(JSON_EXTENSION_PATTERN, `.${timestamp}.bak.json`); } +async function pathExists(path: string): Promise { + try { + await readFile(path, "utf-8"); + return true; + } catch (error) { + if (isMissingFileError(error)) { + return false; + } + throw error; + } +} + function isMissingFileError(error: unknown): boolean { return ( !!error && diff --git a/tests/config-store.test.ts b/tests/config-store.test.ts index deb96f4..3e5b6cd 100644 --- a/tests/config-store.test.ts +++ b/tests/config-store.test.ts @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import test from "node:test"; import { DEFAULT_ASK_CONFIG } from "../src/config/defaults.ts"; import { AskConfigStore } from "../src/config/store.ts"; @@ -13,18 +13,22 @@ async function makeTempPath(name: string): Promise { const root = await import("node:fs/promises").then(({ mkdtemp }) => mkdtemp(join(tmpdir(), name)) ); - return join(root, "extensions", "eko24ive-pi-ask.json"); + return join(root, "eko24ive-pi-ask.json"); } -test("config store uses in-memory defaults when file is missing", async () => { +test("config store writes defaults when file is missing", async () => { const path = await makeTempPath("pi-ask-config-missing-"); const store = new AskConfigStore(path); const result = await store.ensureLoaded(); assert.deepEqual(result.config, DEFAULT_ASK_CONFIG); - await assert.rejects(readFile(path, "utf-8")); - await rm(join(path, "..", ".."), { force: true, recursive: true }); + assert.deepEqual(JSON.parse(await readFile(path, "utf-8")), { + schemaVersion: 1, + behaviour: DEFAULT_ASK_CONFIG.behaviour, + keymaps: DEFAULT_ASK_CONFIG.keymaps, + }); + await rm(dirname(path), { force: true, recursive: true }); }); test("config store writes full normalized config on save", async () => { @@ -35,6 +39,7 @@ test("config store writes full normalized config on save", async () => { behaviour: { autoSubmitWhenAnsweredWithoutNotes: true, confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, showFooterHints: false, }, keymaps: DEFAULT_ASK_CONFIG.keymaps, @@ -46,22 +51,23 @@ test("config store writes full normalized config on save", async () => { behaviour: { autoSubmitWhenAnsweredWithoutNotes: true, confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, showFooterHints: false, }, keymaps: DEFAULT_ASK_CONFIG.keymaps, }); - await rm(join(path, "..", ".."), { force: true, recursive: true }); + await rm(dirname(path), { force: true, recursive: true }); }); test("config store backs up broken json and loads defaults", async () => { const path = await makeTempPath("pi-ask-config-broken-"); - await mkdir(join(path, ".."), { recursive: true }); + await mkdir(dirname(path), { recursive: true }); await writeFile(path, "{bad json", "utf-8"); const store = new AskConfigStore(path); const result = await store.ensureLoaded(); const dirEntries = await import("node:fs/promises").then(({ readdir }) => - readdir(join(path, "..")) + readdir(dirname(path)) ); assert.deepEqual(result.config, DEFAULT_ASK_CONFIG); @@ -71,12 +77,12 @@ test("config store backs up broken json and loads defaults", async () => { ); assert(dirEntries.some((entry) => entry.includes(".bak.json"))); await assert.rejects(readFile(path, "utf-8")); - await rm(join(path, "..", ".."), { force: true, recursive: true }); + await rm(dirname(path), { force: true, recursive: true }); }); test("config store loads current config version without rewriting", async () => { const path = await makeTempPath("pi-ask-config-current-"); - await mkdir(join(path, ".."), { recursive: true }); + await mkdir(dirname(path), { recursive: true }); await writeFile( path, JSON.stringify({ @@ -84,6 +90,7 @@ test("config store loads current config version without rewriting", async () => behaviour: { autoSubmitWhenAnsweredWithoutNotes: true, confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, showFooterHints: false, }, keymaps: DEFAULT_ASK_CONFIG.keymaps, @@ -98,14 +105,60 @@ test("config store loads current config version without rewriting", async () => true ); assert.equal(result.config.behaviour.confirmDismissWhenDirty, true); + assert.equal(result.config.behaviour.doublePressReviewShortcuts, true); assert.equal(result.config.behaviour.showFooterHints, false); assert.deepEqual(result.config.keymaps, DEFAULT_ASK_CONFIG.keymaps); - await rm(join(path, "..", ".."), { force: true, recursive: true }); + await rm(dirname(path), { force: true, recursive: true }); +}); + +test("config store migrates the legacy extensions config path", async () => { + const root = await import("node:fs/promises").then(({ mkdtemp }) => + mkdtemp(join(tmpdir(), "pi-ask-config-legacy-")) + ); + const path = join(root, "eko24ive-pi-ask.json"); + const legacyPath = join(root, "extensions", "eko24ive-pi-ask.json"); + await mkdir(dirname(legacyPath), { recursive: true }); + await writeFile( + legacyPath, + JSON.stringify({ + schemaVersion: 1, + behaviour: { + autoSubmitWhenAnsweredWithoutNotes: true, + confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, + showFooterHints: false, + }, + keymaps: DEFAULT_ASK_CONFIG.keymaps, + }) + ); + const store = new AskConfigStore(path, legacyPath); + + const result = await store.ensureLoaded(); + + assert.equal( + result.config.behaviour.autoSubmitWhenAnsweredWithoutNotes, + true + ); + assert.equal(result.config.behaviour.confirmDismissWhenDirty, true); + assert.equal(result.config.behaviour.doublePressReviewShortcuts, true); + assert.equal(result.config.behaviour.showFooterHints, false); + await assert.rejects(readFile(legacyPath, "utf-8")); + assert.deepEqual(JSON.parse(await readFile(path, "utf-8")), { + schemaVersion: 1, + behaviour: { + autoSubmitWhenAnsweredWithoutNotes: true, + confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, + showFooterHints: false, + }, + keymaps: DEFAULT_ASK_CONFIG.keymaps, + }); + await rm(root, { force: true, recursive: true }); }); test("config store falls back only keymaps when configured keymaps are invalid", async () => { const path = await makeTempPath("pi-ask-config-invalid-keymaps-"); - await mkdir(join(path, ".."), { recursive: true }); + await mkdir(dirname(path), { recursive: true }); await writeFile( path, JSON.stringify({ @@ -113,6 +166,7 @@ test("config store falls back only keymaps when configured keymaps are invalid", behaviour: { autoSubmitWhenAnsweredWithoutNotes: true, confirmDismissWhenDirty: true, + doublePressReviewShortcuts: true, showFooterHints: false, }, keymaps: { @@ -134,8 +188,9 @@ test("config store falls back only keymaps when configured keymaps are invalid", true ); assert.equal(result.config.behaviour.confirmDismissWhenDirty, true); + assert.equal(result.config.behaviour.doublePressReviewShortcuts, true); assert.equal(result.config.behaviour.showFooterHints, false); assert.deepEqual(result.config.keymaps, DEFAULT_ASK_CONFIG.keymaps); assert.match(result.notice?.text ?? "", DEFAULT_KEYMAPS_NOTICE_PATTERN); - await rm(join(path, "..", ".."), { force: true, recursive: true }); + await rm(dirname(path), { force: true, recursive: true }); });