feat: option to insert template or capture links into the frontmatter of active note#1276
feat: option to insert template or capture links into the frontmatter of active note#1276ArjanSeijs wants to merge 3 commits into
Conversation
|
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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds an "inFrontmatter" link placement and optional frontmatterProperty option, updates types and tests, implements frontmatter-array append logic in insertion utilities, surfaces a configurable frontmatter property input in both choice builders, and updates docs to document the new mode. ChangesFrontmatter Link Insertion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 frontmatterProperty silently dropped when toggling link mode between required/optional
When a user toggles the "Enabled (requires active file)" / "Enabled (skip if no active file)" dropdown while the placement is set to "In frontmatter property", the onChange handler constructs a new appendLink object that includes placement, requireActiveFile, and linkType, but omits frontmatterProperty. The normalizedOptions captured at the top of addAppendLinkSetting() contains the frontmatterProperty value (via normalizeAppendLinkOptions at src/types/linkPlacement.ts:85), but it is not forwarded. After this.reload(), the frontmatter property text field resets to empty and the user's configured property name is lost.
The same issue exists in templateChoiceBuilder.ts:336-353.
(Refers to lines 791-806)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🟡 frontmatterProperty silently dropped when toggling link mode in template choice builder
Same issue as in the capture choice builder: when a user toggles between "Enabled (requires active file)" and "Enabled (skip if no active file)" while placement is "In frontmatter property", the frontmatterProperty is not included in the new appendLink object (lines 337-342 and 345-350). The property name is silently lost after this.reload().
(Refers to lines 336-353)
Was this helpful? React with 👍 or 👎 to provide feedback.
| app.fileManager.processFrontMatter(file, (frontmatter) => { | ||
| if (frontmatter[frontmatterProperty] === undefined) { | ||
| frontmatter[frontmatterProperty] = [] | ||
| } | ||
| if (!Array.isArray(frontmatter[frontmatterProperty])) { | ||
| const message = "Could not add into non array property:" + frontmatterProperty; | ||
| throw new Error(message) | ||
| } | ||
| frontmatter[frontmatterProperty].push(text) | ||
| }) | ||
| return |
There was a problem hiding this comment.
🔴 processFrontMatter is async but not awaited, causing silent failures and race conditions
app.fileManager.processFrontMatter() returns a Promise<void> (confirmed by every other callsite in the codebase using await, e.g. src/engine/QuickAddEngine.ts:286), but the call at line 602 is not awaited. This causes three problems:
- The function returns (and
insertFileLinkToActiveViewatsrc/utilityObsidian.ts:768returnstrue) before the frontmatter write has actually completed. - The error thrown inside the callback for non-array properties (line 608) becomes an unhandled promise rejection instead of propagating to the caller.
- Subsequent operations after the
insertFileLinkToActiveViewcall (e.g., opening the file insrc/engine/TemplateChoiceEngine.ts:144) can race with the pending frontmatter write.
Since insertLinkWithPlacement is synchronous (function, not async function), this requires making it async or restructuring the frontmatter path to properly await the promise.
Prompt for agents
The insertLinkWithPlacement function at src/utilityObsidian.ts:552 is currently synchronous (returns void), but the new inFrontmatter code path calls app.fileManager.processFrontMatter which is async and returns Promise<void>. The promise is silently discarded.
To fix this properly, you need to either:
1. Make insertLinkWithPlacement async (returning Promise<void>) and await processFrontMatter. Then update insertFileLinkToActiveView (line 734) to also be async and await the call. Then update all callers (CaptureChoiceEngine.insertCaptureLink at line 173, TemplateChoiceEngine around line 141) to await as well.
2. Or, separate the frontmatter insertion into its own async function and handle it at a higher level in the engine code, keeping insertLinkWithPlacement synchronous for editor-based placements.
Option 1 is simpler but has a wider blast radius. Option 2 preserves the synchronous contract for existing placement modes while properly handling the async frontmatter path.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/utilityObsidian.ts (1)
761-766:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate call site to handle async
insertLinkWithPlacement.If
insertLinkWithPlacementis made async (as required to fix the missingawaitonprocessFrontMatter), this call site should also await the call and potentially propagate the async requirement up the call chain.🔄 Proposed fix
Check if
insertFileLinkToActiveViewis already async or if it needs to become async:-export function insertFileLinkToActiveView( +export async function insertFileLinkToActiveView( app: App, file: TFile, linkOptions: AppendLinkOptions, ): boolean { // ... existing code ... - insertLinkWithPlacement( + await insertLinkWithPlacement( app, linkText, linkOptions.placement, { requireActiveView: false, frontmatterProperty : linkOptions.frontmatterProperty }, );Then check all call sites of
insertFileLinkToActiveViewto ensure they handle the async nature properly.🤖 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/utilityObsidian.ts` around lines 761 - 766, The call to insertLinkWithPlacement must await the now-async function: update the call in insertFileLinkToActiveView to use await insertLinkWithPlacement(...) and mark insertFileLinkToActiveView as async if it isn’t already; then propagate async up the call chain by updating any callers of insertFileLinkToActiveView to await it (or handle the returned Promise). Ensure this ties together with the async change to processFrontMatter so that frontmatter processing completes before continuing.
♻️ Duplicate comments (1)
src/gui/ChoiceBuilder/templateChoiceBuilder.ts (1)
425-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
linkTypefield when updatingappendLink.This is the same issue as in
captureChoiceBuilder.ts: when the user changes the frontmatter property (lines 444-449), the code reconstructsthis.choice.appendLinkbut omits thelinkTypefield, which will silently lose any previously selected embed link type.🔧 Proposed fix
if (placementSupportsFrontmatter(normalizedOptions.placement)) { - const linkTypeSetting: Setting = new Setting(this.contentEl); + const frontmatterPropertySetting: Setting = new Setting(this.contentEl); const current = typeof this.choice.appendLink !== "boolean" ? this.choice.appendLink.frontmatterProperty : ''; - linkTypeSetting + frontmatterPropertySetting .setName("Frontmatter property") .setDesc("Choose the frontmatter property to insert the link into.") .addText((text) => { text.setValue(current ?? '') text.onChange((value: string) => { const currentValue = this.choice.appendLink; const requireActiveFile = typeof currentValue === "boolean" ? normalizedOptions.requireActiveFile : currentValue.requireActiveFile; const placement = typeof currentValue === "boolean" ? normalizedOptions.placement : currentValue.placement; + const linkType = + typeof currentValue === "boolean" + ? normalizedLinkType + : currentValue.linkType ?? normalizedLinkType; this.choice.appendLink = { enabled: true, placement, requireActiveFile, + linkType, frontmatterProperty: value, }; }); }); }🤖 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/gui/ChoiceBuilder/templateChoiceBuilder.ts` around lines 425 - 452, When updating this.choice.appendLink in the addText onChange handler (inside placementSupportsFrontmatter block), preserve the existing linkType field so it is not dropped; read linkType from the current this.choice.appendLink (or fall back to normalizedOptions.linkType/default) and include it in the reconstructed object along with enabled, placement, requireActiveFile, and frontmatterProperty so linkType is retained (refer to placementSupportsFrontmatter, this.choice.appendLink, normalizedOptions, and the addText onChange handler).
🧹 Nitpick comments (2)
src/types/linkPlacement.ts (1)
17-19: ⚡ Quick winAdd explicit return type annotation.
The function signature omits the return type. While TypeScript infers it correctly, exported functions should include explicit return types for clarity and to prevent unintended API changes.
♻️ Proposed fix
-export function placementSupportsFrontmatter(placement : LinkPlacement) : boolean { +export function placementSupportsFrontmatter(placement: LinkPlacement): boolean {🤖 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/types/linkPlacement.ts` around lines 17 - 19, The exported function placementSupportsFrontmatter currently lacks an explicit return type; update its signature to include an explicit ": boolean" return annotation (e.g., export function placementSupportsFrontmatter(placement: LinkPlacement): boolean) so the function's return type is clear and stable; ensure the change is made on the placementSupportsFrontmatter declaration and that it still returns a boolean expression using the LinkPlacement type.src/utilityObsidian.ts (1)
556-558: ⚡ Quick winConsider using
undefinedinstead of empty string as the default.The current implementation defaults
frontmatterPropertyto an empty string, which then fails validation at line 593. Usingundefinedas the default (by omitting the default value assignment) would be more idiomatic TypeScript and clearer in intent.♻️ Proposed fix
export function insertLinkWithPlacement( app: App, text: string, mode: LinkPlacement = "replaceSelection", - options: { requireActiveView?: boolean; frontmatterProperty?: string} = {}, + options: { requireActiveView?: boolean; frontmatterProperty?: string } = {}, ) { - const { requireActiveView = true, frontmatterProperty = '' } = options; + const { requireActiveView = true, frontmatterProperty } = options;Then at line 593, check for undefined:
- if(!frontmatterProperty) { + if (!frontmatterProperty) {🤖 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/utilityObsidian.ts` around lines 556 - 558, Change the default for frontmatterProperty from an empty string to undefined by removing the empty-string default in the options destructuring (update the const { requireActiveView = true, frontmatterProperty } = options in the function that declares options: { requireActiveView?: boolean; frontmatterProperty?: string }). Then update the validation that currently fails when frontmatterProperty is '' (the check around line 593) to explicitly test for undefined (e.g., frontmatterProperty === undefined or frontmatterProperty != null as appropriate) so an omitted property is handled idiomatically.
🤖 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/captureChoiceBuilder.ts`:
- Around line 881-908: When rebuilding this.choice.appendLink in the frontmatter
property text.onChange handler, preserve the existing linkType instead of
omitting it: read linkType from currentValue when it's an object (or fall back
to normalizedOptions.linkType/default) and include linkType in the new object
along with enabled, placement, requireActiveFile, and frontmatterProperty so the
user's previously selected link type (e.g., embed) is not lost; modify the
text.onChange block that updates this.choice.appendLink to include linkType
sourced from currentValue || normalizedOptions.
In `@src/utilityObsidian.ts`:
- Around line 592-613: The processFrontMatter callback is awaited incorrectly:
call app.fileManager.processFrontMatter(...) must be awaited so the frontmatter
write completes before returning; update the function containing this block
(e.g., insertLinkWithPlacement) to be async, prefix the processFrontMatter call
with await, and update any callers (such as the call at the site that previously
invoked insertLinkWithPlacement) to await insertLinkWithPlacement as well;
ensure you still throw the same errors for invalid frontmatterProperty and
missing file and keep the existing logic inside the callback unchanged.
---
Outside diff comments:
In `@src/utilityObsidian.ts`:
- Around line 761-766: The call to insertLinkWithPlacement must await the
now-async function: update the call in insertFileLinkToActiveView to use await
insertLinkWithPlacement(...) and mark insertFileLinkToActiveView as async if it
isn’t already; then propagate async up the call chain by updating any callers of
insertFileLinkToActiveView to await it (or handle the returned Promise). Ensure
this ties together with the async change to processFrontMatter so that
frontmatter processing completes before continuing.
---
Duplicate comments:
In `@src/gui/ChoiceBuilder/templateChoiceBuilder.ts`:
- Around line 425-452: When updating this.choice.appendLink in the addText
onChange handler (inside placementSupportsFrontmatter block), preserve the
existing linkType field so it is not dropped; read linkType from the current
this.choice.appendLink (or fall back to normalizedOptions.linkType/default) and
include it in the reconstructed object along with enabled, placement,
requireActiveFile, and frontmatterProperty so linkType is retained (refer to
placementSupportsFrontmatter, this.choice.appendLink, normalizedOptions, and the
addText onChange handler).
---
Nitpick comments:
In `@src/types/linkPlacement.ts`:
- Around line 17-19: The exported function placementSupportsFrontmatter
currently lacks an explicit return type; update its signature to include an
explicit ": boolean" return annotation (e.g., export function
placementSupportsFrontmatter(placement: LinkPlacement): boolean) so the
function's return type is clear and stable; ensure the change is made on the
placementSupportsFrontmatter declaration and that it still returns a boolean
expression using the LinkPlacement type.
In `@src/utilityObsidian.ts`:
- Around line 556-558: Change the default for frontmatterProperty from an empty
string to undefined by removing the empty-string default in the options
destructuring (update the const { requireActiveView = true, frontmatterProperty
} = options in the function that declares options: { requireActiveView?:
boolean; frontmatterProperty?: string }). Then update the validation that
currently fails when frontmatterProperty is '' (the check around line 593) to
explicitly test for undefined (e.g., frontmatterProperty === undefined or
frontmatterProperty != null as appropriate) so an omitted property is handled
idiomatically.
🪄 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
Run ID: 2852136a-95af-4bb7-9193-cdd0a1ff6149
📒 Files selected for processing (7)
docs/docs/Choices/CaptureChoice.mddocs/docs/Choices/TemplateChoice.mdsrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/gui/ChoiceBuilder/templateChoiceBuilder.tssrc/types/linkPlacement.test.tssrc/types/linkPlacement.tssrc/utilityObsidian.ts
| if (placementSupportsFrontmatter(normalizedOptions.placement)) { | ||
| const linkTypeSetting: Setting = new Setting(this.contentEl); | ||
| const current = typeof this.choice.appendLink !== "boolean" ? this.choice.appendLink.frontmatterProperty : ''; | ||
| linkTypeSetting | ||
| .setName("Frontmatter property") | ||
| .setDesc("Choose the frontmatter property to insert the link into.") | ||
| .addText((text) => { | ||
| text.setValue(current ?? '') | ||
| text.onChange((value: string) => { | ||
| const currentValue = this.choice.appendLink; | ||
| const requireActiveFile = | ||
| typeof currentValue === "boolean" | ||
| ? normalizedOptions.requireActiveFile | ||
| : currentValue.requireActiveFile; | ||
| const placement = | ||
| typeof currentValue === "boolean" | ||
| ? normalizedOptions.placement | ||
| : currentValue.placement; | ||
|
|
||
| this.choice.appendLink = { | ||
| enabled: true, | ||
| placement, | ||
| requireActiveFile, | ||
| frontmatterProperty: value, | ||
| }; | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing linkType field when updating appendLink.
When the user changes the frontmatter property (lines 900-905), the code reconstructs this.choice.appendLink but omits the linkType field. This means that if the user had previously selected an embed link type, changing the frontmatter property will silently lose that setting and revert to undefined/default link type.
🔧 Proposed fix
if (placementSupportsFrontmatter(normalizedOptions.placement)) {
- const linkTypeSetting: Setting = new Setting(this.contentEl);
+ const frontmatterPropertySetting: Setting = new Setting(this.contentEl);
const current = typeof this.choice.appendLink !== "boolean" ? this.choice.appendLink.frontmatterProperty : '';
- linkTypeSetting
+ frontmatterPropertySetting
.setName("Frontmatter property")
.setDesc("Choose the frontmatter property to insert the link into.")
.addText((text) => {
text.setValue(current ?? '')
text.onChange((value: string) => {
const currentValue = this.choice.appendLink;
const requireActiveFile =
typeof currentValue === "boolean"
? normalizedOptions.requireActiveFile
: currentValue.requireActiveFile;
const placement =
typeof currentValue === "boolean"
? normalizedOptions.placement
: currentValue.placement;
+ const linkType =
+ typeof currentValue === "boolean"
+ ? normalizedLinkType
+ : currentValue.linkType ?? normalizedLinkType;
this.choice.appendLink = {
enabled: true,
placement,
requireActiveFile,
+ linkType,
frontmatterProperty: value,
};
});
});
}🤖 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/gui/ChoiceBuilder/captureChoiceBuilder.ts` around lines 881 - 908, When
rebuilding this.choice.appendLink in the frontmatter property text.onChange
handler, preserve the existing linkType instead of omitting it: read linkType
from currentValue when it's an object (or fall back to
normalizedOptions.linkType/default) and include linkType in the new object along
with enabled, placement, requireActiveFile, and frontmatterProperty so the
user's previously selected link type (e.g., embed) is not lost; modify the
text.onChange block that updates this.choice.appendLink to include linkType
sourced from currentValue || normalizedOptions.
| if (mode === "inFrontmatter") { | ||
| if(!frontmatterProperty) { | ||
| const message = "Invalid frontmatter property: " + frontmatterProperty; | ||
| throw new Error(message); | ||
| } | ||
| const file = view.file; | ||
| if(!file) { | ||
| const message = "Could not find file of active view"; | ||
| throw new Error(message); | ||
| } | ||
| app.fileManager.processFrontMatter(file, (frontmatter) => { | ||
| if (frontmatter[frontmatterProperty] === undefined) { | ||
| frontmatter[frontmatterProperty] = [] | ||
| } | ||
| if (!Array.isArray(frontmatter[frontmatterProperty])) { | ||
| const message = "Could not add into non array property:" + frontmatterProperty; | ||
| throw new Error(message) | ||
| } | ||
| frontmatter[frontmatterProperty].push(text) | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing await on async processFrontMatter call.
The app.fileManager.processFrontMatter method returns a Promise<void>, but line 602 does not await it. This means the function returns before the frontmatter is actually written, which can cause race conditions or data loss if subsequent code assumes the frontmatter has been updated.
🐛 Proposed fix
- if (mode === "inFrontmatter") {
- if(!frontmatterProperty) {
+ if (mode === "inFrontmatter") {
+ if (!frontmatterProperty) {
const message = "Invalid frontmatter property: " + frontmatterProperty;
throw new Error(message);
}
const file = view.file;
- if(!file) {
+ if (!file) {
const message = "Could not find file of active view";
throw new Error(message);
}
- app.fileManager.processFrontMatter(file, (frontmatter) => {
+ await app.fileManager.processFrontMatter(file, (frontmatter) => {
if (frontmatter[frontmatterProperty] === undefined) {
- frontmatter[frontmatterProperty] = []
+ frontmatter[frontmatterProperty] = [];
}
if (!Array.isArray(frontmatter[frontmatterProperty])) {
const message = "Could not add into non array property:" + frontmatterProperty;
- throw new Error(message)
+ throw new Error(message);
}
- frontmatter[frontmatterProperty].push(text)
- })
- return
+ frontmatter[frontmatterProperty].push(text);
+ });
+ return;
}Note: This also requires making insertLinkWithPlacement an async function:
-export function insertLinkWithPlacement(
+export async function insertLinkWithPlacement(And updating the call site at line 761 to await it.
🤖 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/utilityObsidian.ts` around lines 592 - 613, The processFrontMatter
callback is awaited incorrectly: call app.fileManager.processFrontMatter(...)
must be awaited so the frontmatter write completes before returning; update the
function containing this block (e.g., insertLinkWithPlacement) to be async,
prefix the processFrontMatter call with await, and update any callers (such as
the call at the site that previously invoked insertLinkWithPlacement) to await
insertLinkWithPlacement as well; ensure you still throw the same errors for
invalid frontmatterProperty and missing file and keep the existing logic inside
the callback unchanged.
|
I can add that that the frontmatterProperty persists after changing the settings, however this not the case for the linkType since this is also just reset when switching between require vs skip active file. For the processFrontMatter
|
|
Hey! Thanks for working on this. I like the feature direction, and I can see why it would be useful for TTRPG/worldbuilding vaults. I did some local testing in the dev vault, and I think this needs a bit more work before I can merge it. On One related thing to keep in mind: template/capture choices can also run inside macros. If frontmatter insertion errors are handled only locally with a notice, macro execution may still continue after a failed link insertion. I tested a small macro in the dev vault where step 1 hit the scalar-property frontmatter error and step 2 created a marker note; step 2 still ran. I don’t think this needs to be treated as a separate blocker by itself, but it’s a good reason to prefer awaiting/propagating the failure through the existing choice/macro error handling instead of making this a local-only notice. The other bigger issue is YAML/frontmatter handling. At the moment this only works for missing/null properties and array properties. For example: related:
- "[[Existing]]"works, but: related: "[[Existing]]"fails with That makes the feature feel more like “append to a frontmatter list property” than “insert into a frontmatter property”. I think we should either handle scalar values by converting them to a list, or make the UI/docs explicitly say that this only supports list properties. My preference would be a small helper with tests, roughly covering:
I also found one settings issue: changing “Enabled (requires active file)” to “Enabled (skip if no active file)” drops Suggested next steps:
I’m happy to review another pass. The core idea is good; I just want this to be reliable across normal Obsidian frontmatter shapes before shipping it. |
2 participants
Summary
This adds an option to insert links for templates and captures into the frontmatter of the active note. For my TTRPG vault i make heavy use of quickadd and needed this feature to quickly add links to the frontmatter of a file. For example for quickly adding npcs or sub locations to the frontmatter of my towns.
Changes
Testing / validation
Obsidian version 1.12.7
I updated the tests in
linkPlacement.test.tsto account for the new LinkPlacement.Testing the feature was done manually, see the gif for results.
Checklist
— it becomes the squash-merge commit and drives the released version.
Summary by CodeRabbit
New Features
Documentation