From 0edcdc1f59fd2632847bff77000acb6861bd14d7 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 17:30:45 +0200 Subject: [PATCH 01/10] fix(preview): resolve {{MVALUE}}, and stop discarding the collected answer The file-name and format previews left {{MVALUE}} literal while the run opened the math modal for it. Both display formatters already overrode promptForMathValue with a "calculation_result" stand-in that nothing could reach - a dead override is the tell that the pass was meant to be there. Positioned where CompleteFormatter.format has it: after {{FILE:}}, before {{RANDOM:}}. Also closes the loop the preview exposed. RequirementCollector registers a requirement keyed "mvalue" for this token, so the one-page input form asks for a "Math expression" and the CLI prints missingFlags: ["value-mvalue="] - and nothing read either answer, so the form asked and then the run opened the modal on top of it, and passing the CLI's own recommended flag changed nothing. replaceMathValueInString now reads the collected value first, the way {{VALUE}} always has. READ, never write: the prompt's answer stays unstored, so {{MVALUE}} {{MVALUE}} is still two questions. The non-interactive abort also named "a {{MATH}} expression", a token QuickAdd has never had (MATH_VALUE_REGEX is /{{MVALUE}}/i) - in the one message whose job is to say which flag to pass. Closes #1587. Closes #1607. --- src/formatters/completeFormatter.test.ts | 35 +++++++++++++++++++ src/formatters/completeFormatter.ts | 5 ++- .../fileNameDisplayFormatter.test.ts | 34 ++++++++++++------ src/formatters/fileNameDisplayFormatter.ts | 6 ++++ src/formatters/formatDisplayFormatter.ts | 4 +++ src/formatters/formatter.ts | 18 +++++++++- 6 files changed, 89 insertions(+), 13 deletions(-) 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..97430abd 100644 --- a/src/formatters/completeFormatter.ts +++ b/src/formatters/completeFormatter.ts @@ -791,7 +791,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.test.ts b/src/formatters/fileNameDisplayFormatter.test.ts index 5f46f568..d188da7a 100644 --- a/src/formatters/fileNameDisplayFormatter.test.ts +++ b/src/formatters/fileNameDisplayFormatter.test.ts @@ -181,22 +181,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. diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index d73c41c2..d063b4bd 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -276,6 +276,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. diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index b54cb629..f168649e 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -124,6 +124,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; diff --git a/src/formatters/formatter.ts b/src/formatters/formatter.ts index 3217dc38..24d821ff 100644 --- a/src/formatters/formatter.ts +++ b/src/formatters/formatter.ts @@ -1371,13 +1371,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; } From 4ab65307370f475c2740b05cee19812ff265a48a Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 17:37:08 +0200 Subject: [PATCH 02/10] fix(preview): give {{VDATE:}} the run's default format, and its answered value Two ways the preview disagreed with the run about the same token. 1. A format-less {{VDATE:due}} previewed as the raw token, while the run supplies YYYY-MM-DD (or YYYY-MM-DD HH:mm under |time) and creates the note. The bail-out narrows from "no name OR no format" to "no name", which is the run's own rule. That token's own text contains a colon, so the illegal-character diagnostic added in #1595 was firing on it: a working {{VDATE:due}} showed a red "a file or folder name cannot contain :" under a choice that creates 2026-07-27.md. Rendering the date deletes the false accusation and, on {{VDATE:due|time}}, lets the true one appear - the run's datetime default really does put HH:mm in the file name, and Obsidian really does refuse it. 2. Neither preview read an ANSWERED date. The one-page input form seeds the user's real picks into the formatter before computing the preview, and VDATE was the only seeded requirement type ignored - so the row showed today beside the date just chosen. Both formatters now resolve a stored value through renderStoredDateVariable, extracted from the run's own tail so there is one implementation and not three. Answered-empty ("") stays empty; an unparseable seeded string keeps the verbatim back-compat branch; only `undefined` falls through to the example date. Both previews also stop throwing on a half-typed |startof: unit, which blanked the row and reddened it between two keystrokes (#1558's failure mode). Closes #1589. --- ...NameDisplayFormatter.audit-cleanup.test.ts | 67 ++++++- src/formatters/fileNameDisplayFormatter.ts | 36 +++- .../formatDisplayFormatter-1589-vdate.test.ts | 95 ++++++++++ src/formatters/formatDisplayFormatter.ts | 39 +++- src/formatters/formatter.ts | 169 ++++++++++++------ src/utils/vdateSyntax.ts | 22 +++ 6 files changed, 354 insertions(+), 74 deletions(-) create mode 100644 src/formatters/formatDisplayFormatter-1589-vdate.test.ts diff --git a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts index af36a9cd..e88aaa0c 100644 --- a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts +++ b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts @@ -103,8 +103,69 @@ 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", + "@date:2026-08-15T00:00:00.000Z", + ); + 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 () => { + // Every prefix of "week" throws out of normalizeDateUnit, and this runs on + // every keystroke (#1558). + const formatter = makeFormatter(); + await expect( + formatter.format("{{VDATE:wk,YYYY-MM-DD|startof:w}}"), + ).resolves.toBe("2023-06-01"); + expect(formatter.diagnostics.list()).toEqual([]); }); }); diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index d063b4bd..ce01e3cb 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -1,9 +1,12 @@ import { + defaultDateVariableFormat, findInlineScriptSpans, Formatter, hasUnterminatedInlineScriptFence, + renderStoredDateVariable, type PromptContext, } from "./formatter"; +import { parseVDateOptionsForPreview } from "../utils/vdateSyntax"; import { describePreviewFailure, PreviewDiagnostics, @@ -545,15 +548,38 @@ 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 { withTime, snap } = parseVDateOptionsForPreview(rawOptions); + 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. Like the + // body preview, this renders 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. const previewDate = new Date(); try { return DateFormatPreviewGenerator.generate(cleanDateFormat, previewDate); diff --git a/src/formatters/formatDisplayFormatter-1589-vdate.test.ts b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts new file mode 100644 index 00000000..551b9e47 --- /dev/null +++ b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts @@ -0,0 +1,95 @@ +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", + "@date:2026-08-15T00:00:00.000Z", + ); + await expect(formatter.format("{{VDATE:due}}")).resolves.toBe("2026-08-15"); + }); + + it("does not blank the row for a half-typed |startof: unit", async () => { + // parseVDateOptions throws on an unfinished unit, and every prefix of + // "week" is one. Before this the whole body preview went blank and red + // between two keystrokes (#1558's failure mode). + const formatter = makeFormatter(); + await expect( + formatter.format("{{VDATE:wk,YYYY-MM-DD|startof:w}}"), + ).resolves.toBe("2023-06-01"); + expect(formatter.diagnostics.list()).toEqual([]); + }); +}); diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index f168649e..5da4a5c6 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,7 @@ import { DateFormatPreviewGenerator } from "./helpers/previewHelpers"; import { getValueVariableBaseName } from "../utils/valueSyntax"; -import { parseVDateOptions } from "../utils/vdateSyntax"; +import { parseVDateOptionsForPreview } from "../utils/vdateSyntax"; import { EnhancedFieldSuggestionFileFilter } from "../utils/EnhancedFieldSuggestionFileFilter"; import { FILE_CUSTOM_PREFIX, FILE_PICK_PREFIX, type ParsedFileToken } from "../utils/fileSyntax"; @@ -310,18 +315,36 @@ 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 { + defaultValue: cleanDefaultValue, + optional, + withTime, + snap, + } = parseVDateOptionsForPreview(rawOptions); + // 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 } + // 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 const previewDate = new Date(); let formattedExample: string; - + try { // Try to generate a realistic preview using the format formattedExample = DateFormatPreviewGenerator.generate(cleanDateFormat, previewDate); @@ -329,7 +352,7 @@ export class FormatDisplayFormatter extends Formatter { // 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 24d821ff..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(); @@ -1510,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); @@ -1558,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/utils/vdateSyntax.ts b/src/utils/vdateSyntax.ts index 0c1397ec..8526f9ff 100644 --- a/src/utils/vdateSyntax.ts +++ b/src/utils/vdateSyntax.ts @@ -87,3 +87,25 @@ 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 is not + * finished being typed, and every prefix of `week` is one of those. In the run + * that throw is the right answer - the token is wrong and the choice should + * stop. In a preview it blanks the row and turns it red between two keystrokes, + * which is the per-keystroke noise #1558 removed. The options are simply not + * known yet, so the preview renders as though none were given and picks the + * real ones up as soon as the token parses. + */ +export function parseVDateOptionsForPreview( + rawOptions: string | undefined | null, +): ParsedVDateOptions { + try { + return parseVDateOptions(rawOptions); + } catch { + return { optional: false, withTime: false }; + } +} From 0cd531ce4d9164b749913a5be5d33e58a73a5d33 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 17:41:23 +0200 Subject: [PATCH 03/10] fix(preview): say so when a file-name format uses {{title}} `{{title}} note` previewed as an ordinary name in ordinary styling, and the run created nothing: formatFileName rejects the token outright, because the title IS the file name. The preview now mirrors both of the run's checks - on the raw format string, and on format()'s output, where an expanded {{GLOBAL_VAR:}} snippet or a {{VALUE}} resolving to the literal text can smuggle one in. The token keeps previewing literally rather than as an example title: it is genuinely unresolved on screen, so the row's "Unresolved:" label is true, and the new diagnostic is what says it will never resolve. The output-side check needed one correction first, or it would have turned a WORKING choice red: a {{title}} inside a spliced-in {{TEMPLATE:}} body is not a failure. At run time that body goes through the child engine's formatFileContent, which resolves the token to the stored title (or "") before formatFileName ever sees the name. The preview now does the same, at the include seam - not through the shared token resolver, which would have invented "My Document Title" for an unstored variable. `/\{\{title\}\}/i` also stopped being copy-pasted: six inlined copies in CompleteFormatter and the new preview check all read TITLE_REGEX, so the rule cannot drift between the surface that warns and the code that throws. Closes #1588. --- src/formatters/completeFormatter.ts | 21 ++++-- .../fileNameDisplayFormatter.test.ts | 52 ++++++++++++++- src/formatters/fileNameDisplayFormatter.ts | 64 ++++++++++++++++++- 3 files changed, 125 insertions(+), 12 deletions(-) diff --git a/src/formatters/completeFormatter.ts b/src/formatters/completeFormatter.ts index 97430abd..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.", ); diff --git a/src/formatters/fileNameDisplayFormatter.test.ts b/src/formatters/fileNameDisplayFormatter.test.ts index d188da7a..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 = { @@ -219,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 ce01e3cb..4ad631de 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -12,7 +12,7 @@ import { PreviewDiagnostics, } from "./previewDiagnostics"; import type { App } from "obsidian"; -import { DATE_VARIABLE_REGEX, GLOBAL_VAR_REGEX } from "../constants"; +import { DATE_VARIABLE_REGEX, GLOBAL_VAR_REGEX, TITLE_REGEX } from "../constants"; import type { IDateParser } from "../parsers/IDateParser"; import { NLDParser } from "../parsers/NLDParser"; import { @@ -144,6 +144,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 @@ -169,6 +174,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). * @@ -464,7 +496,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) { @@ -478,6 +512,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 From 19332b4b899338334cebe2794d1bb7615a849cb5 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 17:44:45 +0200 Subject: [PATCH 04/10] fix(preview): stop labelling a resolved name "Unresolved:" `Bad: {{VALUE:title}}` rendered as "Unresolved: Bad: Example Title". Every token resolved; nothing was unresolved. The label sent the reader hunting for a broken token when the problem was the finished name. Two error classes shared one label, and the split already existed: #1582 added `kind: "path"` for the capture target's hideWhen gate, defined as "the name came out fine, the vault will just not accept it". Reusing that axis rather than adding a parallel one keeps a single classification that cannot disagree with itself. The row now reads Preview: / Won't be created: / Unresolved:, with unresolved winning when both are present - a missing template beside an empty path segment is fundamentally a failure to resolve. The existence probe that suppresses the illegal-character error was shape-blind in both directions, which put a wrong label on two real cases. Obsidian's path map holds no trailing slash, so a capture target of `Meetings: 2026/` - naming a FOLDER to pick inside, which exists - matched neither probe and went red; while a file-name format of `Meetings: 2026` was excused by a folder of that name although the run still cannot create `Meetings: 2026.md`. The probe now requires a TFolder for the trailing-slash shape and a TFile otherwise. FormatPreviewField.test.ts pins all three labels, including the both-classes case; the illegal-character test pins all four probe shapes. Closes #1594. --- ...isplayFormatter-1578-illegal-chars.test.ts | 44 ++++++++++-- src/formatters/fileNameDisplayFormatter.ts | 35 +++++++-- .../components/FormatPreviewField.svelte | 29 ++++++-- .../components/FormatPreviewField.test.ts | 71 +++++++++++++++++++ 4 files changed, 163 insertions(+), 16 deletions(-) diff --git a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts index d3fc7c56..92d040a1 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,34 @@ 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("still reports a colon when only a FOLDER of that name exists", async () => { + // The other direction: a file-name format of `Meetings: 2026` produces + // `Meetings: 2026.md`, which the run cannot create - a same-named folder + // must not excuse it. + existingFolders.add("Meetings: 2026"); + 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.ts b/src/formatters/fileNameDisplayFormatter.ts index 4ad631de..b956b2b2 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -12,6 +12,7 @@ import { PreviewDiagnostics, } from "./previewDiagnostics"; import type { App } from "obsidian"; +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"; @@ -254,10 +255,25 @@ 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, in both directions. Obsidian's path map holds no trailing + * slash, so a bare `getAbstractFileByPath` was wrong twice over: + * + * - 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 meant neither probe matched and the row went red; + * - a file-name format of `Meetings: 2026` was silently excused by a FOLDER + * of that name, although the run would still fail creating + * `Meetings: 2026.md`. + * + * Accepted residual: with "create file if it doesn't exist" on, a + * folder-shaped capture target can still create a new note inside a + * colon-bearing folder, which Obsidian's per-segment check refuses. The + * preview cannot know the name that will be picked. */ private existsInVault(name: string): boolean { const vault = this.app?.vault; @@ -267,9 +283,14 @@ 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("/")) { + const folder = vault.getAbstractFileByPath(trimmed.slice(0, -1)); + return folder instanceof TFolder; + } + return ( + vault.getAbstractFileByPath(trimmed) instanceof TFile || + vault.getAbstractFileByPath(`${trimmed}.md`) instanceof TFile ); } diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index ea49ed49..78a4fb34 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -93,12 +93,26 @@ 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. +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 +194,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..0507d628 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts @@ -179,6 +179,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 From f9b403e2c6901a86edc7ccef7fbed0ab6967da9b Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 17:50:55 +0200 Subject: [PATCH 05/10] fix(preview): give the one-page form its diagnostics and its target folder The one-page input form previews the note's file name while you fill it in, using the same FileNameDisplayFormatter the builder uses - and wired up two fewer things, so the same format previewed differently in the two places. Every diagnostic was thrown away. computePreview returned Record and the modal rendered only those strings, so a name Obsidian will refuse was presented in ordinary styling with nothing said. This is the surface where the message is worth the most: the user's REAL answers are seeded into the formatter, so the row shows the exact name that is about to fail. It is now an ordered list of rows carrying label, text and diagnostics, rendered with the builder's own .qa-preview-issue styling and its three-state label vocabulary. setTargetFolderPath was never called, so `Notes/{{FOLDER}}/x` previewed `Notes//x` plus an empty-segment error nobody could see. It now resolves against the choice's folder when the run has already decided it - a single configured folder, no chooser - and falls back to the builder's neutral placeholder otherwise. Deliberately NOT the raw configured folder in every case: the run formats it first (formatFolderPath) while setTargetFolderPath does not, so a folder of `Journal/{{DATE:YYYY-MM}}` would splice the literal token into the name and, since every argument-bearing token carries a colon, raise the illegal-character error against a choice that works. The builder's row takes the same folder, so the two surfaces agree. Three things found while wiring it up: - The row's key was the raw record key, so it read `fileName:`. - The first pass ran before renderField populated the value map, flashing a stand-in over prefilled answers. - updatePreviews had no generation guard; the 150ms debounce orders the STARTS of these passes, not their completions, and one can read up to 25 templates. Text and problems now commit under one monotonic token, as in the builder. The empty tinted box reading "Preview" is gone too: it was rendered for every Capture, Macro and Multi choice, and every Template using the default note-title prompt, none of which produce a row. Closes #1590. --- .../ChoiceBuilder/TemplateChoiceForm.svelte | 2 + src/preflight/OnePageInputModal.test.ts | 161 +++++++++++++++++- src/preflight/OnePageInputModal.ts | 109 +++++++++--- ...unOnePagePreflight.filenamePreview.test.ts | 113 +++++++++++- src/preflight/runOnePagePreflight.ts | 29 +++- src/styles.css | 10 +- src/utils/previewTargetFolder.ts | 50 ++++++ 7 files changed, 433 insertions(+), 41 deletions(-) create mode 100644 src/utils/previewTargetFolder.ts 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/preflight/OnePageInputModal.test.ts b/src/preflight/OnePageInputModal.test.ts index d4be308b..bed12c5a 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,149 @@ 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); + 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("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..6893afbc 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) @@ -702,34 +724,73 @@ 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({ + const rows = await this.computePreview(values); + 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/previewTargetFolder.ts b/src/utils/previewTargetFolder.ts new file mode 100644 index 00000000..dfa4460c --- /dev/null +++ b/src/utils/previewTargetFolder.ts @@ -0,0 +1,50 @@ +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 contain no format tokens.** 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. 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; + return only; +} From 731f8c3c7fc6d117efa1b82fdfa79a4f9ccac8a2 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 17:57:42 +0200 Subject: [PATCH 06/10] fix: refuse an impossible file name before running the template body A Template choice whose name resolves to something Obsidian refuses did not fail when the name was computed. It failed at vault.create - by which point QuickAdd had already created the target folder and formatted the ENTIRE template body: real {{VALUE}} prompts, macros, and inline `js quickadd` fences with whatever side effects those scripts have. The user answered the whole prompt chain, their scripts ran, and then the choice died. Measured in this worktree's isolated vault (Obsidian 1.13.0), a Template choice named `Bad: {{VALUE:title}}` whose template contains an inline script: before: ok:false "Choice execution failed; no file was created." folderExists: true, inlineScriptRuns: 1 after: ok:false "Cannot create \"Repro1591Folder/Bad: My Note.md\": a file or folder name cannot contain \":\". ..." folderExists: false, inlineScriptRuns: 0 The rule is "reject only if the file does not already exist": ":" is legal on macOS/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. Each guard therefore sits on a branch where non-existence is already established, so it needs no probe of its own: - TemplateEngine.createFileWithTemplate, first statement. Its two production callers are TemplateChoiceEngine's `else` branch of vault.adapter.exists and its freshly incremented collision name. - CaptureChoiceEngine, gated character-for-character on the dispatch condition that routes to onCreateFileIfItDoesntExist - placed above the heading picker, so the user is not asked to choose a heading for a note that cannot be created - and re-asserted at that sink so the invariant is local to it. - The "move note to match choice settings?" offer, which creates the target folder and then fails at renameFile. It declines quietly: the offer is an optional convenience, and the choice's own preview explains the problem. 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 character set and the {{TIME}} hint come from generatedFilePath.ts, which the preview also reads, so the row that warns and the code that stops cannot drift. Only the tense differs. Closes #1591. --- .../CaptureChoiceEngine.selection.test.ts | 78 +++++++++++- src/engine/CaptureChoiceEngine.ts | 18 +++ .../TemplateEngine-1591-refuse-name.test.ts | 111 ++++++++++++++++++ src/engine/TemplateEngine.ts | 17 +++ src/engine/applyTemplateToActiveNote.ts | 7 ++ src/engine/assertCreatableFilePath.test.ts | 56 +++++++++ src/engine/assertCreatableFilePath.ts | 47 ++++++++ src/utils/generatedFilePath.ts | 25 +++- 8 files changed, 355 insertions(+), 4 deletions(-) create mode 100644 src/engine/TemplateEngine-1591-refuse-name.test.ts create mode 100644 src/engine/assertCreatableFilePath.test.ts create mode 100644 src/engine/assertCreatableFilePath.ts diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 26f1bf85..051cd938 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1256,6 +1256,76 @@ 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(); + // No prompt was opened: the queued response is still queued. + expect(promptResponses).toHaveLength(0); + }); + + 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 +1344,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 +1363,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.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/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(", "); } /** From 9963e61c12af90066730b0ce6e0a8f750c034f51 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 18:45:24 +0200 Subject: [PATCH 07/10] fix(preview): fold in the adversarial review of this branch Five things the review caught, each measured. The one-page form previewed an untouched required {{VDATE:}} as answered-empty. submit() deliberately OMITS a required blank date so the sequential date prompt still fires, while updatePreviews copied this.result wholesale - so the row showed a name the run would never create. Both now go through one collectAnswers(), which is where the rule already lived (it needs dateParseErrors, which only the modal has). parseVDateOptionsForPreview swallowed the error as well as the throw. Master's body preview reported `Unknown date unit "we"` for a permanent typo; this branch rendered a finished date and said nothing, for a format the run aborts on - while {{DATE:...|startof:we}} in the same pass still reported it, so the row contradicted itself. The options and the error are now separate channels: the text stays stable while the unit is half-typed, the complaint goes on the diagnostics channel the builder already holds back for 500ms. The example date now applies |startof:/|endof: too. #1595 left snap out 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 an asymmetry ten lines wide, because the answered branch beside it snaps and {{DATE:...|startof:month}} in the same pass always has. Only a token that asks for a snap touches moment, so a dateless preview keeps its old path. existsInVault's bare-name branch is no longer TFile-only. That tightening reddened a working capture target: captureTargetResolution resolves a bare extension-less name to a folder SCOPE when a folder of that name exists with no note beside it, and the picker then writes to a file inside it. The probe mirrors that rule instead of approximating it, and the trailing-slash case it was contradicting stays fixed. Three of the new tests were vacuous, proved by mutation: - |startof:w does not throw (w is a real alias for week), so neither parseVDateOptionsForPreview test entered the catch. Now |startof:we, plus a case pinning that a valid short alias stays quiet. - The capture guard's test passed with the guard deleted. Its placement is what matters, so it now asserts the heading picker never opened - that fails when the guard moves to the sink. - The qa-hidden assertion only checked the true half, which the constructor already satisfies. Adds the missing coverage the review named: the body preview's {{MVALUE}} pass (deleting it left the suite green), and the first-pass ordering in the one-page modal. --- .../CaptureChoiceEngine.selection.test.ts | 46 +++++++++++- ...isplayFormatter-1578-illegal-chars.test.ts | 18 +++-- ...NameDisplayFormatter.audit-cleanup.test.ts | 47 +++++++++---- src/formatters/fileNameDisplayFormatter.ts | 70 ++++++++++++------- ...formatDisplayFormatter-1587-mvalue.test.ts | 57 +++++++++++++++ .../formatDisplayFormatter-1589-vdate.test.ts | 25 +++++-- src/formatters/formatDisplayFormatter.ts | 19 +++-- src/formatters/helpers/snappedExampleDate.ts | 23 ++++++ src/preflight/OnePageInputModal.test.ts | 62 ++++++++++++++++ src/preflight/OnePageInputModal.ts | 44 ++++++++---- src/utils/vdateSyntax.ts | 37 ++++++---- 11 files changed, 367 insertions(+), 81 deletions(-) create mode 100644 src/formatters/formatDisplayFormatter-1587-mvalue.test.ts create mode 100644 src/formatters/helpers/snappedExampleDate.ts diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 051cd938..2096e4fb 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1286,8 +1286,50 @@ describe("CaptureChoiceEngine capture target resolution", () => { await engine.run(); expect(createFileWithInput).not.toHaveBeenCalled(); - // No prompt was opened: the queued response is still queued. - expect(promptResponses).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 () => { diff --git a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts index 92d040a1..5d8cfce5 100644 --- a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts @@ -249,12 +249,22 @@ describe("the file-name preview does not cry wolf", () => { expect(diagnostics).toEqual([]); }); - it("still reports a colon when only a FOLDER of that name exists", async () => { - // The other direction: a file-name format of `Meetings: 2026` produces - // `Meetings: 2026.md`, which the run cannot create - a same-named folder - // must not excuse it. + 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 }, ]); diff --git a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts index e88aaa0c..c0f83678 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([]); }); @@ -160,12 +163,28 @@ describe("FileNameDisplayFormatter VDATE preview", () => { }); it("does not blank the row for a half-typed |startof: unit", async () => { - // Every prefix of "week" throws out of normalizeDateUnit, and this runs on - // every keystroke (#1558). + // "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:w}}"), + 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.ts b/src/formatters/fileNameDisplayFormatter.ts index b956b2b2..5839b5e6 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -7,6 +7,7 @@ import { type PromptContext, } from "./formatter"; import { parseVDateOptionsForPreview } from "../utils/vdateSyntax"; +import { snappedExampleDate } from "./helpers/snappedExampleDate"; import { describePreviewFailure, PreviewDiagnostics, @@ -260,20 +261,24 @@ export class FileNameDisplayFormatter extends Formatter { * engine appends `.md` (`normalizeMarkdownFilePath`), while a capture target * usually carries one already. * - * SHAPE-AWARE, in both directions. Obsidian's path map holds no trailing - * slash, so a bare `getAbstractFileByPath` was wrong twice over: + * 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 meant neither probe matched and the row went red; - * - a file-name format of `Meetings: 2026` was silently excused by a FOLDER - * of that name, although the run would still fail creating - * `Meetings: 2026.md`. + * 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: with "create file if it doesn't exist" on, a - * folder-shaped capture target can still create a new note inside a - * colon-bearing folder, which Obsidian's per-segment check refuses. The - * preview cannot know the name that will be picked. + * 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; @@ -285,13 +290,18 @@ export class FileNameDisplayFormatter extends Formatter { if (!trimmed) return false; if (trimmed.endsWith("/")) { - const folder = vault.getAbstractFileByPath(trimmed.slice(0, -1)); - return folder instanceof TFolder; + return ( + vault.getAbstractFileByPath(trimmed.slice(0, -1)) instanceof TFolder + ); } - return ( - vault.getAbstractFileByPath(trimmed) instanceof TFile || - vault.getAbstractFileByPath(`${trimmed}.md`) instanceof TFile - ); + + 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; } /** @@ -640,7 +650,13 @@ export class FileNameDisplayFormatter extends Formatter { return match; } - const { withTime, snap } = parseVDateOptionsForPreview(rawOptions); + 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); @@ -656,14 +672,20 @@ export class FileNameDisplayFormatter extends Formatter { ); if (stored) return stored.text; - // Nothing answered: a realistic example from the current date. Like the - // body preview, this renders 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. - const previewDate = new Date(); + // 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 index 551b9e47..4e769303 100644 --- a/src/formatters/formatDisplayFormatter-1589-vdate.test.ts +++ b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts @@ -82,14 +82,27 @@ describe("FormatDisplayFormatter VDATE default format (#1589)", () => { await expect(formatter.format("{{VDATE:due}}")).resolves.toBe("2026-08-15"); }); - it("does not blank the row for a half-typed |startof: unit", async () => { - // parseVDateOptions throws on an unfinished unit, and every prefix of - // "week" is one. Before this the whole body preview went blank and red - // between two keystrokes (#1558's failure mode). + 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:w}}"), + 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"); - expect(formatter.diagnostics.list()).toEqual([]); }); }); diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index 5da4a5c6..3dc3729b 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -28,6 +28,7 @@ import { } from "./helpers/previewHelpers"; import { getValueVariableBaseName } from "../utils/valueSyntax"; 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"; @@ -315,12 +316,17 @@ 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 { 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, - } = parseVDateOptionsForPreview(rawOptions); + } = 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). @@ -341,13 +347,18 @@ export class FormatDisplayFormatter extends Formatter { ); if (stored) return stored.text; - // Generate a preview using current date with the specified format - const previewDate = new Date(); + // 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]`; 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/preflight/OnePageInputModal.test.ts b/src/preflight/OnePageInputModal.test.ts index bed12c5a..0077b8c4 100644 --- a/src/preflight/OnePageInputModal.test.ts +++ b/src/preflight/OnePageInputModal.test.ts @@ -925,6 +925,8 @@ describe("OnePageInputModal preview block (#1590)", () => { 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", ); @@ -994,6 +996,66 @@ describe("OnePageInputModal preview block (#1590)", () => { 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. diff --git a/src/preflight/OnePageInputModal.ts b/src/preflight/OnePageInputModal.ts index 6893afbc..33623e05 100644 --- a/src/preflight/OnePageInputModal.ts +++ b/src/preflight/OnePageInputModal.ts @@ -650,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 @@ -680,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; @@ -700,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() { @@ -731,9 +745,11 @@ export class OnePageInputModal extends Modal { // complaint. const token = ++this.previewToken; try { - const values: Record = {}; - this.result.forEach((v, k) => (values[k] = v)); - const rows = await this.computePreview(values); + // 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(); diff --git a/src/utils/vdateSyntax.ts b/src/utils/vdateSyntax.ts index 8526f9ff..61969aea 100644 --- a/src/utils/vdateSyntax.ts +++ b/src/utils/vdateSyntax.ts @@ -92,20 +92,31 @@ export function parseVDateOptions( * {@link parseVDateOptions} for a PREVIEW, which evaluates INCOMPLETE input on * every keystroke. * - * `|startof:` throws out of `normalizeDateUnit` for a unit that is not - * finished being typed, and every prefix of `week` is one of those. In the run - * that throw is the right answer - the token is wrong and the choice should - * stop. In a preview it blanks the row and turns it red between two keystrokes, - * which is the per-keystroke noise #1558 removed. The options are simply not - * known yet, so the preview renders as though none were given and picks the - * real ones up as soon as the token parses. + * `|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, -): ParsedVDateOptions { +export function parseVDateOptionsForPreview(rawOptions: string | undefined | null): { + options: ParsedVDateOptions; + error?: string; +} { try { - return parseVDateOptions(rawOptions); - } catch { - return { optional: false, withTime: false }; + return { options: parseVDateOptions(rawOptions) }; + } catch (error) { + return { + options: { optional: false, withTime: false }, + error: error instanceof Error ? error.message : String(error), + }; } } From 957260dec08563ceba5bafce71eae995e1f5f729 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 18:54:48 +0200 Subject: [PATCH 08/10] test(preview): anchor the seeded VDATE instant to local time The two "renders an ANSWERED date" cases seeded @date:2026-08-15T00:00:00.000Z and asserted "2026-08-15". The stored value is an INSTANT and moment formats it in local time, so anywhere west of UTC it renders as the 14th - green in CET, red on the UTC runners. Built from a local Date instead, which round-trips in every zone (verified UTC-8 through UTC+14). The sibling illegal-chars test file already carried this warning in its header. --- .../fileNameDisplayFormatter.audit-cleanup.test.ts | 6 +++++- src/formatters/formatDisplayFormatter-1589-vdate.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts index c0f83678..f710d11a 100644 --- a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts +++ b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts @@ -146,7 +146,11 @@ describe("FileNameDisplayFormatter VDATE preview", () => { const formatter = makeFormatter(); (formatter as unknown as { variables: Map }).variables.set( "due", - "@date:2026-08-15T00:00:00.000Z", + // 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"); }); diff --git a/src/formatters/formatDisplayFormatter-1589-vdate.test.ts b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts index 4e769303..336aa1db 100644 --- a/src/formatters/formatDisplayFormatter-1589-vdate.test.ts +++ b/src/formatters/formatDisplayFormatter-1589-vdate.test.ts @@ -77,7 +77,11 @@ describe("FormatDisplayFormatter VDATE default format (#1589)", () => { const formatter = makeFormatter(); (formatter as unknown as { variables: Map }).variables.set( "due", - "@date:2026-08-15T00:00:00.000Z", + // 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"); }); From d190bedc0363f896eedebcbca68f641612183000 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 19:08:52 +0200 Subject: [PATCH 09/10] fix(preview): treat an inline-script folder as unknown, and pin two guards Codex spotted a real hole in likelyTargetFolderPath: it refuses a configured folder containing `{{`, but an inline `js quickadd` fence contains none, and the folder validator lets backticks through (INVALID_FOLDER_CHARS_REGEX is \ / : * ? " < > |). formatFolderPath EXECUTES that fence - it is format()'s first pass - so the previews would have spliced the raw script source into {{FOLDER}}, which is exactly what an inert preview must never do (#1558), and its punctuation would then have been read as illegal characters. It falls back to the neutral placeholder now, with the rule taken from INLINE_JAVASCRIPT_REGEX rather than approximated. Two coverage gaps CodeRabbit named, both of which were vacuous when written and are mutation-verified now: - The move offer's guard had no test at all. Driving it needs the INTERACTIVE path (a `templatePath` argument makes the run non-interactive and skips reconciliation entirely, so the first attempt passed with the guard deleted). Both halves are pinned: an impossible target asks nothing and renames nothing, a legal one still offers the move. - The capture guard's test now asserts the capture's own {{VALUE}} prompt never opened, which is the whole point of #1591 - the user must not answer a prompt chain for a note that cannot be created. --- .../CaptureChoiceEngine.selection.test.ts | 4 + src/engine/applyTemplateToActiveNote.test.ts | 109 +++++++++++++++++- src/utils/previewTargetFolder.test.ts | 69 +++++++++++ src/utils/previewTargetFolder.ts | 18 ++- 4 files changed, 193 insertions(+), 7 deletions(-) create mode 100644 src/utils/previewTargetFolder.test.ts diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 2096e4fb..8f663955 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1286,6 +1286,10 @@ describe("CaptureChoiceEngine capture target resolution", () => { 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 () => { 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/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 index dfa4460c..e88a941b 100644 --- a/src/utils/previewTargetFolder.ts +++ b/src/utils/previewTargetFolder.ts @@ -1,3 +1,4 @@ +import { INLINE_JAVASCRIPT_REGEX } from "../constants"; import type { TemplateFolderConfig } from "../types/choices/ITemplateChoice"; /** @@ -12,15 +13,19 @@ import type { TemplateFolderConfig } from "../types/choices/ITemplateChoice"; * 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 contain no format tokens.** The run formats the configured - * folder first (`getFolderPath` -> `CompleteFormatter.formatFolderPath`), - * while `setTargetFolderPath` only trims slashes - so handing over a raw + * 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. 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. + * 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 `` @@ -46,5 +51,6 @@ export function likelyTargetFolderPath( const only = folders[0]?.trim(); if (!only) return undefined; if (only.includes("{{")) return undefined; + if (INLINE_JAVASCRIPT_REGEX.test(only)) return undefined; return only; } From da6e01ca7716002ed5ee4eada217f8be307f0ec5 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 21:23:28 +0200 Subject: [PATCH 10/10] test(preview): pin that a slow preview pass is stale, never mixed CodeRabbit asked whether the previous value's diagnostics can label the new one while an async pass is in flight past the 500ms idle gate. They cannot, and the regression test they asked for is what shows why: `preview` and `diagnostics` commit together under `previewToken`, so the row always holds one pass's text under that same pass's label and complaint. It can lag; it cannot mix. A stale pass landing after a newer one is discarded, which the second case pins. The comment records why the obvious fix is a downgrade: clearing `diagnostics` on a value change would leave the previous, known-bad name on screen under a plain "Preview:" label for the length of the pass - the exact assertion this row exists to withdraw. --- .../components/FormatPreviewField.svelte | 12 ++ .../components/FormatPreviewField.test.ts | 108 ++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index 78a4fb34..1cabdaa0 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -110,6 +110,18 @@ const hidden = $derived( // 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); diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts index 0507d628..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"; @@ -285,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([]); + }); +});