authorgravatar for julius0216@outlook.comJulius Marminge <julius0216@outlook.com> 2026-02-17 21:17:57-08:00
committergravatar for jmarminge@gmail.comJulius Marminge <jmarminge@gmail.com> 2026-02-18 00:59:06-08:00
log9799b42dcd46fec621ab3abf48332f22950537af
treeeafbeb76a750eca24da6b03cb688cc330bf8a20c
parenta999933297969e269a15cda37c4999bc3b8d8c93

Address PR review feedback for scripts/keybindings handling

Co-authored-by: codex <codex@users.noreply.github.com>

7 files changed, 146 insertions(+), 12 deletions(-)

apps/server/src/keybindings.test.ts+45
...@@ -144,4 +144,49 @@ describe("server keybindings", () => {...@@ -144,4 +144,49 @@ describe("server keybindings", () => {
144 }>;144 }>;
145 expect(persisted).toEqual([{ key: "mod+shift+r", command: "script.run-tests.run" }]);145 expect(persisted).toEqual([{ key: "mod+shift+r", command: "script.run-tests.run" }]);
146 });146 });
147
148 it("refuses to overwrite malformed keybindings config", () => {
149 const fakeHome = makeTempDir("t3code-keybindings-malformed-");
150 const configDir = path.join(fakeHome, ".t3");
151 fs.mkdirSync(configDir, { recursive: true });
152 const configPath = path.join(configDir, "keybindings.json");
153 fs.writeFileSync(configPath, "{ not-json", "utf8");
154 vi.spyOn(os, "homedir").mockReturnValue(fakeHome);
155
156 expect(() =>
157 upsertKeybindingRule(logger, {
158 key: "mod+shift+r",
159 command: "script.run-tests.run",
160 }),
161 ).toThrow(/Unable to parse keybindings config/);
162
163 expect(fs.readFileSync(configPath, "utf8")).toBe("{ not-json");
164 });
165
166 it("cleans up temp files when atomic keybinding write fails", () => {
167 const fakeHome = makeTempDir("t3code-keybindings-atomic-");
168 const configDir = path.join(fakeHome, ".t3");
169 fs.mkdirSync(configDir, { recursive: true });
170 const configPath = path.join(configDir, "keybindings.json");
171 fs.writeFileSync(configPath, JSON.stringify([{ key: "mod+j", command: "terminal.toggle" }]), "utf8");
172 vi.spyOn(os, "homedir").mockReturnValue(fakeHome);
173 vi.spyOn(fs, "renameSync").mockImplementation(() => {
174 throw new Error("rename failed");
175 });
176
177 expect(() =>
178 upsertKeybindingRule(logger, {
179 key: "mod+shift+r",
180 command: "script.run-tests.run",
181 }),
182 ).toThrow(/rename failed/);
183
184 expect(fs.readFileSync(configPath, "utf8")).toBe(
185 JSON.stringify([{ key: "mod+j", command: "terminal.toggle" }]),
186 );
187 const tempFiles = fs
188 .readdirSync(configDir)
189 .filter((file) => file.includes("keybindings.json.") && file.endsWith(".tmp"));
190 expect(tempFiles).toEqual([]);
191 });
147});192});
apps/server/src/keybindings.ts+32-3
...@@ -281,12 +281,20 @@ function resolveKeybindingsConfigPath(): string {...@@ -281,12 +281,20 @@ function resolveKeybindingsConfigPath(): string {
281 return path.join(os.homedir(), KEYBINDINGS_CONFIG_PATH);281 return path.join(os.homedir(), KEYBINDINGS_CONFIG_PATH);
282}282}
283283
284function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConfig {284function loadCustomKeybindingsConfig(
285 logger: KeybindingsLogger,
286 options?: {
287 throwOnUnreadableConfig?: boolean;
288 },
289): KeybindingsConfig {
285 const configPath = resolveKeybindingsConfigPath();290 const configPath = resolveKeybindingsConfigPath();
286 try {291 try {
287 const raw = fs.readFileSync(configPath, "utf8");292 const raw = fs.readFileSync(configPath, "utf8");
288 const parsed = JSON.parse(raw) as unknown;293 const parsed = JSON.parse(raw) as unknown;
289 if (!Array.isArray(parsed)) {294 if (!Array.isArray(parsed)) {
295 if (options?.throwOnUnreadableConfig) {
296 throw new Error(`Unable to parse keybindings config at ${configPath}: expected JSON array`);
297 }
290 logger.warn("ignoring keybindings config with unsupported format; expected array", {298 logger.warn("ignoring keybindings config with unsupported format; expected array", {
291 path: configPath,299 path: configPath,
292 });300 });
...@@ -319,6 +327,14 @@ function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConf...@@ -319,6 +327,14 @@ function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConf
319 if ((error as NodeJS.ErrnoException)?.code === "ENOENT") {327 if ((error as NodeJS.ErrnoException)?.code === "ENOENT") {
320 return [];328 return [];
321 }329 }
330 if (options?.throwOnUnreadableConfig) {
331 throw new Error(
332 `Unable to parse keybindings config at ${configPath}: ${
333 error instanceof Error ? error.message : String(error)
334 }`,
335 { cause: error },
336 );
337 }
322 logger.warn("ignoring malformed keybindings config", {338 logger.warn("ignoring malformed keybindings config", {
323 path: configPath,339 path: configPath,
324 error: error instanceof Error ? error.message : String(error),340 error: error instanceof Error ? error.message : String(error),
...@@ -327,6 +343,17 @@ function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConf...@@ -327,6 +343,17 @@ function loadCustomKeybindingsConfig(logger: KeybindingsLogger): KeybindingsConf
327 }343 }
328}344}
329345
346function writeConfigAtomically(configPath: string, config: KeybindingsConfig): void {
347 const tempPath = `${configPath}.${process.pid}.${Date.now()}.tmp`;
348 try {
349 fs.writeFileSync(tempPath, `${JSON.stringify(config, null, 2)}\n`, "utf8");
350 fs.renameSync(tempPath, configPath);
351 } catch (error) {
352 fs.rmSync(tempPath, { force: true });
353 throw error;
354 }
355}
356
330function mergeWithDefaultKeybindings(custom: ResolvedKeybindingsConfig): ResolvedKeybindingsConfig {357function mergeWithDefaultKeybindings(custom: ResolvedKeybindingsConfig): ResolvedKeybindingsConfig {
331 if (custom.length === 0) {358 if (custom.length === 0) {
332 return [...DEFAULT_RESOLVED_KEYBINDINGS];359 return [...DEFAULT_RESOLVED_KEYBINDINGS];
...@@ -375,7 +402,9 @@ export function upsertKeybindingRule(...@@ -375,7 +402,9 @@ export function upsertKeybindingRule(
375 }402 }
376403
377 const configPath = resolveKeybindingsConfigPath();404 const configPath = resolveKeybindingsConfigPath();
378 const customConfig = loadCustomKeybindingsConfig(logger);405 const customConfig = loadCustomKeybindingsConfig(logger, {
406 throwOnUnreadableConfig: true,
407 });
379 const nextConfig = [...customConfig.filter((entry) => entry.command !== rule.command), rule];408 const nextConfig = [...customConfig.filter((entry) => entry.command !== rule.command), rule];
380 const cappedConfig =409 const cappedConfig =
381 nextConfig.length > MAX_KEYBINDINGS ? nextConfig.slice(-MAX_KEYBINDINGS) : nextConfig;410 nextConfig.length > MAX_KEYBINDINGS ? nextConfig.slice(-MAX_KEYBINDINGS) : nextConfig;
...@@ -388,7 +417,7 @@ export function upsertKeybindingRule(...@@ -388,7 +417,7 @@ export function upsertKeybindingRule(
388 }417 }
389418
390 fs.mkdirSync(path.dirname(configPath), { recursive: true });419 fs.mkdirSync(path.dirname(configPath), { recursive: true });
391 fs.writeFileSync(configPath, `${JSON.stringify(cappedConfig, null, 2)}\n`, "utf8");420 writeConfigAtomically(configPath, cappedConfig);
392421
393 const compiledCustomConfig = compileResolvedKeybindingsConfig(cappedConfig);422 const compiledCustomConfig = compileResolvedKeybindingsConfig(cappedConfig);
394 return mergeWithDefaultKeybindings(compiledCustomConfig);423 return mergeWithDefaultKeybindings(compiledCustomConfig);
apps/web/src/components/ChatView.tsx+15-4
...@@ -60,7 +60,11 @@ import {...@@ -60,7 +60,11 @@ import {
60} from "../session-logic";60} from "../session-logic";
61import { isScrollContainerNearBottom } from "../chat-scroll";61import { isScrollContainerNearBottom } from "../chat-scroll";
62import { useStore } from "../store";62import { useStore } from "../store";
63import { MAX_THREAD_TERMINAL_COUNT, type ChatImageAttachment } from "../types";63import {
64 DEFAULT_THREAD_TERMINAL_ID,
65 MAX_THREAD_TERMINAL_COUNT,
66 type ChatImageAttachment,
67} from "../types";
64import { basenameOfPath, getVscodeIconUrlForEntry } from "../vscode-icons";68import { basenameOfPath, getVscodeIconUrlForEntry } from "../vscode-icons";
65import { useTheme } from "../hooks/useTheme";69import { useTheme } from "../hooks/useTheme";
66import BranchToolbar from "./BranchToolbar";70import BranchToolbar from "./BranchToolbar";
...@@ -655,6 +659,7 @@ export default function ChatView() {...@@ -655,6 +659,7 @@ export default function ChatView() {
655 options?: {659 options?: {
656 cwd?: string;660 cwd?: string;
657 env?: Record<string, string>;661 env?: Record<string, string>;
662 worktreePath?: string | null;
658 preferNewTerminal?: boolean;663 preferNewTerminal?: boolean;
659 },664 },
660 ) => {665 ) => {
...@@ -664,9 +669,14 @@ export default function ChatView() {...@@ -664,9 +669,14 @@ export default function ChatView() {
664 return { ...current, [activeProject.id]: script.id };669 return { ...current, [activeProject.id]: script.id };
665 });670 });
666 const targetCwd = options?.cwd ?? gitCwd ?? activeProject.cwd;671 const targetCwd = options?.cwd ?? gitCwd ?? activeProject.cwd;
667 const baseTerminalId = activeThread.activeTerminalId;672 const baseTerminalId =
673 activeThread.activeTerminalId ||
674 activeThread.terminalIds[0] ||
675 DEFAULT_THREAD_TERMINAL_ID;
668 const isBaseTerminalBusy = activeThread.runningTerminalIds.includes(baseTerminalId);676 const isBaseTerminalBusy = activeThread.runningTerminalIds.includes(baseTerminalId);
669 const shouldCreateNewTerminal = Boolean(options?.preferNewTerminal) || isBaseTerminalBusy;677 const wantsNewTerminal = Boolean(options?.preferNewTerminal) || isBaseTerminalBusy;
678 const shouldCreateNewTerminal =
679 wantsNewTerminal && activeThread.terminalIds.length < MAX_THREAD_TERMINAL_COUNT;
670 const targetTerminalId = shouldCreateNewTerminal680 const targetTerminalId = shouldCreateNewTerminal
671 ? `terminal-${crypto.randomUUID()}`681 ? `terminal-${crypto.randomUUID()}`
672 : baseTerminalId;682 : baseTerminalId;
...@@ -701,7 +711,7 @@ export default function ChatView() {...@@ -701,7 +711,7 @@ export default function ChatView() {
701 },711 },
702 script,712 script,
703 threadId: activeThreadId,713 threadId: activeThreadId,
704 worktreePath: activeThread.worktreePath ?? null,714 worktreePath: options?.worktreePath ?? activeThread.worktreePath ?? null,
705 ...(options?.env ? { extraEnv: options.env } : {}),715 ...(options?.env ? { extraEnv: options.env } : {}),
706 }),716 }),
707 );717 );
...@@ -1350,6 +1360,7 @@ export default function ChatView() {...@@ -1350,6 +1360,7 @@ export default function ChatView() {
1350 if (setupScript) {1360 if (setupScript) {
1351 void runProjectScript(setupScript, {1361 void runProjectScript(setupScript, {
1352 cwd: result.worktree.path,1362 cwd: result.worktree.path,
1363 worktreePath: result.worktree.path,
1353 });1364 });
1354 }1365 }
1355 } catch (err) {1366 } catch (err) {
apps/web/src/projectScripts.test.ts+9
...@@ -34,6 +34,15 @@ describe("projectScripts helpers", () => {...@@ -34,6 +34,15 @@ describe("projectScripts helpers", () => {
34 expect(command).toBe('set "T3CODE_PROJECT_ROOT=C:\\\\repo path" && bun install');34 expect(command).toBe('set "T3CODE_PROJECT_ROOT=C:\\\\repo path" && bun install');
35 });35 });
3636
37 it("escapes windows env values that trigger interpolation", () => {
38 const command = injectEnvIntoShellCommand(
39 "bun install",
40 { T3CODE_PROJECT_ROOT: '%USERPROFILE%^"repo"' },
41 "Win32",
42 );
43 expect(command).toBe('set "T3CODE_PROJECT_ROOT=%%USERPROFILE%%^^""repo""" && bun install');
44 });
45
37 it("resolves primary and setup scripts", () => {46 it("resolves primary and setup scripts", () => {
38 const scripts = [47 const scripts = [
39 {48 {
apps/web/src/projectScripts.ts+1-1
...@@ -56,7 +56,7 @@ function shellEscapePosix(value: string): string {...@@ -56,7 +56,7 @@ function shellEscapePosix(value: string): string {
56}56}
5757
58function shellEscapeWindowsSetValue(value: string): string {58function shellEscapeWindowsSetValue(value: string): string {
59 return value.replace(/"/g, '""');59 return value.replace(/\^/g, "^^").replace(/%/g, "%%").replace(/"/g, '""');
60}60}
6161
62export function injectEnvIntoShellCommand(62export function injectEnvIntoShellCommand(
apps/web/src/store.test.ts+43
...@@ -558,6 +558,49 @@ describe("store reducer thread continuity", () => {...@@ -558,6 +558,49 @@ describe("store reducer thread continuity", () => {
558 expect(next.activeThreadId).toBe("thread-a");558 expect(next.activeThreadId).toBe("thread-a");
559 });559 });
560560
561 it("treats empty scripts from sync as authoritative", () => {
562 const state: AppState = {
563 projects: [
564 {
565 id: "project-old-a",
566 name: "A",
567 cwd: "/tmp/a",
568 model: "gpt-5.3-codex",
569 expanded: true,
570 scripts: [
571 {
572 id: "test",
573 name: "Test",
574 command: "bun test",
575 icon: "test",
576 runOnWorktreeCreate: false,
577 },
578 ],
579 },
580 ],
581 threads: [makeThread({ id: "thread-a", projectId: "project-old-a" })],
582 activeThreadId: "thread-a",
583 runtimeMode: "full-access",
584 diffOpen: false,
585 };
586
587 const next = reducer(state, {
588 type: "SYNC_PROJECTS",
589 projects: [
590 {
591 id: "project-new-a",
592 name: "A",
593 cwd: "/tmp/a",
594 model: "gpt-5.3-codex",
595 expanded: false,
596 scripts: [],
597 },
598 ],
599 });
600
601 expect(next.projects[0]?.scripts).toEqual([]);
602 });
603
561 it("updates project scripts", () => {604 it("updates project scripts", () => {
562 const state = makeState(makeThread());605 const state = makeState(makeThread());
563 const next = reducer(state, {606 const next = reducer(state, {
apps/web/src/store.ts+1-4
...@@ -481,10 +481,7 @@ export function reducer(state: AppState, action: Action): AppState {...@@ -481,10 +481,7 @@ export function reducer(state: AppState, action: Action): AppState {
481 );481 );
482 const nextProjects = action.projects.map((project) => {482 const nextProjects = action.projects.map((project) => {
483 const previous = previousByCwd.get(project.cwd);483 const previous = previousByCwd.get(project.cwd);
484 const scripts =484 const scripts = normalizeProjectScripts(project.scripts);
485 project.scripts.length > 0
486 ? normalizeProjectScripts(project.scripts)
487 : normalizeProjectScripts(previous?.scripts ?? []);
488 return {485 return {
489 ...project,486 ...project,
490 model: resolveModelSlug(previous?.model ?? project.model),487 model: resolveModelSlug(previous?.model ?? project.model),