From 939f606732a84c42f207a8e58cd5a308ea5b82de Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Mon, 1 Jun 2026 23:22:25 -0500 Subject: [PATCH] fix(navigation): block invalid book names and preserve them after sync (#1013) Prevent fileDisplayName cleanup from treating periods as file extensions, validate unsafe characters in the edit modal, and reject invalid names on save so sync no longer truncates names like "1. New Items" to "1". --- sharedUtils/bookNameValidation.ts | 71 ++++++++++++++ sharedUtils/index.ts | 1 + .../navigationWebviewProvider.ts | 33 +++++-- src/test/suite/bookNameValidation.test.ts | 78 ++++++++++++++++ .../suite/navigationWebviewProvider.test.ts | 89 ++++++++++++++++++ .../suite/notebookMetadataManager.test.ts | 70 ++++++++++++++ src/utils/notebookMetadataManager.ts | 51 ++++++++-- .../src/NavigationView/index.tsx | 14 ++- .../src/components/RenameModal.tsx | 38 +++++++- .../components/__tests__/RenameModal.test.tsx | 92 +++++++++++++++++++ 10 files changed, 512 insertions(+), 25 deletions(-) create mode 100644 sharedUtils/bookNameValidation.ts create mode 100644 src/test/suite/bookNameValidation.test.ts create mode 100644 webviews/codex-webviews/src/components/__tests__/RenameModal.test.tsx diff --git a/sharedUtils/bookNameValidation.ts b/sharedUtils/bookNameValidation.ts new file mode 100644 index 000000000..5cb90e0e1 --- /dev/null +++ b/sharedUtils/bookNameValidation.ts @@ -0,0 +1,71 @@ +/** + * Validation rules for user-edited book display names (`fileDisplayName`). + * + * Keep this list narrow and conservative. The primary motivation (issue #1013) + * is that earlier metadata-cleanup logic ran `path.extname()` over the display + * name and treated anything after a `.` as a strippable extension, silently + * truncating names like "1. New Items" to "1" on every sync. The safest fix + * is to disallow filesystem-unsafe characters at the entry point so the name + * round-trips cleanly through metadata, file IO, and any future tooling that + * may interpret the value as a path. + * + * Shared between the extension host (defensive validation in providers) and + * the webview (live input warnings + disabled save). + */ + +/** + * Characters that must not appear in a book display name. + * + * `.` is the bug from issue #1013. The remaining characters are reserved on + * common filesystems / URLs and would similarly create downstream surprises + * if they leaked into the display name. + */ +export const DISALLOWED_BOOK_NAME_CHARS: readonly string[] = [ + ".", + "/", + "\\", + ":", + "*", + "?", + '"', + "<", + ">", + "|", +]; + +/** + * Returns the subset of {@link DISALLOWED_BOOK_NAME_CHARS} present in `name`. + * Returns an empty array if the name is valid (or empty). + */ +export function findDisallowedBookNameChars(name: string): string[] { + if (!name) return []; + const seen = new Set(); + for (const ch of name) { + if (DISALLOWED_BOOK_NAME_CHARS.includes(ch)) { + seen.add(ch); + } + } + return Array.from(seen); +} + +/** + * True if the name contains any disallowed character. Whitespace-only / empty + * names are NOT considered "invalid" by this function — those are handled + * separately by callers that already trim and reject empty input. + */ +export function bookNameHasDisallowedChars(name: string): boolean { + return findDisallowedBookNameChars(name).length > 0; +} + +/** + * Human-friendly explanation of why a name is rejected, suitable for showing + * inline beneath an input field. Returns `null` when the name is valid. + */ +export function getBookNameValidationMessage(name: string): string | null { + const offenders = findDisallowedBookNameChars(name); + if (offenders.length === 0) return null; + + const list = offenders.map((c) => `"${c}"`).join(", "); + const noun = offenders.length === 1 ? "character" : "characters"; + return `${list} ${offenders.length === 1 ? "is" : "are"} not allowed in book names. Use dashes (-) or underscores (_) instead.`; +} diff --git a/sharedUtils/index.ts b/sharedUtils/index.ts index 8369f2f51..fba35af20 100644 --- a/sharedUtils/index.ts +++ b/sharedUtils/index.ts @@ -283,3 +283,4 @@ export const deriveTargetPathFromSource = (sourcePath: string): string => { // Re-export corpus utilities export * from "./corpusUtils"; export * from "./exportOptionsEligibility"; +export * from "./bookNameValidation"; diff --git a/src/providers/navigationWebview/navigationWebviewProvider.ts b/src/providers/navigationWebview/navigationWebviewProvider.ts index 17afcf099..076f44664 100644 --- a/src/providers/navigationWebview/navigationWebviewProvider.ts +++ b/src/providers/navigationWebview/navigationWebviewProvider.ts @@ -7,7 +7,7 @@ import { BaseWebviewProvider } from "../../globalProvider"; import { getWebviewHtml } from "../../utils/webviewTemplate"; import { safePostMessageToView } from "../../utils/webviewUtils"; import { CodexItem } from "types"; -import { getCellValueData, cellHasAudioUsingAttachments, computeValidationStats, computeProgressPercents, shouldExcludeCellFromProgress, shouldExcludeQuillCellFromProgress, countActiveValidations, hasTextContent } from "../../../sharedUtils"; +import { getCellValueData, cellHasAudioUsingAttachments, computeValidationStats, computeProgressPercents, shouldExcludeCellFromProgress, shouldExcludeQuillCellFromProgress, countActiveValidations, hasTextContent, getBookNameValidationMessage } from "../../../sharedUtils"; import { normalizeCorpusMarker } from "../../utils/corpusMarkerUtils"; import { addMetadataEdit, addProjectMetadataEdit, EditMapUtils } from "../../utils/editMapUtils"; import { MetadataManager } from "../../utils/metadataManager"; @@ -957,6 +957,21 @@ export class NavigationWebviewProvider extends BaseWebviewProvider { return; } + // Defensive: reject names containing characters that would later be misinterpreted + // by filesystem-style cleanup (see issue #1013) or are otherwise unsafe in paths. + // The webview also blocks these client-side, but we re-validate here in case a + // stale webview, command, or future caller bypasses that check. + const trimmedNewBookName = (newBookName ?? "").trim(); + if (!trimmedNewBookName) { + vscode.window.showErrorMessage("Book name cannot be empty."); + return; + } + const validationError = getBookNameValidationMessage(trimmedNewBookName); + if (validationError) { + vscode.window.showErrorMessage(`Cannot update book name: ${validationError}`); + return; + } + try { // Find codex files matching the book abbreviation and read metadata from first file const { matchingUris, corpusMarker } = await findCodexFilesByBookAbbr(bookAbbr, { readMetadata: true }); @@ -1019,15 +1034,15 @@ export class NavigationWebviewProvider extends BaseWebviewProvider { const oldValue = metadata.fileDisplayName; // Only add edit if value is actually changing - if (oldValue !== newBookName) { + if (oldValue !== trimmedNewBookName) { // Add edit history entry before updating metadata - addMetadataEdit(metadata, "fileDisplayName", newBookName, currentUser); + addMetadataEdit(metadata, "fileDisplayName", trimmedNewBookName, currentUser); } // Update metadata to add fileDisplayName (preserve originalName) notebookData.metadata = { ...metadata, - fileDisplayName: newBookName, + fileDisplayName: trimmedNewBookName, // Preserve originalName if it exists, don't modify it }; @@ -1068,15 +1083,15 @@ export class NavigationWebviewProvider extends BaseWebviewProvider { const sourceOldValue = sourceMetadata.fileDisplayName; // Only add edit if value is actually changing - if (sourceOldValue !== newBookName) { + if (sourceOldValue !== trimmedNewBookName) { // Add edit history entry before updating metadata - addMetadataEdit(sourceMetadata, "fileDisplayName", newBookName, currentUser); + addMetadataEdit(sourceMetadata, "fileDisplayName", trimmedNewBookName, currentUser); } // Update metadata to add fileDisplayName (preserve originalName) sourceNotebookData.metadata = { ...sourceMetadata, - fileDisplayName: newBookName, + fileDisplayName: trimmedNewBookName, // Preserve originalName if it exists, don't modify it }; @@ -1104,11 +1119,11 @@ export class NavigationWebviewProvider extends BaseWebviewProvider { if (updatedCount > 0) { vscode.window.showInformationMessage( - `Book name updated: "${bookAbbr}" → "${newBookName}" (${updatedCount} file(s) updated)` + `Book name updated: "${bookAbbr}" → "${trimmedNewBookName}" (${updatedCount} file(s) updated)` ); } else { vscode.window.showInformationMessage( - `Book name updated: "${bookAbbr}" → "${newBookName}" (no matching codex files found)` + `Book name updated: "${bookAbbr}" → "${trimmedNewBookName}" (no matching codex files found)` ); } } catch (error) { diff --git a/src/test/suite/bookNameValidation.test.ts b/src/test/suite/bookNameValidation.test.ts new file mode 100644 index 000000000..75f261045 --- /dev/null +++ b/src/test/suite/bookNameValidation.test.ts @@ -0,0 +1,78 @@ +import * as assert from "assert"; +import { + DISALLOWED_BOOK_NAME_CHARS, + findDisallowedBookNameChars, + bookNameHasDisallowedChars, + getBookNameValidationMessage, +} from "../../../sharedUtils/bookNameValidation"; + +suite("bookNameValidation Test Suite", () => { + test("DISALLOWED_BOOK_NAME_CHARS includes the period that triggered issue #1013", () => { + assert.ok( + DISALLOWED_BOOK_NAME_CHARS.includes("."), + "'.' must be in the disallowed list (root cause of #1013)" + ); + }); + + test("DISALLOWED_BOOK_NAME_CHARS includes filesystem-unsafe characters", () => { + const expected = ["/", "\\", ":", "*", "?", '"', "<", ">", "|"]; + for (const ch of expected) { + assert.ok( + DISALLOWED_BOOK_NAME_CHARS.includes(ch), + `'${ch}' should be disallowed in book names` + ); + } + }); + + test("findDisallowedBookNameChars returns offenders without duplicates", () => { + assert.deepStrictEqual(findDisallowedBookNameChars("1. New Items"), ["."]); + assert.deepStrictEqual( + findDisallowedBookNameChars("a/b\\c/d"), + ["/", "\\"], + "Repeated offenders should be deduplicated" + ); + assert.deepStrictEqual(findDisallowedBookNameChars("My Book"), []); + assert.deepStrictEqual(findDisallowedBookNameChars(""), []); + }); + + test("bookNameHasDisallowedChars distinguishes valid and invalid names", () => { + assert.strictEqual(bookNameHasDisallowedChars("1. New Items"), true); + assert.strictEqual(bookNameHasDisallowedChars("1.New"), true); + assert.strictEqual(bookNameHasDisallowedChars(".leading"), true); + assert.strictEqual(bookNameHasDisallowedChars("trailing."), true); + + assert.strictEqual(bookNameHasDisallowedChars("1 New Items"), false); + assert.strictEqual(bookNameHasDisallowedChars("1-New-Items"), false); + assert.strictEqual(bookNameHasDisallowedChars("1_New_Items"), false); + assert.strictEqual(bookNameHasDisallowedChars("Genesis"), false); + assert.strictEqual(bookNameHasDisallowedChars(""), false); + }); + + test("getBookNameValidationMessage returns null for valid names", () => { + assert.strictEqual(getBookNameValidationMessage("Genesis"), null); + assert.strictEqual(getBookNameValidationMessage("1-New-Items"), null); + assert.strictEqual(getBookNameValidationMessage("1_New_Items"), null); + assert.strictEqual(getBookNameValidationMessage(""), null); + }); + + test("getBookNameValidationMessage describes the offending characters", () => { + const message = getBookNameValidationMessage("1. New Items"); + assert.ok(message, "Should return a non-null message for invalid names"); + assert.ok(message!.includes('"."'), "Message should reference the period"); + assert.ok( + message!.toLowerCase().includes("dash") || message!.includes("-"), + "Message should suggest dashes as an alternative" + ); + assert.ok( + message!.toLowerCase().includes("underscore") || message!.includes("_"), + "Message should suggest underscores as an alternative" + ); + }); + + test("getBookNameValidationMessage handles multiple offenders", () => { + const message = getBookNameValidationMessage("a/b.c"); + assert.ok(message, "Should return a non-null message for invalid names"); + assert.ok(message!.includes('"/"'), "Message should reference '/'"); + assert.ok(message!.includes('"."'), "Message should reference '.'"); + }); +}); diff --git a/src/test/suite/navigationWebviewProvider.test.ts b/src/test/suite/navigationWebviewProvider.test.ts index 65fb7dec6..0c7406f7b 100644 --- a/src/test/suite/navigationWebviewProvider.test.ts +++ b/src/test/suite/navigationWebviewProvider.test.ts @@ -445,6 +445,95 @@ suite("NavigationWebviewProvider Test Suite", () => { } }); + test("updateBookName rejects names containing periods (issue #1013)", async () => { + if (!vscode.workspace.workspaceFolders?.[0]) { + return; + } + + const codexUri = await createCodexFileWithMetadata("GEN", { + fileDisplayName: "Genesis", + }); + + const loadBibleBookMap = (provider as any).loadBibleBookMap.bind(provider); + loadBibleBookMap(); + + const showErrorMessageStub = sinon.stub(vscode.window, "showErrorMessage"); + const showInformationMessageStub = sinon.stub(vscode.window, "showInformationMessage"); + const withProgressStub = sinon.stub(vscode.window, "withProgress").callsFake(async (options, callback) => { + return callback({ report: () => { } } as any, new vscode.CancellationTokenSource().token); + }); + + try { + const updateBookName = (provider as any).updateBookName.bind(provider); + await updateBookName("GEN", "1. New Items"); + + assert.ok( + showErrorMessageStub.called, + "Should show an error when the new name contains a disallowed character" + ); + + // The original fileDisplayName should be untouched on disk. + const content = await vscode.workspace.fs.readFile(codexUri); + const serializer = new CodexContentSerializer(); + const notebookData = await serializer.deserializeNotebook( + content, + new vscode.CancellationTokenSource().token + ); + assert.strictEqual( + (notebookData.metadata as any).fileDisplayName, + "Genesis", + "fileDisplayName must not be changed when validation fails" + ); + } finally { + showErrorMessageStub.restore(); + showInformationMessageStub.restore(); + withProgressStub.restore(); + } + }); + + test("updateBookName accepts names with dashes and underscores", async () => { + if (!vscode.workspace.workspaceFolders?.[0]) { + return; + } + + const codexUri = await createCodexFileWithMetadata("EXO", {}); + + const loadBibleBookMap = (provider as any).loadBibleBookMap.bind(provider); + loadBibleBookMap(); + + const showErrorMessageStub = sinon.stub(vscode.window, "showErrorMessage"); + const showInformationMessageStub = sinon.stub(vscode.window, "showInformationMessage"); + const withProgressStub = sinon.stub(vscode.window, "withProgress").callsFake(async (options, callback) => { + return callback({ report: () => { } } as any, new vscode.CancellationTokenSource().token); + }); + + try { + const updateBookName = (provider as any).updateBookName.bind(provider); + await updateBookName("EXO", "1-New_Items"); + + assert.ok( + !showErrorMessageStub.called, + "Should not show an error for a valid name with dashes/underscores" + ); + + const content = await vscode.workspace.fs.readFile(codexUri); + const serializer = new CodexContentSerializer(); + const notebookData = await serializer.deserializeNotebook( + content, + new vscode.CancellationTokenSource().token + ); + assert.strictEqual( + (notebookData.metadata as any).fileDisplayName, + "1-New_Items", + "fileDisplayName should be updated to the valid value" + ); + } finally { + showErrorMessageStub.restore(); + showInformationMessageStub.restore(); + withProgressStub.restore(); + } + }); + test("deleteFile closes open webview panels for codex file", async () => { // Skip if no workspace folder if (!vscode.workspace.workspaceFolders?.[0]) { diff --git a/src/test/suite/notebookMetadataManager.test.ts b/src/test/suite/notebookMetadataManager.test.ts index 9340cdd6c..edb084ac2 100644 --- a/src/test/suite/notebookMetadataManager.test.ts +++ b/src/test/suite/notebookMetadataManager.test.ts @@ -678,6 +678,76 @@ suite("NotebookMetadataManager Test Suite", function () { ); }); + test("should preserve fileDisplayName containing periods (issue #1013)", async function () { + this.timeout(5000); + if (!workspaceFolder) { + return; + } + + // Regression test: previously, stripFileExtensionFromDisplayName used path.extname() + // and treated ". New Items" as a strippable extension on every loadMetadata call, + // truncating the user-edited name "1. New Items" to "1" after each sync. + const userEditedName = "1. New Items"; + const codexUri = await createNotebookFile("ACT", false, { + fileDisplayName: userEditedName, + originalName: "Acts", + }); + + await manager.initialize(); + await ensureFileProcessedByLoadMetadata(codexUri); + + // Force a second loadMetadata pass to mimic what happens after a sync + // triggers the file watcher (the original bug only manifested after + // the cleanup pass ran a second time). + await manager.loadMetadata(); + + const serializer = new CodexContentSerializer(); + const content = await vscode.workspace.fs.readFile(codexUri); + const notebookData = await serializer.deserializeNotebook( + content, + new vscode.CancellationTokenSource().token + ); + const metadata = notebookData.metadata as CustomNotebookMetadata; + + assert.strictEqual( + metadata.fileDisplayName, + userEditedName, + "fileDisplayName containing a period should not be truncated" + ); + }); + + test("should still strip known file extensions from fileDisplayName", async function () { + this.timeout(5000); + if (!workspaceFolder) { + return; + } + + // The cleanup is intentionally narrowed to known extensions, but it + // must still kick in for legitimate cases (e.g. an importer accidentally + // wrote "Genesis.codex" into fileDisplayName). + const codexUri = await createNotebookFile("GE2", false, { + fileDisplayName: "Genesis.codex", + originalName: "Genesis", + }); + + await manager.initialize(); + await ensureFileProcessedByLoadMetadata(codexUri); + + const serializer = new CodexContentSerializer(); + const content = await vscode.workspace.fs.readFile(codexUri); + const notebookData = await serializer.deserializeNotebook( + content, + new vscode.CancellationTokenSource().token + ); + const metadata = notebookData.metadata as CustomNotebookMetadata; + + assert.strictEqual( + metadata.fileDisplayName, + "Genesis", + "Known file extensions should be stripped from fileDisplayName" + ); + }); + test("should update both .codex and .source files for same book", async function () { this.timeout(5000); if (!workspaceFolder) { diff --git a/src/utils/notebookMetadataManager.ts b/src/utils/notebookMetadataManager.ts index 790ed5002..f7bb3fbb1 100644 --- a/src/utils/notebookMetadataManager.ts +++ b/src/utils/notebookMetadataManager.ts @@ -33,6 +33,35 @@ interface MetadataValidationResult { errors: string[]; } +/** + * File extensions that may legitimately appear at the tail of a fileDisplayName + * (e.g. when the display name was originally derived from a source filename). + * These are stripped on load so the UI shows a clean name. Anything else after + * a `.` is treated as part of the user's chosen name and preserved as-is. + */ +const KNOWN_DISPLAY_NAME_EXTENSIONS: readonly string[] = [ + ".codex", + ".source", + ".scripture", + ".bible", + ".usfm", + ".sfm", + ".vtt", + ".srt", + ".mp4", + ".mp3", + ".wav", + ".m4a", + ".ogg", + ".webm", + ".txt", + ".md", + ".json", + ".csv", + ".tsv", + ".pdf", +]; + export function getNotebookMetadataManager(): NotebookMetadataManager { return NotebookMetadataManager.getManager(); } @@ -577,10 +606,16 @@ export class NotebookMetadataManager { } /** - * Strips file extensions from a display name if detected. - * Common extensions include: .vtt, .mp4, .mp3, .wav, .srt, .codex, .source, etc. + * Strips a known file extension from the end of a display name. + * + * Only the extensions in {@link KNOWN_DISPLAY_NAME_EXTENSIONS} are stripped. + * Using `path.extname` here would treat any text after the last `.` as an + * extension (e.g. `"1. New Items"` → strip `". New Items"` → `"1"`), which + * silently corrupts user-edited book names on every metadata reload after + * sync. See issue #1013. + * * @param displayName - The display name to clean - * @returns The display name without file extension + * @returns The display name without a known file extension */ private stripFileExtensionFromDisplayName(displayName: string): string { const trimmed = displayName.trim(); @@ -588,11 +623,11 @@ export class NotebookMetadataManager { return trimmed; } - // Check if the display name has a file extension - const ext = path.extname(trimmed); - if (ext && ext.length > 0) { - // Strip the extension using path.basename - return path.basename(trimmed, ext); + const lower = trimmed.toLowerCase(); + for (const ext of KNOWN_DISPLAY_NAME_EXTENSIONS) { + if (lower.endsWith(ext) && trimmed.length > ext.length) { + return trimmed.slice(0, -ext.length); + } } return trimmed; diff --git a/webviews/codex-webviews/src/NavigationView/index.tsx b/webviews/codex-webviews/src/NavigationView/index.tsx index d5b28112a..c103f6db9 100644 --- a/webviews/codex-webviews/src/NavigationView/index.tsx +++ b/webviews/codex-webviews/src/NavigationView/index.tsx @@ -16,6 +16,7 @@ import { Languages, Mic, ShieldCheck } from "lucide-react"; import { Switch } from "../components/ui/switch"; import { Label } from "../components/ui/label"; import { RenameModal } from "../components/RenameModal"; +import { getBookNameValidationMessage } from "@sharedUtils"; import { Dialog, DialogContent, @@ -657,16 +658,22 @@ function NavigationView() { const handleBookNameModalConfirm = () => { const { item, newName } = state.bookNameModal; - if (item && newName.trim() !== "") { + const trimmed = newName.trim(); + if (item && trimmed !== "") { + // Defensive: refuse to send an invalid name. The modal disables Save + // when this returns non-null, so users normally shouldn't reach here. + if (getBookNameValidationMessage(trimmed)) { + return; + } // Use fileDisplayName from metadata if available, otherwise fall back to formatted label const currentDisplayName = item.fileDisplayName || formatLabel(item.label, state.bibleBookMap || new Map()); - if (newName.trim() !== currentDisplayName) { + if (trimmed !== currentDisplayName) { vscode.postMessage({ command: "editBookName", content: { bookAbbr: item.label, - newBookName: newName.trim(), + newBookName: trimmed, }, }); } @@ -1106,6 +1113,7 @@ function NavigationView() { placeholder="Enter new book name" confirmButtonLabel="Save" disabled={disableBookNameButton} + validate={getBookNameValidationMessage} onClose={handleBookNameModalClose} onConfirm={handleBookNameModalConfirm} onValueChange={handleBookNameModalInputChange} diff --git a/webviews/codex-webviews/src/components/RenameModal.tsx b/webviews/codex-webviews/src/components/RenameModal.tsx index e281f910f..8af510357 100644 --- a/webviews/codex-webviews/src/components/RenameModal.tsx +++ b/webviews/codex-webviews/src/components/RenameModal.tsx @@ -19,6 +19,12 @@ interface RenameModalProps { placeholder?: string; confirmButtonLabel?: string; disabled?: boolean; + /** + * Optional inline validation. Returning a non-null string will display the + * message under the input and disable the confirm button. The component + * still respects `disabled` for non-validation reasons (e.g. empty input). + */ + validate?: (value: string) => string | null; onClose: () => void; onConfirm: () => void; onValueChange: (value: string) => void; @@ -33,10 +39,14 @@ export const RenameModal: React.FC = ({ placeholder, confirmButtonLabel = "Save", disabled = false, + validate, onClose, onConfirm, onValueChange, }) => { + const validationMessage = validate ? validate(value) : null; + const isInvalid = validationMessage !== null; + const isConfirmDisabled = disabled || isInvalid; const inputRef = useRef(null); // Focus input when modal opens @@ -53,7 +63,7 @@ export const RenameModal: React.FC = ({ const handleKeyDown = (e: React.KeyboardEvent) => { if (e.key === "Enter") { e.preventDefault(); - if (!disabled) { + if (!isConfirmDisabled) { onConfirm(); } } else if (e.key === "Escape") { @@ -104,22 +114,40 @@ export const RenameModal: React.FC = ({ onChange={(e) => onValueChange(e.target.value)} onKeyDown={handleKeyDown} placeholder={placeholder} - className="w-full mb-5 bg-vscode-input-background placeholder:text-gray-500 text-vscode-input-foreground border-vscode-input-border" + aria-invalid={isInvalid} + aria-describedby={isInvalid ? "rename-modal-error" : undefined} + className="w-full bg-vscode-input-background placeholder:text-gray-500 text-vscode-input-foreground border-vscode-input-border" style={{ padding: "8px", fontSize: "14px", backgroundColor: "var(--vscode-input-background)", color: "var(--vscode-input-foreground)", - borderColor: "var(--vscode-input-border)", + borderColor: isInvalid + ? "var(--vscode-inputValidation-errorBorder, var(--vscode-errorForeground))" + : "var(--vscode-input-border)", borderRadius: "6px", - marginBottom: "20px", + marginBottom: isInvalid ? "8px" : "20px", }} /> + {isInvalid && ( + + )} - diff --git a/webviews/codex-webviews/src/components/__tests__/RenameModal.test.tsx b/webviews/codex-webviews/src/components/__tests__/RenameModal.test.tsx new file mode 100644 index 000000000..09d3a2330 --- /dev/null +++ b/webviews/codex-webviews/src/components/__tests__/RenameModal.test.tsx @@ -0,0 +1,92 @@ +import React from "react"; +import { render, screen, fireEvent } from "@testing-library/react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import "@testing-library/jest-dom"; +import { RenameModal } from "../RenameModal"; +import { getBookNameValidationMessage } from "@sharedUtils"; + +describe("RenameModal validation", () => { + const onClose = vi.fn(); + const onConfirm = vi.fn(); + const onValueChange = vi.fn(); + + beforeEach(() => { + onClose.mockReset(); + onConfirm.mockReset(); + onValueChange.mockReset(); + }); + + const renderModal = (value: string) => + render( + + ); + + it("does not show a warning for a valid name", () => { + renderModal("1-New-Items"); + expect(screen.queryByRole("alert")).toBeNull(); + }); + + it("shows a warning when the value contains a period", () => { + renderModal("1. New Items"); + const alert = screen.getByRole("alert"); + expect(alert).toBeInTheDocument(); + expect(alert.textContent).toContain('"."'); + expect(alert.textContent?.toLowerCase()).toContain("not allowed"); + }); + + it("disables the Save button when the value is invalid", () => { + renderModal("1. New Items"); + const saveButton = screen.getByRole("button", { name: /save/i }); + expect(saveButton).toBeDisabled(); + }); + + it("re-enables Save once the period is removed", () => { + const { rerender } = renderModal("1. New Items"); + expect(screen.getByRole("button", { name: /save/i })).toBeDisabled(); + + rerender( + + ); + + expect(screen.queryByRole("alert")).toBeNull(); + expect(screen.getByRole("button", { name: /save/i })).not.toBeDisabled(); + }); + + it("ignores Enter when the value is invalid", () => { + renderModal("1. New Items"); + const input = screen.getByPlaceholderText("Enter new book name"); + fireEvent.keyDown(input, { key: "Enter" }); + expect(onConfirm).not.toHaveBeenCalled(); + }); + + it("invokes onConfirm on Enter when the value is valid", () => { + renderModal("1-New-Items"); + const input = screen.getByPlaceholderText("Enter new book name"); + fireEvent.keyDown(input, { key: "Enter" }); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); +});