Skip to content

Commit 504469e

Browse files
kubrickcodekubrickcode
committed
fix: add missing shortcut duplicate validation in groups and root level
Commit 7d81194 removed cross-group validation but also removed same-group shortcut validation, causing errors to only appear at runtime instead of in the modal - Group validation: add hasDuplicateShortcutInGroup() and Zod schema refine - Root level validation: pass siblingCommands prop to CommandForm for pre-save check - Error messages: use exact message matching for correct translation display - E2E tests: add tests for within-group and root-level duplicate validation
1 parent fa238d7 commit 504469e

9 files changed

Lines changed: 262 additions & 28 deletions

File tree

‎src/view/e2e/duplicate-shortcut-root-level.spec.ts‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
import { expect, test } from "@playwright/test";
2+
3+
test.describe("Test H7: Duplicate Shortcut - Root Level", () => {
4+
test.beforeEach(async ({ page }) => {
5+
await page.goto("/");
6+
await page.waitForLoadState("networkidle");
7+
});
8+
9+
test("should show validation error when creating command with existing root shortcut", async ({
10+
page,
11+
}) => {
12+
// Click Add button to open new command dialog
13+
await page.getByRole("button", { name: "Add" }).click();
14+
const dialog = page.getByRole("dialog", { name: "Add New Command" });
15+
await expect(dialog).toBeVisible();
16+
17+
// Fill command name
18+
await page.getByPlaceholder(/Terminal, Build, Deploy/).fill("New Command");
19+
20+
// Fill command
21+
await page.getByPlaceholder(/npm start/).fill("echo test");
22+
23+
// Enter shortcut "g" which is already used by Git group
24+
await page.getByPlaceholder(/e\.g\., t/).fill("g");
25+
26+
// Click Save
27+
await page.getByRole("button", { name: "Save" }).click();
28+
29+
// Verify validation error for root level duplicate
30+
await expect(
31+
page.getByText(/this shortcut is already used by another command at the root level/i)
32+
).toBeVisible();
33+
34+
// Dialog should remain open
35+
await expect(dialog).toBeVisible();
36+
});
37+
38+
test("should show validation error for case-insensitive root level duplicate", async ({
39+
page,
40+
}) => {
41+
await page.getByRole("button", { name: "Add" }).click();
42+
const dialog = page.getByRole("dialog", { name: "Add New Command" });
43+
await expect(dialog).toBeVisible();
44+
45+
await page.getByPlaceholder(/Terminal, Build, Deploy/).fill("New Command");
46+
await page.getByPlaceholder(/npm start/).fill("echo test");
47+
48+
// Enter "G" (uppercase) - should conflict with "g" (Git)
49+
await page.getByPlaceholder(/e\.g\., t/).fill("G");
50+
51+
await page.getByRole("button", { name: "Save" }).click();
52+
53+
await expect(
54+
page.getByText(/this shortcut is already used by another command at the root level/i)
55+
).toBeVisible();
56+
await expect(dialog).toBeVisible();
57+
});
58+
59+
test("should allow saving with unique root level shortcut", async ({ page }) => {
60+
await page.getByRole("button", { name: "Add" }).click();
61+
const dialog = page.getByRole("dialog", { name: "Add New Command" });
62+
await expect(dialog).toBeVisible();
63+
64+
await page.getByPlaceholder(/Terminal, Build, Deploy/).fill("New Command");
65+
await page.getByPlaceholder(/npm start/).fill("echo test");
66+
67+
// Enter "x" which is not used
68+
await page.getByPlaceholder(/e\.g\., t/).fill("x");
69+
70+
await page.getByRole("button", { name: "Save" }).click();
71+
72+
// Dialog should close (save succeeded)
73+
await expect(dialog).not.toBeVisible();
74+
});
75+
76+
test("should allow editing command to keep its own shortcut", async ({ page }) => {
77+
// Edit existing Git command
78+
await page.getByRole("button", { name: /Edit command.*Git/ }).click();
79+
const dialog = page.getByRole("dialog", { name: "Edit Command" });
80+
await expect(dialog).toBeVisible();
81+
82+
// Shortcut should already be "g"
83+
const shortcutInput = dialog.getByPlaceholder(/e\.g\., t/);
84+
await expect(shortcutInput).toHaveValue("g");
85+
86+
// Save without changing - should succeed (not a duplicate of itself)
87+
await page.getByRole("button", { name: "Save" }).click();
88+
89+
await expect(dialog).not.toBeVisible();
90+
});
91+
92+
test("should show error when editing command to use another root shortcut", async ({
93+
page,
94+
}) => {
95+
// Edit Test command (shortcut: "t")
96+
await page.getByRole("button", { name: /Edit command.*Test/ }).click();
97+
const dialog = page.getByRole("dialog", { name: "Edit Command" });
98+
await expect(dialog).toBeVisible();
99+
100+
// Change shortcut to "g" (used by Git)
101+
const shortcutInput = dialog.getByPlaceholder(/e\.g\., t/);
102+
await shortcutInput.clear();
103+
await shortcutInput.fill("g");
104+
105+
await page.getByRole("button", { name: "Save" }).click();
106+
107+
await expect(
108+
page.getByText(/this shortcut is already used by another command at the root level/i)
109+
).toBeVisible();
110+
await expect(dialog).toBeVisible();
111+
});
112+
});

