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 6 commits into
Conversation
📝 WalkthroughWalkthroughAdds an "inFrontmatter" link placement and optional frontmatterProperty/frontmatterHandling configuration, updates type contracts and tests, implements frontmatter-array append logic with three handling modes in insertion utilities, makes insertion functions async and awaits them in engines, surfaces configurable UI inputs in both choice builders, and updates documentation. ChangesFrontmatter Link Insertion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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.
|
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. |
…e frontmatter. Behavior on existings properties are dependend on the frontmatterHandling. * Added tests for the insertIntoFrontmatterProperty
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/CaptureChoiceEngine.ts`:
- Around line 304-307: The success notice is being emitted before the awaited
insertCaptureLink operation completes in two locations. In both the method
containing the insertCaptureLink call around line 304-307 and the similar method
around line 384-387, move the success notice block (the code that emits or
displays the success message) to execute after the await
this.insertCaptureLink(file, linkOptions, ...) completes successfully. This
ensures users only see a success notice if the insertion actually succeeds
rather than before the async operation finishes.
In `@src/utilityObsidian.ts`:
- Around line 544-572: The `insertIntoFrontmatterProperty` function validates
`frontmatterProperty` using only a truthy check, which allows whitespace-only
strings like " " to pass validation and be used as unintended keys. Trim the
`frontmatterProperty` parameter at the start of the function before performing
the validation check, then use this trimmed value for all subsequent property
lookups and operations throughout the function.
🪄 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: 2e5f7347-465b-465c-9a67-474fadbbcee1
📒 Files selected for processing (9)
docs/docs/Choices/CaptureChoice.mddocs/docs/Choices/TemplateChoice.mdsrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateChoiceEngine.tssrc/gui/ChoiceBuilder/captureChoiceBuilder.tssrc/gui/ChoiceBuilder/templateChoiceBuilder.tssrc/types/linkPlacement.tssrc/utilityObsidian.test.tssrc/utilityObsidian.ts
✅ Files skipped from review due to trivial changes (1)
- docs/docs/Choices/CaptureChoice.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/docs/Choices/TemplateChoice.md
- src/types/linkPlacement.ts
- src/gui/ChoiceBuilder/templateChoiceBuilder.ts
- src/gui/ChoiceBuilder/captureChoiceBuilder.ts
| await this.insertCaptureLink(file, linkOptions, { | ||
| isCanvasTriggered: !!canvasTarget, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Show success notice only after awaited link insertion succeeds.
Both flows emit success before await this.insertCaptureLink(...). If insertion fails, users still see a success notice first. Move the success notice block below the awaited insertion in both methods.
Also applies to: 384-387
🤖 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/CaptureChoiceEngine.ts` around lines 304 - 307, The success notice
is being emitted before the awaited insertCaptureLink operation completes in two
locations. In both the method containing the insertCaptureLink call around line
304-307 and the similar method around line 384-387, move the success notice
block (the code that emits or displays the success message) to execute after the
await this.insertCaptureLink(file, linkOptions, ...) completes successfully.
This ensures users only see a success notice if the insertion actually succeeds
rather than before the async operation finishes.
| export function insertIntoFrontmatterProperty(frontmatter: any, frontmatterProperty: string, value: string, frontmatterHandling: FrontmatterHandling) { | ||
| if (!frontmatterProperty) { | ||
| const message = `Invalid frontmatter property: '${frontmatterProperty}'`; | ||
| throw new Error(message); | ||
| } | ||
| if (frontmatter[frontmatterProperty] === null) { | ||
| // treat null as empty array | ||
| frontmatter[frontmatterProperty] = [] | ||
| } else if (frontmatter[frontmatterProperty] === undefined) { | ||
| if (frontmatterHandling === "error") { | ||
| const message = `Inserting link into: ${frontmatterProperty} failed: Property does not exist and frontmatterType is set to 'error'`; | ||
| throw new Error(message) | ||
| } | ||
| // create missing property with empty array | ||
| frontmatter[frontmatterProperty] = [] | ||
| } else if (typeof frontmatter[frontmatterProperty] === "object" && !Array.isArray(frontmatter[frontmatterProperty])) { | ||
| const message = `Inserting link into: ${frontmatterProperty} failed: Existing property is object, expected string, scalar or array`; | ||
| throw new Error(message) | ||
| } else if (!Array.isArray(frontmatter[frontmatterProperty])) { | ||
| if (frontmatterHandling !== "alwaysAppend") { | ||
| const message = `Inserting link into: ${frontmatterProperty} failed: Property is not an array and frontmatterType is set to '${frontmatterHandling}'`; | ||
| throw new Error(message) | ||
| } | ||
| // Convert existing value to array | ||
| frontmatter[frontmatterProperty] = [frontmatter[frontmatterProperty]] | ||
| } | ||
| // Append the value | ||
| frontmatter[frontmatterProperty].push(value) | ||
| } |
There was a problem hiding this comment.
Reject whitespace-only frontmatter property names.
frontmatterProperty is validated with a truthy check only, so values like " " are accepted and written as unintended keys. Trim before validating and use the normalized key for all lookups.
Suggested fix
export function insertIntoFrontmatterProperty(frontmatter: any, frontmatterProperty: string, value: string, frontmatterHandling: FrontmatterHandling) {
- if (!frontmatterProperty) {
+ const property = frontmatterProperty.trim();
+ if (!property) {
const message = `Invalid frontmatter property: '${frontmatterProperty}'`;
throw new Error(message);
}
- if (frontmatter[frontmatterProperty] === null) {
+ if (frontmatter[property] === null) {
// treat null as empty array
- frontmatter[frontmatterProperty] = []
- } else if (frontmatter[frontmatterProperty] === undefined) {
+ frontmatter[property] = []
+ } else if (frontmatter[property] === undefined) {
if (frontmatterHandling === "error") {
const message = `Inserting link into: ${frontmatterProperty} failed: Property does not exist and frontmatterType is set to 'error'`;
throw new Error(message)
}
// create missing property with empty array
- frontmatter[frontmatterProperty] = []
- } else if (typeof frontmatter[frontmatterProperty] === "object" && !Array.isArray(frontmatter[frontmatterProperty])) {
+ frontmatter[property] = []
+ } else if (typeof frontmatter[property] === "object" && !Array.isArray(frontmatter[property])) {
const message = `Inserting link into: ${frontmatterProperty} failed: Existing property is object, expected string, scalar or array`;
throw new Error(message)
- } else if (!Array.isArray(frontmatter[frontmatterProperty])) {
+ } else if (!Array.isArray(frontmatter[property])) {
if (frontmatterHandling !== "alwaysAppend") {
const message = `Inserting link into: ${frontmatterProperty} failed: Property is not an array and frontmatterType is set to '${frontmatterHandling}'`;
throw new Error(message)
}
// Convert existing value to array
- frontmatter[frontmatterProperty] = [frontmatter[frontmatterProperty]]
+ frontmatter[property] = [frontmatter[property]]
}
// Append the value
- frontmatter[frontmatterProperty].push(value)
+ frontmatter[property].push(value)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function insertIntoFrontmatterProperty(frontmatter: any, frontmatterProperty: string, value: string, frontmatterHandling: FrontmatterHandling) { | |
| if (!frontmatterProperty) { | |
| const message = `Invalid frontmatter property: '${frontmatterProperty}'`; | |
| throw new Error(message); | |
| } | |
| if (frontmatter[frontmatterProperty] === null) { | |
| // treat null as empty array | |
| frontmatter[frontmatterProperty] = [] | |
| } else if (frontmatter[frontmatterProperty] === undefined) { | |
| if (frontmatterHandling === "error") { | |
| const message = `Inserting link into: ${frontmatterProperty} failed: Property does not exist and frontmatterType is set to 'error'`; | |
| throw new Error(message) | |
| } | |
| // create missing property with empty array | |
| frontmatter[frontmatterProperty] = [] | |
| } else if (typeof frontmatter[frontmatterProperty] === "object" && !Array.isArray(frontmatter[frontmatterProperty])) { | |
| const message = `Inserting link into: ${frontmatterProperty} failed: Existing property is object, expected string, scalar or array`; | |
| throw new Error(message) | |
| } else if (!Array.isArray(frontmatter[frontmatterProperty])) { | |
| if (frontmatterHandling !== "alwaysAppend") { | |
| const message = `Inserting link into: ${frontmatterProperty} failed: Property is not an array and frontmatterType is set to '${frontmatterHandling}'`; | |
| throw new Error(message) | |
| } | |
| // Convert existing value to array | |
| frontmatter[frontmatterProperty] = [frontmatter[frontmatterProperty]] | |
| } | |
| // Append the value | |
| frontmatter[frontmatterProperty].push(value) | |
| } | |
| export function insertIntoFrontmatterProperty(frontmatter: any, frontmatterProperty: string, value: string, frontmatterHandling: FrontmatterHandling) { | |
| const property = frontmatterProperty.trim(); | |
| if (!property) { | |
| const message = `Invalid frontmatter property: '${frontmatterProperty}'`; | |
| throw new Error(message); | |
| } | |
| if (frontmatter[property] === null) { | |
| // treat null as empty array | |
| frontmatter[property] = [] | |
| } else if (frontmatter[property] === undefined) { | |
| if (frontmatterHandling === "error") { | |
| const message = `Inserting link into: ${frontmatterProperty} failed: Property does not exist and frontmatterType is set to 'error'`; | |
| throw new Error(message) | |
| } | |
| // create missing property with empty array | |
| frontmatter[property] = [] | |
| } else if (typeof frontmatter[property] === "object" && !Array.isArray(frontmatter[property])) { | |
| const message = `Inserting link into: ${frontmatterProperty} failed: Existing property is object, expected string, scalar or array`; | |
| throw new Error(message) | |
| } else if (!Array.isArray(frontmatter[property])) { | |
| if (frontmatterHandling !== "alwaysAppend") { | |
| const message = `Inserting link into: ${frontmatterProperty} failed: Property is not an array and frontmatterType is set to '${frontmatterHandling}'`; | |
| throw new Error(message) | |
| } | |
| // Convert existing value to array | |
| frontmatter[property] = [frontmatter[property]] | |
| } | |
| // Append the value | |
| frontmatter[property].push(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/utilityObsidian.ts` around lines 544 - 572, The
`insertIntoFrontmatterProperty` function validates `frontmatterProperty` using
only a truthy check, which allows whitespace-only strings like " " to pass
validation and be used as unintended keys. Trim the `frontmatterProperty`
parameter at the start of the function before performing the validation check,
then use this trimmed value for all subsequent property lookups and operations
throughout the function.
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
Bug Fixes / Improvements
Tests