fix(template): preserve wikilink lists in frontmatter#1154
fix(template): preserve wikilink lists in frontmatter#1154
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds scoped template-property collection and YAML-placeholder handling: collectors can return structured YAML values during formatting, formatter writes YAML-safe placeholders (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Template Engine
participant Formatter as Formatter
participant Collector as TemplatePropertyCollector
participant YamlUtils as yamlValues
participant PostProc as Post-processor
Client->>Formatter: format frontmatter (within withTemplatePropertyCollection)
Formatter->>Collector: maybeCollect(variable, context)
Collector-->>Formatter: structuredValue? (value or undefined)
alt structuredValue returned
Formatter->>YamlUtils: getYamlPlaceholder(structuredValue)
YamlUtils-->>Formatter: YAML-safe placeholder (e.g., "[]")
Formatter-->>Client: write frontmatter with placeholder
else no structuredValue
Formatter-->>Client: write frontmatter with raw transformed string
end
Client->>PostProc: processFrontMatter(file)
PostProc->>Collector: retrieve collected structured values
Collector-->>PostProc: original structured values
PostProc->>PostProc: apply structured values into YAML
PostProc-->>Client: final file with typed YAML properties
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
Deploying quickadd with
|
| Latest commit: |
c06f869
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e3390cb4.quickadd.pages.dev |
| Branch Preview URL: | https://1140-bug-list-properties-wit.quickadd.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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: 00f404a984
ℹ️ 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".
src/formatters/formatter.ts
Outdated
| const rawReplacement = | ||
| getYamlPlaceholder(structuredYamlValue) ?? | ||
| this.getVariableValue(effectiveKey); |
There was a problem hiding this comment.
This now replaces structured {{VALUE:...}} matches with placeholders ([], {}) as soon as maybeCollect returns a value, but Formatter.formatFileContent() is also used in flows that never call getAndClearTemplatePropertyVars()/processFrontMatter (for example the AI prompt formatting callback in MacroChoiceEngine.executeAIAssistant). In those contexts, YAML-frontmatter inputs lose their real structured variable content and keep placeholders in the final output, which is a functional regression from the previous raw-value replacement behavior when template property types are enabled.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/engine/CaptureChoiceEngine.ts (1)
631-635: Extract a helper for scoped formatting + property merge.The same
withTemplatePropertyCollection(...)+getAndClearTemplatePropertyVars()+mergeCapturePropertyVars(...)sequence appears four times. A helper would reduce drift risk and make future changes safer.♻️ Refactor sketch
+ private async formatWithCollection( + work: () => Promise<string>, + ): Promise<string> { + const formatted = await this.formatter.withTemplatePropertyCollection(work); + this.mergeCapturePropertyVars( + this.formatter.getAndClearTemplatePropertyVars(), + ); + return formatted; + }- const formatted = await this.formatter.withTemplatePropertyCollection( - () => this.formatter.formatContentOnly(content), - ); - this.mergeCapturePropertyVars(this.formatter.getAndClearTemplatePropertyVars()); + const formatted = await this.formatWithCollection(() => + this.formatter.formatContentOnly(content), + );- const formattedFileContent: string = - await this.formatter.withTemplatePropertyCollection(() => - this.formatter.formatContentWithFile( - formatted, - this.choice, - fileContent, - file, - ), - ); - this.mergeCapturePropertyVars(this.formatter.getAndClearTemplatePropertyVars()); + const formattedFileContent: string = await this.formatWithCollection(() => + this.formatter.formatContentWithFile( + formatted, + this.choice, + fileContent, + file, + ), + );Also applies to: 640-649, 692-696, 752-762
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/CaptureChoiceEngine.ts` around lines 631 - 635, Extract a small private helper on CaptureChoiceEngine (e.g., runScopedFormatting) that accepts a callback or content string, invokes this.formatter.withTemplatePropertyCollection(() => this.formatter.formatContentOnly(contentOrCallback)), then calls this.mergeCapturePropertyVars(this.formatter.getAndClearTemplatePropertyVars()) and returns the formatted result; replace the four inline occurrences (the blocks that call withTemplatePropertyCollection, getAndClearTemplatePropertyVars, and mergeCapturePropertyVars) with calls to this new helper to centralize the logic and avoid duplication.src/formatters/formatter-template-property-types.test.ts (1)
27-27: Prefer explicit string coercion in the test override.This keeps the override aligned with the
stringreturn contract and avoids leaking non-string runtime values through a cast.♻️ Suggested tweak
protected getVariableValue(variableName: string): string { - return (this.variables.get(variableName) as string) ?? ''; + const value = this.variables.get(variableName); + return value == null ? "" : String(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/formatters/formatter-template-property-types.test.ts` at line 27, The test override currently coerces a possibly non-string value via a TypeScript cast "(this.variables.get(variableName) as string) ?? ''", which can leak non-string runtime values; replace the cast with an explicit string coercion using String(...) and preserve the fallback so the return value always matches the string contract (e.g., use String(this.variables.get(variableName) ?? '')). Update the expression inside the test helper where "this.variables.get(variableName)" is used so the function/method returns a real string rather than relying on "as string".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/engine/CaptureChoiceEngine.ts`:
- Around line 631-635: Extract a small private helper on CaptureChoiceEngine
(e.g., runScopedFormatting) that accepts a callback or content string, invokes
this.formatter.withTemplatePropertyCollection(() =>
this.formatter.formatContentOnly(contentOrCallback)), then calls
this.mergeCapturePropertyVars(this.formatter.getAndClearTemplatePropertyVars())
and returns the formatted result; replace the four inline occurrences (the
blocks that call withTemplatePropertyCollection,
getAndClearTemplatePropertyVars, and mergeCapturePropertyVars) with calls to
this new helper to centralize the logic and avoid duplication.
In `@src/formatters/formatter-template-property-types.test.ts`:
- Line 27: The test override currently coerces a possibly non-string value via a
TypeScript cast "(this.variables.get(variableName) as string) ?? ''", which can
leak non-string runtime values; replace the cast with an explicit string
coercion using String(...) and preserve the fallback so the return value always
matches the string contract (e.g., use String(this.variables.get(variableName)
?? '')). Update the expression inside the test helper where
"this.variables.get(variableName)" is used so the function/method returns a real
string rather than relying on "as string".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ac76770-f229-4658-9c3b-c52e43c55d25
📒 Files selected for processing (9)
src/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/SingleTemplateEngine.tssrc/engine/TemplateChoiceEngine.notice.test.tssrc/engine/TemplateEngine.tssrc/engine/templateEngine-title.test.tssrc/formatters/formatter-template-property-types.test.tssrc/formatters/formatter.tstests/e2e/template-property-links.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/template-property-links.test.ts
1 participant
Preserve structured frontmatter arrays when template values contain multiple wikilinks.
QuickAdd was stringifying array values into the raw template output before Obsidian had a chance to post-process frontmatter. For values like
[\"[[John Doe]]\", \"[[Jane Doe]]\"], that produced invalid YAML such asauthors: [[John Doe]],[[Jane Doe]], soprocessFrontMatter()never got to rewrite the property as a list.This change moves the structured-value checks into the shared YAML helpers and writes YAML-safe placeholders like
[]during the initial replacement pass. Obsidian can then parse the file and apply the real array value through frontmatter post-processing.I considered patching only the template engine, but the placeholder behavior belongs in the shared formatter/YAML path because Capture and other frontmatter flows use the same structured variable mechanism.
Added regression coverage in unit tests and a new
obsidian-e2espec that reproduces issue #1140 against the live dev vault.Fixes #1140
Summary by CodeRabbit
New Features
Tests