feat: keep cursor at capture insertion when opening file#1117
feat: keep cursor at capture insertion when opening file#1117
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements cursor positioning for captured content. It introduces a new cursor mapping module to track insertion boundaries and remap cursor positions across content changes, refactors the capture engine to compute and apply cursor positions, and extends test coverage for the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Engine as CaptureChoiceEngine
participant Formatter as CaptureChoiceFormatter
participant Vault as Vault (File System)
participant Editor as Editor & Leaf
User->>Engine: Trigger capture with choice
Engine->>Engine: prepareCaptureRun()
Engine->>Formatter: Format capture content
Formatter-->>Engine: Return formatted content + insertionMetadata
Engine->>Vault: Read existing file (if applicable)
Vault-->>Engine: Return existing content
Engine->>Engine: getCaptureUpdateResult()
Engine->>Engine: Compute captureCursorPosition via boundary mapping
Engine->>Vault: Modify file with new content
Vault-->>Engine: Confirm modification
Engine->>Engine: applyCaptureUpdate()
Engine->>Editor: openCapturedFileIfNeeded()
Editor-->>Engine: Return leaf/editor reference
Engine->>Editor: setLeafCursorIfPossible(cursorPosition)
Editor-->>Engine: Cursor positioned
Engine-->>User: Capture complete with cursor at insertion point
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
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 |
Deploying quickadd with
|
| Latest commit: |
a2c40e0
|
| Status: | ⚡️ Build in progress... |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/CaptureChoiceEngine.ts (1)
201-230:⚠️ Potential issue | 🟡 MinorPotential timing issue between
vault.modifyandsetCursoron a reused tab.When an existing tab is reused (line 204), the file was just modified via
vault.modify(line 182). The editor in the existing tab picks up the change asynchronously through Obsidian's event system. If the editor hasn't fully processed the new content whensetCursorfires, the cursor position could land on a stale line count, causing an incorrect placement or a silent failure caught by the try/catch.The try/catch on lines 220–226 provides a safety net, so this won't crash. But if users report intermittent cursor misplacement on reused tabs, consider adding a small delay or awaiting a vault/workspace event before calling
setCursor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/CaptureChoiceEngine.ts` around lines 201 - 230, The reused-tab path (openExistingFileTab -> openedLeaf) can race with the earlier vault.modify, so before calling editor.setCursor (when captureCursorPosition is set and editor exists) wait for the editor to apply the vault changes; implement this by detecting when openedLeaf was reused and then either awaiting a short tick (e.g., await new Promise(r => requestAnimationFrame(r)) or a small setTimeout like 50ms) or subscribing to the workspace/editor update event before calling editor.setCursor, keeping the try/catch around setCursor and still calling jumpToNextTemplaterCursorIfPossible afterward.
🧹 Nitpick comments (3)
src/engine/CaptureChoiceEngine.ts (1)
210-218: Verbose but safe type narrowing for the editor.The inline type assertion to extract
editor?.setCursorfrom the leaf view is pragmatic given the Obsidian API's broadViewtype. If this pattern appears elsewhere, extracting a small helper (e.g.,getEditorFromLeaf(leaf)) could reduce duplication, but it's fine as-is for a single use site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/CaptureChoiceEngine.ts` around lines 210 - 218, The current inline type narrowing for extracting editor?.setCursor from openedLeaf?.view is verbose; extract this logic into a small helper like getEditorFromLeaf(leaf) that safely narrows the view and returns the editor or undefined, then replace the inline assertion at the openedLeaf?.view usage with a call to getEditorFromLeaf(openedLeaf) to reduce duplication and improve readability while preserving the same safety around optional chaining and setCursor.src/engine/captureCursor.test.ts (1)
4-28: Tests are correct and cover the primary scenarios well.I verified each expected result against the algorithm. Consider adding a few more edge-case tests for robustness:
- CRLF content – ensures
toLineAndChhandles\r\ncorrectly.- Only-newline insertion (e.g.,
"A"→"A\n") – exercises the fallback toprefixLength.- Empty initial content (e.g.,
""→"Captured") – covers the "new file" case.These aren't blocking but would strengthen confidence in the diff logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/captureCursor.test.ts` around lines 4 - 28, Add unit tests for getCaptureCursorPosition to cover three edge cases: 1) CRLF content — pass before/after strings using "\r\n" line endings to ensure any internal toLineAndCh handling of CRLF is correct; 2) only-newline insertion — test a case like "A" -> "A\n" to exercise the fallback to prefixLength and confirm returned {line,ch} matches expected; 3) empty initial content — test "" -> "Captured" (new file) to ensure getCaptureCursorPosition returns the correct insertion position. Use the existing test structure and assertions to mirror the style of current cases.src/engine/captureCursor.ts (1)
28-43: Consider the edge case wherenextContentis a strict subset ofpreviousContent(content was removed instead of added).If the capture formatter unexpectedly removes content (e.g., a merge or format strips text), the suffix-matching loop can produce
insertionEnd <= prefixLength, correctly returningnull. However, there's a subtle case when content is replaced (some old text removed, some new text added). The function will position the cursor at the start of the replacement region innextContent, which is reasonable but may not always correspond to a "capture insertion". For the current usage (capture always adds content), this is fine—just noting it for future maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/captureCursor.ts` around lines 28 - 43, The suffix-matching logic can misinterpret removals/replacements as insertions; add an explicit guard at the start of the routine (the function that uses previousContent, nextContent, prefixLength, previousIndex/nextIndex) to bail out when nextContent is shorter than previousContent (or when a removal is detected) — e.g., if nextContent.length < previousContent.length return null — so the function only proceeds for pure-addition cases and avoids positioning the cursor into a replacement region; keep the existing suffix loop and insertionEnd check but add this preliminary length/removal check to make intent explicit for future maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/engine/CaptureChoiceEngine.ts`:
- Around line 201-230: The reused-tab path (openExistingFileTab -> openedLeaf)
can race with the earlier vault.modify, so before calling editor.setCursor (when
captureCursorPosition is set and editor exists) wait for the editor to apply the
vault changes; implement this by detecting when openedLeaf was reused and then
either awaiting a short tick (e.g., await new Promise(r =>
requestAnimationFrame(r)) or a small setTimeout like 50ms) or subscribing to the
workspace/editor update event before calling editor.setCursor, keeping the
try/catch around setCursor and still calling jumpToNextTemplaterCursorIfPossible
afterward.
---
Nitpick comments:
In `@src/engine/CaptureChoiceEngine.ts`:
- Around line 210-218: The current inline type narrowing for extracting
editor?.setCursor from openedLeaf?.view is verbose; extract this logic into a
small helper like getEditorFromLeaf(leaf) that safely narrows the view and
returns the editor or undefined, then replace the inline assertion at the
openedLeaf?.view usage with a call to getEditorFromLeaf(openedLeaf) to reduce
duplication and improve readability while preserving the same safety around
optional chaining and setCursor.
In `@src/engine/captureCursor.test.ts`:
- Around line 4-28: Add unit tests for getCaptureCursorPosition to cover three
edge cases: 1) CRLF content — pass before/after strings using "\r\n" line
endings to ensure any internal toLineAndCh handling of CRLF is correct; 2)
only-newline insertion — test a case like "A" -> "A\n" to exercise the fallback
to prefixLength and confirm returned {line,ch} matches expected; 3) empty
initial content — test "" -> "Captured" (new file) to ensure
getCaptureCursorPosition returns the correct insertion position. Use the
existing test structure and assertions to mirror the style of current cases.
In `@src/engine/captureCursor.ts`:
- Around line 28-43: The suffix-matching logic can misinterpret
removals/replacements as insertions; add an explicit guard at the start of the
routine (the function that uses previousContent, nextContent, prefixLength,
previousIndex/nextIndex) to bail out when nextContent is shorter than
previousContent (or when a removal is detected) — e.g., if nextContent.length <
previousContent.length return null — so the function only proceeds for
pure-addition cases and avoids positioning the cursor into a replacement region;
keep the existing suffix loop and insertionEnd check but add this preliminary
length/removal check to make intent explicit for future maintainability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/captureCursor.test.tssrc/engine/captureCursor.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/engine/CaptureChoiceEngine.ts (1)
225-253: Consider extracting a named type or using Obsidian'sMarkdownViewfor safer editor access.The inline type assertion at lines 234–242 works but is verbose and fragile — it will silently produce
undefinedif the view isn't a markdown view. Since Obsidian exposesMarkdownViewwith aneditorproperty, you could import it and use aninstanceofcheck for slightly more robust typing:import { MarkdownView } from "obsidian"; // ... const editor = openedLeaf?.view instanceof MarkdownView ? openedLeaf.view.editor : undefined;This is optional since the current approach is also safe (the
typeof editor?.setCursor === "function"guard on line 243 ensures no crash).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/CaptureChoiceEngine.ts` around lines 225 - 253, Replace the fragile inline union type assertion used to get the editor in CaptureChoiceEngine with an Obsidian MarkdownView instanceof check: import MarkdownView from "obsidian", then set editor by testing openedLeaf?.view instanceof MarkdownView ? openedLeaf.view.editor : undefined (preserving the existing captureCursorPosition and typeof editor?.setCursor guard and the surrounding try/catch). This change should be applied where editor is derived (currently around the openExistingFileTab/openFile handling) and leaves jumpToNextTemplaterCursorIfPossible and other logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/engine/CaptureChoiceEngine.ts`:
- Around line 225-253: Replace the fragile inline union type assertion used to
get the editor in CaptureChoiceEngine with an Obsidian MarkdownView instanceof
check: import MarkdownView from "obsidian", then set editor by testing
openedLeaf?.view instanceof MarkdownView ? openedLeaf.view.editor : undefined
(preserving the existing captureCursorPosition and typeof editor?.setCursor
guard and the surrounding try/catch). This change should be applied where editor
is derived (currently around the openExistingFileTab/openFile handling) and
leaves jumpToNextTemplaterCursorIfPossible and other logic unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/captureCursor.test.tssrc/engine/captureCursor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/captureCursor.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/formatters/captureChoiceFormatter.ts (1)
17-20: DuplicateCaptureCursorPositiontype and helper functions withcaptureCursor.ts.
CaptureCursorPositionis exported from both this file andsrc/engine/captureCursor.tswith identical definitions. Similarly,getCursorOffset(line 666) andtoLineAndCh(line 689) duplicate the same-named helpers incaptureCursor.ts(lines 66–89 and 166–185). Consider importing fromcaptureCursor.tsto keep a single source of truth.♻️ Suggested approach
Re-export the type from
captureCursor.tsand extract the shared helpers there:-export type CaptureCursorPosition = { - line: number; - ch: number; -}; +export type { CaptureCursorPosition } from "../engine/captureCursor";Then import and delegate to the shared
getCursorOffset/toLineAndChfromcaptureCursor.tsinstead of maintaining private copies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/formatters/captureChoiceFormatter.ts` around lines 17 - 20, Duplicate type and helper implementations exist: CaptureCursorPosition, getCursorOffset, and toLineAndCh; remove the duplicates in captureChoiceFormatter and import/re-export the single source from the existing captureCursor module instead. Replace the local CaptureCursorPosition definition by importing and re-exporting the type from captureCursor (so other modules still get the type), and delete the local getCursorOffset and toLineAndCh implementations in captureChoiceFormatter.ts and call/import the shared getCursorOffset and toLineAndCh from captureCursor; update any local calls to delegate to those imported helpers so behavior stays identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/formatters/captureChoiceFormatter.ts`:
- Around line 17-20: Duplicate type and helper implementations exist:
CaptureCursorPosition, getCursorOffset, and toLineAndCh; remove the duplicates
in captureChoiceFormatter and import/re-export the single source from the
existing captureCursor module instead. Replace the local CaptureCursorPosition
definition by importing and re-exporting the type from captureCursor (so other
modules still get the type), and delete the local getCursorOffset and
toLineAndCh implementations in captureChoiceFormatter.ts and call/import the
shared getCursorOffset and toLineAndCh from captureCursor; update any local
calls to delegate to those imported helpers so behavior stays identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/engine/CaptureChoiceEngine.notice.test.tssrc/engine/CaptureChoiceEngine.selection.test.tssrc/engine/CaptureChoiceEngine.template-property-types.test.tssrc/engine/CaptureChoiceEngine.tssrc/engine/captureCursor.test.tssrc/engine/captureCursor.tssrc/formatters/captureChoiceFormatter.tssrc/utilityObsidian.ts
chhoumann
force-pushed
the
348-feature-request-keep-cursor-at-position-of-capture-1
branch
from
e1edc2f to
7e25a8e
Compare
chhoumann
deleted the
348-feature-request-keep-cursor-at-position-of-capture-1
branch
1 participant
Summary
captureCursor.ts)Verification
bun run testbun run build-with-lintobsidian vault=dev plugin:reload id=quickadd)quickadd:choice:ece95fc5-48b3-49fc-b758-8c9a16b29401line 24, ch 0) instead of top-of-fileNotes
currentLine/newLineAbove/newLineBelow) are unchanged.Closes #348
Summary by CodeRabbit
New Features
Refactor
Tests