diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 26f1bf85..8f663955 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1256,6 +1256,122 @@ describe("CaptureChoiceEngine capture target resolution", () => { }); }); + it("refuses an impossible target before prompting or creating (#1591)", async () => { + // The refusal used to arrive from vault.create, after the user had + // answered the whole prompt chain. + const app = createApp(); + const engine = new CaptureChoiceEngine( + app, + { + settings: { + useSelectionAsCaptureValue: false, + showCaptureNotification: true, + }, + } as any, + createChoice({ + captureTo: "Bad: Title.md", + createFileIfItDoesntExist: { + enabled: true, + createWithTemplate: false, + template: "", + }, + format: { enabled: true, format: "{{VALUE}}" }, + }), + createExecutor(), + ); + + const createFileWithInput = vi.fn(); + (engine as any).createFileWithInput = createFileWithInput; + + await engine.run(); + + expect(createFileWithInput).not.toHaveBeenCalled(); + // And the capture's own {{VALUE}} prompt never opened: the whole point of + // #1591 is that the user does not answer a prompt chain for a note that + // cannot be created. + expect(promptHydratedValues).toHaveLength(0); + }); + + it("refuses before the heading picker, not after it (#1591)", async () => { + // Placement, not just presence: the "Choose heading when capturing" picker + // runs between the existence check and the create dispatch, so a guard + // sited at the sink would still have asked the user to pick a heading for + // a note that cannot be created. This case fails if the guard moves down. + const suggestSpy = vi.fn(async () => "## Today"); + (InputSuggester as any).Suggest = suggestSpy; + + const engine = new CaptureChoiceEngine( + createApp(), + { + settings: { + useSelectionAsCaptureValue: false, + showCaptureNotification: true, + }, + } as any, + createChoice({ + captureTo: "Bad: Title.md", + createFileIfItDoesntExist: { + enabled: true, + createWithTemplate: false, + template: "", + }, + insertAfter: { + enabled: true, + after: "", + insertAtEnd: false, + considerSubsections: false, + createIfNotFound: false, + createIfNotFoundLocation: "", + promptHeading: true, + }, + }), + createExecutor(), + ); + const createFileWithInput = vi.fn(); + (engine as any).createFileWithInput = createFileWithInput; + + await engine.run(); + + expect(suggestSpy).not.toHaveBeenCalled(); + expect(createFileWithInput).not.toHaveBeenCalled(); + }); + + it("still appends to an EXISTING file whose name carries a colon (#1591)", async () => { + // ":" is legal on macOS/Linux at the filesystem level, so a note made + // outside Obsidian can have one - and capturing to it works today, + // because nothing ever asks Obsidian to accept the name. + const app = createApp(); + const engine = new CaptureChoiceEngine( + app, + { + settings: { + useSelectionAsCaptureValue: false, + showCaptureNotification: true, + }, + } as any, + createChoice({ + captureTo: "Bad: Title.md", + createFileIfItDoesntExist: { + enabled: true, + createWithTemplate: false, + template: "", + }, + }), + createExecutor(), + ); + (engine as any).fileExists = vi.fn(async () => true); + const onFileExists = vi.fn(async () => ({ + file: { path: "Bad: Title.md", basename: "Bad: Title" }, + newFileContent: "", + captureContent: "x", + })); + (engine as any).onFileExists = onFileExists; + + await engine.run(); + + expect(onFileExists).toHaveBeenCalled(); + }); + it("keeps submitted VALUE prompt draft after failed target creation and clears it after success", async () => { promptResponses.push("Capture body to preserve"); const store = InputPromptDraftStore.getInstance(); @@ -1274,7 +1390,11 @@ describe("CaptureChoiceEngine capture target resolution", () => { }, } as any, createChoice({ - captureTo: "Bad:Title.md", + // A LEGAL target: an impossible one is now refused before the + // prompt is ever opened (#1591), which is a different behaviour + // with its own test below. What this case is about is a create + // that fails for some other reason after the answer is in. + captureTo: "Notes/Target.md", createFileIfItDoesntExist: { enabled: true, createWithTemplate: false, @@ -1289,7 +1409,7 @@ describe("CaptureChoiceEngine capture target resolution", () => { ); (engine as any).createFileWithInput = vi.fn(async () => { - throw new Error("File name cannot contain ':'"); + throw new Error("Disk is on fire"); }); const clipboardWriteText = vi.fn(async () => {}); diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index 94808ec6..037c95be 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -83,6 +83,7 @@ import { shouldPostProcessFrontMatter, } from "./helpers/frontmatterPostProcessor"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; +import { assertCreatableFilePath } from "./assertCreatableFilePath"; import { UserCancelError } from "../errors/UserCancelError"; import { SingleTemplateEngine } from "./SingleTemplateEngine"; import { getCaptureAction, type CaptureAction } from "./captureAction"; @@ -434,6 +435,18 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { let getFileAndAddContentFn: GetFileAndAddContentFn; const fileAlreadyExists = await this.fileExists(filePath); + // Refuse an impossible target here rather than at vault.create (#1591). + // Gated character-for-character on the dispatch condition below, so a + // missing target with creation OFF still gets its own message - and an + // EXISTING file with a colon in its name (legal on macOS/Linux, so a note + // made outside Obsidian can have one) keeps appending as it does today. + // Placed above the heading picker rather than inside + // onCreateFileIfItDoesntExist so the user is not asked to choose a + // heading for a note that cannot be created. + if (!fileAlreadyExists && this.choice?.createFileIfItDoesntExist?.enabled) { + assertCreatableFilePath(filePath); + } + // "Choose heading when capturing" (After line…): prompt for a heading from the resolved // target note and feed the picked line to the formatter as an insert-after // override. Runs after the target file is known and before any formatting/write. @@ -1436,6 +1449,11 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { captureContent: string, linkOptions?: AppendLinkOptions, ): Promise { + // Re-asserted at the sink, so the invariant is local to the one function + // that creates the file, not only to its caller's branch. It is an + // `includes` scan over a short string and costs nothing (#1591). + assertCreatableFilePath(filePath); + // Extract filename without extension from the full path. const fileBasename = basenameWithoutMdOrCanvas(filePath); this.formatter.setTitle(fileBasename); diff --git a/src/engine/TemplateEngine-1591-refuse-name.test.ts b/src/engine/TemplateEngine-1591-refuse-name.test.ts new file mode 100644 index 00000000..a9fc497f --- /dev/null +++ b/src/engine/TemplateEngine-1591-refuse-name.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it, vi } from "vitest"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; + +/** + * Issue #1591. `createFileWithTemplate` reads the template, formats the ENTIRE + * body - real `{{VALUE}}` prompts, macros, and inline `js quickadd` fences with + * their side effects - and only then hands the path to `createFileWithInput`, + * which creates the target FOLDER before `vault.create` refuses the name. + * + * Measured on Obsidian 1.13.0 (isolated e2e vault) before the fix: a Template + * choice named `Bad: {{VALUE:title}}` reported "no file was created" AND left + * `Repro1591Folder` behind AND had run the template's inline script once. + * + * The guard is the first statement of `createFileWithTemplate`, so this asserts + * on the observable consequence: nothing downstream of it is reached. + */ + +vi.mock("obsidian", async () => { + const actual = await import("../../tests/obsidian-stub"); + return actual; +}); +vi.mock("../formatters/completeFormatter", () => ({ + CompleteFormatter: class { + setTitle() {} + setTargetFolderPath() {} + setPromptRunContext() {} + setTemplateInclusionState() {} + }, +})); +vi.mock("../utilityObsidian", () => ({ + getTemplateFile: vi.fn(), + getTemplater: vi.fn(() => null), + overwriteTemplaterOnce: vi.fn(), + templaterParseTemplate: vi.fn(), +})); +vi.mock("../logger/logManager", () => ({ + log: { logWarning: vi.fn(), logError: vi.fn(), logMessage: vi.fn() }, +})); + +const { TemplateEngine } = await import("./TemplateEngine"); + +class ProbeEngine extends (TemplateEngine as never as new ( + ...args: never[] +) => Record) { + public readTemplate = vi.fn(); + public created = vi.fn(); + + constructor() { + super( + { vault: {}, workspace: {} } as never, + { settings: {} } as never, + ); + } + + run() { + return Promise.resolve(); + } + + // Everything the guard must run BEFORE. + protected getTemplateContent(path: string) { + this.readTemplate(path); + return Promise.resolve("body"); + } + protected createFileWithInput(path: string) { + this.created(path); + return Promise.resolve({ path }); + } +} + +function makeEngine() { + return new ProbeEngine() as unknown as ProbeEngine & { + createFileWithTemplate: ( + filePath: string, + templatePath: string, + ) => Promise; + }; +} + +describe("createFileWithTemplate refuses an impossible name first (#1591)", () => { + it("aborts without reading the template or creating anything", async () => { + const engine = makeEngine(); + + await expect( + engine.createFileWithTemplate("Notes/Bad: My Note.md", "T.md"), + ).rejects.toBeInstanceOf(ChoiceAbortError); + + expect(engine.readTemplate).not.toHaveBeenCalled(); + expect(engine.created).not.toHaveBeenCalled(); + }); + + it("aborts for a colon in a FOLDER segment, before the folder is created", async () => { + const engine = makeEngine(); + + await expect( + engine.createFileWithTemplate("Meetings: 2026/Note.md", "T.md"), + ).rejects.toBeInstanceOf(ChoiceAbortError); + + expect(engine.readTemplate).not.toHaveBeenCalled(); + }); + + it("leaves a legal path alone", async () => { + // Getting as far as reading the template is what proves the guard let it + // through; the rest of the create path needs the real formatter and is + // covered by TemplateChoiceEngine's own suites. + const engine = makeEngine(); + + await engine.createFileWithTemplate("Notes/My Note.md", "T.md"); + + expect(engine.readTemplate).toHaveBeenCalledWith("T.md"); + }); +}); diff --git a/src/engine/TemplateEngine.ts b/src/engine/TemplateEngine.ts index 3ead948f..0c0c1ab3 100644 --- a/src/engine/TemplateEngine.ts +++ b/src/engine/TemplateEngine.ts @@ -41,6 +41,7 @@ import { ChoiceAbortError } from "../errors/ChoiceAbortError"; import { isCancellationError } from "../utils/errorUtils"; import type { IChoiceExecutor } from "../IChoiceExecutor"; import { log } from "../logger/logManager"; +import { assertCreatableFilePath } from "./assertCreatableFilePath"; type FolderChoiceOptions = { allowCreate?: boolean; @@ -598,7 +599,23 @@ export abstract class TemplateEngine extends QuickAddEngine { filePath: string, resolvedTemplatePath: string ) { + // Clear the previous run's swallowed cause FIRST, so the reset is + // unconditional even when the guard below aborts (#1617). this.lastTemplateFileFailure = null; + + // Then, before the template is read and long before its body is formatted + // (#1591). Both production callers - TemplateChoiceEngine's `else` branch of + // `vault.adapter.exists`, and its freshly incremented collision name - are + // paths where nothing is at `filePath` yet, so a name Obsidian refuses can + // only end in a failed `vault.create`. Letting it get that far means the + // user answers the whole prompt chain and their inline scripts run first, + // and QuickAddEngine.createFileWithInput leaves the target folder behind. + // + // It THROWS rather than recording into lastTemplateFileFailure: that field + // is for helpers that report-and-return-null, while this aborts the choice + // with a typed ChoiceAbortError carrying its own actionable message (#1606). + assertCreatableFilePath(filePath); + try { const templateContent: string = await this.getTemplateContent( resolvedTemplatePath diff --git a/src/engine/applyTemplateToActiveNote.test.ts b/src/engine/applyTemplateToActiveNote.test.ts index 969cb92a..cc558e1c 100644 --- a/src/engine/applyTemplateToActiveNote.test.ts +++ b/src/engine/applyTemplateToActiveNote.test.ts @@ -5,6 +5,9 @@ const { engineConstructorMock, resolvedPathMock, setPromptRunContextMock, + targetPathMock, + yesNoPromptMock, + suggestMock, } = vi.hoisted(() => ({ engineApplyMock: vi.fn(), @@ -13,8 +16,20 @@ const { // Identity by default (raw == resolved); override to simulate a path token // that resolves to a different extension (issue #620). resolvedPathMock: vi.fn((raw: string) => raw), + // null by default: no move offer. Override to drive the reconcile path. + targetPathMock: vi.fn<() => Promise>(async () => null), + yesNoPromptMock: vi.fn(async () => true), + suggestMock: vi.fn<(...args: unknown[]) => Promise>(), })); +vi.mock("../gui/GenericSuggester/genericSuggester", () => ({ + default: { Suggest: (...args: unknown[]) => suggestMock(...args) }, +})); + +vi.mock("../gui/GenericYesNoPrompt/GenericYesNoPrompt", () => ({ + default: { Prompt: (...args: unknown[]) => yesNoPromptMock(...(args as [])) }, +})); + vi.mock("./TemplateInsertEngine", async (importOriginal) => { const actual = await importOriginal(); @@ -31,7 +46,7 @@ vi.mock("./TemplateInsertEngine", async (importOriginal) => { return await engineApplyMock(); } async computeChoiceTargetPath() { - return null; + return await targetPathMock(); } setPromptRunContext(context: unknown) { setPromptRunContextMock(context); @@ -466,3 +481,95 @@ describe("applyTemplateToNote (non-interactive)", () => { expect(result).toBeNull(); }); }); + +/** + * Issue #1591. Answering "Yes" to the move offer creates the target FOLDER and + * then fails at `renameFile`, leaving an empty folder behind and reporting it + * as a warning the user did nothing to deserve. The offer is an optional + * convenience, so an impossible target declines it quietly. + */ +describe("the move offer refuses an impossible target (#1591)", () => { + function makeFile(): TFile { + const file = new TFile(); + file.path = "notes/My note.md"; + file.basename = "My note"; + file.extension = "md"; + return file; + } + + function makeApp(activeFile: TFile) { + const createFolder = vi.fn(async () => {}); + const renameFile = vi.fn(async () => {}); + const app = { + workspace: { getActiveFile: () => activeFile }, + vault: { + // Empty, so the empty-note fast path picks "replace" and the insert + // MODE picker is skipped; only the template picker is driven below. + cachedRead: async () => "", + adapter: { exists: async () => false }, + createFolder, + }, + fileManager: { renameFile }, + } as unknown as App; + return { app, createFolder, renameFile }; + } + + const choice = makeTemplateChoice("Choice", "templates/tpl.md"); + const plugin = { + settings: { choices: [choice] }, + getTemplateFiles: () => [], + } as unknown as QuickAdd; + const executor = (): IChoiceExecutor => ({ + execute: async () => {}, + variables: new Map(), + }); + + beforeEach(() => { + vi.clearAllMocks(); + engineApplyMock.mockImplementation(async () => makeFile()); + yesNoPromptMock.mockImplementation(async () => true); + }); + + it("does not ask, create a folder, or rename for a name Obsidian refuses", async () => { + // The INTERACTIVE path: reconciliation only runs when the template came + // from a choice picked by the user, so a `templatePath` argument would + // skip the branch under test entirely. + suggestMock.mockImplementation(async (_app, _labels, values) => { + const items = values as Array; + // First call picks the template, second picks the insert mode. + return typeof items[0] === "string" ? items[0] : items[0]; + }); + targetPathMock.mockImplementation(async () => "Bad: Notes/My note.md"); + const file = makeFile(); + const { app, createFolder, renameFile } = makeApp(file); + + await applyTemplateToNote(app, plugin, { + file, + choiceExecutor: executor(), + }); + + expect(targetPathMock).toHaveBeenCalled(); + expect(yesNoPromptMock).not.toHaveBeenCalled(); + expect(createFolder).not.toHaveBeenCalled(); + expect(renameFile).not.toHaveBeenCalled(); + }); + + it("still offers the move for a legal target", async () => { + // The other half, so the case above cannot pass by never reaching the + // reconcile branch at all. + suggestMock.mockImplementation(async (_app, _labels, values) => + (values as Array)[0], + ); + targetPathMock.mockImplementation(async () => "Notes/My note.md"); + const file = makeFile(); + const { app, renameFile } = makeApp(file); + + await applyTemplateToNote(app, plugin, { + file, + choiceExecutor: executor(), + }); + + expect(yesNoPromptMock).toHaveBeenCalled(); + expect(renameFile).toHaveBeenCalled(); + }); +}); diff --git a/src/engine/applyTemplateToActiveNote.ts b/src/engine/applyTemplateToActiveNote.ts index 9d0ffdf3..e3663f1c 100644 --- a/src/engine/applyTemplateToActiveNote.ts +++ b/src/engine/applyTemplateToActiveNote.ts @@ -16,6 +16,7 @@ import { jumpToNextTemplaterCursorIfPossible, } from "../utilityObsidian"; import { flattenChoices } from "../utils/choiceUtils"; +import { isCreatableFilePath } from "./assertCreatableFilePath"; import { isCancellationError, reportError } from "../utils/errorUtils"; import { TemplateInsertEngine, @@ -326,6 +327,12 @@ async function maybeReconcileNoteLocation( const targetPath = await engine.computeChoiceTargetPath(choice); if (!targetPath || targetPath === file.path) return; if (await app.vault.adapter.exists(targetPath)) return; + // Do not offer a move that cannot succeed (#1591). Answering "Yes" here + // creates the target FOLDER and then fails at renameFile, leaving an empty + // folder behind and reporting it as a warning the user did nothing to + // deserve. This offer is an optional convenience, so it declines quietly; + // the choice's own file-name preview is where the problem is explained. + if (!isCreatableFilePath(targetPath)) return; const shouldMove = await GenericYesNoPrompt.Prompt( app, diff --git a/src/engine/assertCreatableFilePath.test.ts b/src/engine/assertCreatableFilePath.test.ts new file mode 100644 index 00000000..29a3872a --- /dev/null +++ b/src/engine/assertCreatableFilePath.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "vitest"; +import { + assertCreatableFilePath, + isCreatableFilePath, +} from "./assertCreatableFilePath"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; + +/** + * Issue #1591. The rule itself (which characters, and why only those) is + * measured and pinned in `fileNameDisplayFormatter-1578-illegal-chars.test.ts`; + * this file pins the RUN-side wrapper: what it throws, and what it says. + */ +describe("assertCreatableFilePath", () => { + it("aborts the choice for a name Obsidian refuses", () => { + expect(() => assertCreatableFilePath("Notes/Bad: My Note.md")).toThrow( + ChoiceAbortError, + ); + }); + + it("names the path and the character, in the run's tense", () => { + // The preview's sentence ends "would fail at run time"; by here the choice + // IS stopping and the name is known, so the wording differs - but the + // character list and the {{TIME}} hint are shared, so the rule cannot + // drift between the surface that warns and the code that stops. + expect(() => assertCreatableFilePath("Bad: My Note.md")).toThrow( + 'Cannot create "Bad: My Note.md": a file or folder name cannot contain ":". Check your own text and tokens like {{TIME}}, which is HH:mm.', + ); + }); + + it("fires on a colon in a FOLDER segment, which Obsidian refuses too", () => { + // QuickAddEngine.createFileWithInput creates the parent folder before + // vault.create, so this is the first thing that would have failed. + expect(() => assertCreatableFilePath("Meetings: 2026/Note.md")).toThrow( + ChoiceAbortError, + ); + }); + + it("allows the separators QuickAdd creates folders for", () => { + expect(() => + assertCreatableFilePath("Notes/Sub/My Note.md"), + ).not.toThrow(); + }); + + it("allows characters Obsidian creates without complaint", () => { + // Measured against vault.create on Obsidian 1.13.0 (macOS): these all + // succeed, so rejecting them would break working setups (#1595). + expect(() => + assertCreatableFilePath(`Notes/a*b?c"df|g^h[i]j#k.md`), + ).not.toThrow(); + }); + + it("exposes the same rule as a predicate for callers that decline quietly", () => { + expect(isCreatableFilePath("Notes/My Note.md")).toBe(true); + expect(isCreatableFilePath("Notes/Bad: My Note.md")).toBe(false); + }); +}); diff --git a/src/engine/assertCreatableFilePath.ts b/src/engine/assertCreatableFilePath.ts new file mode 100644 index 00000000..179eafe1 --- /dev/null +++ b/src/engine/assertCreatableFilePath.ts @@ -0,0 +1,47 @@ +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; +import { + describeIllegalFilePathCharsForRun, + findIllegalFilePathChars, +} from "../utils/generatedFilePath"; + +/** + * Stops a choice at the moment its target path is known, when Obsidian will + * refuse to create it anyway (#1591). + * + * Without this the refusal arrives from `vault.create`, by which point QuickAdd + * has already created the target FOLDER and formatted the entire template body + * or capture - real `{{VALUE}}` prompts, macros, and inline `js quickadd` + * fences with whatever side effects those scripts have. Measured on Obsidian + * 1.13.0: a Template choice named `Bad: {{VALUE:title}}` left an empty folder + * behind and ran the template's inline script once before dying. + * + * CALL ONLY WHERE THE FILE IS ABOUT TO BE CREATED. `:` is legal on macOS and + * Linux at the filesystem level, so a note made outside Obsidian really can + * carry one - and appending to it works today, because nothing ever asks + * Obsidian to accept the name. Every call site below sits on a branch where the + * target's non-existence has already been established, which is why this takes + * no vault and does no probing of its own. + * + * Deliberately NOT inside `normalizeGeneratedFilePath`: `CaptureChoiceEngine` + * calls that as an existence PROBE inside `try/catch`, where a new throw would + * silently answer "does not exist". + * + * The rule and its wording come from `generatedFilePath.ts`, which the preview + * also reads, so the row that warns and the code that stops cannot drift. + */ +export function assertCreatableFilePath(path: string): void { + const illegal = findIllegalFilePathChars(path); + if (illegal.length === 0) return; + throw new ChoiceAbortError( + describeIllegalFilePathCharsForRun(illegal, path), + ); +} + +/** + * {@link assertCreatableFilePath} as a predicate, for a caller that should + * quietly decline rather than abort - the "move this note?" offer, which is an + * optional convenience and must not ask a question that cannot succeed. + */ +export function isCreatableFilePath(path: string): boolean { + return findIllegalFilePathChars(path).length === 0; +} diff --git a/src/formatters/completeFormatter.test.ts b/src/formatters/completeFormatter.test.ts index 04c4b99a..108ecc0b 100644 --- a/src/formatters/completeFormatter.test.ts +++ b/src/formatters/completeFormatter.test.ts @@ -1015,6 +1015,41 @@ describe("CompleteFormatter - math value prompting", () => { MacroAbortError, ); }); + + it("uses an already-collected mvalue instead of prompting (#1607)", async () => { + // RequirementCollector registers this token under the key "mvalue", so the + // one-page input form asks for a "Math expression" and the CLI advertises + // `value-mvalue=`. Both answers used to be discarded and the modal + // opened anyway. + const f = defaultFormatter(); + (f as unknown as { variables: Map }).variables.set( + "mvalue", + "2+2*3", + ); + await expect(f.formatFolderPath("= {{MVALUE}}")).resolves.toBe("= 2+2*3"); + expect(mocks.mathPrompt).not.toHaveBeenCalled(); + }); + + it("still prompts once per occurrence when nothing was collected", async () => { + // READ-only consumption: the prompt's own answer is deliberately not + // stored, so two tokens stay two questions, as they always have been. + mocks.mathPrompt.mockResolvedValueOnce("1").mockResolvedValueOnce("2"); + const f = defaultFormatter(); + await expect(f.formatFolderPath("{{MVALUE}}-{{MVALUE}}")).resolves.toBe( + "1-2", + ); + expect(mocks.mathPrompt).toHaveBeenCalledTimes(2); + }); + + it("ignores a blank collected mvalue and prompts", async () => { + mocks.mathPrompt.mockResolvedValue("7"); + const f = defaultFormatter(); + (f as unknown as { variables: Map }).variables.set( + "mvalue", + " ", + ); + await expect(f.formatFolderPath("{{MVALUE}}")).resolves.toBe("7"); + }); }); describe("CompleteFormatter - field suggestion (suggestForField)", () => { diff --git a/src/formatters/completeFormatter.ts b/src/formatters/completeFormatter.ts index 19c68816..59be771b 100644 --- a/src/formatters/completeFormatter.ts +++ b/src/formatters/completeFormatter.ts @@ -5,7 +5,14 @@ import InputSuggester from "src/gui/InputSuggester/inputSuggester"; import MultiSuggester from "src/gui/MultiSuggester/multiSuggester"; import VDateInputPrompt from "src/gui/VDateInputPrompt/VDateInputPrompt"; import type { IChoiceExecutor } from "../IChoiceExecutor"; -import { GLOBAL_VAR_REGEX, INLINE_JAVASCRIPT_REGEX } from "../constants"; +import { + GLOBAL_VAR_REGEX, + INLINE_JAVASCRIPT_REGEX, + // Replaces six inlined copies of `/\{\{title\}\}/i` in this file. The + // file-name PREVIEW now mirrors the same rule (#1588), so the pattern has to + // be one shared constant rather than a seventh copy that can drift. + TITLE_REGEX, +} from "../constants"; import GenericSuggester from "../gui/GenericSuggester/genericSuggester"; import InputPrompt from "../gui/InputPrompt"; import { MathModal } from "../gui/MathModal"; @@ -122,7 +129,7 @@ export class CompleteFormatter extends Formatter { scope: PromptScopeKind = "generic", ): Promise { // Check for {{title}} usage in filename which would cause infinite recursion - if (/\{\{title\}\}/i.test(input)) { + if (TITLE_REGEX.test(input)) { throw new Error( "{{title}} cannot be used in file names as it would create a circular dependency. The title is derived from the filename itself.", ); @@ -137,7 +144,7 @@ export class CompleteFormatter extends Formatter { // (the token pass below omits `title`, leaving it verbatim). Re-check post // format() so it throws the same circular-dependency error, mirroring // formatTemplateFilePath's post-global-expansion guard. - if (/\{\{title\}\}/i.test(output)) { + if (TITLE_REGEX.test(output)) { throw new Error( "{{title}} cannot be used in file names as it would create a circular dependency. The title is derived from the filename itself.", ); @@ -195,7 +202,7 @@ export class CompleteFormatter extends Formatter { async formatFolderPath(folderName: string): Promise { // Check for {{title}} usage in folder path which would cause issues - if (/\{\{title\}\}/i.test(folderName)) { + if (TITLE_REGEX.test(folderName)) { throw new Error( "{{title}} cannot be used in folder paths as it would create a circular dependency. The title is derived from the filename itself.", ); @@ -208,7 +215,7 @@ export class CompleteFormatter extends Formatter { // {{VALUE}} resolving to "{{title}}" slips past the raw-input check above, // then the folder-only token pass would leave it literal in the path. // Re-check post format() so it throws the circular-dependency error. - if (/\{\{title\}\}/i.test(formatted)) { + if (TITLE_REGEX.test(formatted)) { throw new Error( "{{title}} cannot be used in folder paths as it would create a circular dependency. The title is derived from the filename itself.", ); @@ -255,7 +262,7 @@ export class CompleteFormatter extends Formatter { * {{date}} / {{random}} would re-evaluate to a different value. */ async formatTemplateFilePath(input: string): Promise { - if (/\{\{title\}\}/i.test(input)) { + if (TITLE_REGEX.test(input)) { throw new Error( "{{title}} cannot be used in a template path — the title is derived from the created file, not the source template.", ); @@ -270,7 +277,7 @@ export class CompleteFormatter extends Formatter { // user-input substitution — so a global-injected {{title}} throws the // clear circular-title error, without false-positiving on a user value // that merely contains the literal text "{{title}}". - if (/\{\{title\}\}/i.test(output)) { + if (TITLE_REGEX.test(output)) { throw new Error( "{{title}} cannot be used in a template path — the title is derived from the created file, not the source template.", ); @@ -791,7 +798,10 @@ export class CompleteFormatter extends Formatter { if (provider) { return await provider.inputPrompt("Enter a math expression"); } - this.assertInteractivePrompt("a {{MATH}} expression"); + // The token is {{MVALUE}} (MATH_VALUE_REGEX). "a {{MATH}} expression" + // named a token QuickAdd has never had, in the one message whose whole + // job is to tell a non-interactive caller which flag to pass (#1587). + this.assertInteractivePrompt("a {{MVALUE}} math expression"); try { return await MathModal.Prompt(); } catch (error) { diff --git a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts index d3fc7c56..5d8cfce5 100644 --- a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts @@ -9,7 +9,7 @@ import { vi, } from "vitest"; import type { App } from "obsidian"; -import { TFile } from "obsidian"; +import { TFile, TFolder } from "obsidian"; import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; import type QuickAdd from "../main"; @@ -54,11 +54,13 @@ beforeEach(() => { templates = {}; globalVariables = {}; existingFiles = new Set(); + existingFolders = new Set(); }); let templates: Record = {}; let globalVariables: Record = {}; let existingFiles = new Set(); +let existingFolders = new Set(); function makeApp(): App { return { @@ -71,14 +73,20 @@ function makeApp(): App { }, vault: { getMarkdownFiles: () => [], - getAbstractFileByPath: (path: string) => - path in templates || existingFiles.has(path) + getAbstractFileByPath: (path: string) => { + // Obsidian's path map holds files AND folders, with no trailing + // slash on either, which is what the shape-aware probe turns on. + if (existingFolders.has(path)) { + return Object.assign(new TFolder(), { path, children: [] }); + } + return path in templates || existingFiles.has(path) ? Object.assign(new TFile(), { path, extension: "md", basename: path.replace(/\.md$/, ""), }) - : null, + : null; + }, cachedRead: async (file: { path: string }) => templates[file.path], }, metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, @@ -231,6 +239,44 @@ describe("the file-name preview does not cry wolf", () => { expect(diagnostics).toEqual([]); }); + it("stays quiet for a folder-shaped capture target whose folder exists", async () => { + // A capture target written with a trailing slash names a FOLDER to pick + // inside, and the folder being there is exactly what makes it work. + // Obsidian's path map holds no trailing slash, so a bare lookup missed it + // and the row went red for a capture that runs fine. + existingFolders.add("Meetings: 2026"); + const { diagnostics } = await preview("Meetings: 2026/"); + expect(diagnostics).toEqual([]); + }); + + it("stays quiet for a BARE name that resolves to an existing folder scope", async () => { + // captureTargetResolution: an existing folder with no real note beside it + // is a folder SCOPE, and the capture picks a file inside it - so nothing + // asks Obsidian to accept this string as a name. Reddening it would mark a + // working capture broken, and would contradict the trailing-slash case + // above for the same runtime target. + existingFolders.add("Meetings: 2026"); + const { diagnostics } = await preview("Meetings: 2026"); + expect(diagnostics).toEqual([]); + }); + + it("still reports a colon when a FOLDER is merely named like the note", async () => { + // A folder called `Meetings: 2026.md` is not a note, so it cannot be the + // target of a capture and cannot excuse the name either. + existingFolders.add("Meetings: 2026.md"); + const { diagnostics } = await preview("Meetings: 2026"); + expect(diagnostics).toEqual([ + { severity: "error", kind: "path", message: REFUSED }, + ]); + }); + + it("still reports a colon when the trailing-slash folder does not exist", async () => { + const { diagnostics } = await preview("Meetings: 2026/"); + expect(diagnostics).toEqual([ + { severity: "error", kind: "path", message: REFUSED }, + ]); + }); + it("still reports a colon when no such file exists", async () => { const { diagnostics } = await preview("Notes/a:b.md"); expect(diagnostics).toEqual([{ severity: "error", kind: "path", message: REFUSED }]); diff --git a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts index af36a9cd..f710d11a 100644 --- a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts +++ b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts @@ -75,26 +75,29 @@ describe("FileNameDisplayFormatter VDATE preview", () => { expect(out).toBe("2023-06-01"); }); - it("does NOT apply |startof: snap to the preview (matches body preview)", async () => { + it("applies |startof: snap to the example, as the run does", async () => { const out = await makeFormatter().format( "{{VDATE:wk,gggg.MM.[Wk]w|startof:week}}", ); - // Snap is only resolved in the real CompleteFormatter pass; the preview - // renders the current date (2023-06-01) like the body preview does, so - // the two previews stay consistent. (DateFormatPreviewGenerator leaves - // gggg / [Wk] literal — that's its existing simplified-preview behavior.) - expect(out).toBe("gggg.06.[Wk]22"); + // The clock is Thursday 2023-06-01; start of week (en locale, Sunday + // first) is 2023-05-28. #1595 left snap out here on the grounds that + // snapping only this row would split it from the body row; both rows snap + // now, and {{DATE:...|startof:}} in this same pass always did. + // (DateFormatPreviewGenerator leaves gggg / [Wk] literal — that's its + // existing simplified-preview behavior.) + expect(out).toBe("gggg.05.[Wk]22"); }); - it("ignores both the snap and the default hint", async () => { + it("applies |endof: snap and still appends no hint", async () => { const formatter = makeFormatter(); const out = await formatter.format( "{{VDATE:eom,YYYY-MM-DD|endof:month|tomorrow}}", ); - // No snap applied to the preview, no hint appended: the current date. - // Note the token's own `|endof:month` carries a colon, and it is still - // not reported - the check reads the finished NAME, not the format. - expect(out).toBe("2023-06-01"); + // End of June. No hint appended: this row is a FILE NAME, and the run + // splices in the date and nothing else. Note the token's own + // `|endof:month` carries a colon and is still not reported - the check + // reads the finished NAME, not the format. + expect(out).toBe("2023-06-30"); expect(formatter.diagnostics.list()).toEqual([]); }); @@ -103,8 +106,89 @@ describe("FileNameDisplayFormatter VDATE preview", () => { expect(out).toBe("2023-06-01"); }); - it("returns the token literally when the format is missing", async () => { - const out = await makeFormatter().format("{{VDATE:noformat}}"); - expect(out).toBe("{{VDATE:noformat}}"); + it("supplies the run's default format when the token names none (#1589)", async () => { + // The run's own default (Formatter.replaceDateVariableInString), so a + // working `{{VDATE:due}}` no longer previews as the raw token - and no + // longer collects the colon inside that token as an illegal character. + const formatter = makeFormatter(); + const out = await formatter.format("{{VDATE:noformat}}"); + expect(out).toBe("2023-06-01"); + expect(formatter.diagnostics.list()).toEqual([]); + }); + + it("uses the datetime default under |time, colon and all (#1589)", async () => { + // YYYY-MM-DD HH:mm is what the run splices in, so the file-name preview + // must show it - and then the #1578 diagnostic fires on its own, because + // the finished NAME really does contain a colon Obsidian refuses. + const formatter = makeFormatter(); + const out = await formatter.format("{{VDATE:due|time}}"); + expect(out).toMatch(/^2023-06-01 \d{2}:\d{2}$/); + expect(formatter.diagnostics.list()).toEqual([ + { + severity: "error", + kind: "path", + message: + 'A file or folder name cannot contain ":", so this choice would fail at run time. Check your own text and tokens like {{TIME}}, which is HH:mm.', + }, + ]); + }); + + it("still leaves a NAMELESS token literal, as the run does", async () => { + const formatter = makeFormatter(); + const out = await formatter.format("{{VDATE:}}"); + expect(out).toBe("{{VDATE:}}"); + }); + + it("renders an ANSWERED date instead of today (#1589/#1590)", async () => { + // The one-page input form seeds the user's real picks into this formatter + // before computing the preview; VDATE was the only seeded requirement type + // the preview ignored. + const formatter = makeFormatter(); + (formatter as unknown as { variables: Map }).variables.set( + "due", + // Built from a LOCAL date, not a "Z" literal: the stored value is an + // instant and moment formats it in local time, so a midnight-UTC + // literal renders as the 14th anywhere west of UTC (this test file's + // sibling carries the same warning). + `@date:${new Date(2026, 7, 15, 12, 0, 0).toISOString()}`, + ); + await expect(formatter.format("{{VDATE:due}}")).resolves.toBe("2026-08-15"); + }); + + it("renders an answered-empty optional date as empty, not as today", async () => { + const formatter = makeFormatter(); + (formatter as unknown as { variables: Map }).variables.set( + "due", + "", + ); + await expect( + formatter.format("note {{VDATE:due,YYYY-MM-DD|optional}}"), + ).resolves.toBe("note"); + }); + + it("does not blank the row for a half-typed |startof: unit", async () => { + // "we" does not resolve (unlike "w", which is a real alias for week), so + // normalizeDateUnit throws - and this runs on every keystroke (#1558). The + // TEXT stays a date; the complaint goes on the diagnostics channel, which + // the builder holds back until the field has been still for 500ms. + const formatter = makeFormatter(); + await expect( + formatter.format("{{VDATE:wk,YYYY-MM-DD|startof:we}}"), + ).resolves.toBe("2023-06-01"); + expect(formatter.diagnostics.list()).toEqual([ + { + severity: "error", + message: expect.stringContaining('Unknown date unit "we"') as never, + }, + ]); + }); + + it("says nothing for a SHORT unit that really is an alias", async () => { + // "w" is week. Reporting it would be the cry-wolf this cluster deletes. + const formatter = makeFormatter(); + await expect( + formatter.format("{{VDATE:wk,YYYY-MM-DD|startof:w}}"), + ).resolves.toBe("2023-05-28"); + expect(formatter.diagnostics.list()).toEqual([]); }); }); diff --git a/src/formatters/fileNameDisplayFormatter.test.ts b/src/formatters/fileNameDisplayFormatter.test.ts index 5f46f568..2fbd2265 100644 --- a/src/formatters/fileNameDisplayFormatter.test.ts +++ b/src/formatters/fileNameDisplayFormatter.test.ts @@ -26,6 +26,9 @@ import type QuickAdd from "../main"; const templates: Record = { "Templates/Daily.md": "Daily body\n", + // The run resolves {{title}} inside an included body (child engine -> + // formatFileContent), so this previews as "note", not as an error (#1588). + "Templates/Titled.md": "{{title}} note\n", }; const activeFile = { @@ -181,22 +184,34 @@ describe("FileNameDisplayFormatter resolves the tokens a file name can hold", () }); }); -describe("tokens the file-name preview does NOT resolve today", () => { - /** - * Pinned as CURRENT behaviour with the issue each is filed as, not as - * desired behaviour. - */ - it("leaves {{MVALUE}} literal even though the run prompts for it (#1587)", async () => { +describe("tokens the file-name preview resolves the way the run does", () => { + it("previews {{MVALUE}} with the math stand-in, because the run prompts (#1587)", async () => { // The math token is `{{MVALUE}}` (MATH_VALUE_REGEX, constants.ts). // CompleteFormatter.format runs replaceMathValueInString and // formatFileName goes through format(), so the run really does open the - // math modal here. Neither display formatter has the pass, though both - // override `promptForMathValue` with a stand-in that is therefore - // unreachable - a dead override is the tell. - const { text } = await preview("File {{MVALUE}}"); - expect(text).toBe("File {{MVALUE}}"); + // math modal here. + const { text, diagnostics } = await preview("File {{MVALUE}}"); + expect(text).toBe("File calculation_result"); + expect(diagnostics).toEqual([]); }); + it("previews an already-collected {{MVALUE}} answer rather than the stand-in", async () => { + // The one-page form and the CLI both collect this token under the key + // "mvalue"; the preview reads it exactly as the run now does (#1607). + const formatter = makeFormatter(); + (formatter as unknown as { variables: Map }).variables.set( + "mvalue", + "2+2", + ); + await expect(formatter.format("File {{MVALUE}}")).resolves.toBe("File 2+2"); + }); +}); + +describe("tokens the file-name preview does NOT resolve today", () => { + /** + * Pinned as CURRENT behaviour with the issue each is filed as, not as + * desired behaviour. + */ it("leaves {{MATH:1+1}}, which is not a token at all, as plain text", async () => { // The mock this file replaced asserted `File calculation_result` for // this input, and #1580 was filed because nothing could contradict it. @@ -207,11 +222,54 @@ describe("tokens the file-name preview does NOT resolve today", () => { expect(text).toBe("File {{MATH:1+1}}"); }); - it("leaves {{title}} literal even though the run throws on it (#1588)", async () => { - // formatFileName rejects {{title}} in a file name outright - // ("circular dependency"), so this format string can never create a note. +}); + +describe("{{title}} in a file-name format (#1588)", () => { + const CIRCULAR = + "A file name cannot contain {{title}}, because the title is derived from the file name itself - so this choice would fail at run time."; + + it("reports an error, because formatFileName throws on it", async () => { + // The token stays literal - the run resolves it to nothing here, and an + // example title would be a name that can never exist - so the row keeps + // saying "Unresolved:" and the diagnostic explains why it never will. const { text, diagnostics } = await preview("{{title}} note"); expect(text).toBe("{{title}} note"); + expect(diagnostics).toEqual([{ severity: "error", message: CIRCULAR }]); + }); + + it("matches the run's case-insensitive check", async () => { + const { diagnostics } = await preview("{{TITLE}} note"); + expect(diagnostics).toEqual([{ severity: "error", message: CIRCULAR }]); + }); + + it("catches a {{title}} produced by a global snippet, as the run's second check does", async () => { + // formatFileName re-checks format()'s OUTPUT precisely because a + // {{GLOBAL_VAR:}} snippet can smuggle the token in after the input check. + const formatter = new FileNameDisplayFormatter( + makeApp(), + { + settings: { globalVariables: { circular: "{{title}}" }, choices: [] }, + getTemplateFiles: () => [], + } as unknown as QuickAdd, + ); + formatter.setTargetFolderPath("Folder/Name"); + await formatter.format("{{GLOBAL_VAR:circular}} note"); + expect(formatter.diagnostics.list()).toEqual([ + { severity: "error", message: CIRCULAR }, + ]); + }); + + it("stays quiet for a {{title}} inside an included template body", async () => { + // The run splices that body in through the child engine's + // formatFileContent, which resolves {{title}} to the stored title (or "") + // BEFORE formatFileName's check ever sees it - so the choice works, and + // accusing it would be the cry-wolf this cluster exists to delete. + const { text, diagnostics } = await preview( + "{{TEMPLATE:Templates/Titled.md}}", + ); + // The empty title leaves the space it was followed by, exactly as the run + // does - the name normalizer only trims TRAILING spaces. + expect(text).toBe(" note"); expect(diagnostics).toEqual([]); }); }); diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index d73c41c2..5839b5e6 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -1,15 +1,20 @@ import { + defaultDateVariableFormat, findInlineScriptSpans, Formatter, hasUnterminatedInlineScriptFence, + renderStoredDateVariable, type PromptContext, } from "./formatter"; +import { parseVDateOptionsForPreview } from "../utils/vdateSyntax"; +import { snappedExampleDate } from "./helpers/snappedExampleDate"; import { describePreviewFailure, PreviewDiagnostics, } from "./previewDiagnostics"; import type { App } from "obsidian"; -import { DATE_VARIABLE_REGEX, GLOBAL_VAR_REGEX } from "../constants"; +import { TFile, TFolder } from "obsidian"; +import { DATE_VARIABLE_REGEX, GLOBAL_VAR_REGEX, TITLE_REGEX } from "../constants"; import type { IDateParser } from "../parsers/IDateParser"; import { NLDParser } from "../parsers/NLDParser"; import { @@ -141,6 +146,11 @@ export class FileNameDisplayFormatter extends Formatter { return input; } + // Before the normalizer, mirroring the run: formatFileName's + // circular-dependency check runs on the formatted string, and if it fires + // the choice dies there - no name is ever normalized (#1588). + this.reportCircularTitle(input, output); + // The run puts every generated name through this normalizer on the way to // the vault (TemplateChoiceEngine, TemplateInsertEngine, // templateNoteDiscovery, and the capture target via @@ -166,6 +176,33 @@ export class FileNameDisplayFormatter extends Formatter { return normalized.path; } + /** + * Says so when the format can never produce a name at all, because it uses + * {{title}} (#1588). + * + * `CompleteFormatter.formatFileName` rejects this token outright - the title + * IS the file name, so deriving one from the other is circular - and it does + * so twice: on the raw input, and again on `format()`'s output, because an + * expanded `{{GLOBAL_VAR:}}` snippet or a `{{VALUE}}` that resolves to the + * literal text can smuggle one in after the first check. Both halves are + * mirrored here, on the same inputs, so the preview refuses exactly what the + * run refuses. + * + * NOT `kind: "path"`. The token is still on screen unresolved, so the row + * says "Unresolved:" and means it - and the capture target, which discards + * path problems as "this field may not be a path", must keep this one: + * `formatFileName` is that field's entry point too, so `{{title}}` aborts a + * capture just as hard. + */ + private reportCircularTitle(input: string, output: string): void { + if (!TITLE_REGEX.test(input) && !TITLE_REGEX.test(output)) { + return; + } + this.reportProblem( + "A file name cannot contain {{title}}, because the title is derived from the file name itself - so this choice would fail at run time.", + ); + } + /** * Says so when the name on screen is one Obsidian will not create (#1578). * @@ -219,10 +256,29 @@ export class FileNameDisplayFormatter extends Formatter { } /** - * Is there already a file at this path? Tolerant of the missing extension, - * because a "File name format" produces the name and the engine appends - * `.md` (`normalizeMarkdownFilePath`), while a capture target usually carries - * one already. + * Is the thing this name refers to already in the vault? Tolerant of the + * missing extension, because a "File name format" produces the name and the + * engine appends `.md` (`normalizeMarkdownFilePath`), while a capture target + * usually carries one already. + * + * SHAPE-AWARE, because Obsidian's path map holds files AND folders with no + * trailing slash on either, and a bare `getAbstractFileByPath` conflated + * them: + * + * - a capture target written as `Meetings: 2026/` names a FOLDER to pick + * inside, and the folder existing is exactly what makes it work - yet the + * trailing slash matched neither probe, so the row went red for a capture + * that runs fine; + * - a bare `Meetings: 2026` may ALSO be a folder scope, and + * `captureTargetResolution` says exactly when: an existing folder with no + * real note at `Meetings: 2026.md`. That rule is mirrored here rather than + * approximated, so the preview and the resolver agree. + * + * Accepted residual, unchanged from before: a FOLDER named exactly like a + * Template choice's file-name format excuses the warning, although the run + * would still fail creating the `.md` beside it. Telling the two hosts apart + * needs the host to say which it is, and erring toward silence is this + * cluster's rule - a wrong accusation is worse than a missing one. */ private existsInVault(name: string): boolean { const vault = this.app?.vault; @@ -232,10 +288,20 @@ export class FileNameDisplayFormatter extends Formatter { if (typeof vault?.getAbstractFileByPath !== "function") return false; const trimmed = name.trim(); if (!trimmed) return false; - return Boolean( - vault.getAbstractFileByPath(trimmed) ?? - vault.getAbstractFileByPath(`${trimmed}.md`), - ); + + if (trimmed.endsWith("/")) { + return ( + vault.getAbstractFileByPath(trimmed.slice(0, -1)) instanceof TFolder + ); + } + + if (vault.getAbstractFileByPath(trimmed) instanceof TFile) return true; + const withExtension = vault.getAbstractFileByPath(`${trimmed}.md`); + if (withExtension instanceof TFile) return true; + // `captureTargetResolution` rule: a bare name that is an existing folder + // with no real note beside it is a folder SCOPE, and the capture picks a + // file inside it - nothing asks Obsidian to accept this string as a name. + return vault.getAbstractFileByPath(trimmed) instanceof TFolder; } /** @@ -276,6 +342,12 @@ export class FileNameDisplayFormatter extends Formatter { output = await this.replaceVariableInString(output); output = await this.replaceFieldVarInString(output); output = await this.replaceFileInString(output); + // Where the run has it (CompleteFormatter.format: after {{FILE:}}, before + // {{RANDOM:}}). The run really does prompt here - formatFileName goes + // through format() - so leaving {{MVALUE}} literal made the preview + // promise a name with a token in it (#1587). `promptForMathValue` was + // already overridden with a stand-in below; it was simply unreachable. + output = await this.replaceMathValueInString(output); // Note-derived tokens in ONE pass so no token re-scans another's output // (#1358). Every preview resolver returns a placeholder rather than null, // so neither mode can hit the runtime's missing-active-file throw. @@ -455,7 +527,9 @@ export class FileNameDisplayFormatter extends Formatter { this.templateInclusion.depth++; try { const body = await app.vault.cachedRead(file); - const resolved = await this.formatInternal(body, { included: true }); + const resolved = this.resolveTitleInIncludedBody( + await this.formatInternal(body, { included: true }), + ); this.warnIfJoinedIntoOneLine(templatePath, resolved); return resolved; } catch (error) { @@ -469,6 +543,32 @@ export class FileNameDisplayFormatter extends Formatter { } } + /** + * `{{title}}` inside a spliced-in `{{TEMPLATE:}}` body, resolved the way the + * run's child engine resolves it. + * + * At run time an included body goes through `SingleTemplateEngine` -> + * `CompleteFormatter.formatFileContent`, whose single-pass token resolver + * runs with `title: true` and takes `variables.get("title") ?? ""` - so the + * literal token never survives into the name that `formatFileName`'s + * circular-dependency check then reads, and the run does NOT abort. + * + * Doing it here rather than in `formatInternal`'s `included` branch is + * deliberate: routing `title` through the shared resolver would call this + * class's `getVariableValue`, which invents an example ("My Document Title") + * for an unstored variable - a fresh #1563-class lie in the one place the run + * is guaranteed to produce the empty string. And without it, the + * output-side {{title}} check above would turn a WORKING choice red. + */ + private resolveTitleInIncludedBody(body: string): string { + if (!TITLE_REGEX.test(body)) return body; + const title = this.variables.get("title"); + // A function replacer, not a string: a title containing "$&" would + // otherwise be spliced through String.replace's substitution syntax. + const resolved = typeof title === "string" ? title : ""; + return body.replace(new RegExp(TITLE_REGEX.source, "gi"), () => resolved); + } + /** * A template body is many lines and a file name is one, so the normalizer at * the end of `format()` joins them with spaces - which is what the run does @@ -539,18 +639,53 @@ export class FileNameDisplayFormatter extends Formatter { // date WITHOUT applying |startof:/|endof: snap - snap is only resolved in // the real CompleteFormatter pass, and snapping only the file-name // preview would diverge from the body preview. - output = output.replace(new RegExp(DATE_VARIABLE_REGEX.source, 'gi'), (match, variableName, dateFormat) => { + output = output.replace(new RegExp(DATE_VARIABLE_REGEX.source, 'gi'), (match, variableName, dateFormat, rawOptions) => { const cleanVariableName = variableName?.trim(); - const cleanDateFormat = dateFormat?.trim(); - if (!cleanVariableName || !cleanDateFormat) { - return match; // Return original if incomplete + // Only a NAMELESS token stays literal, which is what the run does with + // it too. A token that names no FORMAT is complete and working - the + // run supplies YYYY-MM-DD, or YYYY-MM-DD HH:mm under |time - so + // echoing it back promised a name with a token in it (#1589). + if (!cleanVariableName) { + return match; } - // Generate a realistic preview using the current date. - const previewDate = new Date(); + const { options, error } = parseVDateOptionsForPreview(rawOptions); + // A unit that never resolves is an authoring mistake the run aborts on, + // so it belongs on the diagnostics channel (held back until the field is + // idle) rather than being swallowed. The options still come back usable, + // so the TEXT does not flicker while the unit is being typed. + if (error) this.reportProblem(error); + const { withTime, snap } = options; + const cleanDateFormat = + dateFormat?.trim() || defaultDateVariableFormat(withTime); + + // An ANSWERED date wins over the example. The one-page input form + // seeds the user's real picks into this formatter before computing the + // preview (runOnePagePreflight.computePreview), so without this the row + // showed today's date beside the date they had just chosen (#1590). + const stored = renderStoredDateVariable( + this.variables.get(cleanVariableName), + cleanDateFormat, + snap, + this.dateParser, + ); + if (stored) return stored.text; + + // Nothing answered: a realistic example from the current date, snapped + // the way the run snaps it. #1595 deliberately left snap out of this + // preview, on the grounds that snapping only the file-name row would + // split it from the body row - both rows do it now, so that reason is + // gone, and the alternative was worse: the ANSWERED branch above snaps + // (it is the run's own renderer), and {{DATE:...|startof:month}} in the + // very same pass has always snapped, so the row contradicted itself + // depending on which token you used. Inside the try, because the snap + // needs moment and a throw here would redden the row per keystroke. try { - return DateFormatPreviewGenerator.generate(cleanDateFormat, previewDate); + return DateFormatPreviewGenerator.generate( + cleanDateFormat, + snappedExampleDate(snap), + ); } catch { return `[${cleanDateFormat}]`; } diff --git a/src/formatters/formatDisplayFormatter-1587-mvalue.test.ts b/src/formatters/formatDisplayFormatter-1587-mvalue.test.ts new file mode 100644 index 00000000..644ac967 --- /dev/null +++ b/src/formatters/formatDisplayFormatter-1587-mvalue.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from "vitest"; +import type { App } from "obsidian"; +import { FormatDisplayFormatter } from "./formatDisplayFormatter"; +import type QuickAdd from "../main"; + +/** + * Issue #1587, the BODY preview's half. + * + * The file-name row's half is pinned in `fileNameDisplayFormatter.test.ts`. Both + * are needed: they are two rows of the same builder, and a regression in one + * would have them contradict each other about the same token. + */ + +const app = { + workspace: { getActiveFile: () => null }, + vault: { getMarkdownFiles: () => [], getAbstractFileByPath: () => null }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, +} as unknown as App; + +const plugin = { + settings: { globalVariables: {}, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +const makeFormatter = () => new FormatDisplayFormatter(app, plugin); + +describe("FormatDisplayFormatter previews {{MVALUE}} (#1587)", () => { + it("shows the math stand-in rather than the raw token", async () => { + const formatter = makeFormatter(); + await expect(formatter.format("= {{MVALUE}}")).resolves.toBe( + "= calculation_result", + ); + // And reaches the stand-in WITHOUT opening the math modal: the inert + // contract (#1558) covers this pass like every other one. + expect(formatter.diagnostics.list()).toEqual([]); + }); + + it("prefers an already-collected answer", async () => { + const formatter = makeFormatter(); + (formatter as unknown as { variables: Map }).variables.set( + "mvalue", + "2+2", + ); + await expect(formatter.format("= {{MVALUE}}")).resolves.toBe("= 2+2"); + }); + + it("fills every occurrence from the one collected answer", async () => { + const formatter = makeFormatter(); + (formatter as unknown as { variables: Map }).variables.set( + "mvalue", + "2+2", + ); + await expect(formatter.format("{{MVALUE}}-{{MVALUE}}")).resolves.toBe( + "2+2-2+2", + ); + }); +}); diff --git a/src/formatters/formatDisplayFormatter-1589-vdate.test.ts b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts new file mode 100644 index 00000000..336aa1db --- /dev/null +++ b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts @@ -0,0 +1,112 @@ +import realMoment from "moment"; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import { FormatDisplayFormatter } from "./formatDisplayFormatter"; +import type QuickAdd from "../main"; + +/** + * The BODY preview's {{VDATE:}} behaviour, the sibling of + * `fileNameDisplayFormatter.audit-cleanup.test.ts`. + * + * The two previews are kept deliberately in step: a format string that is legal + * in both fields must render the same way in both, or the builder contradicts + * itself depending on which row you look at. + */ + +const app = { + workspace: { getActiveFile: () => null }, + vault: { getMarkdownFiles: () => [], getAbstractFileByPath: () => null }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, +} as unknown as App; + +const plugin = { + settings: { globalVariables: {}, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +function makeFormatter() { + return new FormatDisplayFormatter(app, plugin); +} + +// Rendering a stored @date:ISO needs real moment (the obsidian-stub's returns a +// fixed date), plus a frozen clock for the example-date branch. Mirrors +// fileNameDisplayFormatter.audit-cleanup.test.ts. +const originalMoment = (window as unknown as { moment?: unknown }).moment; + +beforeAll(() => { + (window as unknown as { moment: unknown }).moment = realMoment; +}); +afterAll(() => { + (window as unknown as { moment?: unknown }).moment = originalMoment; + vi.useRealTimers(); +}); +beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2023-06-01T10:30:00")); +}); + +describe("FormatDisplayFormatter VDATE default format (#1589)", () => { + it("supplies YYYY-MM-DD when the token names no format", async () => { + const formatter = makeFormatter(); + await expect(formatter.format("{{VDATE:due}}")).resolves.toBe("2023-06-01"); + expect(formatter.diagnostics.list()).toEqual([]); + }); + + it("supplies the datetime default under |time", async () => { + await expect(makeFormatter().format("{{VDATE:due|time}}")).resolves.toMatch( + /^2023-06-01 \d{2}:\d{2}$/, + ); + }); + + it("keeps the (default: X) hint the body preview is entitled to", async () => { + // The hints live here and NOT on the file-name row: the run's prompt + // really does say "(default: tomorrow)", but it splices only the date into + // a file name (#1578). + await expect( + makeFormatter().format("{{VDATE:due|tomorrow}}"), + ).resolves.toBe("2023-06-01 (default: tomorrow)"); + }); + + it("still leaves a NAMELESS token literal, as the run does", async () => { + await expect(makeFormatter().format("{{VDATE:}}")).resolves.toBe( + "{{VDATE:}}", + ); + }); + + it("renders an ANSWERED date instead of today", async () => { + const formatter = makeFormatter(); + (formatter as unknown as { variables: Map }).variables.set( + "due", + // Built from a LOCAL date, not a "Z" literal: the stored value is an + // instant and moment formats it in local time, so a midnight-UTC + // literal renders as the 14th anywhere west of UTC (this test file's + // sibling carries the same warning). + `@date:${new Date(2026, 7, 15, 12, 0, 0).toISOString()}`, + ); + await expect(formatter.format("{{VDATE:due}}")).resolves.toBe("2026-08-15"); + }); + + it("keeps the text stable for a half-typed |startof: unit, and still reports it", async () => { + // parseVDateOptions throws on a unit that does not resolve. Before, that + // blanked the whole body preview between two keystrokes (#1558's failure + // mode); swallowing it outright would have deleted a diagnostic this + // preview already shipped. The text stays a date, the complaint goes on + // the channel the builder holds back until the field is idle. + const formatter = makeFormatter(); + await expect( + formatter.format("{{VDATE:wk,YYYY-MM-DD|startof:we}}"), + ).resolves.toBe("2023-06-01"); + expect(formatter.diagnostics.list()).toEqual([ + { + severity: "error", + message: expect.stringContaining('Unknown date unit "we"') as never, + }, + ]); + }); + + it("applies a valid snap to the example, matching {{DATE:...|startof:}}", async () => { + await expect( + makeFormatter().format("{{VDATE:wk,YYYY-MM-DD|startof:month}}"), + ).resolves.toBe("2023-06-01"); + }); +}); diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index b54cb629..3dc3729b 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -1,4 +1,9 @@ -import { Formatter, type PromptContext } from "./formatter"; +import { + defaultDateVariableFormat, + Formatter, + renderStoredDateVariable, + type PromptContext, +} from "./formatter"; import { describePreviewFailure, PreviewDiagnostics, @@ -22,7 +27,8 @@ import { DateFormatPreviewGenerator } from "./helpers/previewHelpers"; import { getValueVariableBaseName } from "../utils/valueSyntax"; -import { parseVDateOptions } from "../utils/vdateSyntax"; +import { parseVDateOptionsForPreview } from "../utils/vdateSyntax"; +import { snappedExampleDate } from "./helpers/snappedExampleDate"; import { EnhancedFieldSuggestionFileFilter } from "../utils/EnhancedFieldSuggestionFileFilter"; import { FILE_CUSTOM_PREFIX, FILE_PICK_PREFIX, type ParsedFileToken } from "../utils/fileSyntax"; @@ -124,6 +130,10 @@ export class FormatDisplayFormatter extends Formatter { output = await this.replaceTemplateInString(output); output = await this.replaceFieldVarInString(output); output = await this.replaceFileInString(output); + // Where the run has it (CompleteFormatter.format: after {{FILE:}}, before + // {{RANDOM:}}). `promptForMathValue` was already overridden below with a + // stand-in that nothing could reach (#1587). + output = await this.replaceMathValueInString(output); output = this.replaceRandomInString(output); return output; @@ -306,26 +316,54 @@ export class FormatDisplayFormatter extends Formatter { // For preview, show helpful format examples instead of failing output = output.replace(new RegExp(DATE_VARIABLE_REGEX.source, 'gi'), (match, variableName, dateFormat, rawOptions) => { const cleanVariableName = variableName?.trim(); - const cleanDateFormat = dateFormat?.trim(); - const { defaultValue: cleanDefaultValue, optional } = - parseVDateOptions(rawOptions); + const { options, error } = parseVDateOptionsForPreview(rawOptions); + // Reported, not swallowed: a unit that never resolves aborts the run. + // The options still come back usable, so the preview TEXT stays stable + // while the unit is half-typed. + if (error) this.reportProblem(error); + const { + defaultValue: cleanDefaultValue, + optional, + withTime, + snap, + } = options; + // Only a NAMELESS token stays literal, as the run leaves it. A token + // that names no FORMAT is complete and working: the run supplies + // YYYY-MM-DD, or YYYY-MM-DD HH:mm under |time (#1589). + const cleanDateFormat = + dateFormat?.trim() || defaultDateVariableFormat(withTime); - if (!cleanVariableName || !cleanDateFormat) { + if (!cleanVariableName) { return match; // Return original if incomplete } - // Generate a preview using current date with the specified format - const previewDate = new Date(); + // An ANSWERED date wins over the example, resolved through the run's + // own renderer so a seeded @date:ISO renders exactly as it will. + const stored = renderStoredDateVariable( + this.variables.get(cleanVariableName), + cleanDateFormat, + snap, + this.dateParser, + ); + if (stored) return stored.text; + + // Generate a preview using current date with the specified format, + // snapped the way the run snaps it - matching both the ANSWERED branch + // above and {{DATE:...|startof:}}, which has always snapped in this + // same pass. Inside the try: the snap needs moment. let formattedExample: string; - + try { // Try to generate a realistic preview using the format - formattedExample = DateFormatPreviewGenerator.generate(cleanDateFormat, previewDate); + formattedExample = DateFormatPreviewGenerator.generate( + cleanDateFormat, + snappedExampleDate(snap), + ); } catch { // Fallback to showing the format pattern formattedExample = `[${cleanDateFormat} format]`; } - + // If there's a default value, indicate it in the preview if (cleanDefaultValue) { formattedExample += ` (default: ${cleanDefaultValue})`; diff --git a/src/formatters/formatter.ts b/src/formatters/formatter.ts index 3217dc38..e3e36c57 100644 --- a/src/formatters/formatter.ts +++ b/src/formatters/formatter.ts @@ -56,7 +56,7 @@ import { } from "../utils/valueSyntax"; import { SILENT_WARN, type WarnSink } from "../utils/warnSink"; import { parseVDateOptions } from "../utils/vdateSyntax"; -import { applyDateSnap, parseDateSnapSegment } from "../utils/dateModifiers"; +import { applyDateSnap, type DateSnap, parseDateSnapSegment } from "../utils/dateModifiers"; import { parseMacroToken } from "../utils/macroSyntax"; import { formatUnknownValue } from "../utils/conditionalHelpers"; @@ -180,6 +180,102 @@ export function hasUnterminatedInlineScriptFence(input: string): boolean { return false; } +/** + * The default format a `{{VDATE:}}` token gets when it names none. + * + * `{{VDATE:due}}` is a complete, working token - the run supplies this and + * splices the result in. Shared with both preview formatters, which used to + * bail out and echo the raw token instead, so the builder showed + * `{{VDATE:due}}` for a choice that creates `2026-07-27.md` (#1589). A + * `|time`/`|datetime` token needs the picked time in the output, hence the + * second shape - note it contains a colon, which is why fixing this preview is + * what lets the illegal-character diagnostic (#1578) fire on `{{VDATE:d|time}}` + * in a FILE NAME, where it always should have. + */ +export function defaultDateVariableFormat(withTime: boolean): string { + return withTime ? "YYYY-MM-DD HH:mm" : "YYYY-MM-DD"; +} + +/** + * What a STORED `{{VDATE:}}` answer renders to, and the canonical form it + * should be written back as. + * + * Extracted from `replaceDateVariableInString`'s tail so the run and both + * previews share one implementation. The previews need it because the one-page + * input form seeds the user's real answers into the formatter before computing + * the preview: without this they rendered `new Date()` and showed today beside + * the date the user had just picked (#1589/#1590). Every other seeded + * requirement type already round-trips through `getVariableValue`; VDATE was + * the one that did not. + * + * `null` means nothing is stored (`undefined`), which is the run's signal to + * prompt and a preview's signal to fall back to an example date. Every other + * value - including `null` and `""`, which are answered-empty under the same + * contract as VALUE variables (#872) - renders. + * + * `normalized` is present only when the caller should write the coerced + * `@date:ISO` form back into its variables map; a preview may ignore it. + */ +export function renderStoredDateVariable( + stored: unknown, + dateFormat: string, + snap: DateSnap | undefined, + dateParser: IDateParser | undefined, +): { text: string; normalized?: string } | null { + if (stored === undefined) return null; + + let value = stored; + let normalized: string | undefined; + + // A VDATE variable pre-seeded as a plain string (via the JS API, a URI + // parameter, or a `quickadd:run value-x=` flag) is coerced into the internal + // @date:ISO form so formatting works. Only when it parses: an unparseable + // string keeps the verbatim branch below, for back-compat. + if (typeof value === "string" && value && !value.startsWith("@date:")) { + if (dateParser) { + const aliasMap = settingsStore.getState().dateAliases; + const parseAttempt = dateParser.parseDate( + normalizeDateInput(value, aliasMap), + ); + if (parseAttempt) { + normalized = `@date:${parseAttempt.moment.toISOString()}`; + value = normalized; + } + } + } else if (value instanceof Date) { + // Some callers pass actual Date objects through the JS API. + if (!Number.isNaN(value.getTime())) { + normalized = `@date:${value.toISOString()}`; + value = normalized; + } + } + + if (typeof value === "string" && value.startsWith("@date:")) { + const isoString = value.substring(6); + if (dateParser && window.moment) { + const moment = window.moment(isoString); + if (moment?.isValid()) { + // Snap is per-occurrence (issue #511): the stored @date:ISO stays + // raw, so {{VDATE:d,F1|startof:week}} and {{VDATE:d,F2}} share one + // picked date but only one snaps. A fresh moment per call prevents + // leaks. + return { text: applyDateSnap(moment, snap).format(dateFormat), ...(normalized !== undefined ? { normalized } : {}) }; + } + } + return { text: "", ...(normalized !== undefined ? { normalized } : {}) }; + } + if (typeof value === "string" && value) { + // Backward compatibility: use the stored value as-is. + return { text: value }; + } + if (value != null) { + // Avoid throwing if a non-string value is stored. + return { text: formatUnknownValue(value) }; + } + // null, or "" - answered-empty. + return { text: "" }; +} + export abstract class Formatter { protected value: string; protected variables: Map = new Map(); @@ -1371,13 +1467,29 @@ export abstract class Formatter { // This avoids infinite replacement loops when the provided math input contains {{MVALUE}}. const regex = new RegExp(MATH_VALUE_REGEX.source, "gi"); + // An already-collected answer wins, the way {{VALUE}} reads + // `this.variables` before prompting. `RequirementCollector` registers a + // requirement keyed "mvalue" for this token, so the one-page input form + // asks for a "Math expression" and the CLI advertises + // `value-mvalue=` - and, without this, both answers were dropped: + // the form asked, then the run opened the math modal on top of it, and + // the CLI flag it had just recommended made no difference (#1607). + // + // READ, never write: leaving `promptForMathValue`'s answer unstored keeps + // the per-occurrence prompt that `{{MVALUE}} {{MVALUE}}` has always had. + // A single collected answer legitimately fills every occurrence, because + // the collector registers exactly one requirement for the token. + const collected = this.variables.get("mvalue"); + const collectedValue = + typeof collected === "string" && collected.trim() ? collected : null; + let output = ""; let lastIndex = 0; let match: RegExpExecArray | null; while ((match = regex.exec(input)) !== null) { output += input.slice(lastIndex, match.index); - output += await this.promptForMathValue(); + output += collectedValue ?? (await this.promptForMathValue()); lastIndex = match.index + match[0].length; } @@ -1494,7 +1606,7 @@ export abstract class Formatter { // A |time/|datetime token with no explicit format gets a datetime // default so the rendered value carries the picked time. const dateFormat = - match[2]?.trim() || (withTime ? "YYYY-MM-DD HH:mm" : "YYYY-MM-DD"); + match[2]?.trim() || defaultDateVariableFormat(withTime); const existingValue = this.variables.get(variableName); @@ -1542,64 +1654,21 @@ export abstract class Formatter { } } - // Format the date based on what's stored - let formattedDate = ""; - let storedValue = this.variables.get(variableName); - - // If a VDATE variable was pre-seeded (e.g., via API/URL) as a plain string, - // attempt to coerce it into the internal @date:ISO form so formatting works. - if ( - typeof storedValue === "string" && - storedValue && - !storedValue.startsWith("@date:") - ) { - if (this.dateParser) { - const aliasMap = settingsStore.getState().dateAliases; - const normalizedInput = normalizeDateInput(storedValue, aliasMap); - const parseAttempt = this.dateParser.parseDate(normalizedInput); - - // Keep backwards compatibility: only coerce if we can parse it. - if (parseAttempt) { - const iso = parseAttempt.moment.toISOString(); - const coerced = `@date:${iso}`; - this.variables.set(variableName, coerced); - storedValue = coerced; - } - } - } else if (storedValue instanceof Date) { - // Some callers may pass actual Date objects through the JS API. - if (!Number.isNaN(storedValue.getTime())) { - const coerced = `@date:${storedValue.toISOString()}`; - this.variables.set(variableName, coerced); - storedValue = coerced; - } - } - - if (typeof storedValue === "string" && storedValue.startsWith("@date:")) { - // It's a date variable, extract and format it - const isoString = storedValue.substring(6); - - if (this.dateParser && window.moment) { - const moment = window.moment(isoString); - if (moment && moment.isValid()) { - // Snap is per-occurrence (issue #511): the stored - // @date:ISO stays raw, so {{VDATE:d,F1|startof:week}} - // and {{VDATE:d,F2}} share one picked date but only one - // snaps. A fresh moment per iteration prevents leaks. - formattedDate = applyDateSnap(moment, snap).format( - dateFormat, - ); - } - } - } else if (typeof storedValue === "string" && storedValue) { - // Backward compatibility: use the stored value as-is - formattedDate = storedValue; - } else if (storedValue != null) { - // Fallback: avoid throwing if a non-string value is stored. - formattedDate = formatUnknownValue(storedValue); + // Format the date based on what's stored. Shared with both preview + // formatters, which resolve an ANSWERED {{VDATE:}} through the same + // helper so the one-page form's preview shows the date the user just + // picked rather than today's (#1589). + const rendered = renderStoredDateVariable( + this.variables.get(variableName), + dateFormat, + snap, + this.dateParser, + ); + if (rendered?.normalized !== undefined) { + this.variables.set(variableName, rendered.normalized); } - output += formattedDate; + output += rendered?.text ?? ""; } return output + input.slice(lastIndex); diff --git a/src/formatters/helpers/snappedExampleDate.ts b/src/formatters/helpers/snappedExampleDate.ts new file mode 100644 index 00000000..da3ffab9 --- /dev/null +++ b/src/formatters/helpers/snappedExampleDate.ts @@ -0,0 +1,23 @@ +import { applyDateSnap, type DateSnap } from "../../utils/dateModifiers"; + +/** + * "Today", snapped the way `|startof:`/`|endof:` will snap it at run time - for + * the EXAMPLE a `{{VDATE:}}` preview shows when the user has not answered yet. + * + * #1595 deliberately left snap out of the file-name preview, on the grounds + * that snapping only that row would split it from the body row. Both rows use + * this now, so that reason is gone - and the alternative was worse: the + * ANSWERED branch beside it snaps (it runs the run's own renderer), and + * `{{DATE:...|startof:month}}` in the same pass has always snapped, so one row + * gave two answers depending on which token you used. + * + * Without a snap it returns a plain `new Date()` and never touches moment. That + * matters: `window.moment` is Obsidian's, and routing every dateless preview + * through it would make the common case depend on a global the callers' + * `catch` would otherwise turn into a `[YYYY-MM-DD]` placeholder. + */ +export function snappedExampleDate(snap: DateSnap | undefined): Date { + const now = new Date(); + if (!snap) return now; + return applyDateSnap(window.moment(now), snap).toDate(); +} diff --git a/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte b/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte index 44f5867f..901e31ed 100644 --- a/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte +++ b/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte @@ -43,6 +43,7 @@ import ChoiceIconSetting from "./components/ChoiceIconSetting.svelte"; import { suggester } from "./components/suggesterAction"; import { VALUE_SYNTAX } from "../../constants"; import { usesDefaultTemplateTitlePrompt } from "../../utils/templateNoteDiscoveryEligibility"; +import { likelyTargetFolderPath } from "../../utils/previewTargetFolder"; /** * Reactive replacement for TemplateChoiceBuilder.display(). Conditional rows are @@ -227,6 +228,7 @@ function onModeChange(value: string) { diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index ea49ed49..1cabdaa0 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -93,12 +93,38 @@ const hidden = $derived( : false, ); -// A field whose format could not be resolved is not showing a preview of the +// Two different things can be wrong, and one label for both sent readers +// hunting for a broken token when every token had resolved (issue #1594). +// +// A field whose format could not be RESOLVED is not showing a preview of the // output — it is showing the raw text back. Say so, rather than letting // "Preview:" assert that this IS what you will get. -const isUnresolved = $derived( - showDiagnostics && diagnostics.some((d) => d.severity === "error"), -); +// +// The `kind: "path"` problems are the other class: the format resolved +// perfectly, the vault just will not accept the result (a "."/".." or empty +// segment, #1563; a character Obsidian refuses, #1578). That is the ordinary +// case on a file-name field, and the row IS an accurate preview — of a name +// that will never exist. Reusing the axis #1582 already added for `hideWhen` +// keeps one classification instead of two that can disagree. +// +// Unresolved wins when both are present: if a template is missing AND the +// result has an empty segment, the fundamental failure is that it did not +// resolve. +// +// `diagnostics` deliberately SURVIVES a value change until the next pass +// commits. A pass can outlast the 500ms gate (it may read up to 25 templates), +// so the gate can open while the previous result is still shown - but `preview` +// and `diagnostics` are written together under `previewToken`, so what is on +// screen is always one pass's text under that same pass's label and complaint. +// Stale, never mixed. +// +// Clearing `diagnostics` here instead would leave the previous, known-bad name +// on screen under a plain "Preview:" label for the length of the pass, which is +// precisely the assertion this row exists to withdraw. Pinned by "a slow pass +// never mixes two values in one row" in the sibling test. +const errors = $derived(showDiagnostics ? diagnostics.filter((d) => d.severity === "error") : []); +const isUnresolved = $derived(errors.some((d) => d.kind !== "path")); +const isInvalid = $derived(!isUnresolved && errors.length > 0); $effect(() => { const current = value; @@ -180,7 +206,12 @@ $effect(() => { class:qa-preview-row--one-line={formatterKind === "fileName"} title={formatterKind === "fileName" ? preview : undefined} > - {isUnresolved ? "Unresolved: " : "Preview: "}{isUnresolved + ? "Unresolved: " + : isInvalid + ? "Won't be created: " + : "Preview: "}{preview} {#if showDiagnostics && diagnostics.length > 0} diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts index 15a10185..7269c080 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { render } from "@testing-library/svelte"; import { tick } from "svelte"; import type { App } from "obsidian"; +import { TFile } from "obsidian"; import FormatPreviewField from "./FormatPreviewField.svelte"; import type QuickAdd from "../../../main"; import { LogManager } from "../../../logger/logManager"; @@ -179,6 +180,77 @@ describe("FormatPreviewField", () => { ).not.toBeNull(); }); + describe("the label tells the two error classes apart (#1594)", () => { + it("says the name will not be created when everything RESOLVED", async () => { + // Every token resolved; the vault just refuses the result. "Unresolved:" + // here sent the reader hunting for a broken token that does not exist. + const { container } = render(FormatPreviewField, { + props: { + value: "Bad: {{VALUE:title}}", + formatterKind: "fileName" as const, + app, + plugin, + }, + }); + await settleAndIdle(); + + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Won't be created: ", + ); + expect(container.querySelector(".qa-preview-value")?.textContent).toBe( + "Bad: Example Title", + ); + expect( + container.querySelector(".qa-preview-issue--error"), + ).not.toBeNull(); + }); + + it("keeps saying Unresolved when a token could not resolve at all", async () => { + const { container } = render(FormatPreviewField, { + props: { + value: "{{TEMPLATE:missing.md}}", + formatterKind: "fileName" as const, + app, + plugin, + }, + }); + await settleAndIdle(); + + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Unresolved: ", + ); + }); + + it("prefers Unresolved when both classes are present", async () => { + // A missing template AND an empty path segment: the fundamental failure + // is that it did not resolve, so that is what the label says. + const { container } = render(FormatPreviewField, { + props: { + value: "{{TEMPLATE:missing.md}}//x", + formatterKind: "fileName" as const, + app, + plugin, + }, + }); + await settleAndIdle(); + + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Unresolved: ", + ); + }); + + it("says Preview when a pass only produced warnings", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "{{VALUE:title}}", app, plugin }, + }); + await settleAndIdle(); + + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Preview: ", + ); + }); + }); + it("re-previews an edited named option list instead of the stale first value", async () => { // The formatter used to be memoized for the field's lifetime. Its // `variables` map short-circuits an already-resolved key, so editing the @@ -214,3 +286,110 @@ describe("FormatPreviewField", () => { expect(container.querySelector(".qa-preview-row")).toBeNull(); }); }); + +/** + * The row's async pass can outlast the 500ms diagnostics gate (it may read up to + * 25 templates), so the gate can open while the previous value's result is still + * on screen. What must never happen is a MIX: a label or a complaint describing + * one value beside another value's text. + * + * Pinned because the obvious "fix" - clearing `diagnostics` when `value` + * changes - makes it worse, not better: it would leave the previous, known-bad + * name on screen under a plain "Preview:" label, which is the exact assertion + * this cluster exists to withdraw. + */ +describe("a slow pass never mixes two values in one row", () => { + const deferredApp = (release: { fn?: () => void }) => + ({ + workspace: { getActiveFile: () => null }, + vault: { + getMarkdownFiles: () => [], + getAbstractFileByPath: (path: string) => + path === "Slow.md" + ? Object.assign(new TFile(), { + path, + extension: "md", + basename: "Slow", + }) + : null, + cachedRead: () => + new Promise((resolve) => { + release.fn = () => resolve("Bad: slow body"); + }), + }, + metadataCache: { + getFileCache: () => null, + getAllPropertyInfos: () => ({}), + }, + }) as unknown as App; + + it("keeps the old value's text under the old value's label until the new pass lands", async () => { + const release: { fn?: () => void } = {}; + const { container, rerender } = render(FormatPreviewField, { + props: { + value: "Bad: {{VALUE:title}}", + formatterKind: "fileName" as const, + app: deferredApp(release), + plugin, + }, + }); + await settleAndIdle(); + + const label = () => + container.querySelector(".qa-preview-label")?.textContent; + const text = () => + container.querySelector(".qa-preview-value")?.textContent; + + expect(label()).toBe("Won't be created: "); + expect(text()).toBe("Bad: Example Title"); + + // Switch to a format whose pass cannot finish (the template read hangs), + // then let the idle gate open while it is still in flight. + await rerender({ value: "{{TEMPLATE:Slow.md}}" }); + await settleAndIdle(); + + // Stale, but COHERENT: the label still describes the text beside it, and + // the complaint still belongs to that same text. + expect(text()).toBe("Bad: Example Title"); + expect(label()).toBe("Won't be created: "); + expect(issues(container as HTMLElement)).toEqual([ + 'A file or folder name cannot contain ":", so this choice would fail at run time. Check your own text and tokens like {{TIME}}, which is HH:mm.', + ]); + + // Once it lands, text, label and complaint move together. + release.fn?.(); + await settleAndIdle(); + expect(text()).toBe("Bad: slow body"); + expect(label()).toBe("Won't be created: "); + }); + + it("drops a stale pass that lands after a newer one", async () => { + const release: { fn?: () => void } = {}; + const { container, rerender } = render(FormatPreviewField, { + props: { + value: "{{TEMPLATE:Slow.md}}", + formatterKind: "fileName" as const, + app: deferredApp(release), + plugin, + }, + }); + await settle(); + + await rerender({ value: "Clean {{VALUE:title}}" }); + await settleAndIdle(); + expect(container.querySelector(".qa-preview-value")?.textContent).toBe( + "Clean Example Title", + ); + + // The first pass finishes last; previewToken must discard it. + release.fn?.(); + await settleAndIdle(); + expect(container.querySelector(".qa-preview-value")?.textContent).toBe( + "Clean Example Title", + ); + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Preview: ", + ); + expect(issues(container as HTMLElement)).toEqual([]); + }); +}); diff --git a/src/preflight/OnePageInputModal.test.ts b/src/preflight/OnePageInputModal.test.ts index d4be308b..0077b8c4 100644 --- a/src/preflight/OnePageInputModal.test.ts +++ b/src/preflight/OnePageInputModal.test.ts @@ -220,13 +220,26 @@ function ensureObsidianDomPolyfills(): void { return this; }; - proto.createEl ??= function (tag: string, options?: { text?: string }) { + proto.createEl ??= function ( + tag: string, + options?: { text?: string; cls?: string }, + ) { const el = document.createElement(tag); if (options?.text !== undefined) el.textContent = options.text; + if (options?.cls) el.className = options.cls; this.appendChild(el); return el; }; + proto.createSpan ??= function (options?: { text?: string; cls?: string }) { + return this.createEl("span", options); + }; + + proto.appendText ??= function (text: string) { + this.appendChild(document.createTextNode(text)); + return this; + }; + proto.createDiv ??= function (options?: { cls?: string; text?: string }) { const div = document.createElement("div"); if (options?.cls) div.className = options.cls; @@ -871,3 +884,211 @@ describe("OnePageInputModal - image paste wiring (issue #1484)", () => { await expect(modal.waitForClose).resolves.toEqual({ body: "" }); }); }); + +/** + * The live preview block, which used to render only the text `computePreview` + * returned - dropping the formatter's diagnostics entirely, so a name Obsidian + * refuses was presented here in ordinary styling with nothing said (#1590). + */ +describe("OnePageInputModal preview block (#1590)", () => { + beforeEach(() => { + ensureObsidianDomPolyfills(); + }); + + const requirement: FieldRequirement[] = [ + { id: "title", label: "title", type: "text" }, + ]; + + /** Lets the preview's async pass settle. */ + const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); + + const previewEl = (modal: OnePageInputModal) => + (modal as any).contentEl.querySelector( + ".qa-onepage-preview", + ) as HTMLElement; + + it("renders each diagnostic with the builder's error styling", async () => { + const modal = new OnePageInputModal( + {} as App, + requirement, + undefined, + () => [ + { + label: "File name", + text: "Bad: My Note", + diagnostics: [ + { severity: "error", message: "refused", kind: "path" }, + ], + }, + ], + ); + await settle(); + + const block = previewEl(modal); + // The block un-hides for a row (the inverse of the collapse below). + expect(block.classList.contains("qa-hidden")).toBe(false); + expect(block.querySelector(".qa-preview-val")?.textContent).toBe( + "Bad: My Note", + ); + const issue = block.querySelector(".qa-preview-issue"); + expect(issue).not.toBeNull(); + expect(issue?.classList.contains("qa-preview-issue--error")).toBe(true); + // Severity in text, not colour alone (WCAG 1.4.1). + expect( + issue?.querySelector(".qa-visually-hidden")?.textContent, + ).toBe("Error: "); + expect(issue?.textContent).toContain("refused"); + modal.close(); + }); + + it("uses the builder's three-state label vocabulary (#1594)", async () => { + const withDiagnostics = ( + diagnostics: { severity: string; message: string; kind?: string }[], + ) => + new OnePageInputModal({} as App, requirement, undefined, () => [ + { label: "File name", text: "x", diagnostics } as never, + ]); + + const clean = withDiagnostics([]); + await settle(); + expect(previewEl(clean).querySelector(".qa-preview-key")?.textContent).toBe( + "File name:", + ); + clean.close(); + + // Resolved, but the vault will refuse it. + const invalid = withDiagnostics([ + { severity: "error", message: "refused", kind: "path" }, + ]); + await settle(); + expect( + previewEl(invalid).querySelector(".qa-preview-key")?.textContent, + ).toBe("Won't be created:"); + invalid.close(); + + // Could not resolve at all - and it wins when both are present. + const unresolved = withDiagnostics([ + { severity: "error", message: "refused", kind: "path" }, + { severity: "error", message: "Template not found: Gone.md" }, + ]); + await settle(); + expect( + previewEl(unresolved).querySelector(".qa-preview-key")?.textContent, + ).toBe("Unresolved:"); + unresolved.close(); + }); + + it("collapses to nothing when the choice has no preview to show", async () => { + // Every Capture, Macro and Multi choice, and every Template using the + // default note-title prompt, rendered a padded tinted box containing the + // single word "Preview". + const modal = new OnePageInputModal( + {} as App, + requirement, + undefined, + () => [], + ); + await settle(); + + const block = previewEl(modal); + expect(block.classList.contains("qa-hidden")).toBe(true); + expect(block.textContent).toBe(""); + modal.close(); + }); + + it("computes its FIRST preview from the prefilled answers", async () => { + // display() used to fire updatePreviews above the renderField loop, and + // this.result is only populated inside it - so the opening pass ran with + // {} and flashed a stand-in over answers that were already known. + const calls: Array> = []; + const modal = new OnePageInputModal( + {} as App, + requirement, + new Map([["title", "Prefilled"]]), + (values) => { + calls.push({ ...values }); + return []; + }, + ); + await settle(); + + expect(calls[0]).toEqual({ title: "Prefilled" }); + modal.close(); + }); + + it("withholds an untouched required date, as submit() does", async () => { + // A required blank date is OMITTED on submit so the sequential date prompt + // still fires. Previewing it as answered-empty rendered a name the run + // will never create (#1590). + const calls: Array> = []; + const modal = new OnePageInputModal( + {} as App, + [ + { id: "title", label: "title", type: "text" }, + { id: "due", label: "due", type: "date" }, + ], + undefined, + (values) => { + calls.push({ ...values }); + return []; + }, + ); + await settle(); + + expect(calls[0]).not.toHaveProperty("due"); + modal.close(); + }); + + it("keeps an untouched OPTIONAL date, which is answered-empty", async () => { + const calls: Array> = []; + const modal = new OnePageInputModal( + {} as App, + [{ id: "due", label: "due", type: "date", optional: true }], + undefined, + (values) => { + calls.push({ ...values }); + return []; + }, + ); + await settle(); + + expect(calls[0]).toEqual({ due: "" }); + modal.close(); + }); + + it("commits text and problems from the same pass, never a mix", async () => { + // The 150ms debounce orders the STARTS of these passes, not their + // completions, and a pass can read up to 25 templates. + const deferred: Array<(rows: unknown) => void> = []; + const modal = new OnePageInputModal( + {} as App, + requirement, + undefined, + () => new Promise((resolve) => deferred.push(resolve as never)), + ); + // Second pass starts while the first is still pending. + void (modal as any).updatePreviews(); + await settle(); + expect(deferred).toHaveLength(2); + + // The NEWER pass lands first... + deferred[1]([ + { label: "File name", text: "new", diagnostics: [] }, + ] as never); + await settle(); + // ...then the stale one, which must not overwrite it. + deferred[0]([ + { + label: "File name", + text: "stale", + diagnostics: [{ severity: "error", message: "stale problem" }], + }, + ] as never); + await settle(); + + const block = previewEl(modal); + expect(block.querySelector(".qa-preview-val")?.textContent).toBe("new"); + expect(block.querySelector(".qa-preview-issue")).toBeNull(); + modal.close(); + }); +}); diff --git a/src/preflight/OnePageInputModal.ts b/src/preflight/OnePageInputModal.ts index b363c03e..33623e05 100644 --- a/src/preflight/OnePageInputModal.ts +++ b/src/preflight/OnePageInputModal.ts @@ -34,10 +34,26 @@ import { normalizeSliderValue, } from "src/utils/valueSyntax"; import { promptCancelled } from "../errors/UserCancelError"; +import type { PreviewDiagnostic } from "src/formatters/previewDiagnostics"; + +/** + * One row of the live preview block, with the problems that pass ran into. + * + * An ordered list, not a keyed record: the row needs a display LABEL, and the + * record's raw key was being rendered - so the file-name row read `fileName:` + * (#1590). `diagnostics` is the channel the builder's row has had since #1558 + * and this one silently dropped, which is how a name Obsidian refuses could be + * presented here in ordinary styling with nothing said. + */ +export type PreviewRow = { + label: string; + text: string; + diagnostics: readonly PreviewDiagnostic[]; +}; type PreviewComputer = ( values: Record, -) => Promise> | Record; +) => Promise | PreviewRow[]; export class OnePageInputModal extends Modal { private readonly requirements: FieldRequirement[]; @@ -53,6 +69,8 @@ export class OnePageInputModal extends Modal { private readonly dateParseErrors = new Set(); private readonly computePreview?: PreviewComputer; private previewContainerEl: HTMLElement | null = null; + /** Monotonic guard so a slow preview pass cannot commit over a newer one. */ + private previewToken = 0; private updatePreviewDebounced: () => void; private settled = false; private readonly imagePasteHandles: ImagePasteHandle[] = []; @@ -99,20 +117,24 @@ export class OnePageInputModal extends Modal { const title = this.contentEl.createEl("h2", { text: "Provide inputs" }); title.addClass("qa-onepage-title"); - // Optional live preview area + // Optional live preview area. Created (empty and collapsed) before the + // fields so it keeps its place between the title and the first input; the + // standing "Preview" heading is gone, because each row now carries its own + // label and a heading reading "Preview" would re-assert the promise + // "Won't be created:" exists to withdraw (#1590/#1594). if (this.computePreview) { this.previewContainerEl = this.contentEl.createDiv(); - this.previewContainerEl.addClass("qa-onepage-preview"); - const label = this.previewContainerEl.createEl("div", { - text: "Preview", - }); - label.addClass("qa-onepage-preview-label"); - void this.updatePreviews(); + this.previewContainerEl.addClass("qa-onepage-preview", "qa-hidden"); } // Render fields this.requirements.forEach((req) => this.renderField(req)); + // AFTER the fields: `this.result` is populated inside renderField, so the + // first pass used to run against an empty map and flash a stand-in + // ("Example Title") over prefilled answers for 150ms. + if (this.computePreview) void this.updatePreviews(); + // Action bar const btnRow = this.contentEl.createDiv(); new Setting(btnRow) @@ -628,9 +650,6 @@ export class OnePageInputModal extends Modal { return; } const out: Record = {}; - const requirementsById = new Map( - this.requirements.map((req) => [req.id, req]), - ); // A required date whose typed text failed to parse must not slip through: // without a parse error the value would be silently dropped (and the @@ -658,13 +677,32 @@ export class OnePageInputModal extends Modal { if (picks.join(", ") !== text) this.multiSelections.delete(id); } + Object.assign(out, this.collectAnswers()); + this.settled = true; + this.close(); + this.resolvePromise(out); + } + + /** + * The answers as the consumer will actually receive them. + * + * Shared by `submit()` and the live preview so the preview cannot render a + * value the submit path is about to withhold (#1590). The one rule that + * differs from a plain copy of `this.result`: an empty DATE field is only an + * answer when the token said `|optional`. A required blank date, or any date + * whose text failed to parse, is OMITTED so it stays unresolved and the + * sequential date prompt - with its picker and aliases - still fires. + * + * Deriving that in the preflight instead would only get half of it: the + * parse-error set lives here, on the modal. + */ + private collectAnswers(): Record { + const requirementsById = new Map( + this.requirements.map((req) => [req.id, req] as const), + ); + const out: Record = {}; this.result.forEach((v, k) => { const requirement = requirementsById.get(k); - - // Empty date fields: an optional date that is genuinely blank is - // answered-empty (""). A required blank date — or any date whose - // text failed to parse — is OMITTED so it stays unresolved and the - // sequential date prompt (with picker and aliases) still fires. if (requirement?.type === "date" && v === "") { const hasParseError = this.dateParseErrors.has(k); if (!requirement.optional || hasParseError) return; @@ -678,9 +716,7 @@ export class OnePageInputModal extends Modal { // matching the wide and single-line prompts. out[k] = v; }); - this.settled = true; - this.close(); - this.resolvePromise(out); + return out; } private cancel() { @@ -702,34 +738,75 @@ export class OnePageInputModal extends Modal { private async updatePreviews() { if (!this.computePreview || !this.previewContainerEl) return; + // The builder's guard (FormatPreviewField), which this block never had: + // the 150ms debounce orders the STARTS of these passes, not their + // completions, and a pass can read up to 25 templates. Text and problems + // have to commit together, or one pass's name stands beside another's + // complaint. + const token = ++this.previewToken; try { - const values: Record = {}; - this.result.forEach((v, k) => (values[k] = v)); - const preview = await this.computePreview(values); - // Clear old preview lines (leave the label at index 0) - const children = Array.from(this.previewContainerEl.children); - for (let i = 1; i < children.length; i++) { - children[i].remove(); - } - Object.entries(preview).forEach(([k, v]) => { - const row = this.previewContainerEl!.createDiv({ + // The same answers submit() will hand over, so the row never previews a + // value the run is about to re-ask for: an untouched required + // {{VDATE:}} is withheld here too, and the preview falls back to its + // example date instead of rendering an empty one (#1590). + const rows = await this.computePreview(this.collectAnswers()); + if (token !== this.previewToken || !this.previewContainerEl) return; + + this.previewContainerEl.empty(); + // A choice with nothing to preview (every Capture, Macro and Multi, and + // every Template using the default note-title prompt) rendered an empty + // tinted box containing the single word "Preview". The container is + // still created up front - its position between the title and the first + // field is load-bearing, and an async append would land it under the + // Submit row - so it collapses instead. + this.previewContainerEl.toggleClass("qa-hidden", rows.length === 0); + + for (const row of rows) { + const errors = row.diagnostics.filter((d) => d.severity === "error"); + // Same three-state vocabulary as the builder's row (#1594): a format + // that could not RESOLVE is showing raw text back, while a name the + // vault will refuse is an accurate preview of a note that will not + // exist. Unresolved wins when both are present. + const unresolved = errors.some((d) => d.kind !== "path"); + const label = unresolved + ? "Unresolved" + : errors.length > 0 + ? "Won't be created" + : row.label; + + const rowEl = this.previewContainerEl.createDiv({ cls: "qa-onepage-preview-row", }); - row.createEl("div", { - text: `${k}:`, - cls: "qa-preview-key", - }); + rowEl.createEl("div", { text: `${label}:`, cls: "qa-preview-key" }); // Clamped in CSS, with the whole string on the element: this block // sits ABOVE every input and re-renders as you type, and since #1563 // a file-name preview can be a whole included template joined onto // one line - unclamped it would push the fields and Submit down and // shift them under the caret. - const valueEl = row.createEl("div", { - text: String(v), + const valueEl = rowEl.createEl("div", { + text: row.text, cls: "qa-preview-val", }); - valueEl.setAttribute("title", String(v)); - }); + valueEl.setAttribute("title", row.text); + + for (const diagnostic of row.diagnostics) { + const issueEl = this.previewContainerEl.createDiv({ + cls: "qa-preview-issue", + }); + issueEl.toggleClass( + "qa-preview-issue--error", + diagnostic.severity === "error", + ); + // Severity in TEXT, not colour alone (WCAG 1.4.1), matching the + // builder's row. + issueEl.createSpan({ + cls: "qa-visually-hidden", + text: diagnostic.severity === "error" ? "Error: " : "Warning: ", + }); + issueEl.appendText(diagnostic.message); + issueEl.setAttribute("title", diagnostic.message); + } + } } catch { // Ignore preview errors } diff --git a/src/preflight/runOnePagePreflight.filenamePreview.test.ts b/src/preflight/runOnePagePreflight.filenamePreview.test.ts index 99be0038..1982b582 100644 --- a/src/preflight/runOnePagePreflight.filenamePreview.test.ts +++ b/src/preflight/runOnePagePreflight.filenamePreview.test.ts @@ -9,9 +9,15 @@ vi.mock("src/logger/logManager", () => ({ log: { logWarning: vi.fn(), logError: vi.fn(), logMessage: vi.fn() }, })); +type PreviewRow = { + label: string; + text: string; + diagnostics: readonly { severity: string; message: string; kind?: string }[]; +}; + /** Captures the `computePreview` callback the modal is constructed with. */ let computePreview: - | ((values: Record) => Promise>) + | ((values: Record) => Promise) | null = null; vi.mock("./OnePageInputModal", () => ({ @@ -20,9 +26,7 @@ vi.mock("./OnePageInputModal", () => ({ _app: unknown, _requirements: unknown, _variables: unknown, - preview: ( - values: Record, - ) => Promise>, + preview: (values: Record) => Promise, ) { computePreview = preview; } @@ -45,6 +49,9 @@ vi.mock("src/utilityObsidian", async () => { getMarkdownFilesWithTag: vi.fn(() => []), getUserScript: vi.fn(), isFolder: vi.fn(() => false), + // A configured folder can hold {{DATE:}}, which the requirement scan + // resolves through this helper. + getDate: ({ format }: { format: string }) => format, getTemplateFile: vi.fn((app: App, path: string) => { const f = app.vault.getAbstractFileByPath(path); return f instanceof TFileCls ? f : null; @@ -74,7 +81,10 @@ const createApp = (templates: Record = {}) => metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, }) as unknown as App; -const createChoice = (fileNameFormat: string): ITemplateChoice => +const createChoice = ( + fileNameFormat: string, + folder?: Partial, +): ITemplateChoice => ({ id: "tmpl", name: "Template Choice", @@ -88,6 +98,7 @@ const createChoice = (fileNameFormat: string): ITemplateChoice => chooseWhenCreatingNote: false, createInSameFolderAsActiveFile: false, chooseFromSubfolders: false, + ...folder, }, appendLink: false, openFile: false, @@ -141,8 +152,9 @@ describe("one-page preflight previews the file name with the file-name formatter // path); a separator, because that is what the run makes of it - // `normalizeGeneratedFilePath` rewrites "\" to "/" before the note is // created, exactly as Obsidian's own `normalizePath` does (#1563). - expect(out.fileName).toBe("Notes/name-My Note"); - expect(out.fileName).not.toContain("\n"); + expect(out[0].text).toBe("Notes/name-My Note"); + expect(out[0].text).not.toContain("\n"); + expect(out[0].label).toBe("File name"); }); it("resolves a {{TEMPLATE:}} include the way the run does", async () => { @@ -160,7 +172,7 @@ describe("one-page preflight previews the file name with the file-name formatter // The template file's trailing newline is not part of the name: the run's // normalizer collapses the control run and the space around it, so the // seam between the include and "-My Note" reads the same in both. - expect(out.fileName).toBe("Log-My Note -My Note"); + expect(out[0].text).toBe("Log-My Note -My Note"); }); it("says the run would abort when the include does not exist", async () => { @@ -172,6 +184,89 @@ describe("one-page preflight previews the file name with the file-name formatter ); const out = await computePreview!({ title: "My Note" }); - expect(out.fileName).toBe("[QuickAdd: template not found] Gone.md-My Note"); + expect(out[0].text).toBe("[QuickAdd: template not found] Gone.md-My Note"); + // The diagnostic used to be dropped on the floor here: computePreview + // returned strings and the modal rendered only those (#1590). + expect(out[0].diagnostics).toEqual([ + { severity: "error", message: "Template not found: Gone.md" }, + ]); + }); +}); + +describe("the one-page preview carries its problems and its target folder (#1590)", () => { + it("reports a name Obsidian will refuse, with the user's real answer in it", async () => { + await runOnePagePreflight( + createApp(), + createPlugin(), + createExecutor(), + createChoice("Bad: {{VALUE:title}}"), + ); + + const out = await computePreview!({ title: "My Note" }); + expect(out[0].text).toBe("Bad: My Note"); + expect(out[0].diagnostics).toEqual([ + { + severity: "error", + kind: "path", + message: + 'A file or folder name cannot contain ":", so this choice would fail at run time. Check your own text and tokens like {{TIME}}, which is HH:mm.', + }, + ]); + }); + + it("resolves {{FOLDER}} against the choice's single configured folder", async () => { + // Nothing called setTargetFolderPath, so this rendered `Notes//x` plus an + // empty-segment error nobody could see. + await runOnePagePreflight( + createApp(), + createPlugin(), + createExecutor(), + createChoice("Notes/{{FOLDER}}/{{VALUE:title}}", { + enabled: true, + folders: ["Work"], + }), + ); + + const out = await computePreview!({ title: "My Note" }); + expect(out[0].text).toBe("Notes/Work/My Note"); + expect(out[0].diagnostics).toEqual([]); + }); + + it("falls back to the builder's placeholder when the run has not picked a folder", async () => { + // Two configured folders means the run opens a suggester, so no folder can + // be promised - but an empty string would produce `Notes//x` and a false + // "empty path segment" error on a choice that works. + await runOnePagePreflight( + createApp(), + createPlugin(), + createExecutor(), + createChoice("Notes/{{FOLDER}}/{{VALUE:title}}", { + enabled: true, + folders: ["Work", "Personal"], + }), + ); + + const out = await computePreview!({ title: "My Note" }); + expect(out[0].text).toBe("Notes/Folder/Name/My Note"); + expect(out[0].diagnostics).toEqual([]); + }); + + it("does not splice a format token out of a configured folder into the name", async () => { + // The run formats the folder first (formatFolderPath); setTargetFolderPath + // does not. Handing over the raw text would put the literal token in the + // name AND raise the colon error from the token's own syntax. + await runOnePagePreflight( + createApp(), + createPlugin(), + createExecutor(), + createChoice("{{FOLDER}}/{{VALUE:title}}", { + enabled: true, + folders: ["Journal/{{DATE:YYYY-MM}}"], + }), + ); + + const out = await computePreview!({ title: "My Note" }); + expect(out[0].text).toBe("Folder/Name/My Note"); + expect(out[0].diagnostics).toEqual([]); }); }); diff --git a/src/preflight/runOnePagePreflight.ts b/src/preflight/runOnePagePreflight.ts index 78dd7972..49749be1 100644 --- a/src/preflight/runOnePagePreflight.ts +++ b/src/preflight/runOnePagePreflight.ts @@ -7,7 +7,8 @@ import type ITemplateChoice from "src/types/choices/ITemplateChoice"; import { VALUE_SYNTAX } from "src/constants"; import { MacroAbortError } from "src/errors/MacroAbortError"; import { log } from "src/logger/logManager"; -import { OnePageInputModal } from "./OnePageInputModal"; +import { OnePageInputModal, type PreviewRow } from "./OnePageInputModal"; +import { likelyTargetFolderPath } from "src/utils/previewTargetFolder"; import { canonicalizeOnePageFileValue, FILE_VARIABLE_PREFIX, @@ -116,7 +117,9 @@ export async function runOnePagePreflight( // Show modal // Optional live preview of a couple of key outputs (best-effort) - const computePreview = async (values: Record) => { + const computePreview = async ( + values: Record, + ): Promise => { try { // FileNameDisplayFormatter, not FormatDisplayFormatter: this previews // a FILE NAME. The content formatter expands `\n` escapes (not @@ -133,21 +136,37 @@ export async function runOnePagePreflight( // formatter has no inert stand-in for: inline `js quickadd` fences and // macros, which the run really does execute inside an included body. const formatter = new FileNameDisplayFormatter(app, plugin); - const out: Record = {}; + const out: PreviewRow[] = []; // File name preview for Template if (choice.type === "Template") { const tmpl = choice as ITemplateChoice; if (tmpl.fileNameFormat?.enabled) { + // {{FOLDER}} previewed nothing at all here: nobody called + // setTargetFolderPath, so `Notes/{{FOLDER}}/x` rendered `Notes//x` + // plus an empty-segment error that the modal then discarded + // (#1590). The builder's own neutral placeholder is the fallback + // when the run has not decided the folder yet. + formatter.setTargetFolderPath( + likelyTargetFolderPath(tmpl.folder) ?? "Folder/Name", + ); // Seed variables map-like into formatter for (const [k, v] of Object.entries(values)) { formatter["variables"].set(k, v); } - out.fileName = await formatter.format(tmpl.fileNameFormat.format); + out.push({ + label: "File name", + text: await formatter.format(tmpl.fileNameFormat.format), + // The channel #1558 added and #1563/#1578/#1588 fill with + // "this name would abort the run". This is the surface where + // it is worth the most: the user's REAL answers are seeded + // above, so the row shows the exact name about to fail. + diagnostics: formatter.diagnostics.list(), + }); } } return out; } catch { - return {}; + return []; } }; diff --git a/src/styles.css b/src/styles.css index 79406f92..b0c7e2c4 100644 --- a/src/styles.css +++ b/src/styles.css @@ -306,13 +306,11 @@ font-size: 0.9em; } -.quickAddModal .qa-onepage-preview-label, .quickAddModal .vdate-preview-label, .quickAddModal .qa-preview-key { font-weight: 600; } -.quickAddModal .qa-onepage-preview-label, .quickAddModal .vdate-preview-label { margin-bottom: 0.25rem; color: var(--text-muted); @@ -419,6 +417,14 @@ gap: 0.5rem; } +/* The one-page form's problems reuse .qa-preview-issue (already scoped to + .quickAddModal, which this modal carries) so the builder and the run surface + describe a refused name identically. They sit directly under their row rather + than in the builder's .qa-preview-issues grid, so the gap is set here. */ +.quickAddModal .qa-onepage-preview .qa-preview-issue { + margin-top: 0.25rem; +} + .quickAddModal .qa-vdate-input, .quickAddModal .qa-template-format-input, .quickAddModal .qa-folder-path-input { diff --git a/src/utils/generatedFilePath.ts b/src/utils/generatedFilePath.ts index 9f352afa..94459043 100644 --- a/src/utils/generatedFilePath.ts +++ b/src/utils/generatedFilePath.ts @@ -112,8 +112,29 @@ export function findIllegalFilePathChars(path: string): string[] { * wrong (same reason as `describeUnknownFieldFilter`, #1564). */ export function describeIllegalFilePathChars(chars: readonly string[]): string { - const quoted = chars.map((char) => `"${char}"`).join(", "); - return `A file or folder name cannot contain ${quoted}, so this choice would fail at run time. Check your own text and tokens like {{TIME}}, which is HH:mm.`; + return `A file or folder name cannot contain ${quoteChars(chars)}, so this choice would fail at run time. ${CHECK_YOUR_TOKENS}`; +} + +/** + * The same finding, at RUN time (#1591). + * + * Different tense and a concrete path, because by now it is not a warning about + * a hypothetical - the choice is stopping, and the name is known. The character + * set and the `{{TIME}}` hint are shared with the preview's sentence above so + * the two surfaces cannot describe the same rule differently. + */ +export function describeIllegalFilePathCharsForRun( + chars: readonly string[], + path: string, +): string { + return `Cannot create "${path}": a file or folder name cannot contain ${quoteChars(chars)}. ${CHECK_YOUR_TOKENS}`; +} + +const CHECK_YOUR_TOKENS = + "Check your own text and tokens like {{TIME}}, which is HH:mm."; + +function quoteChars(chars: readonly string[]): string { + return chars.map((char) => `"${char}"`).join(", "); } /** diff --git a/src/utils/previewTargetFolder.test.ts b/src/utils/previewTargetFolder.test.ts new file mode 100644 index 00000000..95038703 --- /dev/null +++ b/src/utils/previewTargetFolder.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from "vitest"; +import { likelyTargetFolderPath } from "./previewTargetFolder"; +import type { TemplateFolderConfig } from "../types/choices/ITemplateChoice"; + +const folder = ( + overrides: Partial = {}, +): TemplateFolderConfig => ({ + enabled: true, + folders: ["Work"], + chooseWhenCreatingNote: false, + createInSameFolderAsActiveFile: false, + chooseFromSubfolders: false, + ...overrides, +}); + +/** + * What a PREVIEW may resolve {{FOLDER}} against. Anything this returns is + * spliced into the previewed name verbatim, so every `undefined` below is a + * case where naming a folder would have been a lie - and, because every + * argument-bearing token carries a colon in its own syntax, usually a red + * illegal-character row on a choice that works (#1590). + */ +describe("likelyTargetFolderPath", () => { + it("names a single configured folder", () => { + expect(likelyTargetFolderPath(folder())).toBe("Work"); + }); + + it("declines when the run will open a folder chooser", () => { + expect(likelyTargetFolderPath(folder({ folders: ["Work", "Home"] }))).toBeUndefined(); + expect( + likelyTargetFolderPath(folder({ chooseWhenCreatingNote: true })), + ).toBeUndefined(); + expect( + likelyTargetFolderPath(folder({ chooseFromSubfolders: true })), + ).toBeUndefined(); + expect( + likelyTargetFolderPath(folder({ createInSameFolderAsActiveFile: true })), + ).toBeUndefined(); + }); + + it("declines when QuickAdd's folder setting is off", () => { + // Obsidian's own "default location for new notes" decides it. + expect(likelyTargetFolderPath(folder({ enabled: false }))).toBeUndefined(); + expect(likelyTargetFolderPath(undefined)).toBeUndefined(); + }); + + it("declines a folder holding format syntax, which the run resolves first", () => { + expect( + likelyTargetFolderPath(folder({ folders: ["Journal/{{DATE:YYYY-MM}}"] })), + ).toBeUndefined(); + }); + + it("declines a folder holding an inline js quickadd fence", () => { + // formatFolderPath EXECUTES it (replaceInlineJavascriptInString is + // format()'s first pass), and the folder validator lets backticks + // through - so the raw source would otherwise be spliced into the name, + // which the preview must never do (#1558). + expect( + likelyTargetFolderPath( + folder({ folders: ['```js quickadd\nreturn "Work";\n```'] }), + ), + ).toBeUndefined(); + }); + + it("declines an empty or whitespace-only folder", () => { + expect(likelyTargetFolderPath(folder({ folders: [""] }))).toBeUndefined(); + expect(likelyTargetFolderPath(folder({ folders: [" "] }))).toBeUndefined(); + }); +}); diff --git a/src/utils/previewTargetFolder.ts b/src/utils/previewTargetFolder.ts new file mode 100644 index 00000000..e88a941b --- /dev/null +++ b/src/utils/previewTargetFolder.ts @@ -0,0 +1,56 @@ +import { INLINE_JAVASCRIPT_REGEX } from "../constants"; +import type { TemplateFolderConfig } from "../types/choices/ITemplateChoice"; + +/** + * The folder a PREVIEW should resolve `{{FOLDER}}` against, or `undefined` when + * it cannot honestly name one. + * + * `{{FOLDER}}` is substituted verbatim from `Formatter.targetFolderPath`, so + * whatever is passed here lands in the previewed name unchanged. Two things + * therefore have to be true before a configured folder can be used: + * + * 1. **The run must have already decided it.** `TemplateChoiceEngine` opens a + * folder suggester whenever more than one destination is in play - several + * configured folders, "ask each time", subfolders, or "same folder as the + * current file". Only a single plain folder is knowable up front. + * 2. **It must be a literal.** The run formats the configured folder first + * (`getFolderPath` -> `CompleteFormatter.formatFolderPath`), while + * `setTargetFolderPath` only trims slashes - so handing over a raw + * `Journal/{{DATE:YYYY-MM}}` would splice the literal token into the name + * AND, because every argument-bearing token carries a colon in its own + * syntax, raise the illegal-character error (#1578) against a choice that + * works. The same goes for an inline `js quickadd` fence, which + * `formatFolderPath` really does EXECUTE (it is `format()`'s first pass) and + * which the folder validator permits, because backticks are not among the + * characters it rejects - and which the preview must never run (#1558). + * Resolving the folder through a second nested formatter pass is not worth + * that: `undefined` falls back to the caller's neutral placeholder, which is + * what the builder has always shown here. + * + * KNOWN RESIDUAL, not fixed here: even for a single configured folder the run + * can still prompt, because `TemplateChoiceEngine` adds a `` + * row to the suggester, so answering it can produce a different folder than the + * one previewed. That is a run-side defect with its own issue; the previewed + * folder is nonetheless strictly closer to the truth than the placeholder it + * replaces. + */ +export function likelyTargetFolderPath( + folder: TemplateFolderConfig | undefined, +): string | undefined { + if (!folder?.enabled) return undefined; + if ( + folder.chooseWhenCreatingNote || + folder.chooseFromSubfolders || + folder.createInSameFolderAsActiveFile + ) { + return undefined; + } + const folders = folder.folders ?? []; + if (folders.length !== 1) return undefined; + + const only = folders[0]?.trim(); + if (!only) return undefined; + if (only.includes("{{")) return undefined; + if (INLINE_JAVASCRIPT_REGEX.test(only)) return undefined; + return only; +} diff --git a/src/utils/vdateSyntax.ts b/src/utils/vdateSyntax.ts index 0c1397ec..61969aea 100644 --- a/src/utils/vdateSyntax.ts +++ b/src/utils/vdateSyntax.ts @@ -87,3 +87,36 @@ export function parseVDateOptions( snap, }; } + +/** + * {@link parseVDateOptions} for a PREVIEW, which evaluates INCOMPLETE input on + * every keystroke. + * + * `|startof:` throws out of `normalizeDateUnit` for a unit that does not + * resolve, and half of a unit usually does not (`we`, `mont`) - so in the run + * that throw is the right answer, and in a preview it would blank the row + * between two keystrokes, which is the noise #1558 removed. Note that some + * short forms ARE valid aliases (`w`, `d`, `M`), so "unfinished" is not a + * property of the string. + * + * The two channels are therefore split. The OPTIONS come back usable either + * way, so the preview TEXT stays stable while you type; the ERROR comes back + * separately for the caller to put on the diagnostics channel, which the + * builder holds back until the field has been still for 500ms + * (`DIAGNOSTICS_IDLE_MS`). Swallowing it outright would have deleted a + * diagnostic the body preview already shipped - and left a format the run + * cannot execute previewing as a finished date. + */ +export function parseVDateOptionsForPreview(rawOptions: string | undefined | null): { + options: ParsedVDateOptions; + error?: string; +} { + try { + return { options: parseVDateOptions(rawOptions) }; + } catch (error) { + return { + options: { optional: false, withTime: false }, + error: error instanceof Error ? error.message : String(error), + }; + } +}