From e29cc8de28eae2f35fe057591d5e0abd4993e94d Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 22:57:06 +0200 Subject: [PATCH 01/12] fix(format): warn once for anonymous {{VALUE|...}} option typos `parseAnonymousValueOptions` accepted a `quiet` flag and honoured it for the `|case:` warning, but then called `resolveInputType` without it and `resolveNumericInput` with a hardcoded `false`. So the deliberately-silent prompt-context pre-pass in `Formatter.getValuePromptContext` warned anyway, and every anonymous `{{VALUE|type:...}}` / `|min:` / `|max:` / `|step:` / slider typo produced two identical Notices per parse instead of one. Fix the class of bug, not just the instance: replace the optional `quiet: boolean` with a `WarnSink` - a `(message: string) => void` the caller supplies - and make it a REQUIRED parameter on `parseOptions`, `resolveInputType`, `buildNumericConfig` and `resolveNumericInput`. Forgetting to forward it is now a compile error rather than a silent duplicate Notice. A sink also expresses the three audiences a boolean cannot: loud (the real run), silent (pre-passes) and, next commit, collecting (the builders' live preview). `FieldSuggestionParser.parse` gains the same sink. `warnUnknown` stays as-is: it gates WHICH grammar may warn, the sink decides who hears it. Drive-by, on a line already being edited: the empty `|name:` warning was the one message here that never named its token, at run time as well as in preview. Refs #1558 --- src/formatters/formatter.ts | 11 +- src/utils/FieldSuggestionParser.ts | 6 +- .../valueSyntax.audit-formatter-core.test.ts | 11 +- src/utils/valueSyntax.test.ts | 7 +- src/utils/valueSyntax.ts | 174 +++++++++--------- src/utils/valueSyntax.warnSink.test.ts | 94 ++++++++++ src/utils/warnSink.ts | 28 +++ 7 files changed, 226 insertions(+), 105 deletions(-) create mode 100644 src/utils/valueSyntax.warnSink.test.ts create mode 100644 src/utils/warnSink.ts diff --git a/src/formatters/formatter.ts b/src/formatters/formatter.ts index a8c2af817..2af67a894 100644 --- a/src/formatters/formatter.ts +++ b/src/formatters/formatter.ts @@ -54,6 +54,7 @@ import { type SliderConfig, type ValueInputType, } from "../utils/valueSyntax"; +import { SILENT_WARN } from "../utils/warnSink"; import { parseVDateOptions } from "../utils/vdateSyntax"; import { applyDateSnap, parseDateSnapSegment } from "../utils/dateModifiers"; import { parseMacroToken } from "../utils/macroSyntax"; @@ -413,10 +414,12 @@ export abstract class Formatter { if (optionsIndex === -1) continue; const rawOptions = inner.slice(optionsIndex); - // Quiet: this is the prompt-context pre-pass; the actual replacer + // Silent: this is the prompt-context pre-pass; the actual replacer // pass (replaceValueInString) emits any |case warning so it fires // once, not twice. - const parsed = parseAnonymousValueOptions(rawOptions, { quiet: true }); + const parsed = parseAnonymousValueOptions(rawOptions, { + warn: SILENT_WARN, + }); if (!context) context = {}; if (!context.description && parsed.label) { @@ -985,8 +988,8 @@ export abstract class Formatter { if (!match[1]) continue; let parsed: ParsedValueToken | null; try { - // Quiet: the main pass parses again and owns the user-facing warnings. - parsed = parseValueToken(match[1], { quiet: true }); + // Silent: the main pass parses again and owns the user-facing warnings. + parsed = parseValueToken(match[1], { warn: SILENT_WARN }); } catch { // A malformed token throws in the main pass; abort hoisting so the // error surfaces before any suggester is shown. diff --git a/src/utils/FieldSuggestionParser.ts b/src/utils/FieldSuggestionParser.ts index 017c793ae..466baaff6 100644 --- a/src/utils/FieldSuggestionParser.ts +++ b/src/utils/FieldSuggestionParser.ts @@ -1,4 +1,4 @@ -import { log } from "../logger/logManager"; +import { NOTICE_WARN, type WarnSink } from "./warnSink"; import { parseBooleanFlag, parsePipeKeyValue, @@ -44,7 +44,7 @@ export class FieldSuggestionParser { */ static parse( input: string, - options?: { warnUnknown?: boolean }, + options?: { warnUnknown?: boolean; warn?: WarnSink }, ): { fieldName: string; filters: FieldFilter; @@ -161,7 +161,7 @@ export class FieldSuggestionParser { // false "Unknown FIELD filter" notices and leak internal // sentinels like __capture_scope. if (options?.warnUnknown) { - log.logWarning( + (options.warn ?? NOTICE_WARN)( `Unknown FIELD filter "${filterType}" in "{{FIELD:${input}}}" was ignored. Supported filters: folder, tag, inline, inline-code-blocks, exclude-folder, exclude-tag, exclude-file, default, default-from, default-empty, default-always, case-sensitive, multi.`, ); } diff --git a/src/utils/valueSyntax.audit-formatter-core.test.ts b/src/utils/valueSyntax.audit-formatter-core.test.ts index 1faf7c0fd..32c90551c 100644 --- a/src/utils/valueSyntax.audit-formatter-core.test.ts +++ b/src/utils/valueSyntax.audit-formatter-core.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { parseAnonymousValueOptions, parseValueToken } from "./valueSyntax"; +import { SILENT_WARN } from "./warnSink"; import { log } from "../logger/logManager"; describe("valueSyntax audit (formatter-core)", () => { @@ -48,12 +49,12 @@ describe("valueSyntax audit (formatter-core)", () => { ).toBe(false); }); - it("stays silent in quiet mode (preflight pre-pass)", () => { + it("stays silent with a silent sink (preflight pre-pass)", () => { const warnSpy = vi .spyOn(log, "logWarning") .mockImplementation(() => {}); - parseValueToken("title|case:keb", { quiet: true }); + parseValueToken("title|case:keb", { warn: SILENT_WARN }); expect(warnSpy).not.toHaveBeenCalled(); }); @@ -73,15 +74,15 @@ describe("valueSyntax audit (formatter-core)", () => { ).toBe(true); }); - it("stays silent on the anonymous form in quiet mode (prompt-context pre-pass)", () => { + it("stays silent on the anonymous form with a silent sink (prompt-context pre-pass)", () => { // The formatter calls parseAnonymousValueOptions twice per token (a - // quiet prompt-context pre-pass + the real replacer). Only the + // silent prompt-context pre-pass + the real replacer). Only the // replacer warns, so the |case typo notice fires once, not twice. const warnSpy = vi .spyOn(log, "logWarning") .mockImplementation(() => {}); - const parsed = parseAnonymousValueOptions("|case:keb", { quiet: true }); + const parsed = parseAnonymousValueOptions("|case:keb", { warn: SILENT_WARN }); expect(parsed.caseStyle).toBeUndefined(); expect(warnSpy).not.toHaveBeenCalled(); diff --git a/src/utils/valueSyntax.test.ts b/src/utils/valueSyntax.test.ts index d0ae45ca3..76dba8b21 100644 --- a/src/utils/valueSyntax.test.ts +++ b/src/utils/valueSyntax.test.ts @@ -7,6 +7,7 @@ import { parseValueToken, resolveExistingVariableKey, } from "./valueSyntax"; +import { SILENT_WARN } from "./warnSink"; import { log } from "../logger/logManager"; describe("parseValueToken", () => { @@ -385,10 +386,10 @@ describe("named variables (|name:, issue #148)", () => { expect(warnSpy).toHaveBeenCalled(); }); - it("stays silent when parsed in quiet mode", () => { + it("stays silent when parsed with a silent sink", () => { const warnSpy = vi.spyOn(log, "logWarning").mockImplementation(() => {}); - // Reserved name would normally warn; quiet mode (the pre-pass) suppresses it. - const parsed = parseValueToken("a,b|name:title", { quiet: true }); + // Reserved name would normally warn; a silent sink (the pre-pass) drops it. + const parsed = parseValueToken("a,b|name:title", { warn: SILENT_WARN }); expect(parsed?.aliasName).toBeUndefined(); expect(warnSpy).not.toHaveBeenCalled(); }); diff --git a/src/utils/valueSyntax.ts b/src/utils/valueSyntax.ts index 8d94ac756..ffd47fc6a 100644 --- a/src/utils/valueSyntax.ts +++ b/src/utils/valueSyntax.ts @@ -5,8 +5,8 @@ import { splitPipeParts, stripLeadingPipe, } from "./pipeSyntax"; -import { log } from "../logger/logManager"; import { isSupportedCaseStyle, SUPPORTED_CASE_STYLES } from "./caseTransform"; +import { NOTICE_WARN, SILENT_WARN, type WarnSink } from "./warnSink"; // Internal-only delimiter for scoping labeled VALUE lists. Unlikely to appear in user input. export const VALUE_LABEL_KEY_DELIMITER = "\u001F"; @@ -213,7 +213,8 @@ function parseOptions( optionParts: string[], hasOptions: boolean, allowName: boolean, - quiet = false, + tokenDisplay: string, + warn: WarnSink, ): ParsedOptions { if (optionParts.length === 0) { return { @@ -329,11 +330,13 @@ function parseOptions( break; case "name": // Gated to the named grammar via optionKeys; empty `|name:` is a - // no-op alias and warns so the author notices the typo. + // no-op alias and warns so the author notices the typo. Names the + // token like every other warning here — this was the one message + // that left the author guessing which token it meant. if (value) name = value; - else if (!quiet) - log.logWarning( - `QuickAdd: empty |name: ignored; provide a variable name, e.g. {{VALUE:a,b|name:category}}.`, + else + warn( + `QuickAdd: empty |name: ignored in "${tokenDisplay}"; provide a variable name, e.g. {{VALUE:a,b|name:category}}.`, ); break; default: @@ -367,24 +370,22 @@ function resolveInputType( hasOptions, allowCustomInput, }: { tokenDisplay: string; hasOptions: boolean; allowCustomInput: boolean }, - quiet = false, + warn: WarnSink, ): ValueInputType | undefined { if (!rawType) return undefined; const raw = rawType.trim().toLowerCase(); // `boolean` is a friendly alias for the checkbox true/false picker. const normalized = (raw === "boolean" ? "checkbox" : raw) as ValueInputType; if (!OPTION_INCOMPATIBLE_TYPES.has(normalized)) { - if (!quiet) - log.logWarning( - `QuickAdd: Unsupported VALUE type "${rawType}" in token "${tokenDisplay}". Supported types: multiline, number, slider, checkbox, text.`, - ); + warn( + `QuickAdd: Unsupported VALUE type "${rawType}" in token "${tokenDisplay}". Supported types: multiline, number, slider, checkbox, text.`, + ); return undefined; } if (hasOptions || allowCustomInput) { - if (!quiet) - log.logWarning( - `QuickAdd: Ignoring type:${normalized} for option-list VALUE token "${tokenDisplay}".`, - ); + warn( + `QuickAdd: Ignoring type:${normalized} for option-list VALUE token "${tokenDisplay}".`, + ); return undefined; } return normalized; @@ -409,7 +410,7 @@ function hasNumericConfig(options: ParsedOptions): boolean { function buildNumericConfig( options: ParsedOptions, tokenDisplay: string, - quiet: boolean, + warn: WarnSink, ): NumericInputConfig | undefined { const min = parseFiniteNumber(options.minRaw); const max = parseFiniteNumber(options.maxRaw); @@ -418,10 +419,9 @@ function buildNumericConfig( if (options.minRaw !== undefined) { if (min === undefined) { - if (!quiet) - log.logWarning( - `QuickAdd: Ignoring invalid min in VALUE token "${tokenDisplay}".`, - ); + warn( + `QuickAdd: Ignoring invalid min in VALUE token "${tokenDisplay}".`, + ); } else { config.min = min; } @@ -429,10 +429,9 @@ function buildNumericConfig( if (options.maxRaw !== undefined) { if (max === undefined) { - if (!quiet) - log.logWarning( - `QuickAdd: Ignoring invalid max in VALUE token "${tokenDisplay}".`, - ); + warn( + `QuickAdd: Ignoring invalid max in VALUE token "${tokenDisplay}".`, + ); } else { config.max = max; } @@ -443,20 +442,18 @@ function buildNumericConfig( config.max !== undefined && config.max < config.min ) { - if (!quiet) - log.logWarning( - `QuickAdd: Ignoring invalid numeric range in VALUE token "${tokenDisplay}"; max must be greater than or equal to min.`, - ); + warn( + `QuickAdd: Ignoring invalid numeric range in VALUE token "${tokenDisplay}"; max must be greater than or equal to min.`, + ); delete config.min; delete config.max; } if (options.stepRaw !== undefined) { if (step === undefined || step <= 0) { - if (!quiet) - log.logWarning( - `QuickAdd: Ignoring invalid step in VALUE token "${tokenDisplay}"; step must be greater than 0.`, - ); + warn( + `QuickAdd: Ignoring invalid step in VALUE token "${tokenDisplay}"; step must be greater than 0.`, + ); } else { config.step = step; } @@ -469,15 +466,15 @@ function resolveNumericInput( options: ParsedOptions, tokenDisplay: string, inputTypeOverride: ValueInputType | undefined, - quiet: boolean, + warn: WarnSink, ): { inputTypeOverride?: ValueInputType; numericConfig?: NumericInputConfig; sliderConfig?: SliderConfig; } { if (inputTypeOverride !== "number" && inputTypeOverride !== "slider") { - if (hasNumericConfig(options) && !quiet) { - log.logWarning( + if (hasNumericConfig(options)) { + warn( `QuickAdd: Ignoring numeric range options in "${tokenDisplay}" because type is not number or slider.`, ); } @@ -485,7 +482,7 @@ function resolveNumericInput( } if (inputTypeOverride !== "slider") { - const numericConfig = buildNumericConfig(options, tokenDisplay, quiet); + const numericConfig = buildNumericConfig(options, tokenDisplay, warn); return { inputTypeOverride, numericConfig }; } @@ -504,12 +501,12 @@ function resolveNumericInput( : undefined; if (invalidReason) { - if (!quiet) { - log.logWarning( - `QuickAdd: Invalid slider configuration in "${tokenDisplay}" (${invalidReason}); falling back to type:number.`, - ); - } - const numericConfig = buildNumericConfig(options, tokenDisplay, true); + warn( + `QuickAdd: Invalid slider configuration in "${tokenDisplay}" (${invalidReason}); falling back to type:number.`, + ); + // Silent: the fallback re-parses the same min/max/step and would repeat + // the complaint the slider message just made. + const numericConfig = buildNumericConfig(options, tokenDisplay, SILENT_WARN); return { inputTypeOverride: "number", numericConfig }; } @@ -681,10 +678,10 @@ export function unwrapQuotedValue(value: string): string { export function parseValueToken( raw: string, - opts?: { quiet?: boolean }, + opts?: { warn?: WarnSink }, ): ParsedValueToken | null { if (!raw) return null; - const quiet = opts?.quiet ?? false; + const warn = opts?.warn ?? NOTICE_WARN; const parts = splitPipeParts(raw); const variablePart = (parts.shift() ?? "").trim(); @@ -700,7 +697,8 @@ export function parseValueToken( optional: bareOptional, trim: bareTrim, } = extractBareValueFlags(parts); - const options = parseOptions(optionParts, hasOptions, true, quiet); + const tokenDisplay = `{{VALUE:${raw}}}`; + const options = parseOptions(optionParts, hasOptions, true, tokenDisplay, warn); let { label, caseStyle, defaultValue, allowCustomInput } = options; let multiSelect = options.multiSelect ?? false; const multiEmit: MultiEmit = options.multiEmit ?? "text"; @@ -720,7 +718,6 @@ export function parseValueToken( defaultValue = unwrapQuotedValue(defaultValue); } - const tokenDisplay = `{{VALUE:${raw}}}`; const inputTypeOverride = resolveInputType( options.inputTypeOverride, { @@ -728,13 +725,13 @@ export function parseValueToken( hasOptions, allowCustomInput, }, - quiet, + warn, ); const numericInput = resolveNumericInput( options, tokenDisplay, inputTypeOverride, - quiet, + warn, ); let displayValues: string[] | undefined; @@ -766,23 +763,21 @@ export function parseValueToken( if (aliasName && aliasName.includes(VALUE_LABEL_KEY_DELIMITER)) { // The delimiter is reserved for label-scoped keys; an alias containing it // would corrupt resolveExistingVariableKey's base-name stripping. - if (!quiet) - log.logWarning( - `QuickAdd: |name in "${tokenDisplay}" contains a reserved control character and was ignored.`, - ); + warn( + `QuickAdd: |name in "${tokenDisplay}" contains a reserved control character and was ignored.`, + ); aliasName = undefined; } if (aliasName && RESERVED_VALUE_NAMES.has(aliasName.toLowerCase())) { - if (!quiet) - log.logWarning( - `QuickAdd: |name:${aliasName} is reserved and was ignored in "${tokenDisplay}". Choose a different name.`, - ); + warn( + `QuickAdd: |name:${aliasName} is reserved and was ignored in "${tokenDisplay}". Choose a different name.`, + ); aliasName = undefined; } - if (aliasName && !hasOptions && !quiet) { + if (aliasName && !hasOptions) { // A named single value is just a renamed prompt; the option list is what // makes |name useful. Honor it but steer authors to the simpler form. - log.logWarning( + warn( `QuickAdd: |name on a single value in "${tokenDisplay}" is redundant — use {{VALUE:${aliasName}}} directly.`, ); } @@ -792,27 +787,24 @@ export function parseValueToken( // here — mirroring the |type: unsupported-value warning — so the author sees // their mistake instead of debugging an untransformed value. if (caseStyle && !isSupportedCaseStyle(caseStyle)) { - if (!quiet) - log.logWarning( - `QuickAdd: Unsupported |case style "${caseStyle}" in token "${tokenDisplay}". Supported styles: ${SUPPORTED_CASE_STYLES.join(", ")}.`, - ); + warn( + `QuickAdd: Unsupported |case style "${caseStyle}" in token "${tokenDisplay}". Supported styles: ${SUPPORTED_CASE_STYLES.join(", ")}.`, + ); caseStyle = undefined; } // |multi needs an option list and is incompatible with |case (a list is not // case-transformed, and routing an array through transformCase would throw). if (multiSelect && !hasOptions) { - if (!quiet) - log.logWarning( - `QuickAdd: |multi needs an option list (2+ comma-separated values) in "${tokenDisplay}"; ignoring.`, - ); + warn( + `QuickAdd: |multi needs an option list (2+ comma-separated values) in "${tokenDisplay}"; ignoring.`, + ); multiSelect = false; } if (multiSelect && caseStyle) { - if (!quiet) - log.logWarning( - `QuickAdd: |case is ignored with |multi in "${tokenDisplay}" — a list is not case-transformed.`, - ); + warn( + `QuickAdd: |case is ignored with |multi in "${tokenDisplay}" — a list is not case-transformed.`, + ); caseStyle = undefined; } @@ -826,10 +818,9 @@ export function parseValueToken( !options.usesOptions && optionParts.some((part) => part.trim().toLowerCase() === "custom") ) { - if (!quiet) - log.logWarning( - `QuickAdd: |custom needs an option list (2+ comma-separated values) in "${tokenDisplay}"; ignoring.`, - ); + warn( + `QuickAdd: |custom needs an option list (2+ comma-separated values) in "${tokenDisplay}"; ignoring.`, + ); if (defaultValue.toLowerCase() === "custom") defaultValue = ""; } @@ -861,7 +852,7 @@ export function parseValueToken( export function parseAnonymousValueOptions( rawOptions: string, - opts?: { quiet?: boolean }, + opts?: { warn?: WarnSink }, ): { label?: string; caseStyle?: string; @@ -872,7 +863,7 @@ export function parseAnonymousValueOptions( optional: boolean; trim: boolean; } { - const quiet = opts?.quiet ?? false; + const warn = opts?.warn ?? NOTICE_WARN; const normalized = stripLeadingPipe(rawOptions); const allParts = splitPipeParts(normalized) .map((part) => part.trim()) @@ -888,8 +879,8 @@ export function parseAnonymousValueOptions( return { defaultValue: "", optional: bareOptional, trim: bareTrim }; } - const options = parseOptions(parts, false, false); const tokenDisplay = `{{VALUE${rawOptions}}}`; + const options = parseOptions(parts, false, false, tokenDisplay, warn); if (options.displayValuesRaw !== undefined) { throw new Error( `QuickAdd: VALUE option "text" is only supported for option-list tokens in "${tokenDisplay}".`, @@ -901,27 +892,30 @@ export function parseAnonymousValueOptions( } // Warn on an unrecognized |case style (typo) so the anonymous form gets the - // same feedback as the named/single form (parseValueToken). Gated on quiet - // so the prompt-context pre-pass (which also calls this) does not double the - // notice — mirroring parseValueToken's quiet handling. + // same feedback as the named/single form (parseValueToken). Routed through + // the caller's sink so the prompt-context pre-pass (which also calls this) + // does not double the notice. if (caseStyle && !isSupportedCaseStyle(caseStyle)) { - if (!quiet) - log.logWarning( - `QuickAdd: Unsupported |case style "${caseStyle}" in token "${tokenDisplay}". Supported styles: ${SUPPORTED_CASE_STYLES.join(", ")}.`, - ); + warn( + `QuickAdd: Unsupported |case style "${caseStyle}" in token "${tokenDisplay}". Supported styles: ${SUPPORTED_CASE_STYLES.join(", ")}.`, + ); caseStyle = undefined; } - const inputTypeOverride = resolveInputType(options.inputTypeOverride, { - tokenDisplay, - hasOptions: false, - allowCustomInput: options.allowCustomInput, - }); + const inputTypeOverride = resolveInputType( + options.inputTypeOverride, + { + tokenDisplay, + hasOptions: false, + allowCustomInput: options.allowCustomInput, + }, + warn, + ); const numericInput = resolveNumericInput( options, tokenDisplay, inputTypeOverride, - false, + warn, ); return { diff --git a/src/utils/valueSyntax.warnSink.test.ts b/src/utils/valueSyntax.warnSink.test.ts new file mode 100644 index 000000000..823c570a9 --- /dev/null +++ b/src/utils/valueSyntax.warnSink.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it, vi } from "vitest"; +import { log } from "../logger/logManager"; +import { parseAnonymousValueOptions, parseValueToken } from "./valueSyntax"; +import { SILENT_WARN } from "./warnSink"; + +/** + * Issue #1558. `parseAnonymousValueOptions` accepted a `quiet` flag, honoured it + * for the `|case:` warning, and then forwarded nothing to `resolveInputType` and + * a hardcoded `false` to `resolveNumericInput`. So the deliberately-silent + * prompt-context pre-pass in `Formatter.getValuePromptContext` warned anyway, + * and every anonymous `{{VALUE|type:...}}` typo produced TWO Notices per parse. + * + * The sink is now a required parameter on those helpers, so the omission cannot + * come back silently. These tests pin the observable contract. + */ +describe("valueSyntax warn sink", () => { + function captureWarnings(run: () => void): string[] { + const seen: string[] = []; + const spy = vi + .spyOn(log, "logWarning") + .mockImplementation((m: string) => void seen.push(m)); + try { + run(); + } finally { + spy.mockRestore(); + } + return seen; + } + + it("routes |type:, |min:/|max:/|step: and slider warnings through the sink (anonymous form)", () => { + const cases = [ + "|type:numbr", + "|type:number|min:x", + "|type:number|max:x", + "|type:number|step:0", + "|type:number|min:5|max:1", + "|type:slider|min:1", + ]; + + for (const rawOptions of cases) { + const silent: string[] = []; + expect( + captureWarnings(() => + parseAnonymousValueOptions(rawOptions, { + warn: (m) => void silent.push(m), + }), + ), + `{{VALUE${rawOptions}}} must not reach the default Notice sink`, + ).toEqual([]); + expect( + silent.length, + `{{VALUE${rawOptions}}} should warn through the caller's sink`, + ).toBeGreaterThan(0); + } + }); + + it("stays completely silent on the anonymous form with SILENT_WARN", () => { + const warnings = captureWarnings(() => { + parseAnonymousValueOptions("|type:numbr", { warn: SILENT_WARN }); + parseAnonymousValueOptions("|type:slider|min:1", { warn: SILENT_WARN }); + parseAnonymousValueOptions("|case:pasc", { warn: SILENT_WARN }); + parseAnonymousValueOptions("|min:1|max:3", { warn: SILENT_WARN }); + }); + expect(warnings).toEqual([]); + }); + + it("warns exactly once per anonymous |type: typo when the sink is the default", () => { + const warnings = captureWarnings(() => + parseAnonymousValueOptions("|type:numbr"), + ); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('Unsupported VALUE type "numbr"'); + }); + + it("keeps the named form's sink wiring intact", () => { + const collected: string[] = []; + const warnings = captureWarnings(() => + parseValueToken("x|type:numbr|min:q", { + warn: (m) => void collected.push(m), + }), + ); + expect(warnings).toEqual([]); + expect(collected.some((m) => m.includes("Unsupported VALUE type"))).toBe( + true, + ); + }); + + it("names the token in the empty |name: warning", () => { + const collected: string[] = []; + parseValueToken("a,b|name:", { warn: (m) => void collected.push(m) }); + expect(collected).toHaveLength(1); + expect(collected[0]).toContain("{{VALUE:a,b|name:}}"); + }); +}); diff --git a/src/utils/warnSink.ts b/src/utils/warnSink.ts new file mode 100644 index 000000000..d8a3616ef --- /dev/null +++ b/src/utils/warnSink.ts @@ -0,0 +1,28 @@ +import { log } from "../logger/logManager"; + +/** + * Where a parser sends an authoring warning. + * + * The token parsers are shared by four callers with genuinely different needs: + * the runtime run (which owns the user-facing Notice), the preflight scan and + * the parsers' own pre-passes (which must stay silent so one mistake is not + * reported two or three times), and the builders' live preview (which re-parses + * on every keystroke and needs to *collect* the warnings, not fire them). + * + * A boolean `quiet` flag could express only the first two. It also made the + * omission at a call site invisible: `parseAnonymousValueOptions` accepted + * `{ quiet }`, honoured it for `|case:`, and then forwarded `false` to the + * `|type:`/`|min:`/`|max:`/`|step:` helpers, so the deliberately-quiet pre-pass + * warned anyway and every anonymous `{{VALUE|...}}` typo notified twice. Passing + * the sink as a REQUIRED parameter on the internal helpers turns that class of + * mistake into a compile error. + */ +export type WarnSink = (message: string) => void; + +/** The default: a user-facing warning (a Notice, via GuiLogger). */ +export const NOTICE_WARN: WarnSink = (message) => { + log.logWarning(message); +}; + +/** Drops the warning. For pre-passes whose main pass reports the same mistake. */ +export const SILENT_WARN: WarnSink = () => {}; From a7788237888c9a418191640dbcc3e9cd86a3516b Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 22:59:45 +0200 Subject: [PATCH 02/12] refactor(format): route formatter warnings through overridable hooks Every authoring warning the token parsers produce went straight to `log.logWarning`, i.e. straight to an Obsidian Notice, no matter who was parsing. `Formatter` now owns two overridable reporting hooks - `warn` and `reportProblem` - and hands the parsers a pre-bound `warnSink`. `CompleteFormatter` overrides neither, so the run-time contract is unchanged and stays pinned by formatter.audit-formatter-core.test.ts. The next commits give the preview and the preflight scan the overrides they should always have had. The sink is deliberately a pre-bound property rather than the `warn` method itself: the parsers call it detached, so `{ warn: this.warn }` would run a collecting override with `this === undefined`. Neither tsc (strict off) nor eslint (no type-aware config, so `unbound-method` is unavailable) would catch it, and the resulting TypeError is swallowed by `format()`'s catch. Also removes the bare `console.warn` beside the named-value conflict warning. Its comment claimed it was "retained for the diagnostic log/trail", but `ConsoleErrorLogger.logWarning` already console.warns every message (src/logger/consoleErrorLogger.ts:49), and it bypassed the hook - so once the preview stops firing Notices it would still have written one devtools line per keystroke. The three tests that asserted on it now assert on `log.logWarning`, which is the channel that actually reaches the user. Refs #1558 --- .../formatter-named-suggester.test.ts | 9 +-- src/formatters/formatter.ts | 67 +++++++++++++++---- 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/formatters/formatter-named-suggester.test.ts b/src/formatters/formatter-named-suggester.test.ts index a3c9f60b1..ea0265bea 100644 --- a/src/formatters/formatter-named-suggester.test.ts +++ b/src/formatters/formatter-named-suggester.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { Formatter, type PromptContext } from "./formatter"; +import { log } from "../logger/logManager"; /** * Exercises the REAL Formatter.replaceVariableInString (two-pass named-suggester @@ -157,7 +158,7 @@ describe("named suggester (#148)", () => { }); it("keeps the FIRST definition when two same-name defs have no preceding reuse", async () => { - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const warn = vi.spyOn(log, "logWarning").mockImplementation(() => {}); const f = new TestFormatter(); f.suggestReturns.set("type", "a"); @@ -179,7 +180,7 @@ describe("named suggester (#148)", () => { }); it("warns when the same name differs only by the custom flag", async () => { - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const warn = vi.spyOn(log, "logWarning").mockImplementation(() => {}); const f = new TestFormatter(); f.suggestReturns.set("kind", "a"); @@ -219,7 +220,7 @@ describe("named suggester (#148)", () => { }); it("warns and keeps the first value when a name is reused with different options", async () => { - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const warn = vi.spyOn(log, "logWarning").mockImplementation(() => {}); const f = new TestFormatter(); f.suggestReturns.set("type", "bug"); @@ -239,7 +240,7 @@ describe("named suggester (#148)", () => { }); it("does NOT warn when a seeded value is not one of the options (no false positive)", async () => { - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const warn = vi.spyOn(log, "logWarning").mockImplementation(() => {}); const f = new TestFormatter(); // Simulate api.format / a script / the one-page form pre-seeding the name // with a value outside the literal option list. diff --git a/src/formatters/formatter.ts b/src/formatters/formatter.ts index 2af67a894..835527075 100644 --- a/src/formatters/formatter.ts +++ b/src/formatters/formatter.ts @@ -54,7 +54,7 @@ import { type SliderConfig, type ValueInputType, } from "../utils/valueSyntax"; -import { SILENT_WARN } from "../utils/warnSink"; +import { SILENT_WARN, type WarnSink } from "../utils/warnSink"; import { parseVDateOptions } from "../utils/vdateSyntax"; import { applyDateSnap, parseDateSnapSegment } from "../utils/dateModifiers"; import { parseMacroToken } from "../utils/macroSyntax"; @@ -183,6 +183,42 @@ export abstract class Formatter { this.propertyCollector = new TemplatePropertyCollector(app); } + /** + * Where this formatter reports an authoring mistake in the user's format + * string (an unsupported `|case:` style, an unknown FIELD filter, ...). + * + * The default is the run-time contract: a user-facing Notice. Subclasses that + * are not the authoritative run override it — the builders' live preview + * collects them for passive display instead of stacking a Notice per + * keystroke, and the preflight scan drops them because the run that follows + * reports the same mistake (issue #1558). + */ + protected warn(message: string): void { + log.logWarning(message); + } + + /** + * Like {@link warn}, for a problem that stopped a token from resolving at all + * (a template inclusion cycle, exceeding the inclusion depth). Named + * `reportProblem` rather than `reportError` so it is not mistaken for the + * widely imported `errorUtils.reportError`. + */ + protected reportProblem(message: string): void { + log.logError(message); + } + + /** + * Pre-bound {@link warn}, for handing to the token parsers. + * + * Always pass THIS, never `this.warn`: the parsers call the sink detached, so + * a bare method reference would run an override with `this === undefined`. + * Class bodies are strict mode, so a collecting override would throw a + * TypeError — which `format()`'s catch swallows into "preview shows the raw + * input". Neither `tsc` (strict is off) nor eslint (no type-aware config, so + * `unbound-method` is unavailable) catches that, so the binding is the guard. + */ + protected readonly warnSink: WarnSink = (message) => this.warn(message); + protected abstract format(input: string): Promise; public setTemplateInclusionState(state: TemplateInclusionState): void { @@ -351,7 +387,9 @@ export abstract class Formatter { const optionsIndex = inner.indexOf("|"); if (optionsIndex === -1) return this.value; const rawOptions = inner.slice(optionsIndex); - const parsed = parseAnonymousValueOptions(rawOptions); + const parsed = parseAnonymousValueOptions(rawOptions, { + warn: this.warnSink, + }); // Apply |default on an empty submission, mirroring the named path // (ensureValueVariableResolved): the |default pre-fills the prompt, but // a user who clears the box should still get the default unless the @@ -866,14 +904,16 @@ export abstract class Formatter { } if (previous !== signature && !this.namedSuggesterConflictsWarned.has(nameKey)) { this.namedSuggesterConflictsWarned.add(nameKey); - const message = `QuickAdd: named value "${parsed.variableKey}" is defined with different option lists; the first definition's value is reused.`; - // Surface as a Notice (consistent with the other |name: warnings, which - // all use log.logWarning) — the conflict silently drops the second - // option list, so the user needs to see it on-screen, not only in - // devtools. Warn-once dedupe is kept by the guard above. The console.warn - // is retained for the diagnostic log/trail. - log.logWarning(message); - console.warn(message); + // Surface it (consistent with the other |name: warnings) — the conflict + // silently drops the second option list, so the user needs to see it, + // not only in devtools. Warn-once dedupe is kept by the guard above. + // The bare console.warn this used to carry "for the diagnostic trail" + // was redundant: ConsoleErrorLogger.logWarning already console.warns + // every message, and it bypassed this hook, so a preview would still + // have written one line per keystroke to devtools. + this.warn( + `QuickAdd: named value "${parsed.variableKey}" is defined with different option lists; the first definition's value is reused.`, + ); } } @@ -1037,7 +1077,7 @@ export abstract class Formatter { throw new Error(`Unable to parse variable. Invalid syntax in: "${output.substring(Math.max(0, match.index - 10), Math.min(output.length, match.index + 30))}..."`); } - const parsed = parseValueToken(match[1]); + const parsed = parseValueToken(match[1], { warn: this.warnSink }); if (!parsed) { throw new Error(`Unable to parse variable. Invalid syntax in: "${output.substring(Math.max(0, match.index - 10), Math.min(output.length, match.index + 30))}..."`); } @@ -1131,6 +1171,7 @@ export abstract class Formatter { const fieldVariableKey = this.getFieldVariableKey(fullMatch); const parsed = FieldSuggestionParser.parse(fullMatch, { warnUnknown: true, + warn: this.warnSink, }); if (!this.hasConcreteVariable(fieldVariableKey)) { @@ -1524,14 +1565,14 @@ export abstract class Formatter { if (this.templateInclusion.visited.has(templatePath)) { const placeholder = `[QuickAdd: template inclusion cycle detected at "${templatePath}"]`; - log.logError(placeholder); + this.reportProblem(placeholder); output = this.replacer(output, TEMPLATE_REGEX, placeholder); continue; } if (this.templateInclusion.depth >= MAX_TEMPLATE_INCLUSION_DEPTH) { const placeholder = `[QuickAdd: max template inclusion depth (${MAX_TEMPLATE_INCLUSION_DEPTH}) exceeded at "${templatePath}"]`; - log.logError(placeholder); + this.reportProblem(placeholder); output = this.replacer(output, TEMPLATE_REGEX, placeholder); continue; } From bb58b2a82fda73e0d4fc1c4fccc06ca308781b8f Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:03:17 +0200 Subject: [PATCH 03/12] fix(builder): collect format-preview warnings instead of firing Notices The choice builders re-run the whole format pipeline on every keystroke to draw the "Preview:" row, using FormatDisplayFormatter / FileNameDisplayFormatter. Those override every RESOLUTION method so nothing prompts, but they inherited the base Formatter's parsing verbatim - warnings included - and the autocomplete inserts `{{VALUE:}}` with the closing braces already there, so every prefix of an argument you type is a complete, INVALID token. Typing `pascal` into `{{VALUE:title|case:}}` stacked five Obsidian Notices across half the window. A preview is a speculative evaluation of incomplete input; it must not have the run's side effects. Both display formatters now override the reporting hooks to collect into a `PreviewDiagnostics` list, replaced at the start of every `format()`. The real run is unchanged and still owns the user-facing Notice. Two things fall out of having a channel at all: - Thrown failures join it. Both formatters wrapped the pipeline in `catch { return input; }`, so `{{VALUE:a,b|text:x}}` - and the half-typed `{{VALUE:}}` the autocomplete leaves behind - silently echoed the raw text back, which is exactly the state a confused author is already in. The catch is untyped, so only QuickAdd-authored messages pass through; anything else degrades to a generic line, and a cancelled prompt is dropped. - Messages lose their redundant `QuickAdd: ` prefix on collection. GuiLogger already prepends `QuickAdd: (Warning) `, so the Notice read "QuickAdd: (WARNING) QuickAdd: Unsupported ..." and inline under a QuickAdd field the brand is pure noise. Tests re-run every row of the measurement in the issue against the real formatters and assert zero Notices, plus the collected content - a collecting override that silently failed would otherwise read as success. Refs #1558 --- .../displayFormatter-1558-inert.test.ts | 238 ++++++++++++++++++ src/formatters/fileNameDisplayFormatter.ts | 36 ++- src/formatters/formatDisplayFormatter.ts | 38 ++- src/formatters/previewDiagnostics.ts | Bin 0 -> 3368 bytes 4 files changed, 307 insertions(+), 5 deletions(-) create mode 100644 src/formatters/displayFormatter-1558-inert.test.ts create mode 100644 src/formatters/previewDiagnostics.ts diff --git a/src/formatters/displayFormatter-1558-inert.test.ts b/src/formatters/displayFormatter-1558-inert.test.ts new file mode 100644 index 000000000..48b5b33a8 --- /dev/null +++ b/src/formatters/displayFormatter-1558-inert.test.ts @@ -0,0 +1,238 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import { FormatDisplayFormatter } from "./formatDisplayFormatter"; +import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; +import { Formatter } from "./formatter"; +import type QuickAdd from "../main"; +import { LogManager } from "../logger/logManager"; +import type { ILogger } from "../logger/ilogger"; + +// The preview never reaches a template in these cases, and SingleTemplateEngine's +// module graph pulls obsidian-dataview's CJS require. +vi.mock("../engine/SingleTemplateEngine", () => ({ + SingleTemplateEngine: class { + run(): Promise { + return Promise.resolve(""); + } + }, +})); + +const app = { + workspace: { getActiveFile: () => null }, + vault: { getMarkdownFiles: () => [], getAbstractFileByPath: () => null }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, +} as unknown as App; + +const plugin = { + settings: { globalVariables: {}, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +let reported: string[] = []; + +beforeEach(() => { + reported = []; + LogManager.loggers = [ + { + logError: (m: string) => reported.push(`error ${m}`), + logWarning: (m: string) => reported.push(`warning ${m}`), + logMessage: () => {}, + } as unknown as ILogger, + ]; +}); + +/** + * Types `head + arg + tail` one character of `arg` at a time, the way the token + * autocomplete leaves the caret: it inserts `{{VALUE:}}` with the closing braces + * already present, so every prefix of an argument is a COMPLETE, invalid token. + * Returns how many Notices the whole session produced. + */ +async function typeArgument( + make: () => FormatDisplayFormatter | FileNameDisplayFormatter, + head: string, + arg: string, + tail = "}}", +) { + const diagnostics: string[] = []; + for (let i = 1; i <= arg.length; i++) { + const formatter = make(); + await formatter.format(head + arg.slice(0, i) + tail); + for (const entry of formatter.diagnostics.list()) { + diagnostics.push(entry.message); + } + } + return { notices: reported.length, diagnostics }; +} + +/** + * Issue #1558. Every row of the measurement in the issue, re-run against the + * real display formatters. Each of these fired one Obsidian Notice per keystroke + * (two, on the anonymous form) before this change. + */ +describe("#1558 the live format preview never fires a Notice", () => { + const rows: Array<[label: string, head: string, arg: string]> = [ + ["named |case:", "{{VALUE:title|case:", "pascal"], + ["named |type:", "{{VALUE:x|type:", "number"], + ["anonymous |type:", "{{VALUE|type:", "number"], + ["anonymous |type:slider", "{{VALUE|type:", "slider"], + ["named |name:", "{{VALUE:title|name:", "cat"], + ["unknown FIELD filter", "{{FIELD:status|", "fodler:abc"], + ]; + + for (const [label, head, arg] of rows) { + it(`stays silent while typing ${label}`, async () => { + const { notices } = await typeArgument( + () => new FormatDisplayFormatter(app, plugin), + head, + arg, + ); + expect(notices).toBe(0); + }); + + it(`stays silent in the file-name preview while typing ${label}`, async () => { + const { notices } = await typeArgument( + () => new FileNameDisplayFormatter(app, plugin), + head, + arg, + ); + expect(notices).toBe(0); + }); + } + + it("collects the problem instead of dropping it", async () => { + const formatter = new FormatDisplayFormatter(app, plugin); + await formatter.format("{{VALUE:title|case:pasc}}"); + + expect(reported).toEqual([]); + expect(formatter.diagnostics.list()).toEqual([ + { + severity: "warning", + message: + 'Unsupported |case style "pasc" in token "{{VALUE:title|case:pasc}}". Supported styles: kebab, snake, camel, pascal, title, lower, upper, slug.', + }, + ]); + }); + + it("strips the redundant QuickAdd: prefix from collected messages", async () => { + const formatter = new FormatDisplayFormatter(app, plugin); + await formatter.format("{{VALUE:x|type:numbr}}"); + for (const entry of formatter.diagnostics.list()) { + expect(entry.message.startsWith("QuickAdd:")).toBe(false); + } + }); + + it("reports the same malformed token once, not once per occurrence", async () => { + const formatter = new FormatDisplayFormatter(app, plugin); + await formatter.format("{{VALUE:t|case:pasc}} and {{VALUE:t|case:pasc}}"); + expect(formatter.diagnostics.list()).toHaveLength(1); + }); + + it("starts each pass from a clean slate", async () => { + const formatter = new FormatDisplayFormatter(app, plugin); + await formatter.format("{{VALUE:title|case:pasc}}"); + expect(formatter.diagnostics.list()).toHaveLength(1); + await formatter.format("{{VALUE:title|case:pascal}}"); + expect(formatter.diagnostics.list()).toEqual([]); + }); + + it("surfaces a THROWN parse failure instead of silently echoing the input", async () => { + const formatter = new FormatDisplayFormatter(app, plugin); + // |text: on a token whose option list has a different arity throws. + const out = await formatter.format("{{VALUE:a,b|text:x}}"); + + expect(out).toBe("{{VALUE:a,b|text:x}}"); + expect(reported).toEqual([]); + const entries = formatter.diagnostics.list(); + expect(entries).toHaveLength(1); + expect(entries[0].severity).toBe("error"); + expect(formatter.diagnostics.hasError).toBe(true); + }); + + it("gives the half-typed {{VALUE:}} the autocomplete inserts real copy", async () => { + const formatter = new FormatDisplayFormatter(app, plugin); + await formatter.format("{{VALUE:}}"); + expect(formatter.diagnostics.list()).toEqual([ + { + severity: "error", + message: + "This token is incomplete. {{VALUE:}} needs a name, for example {{VALUE:title}}.", + }, + ]); + }); +}); + +/** + * The other half of the contract: a formatter that does NOT override the hooks + * still warns. A preview that goes quiet is only correct because the + * authoritative run is still loud, and `CompleteFormatter` inherits these + * defaults untouched. + */ +describe("#1558 a formatter that does not override the hooks still warns", () => { + class RuntimeFormatter extends Formatter { + constructor() { + super(undefined); + } + protected async format(input: string): Promise { + return input; + } + public runAnonymous(input: string): Promise { + return this.replaceValueInString(input); + } + public runNamed(input: string): Promise { + return this.replaceVariableInString(input); + } + protected async promptForValue(): Promise { + return ""; + } + protected async promptForVariable(): Promise { + return ""; + } + protected async suggestForValue(): Promise { + return ""; + } + protected async suggestForField(): Promise { + return ""; + } + protected suggestForFile(): string { + return ""; + } + protected getVariableValue(): string { + return ""; + } + protected getCurrentFileLink(): string | null { + return null; + } + protected async getMacroValue(): Promise { + return ""; + } + protected async promptForMathValue(): Promise { + return ""; + } + protected async getTemplateContent(): Promise { + return ""; + } + protected async getSelectedText(): Promise { + return ""; + } + protected async getClipboardContent(): Promise { + return ""; + } + protected isTemplatePropertyTypesEnabled(): boolean { + return false; + } + } + + it("warns for a named |case: typo", async () => { + await new RuntimeFormatter().runNamed("{{VALUE:title|case:pasc}}"); + expect( + reported.some((m) => m.includes('Unsupported |case style "pasc"')), + ).toBe(true); + }); + + it("warns exactly once for an anonymous |type: typo", async () => { + await new RuntimeFormatter().runAnonymous("{{VALUE|type:numbr}}"); + expect( + reported.filter((m) => m.includes('Unsupported VALUE type "numbr"')), + ).toHaveLength(1); + }); +}); diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index 87cf55bb8..0c8dfc14b 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -1,4 +1,8 @@ import { Formatter, type PromptContext } from "./formatter"; +import { + describePreviewFailure, + PreviewDiagnostics, +} from "./previewDiagnostics"; import type { App } from "obsidian"; import { DATE_VARIABLE_REGEX, GLOBAL_VAR_REGEX } from "../constants"; import type { IDateParser } from "../parsers/IDateParser"; @@ -30,8 +34,32 @@ export class FileNameDisplayFormatter extends Formatter { this.dateParser = dateParser || NLDParser; } + + /** + * Problems this pass ran into, for passive display beside the preview. + * + * A preview is a speculative evaluation of INCOMPLETE input, re-run on every + * keystroke, so it must not have the run's side effects: while you type + * `pascal` into `{{VALUE:title|case:}}` every prefix is a complete, invalid + * token, and the inherited `log.logWarning` stacked one Obsidian Notice per + * character (issue #1558). The real run still warns; this collects. + * + * Replaced at the start of every `format()` so a pass never inherits the + * previous one's complaints. + */ + public diagnostics = new PreviewDiagnostics(); + + protected warn(message: string): void { + this.diagnostics.add("warning", message); + } + + protected reportProblem(message: string): void { + this.diagnostics.add("error", message); + } + public async format(input: string): Promise { let output: string = input; + this.diagnostics = new PreviewDiagnostics(); try { // Expand globals first to preview inserted snippets @@ -55,8 +83,12 @@ export class FileNameDisplayFormatter extends Formatter { activeFolder: "path", }); output = this.replaceRandomInString(output); - } catch { - // Return the input as-is if formatting fails during preview + } catch (error) { + // Return the input as-is if formatting fails during preview. The failure + // itself is the most useful thing the preview can say, so it goes on the + // diagnostics channel rather than being swallowed (issue #1558). + const described = describePreviewFailure(error); + if (described) this.diagnostics.add("error", described); return input; } diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index cc5809ea7..f38bbe488 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -1,4 +1,8 @@ import { Formatter, type PromptContext } from "./formatter"; +import { + describePreviewFailure, + PreviewDiagnostics, +} from "./previewDiagnostics"; import type { App } from "obsidian"; import type QuickAdd from "../main"; import { SingleTemplateEngine } from "../engine/SingleTemplateEngine"; @@ -38,8 +42,32 @@ export class FormatDisplayFormatter extends Formatter { this.dateParser = dateParser || NLDParser; } + + /** + * Problems this pass ran into, for passive display beside the preview. + * + * A preview is a speculative evaluation of INCOMPLETE input, re-run on every + * keystroke, so it must not have the run's side effects: while you type + * `pascal` into `{{VALUE:title|case:}}` every prefix is a complete, invalid + * token, and the inherited `log.logWarning` stacked one Obsidian Notice per + * character (issue #1558). The real run still warns; this collects. + * + * Replaced at the start of every `format()` so a pass never inherits the + * previous one's complaints. + */ + public diagnostics = new PreviewDiagnostics(); + + protected warn(message: string): void { + this.diagnostics.add("warning", message); + } + + protected reportProblem(message: string): void { + this.diagnostics.add("error", message); + } + public async format(input: string): Promise { let output: string = input; + this.diagnostics = new PreviewDiagnostics(); try { // Expand global variables first so previews include their content @@ -72,9 +100,13 @@ export class FormatDisplayFormatter extends Formatter { output = await this.replaceFieldVarInString(output); output = await this.replaceFileInString(output); output = this.replaceRandomInString(output); - } catch { - // Return the input as-is if formatting fails during preview - // This prevents crashes when typing incomplete syntax + } catch (error) { + // Return the input as-is if formatting fails during preview: this + // prevents crashes when typing incomplete syntax. The failure itself is + // the most useful thing the preview can say, so it goes on the + // diagnostics channel rather than being swallowed (issue #1558). + const described = describePreviewFailure(error); + if (described) this.diagnostics.add("error", described); return input; } diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts new file mode 100644 index 0000000000000000000000000000000000000000..6d5cc9ada266b3cc1d98dc7697b9a392fc81ba46 GIT binary patch literal 3368 zcma)8+in{-5bdk|6;m}2k|E3JLx4i56F7~M25_4+cH#nQ8d19=i7-WONUqj`BcLDA zFYK4}47n>SNP-s7gG7;YIdkUB=vC!>q+8PACuIvZMn&!H#CzvyOLOm5)EkXX>ZnaP zVL5&+UwaP@5BVm&jhl+;rRTNg=80Ad>q6AU1+UrbxS><(HOgCU7d`q&y=3A)U*py7 z;X!C^jK6OF!QsJ*Lr@DIQ-~g39vp5F+sV<lBx1;W_T6IS^1ITq)LTAURlyM zx*^SxtsLn!Q>jB`)P`mpbjcK#ES9>5+J?xgBsV8TzdB?JqwMLLH}LPSnff|}n(4dm z@8ADnFv44T{=%2+xg=#vD!HsHqYD+e9K#BnuT*TqrvgE(*h&?|HwCp~S}7y9xD;Cv zUhrQ7*TZB`u2sqh_CcPk0-H4>Jw9%fxrT@QLc_50k`Y`aFp%+3ECzSyD!?~`3DmgS zAWsb18FHQv8irY)_?B>?;R`!l&firWM7Anz=d(+9w?^3nMlDjF>@~Re8OJ z89<~;XU&F?wAUQIz_hpTK*qQ4K(x;u1x6iC$#O#%9DmIGd)Oa_vVTjR zlf5LnWVZ5X3!Dsi(nr#}N_s{ID}c7k;Z$;vX7gG88ja)L%^Q4iFhrKy3+p<$AQbc!&f23^B6eOBvp zw^*>BihxvX%OH&I{Z{MZ`m99z_ph=z4yZ@{5AAV&Gm2q=PC;|7AgQ7_(%hsBP8@7e zqvMj47&IEKq0GMF9&STb1XpwiKA=yHNTqcpHc=uRS2ey{Ck?Kx(Yc3nLo4dSL?t7- zc@f&~PHi@?(cYp`!uv1La*xuvHKZbK8CwiT0L7?+`zL>PzQ%uqqZ2)(UJvN+K@T2F z63`vjB($NK5);jpHj;!WIkIZgfQ)yeXefF$wY4!*sS9pI!7MD3z*!d|_42E+omyW) zT)Huk)fu><-dkv(#nlD% zr?^r#SgWL4iXQsZ#_AnAbsuP#u?mL?)-sENlko6#LklOUh?O+I3XB}fMhGWo?LrXt@y-B3$!nYQ7J{Zjq?l#_ zTHh|j3vyYotp)nZ)C)M8)eBnHXcAOYV({oRFkMF(@=e#V?Vv&@d_w0jMi~ z$+LFX>jH{w2!Ky0STB|`r8X%hVguMpUU*eXJ=y@V{}kv<%b$#;UEAPl;q{DL!F!r% z+)v6gMyS^ojD(!Ez4MWT!O}uW;`2R#jOt=qLmZO+?Ub5czM8ywKDm1T^z8Zh1g0n!s}hw94%>@I+8XI1f>F-+3k&LQ{&nLi zL6n1kyn!$zt;;-~d;U6l0CV~fOJC^3tmQRpQ;~f zCuC*_#8 zY)Fzoy}J~3G`!u8gmc@^#zWp~=+Ul&v7mj0*BBL<* Date: Sun, 26 Jul 2026 23:04:22 +0200 Subject: [PATCH 04/12] fix(preflight): stop re-reporting token warnings the real run already reports RequirementCollector is a scan, not a run: it documents itself as never executing anything and returning inert replacements. But it extends Formatter, so it emitted every authoring warning itself - twice per token, because it parses each one in both a pre-pass and a main pass - and the run that followed emitted the same message a third time. Verified live on Obsidian 1.13.0: with one-page inputs enabled, one `{{VALUE:title|case:pasc}}` typo showed two identical Notices before the input form even opened, then another on submit. The run owns the warning. Overriding both hooks covers the inherited paths; the direct `parseValueToken` call in `scanVariableTokens` needs the sink passed explicitly, since hook overrides do not reach a module-level call. The preflight-only diagnostics in collectChoiceRequirements and runOnePagePreflight are untouched and stay loud - those report things only the scan can know. Refs #1558 --- .../RequirementCollector.silence.test.ts | 68 +++++++++++++++++++ src/preflight/RequirementCollector.ts | 16 ++++- 2 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 src/preflight/RequirementCollector.silence.test.ts diff --git a/src/preflight/RequirementCollector.silence.test.ts b/src/preflight/RequirementCollector.silence.test.ts new file mode 100644 index 000000000..7681510f7 --- /dev/null +++ b/src/preflight/RequirementCollector.silence.test.ts @@ -0,0 +1,68 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import { RequirementCollector } from "./RequirementCollector"; +import type QuickAdd from "../main"; +import { LogManager } from "../logger/logManager"; +import type { ILogger } from "../logger/ilogger"; + +vi.mock("../engine/SingleTemplateEngine", () => ({ + SingleTemplateEngine: class { + run(): Promise { + return Promise.resolve(""); + } + }, +})); + +const app = { + workspace: { getActiveFile: () => null }, + vault: { getMarkdownFiles: () => [], getAbstractFileByPath: () => null }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, +} as unknown as App; + +const plugin = { + settings: { globalVariables: {}, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +let reported: string[] = []; + +beforeEach(() => { + reported = []; + LogManager.loggers = [ + { + logError: (m: string) => reported.push(`error ${m}`), + logWarning: (m: string) => reported.push(`warning ${m}`), + logMessage: () => {}, + } as unknown as ILogger, + ]; +}); + +/** + * Issue #1558. The preflight scan walks the same strings the run is about to + * format. It used to emit the run's warnings itself - twice per token, because + * it parses in both a pre-pass and a main pass - so with one-page inputs enabled + * a single `|case:` typo produced two Notices before the form opened and a third + * when the run formatted for real. Verified live in Obsidian 1.13.0. + */ +describe("#1558 the preflight scan does not re-report the run's warnings", () => { + const cases = [ + "{{VALUE:title|case:pasc}}", + "{{VALUE|type:numbr}}", + "{{VALUE:x|type:slider|min:1}}", + "{{FIELD:status|fodler:abc}}", + "{{VALUE:a,b|name:title}}", + ]; + + for (const input of cases) { + it(`stays silent scanning ${input}`, async () => { + await new RequirementCollector(app, plugin).scanString(input); + expect(reported).toEqual([]); + }); + } + + it("still collects the requirement it was scanning for", async () => { + const collector = new RequirementCollector(app, plugin); + await collector.scanString("{{VALUE:title|case:pasc}}"); + expect(collector.requirements.size).toBe(1); + }); +}); diff --git a/src/preflight/RequirementCollector.ts b/src/preflight/RequirementCollector.ts index bcafeefee..f73d72eda 100644 --- a/src/preflight/RequirementCollector.ts +++ b/src/preflight/RequirementCollector.ts @@ -119,6 +119,18 @@ export class RequirementCollector extends Formatter { } } + /** + * Preflight is a SCAN, not the run. It walks the same strings the run is + * about to format, so every authoring warning it produced arrived a second + * time (twice, in fact - it parses each token in both a pre-pass and the main + * pass) before the run reported the same mistake itself. With one-page inputs + * on, a single `|case:` typo showed two Notices before the form even opened + * and a third on submit. The run owns the warning; the scan stays quiet. + * Issue #1558. + */ + protected warn(): void {} + protected reportProblem(): void {} + /** * True while the current scanString walks a path-context string. Flows * into FieldRequirement.pathContext (sticky across occurrences). @@ -250,7 +262,9 @@ export class RequirementCollector extends Formatter { const inner = (match[1] ?? "").trim(); if (!inner) continue; - const parsed = parseValueToken(inner); + // Explicit sink: this is a direct module-level call, so the hook + // overrides above do not cover it. + const parsed = parseValueToken(inner, { warn: this.warnSink }); if (!parsed) continue; const { From b8502ada76f5baf0faf11c99f84e347e2a5fdd25 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:07:25 +0200 Subject: [PATCH 05/12] fix(builder): make {{TEMPLATE:...}} preview inert The worst thing found while fixing #1558, and not in the issue. `FormatDisplayFormatter.getTemplateContent` built a `SingleTemplateEngine`, whose base `TemplateEngine` constructs a real `CompleteFormatter`. So the builder's live preview ran the RUN-TIME formatter over the included template, on every keystroke. Verified live in an isolated Obsidian 1.13.0 vault: typing `{{TEMPLATE:PreviewProbe.md}}` into the Capture format field, with that template containing `{{VALUE:whoAreYou}}`, popped a real blocking "whoAreYou / Ok / Cancel" modal on top of the settings window. A `|case:` typo inside the template fired the run's warning Notice. `{{MACRO:...}}` reached the macro engine, where it threw because the preview passes no choice executor - and the throw was swallowed into "Template (not found): ", which was simply untrue. The preview now reads the template file and resolves it through ITSELF, so an included body previews exactly like the field it is embedded in: no prompts, no macro engine, no inline JS, and no engine import in this file at all. Two details worth calling out: - `format()` splits into a `formatInternal(input, { expandLinebreakEscapes })`. An included body must NOT have `\n` escapes expanded - the runtime treats them as format-template material and never applies them to substituted content (#527), so recursing through the public `format()` would corrupt a template containing `\nabla` or `C:\Users\nadia`. - The inclusion depth counter is incremented HERE, not in the shared `Formatter.replaceTemplateInString`. At run time `CompleteFormatter.getTemplateContent` hands its child engine a copy of the state with `depth + 1`; counting in the shared method too would advance the runtime by two per level and halve its inclusion limit. Preview and runtime both still cap at 10 nested levels. Cycle and depth accounting now spans the preview level itself, and one budget is shared across a field instead of restarting per top-level `{{TEMPLATE:}}` token - previously the preview handed the engine no inclusion state, so the first nested level always started over. (Nothing was unbounded before: inside the engine subtree `visited` was shared by reference, so a self-including template still terminated with a cycle report.) The "not found" / "failed" placeholders are now distinct, so the preview stops blaming a missing file for a problem that was something else. Refs #1558 --- .../displayFormatter-1558-inert.test.ts | 3 + ...rmatDisplayFormatter-1558-template.test.ts | 207 ++++++++++++++++++ src/formatters/formatDisplayFormatter.ts | 130 +++++++---- 3 files changed, 299 insertions(+), 41 deletions(-) create mode 100644 src/formatters/formatDisplayFormatter-1558-template.test.ts diff --git a/src/formatters/displayFormatter-1558-inert.test.ts b/src/formatters/displayFormatter-1558-inert.test.ts index 48b5b33a8..434f4f2f6 100644 --- a/src/formatters/displayFormatter-1558-inert.test.ts +++ b/src/formatters/displayFormatter-1558-inert.test.ts @@ -220,6 +220,9 @@ describe("#1558 a formatter that does not override the hooks still warns", () => protected isTemplatePropertyTypesEnabled(): boolean { return false; } + protected getCurrentFileName(): string | null { + return null; + } } it("warns for a named |case: typo", async () => { diff --git a/src/formatters/formatDisplayFormatter-1558-template.test.ts b/src/formatters/formatDisplayFormatter-1558-template.test.ts new file mode 100644 index 000000000..48400fdca --- /dev/null +++ b/src/formatters/formatDisplayFormatter-1558-template.test.ts @@ -0,0 +1,207 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import { TFile } from "obsidian"; +import { FormatDisplayFormatter } from "./formatDisplayFormatter"; +import type QuickAdd from "../main"; +import { LogManager } from "../logger/logManager"; +import type { ILogger } from "../logger/ilogger"; + +/** + * If the preview ever reaches the runtime engine again, these mocks record it. + * Real ones would open a modal / run a user script. + */ +const spies = vi.hoisted(() => ({ + templateEngineRuns: [] as string[], + macroEngineRuns: [] as string[], + prompts: [] as string[], +})); + +vi.mock("../engine/SingleTemplateEngine", () => ({ + SingleTemplateEngine: class { + constructor( + _app: unknown, + _plugin: unknown, + public templatePath: string, + ) {} + run(): Promise { + spies.templateEngineRuns.push(this.templatePath); + return Promise.resolve("RUNTIME ENGINE RAN"); + } + }, +})); + +vi.mock("../engine/SingleMacroEngine", () => ({ + SingleMacroEngine: class { + runAndGetOutput(name: string): Promise { + spies.macroEngineRuns.push(name); + return Promise.resolve("MACRO RAN"); + } + getVariables() { + return new Map(); + } + }, +})); + +vi.mock("../gui/InputPrompt", () => ({ + default: class { + factory() { + return { + Prompt: async (_app: unknown, header: string) => { + spies.prompts.push(header); + return "PROMPTED"; + }, + }; + } + }, +})); + +let templates: Record = {}; +let reported: string[] = []; + +function makeApp(): App { + return { + workspace: { getActiveFile: () => null }, + 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; + +beforeEach(() => { + templates = {}; + reported = []; + spies.templateEngineRuns.length = 0; + spies.macroEngineRuns.length = 0; + spies.prompts.length = 0; + LogManager.loggers = [ + { + logError: (m: string) => reported.push(`error ${m}`), + logWarning: (m: string) => reported.push(`warning ${m}`), + logMessage: () => {}, + } as unknown as ILogger, + ]; +}); + +/** + * Issue #1558. `FormatDisplayFormatter.getTemplateContent` used to build a + * `SingleTemplateEngine`, whose base `TemplateEngine` constructs a real + * `CompleteFormatter`. So typing `{{TEMPLATE:x.md}}` into a builder field ran the + * RUN-TIME formatter over that template on every keystroke - proven live on + * Obsidian 1.13.0 by a blocking "whoAreYou / Ok / Cancel" modal appearing on top + * of the settings window, and by the run's warning Notices firing for tokens + * inside the included template. + */ +describe("#1558 previewing {{TEMPLATE:...}} is inert", () => { + it("never reaches the runtime template engine", async () => { + templates["Note.md"] = "hello"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + await f.format("{{TEMPLATE:Note.md}}"); + + expect(spies.templateEngineRuns).toEqual([]); + }); + + it("never prompts for a {{VALUE}} inside the included template", async () => { + templates["Note.md"] = "Hi {{VALUE:whoAreYou}}"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + const out = await f.format("{{TEMPLATE:Note.md}}"); + + expect(spies.prompts).toEqual([]); + expect(out).toContain("Hi "); + expect(out).not.toContain("{{VALUE"); + }); + + it("never runs a macro inside the included template", async () => { + templates["Note.md"] = "x {{MACRO:DoSomething}}"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + await f.format("{{TEMPLATE:Note.md}}"); + + expect(spies.macroEngineRuns).toEqual([]); + }); + + it("collects a bad token inside the template instead of firing a Notice", async () => { + templates["Note.md"] = "{{VALUE:x|case:pasc}}"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + await f.format("{{TEMPLATE:Note.md}}"); + + expect(reported).toEqual([]); + expect( + f.diagnostics.list().some((d) => d.message.includes("Unsupported |case")), + ).toBe(true); + }); + + it("does not expand linebreak escapes inside an included body (#527)", async () => { + // `\nabla` and a Windows path are template CONTENT, not format-template + // material: the runtime never expands escapes on substituted content. + templates["Note.md"] = String.raw`\nabla and C:\Users\nadia`; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + const out = await f.format("{{TEMPLATE:Note.md}}"); + + expect(out).toBe(String.raw`\nabla and C:\Users\nadia`); + expect(out).not.toContain("\n"); + }); + + it("still expands linebreak escapes typed in the field itself", async () => { + const f = new FormatDisplayFormatter(makeApp(), plugin); + expect(await f.format(String.raw`a\nb`)).toBe("a\nb"); + }); + + it("says the template is missing without pretending it failed for another reason", async () => { + const f = new FormatDisplayFormatter(makeApp(), plugin); + expect(await f.format("{{TEMPLATE:Gone.md}}")).toBe( + "[QuickAdd: template not found] Gone.md", + ); + }); + + it("stops a self-including template instead of recursing per keystroke", async () => { + templates["Loop.md"] = "before {{TEMPLATE:Loop.md}} after"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + const out = await f.format("{{TEMPLATE:Loop.md}}"); + + expect(out).toContain("template inclusion cycle detected"); + // The cycle report is a preview diagnostic, not a 15-second error Notice. + expect(reported).toEqual([]); + expect(f.diagnostics.hasError).toBe(true); + }); + + it("stops a long include chain at the inclusion depth limit", async () => { + for (let i = 0; i < 15; i++) { + templates[`T${i}.md`] = `${i} {{TEMPLATE:T${i + 1}.md}}`; + } + templates["T15.md"] = "end"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + const out = await f.format("{{TEMPLATE:T0.md}}"); + + expect(out).toContain("max template inclusion depth"); + expect(reported).toEqual([]); + }); + + it("resolves a normal nested include", async () => { + templates["Outer.md"] = "outer {{TEMPLATE:Inner.md}}"; + templates["Inner.md"] = "inner"; + const f = new FormatDisplayFormatter(makeApp(), plugin); + + expect(await f.format("[{{TEMPLATE:Outer.md}}]")).toBe("[outer inner]"); + }); +}); diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index f38bbe488..c62eb0e39 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -5,7 +5,7 @@ import { } from "./previewDiagnostics"; import type { App } from "obsidian"; import type QuickAdd from "../main"; -import { SingleTemplateEngine } from "../engine/SingleTemplateEngine"; +import { getTemplateFile } from "../utils/templateFolderUtils"; import { DATE_VARIABLE_REGEX, GLOBAL_VAR_REGEX } from "../constants"; import type { IDateParser } from "../parsers/IDateParser"; import { NLDParser } from "../parsers/NLDParser"; @@ -66,40 +66,11 @@ export class FormatDisplayFormatter extends Formatter { } public async format(input: string): Promise { - let output: string = input; this.diagnostics = new PreviewDiagnostics(); - try { - // Expand global variables first so previews include their content - output = await this.replaceGlobalVarInString(output); - // Mirror CaptureChoiceFormatter: linebreak escapes are format-template - // material (including global snippets) and expand before token - // substitution, never on substituted content (issue #527). - output = this.expandLinebreakEscapesOutsideTokens(output); - output = this.replaceDateInString(output); - output = this.replaceTimeInString(output); - output = await this.replaceValueInString(output); - output = await this.replaceSelectedInString(output); - output = await this.replaceClipboardInString(output); - output = await this.replaceDateVariableInString(output); - output = await this.replaceVariableInString(output); - // Links + {{filenamecurrent}} + {{folder}} + {{foldercurrent}} in one - // pass so no token re-scans another's output (#1358). ({{title}} has - // never been resolved in this preview formatter — preserved by omitting - // it.) The preview resolver never returns null, so no throw here. - output = this.replaceCurrentFileTokensInString(output, { - links: true, - fileName: true, - folder: true, - ...(this.opts.resolveActiveFolder === false - ? {} - : { activeFolder: "content" as const }), + return await this.formatInternal(input, { + expandLinebreakEscapes: true, }); - output = await this.replaceMacrosInString(output); - output = await this.replaceTemplateInString(output); - output = await this.replaceFieldVarInString(output); - output = await this.replaceFileInString(output); - output = this.replaceRandomInString(output); } catch (error) { // Return the input as-is if formatting fails during preview: this // prevents crashes when typing incomplete syntax. The failure itself is @@ -109,6 +80,51 @@ export class FormatDisplayFormatter extends Formatter { if (described) this.diagnostics.add("error", described); return input; } + } + + /** + * The preview pass list. `expandLinebreakEscapes` is false for an INCLUDED + * template body: the runtime treats `\n` as format-template material and never + * expands it on substituted content (issue #527), so expanding it here would + * corrupt a template containing `\nabla` or `C:\Users\nadia`. + */ + private async formatInternal( + input: string, + { expandLinebreakEscapes }: { expandLinebreakEscapes: boolean }, + ): Promise { + let output: string = input; + // Expand global variables first so previews include their content + output = await this.replaceGlobalVarInString(output); + // Mirror CaptureChoiceFormatter: linebreak escapes are format-template + // material (including global snippets) and expand before token + // substitution, never on substituted content (issue #527). + if (expandLinebreakEscapes) { + output = this.expandLinebreakEscapesOutsideTokens(output); + } + output = this.replaceDateInString(output); + output = this.replaceTimeInString(output); + output = await this.replaceValueInString(output); + output = await this.replaceSelectedInString(output); + output = await this.replaceClipboardInString(output); + output = await this.replaceDateVariableInString(output); + output = await this.replaceVariableInString(output); + // Links + {{filenamecurrent}} + {{folder}} + {{foldercurrent}} in one + // pass so no token re-scans another's output (#1358). ({{title}} has + // never been resolved in this preview formatter — preserved by omitting + // it.) The preview resolver never returns null, so no throw here. + output = this.replaceCurrentFileTokensInString(output, { + links: true, + fileName: true, + folder: true, + ...(this.opts.resolveActiveFolder === false + ? {} + : { activeFolder: "content" as const }), + }); + output = await this.replaceMacrosInString(output); + output = await this.replaceTemplateInString(output); + output = await this.replaceFieldVarInString(output); + output = await this.replaceFileInString(output); + output = this.replaceRandomInString(output); return output; } @@ -191,21 +207,53 @@ export class FormatDisplayFormatter extends Formatter { return Promise.resolve(getVariablePromptExample(variableName)); } + /** + * Previews an included template's body WITHOUT the runtime engine. + * + * This used to build a `SingleTemplateEngine`, whose base `TemplateEngine` + * constructs a real `CompleteFormatter` — so typing `{{TEMPLATE:x.md}}` into a + * builder field ran the RUN-TIME formatter over that template on every + * keystroke. Verified live on Obsidian 1.13.0: it opened real, blocking input + * prompt modals on top of the settings window, fired the run's warning + * Notices for tokens inside the template, and reached the macro engine — where + * it threw, because the preview passes no choice executor, and the throw was + * swallowed into a "Template (not found)" that was simply a lie (issue #1558). + * + * Resolving the body through THIS formatter keeps the preview inert: the same + * substitutions the top level gets, no prompts, no macro engine, no inline JS. + */ protected async getTemplateContent(templatePath: string): Promise { const app = this.app; if (!app) { - return `Template (app unavailable): ${templatePath}`; + return `[QuickAdd: template preview unavailable] ${templatePath}`; } + const file = getTemplateFile(app, templatePath); + if (!file) return `[QuickAdd: template not found] ${templatePath}`; + + // The depth counter is preview-LOCAL on purpose. At run time + // `CompleteFormatter.getTemplateContent` hands the child engine a COPY of + // the state with depth + 1, while `visited` is shared by reference — so + // incrementing depth inside the shared `replaceTemplateInString` would + // advance the runtime by two per level and halve its inclusion limit. The + // preview has no child formatter to carry it, so it counts here. This is + // new capability, not parity: before this change every nested level + // restarted with a fresh `visited` and depth 0, so a self-including + // template recursed unbounded, once per keystroke. + this.templateInclusion ??= { visited: new Set(), depth: 0 }; + this.templateInclusion.depth++; try { - return await new SingleTemplateEngine( - app, - this.plugin, - templatePath, - undefined, - ).run(); - } catch { - return `Template (not found): ${templatePath}`; + return await this.formatInternal(await app.vault.cachedRead(file), { + expandLinebreakEscapes: false, + }); + } catch (error) { + // Contained here rather than by format()'s catch, so one bad token + // inside an included template does not blank the whole field's preview. + const described = describePreviewFailure(error); + if (described) this.diagnostics.add("error", described); + return `[QuickAdd: template preview failed] ${templatePath}`; + } finally { + this.templateInclusion.depth--; } } From 506c60de6a1c1bd1afdf61c95d3e517bb0d47cee Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:09:11 +0200 Subject: [PATCH 06/12] fix(preflight): preview one-page file names with the file-name formatter The one-page input form's live preview computes `out.fileName`, but built a `FormatDisplayFormatter` - the note-CONTENT formatter. So it expanded `\n` escapes into real linebreaks (not a thing in a path) and resolved `{{LINKCURRENT}}` / `{{LINKSECTION}}`, both of which the run-time `formatFileName` deliberately leaves alone. `FileNameDisplayFormatter` is the class that mirrors the runtime file-name path, and the one the builder's own file-name preview already uses. One deliberate divergence, unchanged by this commit and shared with the builder's file-name field: `formatFileName` DOES resolve `{{TEMPLATE:}}` at run time (`format()` -> `replaceTemplateInString`, with path prompt scope propagated), and neither file-name preview does. Splicing a multi-line template body into a name preview is worse than showing the token, and the only inert reader available here returns a fabricated stub. Filed as a follow-up. Found while auditing every site that constructs a display formatter for #1558 - this is the only one that runs inside a real execution rather than in a builder. Refs #1558 --- ...unOnePagePreflight.filenamePreview.test.ts | 146 ++++++++++++++++++ src/preflight/runOnePagePreflight.ts | 10 +- 2 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 src/preflight/runOnePagePreflight.filenamePreview.test.ts diff --git a/src/preflight/runOnePagePreflight.filenamePreview.test.ts b/src/preflight/runOnePagePreflight.filenamePreview.test.ts new file mode 100644 index 000000000..6a99c7a20 --- /dev/null +++ b/src/preflight/runOnePagePreflight.filenamePreview.test.ts @@ -0,0 +1,146 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import { runOnePagePreflight } from "./runOnePagePreflight"; +import type ITemplateChoice from "../types/choices/ITemplateChoice"; +import type { IChoiceExecutor } from "../IChoiceExecutor"; + +vi.mock("src/logger/logManager", () => ({ + log: { logWarning: vi.fn(), logError: vi.fn(), logMessage: vi.fn() }, +})); + +/** Captures the `computePreview` callback the modal is constructed with. */ +let computePreview: + | ((values: Record) => Promise>) + | null = null; + +vi.mock("./OnePageInputModal", () => ({ + OnePageInputModal: class { + constructor( + _app: unknown, + _requirements: unknown, + _variables: unknown, + preview: ( + values: Record, + ) => Promise>, + ) { + computePreview = preview; + } + get waitForClose() { + return Promise.resolve({ title: "My Note" }); + } + }, +})); + +vi.mock("src/quickAddSettingsTab", () => ({ QuickAddSettingsTab: class {} })); +vi.mock("src/main", () => ({ __esModule: true, default: class {} })); +vi.mock("obsidian-dataview", () => ({ + __esModule: true, + getAPI: vi.fn().mockReturnValue(null), +})); +vi.mock("src/utilityObsidian", async () => { + const { TFile: TFileCls } = await import("obsidian"); + return { + getMarkdownFilesInFolder: vi.fn(() => []), + getMarkdownFilesWithTag: vi.fn(() => []), + getUserScript: vi.fn(), + isFolder: vi.fn(() => false), + getTemplateFile: vi.fn((app: App, path: string) => { + const f = app.vault.getAbstractFileByPath(path); + return f instanceof TFileCls ? f : null; + }), + }; +}); + +const createApp = () => + ({ + workspace: { + getActiveViewOfType: vi.fn().mockReturnValue(null), + getActiveFile: () => null, + }, + vault: { + getAbstractFileByPath: vi.fn().mockReturnValue(null), + getMarkdownFiles: () => [], + }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, + }) as unknown as App; + +const createChoice = (fileNameFormat: string): ITemplateChoice => + ({ + id: "tmpl", + name: "Template Choice", + type: "Template", + command: false, + templatePath: "", + fileNameFormat: { enabled: true, format: fileNameFormat }, + folder: { + enabled: false, + folders: [], + chooseWhenCreatingNote: false, + createInSameFolderAsActiveFile: false, + chooseFromSubfolders: false, + }, + appendLink: false, + openFile: false, + fileOpening: { + location: "tab", + direction: "vertical", + mode: "default", + focus: true, + }, + fileExistsMode: "Increment the file name", + setFileExistsBehavior: false, + }) as unknown as ITemplateChoice; + +const createExecutor = (): IChoiceExecutor => ({ + execute: vi.fn(), + variables: new Map(), +}); + +const createPlugin = () => + ({ + settings: { + inputPrompt: "single-line", + globalVariables: {}, + useSelectionAsCaptureValue: false, + }, + }) as never; + +beforeEach(() => { + computePreview = null; +}); + +/** + * The one-page form's live preview previews a FILE NAME, but built a + * `FormatDisplayFormatter` - the note-CONTENT formatter. It therefore expanded + * `\n` escapes into real linebreaks, which are not linebreaks in a path, and + * would have resolved a `{{TEMPLATE:...}}` inclusion into the name. + * `FileNameDisplayFormatter` is what `formatFileName` mirrors at run time, and + * is what the builder's own file-name preview already uses. + */ +describe("one-page preflight previews the file name with the file-name formatter", () => { + it("leaves a backslash-n in the name alone instead of splitting the path", async () => { + await runOnePagePreflight( + createApp(), + createPlugin(), + createExecutor(), + createChoice(String.raw`Notes\name-{{VALUE:title}}`), + ); + + expect(computePreview).not.toBeNull(); + const out = await computePreview!({ title: "My Note" }); + expect(out.fileName).toBe(String.raw`Notes\name-My Note`); + expect(out.fileName).not.toContain("\n"); + }); + + it("does not pull a template's body into the file name", async () => { + await runOnePagePreflight( + createApp(), + createPlugin(), + createExecutor(), + createChoice("{{TEMPLATE:Some.md}}-{{VALUE:title}}"), + ); + + const out = await computePreview!({ title: "My Note" }); + expect(out.fileName).toBe("{{TEMPLATE:Some.md}}-My Note"); + }); +}); diff --git a/src/preflight/runOnePagePreflight.ts b/src/preflight/runOnePagePreflight.ts index b91d31fb5..e563a002a 100644 --- a/src/preflight/runOnePagePreflight.ts +++ b/src/preflight/runOnePagePreflight.ts @@ -1,6 +1,6 @@ import type { App } from "obsidian"; import type { IChoiceExecutor } from "src/IChoiceExecutor"; -import { FormatDisplayFormatter } from "src/formatters/formatDisplayFormatter"; +import { FileNameDisplayFormatter } from "src/formatters/fileNameDisplayFormatter"; import type QuickAdd from "src/main"; import type IChoice from "src/types/choices/IChoice"; import type ITemplateChoice from "src/types/choices/ITemplateChoice"; @@ -118,7 +118,13 @@ export async function runOnePagePreflight( // Optional live preview of a couple of key outputs (best-effort) const computePreview = async (values: Record) => { try { - const formatter = new FormatDisplayFormatter(app, plugin); + // FileNameDisplayFormatter, not FormatDisplayFormatter: this previews + // a FILE NAME, and only the file-name formatter matches what + // `formatFileName` actually does at run time - it resolves + // {{foldercurrent}} in "path" mode, never includes templates, and does + // not expand `\n` escapes (which are not linebreaks in a path). It is + // also the class the builder's own file-name preview uses. + const formatter = new FileNameDisplayFormatter(app, plugin); const out: Record = {}; // File name preview for Template if (choice.type === "Template") { From 9ccd914056088c67fb7f6e0aee27bee2aa192968 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:14:17 +0200 Subject: [PATCH 07/12] feat(builder): show format-preview problems inline, on a fresh formatter per pass Completes #1558. Silencing the preview's Notices would have left the author with no feedback at all until they ran the choice, so the problems now land where the eye already is: one muted line under the preview row, coloured by severity, no glyph (matching .qa-folder-mode-warning), clamped with the full text in `title` because the longest of these runs past 250 characters. Two behaviours make that calm rather than a quieter kind of noise: - They are held back until the field has been still for 500ms. Every one of these messages quotes the partial token it is about, so shown live they would rewrite themselves on every keystroke. The preview TEXT stays live and un-debounced, as before. Lint-on-idle is the editor convention. - When nothing resolved, the row says "Unresolved:" instead of "Preview:". The value shown is the raw input, and labelling that "Preview" asserts it is what you will get. The formatter is now built per pass instead of memoized for the field's lifetime. That is a bug fix, not hygiene: the instance carries a `variables` map whose resolved keys short-circuit the next resolve, so editing the option list of `{{VALUE:alpha,beta|name:t}}` to `{{VALUE:gamma,delta|name:t}}` kept previewing "alpha" and tripped the "conflicting definitions" warn-once guard - which, before this PR, fired an on-screen Notice from a keystroke. It also scopes each pass's diagnostics, which an async pipeline sharing one instance could otherwise interleave. Text and diagnostics commit under the same staleness token, so the row can never show one pass's preview beside another's complaint. Verified live in an isolated Obsidian 1.13.0 vault: typing `pascal` into `{{VALUE:title|case:}}` now produces 0 Notices (was 5) and 0 inline complaints while typing, with the single explanation appearing once you stop. Closes #1558 --- .../components/FormatPreviewField.svelte | 96 +++++++-- .../components/FormatPreviewField.test.ts | 187 ++++++++++++++++++ src/styles.css | 22 +++ 3 files changed, 284 insertions(+), 21 deletions(-) create mode 100644 src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index 621f5dc19..72532f698 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -3,16 +3,18 @@ import type { App } from "obsidian"; import type QuickAdd from "../../../main"; import { FormatDisplayFormatter } from "../../../formatters/formatDisplayFormatter"; import { FileNameDisplayFormatter } from "../../../formatters/fileNameDisplayFormatter"; +import type { PreviewDiagnostic } from "../../../formatters/previewDiagnostics"; /** - * Live "Preview: …" row for a format/filename field. Renders BELOW the input it - * previews (issue #1543 — it used to sit above, where it read as the field's - * label), and not at all while the field is empty, so there is never a dangling - * "Preview:" with nothing after it. + * Live "Preview: …" row for a format/filename field, plus any problems that pass + * ran into. Renders BELOW the input it previews (issue #1543 — it used to sit + * above, where it read as the field's label), and not at all while the field is + * empty, so there is never a dangling "Preview:" with nothing after it. * - * Un-debounced to preserve the imperative builders' per-keystroke behavior (Plan - * 010 debounce is deliberately out of scope for #1130). A monotonic token drops - * stale async results so the latest value always wins. + * The preview TEXT is un-debounced, preserving the imperative builders' + * per-keystroke behavior (Plan 010 debounce is deliberately out of scope for + * #1130). A monotonic token drops stale async results so the latest value + * always wins. */ let { value, @@ -39,7 +41,12 @@ let { targetFolderPath?: string; } = $props(); +/** How long the field must sit still before its problems are shown. */ +const DIAGNOSTICS_IDLE_MS = 500; + let preview = $state(""); +let diagnostics = $state([]); +let showDiagnostics = $state(false); let previewToken = 0; // Gate the row on the RAW value, never on the resolved preview: the formatter is @@ -48,16 +55,11 @@ let previewToken = 0; // populated, and the announcement would be lost. const hasValue = $derived(value.trim().length > 0); -// app/plugin/kind are stable for the field's lifetime, so this $derived computes -// the formatter once; the reactive effect below then only re-runs on `value` -// change. ($derived is a reactive context, so referencing the props here is -// correct — a plain top-level const would capture only their initial value.) -const formatter = $derived( - formatterKind === "fileName" - ? new FileNameDisplayFormatter(app, plugin) - : new FormatDisplayFormatter(app, plugin, undefined, { - resolveActiveFolder: formatterKind !== "lineTarget", - }), +// A field whose format could not be resolved is not showing a preview of the +// output — it is showing the raw text back. Say so, rather than letting +// "Preview:" assert that this IS what you will get. +const isUnresolved = $derived( + showDiagnostics && diagnostics.some((d) => d.severity === "error"), ); $effect(() => { @@ -68,8 +70,20 @@ $effect(() => { if (!current.trim()) { previewToken++; preview = ""; + diagnostics = []; return; } + // A formatter per pass, not one memoized for the field's lifetime. It carries + // per-pass state — this pass's diagnostics, and a `variables` map whose + // resolved keys short-circuit the next resolve — so reusing one instance made + // an edited option list preview the stale value and let a warn-once guard + // fire from a keystroke. Construction is trivial. + const formatter = + formatterKind === "fileName" + ? new FileNameDisplayFormatter(app, plugin) + : new FormatDisplayFormatter(app, plugin, undefined, { + resolveActiveFolder: formatterKind !== "lineTarget", + }); // Resolve {{FOLDER}} / {{FOLDER|name}} against the configured target folder, // or a "Folder/Name" placeholder so the token never previews blank when no // caller wires the real path in (issue: FOLDER preview always empty). @@ -80,17 +94,57 @@ $effect(() => { void (async () => { try { const formatted = await formatter.format(current); - if (token === previewToken) preview = formatted; + // Text and problems commit under the same guard, so the row can never + // show one pass's preview beside another pass's complaint. + if (token !== previewToken) return; + preview = formatted; + diagnostics = formatter.diagnostics.list(); } catch { - if (token === previewToken) preview = "Preview unavailable"; + // format() catches its own failures and reports them as diagnostics, so + // reaching here means something outside the pipeline broke. Say so in + // the same place rather than leaving a stale preview standing. + if (token !== previewToken) return; + preview = current; + diagnostics = [ + { severity: "error", message: "Preview unavailable." }, + ]; } })(); }); + +// Problems are held back until the field has been still for a moment. Every one +// of these messages quotes the partial token it is about, so shown live they +// would rewrite themselves on every keystroke while you type an argument — the +// quieter cousin of the Notice storm this replaced (issue #1558). Lint-on-idle +// is the editor convention. The preview text itself stays live. +$effect(() => { + void value; + showDiagnostics = false; + const timer = setTimeout(() => { + showDiagnostics = true; + }, DIAGNOSTICS_IDLE_MS); + return () => clearTimeout(timer); +}); {#if hasValue}
- Preview: - {preview} + {isUnresolved ? "Unresolved: " : "Preview: "}{preview}
+ {#if showDiagnostics && diagnostics.length > 0} + +
+ {#each diagnostics as diagnostic (diagnostic.severity + diagnostic.message)} +
+ {diagnostic.message} +
+ {/each} +
+ {/if} {/if} diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts new file mode 100644 index 000000000..1b2537593 --- /dev/null +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts @@ -0,0 +1,187 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { render } from "@testing-library/svelte"; +import { tick } from "svelte"; +import type { App } from "obsidian"; +import FormatPreviewField from "./FormatPreviewField.svelte"; +import type QuickAdd from "../../../main"; +import { LogManager } from "../../../logger/logManager"; +import type { ILogger } from "../../../logger/ilogger"; + +// SingleTemplateEngine's module graph pulls obsidian-dataview's CJS require; the +// preview no longer uses it, but FileNameDisplayFormatter's siblings still +// import through the same barrel in this environment. +vi.mock("../../../engine/SingleTemplateEngine", () => ({ + SingleTemplateEngine: class { + run(): Promise { + return Promise.resolve(""); + } + }, +})); + +const app = { + workspace: { getActiveFile: () => null }, + vault: { getMarkdownFiles: () => [], getAbstractFileByPath: () => null }, + metadataCache: { getFileCache: () => null, getAllPropertyInfos: () => ({}) }, +} as unknown as App; + +const plugin = { + settings: { globalVariables: {}, choices: [] }, + getTemplateFiles: () => [], +} as unknown as QuickAdd; + +let reported: string[] = []; + +beforeEach(() => { + vi.useFakeTimers(); + reported = []; + LogManager.loggers = [ + { + logError: (m: string) => reported.push(m), + logWarning: (m: string) => reported.push(m), + logMessage: () => {}, + } as unknown as ILogger, + ]; +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +/** + * Lets the async preview pass settle without advancing the idle timer. Under + * fake timers the formatter's promise chain needs its microtasks flushed; + * advancing by 0ms does that and cannot reach the 500ms diagnostics gate. + */ +async function settle() { + for (let i = 0; i < 4; i++) { + await vi.advanceTimersByTimeAsync(0); + await tick(); + } +} + +/** Settles, then advances past the diagnostics idle gate. */ +async function settleAndIdle() { + await settle(); + await vi.advanceTimersByTimeAsync(600); + await settle(); +} + +const issues = (container: HTMLElement) => + Array.from(container.querySelectorAll(".qa-preview-issue")).map((el) => + el.textContent?.trim(), + ); + +describe("FormatPreviewField", () => { + it("shows nothing while the field is empty", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "", app, plugin }, + }); + await settleAndIdle(); + expect(container.querySelector(".qa-preview-row")).toBeNull(); + }); + + it("previews a resolvable format and reports no problems", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "{{VALUE:title|case:pascal}}", app, plugin }, + }); + await settleAndIdle(); + + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Preview: ", + ); + expect(container.querySelector(".qa-preview-value")?.textContent).not.toBe( + "", + ); + expect(issues(container as HTMLElement)).toEqual([]); + }); + + it("shows a token problem inline instead of firing a Notice", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "{{VALUE:title|case:pasc}}", app, plugin }, + }); + await settleAndIdle(); + + expect(reported).toEqual([]); + expect(issues(container as HTMLElement)).toEqual([ + 'Unsupported |case style "pasc" in token "{{VALUE:title|case:pasc}}". Supported styles: kebab, snake, camel, pascal, title, lower, upper, slug.', + ]); + }); + + it("holds the problem back until the field has been still", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "{{VALUE:title|case:pasc}}", app, plugin }, + }); + await settle(); + + // Resolved, but not yet idle: the preview text is live, the complaint is not. + expect(container.querySelector(".qa-preview-value")?.textContent).not.toBe( + "", + ); + expect(issues(container as HTMLElement)).toEqual([]); + + await vi.advanceTimersByTimeAsync(600); + await settle(); + expect(issues(container as HTMLElement)).toHaveLength(1); + }); + + it("hides a shown problem again as soon as typing resumes", async () => { + const { container, rerender } = render(FormatPreviewField, { + props: { value: "{{VALUE:title|case:pasc}}", app, plugin }, + }); + await settleAndIdle(); + expect(issues(container as HTMLElement)).toHaveLength(1); + + await rerender({ value: "{{VALUE:title|case:pasca}}", app, plugin }); + await settle(); + expect(issues(container as HTMLElement)).toEqual([]); + }); + + it("stops asserting the raw text is the output when nothing resolved", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "{{VALUE:a,b|text:x}}", app, plugin }, + }); + await settleAndIdle(); + + expect(container.querySelector(".qa-preview-label")?.textContent).toBe( + "Unresolved: ", + ); + expect( + container.querySelector(".qa-preview-issue--error"), + ).not.toBeNull(); + }); + + it("re-previews an edited named option list instead of the stale first value", async () => { + // The formatter used to be memoized for the field's lifetime. Its + // `variables` map short-circuits an already-resolved key, so editing the + // option list of a `|name:`d token kept previewing the FIRST list's value + // and tripped the "conflicting definitions" warn-once guard - which, before + // this fix, was an on-screen Notice fired from a keystroke. + const { container, rerender } = render(FormatPreviewField, { + props: { value: "{{VALUE:alpha,beta|name:t}}", app, plugin }, + }); + await settleAndIdle(); + expect(container.querySelector(".qa-preview-value")?.textContent).toContain( + "alpha", + ); + + await rerender({ value: "{{VALUE:gamma,delta|name:t}}", app, plugin }); + await settleAndIdle(); + expect(container.querySelector(".qa-preview-value")?.textContent).toContain( + "gamma", + ); + expect(issues(container as HTMLElement)).toEqual([]); + expect(reported).toEqual([]); + }); + + it("clears the problem when the field is emptied", async () => { + const { container, rerender } = render(FormatPreviewField, { + props: { value: "{{VALUE:title|case:pasc}}", app, plugin }, + }); + await settleAndIdle(); + expect(issues(container as HTMLElement)).toHaveLength(1); + + await rerender({ value: "", app, plugin }); + await settleAndIdle(); + expect(container.querySelector(".qa-preview-row")).toBeNull(); + }); +}); diff --git a/src/styles.css b/src/styles.css index 11c56cc6f..471ef2a13 100644 --- a/src/styles.css +++ b/src/styles.css @@ -216,6 +216,28 @@ color: var(--text-muted); overflow-wrap: anywhere; } +/* Problems the preview pass ran into, shown here instead of as a Notice per + keystroke (#1558). Colour carries severity — no glyph — matching + .qa-folder-mode-warning. Clamped with the full text in `title`, because the + longest of these (the FIELD filter list) runs past 250 characters. */ +.quickAddModal .qa-preview-issues { + margin: -4px 0 8px; + display: grid; + gap: 2px; +} +.quickAddModal .qa-preview-issue { + font-size: var(--font-ui-smaller); + color: var(--text-warning); + overflow-wrap: anywhere; + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 3; + line-clamp: 3; + overflow: hidden; +} +.quickAddModal .qa-preview-issue--error { + color: var(--text-error); +} .quickAddModal .qa-preview-label { font-weight: 600; } From 31c8ca8fee3bbc8c0448b28b49d2ef61f11f9ddf Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:21:59 +0200 Subject: [PATCH 08/12] fix(format): strip a stray NUL byte from previewDiagnostics.ts A heredoc in the authoring pass wrote a literal NUL into a template literal, so git classified the file as binary and would have shown no diff for it in review. --- src/formatters/previewDiagnostics.ts | Bin 3368 -> 3368 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index 6d5cc9ada266b3cc1d98dc7697b9a392fc81ba46..55a8033e2f3985dc452d018388babf0b5541d1a7 100644 GIT binary patch delta 14 VcmZ1>wL)qGBMYO#W+oOjP5>W91494+ delta 14 VcmZ1>wL)qGBMT$LW+oOjP5>T;10w(c From 54bee88598b8421d317929af8df8ed248952cccd Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:56:04 +0200 Subject: [PATCH 09/12] docs(format): correct three comments that overstated run-time behaviour Found by an adversarial review of the diff. No behaviour changes beyond two small ones noted below; the point is that in a codebase that uses comments as its design record, an inverted invariant is the expensive kind of wrong - it survives into commit messages, test names and PR bodies, as all three did here. - `runOnePagePreflight` claimed `formatFileName` "never includes templates". It does: `format()` runs `replaceTemplateInString`, path prompt scope is deliberately propagated into the child engine, and `completeFormatter.promptContext.test.ts` pins it. The real reasons for the swap are `\n` expansion and the link tokens, which run-time `formatFileName` leaves literal - the second of which the old comment did not mention. The `{{TEMPLATE:}}` behaviour is now documented as a deliberate divergence, shared with the builder's own file-name field, with a follow-up filed. - `formatDisplayFormatter` claimed the old preview "recursed unbounded". It did not: only the preview -> engine boundary restarted, and inside the engine subtree `visited` is shared by reference, so a self-including template still terminated with a cycle report. What the preview-local depth counter actually buys is accounting that spans the preview level itself, on one budget per field. The load-bearing half of that comment - why the counter must NOT live in the shared `replaceTemplateInString` - is unchanged and still correct. - `describePreviewFailure` claimed "only QuickAdd-authored messages are passed through". There is no allowlist, deliberately: most reachable throws are authored but unbranded, and a genuine plugin bug's message is what makes a report actionable. The comment now says that instead. Two behaviour fixes in the same pass: - The template cycle/depth reports arrive wrapped as `[QuickAdd: ... ]`, because the identical string is also spliced into the output as a placeholder. The brand strip did not match through the bracket, so the preview showed the same sentence twice, twenty pixels apart. Unwrap the bracket first. - `FileNameDisplayFormatter.getTemplateContent` returned a fabricated `[ template content...]`. It is unreachable (the class never calls `replaceTemplateInString`) but abstract on `Formatter`, so it has to exist - and fabricated text that reads like part of a file name is the worst thing for it to return if the pass is ever wired up. It now names what happened. --- src/formatters/fileNameDisplayFormatter.ts | 18 ++++++++++++++---- ...ormatDisplayFormatter-1558-template.test.ts | 9 ++++++++- src/formatters/formatDisplayFormatter.ts | 14 +++++++++----- src/formatters/previewDiagnostics.ts | 16 ++++++++++++---- src/preflight/RequirementCollector.ts | 4 ++++ ...runOnePagePreflight.filenamePreview.test.ts | 14 +++++++++----- src/preflight/runOnePagePreflight.ts | 17 ++++++++++++----- 7 files changed, 68 insertions(+), 24 deletions(-) diff --git a/src/formatters/fileNameDisplayFormatter.ts b/src/formatters/fileNameDisplayFormatter.ts index 0c8dfc14b..de0b1fbec 100644 --- a/src/formatters/fileNameDisplayFormatter.ts +++ b/src/formatters/fileNameDisplayFormatter.ts @@ -34,7 +34,6 @@ export class FileNameDisplayFormatter extends Formatter { this.dateParser = dateParser || NLDParser; } - /** * Problems this pass ran into, for passive display beside the preview. * @@ -163,10 +162,21 @@ export class FileNameDisplayFormatter extends Formatter { return getVariablePromptExample(variableName); } + /** + * Unreachable today: this formatter's pass list (see `format()`) never calls + * `replaceTemplateInString`, so `{{TEMPLATE:}}` stays literal in a file-name + * preview. `getTemplateContent` is abstract on `Formatter`, so the override has + * to exist. + * + * It used to return a fabricated `[ template content...]`, which would + * have spliced text that READS like a real part of the file name the moment + * anyone wired the pass up. It now names what actually happened. Run-time + * `formatFileName` does resolve the include (`format()` -> + * `replaceTemplateInString`); closing that gap needs a real inert reader, not + * a stub. + */ protected async getTemplateContent(templatePath: string): Promise { - // Show template preview with realistic content length - const templateName = templatePath.split('/').pop()?.replace('.md', '') || templatePath; - return `[${templateName} template content...]`; + return `[QuickAdd: template not resolved in a file name] ${templatePath}`; } protected async getSelectedText(): Promise { diff --git a/src/formatters/formatDisplayFormatter-1558-template.test.ts b/src/formatters/formatDisplayFormatter-1558-template.test.ts index 48400fdca..36b46773e 100644 --- a/src/formatters/formatDisplayFormatter-1558-template.test.ts +++ b/src/formatters/formatDisplayFormatter-1558-template.test.ts @@ -172,7 +172,7 @@ describe("#1558 previewing {{TEMPLATE:...}} is inert", () => { ); }); - it("stops a self-including template instead of recursing per keystroke", async () => { + it("reports a self-including template as a cycle, at the preview level", async () => { templates["Loop.md"] = "before {{TEMPLATE:Loop.md}} after"; const f = new FormatDisplayFormatter(makeApp(), plugin); @@ -182,6 +182,13 @@ describe("#1558 previewing {{TEMPLATE:...}} is inert", () => { // The cycle report is a preview diagnostic, not a 15-second error Notice. expect(reported).toEqual([]); expect(f.diagnostics.hasError).toBe(true); + // The same sentence is ALSO spliced into the output as a placeholder, so + // the inline copy is unwrapped and unbranded rather than repeating the + // bracketed text verbatim twenty pixels below it. + const [entry] = f.diagnostics.list(); + expect(entry.message.startsWith("[")).toBe(false); + expect(entry.message.startsWith("QuickAdd:")).toBe(false); + expect(entry.message).toContain("template inclusion cycle detected"); }); it("stops a long include chain at the inclusion depth limit", async () => { diff --git a/src/formatters/formatDisplayFormatter.ts b/src/formatters/formatDisplayFormatter.ts index c62eb0e39..b460c87cf 100644 --- a/src/formatters/formatDisplayFormatter.ts +++ b/src/formatters/formatDisplayFormatter.ts @@ -42,7 +42,6 @@ export class FormatDisplayFormatter extends Formatter { this.dateParser = dateParser || NLDParser; } - /** * Problems this pass ran into, for passive display beside the preview. * @@ -236,10 +235,15 @@ export class FormatDisplayFormatter extends Formatter { // the state with depth + 1, while `visited` is shared by reference — so // incrementing depth inside the shared `replaceTemplateInString` would // advance the runtime by two per level and halve its inclusion limit. The - // preview has no child formatter to carry it, so it counts here. This is - // new capability, not parity: before this change every nested level - // restarted with a fresh `visited` and depth 0, so a self-including - // template recursed unbounded, once per keystroke. + // preview has no child formatter to carry it, so it counts here. Preview + // and runtime both still cap at MAX_TEMPLATE_INCLUSION_DEPTH levels. + // + // What the local counter buys: cycle and depth accounting that spans the + // preview level itself, on one budget shared across the field. Before this + // change the preview handed the engine no inclusion state at all, so the + // first nested level always started over from an empty `visited` and depth + // 0. (It was not unbounded - inside the engine subtree `visited` was shared + // by reference, so a self-including template still terminated.) this.templateInclusion ??= { visited: new Set(), depth: 0 }; this.templateInclusion.depth++; try { diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index 55a8033e2..b62be02c1 100644 --- a/src/formatters/previewDiagnostics.ts +++ b/src/formatters/previewDiagnostics.ts @@ -44,9 +44,15 @@ export class PreviewDiagnostics { * "QuickAdd: (Warning) " (quickAddLogger.ts) - so most of them also open with a * literal "QuickAdd: " that reads as a stutter there and as pure noise inline * under a QuickAdd settings field. + * + * The template cycle/depth reports arrive wrapped as `[QuickAdd: ... ]` because + * the same string is ALSO spliced into the output as a placeholder. Unwrap those + * first, or the preview shows the identical bracketed sentence twice, twenty + * pixels apart. */ function stripBrandPrefix(message: string): string { - return message.replace(/^QuickAdd:\s*/i, ""); + const unwrapped = message.replace(/^\[(QuickAdd:[\s\S]*)\]$/i, "$1"); + return unwrapped.replace(/^QuickAdd:\s*/i, ""); } /** @@ -59,9 +65,11 @@ function stripBrandPrefix(message: string): string { * already in. These are the most useful diagnostics the preview can produce, so * they go in the same channel as the warnings. * - * The catch is untyped and catches everything, so a genuine plugin bug must not - * render its raw message under a settings field: only QuickAdd-authored - * messages are passed through, anything else degrades to a generic line. + * The catch is untyped and catches everything, so a non-Error throw degrades to + * a generic line. An `Error` message IS shown verbatim: there is deliberately no + * brand allowlist, because most of the reachable throws are QuickAdd-authored but + * unbranded, and on the rare occasion a genuine plugin bug surfaces here, its + * message is what makes the bug report actionable. */ export function describePreviewFailure(error: unknown): string | null { if (!(error instanceof Error)) return PREVIEW_FAILED_MESSAGE; diff --git a/src/preflight/RequirementCollector.ts b/src/preflight/RequirementCollector.ts index f73d72eda..3892669f7 100644 --- a/src/preflight/RequirementCollector.ts +++ b/src/preflight/RequirementCollector.ts @@ -129,6 +129,10 @@ export class RequirementCollector extends Formatter { * Issue #1558. */ protected warn(): void {} + // Defence in depth: the scan's own format() never calls + // replaceTemplateInString (it records template paths for the caller to walk), + // so nothing routes here today. It stays so a future pass added to the scan + // cannot reintroduce a 15-second error Notice from a preflight walk. protected reportProblem(): void {} /** diff --git a/src/preflight/runOnePagePreflight.filenamePreview.test.ts b/src/preflight/runOnePagePreflight.filenamePreview.test.ts index 6a99c7a20..08e3f2c08 100644 --- a/src/preflight/runOnePagePreflight.filenamePreview.test.ts +++ b/src/preflight/runOnePagePreflight.filenamePreview.test.ts @@ -111,11 +111,15 @@ beforeEach(() => { /** * The one-page form's live preview previews a FILE NAME, but built a - * `FormatDisplayFormatter` - the note-CONTENT formatter. It therefore expanded - * `\n` escapes into real linebreaks, which are not linebreaks in a path, and - * would have resolved a `{{TEMPLATE:...}}` inclusion into the name. + * `FormatDisplayFormatter` - the note-CONTENT formatter - so it expanded `\n` + * escapes into real linebreaks, which are not linebreaks in a path. * `FileNameDisplayFormatter` is what `formatFileName` mirrors at run time, and - * is what the builder's own file-name preview already uses. + * what the builder's own file-name preview already uses. + * + * Note the second case is a DIVERGENCE from run time, not parity with it: + * `formatFileName` does resolve `{{TEMPLATE:}}` (see + * completeFormatter.promptContext.test.ts). Neither file-name preview does, and + * pinning that here keeps the gap visible rather than accidental. */ describe("one-page preflight previews the file name with the file-name formatter", () => { it("leaves a backslash-n in the name alone instead of splitting the path", async () => { @@ -132,7 +136,7 @@ describe("one-page preflight previews the file name with the file-name formatter expect(out.fileName).not.toContain("\n"); }); - it("does not pull a template's body into the file name", async () => { + it("leaves a {{TEMPLATE:}} include literal rather than splicing a body into a path", async () => { await runOnePagePreflight( createApp(), createPlugin(), diff --git a/src/preflight/runOnePagePreflight.ts b/src/preflight/runOnePagePreflight.ts index e563a002a..8ce0726dd 100644 --- a/src/preflight/runOnePagePreflight.ts +++ b/src/preflight/runOnePagePreflight.ts @@ -119,11 +119,18 @@ export async function runOnePagePreflight( const computePreview = async (values: Record) => { try { // FileNameDisplayFormatter, not FormatDisplayFormatter: this previews - // a FILE NAME, and only the file-name formatter matches what - // `formatFileName` actually does at run time - it resolves - // {{foldercurrent}} in "path" mode, never includes templates, and does - // not expand `\n` escapes (which are not linebreaks in a path). It is - // also the class the builder's own file-name preview uses. + // a FILE NAME. The content formatter expands `\n` escapes (not + // linebreaks in a path) and resolves {{LINKCURRENT}}/{{LINKSECTION}}, + // both of which the run-time `formatFileName` deliberately leaves + // literal. It is also the class the builder's own file-name preview + // uses. + // + // One deliberate divergence: `formatFileName` DOES resolve + // {{TEMPLATE:}} at run time (format() -> replaceTemplateInString, with + // path prompt scope propagated in CompleteFormatter.getTemplateContent). + // Neither file-name preview does - a multi-line template body is not a + // name, and the only inert reader available here returns a fabricated + // stub. Same gap the builder's file-name field already has. const formatter = new FileNameDisplayFormatter(app, plugin); const out: Record = {}; // File name preview for Template From 555ca94b48121aca11de01349b713908b721694b Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 00:11:13 +0200 Subject: [PATCH 10/12] fix(builder): drop a cancelled prompt from preview diagnostics instead of reporting it `describePreviewFailure` gated on `instanceof Error` before consulting `isCancellationError`. But QuickAdd's modals reject with a bare STRING (`GenericInputPrompt.ts:293` -> `rejectPromise("No input given.")`), and `isCancellationError` only ever matches strings (`errorUtils.ts:77` returns false for anything that is not one). So the early return fired first and the exact case the comment described - a nested resolver that prompts and is cancelled - would have surfaced as "Preview unavailable" under the field. Check the raw value first. An Error whose message is a cancellation string is still covered on the way through. Caught by CodeRabbit on #1560. Test verified to fail without the fix. --- .../displayFormatter-1558-inert.test.ts | 16 ++++++++++++++++ src/formatters/previewDiagnostics.ts | 10 ++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/formatters/displayFormatter-1558-inert.test.ts b/src/formatters/displayFormatter-1558-inert.test.ts index 434f4f2f6..2e9edda88 100644 --- a/src/formatters/displayFormatter-1558-inert.test.ts +++ b/src/formatters/displayFormatter-1558-inert.test.ts @@ -3,6 +3,7 @@ import type { App } from "obsidian"; import { FormatDisplayFormatter } from "./formatDisplayFormatter"; import { FileNameDisplayFormatter } from "./fileNameDisplayFormatter"; import { Formatter } from "./formatter"; +import { describePreviewFailure } from "./previewDiagnostics"; import type QuickAdd from "../main"; import { LogManager } from "../logger/logManager"; import type { ILogger } from "../logger/ilogger"; @@ -148,6 +149,21 @@ describe("#1558 the live format preview never fires a Notice", () => { expect(formatter.diagnostics.hasError).toBe(true); }); + it("drops a cancellation rather than reporting it as a preview failure", async () => { + // QuickAdd's modals reject with a bare STRING, not an Error + // (GenericInputPrompt.ts: `rejectPromise("No input given.")`), and + // isCancellationError only ever matches strings. + expect(describePreviewFailure("No input given.")).toBeNull(); + expect(describePreviewFailure("no input given.")).toBeNull(); + expect(describePreviewFailure("cancelled")).toBeNull(); + expect(describePreviewFailure(new Error("cancelled"))).toBeNull(); + // Anything else still reports. + expect(describePreviewFailure("something else")).not.toBeNull(); + expect(describePreviewFailure(new Error("real problem"))).toBe( + "real problem", + ); + }); + it("gives the half-typed {{VALUE:}} the autocomplete inserts real copy", async () => { const formatter = new FormatDisplayFormatter(app, plugin); await formatter.format("{{VALUE:}}"); diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index b62be02c1..da605ebd2 100644 --- a/src/formatters/previewDiagnostics.ts +++ b/src/formatters/previewDiagnostics.ts @@ -72,10 +72,16 @@ function stripBrandPrefix(message: string): string { * message is what makes the bug report actionable. */ export function describePreviewFailure(error: unknown): string | null { + // A cancelled prompt is a user action, not an authoring mistake. The preview + // formatters never prompt, but a nested resolver still can. Check the RAW + // value first: QuickAdd's modals reject with a bare string + // (`rejectPromise("No input given.")`), and `isCancellationError` only ever + // matches strings - so an `instanceof Error` gate ahead of it would let every + // cancellation through as "Preview unavailable". + if (isCancellationError(error)) return null; if (!(error instanceof Error)) return PREVIEW_FAILED_MESSAGE; const message = error.message ?? ""; - // A cancelled prompt is a user action, not an authoring mistake. The preview - // formatters never prompt, but a nested resolver still can. + // Also covers a cancellation that was wrapped in an Error on the way up. if (isCancellationError(message)) return null; // By far the most frequent throw: the token autocomplete inserts `{{VALUE:}}` // with the caret between the colon and the braces, and VARIABLE_REGEX matches From 87d3f5708aac8c0e0697c285036189c037190651 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 00:28:52 +0200 Subject: [PATCH 11/12] fix(builder): name preview-diagnostic severity in text, not colour alone The inline problems distinguished a warning from an error only by text colour (`--text-warning` vs `--text-error`), which is WCAG 1.4.1. A visually-hidden "Warning: " / "Error: " rather than a printed prefix: these messages already clamp at three lines, and the distinction a sighted user actually needs - did this format resolve at all - is carried by the row label, which reads "Unresolved:" instead of "Preview:" whenever an error is present. So a visible prefix would cost real estate to restate something already in text. Caught by CodeRabbit on #1560. --- .../components/FormatPreviewField.svelte | 9 +++++- .../components/FormatPreviewField.test.ts | 31 ++++++++++++++++++- src/styles.css | 15 +++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index 72532f698..955672135 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -142,7 +142,14 @@ $effect(() => { class:qa-preview-issue--error={diagnostic.severity === "error"} title={diagnostic.message} > - {diagnostic.message} + + {diagnostic.severity === "error" ? "Error: " : "Warning: "}{diagnostic.message} {/each} diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts index 1b2537593..15a10185a 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts @@ -66,9 +66,17 @@ async function settleAndIdle() { await settle(); } +/** The visible text of each problem line (drops the screen-reader severity). */ const issues = (container: HTMLElement) => Array.from(container.querySelectorAll(".qa-preview-issue")).map((el) => - el.textContent?.trim(), + Array.from(el.childNodes) + .filter( + (n) => + !(n instanceof HTMLElement && n.classList.contains("qa-visually-hidden")), + ) + .map((n) => n.textContent ?? "") + .join("") + .trim(), ); describe("FormatPreviewField", () => { @@ -107,6 +115,27 @@ describe("FormatPreviewField", () => { ]); }); + it("names the severity in text, not colour alone", async () => { + const { container } = render(FormatPreviewField, { + props: { value: "{{VALUE:title|case:pasc}}", app, plugin }, + }); + await settleAndIdle(); + expect( + container.querySelector(".qa-preview-issue .qa-visually-hidden") + ?.textContent, + ).toBe("Warning: "); + + const errored = render(FormatPreviewField, { + props: { value: "{{VALUE:a,b|text:x}}", app, plugin }, + }); + await settleAndIdle(); + expect( + errored.container.querySelector( + ".qa-preview-issue .qa-visually-hidden", + )?.textContent, + ).toBe("Error: "); + }); + it("holds the problem back until the field has been still", async () => { const { container } = render(FormatPreviewField, { props: { value: "{{VALUE:title|case:pasc}}", app, plugin }, diff --git a/src/styles.css b/src/styles.css index 471ef2a13..1ab35e438 100644 --- a/src/styles.css +++ b/src/styles.css @@ -216,6 +216,21 @@ color: var(--text-muted); overflow-wrap: anywhere; } +/* Text that only assistive tech reads. The standard clip-rect technique: unlike + `display:none` / `visibility:hidden` it stays in the accessibility tree. */ +.qa-visually-hidden { + position: absolute; + width: 1px; + height: 1px; + margin: -1px; + padding: 0; + overflow: hidden; + clip: rect(0 0 0 0); + clip-path: inset(50%); + white-space: nowrap; + border: 0; +} + /* Problems the preview pass ran into, shown here instead of as a Notice per keystroke (#1558). Colour carries severity — no glyph — matching .qa-folder-mode-warning. Clamped with the full text in `title`, because the From 6d043ddd03b78f00293697bc374950c1c3d0c4b8 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 00:37:09 +0200 Subject: [PATCH 12/12] style(builder): drop the deprecated clip fallback from .qa-visually-hidden `clip: rect(0 0 0 0)` is the legacy half of the visually-hidden idiom and is deprecated; `clip-path: inset(50%)` alone does the clipping. `clip-path` has been supported since Chrome 55 and the plugin's minAppVersion is 1.13.0, so there is no Electron QuickAdd runs on that needs the fallback. Verified in the live vault after the change: the element is still 1x1, still absolutely positioned, computed `clip-path: inset(50%)`, and its "Warning: " text is present in the DOM without being visible. Caught by CodeRabbit's stylelint pass on #1560. --- src/styles.css | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/styles.css b/src/styles.css index 1ab35e438..855e159ed 100644 --- a/src/styles.css +++ b/src/styles.css @@ -216,8 +216,10 @@ color: var(--text-muted); overflow-wrap: anywhere; } -/* Text that only assistive tech reads. The standard clip-rect technique: unlike - `display:none` / `visibility:hidden` it stays in the accessibility tree. */ +/* Text that only assistive tech reads: unlike `display:none` / + `visibility:hidden` it stays in the accessibility tree. No deprecated + `clip: rect()` fallback - `clip-path` has been supported since Chrome 55, and + the plugin's minAppVersion (1.13.0) is far past that. */ .qa-visually-hidden { position: absolute; width: 1px; @@ -225,7 +227,6 @@ margin: -1px; padding: 0; overflow: hidden; - clip: rect(0 0 0 0); clip-path: inset(50%); white-space: nowrap; border: 0;