fix(ai): stop previewing a substitution the system prompt never gets#1581
Merged
Conversation
#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
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
… 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSystem prompts are now transmitted verbatim while prompt templates remain formatted, with literal guidance added to three AI settings modals. User-script format previews now use mounted Svelte components with null-folder handling and explicit lifecycle cleanup. ChangesSystem Prompt Literal Treatment
Format Preview Rework for UserScriptSettingsModal
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying quickadd with
|
| Latest commit: |
cfad90e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ce24d7cf.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1565-modal-inert-p.quickadd.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6364c0fb54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
Owner
Author
That is the deliberate divergence, and it is the first section of the PR description rather than an oversight — flagging it here so a human reader does not have to reconcile the bot's verdict with the diff. Both issues describe the symptom ("the preview is noisy / is an unlabelled duplicate") and propose the fix that follows from assuming those fields are format fields. They are not:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1565. Closes #1568.
The change is not what #1565 asked for, and here is why
#1565 asked for four imperative 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:
runAIAssistanttargetPrompt(AIAssistant.ts:295)systemPrompt(:308)Promptprompt(:408)systemPrompt(:418)ChunkedPromptsystemPrompt(:936) - andestimateTokenCount(systemPrompt)at:897sizes the chunk budget on the raw string, which is the same admissionMacroChoiceEngine.executeAIAssistantformatter.formatFileContenton the prompt (:675)command.systemPrompt(:668)Agent.buildSeedMessagespromptsystem(Agent.ts:271-277)OpenAIRequestthen puts it on the wire unchanged (:198,:246,:312). Thedocs agree -
AIAssistant.md:56says the prompt template "is a Markdown note inthe 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
FormatDisplayFormatterand attached the{{token autocomplete, so{{DATE}}previewed as
2026-07-27and then reached the model as eight literal characters.That is worse than the defect #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 is deleted from those three, and the one field of the four that
really is a format field - the user script's
type: "format"option, which thescript itself resolves with
quickAddApi.format()(EzImport.js:304)
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 beforethe substitution. Filed as #1572. If it lands, the preview and the token
autocomplete come back with it. If you would rather ship that instead, say so
and I will revise - the removal is one commit and reverts cleanly.
Before / after
#1568 - AI Assistant settings, "Default system prompt"
The shipped default prompt contains no tokens, so the preview was a
character-for-character duplicate of the textarea, at full body size - larger and
more prominent than the field it was previewing.
#1565 - AI Assistant macro command, "System prompt"
Before:
{{DATE}}resolved on screen. It does not resolve at run time.What replaces it, for authors who already have a token
One muted line, shown only when the value contains
{{, so the token-free proseprompt (the shipped default, and the seed for every new AI command) never sees
it. Muted rather than warning-coloured, matching
.qa-ai-token-notedirectlyabove it in the same modals: like that line it states how the field behaves, and
a
{{you want the model to read literally is a legitimate thing to type here.#1565 - user script
type: "format"optionThe one field that keeps a preview. Before, the span was created before the
textarea, so it rendered above the field and read as its label - the exact
placement #1543 fixed in the builders. And since #1560 silenced the Notices, the
|case:pasctypo reported nothing at all: 0 Notices, 0 inline complaints,preview rendering "Example Title" as if all were well.
What the user-script field gained
The Svelte
FormatPreviewFieldis mounted rather than reimplemented: labelling,placement, per-pass construction, the staleness token and the inline diagnostics
are ~80 lines of exactly the drift that produced this issue.
mountFormatPreviewis a thin seam over the existingmountComponent, with a$state-backed props object (mirroringcreateCommandListProps). It creates andowns its host element, because
mount()writes anchor comment nodes intowhatever it is given and this modal
createEls straight ontocontentEl.Lifecycle is the trap.
display()is re-entrant - the constructor calls it, andmigrateSecretSettings()calls it again from avoided promise that can land atany time - and it empties
contentEl. Emptying does not stop a Svelte component;it only detaches it.
destroyPreviews()therefore runs as the first statement ofdisplay()and from a newonClose()(the class had none). The regression testasserts 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 that test passed against a build with the teardown removed.
Validation
Every claim below was reproduced and re-verified in this worktree's own isolated
Obsidian 1.13.0 vault, driving the real UI.
bf39e2ba: typing{{VALUE:title|case:pasc}}into the AIcommand's System prompt and into the user-script format option produced
{"notices":0,"inlineIssues":0}in both. Total silence.{"notices":0,"label":"Preview: ","value":"Logged on 2026-07-27 for Example Title","issues":["Warning: Unsupported |case style \"pasc\" ... Supported styles: kebab, snake, camel, pascal, title, lower, upper, slug."]},with the preview positioned after the input.
{"bareSpans":0}; typing{{opens nosuggestion container.
{{FOLDER}}/{{DATE}}in a user-script format option previewedFolder/Name/2026-07-27mid-review and now previews/2026-07-27, matchingquickAddApi.format().type: "format"option with nodefaultValueopened the modal with an emptyfield (it crashed the constructor mid-review; see below).
Suite: 4106 passing.
tsc,eslint,svelte-checkclean.Every new test was mutation-verified - reverted the fix, watched the test
fail, restored. That caught two tests that were passing for the wrong reason: the
teardown test above, and the four-path contract test, whose
runAIAssistantcasedid not exist until a reviewer pointed out that every existing test that names
runAIAssistantmocks it away.Found by the review, fixed here
An adversarial review pass caught one regression this branch introduced:
initializeUserScriptSettingsdeliberately skips options with nodefaultValue,so
addFormatInput'svalue: stringarrives asundefinedfrom a third-partyscript - and
FormatPreviewFieldcallsvalue.trim()during mount. The throwpropagated out of the constructor, so
new UserScriptSettingsModal(...).open()never reached
.open()and the gear button silently did nothing. Coerced at theboundary, with both that case and a non-string stored value pinned.
Also in the diff
aria-describedbyon the note while it is shown (and only while: an accessibledescription may include a directly-referenced hidden node).
aria-labelon the four textareas that are appended tocontentElrather thantheir Setting's
controlEl, so nothing associated them with the name above.docs/.../AIAssistant.md, so the answer is findable beforeyou type the token rather than only after.
addSliderinto the shared obsidian test stub, replacing a local shim.Considered and declined
FormatTokenHinton these fields. feat(builder): advertise the format-token autocomplete #1570 shipped an argued criterion andexcluded insert-after/before because "a token-less value is the documented
normal state there and the hint would nag forever". A system prompt fails that
harder - token-less is the shipped default - and per the above it would
advertise a feature that does not exist. On the user-script format option it is
defensible but it is a call about third-party scripts' fields that this PR
should not make.
hiding it when it adds nothing"). Two independent reviewers refuted it: it
breaks the [FEATURE REQUEST] "Preview:" renders above its field and shows an empty label when there is nothing to preview #1543 regression test on
Inbox.md(a literal file name is the mostcommon configuration QuickAdd has), and worse, both display formatters return an
unrecognised token verbatim with no diagnostic - so
{{VDATE:due}},{{TITLE}},{DATE}would all lose the only signal that they did not resolve. Deleting thepreview from the three AI fields answers [BUG] The AI Assistant's default system prompt shows its preview as an unlabelled duplicate #1568 at the root instead.
{{. Reaches the userswho never reopen the modal, but a per-request string scan for a line almost
nobody reads is the wrong trade. Noted on [FEATURE] Decide whether AI system prompts should support QuickAdd format syntax #1572.
Filed, not folded in
CommandType.InfiniteAIAssistantis unreachable, unconfigurable, andsilently skipped by the macro engine. Its settings modal (one of [BUG] The AI and user-script settings modals still stack a Notice per keystroke in their format previews #1565's four)
is only constructible from the devMode
testQuickAddcommand. Its preview isfixed here anyway so the two AI prompt modals do not diverge, whichever way
that issue is resolved.
Summary by CodeRabbit
New Features
{{DATE}}stay literal—use the prompt template for dynamic values.Bug Fixes