From 7f9ff8df71e482d75ccc39bfee1650720596efe6 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 07:57:06 +0200 Subject: [PATCH 1/4] fix(ai): stop previewing a substitution the system prompt never gets #1565 asked for four modals to be brought under the inert-preview mechanism #1560 built for the choice builders. Three of the four should not have a format preview at all: the AI system prompt is sent to the model VERBATIM. Only the prompt (or prompt template) goes through the formatter. Every path agrees: - `runAIAssistant` formats `targetPrompt` (AIAssistant.ts:295) and passes `systemPrompt` raw to `OpenAIRequest` (:308). - `Prompt` (:408/:418) and `ChunkedPrompt` (:936) do the same - and the chunked path sizes its budget with `estimateTokenCount(systemPrompt)` on the raw string (:897), which is the same admission. - `MacroChoiceEngine.executeAIAssistant` hands `command.systemPrompt` straight through (:668) while its formatter callback wraps only the prompt (:675). - `Agent.buildSeedMessages` pushes `system` raw and formats `prompt`. - `OpenAIRequest` then puts it on the wire unchanged (:198/:246/:312). The docs agree too: AIAssistant.md:56 says the prompt template "is a Markdown note in the prompt template folder, not raw prompt text", and nothing anywhere claims system prompts take tokens. Only the UI disagreed. All three system-prompt fields ran a live FormatDisplayFormatter and attached a `{{` token autocomplete, so `{{DATE}}` previewed as 2026-07-27 and then reached the model as eight literal characters. That is a worse defect than the one #1565 reports: the preview did not merely fail to warn, it asserted a substitution that will not happen. Mounting the GOOD preview implementation there would have made the lie better-typeset, inline-diagnosed and announced to screen readers. So the affordance goes instead. Removing it also closes #1568 outright: with the shipped token-free default prompt, that preview was a character-for-character duplicate of the textarea above it, at full body size, and read as the modal printing the prompt twice. There is nothing to label if there is nothing to show. What replaces it, for the authors who already have a token in there and would otherwise get no signal at all: one muted line under the field, shown only when the value contains `{{`, naming what happens and pointing at the prompt template - the thing that IS formatted. Muted rather than warning-coloured, matching `.qa-ai-token-note` directly above it in the same modals: like that line it states how the field behaves, and a `{{` a user wants the model to read literally is a legitimate thing to type here. Whether system prompts SHOULD be formattable is a real question, and a bigger one than these two issues - silent behaviour change for existing vaults, a prompting `{{VALUE:}}` inside an agent loop, and a chunk budget computed before the substitution. Filed as #1572. If it lands, the preview and the token autocomplete come back with it and this note is deleted. AIAssistant.systemPromptLiteral.test.ts pins the invariant in both directions - the system prompt arrives byte-identical AND the formatter is never called with it - so the person who changes the runtime is told to revisit the UI. Verified to fail against a one-line mutation that formats it. Drive-by: `addSlider` moves into the shared obsidian test stub, replacing the local shim AIAssistantInfiniteCommandSettingsModal.test.ts carried for it. Refs #1565 Closes #1568 --- .../AIAssistant.systemPromptLiteral.test.ts | 175 +++++++++++++ src/gui/AIAssistantSettingsModal.ts | 40 ++- .../AIAssistantCommandSettingsModal.ts | 44 ++-- ...istantInfiniteCommandSettingsModal.test.ts | 26 +- ...AIAssistantInfiniteCommandSettingsModal.ts | 44 ++-- src/gui/ai/systemPromptFields.test.ts | 242 ++++++++++++++++++ src/gui/ai/systemPromptLiteralNote.test.ts | 79 ++++++ src/gui/ai/systemPromptLiteralNote.ts | 52 ++++ src/styles.css | 19 ++ tests/obsidian-stub.ts | 51 ++++ 10 files changed, 669 insertions(+), 103 deletions(-) create mode 100644 src/ai/AIAssistant.systemPromptLiteral.test.ts create mode 100644 src/gui/ai/systemPromptFields.test.ts create mode 100644 src/gui/ai/systemPromptLiteralNote.test.ts create mode 100644 src/gui/ai/systemPromptLiteralNote.ts diff --git a/src/ai/AIAssistant.systemPromptLiteral.test.ts b/src/ai/AIAssistant.systemPromptLiteral.test.ts new file mode 100644 index 00000000..e36b0309 --- /dev/null +++ b/src/ai/AIAssistant.systemPromptLiteral.test.ts @@ -0,0 +1,175 @@ +import { afterAll, beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import type { AIProvider } from "./Provider"; +import type { CommonResponse } from "./OpenAIRequest"; + +/** + * The invariant behind #1565 / #1568: the AI system prompt is sent to the model + * VERBATIM. Only the prompt (or prompt template) goes through the formatter. + * + * Three modals used to claim otherwise — a live preview resolving the tokens + * plus a `{{` autocomplete offering them — so `{{DATE}}` previewed as a date and + * then reached the model as eight literal characters. The affordance is gone; + * this pins the behaviour it was lying about, in both directions: + * + * - the system prompt arrives at OpenAIRequest byte-identical to the input, and + * - the formatter is never invoked with it. + * + * If #1572 ever makes system prompts formattable, this test fails, and whoever + * changes it is the person who should also restore the preview and the token + * autocomplete in AIAssistantSettingsModal / AIAssistantCommandSettingsModal / + * AIAssistantInfiniteCommandSettingsModal. + */ + +const storeState = vi.hoisted(() => ({ + disableOnlineFeatures: false, +})); + +const mocks = vi.hoisted(() => ({ + makeRequest: vi.fn(), + openAIRequest: vi.fn(), + isLikelyContextLimitError: vi.fn(() => false), + getModelMaxTokens: vi.fn(() => 100000), + getMarkdownFilesInFolder: vi.fn(() => []), +})); + +vi.mock("src/settingsStore", () => ({ + settingsStore: { getState: () => storeState }, +})); + +vi.mock("./OpenAIRequest", () => ({ + OpenAIRequest: mocks.openAIRequest, +})); + +vi.mock("./providerErrors", () => ({ + isLikelyContextLimitError: mocks.isLikelyContextLimitError, +})); + +vi.mock("./aiHelpers", () => ({ + getModelMaxTokens: mocks.getModelMaxTokens, +})); + +vi.mock("src/utilityObsidian", () => ({ + getMarkdownFilesInFolder: mocks.getMarkdownFilesInFolder, +})); + +const { Prompt, ChunkedPrompt } = await import("./AIAssistant"); + +vi.stubGlobal("sleep", async () => {}); + +afterAll(() => { + vi.unstubAllGlobals(); +}); + +/** Contains every token shape the removed preview used to resolve on screen. */ +const SYSTEM_PROMPT = + "You are a helpful assistant. Today is {{DATE}} and the note is {{VALUE:title}}."; + +function makeApp(): App { + return {} as App; +} + +function response(content: string): CommonResponse { + return { + id: content, + model: "test-model", + content, + usage: { promptTokens: 1, completionTokens: 1, totalTokens: 2 }, + stopReason: "stop", + stopSequence: null, + created: 0, + }; +} + +function baseSettings() { + return { + apiKey: "key", + model: { name: "test-model", maxTokens: 100000 }, + provider: { + name: "TestProvider", + endpoint: "https://example.test/v1", + apiKey: "", + models: [], + modelSource: "providerApi", + } as AIProvider, + outputVariableName: "output", + showAssistantMessages: false, + systemPrompt: SYSTEM_PROMPT, + modelOptions: {}, + }; +} + +/** Resolves every token, so an accidentally formatted system prompt is obvious. */ +const resolvingFormatter = vi.fn(async (input: string) => + input.replace(/\{\{[^}]*\}\}/g, "RESOLVED"), +); + +/** The `systemPrompt` argument OpenAIRequest was constructed with. */ +function systemPromptSentToProvider(): unknown { + expect(mocks.openAIRequest).toHaveBeenCalled(); + return mocks.openAIRequest.mock.calls[0][4]; +} + +beforeEach(() => { + vi.clearAllMocks(); + storeState.disableOnlineFeatures = false; + mocks.openAIRequest.mockReturnValue(mocks.makeRequest); + mocks.getModelMaxTokens.mockReturnValue(100000); + mocks.makeRequest.mockImplementation(async (prompt: string) => + response(`response:${prompt}`), + ); +}); + +describe("the AI system prompt is sent verbatim", () => { + it("Prompt sends the system prompt unformatted while formatting the prompt", async () => { + await Prompt( + makeApp(), + { ...baseSettings(), prompt: "Summarise {{VALUE:body}}" }, + resolvingFormatter, + ); + + expect(systemPromptSentToProvider()).toBe(SYSTEM_PROMPT); + // The prompt IS formatted — this is the contrast that makes the system + // prompt's exemption a deliberate behaviour rather than a dead code path. + expect(mocks.makeRequest).toHaveBeenCalledWith("Summarise RESOLVED"); + expect(resolvingFormatter).toHaveBeenCalledTimes(1); + expect(resolvingFormatter).not.toHaveBeenCalledWith(SYSTEM_PROMPT); + }); + + it("ChunkedPrompt sends the system prompt unformatted on every chunk request", async () => { + await ChunkedPrompt( + makeApp(), + { + ...baseSettings(), + text: "alpha\nbeta", + promptTemplate: "Chunk: {{VALUE:chunk}}", + chunkSeparator: /\n/g, + resultJoiner: "|", + shouldMerge: false, + maxChunkTokens: 8, + }, + async (_template: string, variables: { [k: string]: unknown }) => + `Chunk: ${String(variables.chunk)}`, + ); + + // One OpenAIRequest is built for the whole run and reused per chunk, so + // checking the constructor argument covers every dispatched request. + expect(systemPromptSentToProvider()).toBe(SYSTEM_PROMPT); + expect(mocks.makeRequest.mock.calls.length).toBeGreaterThan(0); + for (const [prompt] of mocks.makeRequest.mock.calls) { + expect(prompt).not.toContain("RESOLVED"); + } + }); + + it("keeps the token characters intact rather than stripping them", async () => { + await Prompt( + makeApp(), + { ...baseSettings(), prompt: "hello" }, + resolvingFormatter, + ); + + // The specific failure mode the removed preview hid: authors saw a date and + // the model saw the eight characters. + expect(systemPromptSentToProvider()).toContain("{{DATE}}"); + }); +}); diff --git a/src/gui/AIAssistantSettingsModal.ts b/src/gui/AIAssistantSettingsModal.ts index a6f8fc81..0a8aa82b 100644 --- a/src/gui/AIAssistantSettingsModal.ts +++ b/src/gui/AIAssistantSettingsModal.ts @@ -1,9 +1,7 @@ import type { App } from "obsidian"; import { Modal, Setting, TextAreaComponent } from "obsidian"; import type { QuickAddSettings } from "src/settings"; -import { FormatSyntaxSuggester } from "./suggesters/formatSyntaxSuggester"; -import { getQuickAddInstance } from "src/quickAddInstance"; -import { FormatDisplayFormatter } from "src/formatters/formatDisplayFormatter"; +import { mountSystemPromptLiteralNote } from "./ai/systemPromptLiteralNote"; import { AIAssistantProvidersModal } from "./AIAssistantProvidersModal"; import { populateModelDropdown } from "./modelSelect"; import { GenericTextSuggester } from "./suggesters/genericTextSuggester"; @@ -149,32 +147,24 @@ export class AIAssistantSettingsModal extends Modal { .setDesc("The default system prompt for the AI Assistant"); const textAreaComponent = new TextAreaComponent(contentEl); + textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); + + // No format preview and no `{{` token autocomplete here: the system prompt + // is sent to the model verbatim (see mountSystemPromptLiteralNote). The + // preview this replaces resolved the tokens on screen and was, for the + // shipped token-free default, a character-for-character duplicate of the + // textarea above it (#1568). + const updateLiteralNote = mountSystemPromptLiteralNote( + contentEl, + this.settings.defaultSystemPrompt ?? "", + ); + textAreaComponent .setValue(this.settings.defaultSystemPrompt) - .onChange(async (value) => { + .onChange((value) => { this.settings.defaultSystemPrompt = value; - - formatDisplay.innerText = await displayFormatter.format(value); + updateLiteralNote(value); }); - - new FormatSyntaxSuggester( - this.app, - textAreaComponent.inputEl, - getQuickAddInstance() - ); - const displayFormatter = new FormatDisplayFormatter( - this.app, - getQuickAddInstance() - ); - - textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); - - const formatDisplay = this.contentEl.createEl("span"); - - void (async () => - (formatDisplay.innerText = await displayFormatter.format( - this.settings.defaultSystemPrompt ?? "" - )))(); } onClose(): void { diff --git a/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts b/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts index 9d88e2bb..09269ebf 100644 --- a/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts +++ b/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts @@ -1,8 +1,6 @@ import type { App } from "obsidian"; import { ButtonComponent, Modal, Setting, TextAreaComponent, debounce } from "obsidian"; -import { FormatSyntaxSuggester } from "./../suggesters/formatSyntaxSuggester"; -import { getQuickAddInstance } from "src/quickAddInstance"; -import { FormatDisplayFormatter } from "src/formatters/formatDisplayFormatter"; +import { mountSystemPromptLiteralNote } from "../ai/systemPromptLiteralNote"; import type { IAIAssistantCommand } from "src/types/macros/QuickCommands/IAIAssistantCommand"; import { GenericTextSuggester } from "../suggesters/genericTextSuggester"; import { getMarkdownFilesInFolder } from "src/utilityObsidian"; @@ -240,38 +238,30 @@ export class AIAssistantCommandSettingsModal extends Modal { container.appendChild(tokenCountNote); const textAreaComponent = new TextAreaComponent(contentEl); - textAreaComponent - .setValue(this.settings.systemPrompt) - .onChange(async (value) => { - this.settings.systemPrompt = value; - - formatDisplay.innerText = await displayFormatter.format(value); - updateTokenCount(); - }); + textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); - new FormatSyntaxSuggester( - this.app, - textAreaComponent.inputEl, - getQuickAddInstance() - ); - const displayFormatter = new FormatDisplayFormatter( - this.app, - getQuickAddInstance() + // No format preview and no `{{` token autocomplete here: the system prompt + // is sent to the model verbatim (see mountSystemPromptLiteralNote). The + // token count above is already computed on the raw string, which is the + // same admission. + const updateLiteralNote = mountSystemPromptLiteralNote( + contentEl, + this.settings.systemPrompt ?? "", ); - textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); - - const formatDisplay = this.contentEl.createEl("span"); const updateTokenCount = debounce(() => { tokenCount.innerText = `Estimated tokens: ${this.systemPromptTokenLength}`; }, 50); - updateTokenCount(); + textAreaComponent + .setValue(this.settings.systemPrompt) + .onChange((value) => { + this.settings.systemPrompt = value; + updateLiteralNote(value); + updateTokenCount(); + }); - void (async () => - (formatDisplay.innerText = await displayFormatter.format( - this.settings.systemPrompt ?? "" - )))(); + updateTokenCount(); } addShowAdvancedSettingsToggle(container: HTMLElement) { diff --git a/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.test.ts b/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.test.ts index a2844a9f..01f1b5f4 100644 --- a/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.test.ts +++ b/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.test.ts @@ -1,5 +1,5 @@ -import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; -import { App, Setting } from "obsidian"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { App } from "obsidian"; import { DEFAULT_SETTINGS } from "src/settings"; import { settingsStore } from "src/settingsStore"; import { deepClone } from "src/utils/deepClone"; @@ -45,28 +45,6 @@ function makeSettings(model: string): IInfiniteAIAssistantCommand { } describe("InfiniteAIAssistantCommandSettingsModal max-chunk-tokens", () => { - beforeAll(() => { - // The shared obsidian stub's Setting lacks addSlider; shim it locally so - // the modal can render the Max chunk tokens slider (documented harness gap). - const settingProto = Setting.prototype as unknown as { - addSlider?: (cb: (slider: unknown) => void) => unknown; - }; - settingProto.addSlider ??= function addSlider( - this: unknown, - cb: (slider: unknown) => void, - ) { - const slider = { - setLimits: () => slider, - setValue: () => slider, - setInstant: () => slider, - setDynamicTooltip: () => slider, - onChange: () => slider, - }; - cb(slider); - return this; - }; - }); - beforeEach(() => { settingsStore.replaceState(deepClone(DEFAULT_SETTINGS)); }); diff --git a/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts b/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts index 628e146d..15358404 100644 --- a/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts +++ b/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts @@ -1,8 +1,6 @@ import type { App } from "obsidian"; import { Modal, Setting, TextAreaComponent, debounce } from "obsidian"; -import { FormatSyntaxSuggester } from "./../suggesters/formatSyntaxSuggester"; -import { getQuickAddInstance } from "src/quickAddInstance"; -import { FormatDisplayFormatter } from "src/formatters/formatDisplayFormatter"; +import { mountSystemPromptLiteralNote } from "../ai/systemPromptLiteralNote"; import type { IInfiniteAIAssistantCommand } from "src/types/macros/QuickCommands/IAIAssistantCommand"; import GenericInputPrompt from "../GenericInputPrompt/GenericInputPrompt"; import { estimateTokenCount } from "src/ai/tokenEstimator"; @@ -152,38 +150,30 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal { container.appendChild(tokenCountNote); const textAreaComponent = new TextAreaComponent(contentEl); - textAreaComponent - .setValue(this.settings.systemPrompt) - .onChange(async (value) => { - this.settings.systemPrompt = value; - - formatDisplay.innerText = await displayFormatter.format(value); - updateTokenCount(); - }); + textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); - new FormatSyntaxSuggester( - this.app, - textAreaComponent.inputEl, - getQuickAddInstance() - ); - const displayFormatter = new FormatDisplayFormatter( - this.app, - getQuickAddInstance() + // No format preview and no `{{` token autocomplete here: the system prompt + // is sent to the model verbatim (see mountSystemPromptLiteralNote). This + // path is the one that most obviously admitted it - the chunk budget below + // is sized with estimateTokenCount on the raw string. + const updateLiteralNote = mountSystemPromptLiteralNote( + contentEl, + this.settings.systemPrompt ?? "", ); - textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); - - const formatDisplay = this.contentEl.createEl("span"); const updateTokenCount = debounce(() => { tokenCount.innerText = `Estimated tokens: ${this.systemPromptTokenLength}`; }, 50); - updateTokenCount(); + textAreaComponent + .setValue(this.settings.systemPrompt) + .onChange((value) => { + this.settings.systemPrompt = value; + updateLiteralNote(value); + updateTokenCount(); + }); - void (async () => - (formatDisplay.innerText = await displayFormatter.format( - this.settings.systemPrompt ?? "" - )))(); + updateTokenCount(); } addShowAdvancedSettingsToggle(container: HTMLElement) { diff --git a/src/gui/ai/systemPromptFields.test.ts b/src/gui/ai/systemPromptFields.test.ts new file mode 100644 index 00000000..a610a37a --- /dev/null +++ b/src/gui/ai/systemPromptFields.test.ts @@ -0,0 +1,242 @@ +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * The three system-prompt modals must not offer a format affordance: the system + * prompt reaches the model verbatim (pinned by + * AIAssistant.systemPromptLiteral.test.ts), so a live preview resolving its + * tokens asserted a substitution that never happens (#1565), and on the shipped + * token-free default it was a character-for-character duplicate of the textarea + * above it (#1568). + * + * The two mocks below are the load-bearing assertions: they COUNT construction. + * A test that only queried the DOM would keep passing if someone reinstated the + * formatter but rendered it somewhere new. + */ + +const mocks = vi.hoisted(() => ({ + formatDisplayFormatter: vi.fn(), + formatSyntaxSuggester: vi.fn(), +})); + +vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); +vi.mock("src/settingsStore", () => ({ + settingsStore: { + getState: () => ({ + ai: { + promptTemplatesFolderPath: "", + showAssistant: false, + providers: [ + { + id: "test", + name: "TestProvider", + endpoint: "https://example.test/v1", + apiKey: "", + models: [{ name: "gpt-test", maxTokens: 1000 }], + modelSource: "providerApi", + }, + ], + }, + disableOnlineFeatures: false, + }), + }, +})); +vi.mock("src/quickAddInstance", () => ({ + getQuickAddInstance: vi.fn(() => ({})), +})); +vi.mock("src/utilityObsidian", () => ({ + getMarkdownFilesInFolder: vi.fn(() => []), + getAllFolderPathsInVault: vi.fn(() => []), +})); +// Partial: the Infinite modal's chunk-budget slider reaches +// estimateModelInputBudget through aiHelpers, so only the count is stubbed. +vi.mock("src/ai/tokenEstimator", async (importOriginal) => ({ + ...(await importOriginal>()), + estimateTokenCount: vi.fn(() => 0), +})); +vi.mock("src/gui/suggesters/genericTextSuggester", () => ({ + GenericTextSuggester: class {}, +})); +vi.mock("src/formatters/formatDisplayFormatter", () => ({ + FormatDisplayFormatter: class { + constructor(...args: unknown[]) { + mocks.formatDisplayFormatter(...args); + } + async format(input: string) { + return input; + } + }, +})); +vi.mock("src/gui/suggesters/formatSyntaxSuggester", () => ({ + FormatSyntaxSuggester: class { + constructor(...args: unknown[]) { + mocks.formatSyntaxSuggester(...args); + } + }, +})); + +import { App } from "obsidian"; +import type { IAIAssistantCommand } from "src/types/macros/QuickCommands/IAIAssistantCommand"; +import type { IInfiniteAIAssistantCommand } from "src/types/macros/QuickCommands/IAIAssistantCommand"; +import type { QuickAddSettings } from "src/settings"; +import { AIAssistantSettingsModal } from "src/gui/AIAssistantSettingsModal"; +import { AIAssistantCommandSettingsModal } from "src/gui/MacroGUIs/AIAssistantCommandSettingsModal"; +import { InfiniteAIAssistantCommandSettingsModal } from "src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal"; + +const PROSE_PROMPT = "As an AI assistant within Obsidian, help the user."; +const TOKENED_PROMPT = "Today is {{DATE}}. Help the user."; + +function testApp(): App { + const app = new App() as App & { + dom: { appContainerEl: HTMLElement }; + keymap: { pushScope: () => void; popScope: () => void }; + }; + app.dom = { appContainerEl: document.body }; + app.keymap = { pushScope: vi.fn(), popScope: vi.fn() }; + return app; +} + +function aiSettings(defaultSystemPrompt: string): QuickAddSettings["ai"] { + return { + defaultModel: "gpt-test", + defaultSystemPrompt, + promptTemplatesFolderPath: "", + showAssistant: false, + providers: [], + } as unknown as QuickAddSettings["ai"]; +} + +function aiCommand(systemPrompt: string): IAIAssistantCommand { + return { + id: "ai-1", + name: "AI Assistant", + type: "AIAssistant", + model: "gpt-test", + systemPrompt, + outputVariableName: "output", + modelParameters: {}, + promptTemplate: { enable: false, name: "" }, + } as IAIAssistantCommand; +} + +function infiniteCommand(systemPrompt: string): IInfiniteAIAssistantCommand { + return { + ...aiCommand(systemPrompt), + resultJoiner: "\\n", + chunkSeparator: "\\n", + maxChunkTokens: 100, + mergeChunks: false, + } as unknown as IInfiniteAIAssistantCommand; +} + +/** Each modal, paired with a factory that opens it and returns its contentEl. */ +const MODALS: Array<{ + name: string; + open: (systemPrompt: string) => { contentEl: HTMLElement; close: () => void }; +}> = [ + { + name: "AIAssistantSettingsModal (default system prompt)", + open: (systemPrompt) => { + const modal = new AIAssistantSettingsModal( + testApp(), + aiSettings(systemPrompt), + ); + return { contentEl: modal.contentEl, close: () => modal.close() }; + }, + }, + { + name: "AIAssistantCommandSettingsModal (system prompt)", + open: (systemPrompt) => { + const modal = new AIAssistantCommandSettingsModal( + testApp(), + aiCommand(systemPrompt), + ); + return { contentEl: modal.contentEl, close: () => modal.close() }; + }, + }, + { + name: "InfiniteAIAssistantCommandSettingsModal (system prompt)", + open: (systemPrompt) => { + const modal = new InfiniteAIAssistantCommandSettingsModal( + testApp(), + infiniteCommand(systemPrompt), + ); + return { contentEl: modal.contentEl, close: () => modal.close() }; + }, + }, +]; + +function promptTextarea(contentEl: HTMLElement): HTMLTextAreaElement { + const textarea = contentEl.querySelector( + "textarea.qa-ai-prompt-textarea", + ); + if (!textarea) throw new Error("System prompt textarea not found"); + return textarea; +} + +describe("AI system-prompt fields offer no format affordance", () => { + beforeAll(() => { + // The obsidian stub's Modal has no onClose; the modals call super.onClose(). + for (const Ctor of [ + AIAssistantSettingsModal, + AIAssistantCommandSettingsModal, + InfiniteAIAssistantCommandSettingsModal, + ]) { + const proto = Object.getPrototypeOf(Ctor.prototype) as { + onClose?: () => void; + }; + proto.onClose ??= function onClose() {}; + } + }); + + beforeEach(() => { + vi.clearAllMocks(); + document.body.innerHTML = ""; + }); + + for (const { name, open } of MODALS) { + describe(name, () => { + it("builds no preview formatter and no token autocomplete", () => { + const { contentEl, close } = open(PROSE_PROMPT); + + expect(mocks.formatDisplayFormatter).not.toHaveBeenCalled(); + expect(mocks.formatSyntaxSuggester).not.toHaveBeenCalled(); + // #1568: no bare span echoing the prompt back under the field. + expect(contentEl.textContent).not.toContain(PROSE_PROMPT); + expect(promptTextarea(contentEl).value).toBe(PROSE_PROMPT); + + close(); + }); + + it("stays quiet for a prose prompt and explains itself once a token appears", () => { + const { contentEl, close } = open(PROSE_PROMPT); + const note = contentEl.querySelector(".qa-literal-format-note"); + expect(note).not.toBeNull(); + expect( + note?.classList.contains("qa-literal-format-note--shown"), + ).toBe(false); + + const textarea = promptTextarea(contentEl); + textarea.value = TOKENED_PROMPT; + textarea.dispatchEvent(new Event("input", { bubbles: true })); + + expect( + note?.classList.contains("qa-literal-format-note--shown"), + ).toBe(true); + + close(); + }); + + it("shows the note immediately when the stored prompt already has a token", () => { + const { contentEl, close } = open(TOKENED_PROMPT); + + expect( + contentEl + .querySelector(".qa-literal-format-note") + ?.classList.contains("qa-literal-format-note--shown"), + ).toBe(true); + + close(); + }); + }); + } +}); diff --git a/src/gui/ai/systemPromptLiteralNote.test.ts b/src/gui/ai/systemPromptLiteralNote.test.ts new file mode 100644 index 00000000..7bf00c2e --- /dev/null +++ b/src/gui/ai/systemPromptLiteralNote.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from "vitest"; +import { mountSystemPromptLiteralNote } from "./systemPromptLiteralNote"; + +function host(): HTMLElement { + const el = document.createElement("div"); + document.body.appendChild(el); + return el; +} + +function noteOf(container: HTMLElement): HTMLElement { + const note = container.querySelector(".qa-literal-format-note"); + expect(note).not.toBeNull(); + return note as HTMLElement; +} + +/** The class the stylesheet keys `display: block` off. */ +function isShown(container: HTMLElement): boolean { + return noteOf(container).classList.contains("qa-literal-format-note--shown"); +} + +describe("mountSystemPromptLiteralNote", () => { + it("stays hidden for a prose prompt", () => { + const container = host(); + mountSystemPromptLiteralNote( + container, + "As an AI assistant within Obsidian, your primary goal is to help users.", + ); + + expect(isShown(container)).toBe(false); + }); + + it("shows for a prompt that already contains a token", () => { + const container = host(); + mountSystemPromptLiteralNote(container, "Today is {{DATE}}."); + + expect(isShown(container)).toBe(true); + }); + + it("follows the value as the field is edited, in both directions", () => { + const container = host(); + const update = mountSystemPromptLiteralNote(container, ""); + + expect(isShown(container)).toBe(false); + update("You are helpful."); + expect(isShown(container)).toBe(false); + update("You are helpful. Today is {{DATE}}."); + expect(isShown(container)).toBe(true); + update("You are helpful."); + expect(isShown(container)).toBe(false); + }); + + it("names the token syntax it is about and points at the prompt template", () => { + const container = host(); + mountSystemPromptLiteralNote(container, "{{DATE}}"); + + const text = noteOf(container).textContent ?? ""; + expect(text).toContain("{{DATE}}"); + expect(text).toContain("prompt template"); + }); + + it("exists in the DOM while hidden, so it renders under its own field", () => { + // Created up front rather than on demand: these modals append as they build, + // so a lazily created note would land at the bottom of the modal. + const container = host(); + const marker = container.createEl("div"); + mountSystemPromptLiteralNote(container, "no tokens here"); + const trailing = container.createEl("div"); + + const note = noteOf(container); + expect( + marker.compareDocumentPosition(note) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + expect( + trailing.compareDocumentPosition(note) & + Node.DOCUMENT_POSITION_PRECEDING, + ).toBeTruthy(); + }); +}); diff --git a/src/gui/ai/systemPromptLiteralNote.ts b/src/gui/ai/systemPromptLiteralNote.ts new file mode 100644 index 00000000..12b69c64 --- /dev/null +++ b/src/gui/ai/systemPromptLiteralNote.ts @@ -0,0 +1,52 @@ +/** + * The AI system prompt is sent to the model VERBATIM. QuickAdd's format syntax + * is applied to the prompt (or prompt template) only: + * + * - `runAIAssistant` formats `targetPrompt`, passes `systemPrompt` raw + * (src/ai/AIAssistant.ts). + * - `Prompt` / `ChunkedPrompt` do the same, and the chunked path even sizes + * its budget with `estimateTokenCount(systemPrompt)` on the raw string. + * - `MacroChoiceEngine.executeAIAssistant` hands `command.systemPrompt` + * straight through while its formatter callback wraps only the prompt. + * - `Agent.buildSeedMessages` pushes `system` raw and formats `prompt`. + * + * Until #1572 decides otherwise, `{{DATE}}` in a system prompt reaches the model + * as the eight characters `{{DATE}}`. The three system-prompt modals used to + * claim the opposite - a live "preview" resolving the tokens, plus a `{{` + * autocomplete offering them (issues #1565, #1568). Both are gone; this note is + * what replaces them, and only for the authors who already have a token in + * there and would otherwise get no signal at all. + * + * Shown only when the value contains `{{`, so the shipped default prompt (and + * every prose prompt) never sees it. + */ + +const LITERAL_NOTE_TEXT = + "QuickAdd sends this prompt to the model as written - format syntax like {{DATE}} is not resolved here. Use the prompt template for text that needs tokens."; + +/** + * Renders a muted note under a system-prompt field, and returns the updater to + * call from the field's `onChange`. + * + * The element is created up front and hidden rather than created on demand: the + * modals build their DOM imperatively and append as they go, so a lazily created + * note would land at the bottom of the modal instead of under its field. + */ +export function mountSystemPromptLiteralNote( + container: HTMLElement, + initialValue: string, +): (value: string) => void { + const note = container.createEl("div", { + text: LITERAL_NOTE_TEXT, + cls: "qa-literal-format-note", + }); + + // classList, not Obsidian's toggleClass: this runs under the vitest DOM stub + // too, which patches createEl/addClass but not toggleClass. + const update = (value: string): void => { + note.classList.toggle("qa-literal-format-note--shown", value.includes("{{")); + }; + + update(initialValue); + return update; +} diff --git a/src/styles.css b/src/styles.css index d902de45..9078b929 100644 --- a/src/styles.css +++ b/src/styles.css @@ -660,6 +660,25 @@ font-size: var(--font-ui-smaller); } +/* "This field is sent as written" note under a system prompt that contains `{{` + (#1565/#1568). Muted and font-ui-smaller to match .qa-ai-token-note directly + above it in the same modals: like that line it states how the field behaves, + rather than flagging the value as wrong - a `{{` a user wants the model to + read literally is a legitimate thing to type here. + + Hidden by default rather than created on demand, so it can be appended in DOM + order under the field it belongs to instead of at the end of the modal. */ +.qa-literal-format-note { + display: none; + margin: -0.5em 0 1em; + color: var(--text-muted); + font-size: var(--font-ui-smaller); +} + +.qa-literal-format-note--shown { + display: block; +} + .qa-command-button-row { display: flex; justify-content: flex-end; diff --git a/tests/obsidian-stub.ts b/tests/obsidian-stub.ts index fe94dd68..6eebe9e6 100644 --- a/tests/obsidian-stub.ts +++ b/tests/obsidian-stub.ts @@ -136,6 +136,52 @@ export class ToggleComponent extends BaseComponent { } } +export class SliderComponent extends BaseComponent { + sliderEl: HTMLInputElement; + + constructor(containerEl: HTMLElement) { + super(); + this.sliderEl = document.createElement("input"); + this.sliderEl.type = "range"; + containerEl.appendChild(this.sliderEl); + } + + setLimits(min: number, max: number, step: number): this { + this.sliderEl.min = String(min); + this.sliderEl.max = String(max); + this.sliderEl.step = String(step); + return this; + } + + setValue(value: number): this { + this.sliderEl.value = String(value); + return this; + } + + getValue(): number { + return Number(this.sliderEl.value); + } + + setInstant(): this { + return this; + } + + setDynamicTooltip(): this { + return this; + } + + setTooltip(): this { + return this; + } + + onChange(cb: (value: number) => void): this { + this.sliderEl.addEventListener("change", () => + cb(Number(this.sliderEl.value)), + ); + return this; + } +} + export class DropdownComponent extends BaseComponent { selectEl: HTMLSelectElement; @@ -321,6 +367,11 @@ export class Setting { return this; } + addSlider(cb: (component: SliderComponent) => any): this { + cb(new SliderComponent(this.controlEl)); + return this; + } + addComponent(cb: (el: HTMLElement) => T): this { const el = document.createElement("div"); this.controlEl.appendChild(el); From 1b25b8a1b39a6b02f22b0a6d248cc54db3a8b859 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 08:03:20 +0200 Subject: [PATCH 2/4] fix(user-script): give the format option the choice builders' preview The one field of the four #1565 names that really is a format field. A `type: "format"` option is the script author's own declaration, and the documented pattern is that the script resolves it itself with `quickAddApi.format()` (docs/public/scripts/EzImport.js:304) - so unlike the AI system prompt, previewing it is telling the truth. It just wasn't telling it well. `addFormatInput` built its preview span BEFORE the textarea, so it rendered above the field and read as its label - the exact placement #1543 fixed in the builders. It memoized one FormatDisplayFormatter for the modal's lifetime, whose resolved `variables` map short-circuits the next pass. It committed `await format(value)` with no staleness token, so a slow pass could overwrite a newer one. And it threw the collected diagnostics away, so after #1560 silenced the Notices a malformed token reported nothing at all: verified in an isolated Obsidian 1.13.0 vault before this change, typing `{{VALUE:title|case:pasc}}` produced 0 Notices and 0 inline complaints while the preview rendered "Example Title" as if all were well. Rather than reimplement labelling, placement, per-pass construction, staleness and diagnostics imperatively - ~80 lines of the drift that produced this issue - the Svelte FormatPreviewField is mounted. `mountFormatPreview` is a thin seam over the existing `mountComponent`, with a `$state`-backed props object (the documented way to feed reactive props to an imperatively mounted Svelte 5 component, mirroring `createCommandListProps`). It creates and owns its host element: `mount()` writes anchor comment nodes into whatever it is given, and this modal `createEl`s straight onto `contentEl`, so a shared target would interleave the anchors with later `new Setting(...)` rows. Lifecycle is the trap here. `display()` is re-entrant - the constructor calls it, and `migrateSecretSettings()` calls it again from a `void`ed promise that can land at any time - and it empties `contentEl`. Emptying does not stop a Svelte component; it just detaches it, so each re-render would orphan one per format option, each holding a live `$effect` and a 500ms timer. `destroyPreviews()` therefore runs as the first statement of `display()`, before `empty()`, and from a new `onClose()` (the class had none). The regression test for that asserts on the tracked handle list, not on the DOM: an orphan is detached along with contentEl's children, so it vanishes from every query while still running. The first version of this test asserted on `.qa-preview-row` and passed against a build with the teardown removed. Verified in an isolated Obsidian 1.13.0 vault: 0 Notices, "Preview: Logged on 2026-07-27 for Example Title" below the field, and the previously-silent warning inline underneath naming the eight supported |case styles. Opening and closing the modal three times leaves 0 preview rows behind. Closes #1565 --- .../components/mountFormatPreview.svelte.ts | 70 +++++ ...rScriptSettingsModal.formatPreview.test.ts | 265 ++++++++++++++++++ src/gui/MacroGUIs/UserScriptSettingsModal.ts | 53 +++- 3 files changed, 376 insertions(+), 12 deletions(-) create mode 100644 src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts create mode 100644 src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts diff --git a/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts b/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts new file mode 100644 index 00000000..e5ea676d --- /dev/null +++ b/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts @@ -0,0 +1,70 @@ +import type { App } from "obsidian"; +import type QuickAdd from "../../../main"; +import { mountComponent, type MountHandle } from "../../svelte/mountComponent"; +import FormatPreviewField from "./FormatPreviewField.svelte"; + +/** + * Hosts the Svelte `FormatPreviewField` in a modal that builds its DOM + * imperatively. + * + * QuickAdd had one format-preview affordance and two implementations. The Svelte + * one is the correct one: labelled, below the field it previews (#1543), a fresh + * formatter per pass, a monotonic token so a slow pass cannot overwrite a newer + * one's result, and the parse warnings collected inline instead of fired as an + * Obsidian Notice per keystroke (#1558/#1560). The imperative copy had none of + * that, and a second imperative fix would have left the same drift in place - so + * the copy is deleted and the component is mounted instead (#1565). + * + * Lives in a `.svelte.ts` module so `$state` is available: mutating a + * `$state`-backed props object is the documented way to feed reactive props to + * an imperatively mounted Svelte 5 component (see `createCommandListProps`, the + * same pattern for `CommandList`). + */ +export interface FormatPreviewHandle { + /** Push the field's current value; call from the input's `onChange`. */ + setValue(value: string): void; + /** + * Unmount and remove the host. Idempotent, because a Modal's `onClose()` runs + * after a `display()`/`reload()` that already tore the component down. + */ + destroy(): void; +} + +export function mountFormatPreview( + container: HTMLElement, + options: { + app: App; + plugin: QuickAdd; + /** The field's value at mount time. */ + value: string; + }, +): FormatPreviewHandle { + // The helper creates and owns its host rather than accepting an arbitrary + // target: `mount()` writes anchor comment nodes into whatever it is given, and + // these modals `createEl` straight onto `contentEl`, so mounting into a shared + // container would interleave the anchors with later `new Setting(...)` rows. + const host = container.ownerDocument.createElement("div"); + container.appendChild(host); + + const props = $state({ + value: options.value, + app: options.app, + plugin: options.plugin, + }); + + const mounted: MountHandle = mountComponent(host, FormatPreviewField, props); + let destroyed = false; + + return { + setValue(value: string) { + if (destroyed) return; + props.value = value; + }, + destroy() { + if (destroyed) return; + destroyed = true; + mounted.destroy(); + host.remove(); + }, + }; +} diff --git a/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts b/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts new file mode 100644 index 00000000..6a0890a9 --- /dev/null +++ b/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts @@ -0,0 +1,265 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { App } from "obsidian"; +import { CommandType } from "../../types/macros/CommandType"; +import type { IUserScript } from "../../types/macros/IUserScript"; +import { UserScriptSettingsModal } from "./UserScriptSettingsModal"; + +/** + * A user script's `type: "format"` option is the one field of the four #1565 + * names that really is a format field - the option type is the script author's + * own declaration, and the shipped example script resolves it with + * `quickAddApi.format()` (docs/public/scripts/EzImport.js). + * + * So it gets the choice builders' preview rather than losing one: labelled, + * below the field, a formatter per pass, a staleness token, and the parse + * warnings inline instead of silence. + */ + +const mocks = vi.hoisted(() => ({ + /** Every FormatDisplayFormatter built, in construction order. */ + instances: [] as Array<{ formatted: string[] }>, + /** Resolves the next format() call manually, to test out-of-order passes. */ + deferrals: [] as Array<() => void>, + deferNext: false, + diagnostics: [] as Array<{ severity: string; message: string }>, +})); + +vi.mock("../../quickAddInstance", () => ({ + getQuickAddInstance: vi.fn(() => ({})), +})); + +vi.mock("../suggesters/formatSyntaxSuggester", () => ({ + FormatSyntaxSuggester: class {}, +})); + +// Full contract, not just format(): FormatPreviewField also calls +// setTargetFolderPath and reads `diagnostics`. +vi.mock("../../formatters/formatDisplayFormatter", () => ({ + FormatDisplayFormatter: class { + private readonly record = { formatted: [] as string[] }; + diagnostics = { + list: () => mocks.diagnostics, + }; + + constructor() { + mocks.instances.push(this.record); + } + + setTargetFolderPath(): void {} + + format(value: string): Promise { + this.record.formatted.push(value); + const resolved = `resolved(${value})`; + if (mocks.deferNext) { + return new Promise((resolve) => { + mocks.deferrals.push(() => resolve(resolved)); + }); + } + return Promise.resolve(resolved); + } + }, +})); + +function createCommand(): IUserScript { + return { + id: "command-1", + name: "Script", + type: CommandType.UserScript, + path: "scripts/script.js", + settings: {}, + }; +} + +function scriptSettings(options: Record) { + return { name: "Script Settings", options }; +} + +function formatOption(defaultValue: string) { + return { + "Note format": { + type: "format" as const, + defaultValue, + placeholder: "Format", + }, + }; +} + +function flush(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)); +} + +function openModal( + options: Record, +): UserScriptSettingsModal & { contentEl: HTMLElement } { + return new UserScriptSettingsModal( + new App(), + createCommand(), + scriptSettings(options) as ConstructorParameters< + typeof UserScriptSettingsModal + >[2], + ) as UserScriptSettingsModal & { contentEl: HTMLElement }; +} + +function textarea(contentEl: HTMLElement): HTMLTextAreaElement { + const el = contentEl.querySelector( + "textarea.qa-user-script-format-textarea", + ); + if (!el) throw new Error("Format textarea not found"); + return el; +} + +function type(el: HTMLTextAreaElement, value: string) { + el.value = value; + el.dispatchEvent(new Event("input", { bubbles: true })); +} + +beforeEach(() => { + mocks.instances.length = 0; + mocks.deferrals.length = 0; + mocks.diagnostics.length = 0; + mocks.deferNext = false; +}); + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("UserScriptSettingsModal format option preview", () => { + it("renders a labelled preview row BELOW the field it previews", async () => { + const modal = openModal(formatOption("Logged on {{DATE}}")); + await flush(); + + const row = modal.contentEl.querySelector(".qa-preview-row"); + expect(row).not.toBeNull(); + expect(row?.querySelector(".qa-preview-label")?.textContent).toBe( + "Preview: ", + ); + expect(row?.querySelector(".qa-preview-value")?.textContent).toBe( + "resolved(Logged on {{DATE}})", + ); + // #1543: the bare span used to be created before the input and rendered + // above it, where it read as the field's label. + expect( + textarea(modal.contentEl).compareDocumentPosition(row as Node) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + + modal.close(); + }); + + it("builds a formatter per pass rather than memoizing one for the modal", async () => { + const modal = openModal(formatOption("start")); + await flush(); + const afterMount = mocks.instances.length; + + type(textarea(modal.contentEl), "next"); + await flush(); + + // A memoized instance carries a resolved `variables` map into the next + // pass, so an edited option list kept previewing the stale value. + expect(mocks.instances.length).toBeGreaterThan(afterMount); + expect(mocks.instances.at(-1)?.formatted).toEqual(["next"]); + + modal.close(); + }); + + it("drops a stale pass that resolves after a newer one", async () => { + const modal = openModal(formatOption("first")); + await flush(); + + mocks.deferNext = true; + type(textarea(modal.contentEl), "slow"); + await flush(); + type(textarea(modal.contentEl), "fast"); + await flush(); + + expect(mocks.deferrals).toHaveLength(2); + // Resolve the OLDER pass last: without a staleness token it would win. + mocks.deferrals[1](); + await flush(); + mocks.deferrals[0](); + await flush(); + + expect( + modal.contentEl.querySelector(".qa-preview-value")?.textContent, + ).toBe("resolved(fast)"); + + modal.close(); + }); + + it("shows the pass's problems inline instead of firing nothing at all", async () => { + mocks.diagnostics.push({ + severity: "warning", + message: "Unsupported case option 'pasc'", + }); + const modal = openModal(formatOption("{{VALUE:title|case:pasc}}")); + await flush(); + + // Held back until the field has been still (500ms), matching the builders. + expect(modal.contentEl.querySelector(".qa-preview-issue")).toBeNull(); + await new Promise((resolve) => setTimeout(resolve, 600)); + + const issue = modal.contentEl.querySelector(".qa-preview-issue"); + expect(issue?.textContent).toContain("Unsupported case option 'pasc'"); + + modal.close(); + }); + + it("still writes the edited value into the command settings", async () => { + const command = createCommand(); + const modal = new UserScriptSettingsModal( + new App(), + command, + scriptSettings(formatOption("start")) as ConstructorParameters< + typeof UserScriptSettingsModal + >[2], + ); + await flush(); + + type(textarea(modal.contentEl), "Logged on {{DATE}}"); + + expect(command.settings["Note format"]).toBe("Logged on {{DATE}}"); + + modal.close(); + }); + + it("mounts one preview per format option and tears them all down on close", async () => { + const modal = openModal({ + First: { type: "format", defaultValue: "one", placeholder: "" }, + Second: { type: "format", defaultValue: "two", placeholder: "" }, + }); + await flush(); + + expect(modal.contentEl.querySelectorAll(".qa-preview-row")).toHaveLength(2); + + modal.close(); + expect(document.querySelectorAll(".qa-preview-row")).toHaveLength(0); + }); + + it("does not accumulate previews when display() re-runs", async () => { + const modal = openModal(formatOption("start")); + const internals = modal as unknown as { + display: () => void; + previewHandles: unknown[]; + }; + await flush(); + + // migrateSecretSettings() re-runs display() from a voided promise, which + // empties contentEl. Assert on the tracked handles, NOT on the DOM: an + // orphaned component is detached along with contentEl's children, so it + // disappears from every query while its effects and its 500ms diagnostics + // timer keep running. The handle list is the only thing that can tell the + // difference between torn down and merely invisible. + internals.display(); + await flush(); + internals.display(); + await flush(); + + expect(internals.previewHandles).toHaveLength(1); + expect(modal.contentEl.querySelectorAll(".qa-preview-row")).toHaveLength(1); + + modal.close(); + expect(internals.previewHandles).toHaveLength(0); + expect(document.querySelectorAll(".qa-preview-row")).toHaveLength(0); + }); +}); diff --git a/src/gui/MacroGUIs/UserScriptSettingsModal.ts b/src/gui/MacroGUIs/UserScriptSettingsModal.ts index 03f0b2ac..388e56e9 100644 --- a/src/gui/MacroGUIs/UserScriptSettingsModal.ts +++ b/src/gui/MacroGUIs/UserScriptSettingsModal.ts @@ -2,7 +2,10 @@ import type { App } from "obsidian"; import { Modal, Notice, Setting, TextAreaComponent } from "obsidian"; import type { IUserScript } from "../../types/macros/IUserScript"; import { getQuickAddInstance } from "../../quickAddInstance"; -import { FormatDisplayFormatter } from "../../formatters/formatDisplayFormatter"; +import { + mountFormatPreview, + type FormatPreviewHandle, +} from "../ChoiceBuilder/components/mountFormatPreview.svelte"; import { FormatSyntaxSuggester } from "../suggesters/formatSyntaxSuggester"; import { setPasswordOnBlur } from "../../utils/setPasswordOnBlur"; import { initializeUserScriptSettings } from "../../utils/userScriptSettings"; @@ -68,6 +71,9 @@ function formatTitlePart(value: unknown): string { } export class UserScriptSettingsModal extends Modal { + /** One per `type: "format"` option; a script may declare several. */ + private previewHandles: FormatPreviewHandle[] = []; + constructor( app: App, private command: IUserScript, @@ -89,6 +95,9 @@ export class UserScriptSettingsModal extends Modal { protected display() { this.containerEl.addClass("quickAddModal", "userScriptSettingsModal"); + // Before empty(), not after: emptying orphans the mounted components + // instead of tearing them down. + this.destroyPreviews(); this.contentEl.empty(); const titleName = formatTitlePart(this.settings?.name ?? this.command.name); @@ -313,29 +322,49 @@ export class UserScriptSettingsModal extends Modal { private addFormatInput(name: string, value: string, placeholder?: string) { const setting = new Setting(this.contentEl).setName(name); - const formatDisplay = this.contentEl.createEl("span"); const input = new TextAreaComponent(this.contentEl); new FormatSyntaxSuggester(this.app, input.inputEl, getQuickAddInstance()); - const displayFormatter = new FormatDisplayFormatter( - this.app, - getQuickAddInstance() - ); + input.inputEl.addClass("qa-user-script-format-textarea"); + + // Mounted AFTER the textarea, so the preview reads as a result of the field + // rather than as its label - it used to be created first and rendered above + // the input, the exact placement #1543 fixed in the choice builders. + const preview = mountFormatPreview(this.contentEl, { + app: this.app, + plugin: getQuickAddInstance(), + value, + }); + this.previewHandles.push(preview); input .setValue(value) - .onChange(async (value) => { + .onChange((value) => { this.command.settings[name] = value; - formatDisplay.innerText = await displayFormatter.format(value); + preview.setValue(value); this.onCommandChange?.(); }) .setPlaceholder(placeholder ?? ""); - input.inputEl.addClass("qa-user-script-format-textarea"); + return setting; + } - void (async () => - (formatDisplay.innerText = await displayFormatter.format(value)))(); + /** + * Unmount every mounted preview. + * + * `display()` is re-entrant - the constructor calls it, and + * `migrateSecretSettings()` calls it again from a `void`ed promise that can + * land at any time - and it empties `contentEl`. Emptying the DOM does not + * stop a Svelte component: each orphan would keep a live `$effect` and the + * preview's 500ms diagnostics timer rescheduling forever. + */ + private destroyPreviews(): void { + for (const handle of this.previewHandles) handle.destroy(); + this.previewHandles = []; + } - return setting; + onClose(): void { + this.destroyPreviews(); + super.onClose(); } private async migrateSecretSettings() { From 6364c0fb54f7e0cab40816c9756b09d2a2b67b28 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 08:29:13 +0200 Subject: [PATCH 3/4] fix(user-script): open the settings modal when a format option has no default Follow-ups from an adversarial review of the two commits above. The first is a real regression the branch introduced; the rest harden what it shipped. **A format option with no `defaultValue` crashed the modal.** `initializeUserScriptSettings` deliberately skips options whose `defaultValue` is undefined, so `addFormatInput`'s `value: string` arrives as `undefined` from a third-party script - and `FormatPreviewField` calls `value.trim()` during mount. The throw propagates out of `display()` and out of the constructor, so `new UserScriptSettingsModal(...).open()` in CommandList never reaches `.open()` and the gear button silently does nothing. Same for a non-string stored value, e.g. a script that changed an option from `toggle` to `format` between versions. The old code only degraded (a textarea reading the literal text "undefined"), so this is new. Coerced at the boundary where untrusted script data enters typed code, which fixes the "undefined" wart too. Both cases pinned, both verified to fail without the coercion, and verified live: the modal now opens with an empty field and no dangling preview row. **`{{FOLDER}}` previewed a folder the runtime never produces.** The builders fall back to a "Folder/Name" placeholder because a choice HAS a configured target folder that no caller wires in yet. A user-script option has none at all, and `Formatter.replaceMacrosInString` resolves `{{FOLDER}}` to `this.targetFolderPath ?? ""` there. `targetFolderPath` now accepts an explicit `null` meaning "this host has no folder concept", distinct from `undefined` meaning "unknown", and `mountFormatPreview` passes it. The builders pass neither, so their behaviour is byte-identical. Verified live: `{{FOLDER}}/{{DATE}}` previewed `Folder/Name/2026-07-27`, now previews `/2026-07-27`. **The contract test covered two of the four paths its own comment cites.** Every existing test that names `runAIAssistant` mocks it away, so the macro AI command's runtime - the path behind the modal this branch strips - had no assertion on its `systemPrompt` argument anywhere in the suite. `runAIAssistant` and `Agent.buildSeedMessages` are now driven for real. All four cases verified to fail against a one-line mutation that formats the system prompt in that path. **A11y.** The note is now referenced by `aria-describedby` while it is shown (and only while, since an accessible description may include a directly-referenced hidden node). The four textareas that are appended to `contentEl` rather than their Setting's `controlEl` - so nothing associates them with the name above - get an explicit `aria-label`. **Copy.** "Use the prompt template for text that needs tokens" pointed at a control the global AI settings modal does not show; it now says which input IS formatted. The same sentence is added to the AI Assistant docs, so the answer is findable before you type the token rather than only after. **Polish.** The preview host takes a class and a negative top margin: the textareas carry `margin-bottom: 1em` for the gap to the NEXT setting, which left the preview row floating midway between the field it describes and the one below. Also: coverage for the AI modals' `reload()` path (the note must be rebuilt with its field, not hoisted), for `mountFormatPreview`'s destroy-then-setValue path, and a dead `beforeAll` removed - it justified itself with a false claim about the shared obsidian stub, which does define `onClose`. Refs #1565, #1568 --- docs/src/content/docs/docs/AIAssistant.md | 9 ++ .../AIAssistant.systemPromptLiteral.test.ts | 72 +++++++++++- src/gui/AIAssistantSettingsModal.ts | 8 ++ .../components/FormatPreviewField.svelte | 19 ++- .../components/mountFormatPreview.svelte.ts | 17 ++- .../AIAssistantCommandSettingsModal.ts | 4 + ...AIAssistantInfiniteCommandSettingsModal.ts | 4 + ...rScriptSettingsModal.formatPreview.test.ts | 86 +++++++++++++- src/gui/MacroGUIs/UserScriptSettingsModal.ts | 25 +++- src/gui/ai/systemPromptFields.test.ts | 111 ++++++++++++------ src/gui/ai/systemPromptLiteralNote.test.ts | 69 ++++++++--- src/gui/ai/systemPromptLiteralNote.ts | 42 +++++-- src/styles.css | 9 ++ 13 files changed, 394 insertions(+), 81 deletions(-) diff --git a/docs/src/content/docs/docs/AIAssistant.md b/docs/src/content/docs/docs/AIAssistant.md index a8b33f23..ef0d07ee 100644 --- a/docs/src/content/docs/docs/AIAssistant.md +++ b/docs/src/content/docs/docs/AIAssistant.md @@ -59,6 +59,15 @@ Individual AI Assistant Macro commands can override: - **System prompt**, which overrides the default system prompt for that command. - Advanced model parameters, described in [Advanced sampling settings](#advanced-sampling-settings). +### System prompts are sent as written {#system-prompt-is-literal} + +QuickAdd resolves [Format Syntax](/docs/FormatSyntax/) in the **prompt template** +only. The system prompt - both the default and a command's override - goes to the +model exactly as you typed it, so `{{DATE}}` in a system prompt arrives at the +model as the eight characters `{{DATE}}`. + +Put anything that needs a token in the prompt template instead. + ## Connect a provider {#providers-and-local-models} QuickAdd supports OpenAI-compatible providers, Google Gemini, and Anthropic. diff --git a/src/ai/AIAssistant.systemPromptLiteral.test.ts b/src/ai/AIAssistant.systemPromptLiteral.test.ts index e36b0309..db558c67 100644 --- a/src/ai/AIAssistant.systemPromptLiteral.test.ts +++ b/src/ai/AIAssistant.systemPromptLiteral.test.ts @@ -1,4 +1,5 @@ import { afterAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { TFile } from "obsidian"; import type { App } from "obsidian"; import type { AIProvider } from "./Provider"; import type { CommonResponse } from "./OpenAIRequest"; @@ -7,8 +8,8 @@ import type { CommonResponse } from "./OpenAIRequest"; * The invariant behind #1565 / #1568: the AI system prompt is sent to the model * VERBATIM. Only the prompt (or prompt template) goes through the formatter. * - * Three modals used to claim otherwise — a live preview resolving the tokens - * plus a `{{` autocomplete offering them — so `{{DATE}}` previewed as a date and + * Three modals used to claim otherwise - a live preview resolving the tokens + * plus a `{{` autocomplete offering them - so `{{DATE}}` previewed as a date and * then reached the model as eight literal characters. The affordance is gone; * this pins the behaviour it was lying about, in both directions: * @@ -30,9 +31,13 @@ const mocks = vi.hoisted(() => ({ openAIRequest: vi.fn(), isLikelyContextLimitError: vi.fn(() => false), getModelMaxTokens: vi.fn(() => 100000), - getMarkdownFilesInFolder: vi.fn(() => []), + getMarkdownFilesInFolder: vi.fn((): unknown[] => []), })); +// Reached transitively from Agent -> CompleteFormatter; its real entry point +// `require`s "obsidian", which does not exist outside the app. +vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); + vi.mock("src/settingsStore", () => ({ settingsStore: { getState: () => storeState }, })); @@ -53,7 +58,7 @@ vi.mock("src/utilityObsidian", () => ({ getMarkdownFilesInFolder: mocks.getMarkdownFilesInFolder, })); -const { Prompt, ChunkedPrompt } = await import("./AIAssistant"); +const { runAIAssistant, Prompt, ChunkedPrompt } = await import("./AIAssistant"); vi.stubGlobal("sleep", async () => {}); @@ -129,7 +134,7 @@ describe("the AI system prompt is sent verbatim", () => { ); expect(systemPromptSentToProvider()).toBe(SYSTEM_PROMPT); - // The prompt IS formatted — this is the contrast that makes the system + // The prompt IS formatted - this is the contrast that makes the system // prompt's exemption a deliberate behaviour rather than a dead code path. expect(mocks.makeRequest).toHaveBeenCalledWith("Summarise RESOLVED"); expect(resolvingFormatter).toHaveBeenCalledTimes(1); @@ -161,6 +166,63 @@ describe("the AI system prompt is sent verbatim", () => { } }); + it("runAIAssistant formats the prompt TEMPLATE and leaves the system prompt alone", async () => { + // The macro AI Assistant command's path, i.e. the modal that lost its + // preview. Covered explicitly because a partial #1572 implementation could + // format here and nowhere else, leaving a field that IS formatted with a + // note under it saying it is not. + const template = new TFile(); + template.path = "Prompts/Summarise.md"; + template.basename = "Summarise"; + mocks.getMarkdownFilesInFolder.mockReturnValue([template]); + + const app = { + vault: { + getAbstractFileByPath: () => template, + cachedRead: async () => "Summarise {{VALUE:body}}", + }, + } as unknown as App; + + await runAIAssistant( + app, + { + ...baseSettings(), + promptTemplate: { enable: true, name: "Summarise.md" }, + promptTemplateFolder: "Prompts", + }, + resolvingFormatter, + ); + + expect(systemPromptSentToProvider()).toBe(SYSTEM_PROMPT); + expect(mocks.makeRequest).toHaveBeenCalledWith("Summarise RESOLVED"); + expect(resolvingFormatter).toHaveBeenCalledTimes(1); + expect(resolvingFormatter).not.toHaveBeenCalledWith(SYSTEM_PROMPT); + }); + + it("Agent seeds the system message unformatted", async () => { + // `ai.agent()` falls back to the AI settings modal's default system prompt + // when the script passes none, so this path is fed by the same field. + const { Agent } = await import("./tools/Agent"); + const agent = new Agent( + makeApp(), + {} as never, + {} as never, + { model: "test-model", system: SYSTEM_PROMPT }, + ); + + // prompt omitted: buildSeedMessages short-circuits to "" and never builds a + // CompleteFormatter, so this stays a unit test of the system-message seam. + const messages = await ( + agent as unknown as { + buildSeedMessages: (o: object) => Promise< + Array<{ role: string; content: unknown }> + >; + } + ).buildSeedMessages({}); + + expect(messages[0]).toEqual({ role: "system", content: SYSTEM_PROMPT }); + }); + it("keeps the token characters intact rather than stripping them", async () => { await Prompt( makeApp(), diff --git a/src/gui/AIAssistantSettingsModal.ts b/src/gui/AIAssistantSettingsModal.ts index 0a8aa82b..f86cf0c6 100644 --- a/src/gui/AIAssistantSettingsModal.ts +++ b/src/gui/AIAssistantSettingsModal.ts @@ -148,6 +148,13 @@ export class AIAssistantSettingsModal extends Modal { const textAreaComponent = new TextAreaComponent(contentEl); textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); + // The textarea is appended to contentEl rather than to the Setting's + // controlEl (it needs the full modal width), so nothing associates it with + // the "Default system prompt" name above. + textAreaComponent.inputEl.setAttribute( + "aria-label", + "Default system prompt", + ); // No format preview and no `{{` token autocomplete here: the system prompt // is sent to the model verbatim (see mountSystemPromptLiteralNote). The @@ -156,6 +163,7 @@ export class AIAssistantSettingsModal extends Modal { // textarea above it (#1568). const updateLiteralNote = mountSystemPromptLiteralNote( contentEl, + textAreaComponent.inputEl, this.settings.defaultSystemPrompt ?? "", ); diff --git a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte index 95567213..92ecfd95 100644 --- a/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte +++ b/src/gui/ChoiceBuilder/components/FormatPreviewField.svelte @@ -34,11 +34,17 @@ let { plugin: QuickAdd; /** * The choice's configured target folder, so {{FOLDER}} / {{FOLDER|name}} - * preview meaningfully. When unknown we fall back to a "Folder/Name" + * preview meaningfully. When `undefined` we fall back to a "Folder/Name" * placeholder rather than an empty string (no caller wires the real path in * yet, but the placeholder keeps the FOLDER token from previewing blank). + * + * `null` means the host has no target folder CONCEPT at all — not "unknown" + * — and {{FOLDER}} previews as the empty string the runtime produces + * (`Formatter.replaceMacrosInString`: `this.targetFolderPath ?? ""`). The + * user-script format option is such a host: nothing there ever creates a note + * in a folder, so the placeholder would invent a path the script cannot get. */ - targetFolderPath?: string; + targetFolderPath?: string | null; } = $props(); /** How long the field must sit still before its problems are shown. */ @@ -86,9 +92,14 @@ $effect(() => { }); // 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). + // caller wires the real path in (issue: FOLDER preview always empty). An + // explicit null says this host has no folder at all, and previews "". formatter.setTargetFolderPath( - targetFolderPath?.trim() ? targetFolderPath : "Folder/Name", + targetFolderPath === null + ? null + : targetFolderPath?.trim() + ? targetFolderPath + : "Folder/Name", ); const token = ++previewToken; void (async () => { diff --git a/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts b/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts index e5ea676d..a8707753 100644 --- a/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts +++ b/src/gui/ChoiceBuilder/components/mountFormatPreview.svelte.ts @@ -24,8 +24,14 @@ export interface FormatPreviewHandle { /** Push the field's current value; call from the input's `onChange`. */ setValue(value: string): void; /** - * Unmount and remove the host. Idempotent, because a Modal's `onClose()` runs - * after a `display()`/`reload()` that already tore the component down. + * Unmount the component and remove the host. + * + * The `destroyed` guard is not only about a double `destroy()` (mountComponent + * already guards that): it also makes `setValue` a no-op afterwards. A + * destroyed preview's textarea can still be alive and holding an `onChange` + * closure over this handle - `display()` tears the previews down and only then + * empties `contentEl` - and pushing into an unmounted component's `$state` is + * a write nothing will ever read. */ destroy(): void; } @@ -44,12 +50,19 @@ export function mountFormatPreview( // these modals `createEl` straight onto `contentEl`, so mounting into a shared // container would interleave the anchors with later `new Setting(...)` rows. const host = container.ownerDocument.createElement("div"); + host.className = "qa-format-preview-host"; container.appendChild(host); const props = $state({ value: options.value, app: options.app, plugin: options.plugin, + // Explicitly "no folder", not "folder unknown". The builders' "Folder/Name" + // placeholder stands in for a choice's configured target folder; a modal + // field that never creates a note has none, and inventing one would preview + // `Folder/Name/2026-07-27` for a format the runtime resolves to + // `/2026-07-27`. + targetFolderPath: null, }); const mounted: MountHandle = mountComponent(host, FormatPreviewField, props); diff --git a/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts b/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts index 09269ebf..3343c36b 100644 --- a/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts +++ b/src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts @@ -239,6 +239,9 @@ export class AIAssistantCommandSettingsModal extends Modal { const textAreaComponent = new TextAreaComponent(contentEl); textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); + // Appended to contentEl rather than the Setting's controlEl (it needs the + // full modal width), so nothing associates it with the name above. + textAreaComponent.inputEl.setAttribute("aria-label", "System prompt"); // No format preview and no `{{` token autocomplete here: the system prompt // is sent to the model verbatim (see mountSystemPromptLiteralNote). The @@ -246,6 +249,7 @@ export class AIAssistantCommandSettingsModal extends Modal { // same admission. const updateLiteralNote = mountSystemPromptLiteralNote( contentEl, + textAreaComponent.inputEl, this.settings.systemPrompt ?? "", ); diff --git a/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts b/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts index 15358404..0ce31626 100644 --- a/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts +++ b/src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts @@ -151,6 +151,9 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal { const textAreaComponent = new TextAreaComponent(contentEl); textAreaComponent.inputEl.addClass("qa-ai-prompt-textarea"); + // Appended to contentEl rather than the Setting's controlEl (it needs the + // full modal width), so nothing associates it with the name above. + textAreaComponent.inputEl.setAttribute("aria-label", "System prompt"); // No format preview and no `{{` token autocomplete here: the system prompt // is sent to the model verbatim (see mountSystemPromptLiteralNote). This @@ -158,6 +161,7 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal { // is sized with estimateTokenCount on the raw string. const updateLiteralNote = mountSystemPromptLiteralNote( contentEl, + textAreaComponent.inputEl, this.settings.systemPrompt ?? "", ); diff --git a/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts b/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts index 6a0890a9..d2fa4f71 100644 --- a/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts +++ b/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts @@ -18,6 +18,8 @@ import { UserScriptSettingsModal } from "./UserScriptSettingsModal"; const mocks = vi.hoisted(() => ({ /** Every FormatDisplayFormatter built, in construction order. */ instances: [] as Array<{ formatted: string[] }>, + /** Every setTargetFolderPath argument, in call order. */ + targetFolderPaths: [] as Array, /** Resolves the next format() call manually, to test out-of-order passes. */ deferrals: [] as Array<() => void>, deferNext: false, @@ -45,7 +47,9 @@ vi.mock("../../formatters/formatDisplayFormatter", () => ({ mocks.instances.push(this.record); } - setTargetFolderPath(): void {} + setTargetFolderPath(path: string | null): void { + mocks.targetFolderPaths.push(path); + } format(value: string): Promise { this.record.formatted.push(value); @@ -115,6 +119,7 @@ function type(el: HTMLTextAreaElement, value: string) { beforeEach(() => { mocks.instances.length = 0; + mocks.targetFolderPaths.length = 0; mocks.deferrals.length = 0; mocks.diagnostics.length = 0; mocks.deferNext = false; @@ -205,6 +210,85 @@ describe("UserScriptSettingsModal format option preview", () => { modal.close(); }); + it("previews {{FOLDER}} as empty, not as an invented folder", async () => { + // The builders fall back to a "Folder/Name" placeholder because a choice HAS + // a configured target folder that no caller wires in yet. A user-script + // option has none at all, and the runtime resolves {{FOLDER}} to "" there + // (Formatter.replaceMacrosInString: `this.targetFolderPath ?? ""`), so the + // placeholder would preview a path the script can never produce. + const modal = openModal(formatOption("{{FOLDER}}/{{DATE}}")); + await flush(); + + expect(mocks.targetFolderPaths).not.toHaveLength(0); + for (const path of mocks.targetFolderPaths) expect(path).toBeNull(); + + modal.close(); + }); + + it("opens for a format option that declares no defaultValue", async () => { + // `initializeUserScriptSettings` deliberately skips options with no + // defaultValue, so `value` reaches addFormatInput as undefined despite its + // `string` type. FormatPreviewField calls `value.trim()` during mount, so an + // uncoerced undefined throws out of display() and out of the constructor - + // and CommandList's `new UserScriptSettingsModal(...).open()` never reaches + // `.open()`, so the gear button silently does nothing. + const modal = openModal({ + "Note format": { type: "format", placeholder: "Format" }, + }); + await flush(); + + expect(textarea(modal.contentEl).value).toBe(""); + // Empty field, so no dangling "Preview:" row. + expect(modal.contentEl.querySelector(".qa-preview-row")).toBeNull(); + + modal.close(); + }); + + it("opens when a stored value is not a string", async () => { + // A script that changed an option from `type: "toggle"` to `type: "format"` + // between versions leaves a boolean in the saved command settings. + const command = createCommand(); + command.settings["Note format"] = true; + const modal = new UserScriptSettingsModal( + new App(), + command, + scriptSettings(formatOption("ignored")) as ConstructorParameters< + typeof UserScriptSettingsModal + >[2], + ) as UserScriptSettingsModal & { contentEl: HTMLElement }; + await flush(); + + expect(textarea(modal.contentEl).value).toBe(""); + expect(modal.contentEl.querySelector(".qa-preview-row")).toBeNull(); + + modal.close(); + }); + + it("ignores setValue after destroy, and destroys idempotently", async () => { + const modal = openModal(formatOption("start")); + const internals = modal as unknown as { + previewHandles: Array<{ + setValue: (v: string) => void; + destroy: () => void; + }>; + }; + await flush(); + const handle = internals.previewHandles[0]; + const passesBefore = mocks.instances.length; + + handle.destroy(); + handle.destroy(); + // The textarea outlives the handle: display() tears the previews down and + // only THEN empties contentEl, so its onChange closure can still fire. + handle.setValue("after destroy"); + await flush(); + + expect(mocks.instances.length).toBe(passesBefore); + expect(document.querySelectorAll(".qa-preview-row")).toHaveLength(0); + + modal.close(); + }); + it("still writes the edited value into the command settings", async () => { const command = createCommand(); const modal = new UserScriptSettingsModal( diff --git a/src/gui/MacroGUIs/UserScriptSettingsModal.ts b/src/gui/MacroGUIs/UserScriptSettingsModal.ts index 388e56e9..908be2d9 100644 --- a/src/gui/MacroGUIs/UserScriptSettingsModal.ts +++ b/src/gui/MacroGUIs/UserScriptSettingsModal.ts @@ -95,8 +95,9 @@ export class UserScriptSettingsModal extends Modal { protected display() { this.containerEl.addClass("quickAddModal", "userScriptSettingsModal"); - // Before empty(), not after: emptying orphans the mounted components - // instead of tearing them down. + // Emptying contentEl does not stop a Svelte component, it only detaches it, + // so every re-render must tear the previews down explicitly or each orphan + // keeps a live $effect and the preview's 500ms diagnostics timer. this.destroyPreviews(); this.contentEl.empty(); @@ -322,9 +323,25 @@ export class UserScriptSettingsModal extends Modal { private addFormatInput(name: string, value: string, placeholder?: string) { const setting = new Setting(this.contentEl).setName(name); + // `value` comes from a third-party script's `settings.options`, so its + // declared `string` is a promise, not a guarantee: an option with no + // `defaultValue` arrives as undefined (initializeUserScriptSettings + // deliberately skips those), and an option whose `type` changed from + // `toggle` to `format` between script versions arrives as a boolean. + // Coerce here, at the boundary where untrusted data enters typed code - + // FormatPreviewField's `value.trim()` runs during mount, so a non-string + // would throw out of display() and out of the constructor, and the gear + // button in the command list would silently do nothing. Passing it to + // setValue is a pre-existing wart too: a real TextAreaComponent renders + // the literal text "undefined". + const text = typeof value === "string" ? value : ""; + const input = new TextAreaComponent(this.contentEl); new FormatSyntaxSuggester(this.app, input.inputEl, getQuickAddInstance()); input.inputEl.addClass("qa-user-script-format-textarea"); + // Appended to contentEl rather than the Setting's controlEl (it needs the + // full modal width), so nothing associates it with the option name above. + input.inputEl.setAttribute("aria-label", name); // Mounted AFTER the textarea, so the preview reads as a result of the field // rather than as its label - it used to be created first and rendered above @@ -332,12 +349,12 @@ export class UserScriptSettingsModal extends Modal { const preview = mountFormatPreview(this.contentEl, { app: this.app, plugin: getQuickAddInstance(), - value, + value: text, }); this.previewHandles.push(preview); input - .setValue(value) + .setValue(text) .onChange((value) => { this.command.settings[name] = value; preview.setValue(value); diff --git a/src/gui/ai/systemPromptFields.test.ts b/src/gui/ai/systemPromptFields.test.ts index a610a37a..7ae525b0 100644 --- a/src/gui/ai/systemPromptFields.test.ts +++ b/src/gui/ai/systemPromptFields.test.ts @@ -1,4 +1,4 @@ -import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; /** * The three system-prompt modals must not offer a format affordance: the system @@ -128,40 +128,58 @@ function infiniteCommand(systemPrompt: string): IInfiniteAIAssistantCommand { } as unknown as IInfiniteAIAssistantCommand; } -/** Each modal, paired with a factory that opens it and returns its contentEl. */ +interface OpenedModal { + contentEl: HTMLElement; + /** Every one of these modals re-renders in place; the AI settings modal does + * it on every "Edit providers", the command modals on every model change. */ + reload: () => void; + close: () => void; + label: string; +} + +function opened( + modal: { contentEl: HTMLElement; close: () => void }, + label: string, +): OpenedModal { + return { + contentEl: modal.contentEl, + reload: () => (modal as unknown as { reload: () => void }).reload(), + close: () => modal.close(), + label, + }; +} + +/** Each modal, paired with a factory that opens it. */ const MODALS: Array<{ name: string; - open: (systemPrompt: string) => { contentEl: HTMLElement; close: () => void }; + open: (systemPrompt: string) => OpenedModal; }> = [ { name: "AIAssistantSettingsModal (default system prompt)", - open: (systemPrompt) => { - const modal = new AIAssistantSettingsModal( - testApp(), - aiSettings(systemPrompt), - ); - return { contentEl: modal.contentEl, close: () => modal.close() }; - }, + open: (systemPrompt) => + opened( + new AIAssistantSettingsModal(testApp(), aiSettings(systemPrompt)), + "Default system prompt", + ), }, { name: "AIAssistantCommandSettingsModal (system prompt)", - open: (systemPrompt) => { - const modal = new AIAssistantCommandSettingsModal( - testApp(), - aiCommand(systemPrompt), - ); - return { contentEl: modal.contentEl, close: () => modal.close() }; - }, + open: (systemPrompt) => + opened( + new AIAssistantCommandSettingsModal(testApp(), aiCommand(systemPrompt)), + "System prompt", + ), }, { name: "InfiniteAIAssistantCommandSettingsModal (system prompt)", - open: (systemPrompt) => { - const modal = new InfiniteAIAssistantCommandSettingsModal( - testApp(), - infiniteCommand(systemPrompt), - ); - return { contentEl: modal.contentEl, close: () => modal.close() }; - }, + open: (systemPrompt) => + opened( + new InfiniteAIAssistantCommandSettingsModal( + testApp(), + infiniteCommand(systemPrompt), + ), + "System prompt", + ), }, ]; @@ -174,20 +192,6 @@ function promptTextarea(contentEl: HTMLElement): HTMLTextAreaElement { } describe("AI system-prompt fields offer no format affordance", () => { - beforeAll(() => { - // The obsidian stub's Modal has no onClose; the modals call super.onClose(). - for (const Ctor of [ - AIAssistantSettingsModal, - AIAssistantCommandSettingsModal, - InfiniteAIAssistantCommandSettingsModal, - ]) { - const proto = Object.getPrototypeOf(Ctor.prototype) as { - onClose?: () => void; - }; - proto.onClose ??= function onClose() {}; - } - }); - beforeEach(() => { vi.clearAllMocks(); document.body.innerHTML = ""; @@ -237,6 +241,37 @@ describe("AI system-prompt fields offer no format affordance", () => { close(); }); + + it("names the textarea, which sits outside its Setting row", () => { + const { contentEl, close, label } = open(PROSE_PROMPT); + + expect(promptTextarea(contentEl).getAttribute("aria-label")).toBe( + label, + ); + + close(); + }); + + it("keeps exactly one note, still correct, across a reload", () => { + // reload() empties contentEl and rebuilds. A note hoisted out of the + // field's own builder would survive the first render and vanish here, + // silently taking the field's last remaining signal with it. + const { contentEl, close, reload } = open(TOKENED_PROMPT); + reload(); + + const notes = contentEl.querySelectorAll(".qa-literal-format-note"); + expect(notes).toHaveLength(1); + expect( + notes[0].classList.contains("qa-literal-format-note--shown"), + ).toBe(true); + expect(promptTextarea(contentEl).getAttribute("aria-describedby")).toBe( + notes[0].id, + ); + expect(mocks.formatDisplayFormatter).not.toHaveBeenCalled(); + expect(mocks.formatSyntaxSuggester).not.toHaveBeenCalled(); + + close(); + }); }); } }); diff --git a/src/gui/ai/systemPromptLiteralNote.test.ts b/src/gui/ai/systemPromptLiteralNote.test.ts index 7bf00c2e..8c68dc02 100644 --- a/src/gui/ai/systemPromptLiteralNote.test.ts +++ b/src/gui/ai/systemPromptLiteralNote.test.ts @@ -1,10 +1,12 @@ import { describe, expect, it } from "vitest"; import { mountSystemPromptLiteralNote } from "./systemPromptLiteralNote"; -function host(): HTMLElement { - const el = document.createElement("div"); - document.body.appendChild(el); - return el; +function host(): { container: HTMLElement; field: HTMLTextAreaElement } { + const container = document.createElement("div"); + const field = document.createElement("textarea"); + container.appendChild(field); + document.body.appendChild(container); + return { container, field }; } function noteOf(container: HTMLElement): HTMLElement { @@ -20,9 +22,10 @@ function isShown(container: HTMLElement): boolean { describe("mountSystemPromptLiteralNote", () => { it("stays hidden for a prose prompt", () => { - const container = host(); + const { container, field } = host(); mountSystemPromptLiteralNote( container, + field, "As an AI assistant within Obsidian, your primary goal is to help users.", ); @@ -30,15 +33,15 @@ describe("mountSystemPromptLiteralNote", () => { }); it("shows for a prompt that already contains a token", () => { - const container = host(); - mountSystemPromptLiteralNote(container, "Today is {{DATE}}."); + const { container, field } = host(); + mountSystemPromptLiteralNote(container, field, "Today is {{DATE}}."); expect(isShown(container)).toBe(true); }); it("follows the value as the field is edited, in both directions", () => { - const container = host(); - const update = mountSystemPromptLiteralNote(container, ""); + const { container, field } = host(); + const update = mountSystemPromptLiteralNote(container, field, ""); expect(isShown(container)).toBe(false); update("You are helpful."); @@ -49,9 +52,42 @@ describe("mountSystemPromptLiteralNote", () => { expect(isShown(container)).toBe(false); }); + it("describes the field only while the note is visible", () => { + // An accessible description may include a directly-referenced HIDDEN node, + // so a permanent reference would describe the field with a line the sighted + // user cannot see. + const { container, field } = host(); + const update = mountSystemPromptLiteralNote(container, field, ""); + + expect(field.hasAttribute("aria-describedby")).toBe(false); + + update("Today is {{DATE}}."); + const id = field.getAttribute("aria-describedby"); + expect(id).toBeTruthy(); + expect(noteOf(container).id).toBe(id); + + update("Today is Monday."); + expect(field.hasAttribute("aria-describedby")).toBe(false); + }); + + it("gives each mount its own id, so two notes never collide", () => { + const a = host(); + const b = host(); + mountSystemPromptLiteralNote(a.container, a.field, "{{DATE}}"); + mountSystemPromptLiteralNote(b.container, b.field, "{{DATE}}"); + + expect(noteOf(a.container).id).not.toBe(noteOf(b.container).id); + expect(a.field.getAttribute("aria-describedby")).toBe( + noteOf(a.container).id, + ); + expect(b.field.getAttribute("aria-describedby")).toBe( + noteOf(b.container).id, + ); + }); + it("names the token syntax it is about and points at the prompt template", () => { - const container = host(); - mountSystemPromptLiteralNote(container, "{{DATE}}"); + const { container, field } = host(); + mountSystemPromptLiteralNote(container, field, "{{DATE}}"); const text = noteOf(container).textContent ?? ""; expect(text).toContain("{{DATE}}"); @@ -61,19 +97,16 @@ describe("mountSystemPromptLiteralNote", () => { it("exists in the DOM while hidden, so it renders under its own field", () => { // Created up front rather than on demand: these modals append as they build, // so a lazily created note would land at the bottom of the modal. - const container = host(); - const marker = container.createEl("div"); - mountSystemPromptLiteralNote(container, "no tokens here"); + const { container, field } = host(); + mountSystemPromptLiteralNote(container, field, "no tokens here"); const trailing = container.createEl("div"); const note = noteOf(container); expect( - marker.compareDocumentPosition(note) & - Node.DOCUMENT_POSITION_FOLLOWING, + field.compareDocumentPosition(note) & Node.DOCUMENT_POSITION_FOLLOWING, ).toBeTruthy(); expect( - trailing.compareDocumentPosition(note) & - Node.DOCUMENT_POSITION_PRECEDING, + trailing.compareDocumentPosition(note) & Node.DOCUMENT_POSITION_PRECEDING, ).toBeTruthy(); }); }); diff --git a/src/gui/ai/systemPromptLiteralNote.ts b/src/gui/ai/systemPromptLiteralNote.ts index 12b69c64..35921c71 100644 --- a/src/gui/ai/systemPromptLiteralNote.ts +++ b/src/gui/ai/systemPromptLiteralNote.ts @@ -2,7 +2,7 @@ * The AI system prompt is sent to the model VERBATIM. QuickAdd's format syntax * is applied to the prompt (or prompt template) only: * - * - `runAIAssistant` formats `targetPrompt`, passes `systemPrompt` raw + * - `runAIAssistant` formats `targetPrompt`, passes `systemPrompt` raw * (src/ai/AIAssistant.ts). * - `Prompt` / `ChunkedPrompt` do the same, and the chunked path even sizes * its budget with `estimateTokenCount(systemPrompt)` on the raw string. @@ -10,19 +10,34 @@ * straight through while its formatter callback wraps only the prompt. * - `Agent.buildSeedMessages` pushes `system` raw and formats `prompt`. * + * All four are pinned by AIAssistant.systemPromptLiteral.test.ts. + * * Until #1572 decides otherwise, `{{DATE}}` in a system prompt reaches the model * as the eight characters `{{DATE}}`. The three system-prompt modals used to * claim the opposite - a live "preview" resolving the tokens, plus a `{{` * autocomplete offering them (issues #1565, #1568). Both are gone; this note is - * what replaces them, and only for the authors who already have a token in - * there and would otherwise get no signal at all. + * what replaces them. + * + * Deliberately conditional on the value containing `{{`, rather than a sentence + * in `setDesc()` that is always visible. The token-free prose prompt is the + * overwhelming majority - it is the shipped default and the seed for every new + * AI command - and a permanent line about a syntax that does not apply is the + * kind of standing chrome the builders' own hints were designed to avoid + * (#1570). The audience here is narrow and specific: an author who already + * typed a token, most likely on the deleted autocomplete's advice, and who would + * otherwise get no signal at all. * - * Shown only when the value contains `{{`, so the shipped default prompt (and - * every prose prompt) never sees it. + * The `{{` trigger over-fires on prose or LaTeX that happens to contain two + * braces (`\frac{{a}}{b}`). That is deliberate: the note is muted and states a + * fact about the field, so a false positive costs one quiet line, while a false + * negative costs a silently broken prompt. */ const LITERAL_NOTE_TEXT = - "QuickAdd sends this prompt to the model as written - format syntax like {{DATE}} is not resolved here. Use the prompt template for text that needs tokens."; + "QuickAdd sends the system prompt to the model as written - format syntax like {{DATE}} is not resolved here. Only the prompt template is formatted."; + +/** Distinguishes the notes of two modals open at once, and across `reload()`. */ +let noteSequence = 0; /** * Renders a muted note under a system-prompt field, and returns the updater to @@ -34,17 +49,26 @@ const LITERAL_NOTE_TEXT = */ export function mountSystemPromptLiteralNote( container: HTMLElement, + field: HTMLTextAreaElement, initialValue: string, ): (value: string) => void { const note = container.createEl("div", { text: LITERAL_NOTE_TEXT, cls: "qa-literal-format-note", }); + const noteId = `qa-literal-format-note-${++noteSequence}`; + note.id = noteId; - // classList, not Obsidian's toggleClass: this runs under the vitest DOM stub - // too, which patches createEl/addClass but not toggleClass. + // classList/setAttribute, not Obsidian's toggleClass: this runs under the + // vitest DOM stub too, which patches createEl/addClass but not toggleClass. const update = (value: string): void => { - note.classList.toggle("qa-literal-format-note--shown", value.includes("{{")); + const shown = value.includes("{{"); + note.classList.toggle("qa-literal-format-note--shown", shown); + // Referenced only while shown. An accessible description may include a + // directly-referenced hidden node, so leaving the reference in place would + // describe the field with a note the sighted user cannot see. + if (shown) field.setAttribute("aria-describedby", noteId); + else field.removeAttribute("aria-describedby"); }; update(initialValue); diff --git a/src/styles.css b/src/styles.css index 9078b929..16f60860 100644 --- a/src/styles.css +++ b/src/styles.css @@ -651,6 +651,15 @@ box-sizing: border-box; } +/* A format preview mounted under one of the textareas above (#1565). Those carry + `margin-bottom: 1em` for the gap to the NEXT setting, which would leave the + preview row floating midway between the field it describes and the one below. + Pull it back up so it reads as bound to its own field, and let the host carry + the separation instead. */ +.quickAddModal .qa-format-preview-host { + margin-top: -0.75em; +} + .qa-ai-token-count, .qa-ai-token-note { color: var(--text-muted); From cfad90e1f3c80324eb86efe2f464ee1d4d2e9002 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 08:46:11 +0200 Subject: [PATCH 4/4] fix(user-script): show a non-string format value instead of blanking it Caught by Codex on #1581. The previous commit coerced a non-string stored value to "" so `FormatPreviewField`'s `value.trim()` could not throw out of the constructor. But `resolveUserScriptSettings` still forwards the original to the script, so a `toggle`-to-`format` type change across script versions left the field claiming the setting was empty while execution received `true`. Absent stays absent - `initializeUserScriptSettings` deliberately leaves an option with no `defaultValue` unset so the script can apply its own, and printing "undefined" would be its own lie. Anything present is stringified, which also makes this method consistent with its four siblings: they all render `value as string` unchanged. Persisting the normalization instead was rejected: for the absent case it would overwrite the deliberate undefined, and for the present case it means a disk write from a render pass. Refs #1565 --- ...rScriptSettingsModal.formatPreview.test.ts | 14 +++++-- src/gui/MacroGUIs/UserScriptSettingsModal.ts | 40 ++++++++++++++----- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts b/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts index d2fa4f71..a86f22fb 100644 --- a/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts +++ b/src/gui/MacroGUIs/UserScriptSettingsModal.formatPreview.test.ts @@ -244,9 +244,11 @@ describe("UserScriptSettingsModal format option preview", () => { modal.close(); }); - it("opens when a stored value is not a string", async () => { + it("opens when a stored value is not a string, and shows what the script will get", async () => { // A script that changed an option from `type: "toggle"` to `type: "format"` - // between versions leaves a boolean in the saved command settings. + // between versions leaves a boolean in the saved command settings. Blanking + // the field would claim the setting is empty while + // `resolveUserScriptSettings` still forwards `true` to the script. const command = createCommand(); command.settings["Note format"] = true; const modal = new UserScriptSettingsModal( @@ -258,8 +260,12 @@ describe("UserScriptSettingsModal format option preview", () => { ) as UserScriptSettingsModal & { contentEl: HTMLElement }; await flush(); - expect(textarea(modal.contentEl).value).toBe(""); - expect(modal.contentEl.querySelector(".qa-preview-row")).toBeNull(); + expect(textarea(modal.contentEl).value).toBe("true"); + expect( + modal.contentEl.querySelector(".qa-preview-value")?.textContent, + ).toBe("resolved(true)"); + // Untouched until the user actually edits the field. + expect(command.settings["Note format"]).toBe(true); modal.close(); }); diff --git a/src/gui/MacroGUIs/UserScriptSettingsModal.ts b/src/gui/MacroGUIs/UserScriptSettingsModal.ts index 908be2d9..e47a7d31 100644 --- a/src/gui/MacroGUIs/UserScriptSettingsModal.ts +++ b/src/gui/MacroGUIs/UserScriptSettingsModal.ts @@ -57,6 +57,22 @@ type Option = { description?: string; id?: string } & ( } ); +/** + * The text a `type: "format"` option should render for a stored value that a + * third-party script may have left in any shape. + * + * Absent means absent: `initializeUserScriptSettings` deliberately leaves an + * option with no `defaultValue` unset so the script can apply its own, and + * printing "undefined" into the field would be a lie about what it will receive. + * Anything else is stringified rather than blanked, so the field agrees with the + * value `resolveUserScriptSettings` forwards. + */ +function formatOptionText(value: unknown): string { + if (typeof value === "string") return value; + if (value === undefined || value === null) return ""; + return String(value); +} + function formatTitlePart(value: unknown): string { if (typeof value === "string") return value; if (value === null || value === undefined) return ""; @@ -326,15 +342,21 @@ export class UserScriptSettingsModal extends Modal { // `value` comes from a third-party script's `settings.options`, so its // declared `string` is a promise, not a guarantee: an option with no // `defaultValue` arrives as undefined (initializeUserScriptSettings - // deliberately skips those), and an option whose `type` changed from - // `toggle` to `format` between script versions arrives as a boolean. - // Coerce here, at the boundary where untrusted data enters typed code - - // FormatPreviewField's `value.trim()` runs during mount, so a non-string - // would throw out of display() and out of the constructor, and the gear - // button in the command list would silently do nothing. Passing it to - // setValue is a pre-existing wart too: a real TextAreaComponent renders - // the literal text "undefined". - const text = typeof value === "string" ? value : ""; + // deliberately skips those, so the script can apply its own default), and + // an option whose `type` changed from `toggle` to `format` between script + // versions arrives as a boolean. Coerce here, at the boundary where + // untrusted data enters typed code - FormatPreviewField's `value.trim()` + // runs during mount, so a non-string would throw out of display() and out + // of the constructor, and the gear button in the command list would + // silently do nothing. + // + // A present non-string is STRINGIFIED rather than blanked, so what the + // field shows still corresponds to what `resolveUserScriptSettings` will + // forward to the script. That also keeps this method consistent with its + // four siblings, which all render `value as string` unchanged. Absent + // (undefined/null) renders empty instead of the literal text "undefined", + // which is what a real TextAreaComponent would print. + const text = formatOptionText(value); const input = new TextAreaComponent(this.contentEl); new FormatSyntaxSuggester(this.app, input.inputEl, getQuickAddInstance());