fix: append link respects caret when focused in a frontmatter property (#768)#1340
fix: append link respects caret when focused in a frontmatter property (#768)#1340Ivikhostrup wants to merge 5 commits into
Conversation
… fields When "Append link to current file" runs while the caret is inside a frontmatter property (Obsidian's Properties widget), the markdown body editor is not focused, so editor.getCursor()/listSelections() report a stale body caret. The link was inserted at the first body line instead of at the user's cursor. insertLinkWithPlacement now detects when focus is in a property field (via the focused DOM element inside .metadata-property / .metadata-properties-container) and inserts the link at that element's caret, firing an input event so Obsidian persists it to the frontmatter. It falls back to the existing body-editor behavior when no property field is focused or the element type is unsupported. Adds src/utilityObsidian.appendLink-768.test.ts covering the focus detection helper, the editable-element insertion helper, and the property-vs-body routing in insertLinkWithPlacement. Co-Authored-By: Claude <noreply@anthropic.com>
|
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 (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWhen a QuickAdd choice runs, the code now captures the currently focused frontmatter property field (file and property key) before any prompts or suggesters appear. If a property was focused, capture and template choices route their link insertion through a persistent frontmatter-aware path that merges the link into the correct frontmatter property using ChangesFrontmatter Property Link Insertion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
⚔️ Resolve merge conflicts
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 |
Ivikhostrup
changed the title
Funnel the focus-detection + insertion behind a single intention-revealing tryInsertIntoFocusedProperty(text) call, name the Properties-widget selector and the editable-element check, and use descriptive variable names.
chhoumann
left a comment
chhoumann
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @Ivikhostrup — the diagnosis in #768 is exactly right (a focused property is a separate input, not the CodeMirror caret), and the naming refactor in dda12e5b reads well. I want to call out what's solid before the problems: the body-insertion path is left completely untouched (purely additive, multi-cursor + every placement mode byte-for-byte identical) so there's zero regression to the existing feature, the early-return is positioned correctly, and there's no injection vector (text nodes / element.value, never innerHTML).
I took this into a live Obsidian vault — the part you flagged as not-yet-verified — and that's unfortunately where the property helper runs into trouble. Requesting changes; details are inline, but the headlines:
🔴 Blockers (reproduced live — data loss)
- Number / Date / Datetime properties get wiped. They render as native
<input type="number">/type="date". Slicing[[Note]]into them makes the browser reject the value, soelement.valuebecomes"",insertTextAtCaretreturnstrue(suppressing the body fallback), and Obsidian persists the emptied value on blur. Reproduced:count: 5 → count:(empty),date: 2026-06-14 →(empty). - Caret in the property KEY field corrupts the key name (
count[[Note]]: 5) —.metadata-property-key-inputmatches the same.metadata-propertyselector as the value.
🟠 Majors
- Default text properties are contenteditable
.metadata-input-longtext, not<input>. So the branch the tests cover (<input type="text">) isn't the one real text properties use, and the contenteditable branch is untested. The good news: that branch does persist for longtext — but only on blur, and the helper never blurs (so it can sit uncommitted / be discarded on re-render). - List / tags (
.multi-select-input) silently swallow the text — they commit pills on Enter, not on a syntheticinputevent — so it's a no-op that still returnstrueand discards the link. - Focus timing: append-link runs after the choice. Any choice with a prompt/suggester/modal (or launched from the command palette) moves focus off the property before insertion → it falls back to the stale body caret → the original #768. The focused target isn't captured before the UI opens. Worth verifying with the built PR across capture-with-prompt / template-with-VALUE-suggester / command-palette / split panes.
textContentfallback flattens an already-linked property's child nodes and drops the existing link.
Suggested direction
The brittle part is using Obsidian's private Properties DOM to both detect the caret and persist the value. Consider (1) capturing the focused property target before the choice opens any UI, and (2) persisting via app.fileManager.processFrontMatter(...) keyed to the resolved property + the target TFile, using the DOM only to read which property/caret. That sidesteps the typed-input wipe, the blur dependency, pop-out windows, and list/tags in one move.
At minimum the two blockers need fixing, plus a live-vault e2e (text, longtext, number, date, list/tags, key field) before merge. Happy to review a follow-up — the core idea is right and worth getting in. 🙏
(Reviewed at head dda12e5b. Findings verified by driving a real Obsidian dev vault, not just jsdom.)
| const endOffset = element.selectionEnd ?? element.value.length; | ||
| const before = element.value.slice(0, startOffset); | ||
| const after = element.value.slice(endOffset); | ||
| element.value = before + text + after; |
There was a problem hiding this comment.
Blocker (data loss): this <input> branch wipes existing data for non-text inputs. Obsidian renders Number/Date/Datetime properties as native typed inputs (<input type="number">, type="date", type="datetime-local"). element.value = before + text + after makes the browser reject the non-numeric string, so element.value becomes "" and Obsidian persists null on blur — the original value is destroyed (reproduced live: count: 5 → count: empty, date: 2026-06-14 → empty). Because insertTextAtCaret then returns true, the body fallback is suppressed too. Please restrict this branch to text-like inputs (e.g. only when element.type is text/search/empty) and return false otherwise so it falls back to the body editor.
| if (typeof document === "undefined") return false; | ||
| const focused = document.activeElement; | ||
| if (!(focused instanceof HTMLElement)) return false; | ||
| if (!focused.closest(PROPERTY_WIDGET_SELECTOR)) return false; |
There was a problem hiding this comment.
Blocker (key corruption): a .metadata-property row contains both .metadata-property-key-input (the property NAME) and the value editor, and both are editable elements inside .metadata-property. If the caret is in the key field (e.g. mid-naming a new property), the link is injected into the property name and persisted as a corrupted key on blur (count[[Note]]: 5). Please scope detection to the value side — require focused.closest(".metadata-property-value") and/or bail when focused.closest(".metadata-property-key").
Minor on PROPERTY_WIDGET_SELECTOR (line 563): the real container class is .metadata-properties (not .metadata-properties-container), so that half of the selector never matches — detection currently survives only via .metadata-property.
| return true; | ||
| } | ||
|
|
||
| if (element.isContentEditable) { |
There was a problem hiding this comment.
Two real cases land in this contenteditable branch:
- Default text properties render as a contenteditable
.metadata-input-longtextdiv (not the<input>the tests mount), so this is the real text path. It does persist to frontmatter — but only on blur, andinsertTextAtCaretnever blurs, so the change can sit uncommitted and be discarded if Obsidian re-renders the widget first. - List / tags render as
.multi-select-input(also contenteditable) and commit pills on Enter, not on a syntheticinputevent. So this inserts the text, firesinput, returnstrue(suppressing the body fallback), but the text never reaches frontmatter and is discarded on blur (reproduced live: tags stayed[a, b]).
Please detect .multi-select-container/.multi-select-input and return false (so the body fallback runs) rather than reporting false success; and consider explicitly committing (blur) after a longtext insert.
| selection.removeAllRanges(); | ||
| selection.addRange(range); | ||
| } else { | ||
| element.textContent = `${element.textContent ?? ""}${text}`; |
There was a problem hiding this comment.
Setting element.textContent = existing + text on a contenteditable removes ALL child nodes (replacing them with one flat text node) and appends at the end, ignoring the caret. For an already-linked longtext property (rendered as .metadata-link child nodes whose textContent is only the display text) this silently drops the original link. This branch is reachable when focus is restored after a modal closes without an in-element selection. Please return false here so the caller falls back to the body editor instead of doing a destructive append.
| */ | ||
| function tryInsertIntoFocusedProperty(text: string): boolean { | ||
| if (typeof document === "undefined") return false; | ||
| const focused = document.activeElement; |
There was a problem hiding this comment.
Use the popout-aware document, not the global one. This codebase's convention is getOwnerDocument/activeDocument (see activeWindow.ts, suggest.ts, fileSuggester.ts). With global document.activeElement, a property focused in a popped-out window is never detected and #768 silently falls back to body insertion in popouts. The active view is in scope at the insertLinkWithPlacement call site, so you can thread view.containerEl.ownerDocument.activeElement in.
Also cross-realm: a popout input is not instanceof the main realm's HTMLInputElement (see isEditableElement, lines 566-572), so even after fixing the document it would be rejected — prefer duck-typing (element.matches("input,textarea") / "selectionStart" in element).
| // #768: a focused frontmatter property field leaves the body editor's caret | ||
| // stale; insert into the property at its caret instead. Placement modes only | ||
| // apply to body insertion. | ||
| if (tryInsertIntoFocusedProperty(text)) return; |
There was a problem hiding this comment.
Two concerns at this routing point:
- Focus timing:
insertFileLinkToActiveViewruns after the Template/Capture choice. Any flow that opens a prompt/suggester/modal — or is launched from the command palette — can move focus off the property before this runs, sotryInsertIntoFocusedPropertyreturns false and we fall back to the stale body caret (the original [BUG] Append link to current file in properties #768). Capturing the focused property target before QuickAdd opens UI (and threading it through) would make this robust. - Wrong-file write: there's no check that the focused property belongs to the active view. With split panes / multiple MarkdownViews,
document.activeElementcould be a property in a different leaf thangetActiveViewOfType(MarkdownView)returns, so the link is written into the wrong note's frontmatter. Consider assertingview.containerEl.contains(focused)before inserting.
Both need a live-vault check with the built PR (capture-with-prompt, template-with-VALUE-suggester, command-palette, split panes).
| } | ||
|
|
||
| /** Mounts an Obsidian-like Properties widget with one focused text property. */ | ||
| function mountFocusedTextProperty(initialValue = ""): HTMLInputElement { |
There was a problem hiding this comment.
This suite only mounts <input type="text">, which is the one shape that trivially persists and isn't how a default text property renders (default text = contenteditable .metadata-input-longtext). So the contenteditable Range branch, the textContent fallback, the typed-input wipe (number/date), and the key-field case are all untested, and the 7 green tests give false confidence for the most common real property shapes. Note: jsdom doesn't implement isContentEditable, so a contenteditable test must force it (e.g. Object.defineProperty(el, "isContentEditable", { value: true })). Also, the input-event assertion only proves a locally-attached listener fired — it can't prove Obsidian persists — so it's worth a real dev-vault e2e gating merge rather than relying on these unit tests.
|
Marking this draft — the fix is verified non-functional in real Obsidian. Posting the verification so the approach can be corrected rather than silently shipped. Verified against a live Obsidian 1.9.14 instance, inspected over its remote-debugging (CDP) port in an isolated throwaway vault. Mechanism — confirmed ✅With the caret in a frontmatter property, Why this PR's remedy does not work ❌The real property value editor is a
Correct direction
Open design questions before reworking
Happy to rework around AI-assisted verification (Claude). DOM facts gathered via CDP inspection of a live vault, not inferred. |
…cessFrontMatter Addresses @chhoumann's review. The DOM-mutation approach was non-functional (synthetic input events are discarded) and unsafe (wiped number/date inputs, corrupted the key field). Replaced with the reviewer's suggested architecture: - ChoiceExecutor.execute() captures the focused frontmatter property (file + key) BEFORE any prompt/suggester opens, fixing the focus-timing race where the caret had already left the property by insertion time. - getFocusedPropertyTarget(): popout-aware (per-view ownerDocument), scoped to .metadata-property-value (never the key field), binds to the owning view's file (split-pane safe), and skips typed number/date/checkbox inputs. - appendLinkToFrontmatterProperty(): persists via app.fileManager .processFrontMatter — pushes to list properties, appends to scalars, sets empty ones. Never wipes (the previous data-loss blockers). - insertLinkWithPlacement() restored to its original body-only behavior; the brittle DOM-insertion helpers are removed. Engines route to the property when one was captured, else body insertion. Verified live in a real Obsidian vault (CDP): the built plugin loads, and the full command flow appends the link into the focused text property (text_prop: hello [[Captured]]) instead of the body, with sibling properties untouched. Unit tests cover detection (value/key/typed/none/split) and the processFrontMatter append (list/scalar/empty/number/embed). Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the incredibly thorough live review, @chhoumann — you found the data-loss cases and the focus-timing race I'd missed, and the direction you laid out was right. I've reworked it along exactly those lines. What changedThe brittle "DOM detect + DOM mutate" approach is gone. Now:
Point-by-point
Live verification (real Obsidian vault, via CDP)Built the plugin, installed it in an isolated vault, and ran the full command flow with the caret in a text property: Also confirmed live: the property value editor is Honest about coverage
Marking ready for another look. 🙏 AI-assisted (Claude). Live checks via CDP against a real dev vault. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utilityObsidian.appendLink-768.test.ts (1)
92-101: ⚡ Quick winExpand typed-input exclusion coverage to all excluded types.
This test currently validates only
type="number", while production filtering also excludes date/datetime/time/month/checkbox. Parameterizing this case will better guard the selector contract.Suggested test refactor
-it("returns null for typed inputs (number/date) so they fall back to body", () => { - const file = makeFile("Note.md"); - const { app } = mountViewWithProperty({ - key: "count", - focus: "value", - file, - valueType: "number", - }); - expect(getFocusedPropertyTarget(app)).toBeNull(); -}); +it.each(["number", "date", "datetime-local", "time", "month", "checkbox"])( + "returns null for typed input '%s' so it falls back to body", + (valueType) => { + const file = makeFile("Note.md"); + const { app } = mountViewWithProperty({ + key: "count", + focus: "value", + file, + valueType, + }); + expect(getFocusedPropertyTarget(app)).toBeNull(); + }, +);🤖 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.appendLink-768.test.ts` around lines 92 - 101, The test case `"returns null for typed inputs (number/date) so they fall back to body"` currently only validates the "number" valueType, but the production code also excludes date, datetime, time, month, and checkbox types. Parameterize this test using a test iteration pattern to cover all excluded types (number, date, datetime, time, month, checkbox) instead of testing only one type. This ensures the getFocusedPropertyTarget selector contract is properly guarded for all the types that should return null and fall back to the body.
🤖 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/utilityObsidian.appendLink-768.test.ts`:
- Around line 92-101: The test case `"returns null for typed inputs
(number/date) so they fall back to body"` currently only validates the "number"
valueType, but the production code also excludes date, datetime, time, month,
and checkbox types. Parameterize this test using a test iteration pattern to
cover all excluded types (number, date, datetime, time, month, checkbox) instead
of testing only one type. This ensures the getFocusedPropertyTarget selector
contract is properly guarded for all the types that should return null and fall
back to the body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27baf57c-0af9-45b0-9ce4-1f16efce512c
📒 Files selected for processing (6)
src/IChoiceExecutor.tssrc/choiceExecutor.tssrc/engine/CaptureChoiceEngine.tssrc/engine/TemplateChoiceEngine.tssrc/utilityObsidian.appendLink-768.test.tssrc/utilityObsidian.ts
…ed path Follow-up to the chhoumann#768 rework, fixing issues from review: - ChoiceExecutor: replace the latching `focusCaptured` flag with an `executionDepth` counter. The flag never reset, so a reused executor (e.g. quickAddApi.executeChoice) leaked a stale property target into the next top-level call. Now the focused property is captured at depth 0→1 and cleared in `finally` when the chain unwinds — nested Multi/Macro keep the original caret; independent calls each capture fresh. - appendLinkToFrontmatterProperty: drop the embed branch and unused linkOptions param — an embed (![[…]]) in a YAML value isn't rendered, so it was dead/incidental coupling. Property links are always plain links. - getFocusedPropertyTarget: pin the Obsidian-internals comment to the verified version; trim the overstated doc comment. - Tests updated accordingly. Co-Authored-By: Claude <noreply@anthropic.com>
|
Did a self-review pass and closed the live-verification gaps I'd flagged. Two commits on top ( Review fixes
Live verification (real Obsidian vault, built plugin, driven via CDP)Ran the full command flow end-to-end for the paths you asked about:
What I couldn't fully automate (being upfront)
CI is green locally (2004 tests, build, lint, svelte-check). Ready for another look. AI-assisted (Claude). Live checks via CDP against a real dev vault. |
3 participants
Problem
Fixes #768. When Append link to current file runs while the caret is in a
frontmatter property field (Obsidian's Properties widget), the link is
inserted at the first body line (right after the YAML) instead of at the
cursor. It works fine when the caret is in the document body.
Steps to reproduce (on
master)Manual, in a live vault:
Append link to current file.
Live Preview, e.g.:
title) so the caret is insidethe Properties widget — not the note body.
Expected: the link is appended at the caret (in the property).
Actual: the link is inserted on the first body line, right after the
---:Automated (no Obsidian needed) — the new test doubles as a repro:
pnpm install pnpm exec vitest run src/utilityObsidian.appendLink-768.test.tsOn
mastertheinsertLinkWithPlacement (#768 property-field routing)casefails (the body editor's
replaceSelectionis called instead of the focusedproperty); with this PR all 7 cases pass.
Root cause
The whole append-link path reads the cursor from the markdown body editor:
insertFileLinkToActiveView(src/utilityObsidian.ts) →insertLinkWithPlacement, which useseditor.listSelections()/editor.getCursor().In Live Preview the Properties panel is a separate set of input elements, not
the CodeMirror document. When a property is focused, the body editor isn't, so
getCursor()returns a stale body caret (the first line after the frontmatter).There was no check for this case, so the link landed in the body.
Fix
insertLinkWithPlacementnow detects when focus is inside the Properties widget(the focused DOM element under
.metadata-property/.metadata-properties-container) and inserts the link at that element'scaret, dispatching an
inputevent so Obsidian persists it to thefrontmatter. It falls back to the existing body-editor behavior when no property
field is focused or the element type is unsupported. Placement modes
(
replaceSelection, etc.) only describe body insertion, so they don't apply inthe property case.
Two small helpers (
getFocusedEditablePropertyEl,insertTextIntoEditableEl)are added and exported via
__testfor unit coverage.Validation
pnpm exec vitest run src/utilityObsidian.appendLink-768.test.ts→ 7 passed(focus detection, editable-element insertion, and property-vs-body routing).
(
utilityObsidian,linkPlacement,engine/,formatter-linkcurrent) →345 passed, 2 skipped, no regressions.
eslinton the changed files → clean;tsc -noEmit -skipLibCheck→ nosrc/errors.insertFileLinkToActiveViewover a frontmatter document with a jsdomProperties widget: pre-fix the link lands at the first body line; post-fix it
lands in the focused property and the body editor is left untouched.
Notes for reviewers
main.js/styles.cssare gitignored in this repo, so only source is included.insertTextIntoEditableElcovers Obsidian'srich text/list property inputs; the
<input>/<textarea>branch covers thesimpler ones.
Summary by CodeRabbit
Bug Fixes
Tests