Skip to content

Commit d15510b

Browse files
KubrickCodeKubrickCode
committed
Fix same-command buttons sharing terminal
Buttons with identical settings shared terminals due to incomplete uniqueness - Track button object references with WeakMap and auto-incrementing IDs - Generate terminal keys from JSON-serialized config including unique button ID - Maintain index-based IDs for executeAll group commands - Enable terminal reuse only for exact same button instance re-clicks fix #56
1 parent 58ac6ca commit d15510b

File tree

6 files changed

+109
-60
lines changed

6 files changed

+109
-60
lines changed

‎src/extension/src/adapters.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ const DEFAULT_REFRESH_CONFIG: RefreshButtonConfig = {
1111
export type TerminalExecutor = (
1212
command: string,
1313
useVsCodeApi?: boolean,
14-
terminalName?: string
14+
terminalName?: string,
15+
buttonName?: string,
16+
buttonRef?: object
1517
) => void;
1618

1719
export type ConfigReader = {

‎src/extension/src/command-executor.test.ts‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,7 @@ describe("command-executor", () => {
498498

499499
executeTerminalCommand(button, mockTerminalExecutor);
500500

501-
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", false, undefined);
501+
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", false, undefined, "Test Button", expect.objectContaining({ command: "echo test", name: "Test Button" }));
502502
});
503503

504504
it("should call terminalExecutor with useVsCodeApi true", () => {
@@ -511,7 +511,7 @@ describe("command-executor", () => {
511511

512512
executeTerminalCommand(button, mockTerminalExecutor);
513513

514-
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", true, undefined);
514+
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", true, undefined, "Test Button", expect.objectContaining({ command: "echo test", name: "Test Button", useVsCodeApi: true }));
515515
});
516516

517517
it("should call terminalExecutor with custom terminal name", () => {
@@ -524,7 +524,7 @@ describe("command-executor", () => {
524524

525525
executeTerminalCommand(button, mockTerminalExecutor);
526526

527-
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", false, "Custom Terminal");
527+
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", false, "Custom Terminal", "Test Button", expect.objectContaining({ command: "echo test", name: "Test Button", terminalName: "Custom Terminal" }));
528528
});
529529

530530
it("should call terminalExecutor with all parameters", () => {
@@ -538,7 +538,7 @@ describe("command-executor", () => {
538538

539539
executeTerminalCommand(button, mockTerminalExecutor);
540540

541-
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", true, "Custom Terminal");
541+
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo test", true, "Custom Terminal", "Test Button", expect.objectContaining({ command: "echo test", name: "Test Button", terminalName: "Custom Terminal", useVsCodeApi: true }));
542542
});
543543

544544
it("should not call terminalExecutor when command is undefined", () => {
@@ -588,13 +588,14 @@ describe("command-executor", () => {
588588
executeCommandsRecursively(commands, mockTerminalExecutor);
589589

590590
expect(mockTerminalExecutor).toHaveBeenCalledTimes(3);
591-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo test1", false, undefined);
592-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo test2", true, undefined);
591+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo test1", false, undefined, "Command 1[0]");
592+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo test2", true, undefined, "Command 2[1]");
593593
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(
594594
3,
595595
"echo test3",
596596
false,
597-
"Custom Terminal"
597+
"Custom Terminal",
598+
"Command 3[2]"
598599
);
599600
});
600601

@@ -621,8 +622,8 @@ describe("command-executor", () => {
621622
executeCommandsRecursively(commands, mockTerminalExecutor);
622623

623624
expect(mockTerminalExecutor).toHaveBeenCalledTimes(2);
624-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo child1", false, undefined);
625-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo child2", true, undefined);
625+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo child1", false, undefined, "Group Command[0]>Child 1[0]");
626+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo child2", true, undefined, "Group Command[0]>Child 2[1]");
626627
});
627628

628629
it("should not execute commands for buttons with groups but no executeAll flag", () => {
@@ -673,8 +674,8 @@ describe("command-executor", () => {
673674
executeCommandsRecursively(commands, mockTerminalExecutor);
674675

675676
expect(mockTerminalExecutor).toHaveBeenCalledTimes(2);
676-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo level3", false, undefined);
677-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo level2", false, undefined);
677+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo level3", false, undefined, "Level 1 Group[0]>Level 2 Group[0]>Level 3 Command[0]");
678+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo level2", false, undefined, "Level 1 Group[0]>Level 2 Command[1]");
678679
});
679680

680681
it("should skip buttons without commands and without groups", () => {
@@ -692,7 +693,7 @@ describe("command-executor", () => {
692693
executeCommandsRecursively(commands, mockTerminalExecutor);
693694

694695
expect(mockTerminalExecutor).toHaveBeenCalledTimes(1);
695-
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo valid", false, undefined);
696+
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo valid", false, undefined, "Valid Command[0]");
696697
});
697698

698699
it("should skip buttons with empty command strings", () => {
@@ -711,7 +712,7 @@ describe("command-executor", () => {
711712
executeCommandsRecursively(commands, mockTerminalExecutor);
712713

713714
expect(mockTerminalExecutor).toHaveBeenCalledTimes(1);
714-
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo valid", false, undefined);
715+
expect(mockTerminalExecutor).toHaveBeenCalledWith("echo valid", false, undefined, "Valid Command[0]");
715716
});
716717

717718
it("should handle empty commands array", () => {
@@ -758,8 +759,8 @@ describe("command-executor", () => {
758759
executeCommandsRecursively(commands, mockTerminalExecutor);
759760

760761
expect(mockTerminalExecutor).toHaveBeenCalledTimes(2);
761-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo regular", false, undefined);
762-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo child", false, undefined);
762+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo regular", false, undefined, "Regular Command[0]");
763+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo child", false, undefined, "Group with executeAll[1]>Child Command[0]");
763764
});
764765

765766
it("should handle complex nested structure with mixed executeAll flags", () => {
@@ -804,9 +805,9 @@ describe("command-executor", () => {
804805
executeCommandsRecursively(commands, mockTerminalExecutor);
805806

806807
expect(mockTerminalExecutor).toHaveBeenCalledTimes(3);
807-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo leaf1", false, undefined);
808-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo leaf2", false, undefined);
809-
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(3, "echo direct", false, undefined);
808+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(1, "echo leaf1", false, undefined, "Root Group[0]>Branch 1[0]>Leaf 1[0]");
809+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(2, "echo leaf2", false, undefined, "Root Group[0]>Branch 1[0]>Leaf 2[1]");
810+
expect(mockTerminalExecutor).toHaveBeenNthCalledWith(3, "echo direct", false, undefined, "Root Group[0]>Direct Command[2]");
810811
});
811812
});
812813
});

‎src/extension/src/command-executor.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,13 @@ export const executeTerminalCommand = (
135135
) => {
136136
if (!button.command) return;
137137

138-
terminalExecutor(button.command, button.useVsCodeApi || false, button.terminalName);
138+
terminalExecutor(
139+
button.command,
140+
button.useVsCodeApi || false,
141+
button.terminalName,
142+
button.name,
143+
button
144+
);
139145
};
140146

141147
export const executeButtonCommand = (
@@ -187,19 +193,22 @@ const showGroupQuickPick = (
187193

188194
export const executeCommandsRecursively = (
189195
commands: ButtonConfig[],
190-
terminalExecutor: TerminalExecutor
196+
terminalExecutor: TerminalExecutor,
197+
parentPath = ""
191198
): void => {
192-
commands.forEach((cmd) => {
199+
commands.forEach((cmd, index) => {
200+
const buttonId = parentPath ? `${parentPath}>${cmd.name}[${index}]` : `${cmd.name}[${index}]`;
201+
193202
if (cmd.group && cmd.executeAll) {
194-
executeCommandsRecursively(cmd.group, terminalExecutor);
203+
executeCommandsRecursively(cmd.group, terminalExecutor, buttonId);
195204
} else if (cmd.command) {
196-
terminalExecutor(cmd.command, cmd.useVsCodeApi || false, cmd.terminalName);
205+
terminalExecutor(cmd.command, cmd.useVsCodeApi || false, cmd.terminalName, buttonId);
197206
}
198207
});
199208
};
200209

201210
const executeAllCommands = (button: ButtonConfig, terminalExecutor: TerminalExecutor) => {
202211
if (!button.group) return;
203212

204-
executeCommandsRecursively(button.group, terminalExecutor);
213+
executeCommandsRecursively(button.group, terminalExecutor, button.name);
205214
};

‎src/extension/src/command-tree-provider.ts‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { ConfigReader, TerminalExecutor } from "./adapters";
33
import { ButtonConfig } from "./types";
44

55
export class CommandTreeItem extends vscode.TreeItem {
6+
public readonly buttonName: string;
67
public readonly commandString: string;
78
public readonly terminalName?: string;
89
public readonly useVsCodeApi: boolean;
@@ -11,12 +12,14 @@ export class CommandTreeItem extends vscode.TreeItem {
1112
label: string,
1213
commandString: string,
1314
useVsCodeApi: boolean = false,
14-
terminalName?: string
15+
terminalName?: string,
16+
buttonName?: string
1517
) {
1618
super(label, vscode.TreeItemCollapsibleState.None);
1719
this.commandString = commandString;
1820
this.useVsCodeApi = useVsCodeApi;
1921
this.terminalName = terminalName;
22+
this.buttonName = buttonName || label;
2023
this.tooltip = commandString;
2124
this.contextValue = "command";
2225
this.command = {
@@ -38,8 +41,10 @@ export class GroupTreeItem extends vscode.TreeItem {
3841

3942
type TreeItem = CommandTreeItem | GroupTreeItem;
4043

41-
export const createTreeItemsFromGroup = (commands: ButtonConfig[]): TreeItem[] => {
42-
return commands.map((cmd) => {
44+
export const createTreeItemsFromGroup = (commands: ButtonConfig[], parentPath = ""): TreeItem[] => {
45+
return commands.map((cmd, index) => {
46+
const buttonId = parentPath ? `${parentPath}>${cmd.name}[${index}]` : `${cmd.name}[${index}]`;
47+
4348
if (cmd.group) {
4449
return new GroupTreeItem(cmd.name, cmd.group);
4550
}
@@ -48,13 +53,16 @@ export const createTreeItemsFromGroup = (commands: ButtonConfig[]): TreeItem[] =
4853
cmd.name,
4954
cmd.command || "",
5055
cmd.useVsCodeApi || false,
51-
cmd.terminalName
56+
cmd.terminalName,
57+
buttonId
5258
);
5359
});
5460
};
5561

5662
export const createRootTreeItems = (buttons: ButtonConfig[]): TreeItem[] => {
57-
return buttons.map((button) => {
63+
return buttons.map((button, index) => {
64+
const buttonId = `${button.name}[${index}]`;
65+
5866
if (button.group) {
5967
return new GroupTreeItem(button.name, button.group);
6068
}
@@ -64,11 +72,12 @@ export const createRootTreeItems = (buttons: ButtonConfig[]): TreeItem[] => {
6472
button.name,
6573
button.command,
6674
button.useVsCodeApi || false,
67-
button.terminalName
75+
button.terminalName,
76+
buttonId
6877
);
6978
}
7079

71-
return new CommandTreeItem(button.name, "", false);
80+
return new CommandTreeItem(button.name, "", false, undefined, buttonId);
7281
});
7382
};
7483

@@ -82,7 +91,7 @@ export class CommandTreeProvider implements vscode.TreeDataProvider<TreeItem> {
8291
new CommandTreeProvider(configReader);
8392

8493
static executeFromTree = (item: CommandTreeItem, terminalExecutor: TerminalExecutor) => {
85-
terminalExecutor(item.commandString, item.useVsCodeApi, item.terminalName);
94+
terminalExecutor(item.commandString, item.useVsCodeApi, item.terminalName, item.buttonName);
8695
};
8796

8897
getChildren = (element?: TreeItem): Thenable<TreeItem[]> => {
@@ -91,7 +100,7 @@ export class CommandTreeProvider implements vscode.TreeDataProvider<TreeItem> {
91100
}
92101

93102
if (element instanceof GroupTreeItem) {
94-
return Promise.resolve(createTreeItemsFromGroup(element.commands));
103+
return Promise.resolve(createTreeItemsFromGroup(element.commands, element.label));
95104
}
96105

97106
return Promise.resolve([]);

‎src/extension/src/terminal-manager.test.ts‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -78,36 +78,43 @@ describe("terminal-manager", () => {
7878
jest.restoreAllMocks();
7979
});
8080

81-
it("should create separate terminals when customTerminalName is set", () => {
82-
manager.executeCommand("npm start", false, "build");
83-
manager.executeCommand("npm test", false, "build");
81+
it("should create separate terminals for different buttonNames with same command", () => {
82+
manager.executeCommand("npm start", false, "build", "Button A");
83+
manager.executeCommand("npm start", false, "build", "Button B");
8484

8585
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(2);
8686
expect(vscode.window.createTerminal).toHaveBeenNthCalledWith(1, "build");
8787
expect(vscode.window.createTerminal).toHaveBeenNthCalledWith(2, "build");
8888
});
8989

90-
it("should create new terminal every time when customTerminalName is set", () => {
91-
manager.executeCommand("npm start", false, "build");
92-
manager.executeCommand("npm start", false, "build");
90+
it("should reuse terminal for same button configuration", () => {
91+
manager.executeCommand("npm start", false, "build", "Button A");
92+
manager.executeCommand("npm start", false, "build", "Button A");
9393

94-
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(2);
94+
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(1);
9595
});
9696

97-
it("should reuse terminal for same command without customTerminalName", () => {
98-
manager.executeCommand("npm start", false);
99-
manager.executeCommand("npm start", false);
97+
it("should create separate terminals for same command with different terminalNames", () => {
98+
manager.executeCommand("just test", false, "", "just test");
99+
manager.executeCommand("just test", false, undefined, "just test");
100100

101-
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(1);
101+
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(2);
102+
expect(vscode.window.createTerminal).toHaveBeenNthCalledWith(1, "just");
103+
expect(vscode.window.createTerminal).toHaveBeenNthCalledWith(2, "just");
102104
});
103105

104-
it("should create separate terminals for different commands without customTerminalName", () => {
105-
manager.executeCommand("npm start", false);
106-
manager.executeCommand("npm test", false);
106+
it("should create separate terminals for executeAll group with same command", () => {
107+
manager.executeCommand("just test", false, "", "just test 1");
108+
manager.executeCommand("just test", false, undefined, "just test 2");
107109

108110
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(2);
109-
expect(vscode.window.createTerminal).toHaveBeenNthCalledWith(1, "npm");
110-
expect(vscode.window.createTerminal).toHaveBeenNthCalledWith(2, "npm");
111+
});
112+
113+
it("should reuse terminal when same button is clicked again", () => {
114+
manager.executeCommand("npm test", false, undefined, "Test Button");
115+
manager.executeCommand("npm test", false, undefined, "Test Button");
116+
117+
expect(vscode.window.createTerminal).toHaveBeenCalledTimes(1);
111118
});
112119
});
113120
});

‎src/extension/src/terminal-manager.ts‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ export const determineTerminalName = (
1313
};
1414

1515
export class TerminalManager {
16+
private buttonIds = new WeakMap<object, string>();
17+
private idCounter = 0;
1618
private terminals = new Map<string, vscode.Terminal>();
1719

1820
static create = (): TerminalManager => new TerminalManager();
@@ -24,29 +26,48 @@ export class TerminalManager {
2426
this.terminals.clear();
2527
};
2628

27-
executeCommand: TerminalExecutor = (command, useVsCodeApi = false, customTerminalName) => {
29+
executeCommand: TerminalExecutor = (
30+
command,
31+
useVsCodeApi = false,
32+
customTerminalName,
33+
buttonName,
34+
buttonRef
35+
) => {
2836
if (useVsCodeApi) {
2937
vscode.commands.executeCommand(command);
3038
return;
3139
}
3240

3341
const terminalName = determineTerminalName(customTerminalName, command);
42+
const uniqueId = this.getUniqueButtonId(buttonRef, buttonName);
43+
const terminalKey = JSON.stringify({
44+
command,
45+
name: uniqueId,
46+
terminalName: customTerminalName,
47+
useVsCodeApi,
48+
});
3449

35-
if (customTerminalName) {
36-
const terminal = vscode.window.createTerminal(terminalName);
37-
terminal.show();
38-
terminal.sendText(command);
39-
return;
40-
}
41-
42-
let terminal = this.terminals.get(command);
50+
let terminal = this.terminals.get(terminalKey);
4351

4452
if (shouldCreateNewTerminal(terminal)) {
4553
terminal = vscode.window.createTerminal(terminalName);
46-
this.terminals.set(command, terminal);
54+
this.terminals.set(terminalKey, terminal);
4755
}
4856

4957
terminal!.show();
5058
terminal!.sendText(command);
5159
};
60+
61+
private getUniqueButtonId(buttonRef?: object, buttonName?: string): string {
62+
if (buttonRef) {
63+
let id = this.buttonIds.get(buttonRef);
64+
if (!id) {
65+
id = `btn-${this.idCounter++}`;
66+
this.buttonIds.set(buttonRef, id);
67+
}
68+
return id;
69+
}
70+
71+
return buttonName ?? `temp-${this.idCounter++}`;
72+
}
5273
}

0 commit comments

Comments
 (0)