feat: add start_line/limit line paging to vault_read_note - #406
Merged
Conversation
Mirror PR #397's vault_read_file paging on note reads — one paging idiom across both read tools. Pages the delivered rendition (full body or a heading section) by line range; JSON modes (outline, properties_only) reject paging with a clear error. Implementation: - Extract `pageTextByLines` + `LineWindow` to obsidian-markdown/lines.ts as a shared primitive (split, validate, slice — no byte cap) - Refactor `buildPagedTextResult` in asset-operations.ts to use it - Move `describeTextWindow` to tool-helpers.ts for shared use - Add start_line/limit schema params + paging logic to vault_read_note - 21 new tests (12 unit for pageTextByLines, 9 integration) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
|
umm-actually re-reviewed at No new findings (3 tracked finding(s) across all runs). Context notes
umm-actually · deepseek/deepseek-v4-pro |
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangesLine-window pagination
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/vault-mcp/mcp-core/tools/vault-crud-tools.ts`:
- Around line 254-260: Remove the `as const` assertions from the text content
objects returned by `vault_read_note` and the `safeHandler` helper. Use
contextual MCP text-block typing or a small typed text-content helper so these
objects satisfy the expected type without any `as` assertions, preserving their
existing text values and return structure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 463899db-47e4-45bb-ac25-02b531b26f70
📒 Files selected for processing (8)
ARCHITECTURE.mdsrc/vault-mcp/mcp-core/__tests__/tool-definitions.test.tssrc/vault-mcp/mcp-core/tools/asset-tools.tssrc/vault-mcp/mcp-core/tools/tool-helpers.tssrc/vault-mcp/mcp-core/tools/vault-crud-tools.tssrc/vault-mcp/obsidian-markdown/__tests__/lines.test.tssrc/vault-mcp/obsidian-markdown/lines.tssrc/vault-mcp/vault-operations/asset-operations.ts
lines.ts now also exports pageTextByLines; tool-helpers.ts now also exports describeTextWindow — update the file descriptions so agents see the current module surfaces. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kum27K9QPrK5TetJuYtuRK
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kum27K9QPrK5TetJuYtuRK
…overage Replace decomposed/partial assertions with whole-object toEqual on paging content blocks and lineWindow shapes; replace toContain on a deterministic error message with exact toBe. Add direct unit tests for the shared describeTextWindow helper (3 branches: zero-lines, end-of-file, mid-file). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kum27K9QPrK5TetJuYtuRK
The type is now defined in obsidian-markdown/lines.ts. asset-operations still imports it (for AssetReadResult), but no longer re-exports it — no external consumer imports LineWindow from this module anymore. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the project-specific "250+ card Done lane" example and broaden the count-probe guidance to cover both full-note and section reads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Map snake_case wire params to camelCase in all tool_call log objects: start_line → startLine, heading_level → headingLevel, properties_only → propertiesOnly, replace_all_occurrences → replaceAllOccurrences, first_match → firstMatch. Matches the convention already used in asset-tools.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
vault_read_filepaging onvault_read_note— one paging idiom across both read toolsheading:section) by 1-based line range; JSON modes (outline,properties_only) reject paging with a clear errorheading: "Done", start_line: 1, limit: 20now reads a window instead of the entire laneImplementation
pageTextByLines+LineWindowtype toobsidian-markdown/lines.tsas a shared primitive (split, validate, slice — no byte cap; byte cap enforcement stays in asset-operations)buildPagedTextResultin asset-operations.ts to delegate topageTextByLinesdescribeTextWindowtotool-helpers.tsfor shared use by both read toolsstart_line/limitschema params + paging validation + two-block result formatting tovault_read_notehandlervault_read_notedescription (examples, errors, returns)Test plan
pageTextByLines(window, clamp, CRLF, trailing newline, empty, past-EOF, invalid range)vault_read_notepaging (full note, heading section, end-of-file, empty note, past-EOF error, outline/properties_only rejection, byte-identical unpaged, description assertions)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation