From 9799b42dcd46fec621ab3abf48332f22950537af Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 17 Feb 2026 21:17:57 -0800 Subject: [PATCH] Address PR review feedback for scripts/keybindings handling Co-authored-by: codex --- apps/server/src/keybindings.test.ts | 45 ++++++++++++++++++++++++++++ apps/server/src/keybindings.ts | 35 ++++++++++++++++++++-- apps/web/src/components/ChatView.tsx | 19 +++++++++--- apps/web/src/projectScripts.test.ts | 9 ++++++ apps/web/src/projectScripts.ts | 2 +- apps/web/src/store.test.ts | 43 ++++++++++++++++++++++++++ apps/web/src/store.ts | 5 +--- 7 files changed, 146 insertions(+), 12 deletions(-) diff --git a/apps/server/src/keybindings.test.ts b/apps/server/src/keybindings.test.ts index f525139ddf2bb110c538be272c195dcd2c1847d7..66d90a519e877dab66263f232fcb7a68002d82f7 100644 --- a/apps/server/src/keybindings.test.ts +++ b/apps/server/src/keybindings.test.ts @@ -144,4 +144,49 @@ describe("server keybindings", () => { }>; expect(persisted).toEqual([{ key: "mod+shift+r", command: "script.run-tests.run" }]); }); + + it("refuses to overwrite malformed keybindings config", () => { + const fakeHome = makeTempDir("t3code-keybindings-malformed-"); + const configDir = path.join(fakeHome, ".t3"); + fs.mkdirSync(configDir, { recursive: true }); + const configPath = path.join(configDir, "keybindings.json"); + fs.writeFileSync(configPath, "{ not-json", "utf8"); + vi.spyOn(os, "homedir").mockReturnValue(fakeHome); + + expect(() => + upsertKeybindingRule(logger, { + key: "mod+shift+r", + command: "script.run-tests.run", + }), + ).toThrow(/Unable to parse keybindings config/); + + expect(fs.readFileSync(configPath, "utf8")).toBe("{ not-json"); + }); + + it("cleans up temp files when atomic keybinding write fails", () => { + const fakeHome = makeTempDir("t3code-keybindings-atomic-"); + const configDir = path.join(fakeHome, ".t3"); + fs.mkdirSync(configDir, { recursive: true }); + const configPath = path.join(configDir, "keybindings.json"); + fs.writeFileSync(configPath, JSON.stringify([{ key: "mod+j", command: "terminal.toggle" }]), "utf8"); + vi.spyOn(os, "homedir").mockReturnValue(fakeHome); + vi.spyOn(fs, "renameSync").mockImplementation(() => { + throw new Error("rename failed"); + }); + + expect(() => + upsertKeybindingRule(logger, { + key: "mod+shift+r", + command: "script.run-tests.run", + }), + ).toThrow(/rename failed/); + + expect(fs.readFileSync(configPath, "utf8")).toBe( + JSON.stringify([{ key: "mod+j", command: "terminal.toggle" }]), + ); + const tempFiles = fs + .readdirSync(configDir) + .filter((file) => file.includes("keybindings.json.") && file.endsWith(".tmp")); + expect(tempFiles).toEqual([]); + }); }); diff --git a/apps/server/src/keybindings.ts b/apps/server/src/keybindings.ts index 631e4a2a829c0333ab37a59fd0e24f9936aeb222..5b1a1b4738694cd4421a5c59a4116b2cb29f23a4 100644 --- a/apps/server/src/keybindings.ts +++ b/apps/server/src/keybindings.ts @@ -281,12 +281,20 @@ function resolveKeybindingsConfigPath(): string { return path.join(os.homedir(), KEYBINDINGS_CONFIG_PATH); } -function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConfig { +function loadCustomKeybindingsConfig( + logger: KeybindingsLogger, + options?: { + throwOnUnreadableConfig?: boolean; + }, +): KeybindingsConfig { const configPath = resolveKeybindingsConfigPath(); try { const raw = fs.readFileSync(configPath, "utf8"); const parsed = JSON.parse(raw) as unknown; if (!Array.isArray(parsed)) { + if (options?.throwOnUnreadableConfig) { + throw new Error(`Unable to parse keybindings config at ${configPath}: expected JSON array`); + } logger.warn("ignoring keybindings config with unsupported format; expected array", { path: configPath, }); @@ -319,6 +327,14 @@ function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConf if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { return []; } + if (options?.throwOnUnreadableConfig) { + throw new Error( + `Unable to parse keybindings config at ${configPath}: ${ + error instanceof Error ? error.message : String(error) + }`, + { cause: error }, + ); + } logger.warn("ignoring malformed keybindings config", { path: configPath, error: error instanceof Error ? error.message : String(error), @@ -327,6 +343,17 @@ function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConf } } +function writeConfigAtomically(configPath: string, config: KeybindingsConfig): void { + const tempPath = `${configPath}.${process.pid}.${Date.now()}.tmp`; + try { + fs.writeFileSync(tempPath, `${JSON.stringify(config, null, 2)}\n`, "utf8"); + fs.renameSync(tempPath, configPath); + } catch (error) { + fs.rmSync(tempPath, { force: true }); + throw error; + } +} + function mergeWithDefaultKeybindings(custom: ResolvedKeybindingsConfig): ResolvedKeybindingsConfig { if (custom.length === 0) { return [...DEFAULT_RESOLVED_KEYBINDINGS]; @@ -375,7 +402,9 @@ export function upsertKeybindingRule( } const configPath = resolveKeybindingsConfigPath(); - const customConfig = loadCustomKeybindingsConfig(logger); + const customConfig = loadCustomKeybindingsConfig(logger, { + throwOnUnreadableConfig: true, + }); const nextConfig = [...customConfig.filter((entry) => entry.command !== rule.command), rule]; const cappedConfig = nextConfig.length > MAX_KEYBINDINGS ? nextConfig.slice(-MAX_KEYBINDINGS) : nextConfig; @@ -388,7 +417,7 @@ export function upsertKeybindingRule( } fs.mkdirSync(path.dirname(configPath), { recursive: true }); - fs.writeFileSync(configPath, `${JSON.stringify(cappedConfig, null, 2)}\n`, "utf8"); + writeConfigAtomically(configPath, cappedConfig); const compiledCustomConfig = compileResolvedKeybindingsConfig(cappedConfig); return mergeWithDefaultKeybindings(compiledCustomConfig); diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 0a5a5ea951705dfd8f891f9d83e823b6db9e012d..b9b06ac1a63fbd348527c8ca395fbf09836bc8c7 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -60,7 +60,11 @@ import { } from "../session-logic"; import { isScrollContainerNearBottom } from "../chat-scroll"; import { useStore } from "../store"; -import { MAX_THREAD_TERMINAL_COUNT, type ChatImageAttachment } from "../types"; +import { + DEFAULT_THREAD_TERMINAL_ID, + MAX_THREAD_TERMINAL_COUNT, + type ChatImageAttachment, +} from "../types"; import { basenameOfPath, getVscodeIconUrlForEntry } from "../vscode-icons"; import { useTheme } from "../hooks/useTheme"; import BranchToolbar from "./BranchToolbar"; @@ -655,6 +659,7 @@ export default function ChatView() { options?: { cwd?: string; env?: Record; + worktreePath?: string | null; preferNewTerminal?: boolean; }, ) => { @@ -664,9 +669,14 @@ export default function ChatView() { return { ...current, [activeProject.id]: script.id }; }); const targetCwd = options?.cwd ?? gitCwd ?? activeProject.cwd; - const baseTerminalId = activeThread.activeTerminalId; + const baseTerminalId = + activeThread.activeTerminalId || + activeThread.terminalIds[0] || + DEFAULT_THREAD_TERMINAL_ID; const isBaseTerminalBusy = activeThread.runningTerminalIds.includes(baseTerminalId); - const shouldCreateNewTerminal = Boolean(options?.preferNewTerminal) || isBaseTerminalBusy; + const wantsNewTerminal = Boolean(options?.preferNewTerminal) || isBaseTerminalBusy; + const shouldCreateNewTerminal = + wantsNewTerminal && activeThread.terminalIds.length < MAX_THREAD_TERMINAL_COUNT; const targetTerminalId = shouldCreateNewTerminal ? `terminal-${crypto.randomUUID()}` : baseTerminalId; @@ -701,7 +711,7 @@ export default function ChatView() { }, script, threadId: activeThreadId, - worktreePath: activeThread.worktreePath ?? null, + worktreePath: options?.worktreePath ?? activeThread.worktreePath ?? null, ...(options?.env ? { extraEnv: options.env } : {}), }), ); @@ -1350,6 +1360,7 @@ export default function ChatView() { if (setupScript) { void runProjectScript(setupScript, { cwd: result.worktree.path, + worktreePath: result.worktree.path, }); } } catch (err) { diff --git a/apps/web/src/projectScripts.test.ts b/apps/web/src/projectScripts.test.ts index cba0f418d26ee20cf1663e3483529082f7f8cf22..3180e793ef11ace0785c6d766a0555dee5df5783 100644 --- a/apps/web/src/projectScripts.test.ts +++ b/apps/web/src/projectScripts.test.ts @@ -34,6 +34,15 @@ describe("projectScripts helpers", () => { expect(command).toBe('set "T3CODE_PROJECT_ROOT=C:\\\\repo path" && bun install'); }); + it("escapes windows env values that trigger interpolation", () => { + const command = injectEnvIntoShellCommand( + "bun install", + { T3CODE_PROJECT_ROOT: '%USERPROFILE%^"repo"' }, + "Win32", + ); + expect(command).toBe('set "T3CODE_PROJECT_ROOT=%%USERPROFILE%%^^""repo""" && bun install'); + }); + it("resolves primary and setup scripts", () => { const scripts = [ { diff --git a/apps/web/src/projectScripts.ts b/apps/web/src/projectScripts.ts index a08f73bcac428e4fa3d3fd04b169fd52320ab6f9..a71bd3e3be651b7d9b01598ca0435cabf5b91a71 100644 --- a/apps/web/src/projectScripts.ts +++ b/apps/web/src/projectScripts.ts @@ -56,7 +56,7 @@ function shellEscapePosix(value: string): string { } function shellEscapeWindowsSetValue(value: string): string { - return value.replace(/"/g, '""'); + return value.replace(/\^/g, "^^").replace(/%/g, "%%").replace(/"/g, '""'); } export function injectEnvIntoShellCommand( diff --git a/apps/web/src/store.test.ts b/apps/web/src/store.test.ts index c6b9f4cb7188cf94f292043523e6f42ee13c513f..764d6ec4d479333629e9d1979b44a96c6e6330f7 100644 --- a/apps/web/src/store.test.ts +++ b/apps/web/src/store.test.ts @@ -558,6 +558,49 @@ describe("store reducer thread continuity", () => { expect(next.activeThreadId).toBe("thread-a"); }); + it("treats empty scripts from sync as authoritative", () => { + const state: AppState = { + projects: [ + { + id: "project-old-a", + name: "A", + cwd: "/tmp/a", + model: "gpt-5.3-codex", + expanded: true, + scripts: [ + { + id: "test", + name: "Test", + command: "bun test", + icon: "test", + runOnWorktreeCreate: false, + }, + ], + }, + ], + threads: [makeThread({ id: "thread-a", projectId: "project-old-a" })], + activeThreadId: "thread-a", + runtimeMode: "full-access", + diffOpen: false, + }; + + const next = reducer(state, { + type: "SYNC_PROJECTS", + projects: [ + { + id: "project-new-a", + name: "A", + cwd: "/tmp/a", + model: "gpt-5.3-codex", + expanded: false, + scripts: [], + }, + ], + }); + + expect(next.projects[0]?.scripts).toEqual([]); + }); + it("updates project scripts", () => { const state = makeState(makeThread()); const next = reducer(state, { diff --git a/apps/web/src/store.ts b/apps/web/src/store.ts index 8cc75e235b9c6f326d370c2ef4741f78c0dfc680..1e790fbea8674f830185c73a5ba0c80ce632ea5b 100644 --- a/apps/web/src/store.ts +++ b/apps/web/src/store.ts @@ -481,10 +481,7 @@ export function reducer(state: AppState, action: Action): AppState { ); const nextProjects = action.projects.map((project) => { const previous = previousByCwd.get(project.cwd); - const scripts = - project.scripts.length > 0 - ? normalizeProjectScripts(project.scripts) - : normalizeProjectScripts(previous?.scripts ?? []); + const scripts = normalizeProjectScripts(project.scripts); return { ...project, model: resolveModelSlug(previous?.model ?? project.model), -- 2.54.0