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 3 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 (2)
📝 WalkthroughWalkthrough
ChangesProperties Widget Link Insertion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
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
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. |
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