From 0d5abe147e91cbb6466d2d63f9f252e2288a9705 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 10:20:59 +0200 Subject: [PATCH 1/5] test: pin the file-name preview against the real formatter (#1580) fileNameDisplayFormatter.test.ts defined its own TestFileNameDisplayFormatter - a dozen hand-written regex replaces - and asserted that those regexes did what they said. Eleven green tests that never imported FileNameDisplayFormatter and could not fail for any change to it. They had also drifted into asserting behaviour the plugin cannot produce: {{TEMPLATE:daily-note}} was pinned to a fabricated '[daily-note template content...]' placeholder that #1560 deleted and #1563 replaced with a real inert read. Replaced with one case per token against the real class. Two of them pin CURRENT behaviour that the mock claimed otherwise for, each with the issue it is filed as: {{MATH:}} is left literal by the preview while the run prompts for it (#1587), and {{title}} previews as a name although formatFileName throws on it (#1588). --- .../fileNameDisplayFormatter.test.ts | 219 ++++++++++++------ 1 file changed, 143 insertions(+), 76 deletions(-) diff --git a/src/formatters/fileNameDisplayFormatter.test.ts b/src/formatters/fileNameDisplayFormatter.test.ts index 0340949c8..8be80fa55 100644 --- a/src/formatters/fileNameDisplayFormatter.test.ts +++ b/src/formatters/fileNameDisplayFormatter.test.ts @@ -1,104 +1,171 @@ -import { describe, it, expect, beforeEach } from 'vitest'; - -// Simple mock for testing -class TestFileNameDisplayFormatter { - private mockApp: unknown; - - constructor(app: unknown) { - this.mockApp = app; - } - - public async format(input: string): Promise { - // Simplified format implementation for testing - let output = input; - - // Replace basic patterns for testing - output = output.replace(/\{\{DATE\}\}/g, '2024-01-15'); - output = output.replace(/\{\{VALUE\}\}/g, 'user input'); - output = output.replace(/\{\{VALUE:title\}\}/g, 'My Document Title'); - output = output.replace(/\{\{VALUE:project\}\}/g, 'Project Alpha'); - output = output.replace(/\{\{MACRO:clipboard\}\}/g, 'clipboard_content'); - output = output.replace(/\{\{MACRO:uuid\}\}/g, 'unique_id'); - output = output.replace(/\{\{LINKTOCURRENT\}\}/g, 'example'); - output = output.replace(/\{\{VDATE:[^}]+\}\}/g, '2024-01-15'); - output = output.replace(/\{\{MATH:[^}]+\}\}/g, 'calculation_result'); - output = output.replace(/\{\{FIELD:[^}]+\}\}/g, 'category_field_value'); - output = output.replace(/\{\{SELECTED\}\}/g, 'selected_text'); - output = output.replace(/\{\{TEMPLATE:[^}]+\}\}/g, '[daily-note template content...]'); - - return output; - } -} +import { describe, it, expect } from "vitest"; +import type { App } from "obsidian"; +import { TFile } from "obsidian"; +import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; +import type QuickAdd from "../main"; + +/** + * The file-name preview's token vocabulary, pinned against the REAL formatter. + * + * This file used to define a `TestFileNameDisplayFormatter` class with a + * hand-written `format()` of a dozen regex replaces and assert that those + * regexes did what they said - eleven green tests that never imported + * `FileNameDisplayFormatter` and could not fail for any change to it (issue + * #1580). It had also drifted into asserting behaviour the plugin cannot + * produce: `{{TEMPLATE:daily-note}}` was pinned to a fabricated + * `[daily-note template content...]` placeholder that #1560 deleted and #1563 + * replaced with a real inert read. + * + * Every case below constructs the real class. The sibling files cover the + * behaviours in depth - `fileNameDisplayFormatter.audit-cleanup.test.ts` (VDATE + * hints), `fileNameDisplayFormatter-1563-normalize.test.ts` (the run's name + * normalizer), `fileNameDisplayFormatter-1563-template.test.ts` ({{TEMPLATE:}} + * inertness) - so this one is deliberately the broad, shallow pass: one case per + * token, so that deleting a pass from `formatInternal` fails a test. + */ + +const templates: Record = { + "Templates/Daily.md": "Daily body\n", +}; -// Mock Obsidian App -const mockApp = { - workspace: { - getActiveFile: () => ({ - path: 'test/example.md', - basename: 'example' - }) - } +const activeFile = { + basename: "example", + path: "test/example.md", + parent: { path: "test" }, }; -describe('FileNameDisplayFormatter', () => { - let formatter: TestFileNameDisplayFormatter; +function makeApp(): App { + return { + workspace: { getActiveFile: () => activeFile }, + vault: { + getMarkdownFiles: () => [], + getAbstractFileByPath: (path: string) => + path in templates + ? Object.assign(new TFile(), { + path, + extension: "md", + basename: path.replace(/\.md$/, ""), + }) + : null, + cachedRead: async (file: { path: string }) => templates[file.path], + }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, + } as unknown as App; +} + +const plugin = { + settings: { globalVariables: {}, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +function makeFormatter(): FileNameDisplayFormatter { + return new FileNameDisplayFormatter(makeApp(), plugin); +} + +async function preview(input: string) { + const formatter = makeFormatter(); + const text = await formatter.format(input); + return { text, diagnostics: formatter.diagnostics.list() }; +} - beforeEach(() => { - formatter = new TestFileNameDisplayFormatter(mockApp); +describe("FileNameDisplayFormatter resolves the tokens a file name can hold", () => { + it("previews {{DATE}}", async () => { + // Date FORMAT is covered by the date helpers' own tests; what matters here + // is that the pass runs at all and leaves no token behind. + const { text } = await preview("{{DATE}} - {{VALUE}}"); + expect(text).toMatch(/^\d{4}-\d{2}-\d{2} - user input$/); }); - it('should format a simple filename with date', async () => { - const result = await formatter.format('{{DATE}} - {{VALUE}}'); - expect(result).toMatch(/\d{4}-\d{2}-\d{2} - user input/); + it("previews named {{VALUE:x}} prompts with per-name examples", async () => { + const { text } = await preview("{{VALUE:title}} - {{VALUE:project}}"); + expect(text).toBe("Example Title - Project Alpha"); }); - it('should format filename with variables', async () => { - const result = await formatter.format('{{VALUE:title}} - {{VALUE:project}}'); - expect(result).toBe('My Document Title - Project Alpha'); + it("previews {{MACRO:x}} without running the macro engine", async () => { + const { text } = await preview("{{MACRO:clipboard}} - {{MACRO:uuid}}"); + expect(text).toBe("clipboard_content - unique_id"); }); - it('should format filename with macros', async () => { - const result = await formatter.format('{{MACRO:clipboard}} - {{MACRO:uuid}}'); - expect(result).toBe('clipboard_content - unique_id'); + it("previews {{VDATE:name,format}}", async () => { + const { text } = await preview("{{VDATE:dueDate, YYYY-MM-DD}}"); + expect(text).toMatch(/^\d{4}-\d{2}-\d{2}$/); }); - it('should format filename with current file link', async () => { - const result = await formatter.format('Related to {{LINKTOCURRENT}}'); - expect(result).toBe('Related to example'); + it("previews {{FIELD:x}} as a value of that field", async () => { + const { text } = await preview("{{FIELD:category}}"); + expect(text).toBe("category_field_value"); }); - it('should handle date variables with format', async () => { - const result = await formatter.format('{{VDATE:dueDate, YYYY-MM-DD}}'); - expect(result).toMatch(/\d{4}-\d{2}-\d{2}/); + it("previews {{SELECTED}} and {{CLIPBOARD}} without reading either", async () => { + const { text } = await preview("{{SELECTED}} {{CLIPBOARD}}"); + expect(text).toBe("selected_text clipboard_content"); }); - it('should handle math expressions', async () => { - const result = await formatter.format('File {{MATH:1+1}}'); - expect(result).toBe('File calculation_result'); + it("previews {{RANDOM:n}}", async () => { + const { text } = await preview("{{RANDOM:4}}"); + expect(text).toBe("ABC1"); }); - it('should handle field variables', async () => { - const result = await formatter.format('{{FIELD:category}}'); - expect(result).toBe('category_field_value'); + it("previews {{FOLDERCURRENT}} as the active file's folder", async () => { + const { text } = await preview("{{FOLDERCURRENT}}/Note"); + expect(text).toBe("test/Note"); }); - it('should handle selected text', async () => { - const result = await formatter.format('Note about {{SELECTED}}'); - expect(result).toBe('Note about selected_text'); + it("reads a {{TEMPLATE:}} body inertly", async () => { + const { text, diagnostics } = await preview("{{TEMPLATE:Templates/Daily.md}}"); + expect(text).toBe("Daily body"); + expect(diagnostics).toEqual([]); }); - it('should handle templates', async () => { - const result = await formatter.format('{{TEMPLATE:daily-note}}'); - expect(result).toBe('[daily-note template content...]'); + it("reports a missing {{TEMPLATE:}} as an error, because the run aborts", async () => { + const { text, diagnostics } = await preview("{{TEMPLATE:missing.md}}"); + expect(text).toBe("[QuickAdd: template not found] missing.md"); + expect(diagnostics).toEqual([ + { severity: "error", message: "Template not found: missing.md" }, + ]); }); - it('should handle empty input', async () => { - const result = await formatter.format(''); - expect(result).toBe(''); + it("leaves link tokens literal, as the run's formatFileName does", async () => { + // `formatFileName` resolves {{filenamecurrent}}/{{folder}}/{{foldercurrent}} + // but never the link tokens - a file name is not a place for a wikilink. + const { text } = await preview("Related to {{LINKTOCURRENT}}"); + expect(text).toBe("Related to {{LINKTOCURRENT}}"); + }); + + it("previews nothing for empty input", async () => { + const { text, diagnostics } = await preview(""); + expect(text).toBe(""); + expect(diagnostics).toEqual([]); + }); + + it("echoes an unterminated token instead of throwing", async () => { + const { text, diagnostics } = await preview("{{INVALID"); + expect(text).toBe("{{INVALID"); + expect(diagnostics).toEqual([]); + }); +}); + +describe("tokens the file-name preview does NOT resolve today", () => { + /** + * Both pinned as CURRENT behaviour with an issue number, not as desired + * behaviour. The old mock in this file asserted the opposite for {{MATH:}} + * ("File calculation_result") - which is exactly the kind of claim a test + * that mocks itself can make forever without anyone noticing. + */ + it("leaves {{MATH:}} literal even though the run resolves it (#1587)", async () => { + // CompleteFormatter.format runs replaceMathValueInString and + // formatFileName goes through format(), so the run really does prompt + // here. Neither display formatter has the pass, though both override + // `promptForMathValue` with a stand-in that is therefore unreachable. + const { text } = await preview("File {{MATH:1+1}}"); + expect(text).toBe("File {{MATH:1+1}}"); }); - it('should handle malformed syntax gracefully', async () => { - const result = await formatter.format('{{INVALID'); - expect(result).toBe('{{INVALID'); + 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. + const { text, diagnostics } = await preview("{{title}} note"); + expect(text).toBe("{{title}} note"); + expect(diagnostics).toEqual([]); }); }); From 9381ed4a1fb6f95beda706f05235dd6d5a480328 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 10:32:44 +0200 Subject: [PATCH 2/5] fix: the {{FIELD:}} preview names the field, not the filter syntax (#1579) Both preview formatters built the placeholder out of the token's whole inner text, filters included, so the more precisely you filtered the less the preview looked like a value: {{FIELD:status|folder:Work}} previewed 'status|folder:Work_field_value'. The field is status; at run time the token resolves to one of that property's values. replaceFieldVarInString already parsed the specifier one line above the call, so the parsed field name is handed to suggestForField. The variable KEY stays keyed on the whole specifier - two {{FIELD:status}} tokens with different filters are different prompts. Also fixes the fallback beside it: getVariableValue was called with the bare specifier instead of the FIELD-prefixed key, so on the one path where a suggester resolves undefined (a remote prompt provider can) it both missed the value that had been stored and cross-read the {{VALUE}} namespace - a {{VALUE:status}} answer could be served to a {{FIELD:status}} token. --- ...yFormatters-1579-field-placeholder.test.ts | 68 +++++++++++++++++++ src/formatters/fileNameDisplayFormatter.ts | 8 ++- src/formatters/formatDisplayFormatter.ts | 8 ++- src/formatters/formatter.ts | 19 +++++- src/formatters/helpers/previewHelpers.ts | 25 +++++++ 5 files changed, 122 insertions(+), 6 deletions(-) create mode 100644 src/formatters/displayFormatters-1579-field-placeholder.test.ts diff --git a/src/formatters/displayFormatters-1579-field-placeholder.test.ts b/src/formatters/displayFormatters-1579-field-placeholder.test.ts new file mode 100644 index 000000000..3931a854c --- /dev/null +++ b/src/formatters/displayFormatters-1579-field-placeholder.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from "vitest"; +import type { App } from "obsidian"; +import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; +import { FormatDisplayFormatter } from "./formatDisplayFormatter"; +import type QuickAdd from "../main"; + +/** + * Issue #1579. Both preview formatters built the `{{FIELD:...}}` placeholder out + * of the token's WHOLE inner text, filters included, so the more precisely you + * filtered the less the preview looked like a value: + * `{{FIELD:status|folder:Work}}` previewed `status|folder:Work_field_value`. + * The field is `status`. + */ +const mockApp = { + 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 formatters = [ + ["file name", () => new FileNameDisplayFormatter(mockApp, plugin)], + ["format", () => new FormatDisplayFormatter(mockApp, plugin)], +] as const; + +describe.each(formatters)("the %s preview names the FIELD", (_label, make) => { + it.each([ + ["no filters", "{{FIELD:status}}", "status_field_value"], + ["one filter", "{{FIELD:status|folder:Work}}", "status_field_value"], + [ + "several filters", + "{{FIELD:status|folder:Work|exclude-tag:archive|multi}}", + "status_field_value", + ], + // Not a case: `{{FIELD:status,Work}}` previews `status,Work_field_value`, + // and that is faithful - FieldSuggestionParser splits on `|` only, so the + // run looks up a property literally named "status,Work" too. + ])("%s", async (_case, input, expected) => { + expect(await make().format(input)).toBe(expected); + }); + + it("says something neutral when the field name is missing", async () => { + // Reachable on every keystroke of `{{FIELD:|folder:x}}`. Echoing the raw + // specifier back here would reprint the filters, which is the bug. + expect(await make().format("{{FIELD:|folder:Work}}")).toBe("field_value"); + }); +}); + +describe("#1579 the variable KEY still carries the whole specifier", () => { + it("keeps two differently filtered {{FIELD:status}} tokens apart", async () => { + // They are different prompts at run time, so they must not collapse onto + // one variable - even though they now PREVIEW identically. + const formatter = new FileNameDisplayFormatter(mockApp, plugin); + await formatter.format("{{FIELD:status|folder:Work}} {{FIELD:status}}"); + + const keys = [ + ...(formatter as unknown as { variables: Map }).variables.keys(), + ]; + expect(keys).toEqual([ + "FIELD:status|folder:Work", + "FIELD:status", + ]); + }); +}); diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index f598acff5..eb8e10d08 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -16,6 +16,7 @@ import { getMacroPreview, getVariablePromptExample, getSuggestionPreview, + fieldValuePreview, getCurrentFileLinkPreview, getCurrentFileLinkToSectionPreview, getCurrentFileNamePreview, @@ -366,8 +367,11 @@ export class FileNameDisplayFormatter extends Formatter { return "clipboard_content"; } - protected async suggestForField(variableName: string): Promise { - return `${variableName}_field_value`; + protected async suggestForField( + _variableName: string, + parsed: { fieldName: string }, + ): Promise { + return fieldValuePreview(parsed); } protected suggestForFile(parsed: ParsedFileToken): string { diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index 90e20e7b5..b54cb6294 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -14,6 +14,7 @@ import { getMacroPreview, getVariablePromptExample, getSuggestionPreview, + fieldValuePreview, getCurrentFileLinkPreview, getCurrentFileLinkToSectionPreview, getCurrentFileNamePreview, @@ -279,8 +280,11 @@ export class FormatDisplayFormatter extends Formatter { return "clipboard_content"; } - protected async suggestForField(variableName: string) { - return Promise.resolve(`${variableName}_field_value`); + protected async suggestForField( + _variableName: string, + parsed: { fieldName: string }, + ) { + return Promise.resolve(fieldValuePreview(parsed)); } protected suggestForFile(parsed: ParsedFileToken): string { diff --git a/src/formatters/formatter.ts b/src/formatters/formatter.ts index 835527075..65c5c31ce 100644 --- a/src/formatters/formatter.ts +++ b/src/formatters/formatter.ts @@ -1177,13 +1177,20 @@ export abstract class Formatter { if (!this.hasConcreteVariable(fieldVariableKey)) { this.variables.set( fieldVariableKey, - await this.suggestForField(fullMatch), + await this.suggestForField(fullMatch, parsed), ); } + // The FIELD key, not the bare specifier. `getVariableValue` is only + // reached when the suggester resolved `undefined` (a remote prompt + // provider can), and looking up `status|folder:Work` there both + // misses the value that WAS stored and cross-reads the {{VALUE}} + // namespace, so a `{{VALUE:status}}` answer could be served to a + // `{{FIELD:status}}` token - the separation FIELD_VARIABLE_PREFIX + // exists for. const rawValue = this.hasConcreteVariable(fieldVariableKey) ? this.variables.get(fieldVariableKey) - : this.getVariableValue(fullMatch); + : this.getVariableValue(fieldVariableKey); let replacement: string; if (Array.isArray(rawValue)) { @@ -1409,8 +1416,16 @@ export abstract class Formatter { return []; } + /** + * @param variableName the WHOLE `{{FIELD:...}}` specifier, filters included. + * It is what the runtime suggesters parse and what the variable is keyed on. + * @param parsed the same specifier already parsed by the caller. The preview + * formatters need only `fieldName` from it, and passing it in is what keeps + * their placeholder from reading `status|folder:Work_field_value` (#1579). + */ protected abstract suggestForField( variableName: string, + parsed: { fieldName: string }, ): Promise; protected async replaceDateVariableInString(input: string) { diff --git a/src/formatters/helpers/previewHelpers.ts b/src/formatters/helpers/previewHelpers.ts index 38782a3af..dc9d7b721 100644 --- a/src/formatters/helpers/previewHelpers.ts +++ b/src/formatters/helpers/previewHelpers.ts @@ -83,6 +83,31 @@ export function getSuggestionPreview(suggestedValues: string[]): string { return "suggestion_list"; } +/** + * The stand-in a preview shows for a `{{FIELD:...}}` token. + * + * Named after the FIELD, not the whole specifier: the token's inner text is the + * field name plus any filters, and building the placeholder out of all of it + * meant the more precisely you filtered, the less the preview looked like a + * value - `{{FIELD:status|folder:Work}}` previewed + * `status|folder:Work_field_value` (#1579). The field is `status`; at run time + * the token resolves to one of that property's values. + * + * Deliberately still a placeholder rather than a real value from the vault, the + * way `{{FILE:}}` previews a real file: FIELD PROMPTS at run time, so any + * concrete value would assert a pick the user has not made, and would change + * from keystroke to keystroke as the filter narrowed the candidate set. + * + * `fieldName` is empty for a specifier that starts with a pipe + * (`{{FIELD:|folder:x}}`, and every prefix of it while that is being typed). + * Falling back to the raw specifier there would print the filters again, which + * is the bug; a neutral noun is what is left to say. + */ +export function fieldValuePreview(parsed: { fieldName: string }): string { + const fieldName = parsed.fieldName.trim(); + return fieldName ? `${fieldName}_field_value` : "field_value"; +} + /** * Gets a current file link preview */ From 45d77bcb1b4e969477888e7454c285dabefee54e Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 10:46:07 +0200 Subject: [PATCH 3/5] fix: the file-name preview says when Obsidian will refuse the name (#1578) 'Bad: {{VALUE:title}}' previewed 'Bad: Example Title' in the ordinary 'Preview:' styling, and running the choice created nothing - the Notice was Obsidian's own: 'File name cannot contain any of the following characters: \\ / :'. #1563 made this row mirror the run's name normalizer; a character Obsidian refuses is the same class of truth and the last one missing. Measured against vault.create/createFolder on Obsidian 1.13.0 (macOS), one candidate character per name: ':' throws Obsidian's own guard for files AND folder segments; '* ? " < > | ^ [ ] #' and tab all create successfully, so copying the stricter set from TemplateEngine.validateFolderSegment would reject names Obsidian makes without complaint; '\\' and '/' are separators and QuickAdd creates the parent folder. The rule is ':' and only ':'. The check reads the FINISHED name rather than the format string. That is the only place all the sources meet: a colon the author typed, one {{TIME}} produced (it is HH:mm, and the token autocomplete offers it in this field), one a global snippet or an included template body carried in, and one left behind by a token that never matched - {{TEMPLATE:Naming}} without the extension is not a token, so the literal text goes to the vault, and a mask over {{...}}-shaped spans would be blind to exactly that, the most likely {{TEMPLATE:}} typo. Reading the finished name only works if the preview stops writing text that is not part of the name, so two stand-ins were corrected first: - the VDATE '(default: X)' / '(optional)' hints are gone from the file-name preview. The run splices in the formatted date and nothing else, so '2026-07-27 (default: tomorrow)' was already a name that could not exist - and the colon in it would have been blamed on the author. The hints stay on the body preview and in the run's own prompt placeholder. - an inline {{VALUE:a,b}} option list previews the option, without the body preview's ' (N options)' count. Three guards keep it from crying wolf: a pass that already reported an error says nothing more (all four [QuickAdd: ...] placeholders carry a colon and each already named its real problem); an unterminated '{{' means the author is mid-token with the format suggester open; and inline 'js quickadd' fences are excluded, since the run replaces a fence with what the script RETURNS while the preview must leave the source verbatim. Stand-ins that echo a token's own argument - a {{VALUE:}} prompt header, a macro name, a field name - degrade to a neutral placeholder when they would otherwise invent one of these characters. A stand-in is fiction either way; fiction that could not be a real file name is worse fiction. The run is deliberately unchanged. Failing fast at the create sinks would help - the folder is created and the template body formatted (prompts, macros, script fences) before Obsidian rejects the name - but a hard throw would also break appending to a colon-named file that already exists, which is legal on macOS/Linux. Filed separately. The formatter reports the colon for capture-target syntax like 'property:status=done' too, because it previews FILE NAMES and the same class previews a Template choice's file name, where that literal IS a path. CaptureTargetSetting renders no preview row for recognised picker syntax, and that gate is now pinned by a component test instead of a comment. --- ...ameDisplayFormatter-1563-normalize.test.ts | 25 +- ...NameDisplayFormatter-1563-template.test.ts | 10 + ...isplayFormatter-1578-illegal-chars.test.ts | 247 ++++++++++++++++++ ...NameDisplayFormatter.audit-cleanup.test.ts | 58 ++-- src/formatters/fileNameDisplayFormatter.ts | 160 +++++++++--- src/formatters/helpers/previewHelpers.ts | 16 ++ ...eTargetSetting-1578-picker-preview.test.ts | 95 +++++++ src/utils/generatedFilePath.ts | 58 ++++ 8 files changed, 603 insertions(+), 66 deletions(-) create mode 100644 src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts create mode 100644 src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts diff --git a/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts b/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts index 251b66856..07937b466 100644 --- a/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts @@ -87,9 +87,30 @@ describe("the file-name preview mirrors the run's name normalizer", () => { "tag:#inbox", "Daily/2026-07-27.md", ]) { - const { out, problems } = await preview(target); + const { out } = await preview(target); expect(out).toBe(target); - expect(problems).toEqual([]); } }); + + it("does report the colon in a picker target, which the builder hides", async () => { + // This formatter previews FILE NAMES. `property:x=y` and `tag:#inbox` are + // capture-target syntax, not paths, and a colon in an actual name really + // is fatal - so the rule is right and the SURFACE is what knows the + // difference: CaptureTargetSetting.svelte renders no preview row at all + // while the field holds recognised picker syntax + // (`{#if !usesPickerTargetSyntax}`), and that gate is pinned by + // CaptureTargetSetting-1578-picker-preview.test.ts. + // + // Teaching the formatter capture semantics would be the wrong layer: the + // same class previews a Template choice's file name, where a literal + // `property:x=y` IS a path and the colon IS the problem. + const { problems } = await preview("property:status=done"); + expect(problems).toEqual([ + { + severity: "error", + message: + 'A file or folder name cannot contain ":". Obsidian refuses it, so this choice would fail at run time.', + }, + ]); + }); }); diff --git a/src/formatters/fileNameDisplayFormatter-1563-template.test.ts b/src/formatters/fileNameDisplayFormatter-1563-template.test.ts index 61d62194e..10659bb77 100644 --- a/src/formatters/fileNameDisplayFormatter-1563-template.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1563-template.test.ts @@ -146,6 +146,16 @@ describe("#1563 the file-name preview resolves {{TEMPLATE:}}", () => { message: 'Template "Body.md" is 5 lines; a file name is one line, so they are joined with spaces.', }, + { + // The frontmatter's "title: x" is now IN the name, so the name has a + // colon in it and Obsidian would refuse it (#1578). The + // "Obsidian refuses it" variant, because a colon IS visible in the + // field - `{{TEMPLATE:Body.md}}` has one, even though that is not the + // one that landed in the name. + severity: "error", + message: + 'A file or folder name cannot contain ":". Obsidian refuses it, so this choice would fail at run time.', + }, ]); }); diff --git a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts new file mode 100644 index 000000000..76d77298d --- /dev/null +++ b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts @@ -0,0 +1,247 @@ +import realMoment from "moment"; +import { + afterAll, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; +import type { App } from "obsidian"; +import { TFile } from "obsidian"; +import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; +import type QuickAdd from "../main"; + +/** + * Issue #1578. The file-name preview used to present names Obsidian refuses: + * `Bad: {{VALUE:title}}` previewed `Bad: Example Title` in the ordinary + * "Preview:" styling, and running the choice created nothing. + * + * MEASURED against `vault.create` / `vault.createFolder` on Obsidian 1.13.0 + * (macOS, isolated e2e vault), one candidate character per name: `:` throws + * Obsidian's own "File name cannot contain any of the following characters: + * \ / :" for files AND folder segments; `* ? " < > | ^ [ ] #` and tab all create + * successfully; `\` and `/` are separators (QuickAdd creates the parent folder). + * So the rule is `:` and only `:`. + * + * The check reads the FINISHED name rather than the format string, because that + * is the only place all the sources meet - typed text, `{{TIME}}`, a global + * snippet, an included template body, and the literal text left behind by a + * token that never matched. That only works if the preview's own stand-ins stay + * name-shaped, which is what the "does not invent" block below pins. + * + * Real moment + a frozen clock: the obsidian-stub moment returns the same string + * for every format (tests/obsidian-stub.ts), so `{{TIME}}` and `{{DATE:HH:mm}}` + * would produce no colon at all and every date case here would pass vacuously. + * A LOCAL-time literal, not a `Z` instant, or `HH:mm` becomes TZ-dependent. + */ +const originalMoment = (window as unknown as { moment?: unknown }).moment; +const previousLocale = realMoment.locale(); + +beforeAll(() => { + realMoment.locale("en"); + (window as unknown as { moment: unknown }).moment = realMoment; +}); +afterAll(() => { + (window as unknown as { moment?: unknown }).moment = originalMoment; + realMoment.locale(previousLocale); + vi.useRealTimers(); +}); +beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2023-06-01T14:30:05")); + templates = {}; + globalVariables = {}; +}); + +let templates: Record = {}; +let globalVariables: Record = {}; + +function makeApp(): App { + return { + workspace: { + getActiveFile: () => ({ + basename: "example", + path: "test/example.md", + parent: { path: "test" }, + }), + }, + vault: { + getMarkdownFiles: () => [], + getAbstractFileByPath: (path: string) => + path in templates + ? Object.assign(new TFile(), { + path, + extension: "md", + basename: path.replace(/\.md$/, ""), + }) + : null, + cachedRead: async (file: { path: string }) => templates[file.path], + }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, + } as unknown as App; +} + +function makeFormatter(): FileNameDisplayFormatter { + const formatter = new FileNameDisplayFormatter(makeApp(), { + settings: { globalVariables, choices: [] }, + getTemplateFiles: () => [], + } as unknown as QuickAdd); + // Every real caller sets this (FormatPreviewField passes the choice's folder, + // or a "Folder/Name" placeholder), and leaving it unset makes {{FOLDER}} + // collapse to an empty segment. + formatter.setTargetFolderPath("Folder/Name"); + return formatter; +} + +async function preview(input: string) { + const formatter = makeFormatter(); + const text = await formatter.format(input); + return { text, diagnostics: formatter.diagnostics.list() }; +} + +const TYPED = + 'A file or folder name cannot contain ":". Obsidian refuses it, so this choice would fail at run time.'; +const FROM_TOKEN = + 'A file or folder name cannot contain ":". A token in this format resolves to one - {{TIME}} is the usual cause.'; + +describe("the file-name preview says when Obsidian will refuse the name", () => { + it("flags the colon the author typed - the reported case", async () => { + const { text, diagnostics } = await preview("Bad: {{VALUE:title}}"); + // Still shows the best-effort name: the diagnostic is what says it is + // unusable, and blanking the row would hide the shape of the mistake. + expect(text).toBe("Bad: Example Title"); + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); + + it("flags a colon in a folder segment, which Obsidian refuses too", async () => { + const { diagnostics } = await preview("Bad: folder/{{VALUE:title}}"); + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); + + it("flags {{TIME}}, which is HH:mm and which the author never typed", async () => { + const { text, diagnostics } = await preview("Meeting {{TIME}}"); + expect(text).toBe("Meeting 14:30"); + expect(diagnostics).toEqual([{ severity: "error", message: FROM_TOKEN }]); + }); + + it("flags a time format inside {{DATE:}}", async () => { + const { text, diagnostics } = await preview("Log {{DATE:HH:mm}}"); + expect(text).toBe("Log 14:30"); + // A colon IS visible in the field here, inside the token. + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); + + it("flags a colon a global snippet brought in", async () => { + globalVariables = { prefix: "Meeting: " }; + const { text, diagnostics } = await preview( + "{{GLOBAL_VAR:prefix}}{{VALUE:title}}", + ); + expect(text).toBe("Meeting: Example Title"); + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); + + it("flags a colon an included template body brought in", async () => { + templates["Naming.md"] = "Meeting: notes\n"; + const { text, diagnostics } = await preview("{{TEMPLATE:Naming.md}}"); + expect(text).toBe("Meeting: notes"); + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); + + it("flags a token that never matched and went to the vault verbatim", async () => { + // TEMPLATE_REGEX requires .md/.canvas/.base, so this is not a token at + // all - the literal text is the file name. A rule that masked + // `{{...}}`-shaped spans would be silent on the single most likely + // {{TEMPLATE:}} typo. + const { text, diagnostics } = await preview("{{TEMPLATE:Naming}}"); + expect(text).toBe("{{TEMPLATE:Naming}}"); + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); +}); + +describe("the file-name preview does not cry wolf", () => { + it("stays quiet on a name that is fine", async () => { + const { text, diagnostics } = await preview( + "{{DATE:YYYY-MM-DD}} {{VALUE:title}}", + ); + expect(text).toBe("2023-06-01 Example Title"); + expect(diagnostics).toEqual([]); + }); + + it("stays quiet mid-token, while the format suggester is open", async () => { + // `Notes/{{DATE:` is exactly what the field holds for as long as someone + // reads the popup this prefix opens, and the diagnostics row appears after + // 500ms of stillness - so without the guard, pausing to read the popup + // turns the row red. + const { text, diagnostics } = await preview("Notes/{{DATE:"); + expect(text).toBe("Notes/{{DATE:"); + expect(diagnostics).toEqual([]); + }); + + it("stays quiet about punctuation inside an inline script fence", async () => { + // The run replaces the fence with what the script RETURNS; the preview + // leaves the source verbatim because it must not execute anything (#1558). + // So the source's colons are never in the created name. + const { diagnostics } = await preview( + '```js quickadd\nconst a = {b: 1};\nreturn "Name";\n```', + ); + expect(diagnostics).toEqual([]); + }); + + it("does not pile on when the pass already failed", async () => { + // All four of this formatter's own placeholders are bracketed + // `[QuickAdd: ...]` strings with a colon in them, and each already reported + // the real problem. + const { diagnostics } = await preview("{{TEMPLATE:missing.md}}"); + expect(diagnostics).toEqual([ + { severity: "error", message: "Template not found: missing.md" }, + ]); + }); + + it("does not invent a colon out of a prompt header", async () => { + // The run prompts with this header and splices in the ANSWER, so a colon + // in the header is never in the name. The stand-in degrades to the + // generic one rather than accusing the author. + const { text, diagnostics } = await preview("{{VALUE:Cost: USD}}"); + expect(text).toBe("user input"); + expect(diagnostics).toEqual([]); + }); + + it("does not invent a colon out of a macro name", async () => { + const { text, diagnostics } = await preview("{{MACRO:my:macro}}"); + expect(text).toBe("macro_output"); + expect(diagnostics).toEqual([]); + }); + + it("does not invent a colon out of a field name", async () => { + const { text, diagnostics } = await preview("{{FIELD:a:b}}"); + expect(text).toBe("field_value"); + expect(diagnostics).toEqual([]); + }); + + it("does not invent a colon out of a VDATE default hint", async () => { + // The hint used to be appended to the NAME: `2023-06-01 (default: tomorrow)`. + const { text, diagnostics } = await preview( + "{{VDATE:due,YYYY-MM-DD|tomorrow}}", + ); + expect(text).toBe("2023-06-01"); + expect(diagnostics).toEqual([]); + }); + + it("does not invent an option count", async () => { + // The body preview says `Meeting (2 options)`; the run splices in the + // option that gets picked and nothing else. + const { text, diagnostics } = await preview("{{VALUE:Meeting,Note}}"); + expect(text).toBe("Meeting"); + expect(diagnostics).toEqual([]); + }); + + it("still reports an inline option that could not be a file name", async () => { + // Not invention: an inline option is literal text that becomes the whole + // name if it is the one picked, and then it cannot be created. + const { text, diagnostics } = await preview("{{VALUE:Meeting: standup,Note}}"); + expect(text).toBe("Meeting: standup"); + expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + }); +}); diff --git a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts index b746f8268..af36a9cd6 100644 --- a/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts +++ b/src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts @@ -4,15 +4,23 @@ import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; import type { App } from "obsidian"; /** - * Regression for the audit-cleanup fix (bucket cu-filename-preview, task - * format-core-format-preview): FileNameDisplayFormatter.replaceDateVariableInString - * used to stop at (match, variableName, dateFormat) and ignore the third capture - * group, so the file-name VDATE preview dropped the "(default: X)" / "(optional)" - * hints and the |startof:/|endof: period-snap that FormatDisplayFormatter's body - * preview shows. It now mirrors that behaviour. + * The file-name VDATE preview: the formatted date, and nothing else. * - * Snap rendering needs real moment + a frozen clock (the obsidian-stub moment has - * no startOf/endOf), mirroring formatter-datesnap.test.ts. en locale = + * The audit-cleanup fix (bucket cu-filename-preview, task + * format-core-format-preview) had this formatter mirror FormatDisplayFormatter + * and append " (default: X)" / " (optional)" hints about the token. #1578 + * removed them again, for the same reason #1563 put the run's name normalizer + * here: this row is a FILE NAME, the run splices in the formatted date and + * nothing else, so a hint made the preview assert a name that could never be + * created - and `(default: X)` put a colon, which Obsidian refuses outright, + * into the middle of it. + * + * The hints survive where they are true: on the body preview + * (FormatDisplayFormatter), and in the run's own prompt placeholder ("Enter + * value for due (default: tomorrow)"). + * + * Date rendering needs real moment + a frozen clock (the obsidian-stub moment + * has no startOf/endOf), mirroring formatter-datesnap.test.ts. en locale = * Sunday-first week. */ const originalMoment = (window as unknown as { moment?: unknown }).moment; @@ -43,26 +51,28 @@ function makeFormatter(): FileNameDisplayFormatter { return new FileNameDisplayFormatter(mockApp); } -describe("FileNameDisplayFormatter VDATE preview (audit-cleanup)", () => { - it("appends the (default: X) hint", async () => { - const out = await makeFormatter().format( - "{{VDATE:due,YYYY-MM-DD|tomorrow}}", - ); - expect(out).toBe("2023-06-01 (default: tomorrow)"); +describe("FileNameDisplayFormatter VDATE preview", () => { + it("shows the date alone, not the (default: X) hint (#1578)", async () => { + const formatter = makeFormatter(); + const out = await formatter.format("{{VDATE:due,YYYY-MM-DD|tomorrow}}"); + expect(out).toBe("2023-06-01"); + // And therefore no colon, so the row does not accuse the author of a + // character that only the preview ever wrote. + expect(formatter.diagnostics.list()).toEqual([]); }); - it("appends the (optional) hint", async () => { + it("shows the date alone, not the (optional) hint (#1578)", async () => { const out = await makeFormatter().format( "{{VDATE:due,YYYY-MM-DD|optional}}", ); - expect(out).toBe("2023-06-01 (optional)"); + expect(out).toBe("2023-06-01"); }); - it("appends both hints together (default + optional, order-insensitive)", async () => { + it("shows the date alone when both options are present (#1578)", async () => { const out = await makeFormatter().format( "{{VDATE:due,YYYY-MM-DD|optional|tomorrow}}", ); - expect(out).toBe("2023-06-01 (default: tomorrow) (optional)"); + expect(out).toBe("2023-06-01"); }); it("does NOT apply |startof: snap to the preview (matches body preview)", async () => { @@ -76,12 +86,16 @@ describe("FileNameDisplayFormatter VDATE preview (audit-cleanup)", () => { expect(out).toBe("gggg.06.[Wk]22"); }); - it("ignores snap but still appends a default hint", async () => { - const out = await makeFormatter().format( + it("ignores both the snap and the default hint", async () => { + const formatter = makeFormatter(); + const out = await formatter.format( "{{VDATE:eom,YYYY-MM-DD|endof:month|tomorrow}}", ); - // No snap applied to the preview; current date + default hint. - expect(out).toBe("2023-06-01 (default: 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"); + expect(formatter.diagnostics.list()).toEqual([]); }); it("leaves a snap-free VDATE preview unchanged (no spurious hints)", async () => { diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index eb8e10d08..2a2fe7042 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -17,16 +17,20 @@ import { getVariablePromptExample, getSuggestionPreview, fieldValuePreview, + fileNameSafeStandIn, getCurrentFileLinkPreview, getCurrentFileLinkToSectionPreview, getCurrentFileNamePreview, getCurrentFolderPathPreview, DateFormatPreviewGenerator } from "./helpers/previewHelpers"; -import { previewGeneratedFilePath } from "../utils/generatedFilePath"; +import { + describeIllegalFilePathChars, + findIllegalFilePathChars, + previewGeneratedFilePath, +} from "../utils/generatedFilePath"; import { getTemplateFile } from "../utils/templateFolderUtils"; import { getValueVariableBaseName } from "../utils/valueSyntax"; -import { parseVDateOptions } from "../utils/vdateSyntax"; import { EnhancedFieldSuggestionFileFilter } from "../utils/EnhancedFieldSuggestionFileFilter"; import { FILE_CUSTOM_PREFIX, FILE_PICK_PREFIX, type ParsedFileToken } from "../utils/fileSyntax"; @@ -39,6 +43,43 @@ import type QuickAdd from "../main"; */ const MAX_PREVIEW_TEMPLATE_INCLUDES = 25; +/** + * Is the last `{{` in `input` still waiting for its `}}`? + * + * `indexOf` scans, not a regex: this runs on every keystroke over a string that + * can be a whole included template body, and a lazy `/\{\{[\s\S]*?\}\}/` over + * that is quadratic on pathological input (the shape this repo has fixed four + * times over). + */ +function hasUnterminatedToken(input: string): boolean { + const lastOpen = input.lastIndexOf("{{"); + if (lastOpen === -1) return false; + return input.indexOf("}}", lastOpen + 2) === -1; +} + +/** + * `text` with any inline `js quickadd` fence removed. + * + * The run replaces a fence with whatever the script RETURNS + * (`replaceInlineJavascriptInString` is its very first pass), while the preview + * leaves the source verbatim - by design, it must not execute anything (#1558). + * So the fence's own punctuation is never in the created name, and reading the + * preview literally there would report a colon out of somebody's JavaScript. + * Same helper and the same reason as the template pass above (#1467). + */ +function textOutsideScriptSpans(text: string): string { + const spans = findInlineScriptSpans(text); + if (spans.length === 0) return text; + + let output = ""; + let index = 0; + for (const span of spans) { + output += text.slice(index, span.start); + index = span.end; + } + return output + text.slice(index); +} + export class FileNameDisplayFormatter extends Formatter { constructor( app: App, @@ -105,9 +146,56 @@ export class FileNameDisplayFormatter extends Formatter { for (const problem of normalized.problems) { this.diagnostics.add("error", problem); } + // On `output`, not `normalized.path`: the normalizer collapses the line + // breaks that delimit an inline script fence, and the scan below has to + // still be able to find one. It costs nothing - the normalizer only trims + // trailing dots/spaces and collapses control runs, so it can neither add + // nor remove one of these characters. + this.reportIllegalChars(input, output); return normalized.path; } + /** + * Says so when the name on screen is one Obsidian will not create (#1578). + * + * The check reads the FINISHED name rather than the format string, because + * that is the only place all the sources meet: a colon the author typed, one + * `{{TIME}}` produced (it is `HH:mm`, and the token autocomplete offers it in + * this field), one a `{{GLOBAL_VAR:}}` snippet or an included `{{TEMPLATE:}}` + * body carried in, and one left behind by a token that never matched + * (`{{TEMPLATE:Naming}}` without the extension is not a token, so the literal + * text goes to the vault). Reading the format string instead would need a + * token mask, and a mask is blind to exactly the last case - a typo, which is + * when the preview most needs to speak. + * + * Two things keep it from crying wolf: the preview's own stand-ins are kept + * name-shaped ({@link fileNameSafeStandIn}, and the VDATE hints are gone), and + * the two guards below. + */ + private reportIllegalChars(input: string, name: string): void { + // A pass that already failed has said something better. All four of this + // formatter's `[QuickAdd: ...]` placeholders carry a colon and all four + // report their real problem first, so without this the row would pile a + // second, misleading sentence on top of "Template not found". + if (this.diagnostics.hasError) return; + + // Mid-token. `Notes/{{DATE:` is what the field holds for as long as + // someone reads the format-suggester popup that this exact prefix opens, + // and the unmatched token stays literal in the output - so the colon is + // the caret's position, not a mistake. Costs only a literal `{{` in a + // name, which no format string has. + if (hasUnterminatedToken(input)) return; + + const illegal = findIllegalFilePathChars(textOutsideScriptSpans(name)); + if (illegal.length === 0) return; + this.reportProblem( + describeIllegalFilePathChars(illegal, { + visibleInFormat: + findIllegalFilePathChars(textOutsideScriptSpans(input)).length > 0, + }), + ); + } + /** * The preview pass list. * @@ -177,14 +265,17 @@ export class FileNameDisplayFormatter extends Formatter { } protected promptForValue(header?: string): string { - return header || "user input"; + // The header is a PROMPT header at run time, not part of the name, so an + // unusable one degrades to the generic stand-in rather than putting a + // character in the preview that the run would never produce. + return fileNameSafeStandIn(header || "user input", "user input"); } protected getVariableValue(variableName: string): string { const stored = this.variables.get(variableName); if (typeof stored === "string") return stored; const baseName = getValueVariableBaseName(variableName); - return getVariableExample(baseName); + return fileNameSafeStandIn(getVariableExample(baseName), "user input"); } protected getCurrentFileLink(): string | null { @@ -207,7 +298,10 @@ export class FileNameDisplayFormatter extends Formatter { allowCustomInput = false, _context?: { placeholder?: string; variableKey?: string }, ) { - return getSuggestionPreview(suggestedValues); + // The first option, without the body preview's " (N options)" count: the + // run splices in exactly the option that gets picked, so the count would + // be text in a file name that no created file can have. + return suggestedValues[0] ?? getSuggestionPreview(suggestedValues); } protected promptForMathValue(): Promise { @@ -218,14 +312,17 @@ export class FileNameDisplayFormatter extends Formatter { macroName: string, _context?: { label?: string }, ) { - return getMacroPreview(macroName); + return fileNameSafeStandIn(getMacroPreview(macroName), "macro_output"); } protected async promptForVariable( variableName: string, context?: PromptContext ): Promise { - return getVariablePromptExample(variableName); + return fileNameSafeStandIn( + getVariablePromptExample(variableName), + "user input", + ); } /** @@ -371,7 +468,7 @@ export class FileNameDisplayFormatter extends Formatter { _variableName: string, parsed: { fieldName: string }, ): Promise { - return fieldValuePreview(parsed); + return fileNameSafeStandIn(fieldValuePreview(parsed), "field_value"); } protected suggestForFile(parsed: ParsedFileToken): string { @@ -390,28 +487,19 @@ export class FileNameDisplayFormatter extends Formatter { protected async replaceDateVariableInString(input: string): Promise { let output: string = input; - // Mirror FormatDisplayFormatter's VDATE preview so the file-name preview - // shows the same default/optional hints (issue #511). Like the body - // preview, this renders the current 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, rawOptions) => { + // The date only. FormatDisplayFormatter appends " (default: X)" / + // " (optional)" hints about the token; this row is a FILE NAME, and the + // run splices in the formatted date and nothing else - so a hint here + // asserted a name that could never be created, which is the whole point + // of #1563/#1578. The hints survive where they are true: on the body + // preview, and in the run's own prompt placeholder ("Enter value for due + // (default: tomorrow)"). Like the body preview, this renders the current + // 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) => { const cleanVariableName = variableName?.trim(); const cleanDateFormat = dateFormat?.trim(); - // Parse defensively: a malformed |startof:/|endof: option can throw, and - // since format() catches and returns the whole raw input on any error, an - // unparseable VDATE option would otherwise blank out EVERY other preview - // substitution. Treat a parse failure as "no options". - let cleanDefaultValue: string | undefined; - let optional = false; - try { - ({ defaultValue: cleanDefaultValue, optional } = - parseVDateOptions(rawOptions)); - } catch { - cleanDefaultValue = undefined; - optional = false; - } if (!cleanVariableName || !cleanDateFormat) { return match; // Return original if incomplete @@ -419,23 +507,11 @@ export class FileNameDisplayFormatter extends Formatter { // Generate a realistic preview using the current date. const previewDate = new Date(); - let formattedExample: string; - try { - formattedExample = DateFormatPreviewGenerator.generate(cleanDateFormat, previewDate); + return DateFormatPreviewGenerator.generate(cleanDateFormat, previewDate); } catch { - formattedExample = `[${cleanDateFormat}]`; - } - - // If there's a default value, indicate it in the preview - if (cleanDefaultValue) { - formattedExample += ` (default: ${cleanDefaultValue})`; - } - if (optional) { - formattedExample += ` (optional)`; + return `[${cleanDateFormat}]`; } - - return formattedExample; }); return output; diff --git a/src/formatters/helpers/previewHelpers.ts b/src/formatters/helpers/previewHelpers.ts index dc9d7b721..17ea130ff 100644 --- a/src/formatters/helpers/previewHelpers.ts +++ b/src/formatters/helpers/previewHelpers.ts @@ -1,6 +1,7 @@ /** * Shared utilities for generating realistic preview examples in display formatters */ +import { findIllegalFilePathChars } from "../../utils/generatedFilePath"; /** Common variable examples for consistent previews across formatters */ export const VARIABLE_EXAMPLES: Record = { @@ -83,6 +84,21 @@ export function getSuggestionPreview(suggestedValues: string[]): string { return "suggestion_list"; } +/** + * A file-name preview stand-in, or a neutral one when it could not be a name. + * + * Most stand-ins echo the token's own argument - `{{VALUE:Cost: USD}}` previews + * `Cost: USD_value`, `{{MACRO:a:b}}` previews `a:b_output` - and at run time + * that argument is a PROMPT HEADER or a macro name, not part of the file name. + * So the echoed colon is the preview's own invention, and the illegal-character + * diagnostic that reads the finished preview would blame the author for a + * character the run never produces. A stand-in is fiction either way; fiction + * that could not be a real file name is worse fiction (#1578). + */ +export function fileNameSafeStandIn(standIn: string, neutral: string): string { + return findIllegalFilePathChars(standIn).length > 0 ? neutral : standIn; +} + /** * The stand-in a preview shows for a `{{FIELD:...}}` token. * diff --git a/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts b/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts new file mode 100644 index 000000000..8b3d07e5a --- /dev/null +++ b/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it, vi } from "vitest"; + +vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); + +import { App } from "obsidian"; +import { render } from "@testing-library/svelte"; +import { tick } from "svelte"; +import type QuickAdd from "../../../main"; +import type ICaptureChoice from "../../../types/choices/ICaptureChoice"; +import CaptureTargetSetting from "./CaptureTargetSetting.svelte"; + +/** + * Pins the gate that keeps #1578's illegal-character diagnostic off the capture + * target field. + * + * `FileNameDisplayFormatter` previews FILE NAMES, and a colon in a name is + * fatal - so it reports one for `property:status=done` too, which is capture + * TARGET syntax and not a path at all. Teaching that formatter capture semantics + * would be the wrong layer (the same class previews a Template choice's file + * name, where a literal `property:x=y` IS a path and the colon IS the problem), + * so the surface is what knows the difference: this component renders no + * preview row while the field holds recognised picker syntax. + * + * Without a test the gate is one `{#if}` away from silently disappearing and + * putting a wrong red error under every property/tag capture target. + */ +const plugin = { + getTemplateFiles: () => [], + settings: { choices: [], globalVariables: {} }, +} as unknown as QuickAdd; + +function captureChoice(captureTo: string): ICaptureChoice { + return { + id: "c1", + name: "My Capture", + type: "Capture", + command: false, + captureTo, + captureToActiveFile: false, + captureToCanvasNodeId: "", + activeFileWritePosition: "cursor", + createFileIfItDoesntExist: { + enabled: false, + createWithTemplate: false, + template: "", + }, + format: { enabled: false, format: "" }, + prepend: false, + appendLink: false, + task: false, + insertAfter: { + enabled: false, + after: "", + insertAtEnd: false, + considerSubsections: false, + createIfNotFound: false, + createIfNotFoundLocation: "top", + }, + newLineCapture: { enabled: false, direction: "below" }, + openFile: false, + fileOpening: { + location: "tab", + direction: "vertical", + mode: "default", + focus: true, + }, + } as ICaptureChoice; +} + +async function renderTarget(captureTo: string) { + const { container } = render(CaptureTargetSetting, { + props: { choice: captureChoice(captureTo), app: new App(), plugin }, + }); + await tick(); + await tick(); + return container; +} + +describe("#1578 the capture target's picker syntax gets no file-name preview", () => { + it.each([ + ["a property target", "property:status=done"], + ["a tag filter target", "tag:#inbox"], + ["a folder filter target", "folder:Work"], + ["a bare tag target", "#inbox"], + ])("renders no preview row for %s", async (_label, captureTo) => { + const container = await renderTarget(captureTo); + expect(container.querySelector(".qa-preview-row")).toBeNull(); + expect(container.querySelector(".qa-preview-issue")).toBeNull(); + }); + + it("still previews an ordinary path target", async () => { + const container = await renderTarget("Inbox.md"); + expect(container.querySelector(".qa-preview-row")).not.toBeNull(); + }); +}); diff --git a/src/utils/generatedFilePath.ts b/src/utils/generatedFilePath.ts index f6bcd2171..97a0f6e86 100644 --- a/src/utils/generatedFilePath.ts +++ b/src/utils/generatedFilePath.ts @@ -63,6 +63,64 @@ function trimTrailingCharsLinear(value: string, chars: string): string { return value.slice(0, end); } +/** + * Characters Obsidian itself refuses inside a path segment. + * + * MEASURED against `vault.create` / `vault.createFolder` on Obsidian 1.13.0 + * (macOS), one candidate character per name: + * + * - `:` throws Obsidian's own guard, "File name cannot contain any of the + * following characters: \ / :", for BOTH files and folder segments. + * - `\` and `/` never reach a segment: Obsidian's `normalizePath` treats them as + * separators, and {@link normalizeGeneratedFilePathCore} converts `\` to `/` + * before this check for exactly that reason. QuickAdd then creates the parent + * folder (QuickAddEngine.createFileWithInput), so they are legal here. + * - `* ? " < > | ^ [ ] #` and tab all CREATE SUCCESSFULLY on macOS/Linux, so + * copying the stricter set from `TemplateEngine.validateFolderSegment` would + * reject names that Obsidian makes without complaint. (On Windows the + * filesystem rejects `* ? " < > |`; a platform-gated portability warning is + * deliberately not attempted from an untestable platform.) + * - Control characters and NUL are already collapsed away by the normalizer. + */ +const OBSIDIAN_ILLEGAL_PATH_CHARS = [":"] as const; + +/** + * The characters in `path` that Obsidian will refuse, in the order listed above. + * + * A plain `includes` scan per character: no regex, because every path here + * embeds untrusted format output ({{VALUE}}, clipboard, an included template + * body) and this runs on every keystroke of a preview. + */ +export function findIllegalFilePathChars(path: string): string[] { + return OBSIDIAN_ILLEGAL_PATH_CHARS.filter((char) => path.includes(char)); +} + +/** + * The sentence a preview shows for {@link findIllegalFilePathChars}' result. + * + * Two variants, keyed on whether the character is anywhere in the format string + * the author is looking at. When it is, naming the rule is enough - they can see + * what to change. When it is NOT, they have nothing to look for: `{{TIME}}` is + * `HH:mm`, and QuickAdd's own autocomplete offers it in this very field + * (formatTokenRegistry, `contexts: ALL`), so the message has to say that a token + * produced it. + * + * The rule comes BEFORE the explanation either way, so the three-line clamp on + * the inline diagnostic (styles.css `.qa-preview-issue`) can never cut off the + * part that says what is wrong (same reason as `describeUnknownFieldFilter`, + * #1564). + */ +export function describeIllegalFilePathChars( + chars: readonly string[], + { visibleInFormat }: { visibleInFormat: boolean }, +): string { + const quoted = chars.map((char) => `"${char}"`).join(", "); + const rule = `A file or folder name cannot contain ${quoted}.`; + return visibleInFormat + ? `${rule} Obsidian refuses it, so this choice would fail at run time.` + : `${rule} A token in this format resolves to one - {{TIME}} is the usual cause.`; +} + /** * The normalized path plus the reasons the strict entry point would have * rejected it. See {@link previewGeneratedFilePath}. From 8b51133d9cfe49ff496a5924734ff8231389c43d Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 12:55:05 +0200 Subject: [PATCH 4/5] fix: fold in the adversarial review of the #1578 diagnostic Three lenses attacked the shipped diff and a verifier reproduced each finding. - The capture target's picker-syntax gate read the RAW field, but the run resolves the target's format tokens BEFORE parsing it. So a target written as {{GLOBAL_VAR:inbox}} expanding to 'property:type=draft' passed the gate, was previewed as a path, and got a red 'cannot contain ":"' for a capture that runs perfectly well. FormatPreviewField takes a hideWhen predicate over the RESOLVED text, and CaptureTargetSetting asks the same parser again. - The two message variants are one. Splitting on 'is the colon anywhere in the format string' sounds right and is not: every argument-bearing token carries a colon in its own syntax, so {{DATE:YYYY-MM-DD}} {{TIME}} - the shape where the hint is needed most - was told the author could see it, and only a format whose tokens take no argument at all reached the other variant. One sentence names both sources. - A half-typed 'js quickadd' fence now suppresses the diagnostic the way a half-typed {{ does. Until the closing backticks exist there is no span to strip, so a script holding {a: 1} or "HH:mm" turned the row red on every pause. hasUnterminatedInlineScriptFence lives beside findInlineScriptSpans and shares its opener rules. - The {{MATH:}} pin from the #1580 commit was itself the fiction #1580 exists to delete: {{MATH:...}} is not a token (MATH_VALUE_REGEX is /{{MVALUE}}/i), so preview and run agree on it. The case now pins {{MVALUE}}, which is the real divergence, and {{MATH:1+1}} is kept as plain text with the reason. Issue #1587 was corrected to match. - The picker-preview component test asserted an absence that could not fail (diagnostics wait for 500ms of stillness). It now advances the clock and carries a positive control, plus the token-expansion case above. - Added the {{TIME}}, {{FILE:}}, {{FILENAMECURRENT}} and {{GLOBAL_VAR:}} cases the rewritten test file's docstring claimed, and gave its formatter the target folder every real caller sets. Accepted and documented rather than fixed: a fence carried in by a {{GLOBAL_VAR:}} snippet is stripped from the scan although the run would keep it as literal text - mapping spans back through the passes is not worth it for a script inside a global variable inside a file name, and the failure is silence rather than a wrong accusation. --- ...ameDisplayFormatter-1563-normalize.test.ts | 2 +- ...NameDisplayFormatter-1563-template.test.ts | 7 +- ...isplayFormatter-1578-illegal-chars.test.ts | 38 +++++++---- .../fileNameDisplayFormatter.test.ts | 68 ++++++++++++++++--- src/formatters/fileNameDisplayFormatter.ts | 23 +++++-- src/formatters/formatter.ts | 37 ++++++++++ ...eTargetSetting-1578-picker-preview.test.ts | 39 +++++++++-- .../components/CaptureTargetSetting.svelte | 16 ++++- .../components/FormatPreviewField.svelte | 20 +++++- src/utils/generatedFilePath.ts | 31 ++++----- 10 files changed, 220 insertions(+), 61 deletions(-) diff --git a/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts b/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts index 07937b466..9394c6d49 100644 --- a/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts @@ -109,7 +109,7 @@ describe("the file-name preview mirrors the run's name normalizer", () => { { severity: "error", message: - 'A file or folder name cannot contain ":". Obsidian refuses it, so this choice would fail at run time.', + '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.', }, ]); }); diff --git a/src/formatters/fileNameDisplayFormatter-1563-template.test.ts b/src/formatters/fileNameDisplayFormatter-1563-template.test.ts index 10659bb77..063bdef52 100644 --- a/src/formatters/fileNameDisplayFormatter-1563-template.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1563-template.test.ts @@ -148,13 +148,10 @@ describe("#1563 the file-name preview resolves {{TEMPLATE:}}", () => { }, { // The frontmatter's "title: x" is now IN the name, so the name has a - // colon in it and Obsidian would refuse it (#1578). The - // "Obsidian refuses it" variant, because a colon IS visible in the - // field - `{{TEMPLATE:Body.md}}` has one, even though that is not the - // one that landed in the name. + // colon in it and Obsidian would refuse it (#1578). severity: "error", message: - 'A file or folder name cannot contain ":". Obsidian refuses it, so this choice would fail at run time.', + '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.', }, ]); }); diff --git a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts index 76d77298d..08ce23d91 100644 --- a/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts @@ -101,10 +101,15 @@ async function preview(input: string) { return { text, diagnostics: formatter.diagnostics.list() }; } -const TYPED = - 'A file or folder name cannot contain ":". Obsidian refuses it, so this choice would fail at run time.'; -const FROM_TOKEN = - 'A file or folder name cannot contain ":". A token in this format resolves to one - {{TIME}} is the usual cause.'; +/** + * One sentence, naming both places the character can come from. An earlier draft + * split it on "is the colon anywhere in the format string", which sounds right + * and is not: every argument-bearing token carries a colon in its own syntax, so + * `{{DATE:YYYY-MM-DD}} {{TIME}}` - where the hint is needed most - would be told + * the author can see it. + */ +const REFUSED = + '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.'; describe("the file-name preview says when Obsidian will refuse the name", () => { it("flags the colon the author typed - the reported case", async () => { @@ -112,25 +117,24 @@ describe("the file-name preview says when Obsidian will refuse the name", () => // Still shows the best-effort name: the diagnostic is what says it is // unusable, and blanking the row would hide the shape of the mistake. expect(text).toBe("Bad: Example Title"); - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); it("flags a colon in a folder segment, which Obsidian refuses too", async () => { const { diagnostics } = await preview("Bad: folder/{{VALUE:title}}"); - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); it("flags {{TIME}}, which is HH:mm and which the author never typed", async () => { const { text, diagnostics } = await preview("Meeting {{TIME}}"); expect(text).toBe("Meeting 14:30"); - expect(diagnostics).toEqual([{ severity: "error", message: FROM_TOKEN }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); it("flags a time format inside {{DATE:}}", async () => { const { text, diagnostics } = await preview("Log {{DATE:HH:mm}}"); expect(text).toBe("Log 14:30"); - // A colon IS visible in the field here, inside the token. - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); it("flags a colon a global snippet brought in", async () => { @@ -139,14 +143,14 @@ describe("the file-name preview says when Obsidian will refuse the name", () => "{{GLOBAL_VAR:prefix}}{{VALUE:title}}", ); expect(text).toBe("Meeting: Example Title"); - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); it("flags a colon an included template body brought in", async () => { templates["Naming.md"] = "Meeting: notes\n"; const { text, diagnostics } = await preview("{{TEMPLATE:Naming.md}}"); expect(text).toBe("Meeting: notes"); - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); it("flags a token that never matched and went to the vault verbatim", async () => { @@ -156,7 +160,7 @@ describe("the file-name preview says when Obsidian will refuse the name", () => // {{TEMPLATE:}} typo. const { text, diagnostics } = await preview("{{TEMPLATE:Naming}}"); expect(text).toBe("{{TEMPLATE:Naming}}"); - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); }); @@ -179,6 +183,14 @@ describe("the file-name preview does not cry wolf", () => { expect(diagnostics).toEqual([]); }); + it("stays quiet mid-SCRIPT, before the closing backticks are typed", async () => { + // Until the fence closes there is no span to strip, so the half-written + // JavaScript is read as part of the name. Without the guard, every pause + // while typing `{a: 1}` or a "HH:mm" literal turns the row red. + const { diagnostics } = await preview('```js quickadd\nreturn "a: b";'); + expect(diagnostics).toEqual([]); + }); + it("stays quiet about punctuation inside an inline script fence", async () => { // The run replaces the fence with what the script RETURNS; the preview // leaves the source verbatim because it must not execute anything (#1558). @@ -242,6 +254,6 @@ describe("the file-name preview does not cry wolf", () => { // name if it is the one picked, and then it cannot be created. const { text, diagnostics } = await preview("{{VALUE:Meeting: standup,Note}}"); expect(text).toBe("Meeting: standup"); - expect(diagnostics).toEqual([{ severity: "error", message: TYPED }]); + expect(diagnostics).toEqual([{ severity: "error", message: REFUSED }]); }); }); diff --git a/src/formatters/fileNameDisplayFormatter.test.ts b/src/formatters/fileNameDisplayFormatter.test.ts index 8be80fa55..5f46f5687 100644 --- a/src/formatters/fileNameDisplayFormatter.test.ts +++ b/src/formatters/fileNameDisplayFormatter.test.ts @@ -38,7 +38,14 @@ function makeApp(): App { return { workspace: { getActiveFile: () => activeFile }, vault: { - getMarkdownFiles: () => [], + getMarkdownFiles: () => [ + Object.assign(new TFile(), { + path: "Templates/Daily.md", + extension: "md", + basename: "Daily", + parent: { path: "Templates" }, + }), + ], getAbstractFileByPath: (path: string) => path in templates ? Object.assign(new TFile(), { @@ -54,12 +61,17 @@ function makeApp(): App { } const plugin = { - settings: { globalVariables: {}, choices: [] }, + settings: { globalVariables: { prefix: "Draft " }, choices: [] }, getTemplateFiles: () => [], } as unknown as QuickAdd; function makeFormatter(): FileNameDisplayFormatter { - return new FileNameDisplayFormatter(makeApp(), plugin); + const formatter = new FileNameDisplayFormatter(makeApp(), plugin); + // Every real caller sets this; leaving it unset makes {{FOLDER}} collapse to + // an empty path segment (FormatPreviewField passes the choice's folder, or a + // "Folder/Name" placeholder). + formatter.setTargetFolderPath("Folder/Name"); + return formatter; } async function preview(input: string) { @@ -111,6 +123,30 @@ describe("FileNameDisplayFormatter resolves the tokens a file name can hold", () expect(text).toBe("test/Note"); }); + it("previews {{FILENAMECURRENT}} as the active file's name", async () => { + const { text } = await preview("Re {{FILENAMECURRENT}}"); + expect(text).toBe("Re example"); + }); + + it("previews {{TIME}}", async () => { + // The stub moment returns one string for every format, so this can only + // assert that the pass RAN. What {{TIME}} really renders (HH:mm, colon + // and all) is pinned with real moment in + // fileNameDisplayFormatter-1578-illegal-chars.test.ts. + const { text } = await preview("At {{TIME}}"); + expect(text).not.toContain("{{TIME}}"); + }); + + it("previews {{FILE:folder}} as a file from that folder", async () => { + const { text } = await preview("{{FILE:Templates}}"); + expect(text).toBe("Daily"); + }); + + it("expands a {{GLOBAL_VAR:}} snippet", async () => { + const { text } = await preview("{{GLOBAL_VAR:prefix}}Note"); + expect(text).toBe("Draft Note"); + }); + it("reads a {{TEMPLATE:}} body inertly", async () => { const { text, diagnostics } = await preview("{{TEMPLATE:Templates/Daily.md}}"); expect(text).toBe("Daily body"); @@ -147,16 +183,26 @@ describe("FileNameDisplayFormatter resolves the tokens a file name can hold", () describe("tokens the file-name preview does NOT resolve today", () => { /** - * Both pinned as CURRENT behaviour with an issue number, not as desired - * behaviour. The old mock in this file asserted the opposite for {{MATH:}} - * ("File calculation_result") - which is exactly the kind of claim a test - * that mocks itself can make forever without anyone noticing. + * Pinned as CURRENT behaviour with the issue each is filed as, not as + * desired behaviour. */ - it("leaves {{MATH:}} literal even though the run resolves it (#1587)", async () => { + it("leaves {{MVALUE}} literal even though the run prompts for it (#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 prompt - // here. Neither display formatter has the pass, though both override - // `promptForMathValue` with a stand-in that is therefore unreachable. + // 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}}"); + }); + + 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. + // `{{MATH:...}}` matches no regex in QuickAdd; the run puts the literal + // text in the name, and Obsidian then refuses it over the colon - which + // is pinned in fileNameDisplayFormatter-1578-illegal-chars.test.ts. const { text } = await preview("File {{MATH:1+1}}"); expect(text).toBe("File {{MATH:1+1}}"); }); diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index 2a2fe7042..21fa25a04 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -1,6 +1,7 @@ import { findInlineScriptSpans, Formatter, + hasUnterminatedInlineScriptFence, type PromptContext, } from "./formatter"; import { @@ -66,6 +67,13 @@ function hasUnterminatedToken(input: string): boolean { * So the fence's own punctuation is never in the created name, and reading the * preview literally there would report a colon out of somebody's JavaScript. * Same helper and the same reason as the template pass above (#1467). + * + * Known and accepted gap: a fence that only APPEARS after expansion - carried in + * by a `{{GLOBAL_VAR:}}` snippet, whose pass runs after the run's inline-JS pass + * has already gone by - is stripped here although the run would keep it as + * literal text. Mapping spans back through the passes to tell the two apart is + * not worth it for a script inside a global variable inside a file name, and the + * failure is silence rather than a wrong accusation. */ function textOutsideScriptSpans(text: string): string { const spans = findInlineScriptSpans(text); @@ -184,16 +192,17 @@ export class FileNameDisplayFormatter extends Formatter { // and the unmatched token stays literal in the output - so the colon is // the caret's position, not a mistake. Costs only a literal `{{` in a // name, which no format string has. - if (hasUnterminatedToken(input)) return; + // ...and mid-SCRIPT, for the same reason: until the closing backticks are + // typed there is no span to strip, so the half-written JavaScript is read + // as part of the name, and `{a: 1}` or `"HH:mm"` in it turns the row red + // on every pause. + if (hasUnterminatedToken(input) || hasUnterminatedInlineScriptFence(input)) { + return; + } const illegal = findIllegalFilePathChars(textOutsideScriptSpans(name)); if (illegal.length === 0) return; - this.reportProblem( - describeIllegalFilePathChars(illegal, { - visibleInFormat: - findIllegalFilePathChars(textOutsideScriptSpans(input)).length > 0, - }), - ); + this.reportProblem(describeIllegalFilePathChars(illegal)); } /** diff --git a/src/formatters/formatter.ts b/src/formatters/formatter.ts index 65c5c31ce..3217dc38b 100644 --- a/src/formatters/formatter.ts +++ b/src/formatters/formatter.ts @@ -143,6 +143,43 @@ export function findInlineScriptSpans( return spans; } +/** + * Has an inline script fence been OPENED without a closing backtick run? + * + * `findInlineScriptSpans` reports only complete fences, which is right for the + * passes that skip over script source - a half-written fence is not a script + * yet. A live preview needs the other half of that fact: while the closing + * backticks are missing, the script's own text is being read as content, so a + * preview that judges the result is judging somebody's half-typed JavaScript + * (#1578). + * + * Shares the opener rules with the scanner above rather than re-deriving them, + * and stops at the first unterminated opener for the same reason the scanner + * does: no later opener can match, because its backticks would have closed this + * one. + */ +export function hasUnterminatedInlineScriptFence(input: string): boolean { + const spans = findInlineScriptSpans(input); + const n = input.length; + let i = spans.length > 0 ? spans[spans.length - 1].end : 0; + + while (i < n) { + const runStart = input.indexOf("`", i); + if (runStart === -1) return false; + let runEnd = runStart; + while (runEnd < n && input[runEnd] === "`") runEnd++; + + if ( + runEnd - runStart >= 3 && + input.startsWith(INLINE_SCRIPT_FENCE_LANG, runEnd) + ) { + return true; + } + i = runEnd; + } + return false; +} + export abstract class Formatter { protected value: string; protected variables: Map = new Map(); diff --git a/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts b/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts index 8b3d07e5a..95c541b40 100644 --- a/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts +++ b/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); @@ -26,7 +26,12 @@ import CaptureTargetSetting from "./CaptureTargetSetting.svelte"; */ const plugin = { getTemplateFiles: () => [], - settings: { choices: [], globalVariables: {} }, + settings: { + choices: [], + // The run resolves a capture target's format tokens BEFORE parsing it, so + // picker syntax can arrive from a snippet rather than being typed. + globalVariables: { inbox: "property:type=draft" }, + }, } as unknown as QuickAdd; function captureChoice(captureTo: string): ICaptureChoice { @@ -67,10 +72,19 @@ function captureChoice(captureTo: string): ICaptureChoice { } as ICaptureChoice; } +beforeEach(() => { + vi.useFakeTimers(); +}); +afterEach(() => { + vi.useRealTimers(); +}); + async function renderTarget(captureTo: string) { const { container } = render(CaptureTargetSetting, { props: { choice: captureChoice(captureTo), app: new App(), plugin }, }); + // The preview resolves asynchronously; the row mounts empty and is filled. + await vi.advanceTimersByTimeAsync(0); await tick(); await tick(); return container; @@ -82,14 +96,31 @@ describe("#1578 the capture target's picker syntax gets no file-name preview", ( ["a tag filter target", "tag:#inbox"], ["a folder filter target", "folder:Work"], ["a bare tag target", "#inbox"], + [ + "picker syntax a token expands to", + "{{GLOBAL_VAR:inbox}}", + ], ])("renders no preview row for %s", async (_label, captureTo) => { const container = await renderTarget(captureTo); expect(container.querySelector(".qa-preview-row")).toBeNull(); - expect(container.querySelector(".qa-preview-issue")).toBeNull(); }); it("still previews an ordinary path target", async () => { const container = await renderTarget("Inbox.md"); - expect(container.querySelector(".qa-preview-row")).not.toBeNull(); + expect(container.querySelector(".qa-preview-row")?.textContent).toContain( + "Inbox.md", + ); + }); + + it("still reports an impossible PATH target - the positive control", async () => { + // Without this the suite could not tell "the gate works" from "this + // component never shows a diagnostic": the row's problems are held back + // until the field has been still for DIAGNOSTICS_IDLE_MS. + const container = await renderTarget("Bad: name.md"); + await vi.advanceTimersByTimeAsync(600); + await tick(); + expect(container.querySelector(".qa-preview-issue")?.textContent).toContain( + 'cannot contain ":"', + ); }); }); diff --git a/src/gui/ChoiceBuilder/components/CaptureTargetSetting.svelte b/src/gui/ChoiceBuilder/components/CaptureTargetSetting.svelte index 0ac8f382b..bbddd4827 100644 --- a/src/gui/ChoiceBuilder/components/CaptureTargetSetting.svelte +++ b/src/gui/ChoiceBuilder/components/CaptureTargetSetting.svelte @@ -55,6 +55,14 @@ const captureTargetFeedback = $derived.by(() => // Filter/property targets are not paths, so showing the file-name format preview // would render a misleading fake path. const usesPickerTargetSyntax = $derived(captureTargetFeedback !== null); +// The same question, asked of the RESOLVED text. The gate above reads the raw +// field, but the run resolves the target's format tokens BEFORE parsing it +// (CaptureChoiceEngine formats captureTo, then resolveCaptureTarget), so a +// target written as `{{GLOBAL_VAR:inbox}}` that expands to `property:type=draft` +// passes the raw gate and then gets previewed as a path - complete with #1578's +// "cannot contain a colon" error for a capture that runs perfectly well. +const isPickerTargetSyntax = (resolved: string) => + getCaptureTargetFeedback(resolved) !== null; // Exclude picker syntax from canvas detection so a contrived value like // `property:type=foo.canvas` never offers the (meaningless) canvas-node picker. const isCanvasTarget = $derived( @@ -149,7 +157,13 @@ function validateCaptureTo(value: string) { /> {#if !usesPickerTargetSyntax} - + {/if} {/snippet} diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index e89d080c0..c39fdf781 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -22,6 +22,7 @@ let { app, plugin, targetFolderPath, + hideWhen, }: { value: string; /** @@ -45,6 +46,18 @@ let { * in a folder, so the placeholder would invent a path the script cannot get. */ targetFolderPath?: string | null; + /** + * Asked of the RESOLVED preview text: does this field's host consider the + * result something other than the thing this row previews? When it answers + * yes the row does not render at all. + * + * Exists for the capture target, whose value can be a path OR picker syntax + * (`property:type=draft`), and which the run resolves BEFORE deciding which. + * The host can gate on the raw field itself, but not on what a token expands + * to - and previewing picker syntax as a path invents a fake path and, since + * #1578, an illegal-character error for a capture that runs fine. + */ + hideWhen?: (resolvedPreview: string) => boolean; } = $props(); /** How long the field must sit still before its problems are shown. */ @@ -61,6 +74,11 @@ let previewToken = 0; // populated, and the announcement would be lost. const hasValue = $derived(value.trim().length > 0); +// Gated on the RESOLVED text, so it can only be answered once the async pass +// has produced one. Until then `preview` is "" and the row mounts empty, which +// is what `aria-live` needs anyway. +const hidden = $derived(hideWhen ? hideWhen(preview) : false); + // 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. @@ -138,7 +156,7 @@ $effect(() => { }); -{#if hasValue} +{#if hasValue && !hidden}