‎src/view/e2e/duplicate-shortcut-within-group.spec.ts‎

Lines changed: 62 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,7 @@ import { expect, test } from "@playwright/test";
33
test.describe("Test H6: Duplicate Shortcut - Within Group", () => {
44
test.beforeEach(async ({ page }) => {
55
await page.goto("/");
6-
// Wait for mock data to load
7-
await page.waitForTimeout(500);
6+
await page.waitForLoadState("networkidle");
87
});
98

109
test("should show validation error when entering duplicate shortcut within group", async ({
@@ -30,26 +29,37 @@ test.describe("Test H6: Duplicate Shortcut - Within Group", () => {
3029
await page.getByRole("button", { name: "Save" }).click();
3130

3231
// Verify validation error is shown for duplicate shortcut
33-
// Or dialog should remain open if validation blocks save
34-
const duplicateErrorVisible = await page
35-
.getByText(/duplicate|already in use|shortcut.*l/i)
36-
.isVisible()
37-
.catch(() => false);
38-
39-
// If no error shown, dialog should have closed (validation may not exist for within-group)
40-
const dialogStillOpen = await dialog.isVisible();
41-
42-
// Document the behavior
43-
if (!duplicateErrorVisible && !dialogStillOpen) {
44-
// Validation doesn't block - this might be expected behavior
45-
// (shortcuts within a group might be allowed to be the same)
46-
console.log(
47-
"Note: Duplicate shortcuts within group are allowed (dialog closed without error)"
48-
);
49-
}
50-
51-
// At minimum, verify the dialog behavior is consistent
52-
expect(duplicateErrorVisible || !dialogStillOpen).toBe(true);
32+
await expect(page.getByText(/duplicate shortcuts found in group/i)).toBeVisible();
33+
34+
// Dialog should remain open (validation blocks save)
35+
await expect(dialog).toBeVisible();
36+
});
37+
38+
test("should show validation error for case-insensitive duplicate shortcuts", async ({
39+
page,
40+
}) => {
41+
// Open Git group edit dialog
42+
await page.getByRole("button", { name: /Edit command.*Git/ }).click();
43+
const dialog = page.getByRole("dialog", { name: "Edit Command" });
44+
await expect(dialog).toBeVisible();
45+
46+
// Get shortcut inputs within group
47+
const shortcutInputs = dialog.getByRole("textbox", {
48+
name: "Shortcut (optional)",
49+
});
50+
51+
// Change Push shortcut to "L" (uppercase, same as Pull's "l")
52+
await shortcutInputs.nth(1).clear();
53+
await shortcutInputs.nth(1).fill("L");
54+
55+
// Click Save
56+
await page.getByRole("button", { name: "Save" }).click();
57+
58+
// Verify validation error is shown (case-insensitive check)
59+
await expect(page.getByText(/duplicate shortcuts found in group/i)).toBeVisible();
60+
61+
// Dialog should remain open
62+
await expect(dialog).toBeVisible();
5363
});
5464

5565
test("should have different shortcuts for child commands in Git group", async ({
@@ -69,4 +79,34 @@ test.describe("Test H6: Duplicate Shortcut - Within Group", () => {
6979
await expect(pushShortcut).toBeVisible();
7080
await expect(checkStatusShortcut).toBeVisible();
7181
});
82+
83+
test("should allow saving when duplicate shortcut is fixed", async ({ page }) => {
84+
// Open Git group edit dialog
85+
await page.getByRole("button", { name: /Edit command.*Git/ }).click();
86+
const dialog = page.getByRole("dialog", { name: "Edit Command" });
87+
await expect(dialog).toBeVisible();
88+
89+
// Get shortcut inputs within group
90+
const shortcutInputs = dialog.getByRole("textbox", {
91+
name: "Shortcut (optional)",
92+
});
93+
94+
// Create duplicate shortcut
95+
await shortcutInputs.nth(1).clear();
96+
await shortcutInputs.nth(1).fill("l");
97+
98+
// Try to save - should fail
99+
await page.getByRole("button", { name: "Save" }).click();
100+
await expect(page.getByText(/duplicate shortcuts found in group/i)).toBeVisible();
101+
102+
// Fix the duplicate by using a unique shortcut
103+
await shortcutInputs.nth(1).clear();
104+
await shortcutInputs.nth(1).fill("x");
105+
106+
// Now save should succeed
107+
await page.getByRole("button", { name: "Save" }).click();
108+
109+
// Dialog should close (save succeeded)
110+
await expect(dialog).not.toBeVisible();
111+
});
72112
});

‎src/view/src/components/command-form-dialog.tsx‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,12 @@ import {
1414

1515
import { CommandForm } from "./command-form";
1616
import { useCommandForm } from "../context/command-form-context.tsx";
17+
import { useVscodeCommand } from "../context/vscode-command-context.tsx";
1718

1819
export const CommandFormDialog = () => {
1920
const { t } = useTranslation();
2021
const { closeForm, editingCommand, handleSave, resetFormState, showForm } = useCommandForm();
22+
const { commands } = useVscodeCommand();
2123
const formId = useId();
2224

2325
const handleOpenChange = (open: boolean) => {
@@ -48,7 +50,12 @@ export const CommandFormDialog = () => {
4850
</DialogDescription>
4951
</DialogHeader>
5052
<DialogBody>
51-
<CommandForm command={editingCommand} formId={formId} onSave={handleSave} />
53+
<CommandForm
54+
command={editingCommand}
55+
formId={formId}
56+
onSave={handleSave}
57+
siblingCommands={commands}
58+
/>
5259
</DialogBody>
5360
<DialogFooter>
5461
<Button onClick={closeForm} type="button" variant="outline">

‎src/view/src/components/command-form.tsx‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ type CommandFormProps = {
3838
command?: (ButtonConfig & { index?: number }) | null;
3939
formId?: string;
4040
onSave: (command: ButtonConfig) => void;
41+
siblingCommands?: ButtonConfig[];
4142
};
4243

4344
const createDefaultValues = (command?: ButtonConfig | null): ButtonConfigDraft =>
@@ -68,18 +69,22 @@ const buildCommandConfig = (data: ButtonConfigDraft, isGroup: boolean): ButtonCo
6869
return isGroup ? toGroupButton(normalized) : toCommandButton(normalized);
6970
};
7071

71-
export const CommandForm = ({ command, formId, onSave }: CommandFormProps) => {
72+
export const CommandForm = ({
73+
command,
74+
formId,
75+
onSave,
76+
siblingCommands = [],
77+
}: CommandFormProps) => {
7278
const { t } = useTranslation();
7379

74-
// Note: Shortcut uniqueness validation is handled at runtime in command-executor.ts
75-
// Shortcuts only need to be unique within the same group (QuickPick menu)
7680
const schema = useMemo(() => createCommandFormSchema(), []);
7781

7882
const {
7983
control,
8084
formState: { errors },
8185
handleSubmit,
8286
register,
87+
setError,
8388
setValue,
8489
watch,
8590
} = useForm<ButtonConfigDraft>({
@@ -90,6 +95,16 @@ export const CommandForm = ({ command, formId, onSave }: CommandFormProps) => {
9095
resolver: zodResolver(schema as any),
9196
});
9297

98+
const checkRootLevelShortcutDuplicate = (shortcut: string | undefined): boolean => {
99+
if (!shortcut || !shortcut.trim()) return false;
100+
const normalizedShortcut = shortcut.toLowerCase().trim();
101+
return siblingCommands.some((cmd) => {
102+
if (command?.id && cmd.id === command.id) return false;
103+
const cmdShortcut = cmd.shortcut?.toLowerCase().trim();
104+
return cmdShortcut && cmdShortcut.length > 0 && cmdShortcut === normalizedShortcut;
105+
});
106+
};
107+
93108
const hasGroup = command != null && "group" in command;
94109
const [isGroupMode, setIsGroupMode] = useState<boolean>(hasGroup);
95110
const [showWarningDialog, setShowWarningDialog] = useState(false);
@@ -102,6 +117,14 @@ export const CommandForm = ({ command, formId, onSave }: CommandFormProps) => {
102117
const insertOnly = watch("insertOnly");
103118

104119
const onSubmit = handleSubmit((data) => {
120+
if (checkRootLevelShortcutDuplicate(data.shortcut)) {
121+
setError("shortcut", {
122+
message: t("commandForm.errors.duplicateShortcutRoot"),
123+
type: "manual",
124+
});
125+
return;
126+
}
127+
105128
const hasChildCommands = data.group && data.group.length > 0;
106129
const isConvertingToSingle = originalIsGroupMode && !isGroupMode && hasChildCommands;
107130

@@ -167,6 +190,9 @@ export const CommandForm = ({ command, formId, onSave }: CommandFormProps) => {
167190
if (message.includes("name")) {
168191
return t("commandForm.errors.groupNameRequired");
169192
}
193+
if (message === "Duplicate shortcuts found in group") {
194+
return t("commandForm.errors.duplicateShortcut");
195+
}
170196
return t("commandForm.errors.groupCommandRequired");
171197
};
172198

‎src/view/src/components/group-edit-dialog.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ export const GroupEditDialog = ({
6363
return t("commandForm.errors.groupCommandRequired");
6464
case "emptyNestedGroup":
6565
return t("commandForm.errors.nestedGroupEmpty");
66+
case "duplicateShortcut":
67+
return t("commandForm.errors.duplicateShortcut");
6668
}
6769
};
6870

‎src/view/src/i18n/locales/en.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@
5656
"groupEmpty": "Group must have at least one command",
5757
"groupCommandRequired": "All commands in group must have a command",
5858
"groupNameRequired": "All items in group must have a name",
59-
"nestedGroupEmpty": "Nested groups must have at least one command"
59+
"nestedGroupEmpty": "Nested groups must have at least one command",
60+
"duplicateShortcut": "Duplicate shortcuts found in group. Each command must have a unique shortcut.",
61+
"duplicateShortcutRoot": "This shortcut is already used by another command at the root level."
6062
}
6163
},
6264
"commandFormDialog": {

‎src/view/src/i18n/locales/ko.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@
5656
"groupEmpty": "그룹에 최소 하나의 커맨드가 필요합니다",
5757
"groupCommandRequired": "그룹 내 모든 커맨드에 명령어를 입력해주세요",
5858
"groupNameRequired": "그룹 내 모든 항목에 이름을 입력해주세요",
59-
"nestedGroupEmpty": "중첩 그룹에 최소 하나의 커맨드가 필요합니다"
59+
"nestedGroupEmpty": "중첩 그룹에 최소 하나의 커맨드가 필요합니다",
60+
"duplicateShortcut": "그룹 내에 중복된 단축키가 있습니다. 각 명령은 고유한 단축키를 사용해야 합니다.",
61+
"duplicateShortcutRoot": "이 단축키는 이미 최상위 레벨의 다른 명령에서 사용 중입니다."
6062
}
6163
},
6264
"commandFormDialog": {

‎src/view/src/schemas/command-form-schema.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { z } from "zod";
22

33
import { type ButtonConfig } from "../types";
44
import {
5+
hasDuplicateShortcutInGroup,
56
hasEmptyCommandInGroup,
67
hasEmptyNameInGroup,
78
hasEmptyNestedGroup,
@@ -106,6 +107,17 @@ export const createCommandFormSchema = (): z.ZodType<ButtonConfig> => {
106107
message: "All commands in group must have a command",
107108
path: ["group"],
108109
}
110+
)
111+
.refine(
112+
(data) => {
113+
const hasGroup = data.group && data.group.length > 0;
114+
if (!hasGroup) return true;
115+
return !hasDuplicateShortcutInGroup(data.group!);
116+
},
117+
{
118+
message: "Duplicate shortcuts found in group",
119+
path: ["group"],
120+
}
109121
) as unknown as z.ZodType<ButtonConfig>;
110122
};
111123

0 commit comments

Comments
 (0)