fix: the file-name preview tells the truth on every surface, and the run stops before the damage - #1618
Conversation
Deploying quickadd with
|
| Latest commit: |
da6e01c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fabf3c2e.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1587-preview-name.quickadd.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (31)
🚧 Files skipped from review as they are similar to previous changes (28)
📝 WalkthroughWalkthroughThe PR adds early illegal-path validation, aligns filename and body previews with runtime token behavior, and changes one-page previews to structured diagnostic rows with race-safe rendering and distinct unresolved versus non-creatable labels. ChangesPreview and path validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5db6d5dda8
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/formatters/formatter.ts (1)
1465-1492: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNamespaced storage for
{{MVALUE}}avoids clashing with{{VALUE:mvalue}}.
replaceMathValueInStringreadsthis.variables.get("mvalue"), exactly the same bare key that{{VALUE:mvalue}}uses.FIELDvariables avoid this withFIELD_VARIABLE_PREFIX, so a template containing both{{MVALUE}}and{{VALUE:mvalue}}shares one internal value. Use a dedicated internal key such asMVALUE_VARIABLE_KEYfor{{MVALUE}}while keeping the user-facing requirement/CLI flag as"mvalue".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/formatters/formatter.ts` around lines 1465 - 1492, Use a dedicated internal storage key such as MVALUE_VARIABLE_KEY in replaceMathValueInString when reading the collected math answer, rather than the bare "mvalue" key used by {{VALUE:mvalue}}. Keep the user-facing requirement name and CLI flag as "mvalue", and update the corresponding MVALUE collection/storage path to use the same namespaced key.
🧹 Nitpick comments (1)
src/preflight/OnePageInputModal.ts (1)
792-808: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAppend diagnostics to
rowEl, not the container.Each row's issues are created on
this.previewContainerElwhile the value lives inrowEl, so a row and its problems are siblings rather than one grouped unit. It renders correctly today only because there is exactly one row and rows are appended in order; any second row (or row-level flex/grid styling) breaks the association, and assistive tech gets no structural link between a name and the reason it will be refused. The CSS selector.qa-onepage-preview .qa-preview-issueis a descendant match, so it keeps working.♻️ Proposed change
for (const diagnostic of row.diagnostics) { - const issueEl = this.previewContainerEl.createDiv({ + const issueEl = rowEl.createDiv({ cls: "qa-preview-issue", });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preflight/OnePageInputModal.ts` around lines 792 - 808, Change the diagnostic rendering loop to append each `qa-preview-issue` element to the current `rowEl` rather than `this.previewContainerEl`, keeping the existing severity text, message, and title behavior unchanged so each row’s value and issues remain grouped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/applyTemplateToActiveNote.ts`:
- Around line 330-335: Add regression coverage for the move flow around
isCreatableFilePath and the targetPath guard: use an illegal computed target,
then verify no prompt is shown and neither createFolder nor renameFile is
called. Keep the test focused on the declined move path and its existing
quiet-return behavior.
In `@src/engine/CaptureChoiceEngine.selection.test.ts`:
- Around line 1256-1285: Update the test around CaptureChoiceEngine.run to spy
on the executor or prompt path responsible for resolving the format’s {{VALUE}}
placeholder, then assert it is never called for the impossible “Bad: Title.md”
target. Keep the existing createFileWithInput assertion and verify both
prompting and file creation are skipped.
---
Outside diff comments:
In `@src/formatters/formatter.ts`:
- Around line 1465-1492: Use a dedicated internal storage key such as
MVALUE_VARIABLE_KEY in replaceMathValueInString when reading the collected math
answer, rather than the bare "mvalue" key used by {{VALUE:mvalue}}. Keep the
user-facing requirement name and CLI flag as "mvalue", and update the
corresponding MVALUE collection/storage path to use the same namespaced key.
---
Nitpick comments:
In `@src/preflight/OnePageInputModal.ts`:
- Around line 792-808: Change the diagnostic rendering loop to append each
`qa-preview-issue` element to the current `rowEl` rather than
`this.previewContainerEl`, keeping the existing severity text, message, and
title behavior unchanged so each row’s value and issues remain grouped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fb8dee9-2687-482a-af80-21d4bbe0dae8
📒 Files selected for processing (29)
src/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateEngine-1591-refuse-name.test.tssrc/engine/TemplateEngine.tssrc/engine/applyTemplateToActiveNote.tssrc/engine/assertCreatableFilePath.test.tssrc/engine/assertCreatableFilePath.tssrc/formatters/completeFormatter.test.tssrc/formatters/completeFormatter.tssrc/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.tssrc/formatters/fileNameDisplayFormatter.audit-cleanup.test.tssrc/formatters/fileNameDisplayFormatter.test.tssrc/formatters/fileNameDisplayFormatter.tssrc/formatters/formatDisplayFormatter-1587-mvalue.test.tssrc/formatters/formatDisplayFormatter-1589-vdate.test.tssrc/formatters/formatDisplayFormatter.tssrc/formatters/formatter.tssrc/formatters/helpers/snappedExampleDate.tssrc/gui/ChoiceBuilder/TemplateChoiceForm.sveltesrc/gui/ChoiceBuilder/components/FormatPreviewField.sveltesrc/gui/ChoiceBuilder/components/FormatPreviewField.test.tssrc/preflight/OnePageInputModal.test.tssrc/preflight/OnePageInputModal.tssrc/preflight/runOnePagePreflight.filenamePreview.test.tssrc/preflight/runOnePagePreflight.tssrc/styles.csssrc/utils/generatedFilePath.tssrc/utils/previewTargetFolder.tssrc/utils/vdateSyntax.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/engine/applyTemplateToActiveNote.test.ts (1)
537-541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead ternary branch and stale comment in
suggestMockmock.
typeof items[0] === "string" ? items[0] : items[0]returnsitems[0]regardless of the condition — the branch is dead code. The comment "First call picks the template, second picks the insert mode" also contradicts the earlier comment (Lines 506-508) stating the mode picker is skipped for this empty-note test, since only the template picker actually runs here.🧹 Proposed cleanup
suggestMock.mockImplementation(async (_app, _labels, values) => { - const items = values as Array<unknown>; - // First call picks the template, second picks the insert mode. - return typeof items[0] === "string" ? items[0] : items[0]; + const items = values as Array<unknown>; + // Only the template picker runs here; the empty-note fast path + // skips the insert-mode picker (see makeApp above). + return items[0]; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/applyTemplateToActiveNote.test.ts` around lines 537 - 541, Clean up the suggestMock implementation by returning items[0] directly without the redundant ternary. Remove or update the comment above it so it accurately reflects that this empty-note test invokes only the template picker, not an insert-mode picker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/engine/applyTemplateToActiveNote.test.ts`:
- Around line 537-541: Clean up the suggestMock implementation by returning
items[0] directly without the redundant ternary. Remove or update the comment
above it so it accurately reflects that this empty-note test invokes only the
template picker, not an insert-mode picker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad421fdf-bbc7-46a1-aa85-6889eeb343e5
📒 Files selected for processing (4)
src/engine/CaptureChoiceEngine.selection.test.tssrc/engine/applyTemplateToActiveNote.test.tssrc/utils/previewTargetFolder.test.tssrc/utils/previewTargetFolder.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/previewTargetFolder.ts
- src/engine/CaptureChoiceEngine.selection.test.ts
|
Re-verified end to end against Also still passing: a Capture that creates then appends, a plain Template, a Full suite green under |
970cbb5 to
f6f0140
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/formatters/fileNameDisplayFormatter.ts (1)
675-688: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment above now contradicts the code. The block comment at Lines 638-641 still states this preview renders the date without applying
|startof:/|endof:snap, while the new example branch snaps viasnappedExampleDate(snap)and the inline comment at Lines 675-683 explains why. Worth dropping the outdated sentences so the two comments don't disagree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/formatters/fileNameDisplayFormatter.ts` around lines 675 - 688, Update the stale documentation comment above the preview example branch to remove statements claiming the date is rendered without applying startof/endof snapping. Keep the current behavior in DateFormatPreviewGenerator.generate and snappedExampleDate(snap) unchanged, and retain only wording consistent with the snapping implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gui/ChoiceBuilder/components/FormatPreviewField.svelte`:
- Around line 113-115: Update the diagnostics flow surrounding the derived
values errors, isUnresolved, and isInvalid so changing value or targetFolderPath
cannot reuse errors from the prior value while the delayed async formatting pass
is pending. Clear diagnostics or gate reads using the latest completed pass,
while preserving previewToken commit protection, and add a regression test
covering a delayed format beyond the 500 ms idle delay.
---
Nitpick comments:
In `@src/formatters/fileNameDisplayFormatter.ts`:
- Around line 675-688: Update the stale documentation comment above the preview
example branch to remove statements claiming the date is rendered without
applying startof/endof snapping. Keep the current behavior in
DateFormatPreviewGenerator.generate and snappedExampleDate(snap) unchanged, and
retain only wording consistent with the snapping implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6309a3b0-3757-46f2-8af4-1b49650464c6
📒 Files selected for processing (31)
src/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateEngine-1591-refuse-name.test.tssrc/engine/TemplateEngine.tssrc/engine/applyTemplateToActiveNote.test.tssrc/engine/applyTemplateToActiveNote.tssrc/engine/assertCreatableFilePath.test.tssrc/engine/assertCreatableFilePath.tssrc/formatters/completeFormatter.test.tssrc/formatters/completeFormatter.tssrc/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.tssrc/formatters/fileNameDisplayFormatter.audit-cleanup.test.tssrc/formatters/fileNameDisplayFormatter.test.tssrc/formatters/fileNameDisplayFormatter.tssrc/formatters/formatDisplayFormatter-1587-mvalue.test.tssrc/formatters/formatDisplayFormatter-1589-vdate.test.tssrc/formatters/formatDisplayFormatter.tssrc/formatters/formatter.tssrc/formatters/helpers/snappedExampleDate.tssrc/gui/ChoiceBuilder/TemplateChoiceForm.sveltesrc/gui/ChoiceBuilder/components/FormatPreviewField.sveltesrc/gui/ChoiceBuilder/components/FormatPreviewField.test.tssrc/preflight/OnePageInputModal.test.tssrc/preflight/OnePageInputModal.tssrc/preflight/runOnePagePreflight.filenamePreview.test.tssrc/preflight/runOnePagePreflight.tssrc/styles.csssrc/utils/generatedFilePath.tssrc/utils/previewTargetFolder.test.tssrc/utils/previewTargetFolder.tssrc/utils/vdateSyntax.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- src/formatters/helpers/snappedExampleDate.ts
- src/gui/ChoiceBuilder/TemplateChoiceForm.svelte
- src/engine/assertCreatableFilePath.test.ts
- src/utils/previewTargetFolder.ts
- src/formatters/formatDisplayFormatter-1587-mvalue.test.ts
- src/utils/vdateSyntax.ts
- src/utils/previewTargetFolder.test.ts
- src/utils/generatedFilePath.ts
- src/engine/CaptureChoiceEngine.ts
- src/engine/TemplateEngine.ts
- src/engine/assertCreatableFilePath.ts
- src/gui/ChoiceBuilder/components/FormatPreviewField.test.ts
- src/preflight/runOnePagePreflight.ts
- src/formatters/completeFormatter.test.ts
- src/engine/applyTemplateToActiveNote.ts
- src/formatters/formatDisplayFormatter-1589-vdate.test.ts
- src/preflight/OnePageInputModal.test.ts
- src/engine/CaptureChoiceEngine.selection.test.ts
- src/engine/TemplateEngine-1591-refuse-name.test.ts
- src/styles.css
- src/preflight/OnePageInputModal.ts
- src/preflight/runOnePagePreflight.filenamePreview.test.ts
- src/formatters/fileNameDisplayFormatter-1578-illegal-chars.test.ts
- src/engine/applyTemplateToActiveNote.test.ts
- src/formatters/completeFormatter.ts
- src/formatters/formatDisplayFormatter.ts
- src/formatters/fileNameDisplayFormatter.test.ts
- src/formatters/fileNameDisplayFormatter.audit-cleanup.test.ts
…nswer
The file-name and format previews left {{MVALUE}} literal while the run opened
the math modal for it. Both display formatters already overrode
promptForMathValue with a "calculation_result" stand-in that nothing could
reach - a dead override is the tell that the pass was meant to be there.
Positioned where CompleteFormatter.format has it: after {{FILE:}}, before
{{RANDOM:}}.
Also closes the loop the preview exposed. RequirementCollector registers a
requirement keyed "mvalue" for this token, so the one-page input form asks for
a "Math expression" and the CLI prints missingFlags: ["value-mvalue=<value>"] -
and nothing read either answer, so the form asked and then the run opened the
modal on top of it, and passing the CLI's own recommended flag changed nothing.
replaceMathValueInString now reads the collected value first, the way {{VALUE}}
always has. READ, never write: the prompt's answer stays unstored, so
{{MVALUE}} {{MVALUE}} is still two questions.
The non-interactive abort also named "a {{MATH}} expression", a token QuickAdd
has never had (MATH_VALUE_REGEX is /{{MVALUE}}/i) - in the one message whose
job is to say which flag to pass.
Closes #1587. Closes #1607.
…red value
Two ways the preview disagreed with the run about the same token.
1. A format-less {{VDATE:due}} previewed as the raw token, while the run
supplies YYYY-MM-DD (or YYYY-MM-DD HH:mm under |time) and creates the note.
The bail-out narrows from "no name OR no format" to "no name", which is the
run's own rule.
That token's own text contains a colon, so the illegal-character diagnostic
added in #1595 was firing on it: a working {{VDATE:due}} showed a red "a file
or folder name cannot contain :" under a choice that creates 2026-07-27.md.
Rendering the date deletes the false accusation and, on {{VDATE:due|time}},
lets the true one appear - the run's datetime default really does put HH:mm in
the file name, and Obsidian really does refuse it.
2. Neither preview read an ANSWERED date. The one-page input form seeds the
user's real picks into the formatter before computing the preview, and VDATE
was the only seeded requirement type ignored - so the row showed today beside
the date just chosen. Both formatters now resolve a stored value through
renderStoredDateVariable, extracted from the run's own tail so there is one
implementation and not three. Answered-empty ("") stays empty; an unparseable
seeded string keeps the verbatim back-compat branch; only `undefined` falls
through to the example date.
Both previews also stop throwing on a half-typed |startof: unit, which blanked
the row and reddened it between two keystrokes (#1558's failure mode).
Closes #1589.
`{{title}} note` previewed as an ordinary name in ordinary styling, and the run
created nothing: formatFileName rejects the token outright, because the title
IS the file name. The preview now mirrors both of the run's checks - on the raw
format string, and on format()'s output, where an expanded {{GLOBAL_VAR:}}
snippet or a {{VALUE}} resolving to the literal text can smuggle one in.
The token keeps previewing literally rather than as an example title: it is
genuinely unresolved on screen, so the row's "Unresolved:" label is true, and
the new diagnostic is what says it will never resolve.
The output-side check needed one correction first, or it would have turned a
WORKING choice red: a {{title}} inside a spliced-in {{TEMPLATE:}} body is not a
failure. At run time that body goes through the child engine's
formatFileContent, which resolves the token to the stored title (or "") before
formatFileName ever sees the name. The preview now does the same, at the
include seam - not through the shared token resolver, which would have invented
"My Document Title" for an unstored variable.
`/\{\{title\}\}/i` also stopped being copy-pasted: six inlined copies in
CompleteFormatter and the new preview check all read TITLE_REGEX, so the rule
cannot drift between the surface that warns and the code that throws.
Closes #1588.
`Bad: {{VALUE:title}}` rendered as "Unresolved: Bad: Example Title". Every token
resolved; nothing was unresolved. The label sent the reader hunting for a broken
token when the problem was the finished name.
Two error classes shared one label, and the split already existed: #1582 added
`kind: "path"` for the capture target's hideWhen gate, defined as "the name came
out fine, the vault will just not accept it". Reusing that axis rather than
adding a parallel one keeps a single classification that cannot disagree with
itself. The row now reads Preview: / Won't be created: / Unresolved:, with
unresolved winning when both are present - a missing template beside an empty
path segment is fundamentally a failure to resolve.
The existence probe that suppresses the illegal-character error was shape-blind
in both directions, which put a wrong label on two real cases. Obsidian's path
map holds no trailing slash, so a capture target of `Meetings: 2026/` - naming
a FOLDER to pick inside, which exists - matched neither probe and went red;
while a file-name format of `Meetings: 2026` was excused by a folder of that
name although the run still cannot create `Meetings: 2026.md`. The probe now
requires a TFolder for the trailing-slash shape and a TFile otherwise.
FormatPreviewField.test.ts pins all three labels, including the both-classes
case; the illegal-character test pins all four probe shapes.
Closes #1594.
…older
The one-page input form previews the note's file name while you fill it in,
using the same FileNameDisplayFormatter the builder uses - and wired up two
fewer things, so the same format previewed differently in the two places.
Every diagnostic was thrown away. computePreview returned Record<string,string>
and the modal rendered only those strings, so a name Obsidian will refuse was
presented in ordinary styling with nothing said. This is the surface where the
message is worth the most: the user's REAL answers are seeded into the
formatter, so the row shows the exact name that is about to fail. It is now an
ordered list of rows carrying label, text and diagnostics, rendered with the
builder's own .qa-preview-issue styling and its three-state label vocabulary.
setTargetFolderPath was never called, so `Notes/{{FOLDER}}/x` previewed
`Notes//x` plus an empty-segment error nobody could see. It now resolves
against the choice's folder when the run has already decided it - a single
configured folder, no chooser - and falls back to the builder's neutral
placeholder otherwise. Deliberately NOT the raw configured folder in every
case: the run formats it first (formatFolderPath) while setTargetFolderPath
does not, so a folder of `Journal/{{DATE:YYYY-MM}}` would splice the literal
token into the name and, since every argument-bearing token carries a colon,
raise the illegal-character error against a choice that works. The builder's
row takes the same folder, so the two surfaces agree.
Three things found while wiring it up:
- The row's key was the raw record key, so it read `fileName:`.
- The first pass ran before renderField populated the value map, flashing a
stand-in over prefilled answers.
- updatePreviews had no generation guard; the 150ms debounce orders the STARTS
of these passes, not their completions, and one can read up to 25 templates.
Text and problems now commit under one monotonic token, as in the builder.
The empty tinted box reading "Preview" is gone too: it was rendered for every
Capture, Macro and Multi choice, and every Template using the default
note-title prompt, none of which produce a row.
Closes #1590.
A Template choice whose name resolves to something Obsidian refuses did not
fail when the name was computed. It failed at vault.create - by which point
QuickAdd had already created the target folder and formatted the ENTIRE
template body: real {{VALUE}} prompts, macros, and inline `js quickadd` fences
with whatever side effects those scripts have. The user answered the whole
prompt chain, their scripts ran, and then the choice died.
Measured in this worktree's isolated vault (Obsidian 1.13.0), a Template choice
named `Bad: {{VALUE:title}}` whose template contains an inline script:
before: ok:false "Choice execution failed; no file was created."
folderExists: true, inlineScriptRuns: 1
after: ok:false "Cannot create \"Repro1591Folder/Bad: My Note.md\": a file
or folder name cannot contain \":\". ..."
folderExists: false, inlineScriptRuns: 0
The rule is "reject only if the file does not already exist": ":" is legal on
macOS/Linux at the filesystem level, so a note made outside Obsidian really can
carry one, and appending to it works today because nothing ever asks Obsidian
to accept the name. Each guard therefore sits on a branch where non-existence
is already established, so it needs no probe of its own:
- TemplateEngine.createFileWithTemplate, first statement. Its two production
callers are TemplateChoiceEngine's `else` branch of vault.adapter.exists and
its freshly incremented collision name.
- CaptureChoiceEngine, gated character-for-character on the dispatch condition
that routes to onCreateFileIfItDoesntExist - placed above the heading picker,
so the user is not asked to choose a heading for a note that cannot be
created - and re-asserted at that sink so the invariant is local to it.
- The "move note to match choice settings?" offer, which creates the target
folder and then fails at renameFile. It declines quietly: the offer is an
optional convenience, and the choice's own preview explains the problem.
Deliberately NOT inside normalizeGeneratedFilePath: CaptureChoiceEngine calls
that as an existence PROBE inside try/catch, where a new throw would silently
answer "does not exist".
The character set and the {{TIME}} hint come from generatedFilePath.ts, which
the preview also reads, so the row that warns and the code that stops cannot
drift. Only the tense differs.
Closes #1591.
Five things the review caught, each measured.
The one-page form previewed an untouched required {{VDATE:}} as answered-empty.
submit() deliberately OMITS a required blank date so the sequential date prompt
still fires, while updatePreviews copied this.result wholesale - so the row
showed a name the run would never create. Both now go through one
collectAnswers(), which is where the rule already lived (it needs
dateParseErrors, which only the modal has).
parseVDateOptionsForPreview swallowed the error as well as the throw. Master's
body preview reported `Unknown date unit "we"` for a permanent typo; this
branch rendered a finished date and said nothing, for a format the run aborts
on - while {{DATE:...|startof:we}} in the same pass still reported it, so the
row contradicted itself. The options and the error are now separate channels:
the text stays stable while the unit is half-typed, the complaint goes on the
diagnostics channel the builder already holds back for 500ms.
The example date now applies |startof:/|endof: too. #1595 left snap out on the
grounds that snapping only the file-name row would split it from the body row;
both rows do it now, so that reason is gone - and the alternative was an
asymmetry ten lines wide, because the answered branch beside it snaps and
{{DATE:...|startof:month}} in the same pass always has. Only a token that asks
for a snap touches moment, so a dateless preview keeps its old path.
existsInVault's bare-name branch is no longer TFile-only. That tightening
reddened a working capture target: captureTargetResolution resolves a bare
extension-less name to a folder SCOPE when a folder of that name exists with no
note beside it, and the picker then writes to a file inside it. The probe
mirrors that rule instead of approximating it, and the trailing-slash case it
was contradicting stays fixed.
Three of the new tests were vacuous, proved by mutation:
- |startof:w does not throw (w is a real alias for week), so neither
parseVDateOptionsForPreview test entered the catch. Now |startof:we, plus a
case pinning that a valid short alias stays quiet.
- The capture guard's test passed with the guard deleted. Its placement is what
matters, so it now asserts the heading picker never opened - that fails when
the guard moves to the sink.
- The qa-hidden assertion only checked the true half, which the constructor
already satisfies.
Adds the missing coverage the review named: the body preview's {{MVALUE}} pass
(deleting it left the suite green), and the first-pass ordering in the one-page
modal.
The two "renders an ANSWERED date" cases seeded @Date:2026-08-15T00:00:00.000Z and asserted "2026-08-15". The stored value is an INSTANT and moment formats it in local time, so anywhere west of UTC it renders as the 14th - green in CET, red on the UTC runners. Built from a local Date instead, which round-trips in every zone (verified UTC-8 through UTC+14). The sibling illegal-chars test file already carried this warning in its header.
…uards
Codex spotted a real hole in likelyTargetFolderPath: it refuses a configured
folder containing `{{`, but an inline `js quickadd` fence contains none, and
the folder validator lets backticks through (INVALID_FOLDER_CHARS_REGEX is
\ / : * ? " < > |). formatFolderPath EXECUTES that fence - it is format()'s
first pass - so the previews would have spliced the raw script source into
{{FOLDER}}, which is exactly what an inert preview must never do (#1558), and
its punctuation would then have been read as illegal characters. It falls back
to the neutral placeholder now, with the rule taken from INLINE_JAVASCRIPT_REGEX
rather than approximated.
Two coverage gaps CodeRabbit named, both of which were vacuous when written and
are mutation-verified now:
- The move offer's guard had no test at all. Driving it needs the INTERACTIVE
path (a `templatePath` argument makes the run non-interactive and skips
reconciliation entirely, so the first attempt passed with the guard deleted).
Both halves are pinned: an impossible target asks nothing and renames nothing,
a legal one still offers the move.
- The capture guard's test now asserts the capture's own {{VALUE}} prompt never
opened, which is the whole point of #1591 - the user must not answer a prompt
chain for a note that cannot be created.
CodeRabbit asked whether the previous value's diagnostics can label the new one while an async pass is in flight past the 500ms idle gate. They cannot, and the regression test they asked for is what shows why: `preview` and `diagnostics` commit together under `previewToken`, so the row always holds one pass's text under that same pass's label and complaint. It can lag; it cannot mix. A stale pass landing after a newer one is discarded, which the second case pins. The comment records why the obvious fix is a downgrade: clearing `diagnostics` on a value change would leave the previous, known-bad name on screen under a plain "Preview:" label for the length of the pass - the exact assertion this row exists to withdraw.
f6f0140 to
da6e01c
Compare
|
Verified the Fixture: a Template choice whose file name format is Before (master @ bcc0f0a) — the CLI advertised a flag it then refused: After (this branch) — the advertised flag works, and the copy no longer names a token QuickAdd does not have: The read-first-never-write shape holds up: a single collected answer fills every occurrence (the collector registers exactly one requirement for the token), and an uncollected run keeps the per-occurrence prompt it has always had. One residual, noted rather than filed — it is arguably correct behaviour: an explicitly empty value ( Context: I own #1614/#1615 on |
Closes #1587. Closes #1588. Closes #1589. Closes #1590. Closes #1591. Closes #1594. Closes #1607.
The fourth round of the preview-truth cluster, after #1582 (#1563/#1564) and
#1595 (#1578/#1579/#1580), and the last one that was outstanding. One sentence:
every surface that previews a file name shows the name the run will produce,
and the run stops as soon as it knows it cannot produce one.
Six issues, three layers, all reproduced in this worktree's isolated vault
(Obsidian 1.13.0) before anything was written:
Preview: File {{MVALUE}}mvalueas a required inputPreview: {{title}} noteok:false- "{{title}} cannot be used in file names…"Unresolved: {{VDATE:due}}+ a red":"errorok:true, createdRepro/2026-07-27.mdfileName:Notes//Example Title, zero diagnosticsNotes/Folder/Name/Example Titleok:falseafter creating the folder and running the template's inline scriptUnresolved: Bad: Example TitleWhat was actually wrong
The pass lists had drifted from the run's (#1587, #1589)
{{MVALUE}}was left literal by both preview formatters, whileCompleteFormatter.formatrunsreplaceMathValueInStringandformatFileNamegoes through
format(). Both preview classes already overrodepromptForMathValuewith a"calculation_result"stand-in that nothing couldreach - a dead override is the tell.
{{VDATE:due}}with no format previewed the raw token, while the run suppliesYYYY-MM-DDand creates the note. That token's own text contains a colon,so #1595's illegal-character diagnostic was firing on it: a working format got
a red "a file or folder name cannot contain :" under a choice that creates
2026-07-27.md.Rendering the date deletes that false accusation and lets the true one appear
where it belongs. Under
|timethe run's default isYYYY-MM-DD HH:mm, whichreally does put a colon in the file name - so #1578's diagnostic now fires on
its own, with no new code:
Neither preview read an ANSWERED date either. The one-page input form seeds
the user's real picks into the formatter before computing the preview, and VDATE
was the only seeded requirement type ignored - so the row showed today's date
beside the date just chosen. Both formatters resolve a stored value through
renderStoredDateVariable, extracted from the run's own tail so there is oneimplementation and not three.
A format that can never work looked fine (#1588)
formatFileNamerejects{{title}}outright - the title is the file name -and it checks twice: the raw format string, and
format()'s output, where a{{GLOBAL_VAR:}}snippet or a{{VALUE}}can smuggle one in. Both halves aremirrored, on the same inputs.
The token keeps previewing literally rather than as an example title: it is
genuinely unresolved on screen, so "Unresolved:" is true, and the diagnostic is
what says it never will resolve.
The output-side check needed a correction first, or it would have turned a
working choice red. A
{{title}}inside a spliced-in{{TEMPLATE:}}bodyis not a failure: at run time that body goes through the child engine's
formatFileContent, which resolves the token beforeformatFileNamesees thename. The preview now does the same, at the include seam - not through the
shared token resolver, which would have invented "My Document Title" for an
unstored variable.
/\{\{title\}\}/ialso stopped being copy-pasted: six inlined copies and thenew check all read
TITLE_REGEX.One label for two different problems (#1594)
The split already existed. #1582 added
kind: "path"for the capture target'shideWhengate, defined as "the name came out fine, the vault will just notaccept it" - which is exactly the second class. Reusing that axis rather than
adding a parallel one keeps a single classification that cannot disagree with
itself. Unresolved wins when both are present: a missing template beside an
empty path segment is fundamentally a failure to resolve.
The run surface was the one place the message was worth most (#1590)
The one-page form previews the file name while you fill it in, using the same
formatter the builder uses - and wired up two fewer things.
Every diagnostic was discarded:
computePreviewreturnedRecord<string, string>and the modal rendered only those strings. Yet this isthe surface where the message matters most, because the user's real answers
are seeded in - so a title typed as
Standup: Tuesdaypreviews the exact namethat is about to fail:
setTargetFolderPathwas never called, soNotes/{{FOLDER}}/xpreviewedNotes//xplus an empty-segment error nobody could see:It now resolves against the choice's folder when the run has already decided it,
and falls back to the builder's neutral placeholder otherwise. Deliberately
not the raw configured folder in every case: the run formats it first
(
formatFolderPath) whilesetTargetFolderPathdoes not, so a folder ofJournal/{{DATE:YYYY-MM}}would splice the literal token into the name and -since every argument-bearing token carries a colon - raise the
illegal-character error against a choice that works. The builder's row takes the
same folder, so the two surfaces agree.
Three more things surfaced while wiring it up: the row's key was the raw record
key (it read
fileName:); the first pass ran beforerenderFieldpopulatedthe value map, flashing a stand-in over prefilled answers; and
updatePreviewshad no generation guard, though the 150ms debounce orders the starts of these
passes, not their completions. The empty tinted box reading "Preview" - rendered
for every Capture, Macro and Multi choice - is gone too.
The run found out far too late (#1591)
A name Obsidian refuses did not fail when the name was computed. It failed at
vault.create- by which point QuickAdd had created the target folder andformatted the entire template body: real
{{VALUE}}prompts, macros, andinline
js quickaddfences with their side effects.Measured, with a template containing an inline script:
The rule is reject only if the file does not already exist:
:is legal onmacOS/Linux at the filesystem level, so a note made outside Obsidian can carry
one, and appending to it works today because nothing ever asks Obsidian to
accept the name. Each guard sits on a branch where non-existence is already
established, so none of them probes:
TemplateEngine.createFileWithTemplate, first statement - its two productioncallers are the
elseofvault.adapter.existsand a freshly incrementedcollision name;
CaptureChoiceEngine, gated character-for-character on the dispatch conditionthat routes to
onCreateFileIfItDoesntExist, placed above the headingpicker so the user is not asked to choose a heading for a note that cannot be
created, and re-asserted at that sink;
folder and then fails at
renameFile. It declines quietly - the offer is anoptional convenience, and the choice's own preview explains why.
Explicitly not inside
normalizeGeneratedFilePath:CaptureChoiceEnginecalls that as an existence PROBE inside
try/catch, where a new throw wouldsilently answer "does not exist".
#1607, found while fixing #1587 and folded in
RequirementCollectorregisters a requirement keyedmvalue, so the one-pageform asks for a "Math expression" and the CLI prints
missingFlags: ["value-mvalue=<value>"]- and nothing read either answer. Theform asked, then the run opened the modal on top of it; passing the CLI's own
recommended flag changed nothing. It was filed separately, then folded in here,
because #1587 asks to fix the abort message that names
{{MATH}}(a tokenQuickAdd has never had), and polishing the wording of unfollowable advice is
worse than leaving it alone. Read-only consumption, so
{{MVALUE}} {{MVALUE}}is still two questions.
Tradeoffs and things deliberately not done
The illegal-character set is unchanged. #1595 measured it:
:and only:.* ? " < > | ^ [ ] #and tab all create successfully on macOS/Linux.The example date now snaps. #1595 deliberately left
|startof:/|endof:out of the file-name preview, on the grounds that snapping only that row would
split it from the body row. Both rows do it now, so that reason is gone - and
the alternative was an asymmetry ten lines wide, because the answered branch
beside it snaps (it runs the run's own renderer) and
{{DATE:...|startof:month}}in the same pass always has. Only a token that asks for a snap touches moment.
A half-typed
|startof:unit no longer blanks the row, but is stillreported. An earlier draft of this branch swallowed the error with the throw;
that deleted a diagnostic the body preview already shipped and left a format
the run aborts on previewing as a finished date. The options and the error are
separate channels now: the text stays stable while you type, the complaint waits
for the 500ms idle gate the builder already has.
existsInVaultmirrorscaptureTargetResolutionrather than approximatingit. A draft required a
TFilefor the bare-name shape, which reddened aworking capture target - a bare extension-less name that is an existing folder
with no note beside it is a folder scope, and the picker writes to a file
inside it. Residual, unchanged from before this PR: a folder named exactly like
a Template choice's file-name format still excuses the warning. Telling the two
hosts apart needs the host to say which it is, and erring toward silence is this
cluster's rule - a wrong accusation is worse than a missing one.
The one-page preview withholds an untouched required
{{VDATE:}}, becausesubmit()does: a required blank date is omitted so the sequential date promptstill fires. Both paths go through one
collectAnswers(), which lives on themodal because the rule needs
dateParseErrors, which only the modal has.Filed rather than folded in: the
<current folder>row thatTemplateChoiceEngineadds to the folder suggester, which can turn asingle-configured-folder run into a prompt (and, non-interactively, into an
abort). It is the reason
likelyTargetFolderPathis likely and not known,and fixing it changes run behaviour for people who rely on that row.
Validation
Everything below was measured in this worktree's isolated vault (Obsidian
1.13.0), against a build of this branch, after a full instance restart.
creations before and after.
Bad: Existing.mdnote written to disk outsideObsidian: the capture still appends to it, and its preview row stays quiet.
{{VDATE:}}template with a supplied answer, and the capture-target andcapture-format preview rows.
pnpm run build,tsc -noEmit,svelte-check,eslint, and the fullVitest suite (4517 passing).
An adversarial review of the design ran before any code was written, and a
second one over the finished diff; both fed back in. Three of the new tests were
vacuous and were proved so by mutation - notably the capture guard's, which
passed with the guard deleted, and now asserts the heading picker never opened.
Summary by CodeRabbit
:targets before prompts, template reading, or folder creation.{{MVALUE}}/{{VDATE}}previews now correctly reuse already-collected values, apply default time/options, and follow snapping rules.