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-1563-normalize.test.ts b/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts index 251b66856..a73af366f 100644 --- a/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1563-normalize.test.ts @@ -57,6 +57,7 @@ describe("the file-name preview mirrors the run's name normalizer", () => { expect(problems).toEqual([ { severity: "error", + kind: "path", message: 'File path cannot contain "." or ".." path segments.', }, ]); @@ -70,6 +71,7 @@ describe("the file-name preview mirrors the run's name normalizer", () => { expect(problems).toEqual([ { severity: "error", + kind: "path", message: "File path contains an empty path segment after formatting.", }, ]); @@ -87,9 +89,31 @@ 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", + 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.', + }, + ]); + }); }); diff --git a/src/formatters/fileNameDisplayFormatter-1563-template.test.ts b/src/formatters/fileNameDisplayFormatter-1563-template.test.ts index 61d62194e..6751e05d3 100644 --- a/src/formatters/fileNameDisplayFormatter-1563-template.test.ts +++ b/src/formatters/fileNameDisplayFormatter-1563-template.test.ts @@ -146,6 +146,14 @@ 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). + 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.', + }, ]); }); 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..d3fc7c56a --- /dev/null +++ b/src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts @@ -0,0 +1,284 @@ +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 = {}; + existingFiles = new Set(); +}); + +let templates: Record = {}; +let globalVariables: Record = {}; +let existingFiles = new Set(); + +function makeApp(): App { + return { + workspace: { + getActiveFile: () => ({ + basename: "example", + path: "test/example.md", + parent: { path: "test" }, + }), + }, + vault: { + getMarkdownFiles: () => [], + getAbstractFileByPath: (path: string) => + path in templates || existingFiles.has(path) + ? 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() }; +} + +/** + * 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 () => { + 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", kind: "path", 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", kind: "path", 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", kind: "path", 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"); + expect(diagnostics).toEqual([{ severity: "error", kind: "path", message: REFUSED }]); + }); + + 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", kind: "path", 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", kind: "path", message: REFUSED }]); + }); + + 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", kind: "path", message: REFUSED }]); + }); +}); + +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 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). + // 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("stays quiet when the file is already there", async () => { + // `:` is legal on macOS/Linux at the filesystem level, so a note made + // outside Obsidian can carry one. A capture pointed at it appends + // (CaptureChoiceEngine takes the fileExists branch and never reaches + // vault.create), so nothing asks Obsidian to accept the name. + existingFiles.add("Notes/a:b.md"); + const { diagnostics } = await preview("Notes/a:b.md"); + expect(diagnostics).toEqual([]); + }); + + it("tolerates the missing extension a file-name format leaves off", async () => { + // The engine appends `.md` (normalizeMarkdownFilePath), so the preview's + // name and the file on disk differ by the extension. + existingFiles.add("Notes/a:b.md"); + const { diagnostics } = await preview("Notes/a:b"); + expect(diagnostics).toEqual([]); + }); + + 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 }]); + }); + + 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", kind: "path", message: REFUSED }]); + }); +}); 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.test.ts b/src/formatters/fileNameDisplayFormatter.test.ts index 0340949c8..5f46f5687 100644 --- a/src/formatters/fileNameDisplayFormatter.test.ts +++ b/src/formatters/fileNameDisplayFormatter.test.ts @@ -1,104 +1,217 @@ -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: () => [ + 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(), { + 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: { prefix: "Draft " }, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +function makeFormatter(): FileNameDisplayFormatter { + 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) { + const formatter = makeFormatter(); + const text = await formatter.format(input); + return { text, diagnostics: formatter.diagnostics.list() }; +} + +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("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("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("previews {{VDATE:name,format}}", async () => { + const { text } = await preview("{{VDATE:dueDate, YYYY-MM-DD}}"); + expect(text).toMatch(/^\d{4}-\d{2}-\d{2}$/); + }); + + it("previews {{FIELD:x}} as a value of that field", async () => { + const { text } = await preview("{{FIELD:category}}"); + expect(text).toBe("category_field_value"); + }); + + it("previews {{SELECTED}} and {{CLIPBOARD}} without reading either", async () => { + const { text } = await preview("{{SELECTED}} {{CLIPBOARD}}"); + expect(text).toBe("selected_text clipboard_content"); + }); + + it("previews {{RANDOM:n}}", async () => { + const { text } = await preview("{{RANDOM:4}}"); + expect(text).toBe("ABC1"); + }); - beforeEach(() => { - formatter = new TestFileNameDisplayFormatter(mockApp); + it("previews {{FOLDERCURRENT}} as the active file's folder", async () => { + const { text } = await preview("{{FOLDERCURRENT}}/Note"); + expect(text).toBe("test/Note"); }); - 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 {{FILENAMECURRENT}} as the active file's name", async () => { + const { text } = await preview("Re {{FILENAMECURRENT}}"); + expect(text).toBe("Re example"); }); - 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 {{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('should format filename with macros', async () => { - const result = await formatter.format('{{MACRO:clipboard}} - {{MACRO:uuid}}'); - expect(result).toBe('clipboard_content - unique_id'); + it("previews {{FILE:folder}} as a file from that folder", async () => { + const { text } = await preview("{{FILE:Templates}}"); + expect(text).toBe("Daily"); }); - it('should format filename with current file link', async () => { - const result = await formatter.format('Related to {{LINKTOCURRENT}}'); - expect(result).toBe('Related to example'); + it("expands a {{GLOBAL_VAR:}} snippet", async () => { + const { text } = await preview("{{GLOBAL_VAR:prefix}}Note"); + expect(text).toBe("Draft Note"); }); - 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("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 math expressions', async () => { - const result = await formatter.format('File {{MATH:1+1}}'); - expect(result).toBe('File calculation_result'); + 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 field variables', async () => { - const result = await formatter.format('{{FIELD:category}}'); - expect(result).toBe('category_field_value'); + 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('should handle selected text', async () => { - const result = await formatter.format('Note about {{SELECTED}}'); - expect(result).toBe('Note about selected_text'); + it("previews nothing for empty input", async () => { + const { text, diagnostics } = await preview(""); + expect(text).toBe(""); + expect(diagnostics).toEqual([]); }); - it('should handle templates', async () => { - const result = await formatter.format('{{TEMPLATE:daily-note}}'); - expect(result).toBe('[daily-note template content...]'); + 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", () => { + /** + * 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 () => { + // 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}}"); }); - it('should handle empty input', async () => { - const result = await formatter.format(''); - expect(result).toBe(''); + 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}}"); }); - 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([]); }); }); diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index f598acff5..d73c41c29 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -1,6 +1,7 @@ import { findInlineScriptSpans, Formatter, + hasUnterminatedInlineScriptFence, type PromptContext, } from "./formatter"; import { @@ -16,16 +17,21 @@ import { getMacroPreview, 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"; @@ -38,6 +44,50 @@ 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). + * + * 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); + 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, @@ -102,11 +152,92 @@ export class FileNameDisplayFormatter extends Formatter { // the run would abort on become diagnostics instead. const normalized = previewGeneratedFilePath(output); for (const problem of normalized.problems) { - this.diagnostics.add("error", problem); + // "path": the format resolved fine, the vault just will not take the + // result. A host that knows this field may not be a path at all (the + // capture target) discards exactly these. + this.diagnostics.add("error", problem, "path"); } + // 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. + * + * What keeps it from crying wolf: the preview's own stand-ins are kept + * name-shaped ({@link fileNameSafeStandIn}, and the VDATE hints are gone), + * plus the 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. + // ...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; + + // A file that is already there is never created, so Obsidian is never + // asked to accept its name. `:` is legal on macOS/Linux at the filesystem + // level, so a note made outside Obsidian really can carry one - and a + // capture pointed at it appends happily (CaptureChoiceEngine takes the + // `fileExists` branch and never reaches `vault.create`), as does a + // Template choice set to append/increment. Claiming otherwise would mark a + // working configuration broken. + if (this.existsInVault(name)) return; + + this.diagnostics.add("error", describeIllegalFilePathChars(illegal), "path"); + } + + /** + * 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. + */ + private existsInVault(name: string): boolean { + const vault = this.app?.vault; + // Defensive because this runs OUTSIDE format()'s try/catch: a preview that + // throws its way out of a keystroke is the #1558 failure, and this is the + // only vault call the pass makes. + if (typeof vault?.getAbstractFileByPath !== "function") return false; + const trimmed = name.trim(); + if (!trimmed) return false; + return Boolean( + vault.getAbstractFileByPath(trimmed) ?? + vault.getAbstractFileByPath(`${trimmed}.md`), + ); + } + /** * The preview pass list. * @@ -176,14 +307,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 { @@ -206,7 +340,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 { @@ -217,14 +354,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", + ); } /** @@ -366,8 +506,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 fileNameSafeStandIn(fieldValuePreview(parsed), "field_value"); } protected suggestForFile(parsed: ParsedFileToken): string { @@ -386,28 +529,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 @@ -415,23 +549,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}]`; + return `[${cleanDateFormat}]`; } - - // If there's a default value, indicate it in the preview - if (cleanDefaultValue) { - formattedExample += ` (default: ${cleanDefaultValue})`; - } - if (optional) { - formattedExample += ` (optional)`; - } - - return formattedExample; }); return output; 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..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(); @@ -1177,13 +1214,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 +1453,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..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,46 @@ 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. + * + * 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 */ diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index da605ebd2..608b64f91 100644 --- a/src/formatters/previewDiagnostics.ts +++ b/src/formatters/previewDiagnostics.ts @@ -2,9 +2,23 @@ import { isCancellationError } from "../utils/errorUtils"; export type PreviewDiagnosticSeverity = "warning" | "error"; +/** + * `"path"` marks a problem with the resulting PATH rather than with resolving + * the format: the name came out fine, the vault will just not accept it (a "." + * or ".." segment, #1563; a character Obsidian refuses, #1578). + * + * The distinction exists because a host can know the field is not a path at all + * - the capture target may hold picker syntax like `property:type=draft`, which + * the run parses before ever treating it as a file - and must then be able to + * discard exactly these while keeping every other diagnostic. See #1594 for the + * "Unresolved:" label, which wants the same split. + */ +export type PreviewDiagnosticKind = "path"; + export type PreviewDiagnostic = { severity: PreviewDiagnosticSeverity; message: string; + kind?: PreviewDiagnosticKind; }; /** @@ -21,13 +35,17 @@ export class PreviewDiagnostics { private readonly entries: PreviewDiagnostic[] = []; private readonly seen = new Set(); - add(severity: PreviewDiagnosticSeverity, message: string): void { + add( + severity: PreviewDiagnosticSeverity, + message: string, + kind?: PreviewDiagnosticKind, + ): void { const cleaned = stripBrandPrefix(message).trim(); if (!cleaned) return; const key = `${severity} ${cleaned}`; if (this.seen.has(key)) return; this.seen.add(key); - this.entries.push({ severity, message: cleaned }); + this.entries.push({ severity, message: cleaned, ...(kind ? { kind } : {}) }); } list(): readonly PreviewDiagnostic[] { 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..f2435a0d5 --- /dev/null +++ b/src/gui/ChoiceBuilder/components/CaptureTargetSetting-1578-picker-preview.test.ts @@ -0,0 +1,141 @@ +import { afterEach, beforeEach, 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: [], + // 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 { + 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; +} + +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; +} + +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"], + [ + "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(); + }); + + it("still previews an ordinary path target", async () => { + const container = await renderTarget("Inbox.md"); + expect(container.querySelector(".qa-preview-row")?.textContent).toContain( + "Inbox.md", + ); + }); + + it("shows the row when picker-looking text came out of a FAILED pass", async () => { + // `{{GLOBAL_VAR:inbox}}{{TEMPLATE:missing.md}}` resolves to picker syntax + // followed by a not-found placeholder, and the run aborts on the missing + // template. Hiding the row on the resolved text alone would take the one + // message that explains that with it. + const container = await renderTarget( + "{{GLOBAL_VAR:inbox}}{{TEMPLATE:missing.md}}", + ); + await vi.advanceTimersByTimeAsync(600); + await tick(); + expect(container.querySelector(".qa-preview-issue")?.textContent).toContain( + "Template not found", + ); + }); + + 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..ea49ed495 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,25 @@ 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. +// +// Never over an error about something OTHER than the path: a pass can fail and +// still leave text that looks like picker syntax - +// `{{GLOBAL_VAR:inbox}}{{TEMPLATE:missing.md}}` resolves to `property:type=draft` +// followed by a not-found placeholder, and the run aborts on the missing +// template. Hiding the row there would take the one message that explains it. +// The `kind: "path"` problems are the ones this host is entitled to discard: +// they say the result is not a usable path, which is exactly what it is not +// trying to be. +const hidden = $derived( + hideWhen + ? hideWhen(preview) && + !diagnostics.some((d) => d.severity === "error" && d.kind !== "path") + : 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 +170,7 @@ $effect(() => { }); -{#if hasValue} +{#if hasValue && !hidden}