feat: extend file content search to PDF and text files - #425
Conversation
pdf-engine.ts is PDF domain logic (pdfjs build swaps, font proxy wiring, CMap path resolution) — it fails the utils/ admission bar of "generic with zero domain knowledge." Move it to obsidian-markdown/ where its dependency profile (node:module, node:path, unpdf, @napi-rs/canvas) is compatible with the leaf-layer lint rules. Create obsidian-markdown/pdf.ts with extractPdfText(Uint8Array → string) — manages the pdfjs proxy lifecycle internally so callers pass bytes, get text. Moves reconstructPdfMarkdown, groupIntoLines, buildHeadingLevels, roundFontSize from asset-operations.ts (~120 lines). asset-operations.ts's non-raw PDF branch now calls extractPdfText directly; the raw/page-rendering branch keeps its own proxy for renderPdfPages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the .canvas-only guard in upsertFileContent and generalize to all INDEXABLE_TEXT_EXTENSIONS (.pdf, .txt, .csv, .json, .xml, .svg, .log, .yaml, .yml, .base). Canvas files still get linearization + link extraction; other types are indexed as pre-rendered text (PDF via extractPdfText, text files as raw UTF-8). File watcher: extended to read and index all indexable file types on change events. PDF extraction is async with error isolation per file. Delete handler extended to clean up content for all indexable types. Rebuild pipeline: scans canvas, PDF, and text files separately. PDF extraction runs with bounded concurrency (mapWithConcurrency, 4 slots). All file content inserted in the transaction alongside canvas. Content cap: MAX_INDEXED_CONTENT_BYTES (100KB) truncates large files before FTS insertion to prevent pathological rows. Tests: updated non-canvas test to verify FTS indexing + no link extraction. Added plain text file indexing test and content cap test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- vault_search description: expand kind/extension examples to include PDF and text file types in both hybrid and non-hybrid variants - ARCHITECTURE.md: update pdf-engine.ts path (now obsidian-markdown/), replace "not yet searchable" with the full indexed file type list - AGENTS.md: move pdf-engine.ts from utils/ to obsidian-markdown/ in the structure tree, add pdf.ts entry - New pdf.test.ts: extractPdfText integration test with real PDF fixture + empty-stream PDF edge case 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 (20 tracked finding(s) across all runs). Context notes
umm-actually · deepseek/deepseek-v4-pro |
…rch gap
- .devin/wiki.json: update utils/pdf-engine.ts → obsidian-markdown/pdf-engine.ts
and add pdf.ts + file_content tables to the Non-Markdown Files page purpose
- search-index.ts: compute visibleFilesOfKind("file") once in rebuild instead
of calling it twice (stat pass + content indexing pass)
- README.md: mention PDF and text file full-text searchability in the Files
section; add YAML to the text formats list
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
INDEXABLE_TEXT_EXTENSIONS includes .yaml/.yml (searchable via vault_search), but TEXT_PASSTHROUGH_EXTENSIONS did not (not readable via vault_read_file). A user finding a YAML file in search results couldn't follow up with a read — broken UX. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Missing test coverage for file-watcher PDF and text file indexing
The file watcher's Failure scenario: A future refactor drops the Suggested fixAdd file-watcher integration tests for: (1) a `.txt` file is indexed via `upsertFileContent` and searchable, (2) deleting an indexable text file calls `removeFileContent`, (3) a `.pdf` file channels through `extractPdfText` (mock `extractPdfText` to verify the call). |
…t assertion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… indexing 3 test quality fixes: exact `toBe` on deterministic extractPdfText output (was 3x toContain), consolidated decomposed assertions on search result (was toBeDefined + .kind + .extension), and truncation test now proves content was actually truncated via start/end phrase strategy (mutation-verified). 3 coverage gaps filled: INDEXABLE_TEXT_EXTENSIONS drift guard (mutation-verified), removeFileContent for non-canvas files, and the truncation test's two-bar fix doubles as a coverage improvement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Text formats entry listed .svg/.json/.txt/.csv/.xml/.log/.base but omitted .yaml and .yml, which are supported by both TEXT_PASSTHROUGH_EXTENSIONS (asset-operations.ts) and INDEXABLE_TEXT_EXTENSIONS (search-index.ts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
vault_read_file tool description omits .yaml/.yml from readable text formats list
The Failure scenario: An agent calls vault_list_files, sees some .yaml files, checks vault_read_file's description for supported types, sees .yaml not listed, and incorrectly concludes YAML files are unsupported — avoiding reading them or using a workaround. Suggested fixUpdate the vault_read_file tool description's text formats list to include `.yaml` and `.yml`: `(.svg/.json/.txt/.csv/.xml/.log/.yaml/.yml/.base)` |
|
Pre-existing: DOCKERHUB.md Files section is stale — needs regeneration after README.md update
DOCKERHUB.md's 'Files' section (auto-generated from README.md) contains a stale version of the text formats list. The README.md was updated to include YAML and YML extensions and to mention FTS searchability for text files, but DOCKERHUB.md still references the older 'SVG, JSON, CSV, logs, and Bases files' wording without Failure scenario: Pre-existing: A user reading DOCKERHUB.md sees 'Text and data files — SVG, JSON, CSV, logs, and Bases files return exactly as written' without the YAML/YML extensions or the FTS searchability claim that README.md now states. The generated content is stale until the next regeneration. |
…-8 truncation
- extractPdfText now returns { text, totalPages } so the scanned-PDF
error message in asset-operations.ts can include page count again
(restored parity with the raw branch's error format)
- File-watcher integration tests: text file indexing + delete cleanup
(coverage gap flagged by umm-actually)
- UTF-8 truncation: character-level string.slice instead of byte-level
Buffer.subarray to avoid splitting multi-byte characters at the cap
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressing non-thread bot finding: file-watcher coverage gap The umm-actually PR-level comment flagged missing file-watcher tests for PDF and text file indexing paths. This was also raised as an inline thread. Fixed — two file-watcher integration tests added:
🔍 ship-check · pr-monitor · claude-opus-4-6 |
|
Addressing bot findings from the latest review round:
🔍 ship-check · pr-monitor · claude-opus-4-6 |
…RHUB.md - vault_read_file tool description: text formats list now includes .yaml/.yml (matches TEXT_PASSTHROUGH_EXTENSIONS) - README: expanded text formats list to include TXT and XML - DOCKERHUB.md: regenerated to reflect README changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Writes a real buildMinimalPdf() fixture to the temp vault, starts the watcher, and asserts extracted text appears in hybridSearch with kind: "file" and extension: ".pdf". Exercises the binary readFile → Uint8Array → extractPdfText path that the .txt tests don't cover. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR adds shared Node PDF processing, reconstructs PDF text as Markdown, extends asset support to YAML, and indexes PDFs and supported text files in file-content search. Watchers, rebuilds, search descriptions, tests, and documentation now reflect these formats. ChangesNon-Markdown processing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vault-mcp/search/search-index.ts (1)
1615-1728: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftSkip PDF and text extraction when file-content indexing is disabled.
When
fileToolsEnabledis false,file_contentandfile_content_ftsdo not exist. These paths still read every supported text file and run PDF extraction. This adds rebuild and watcher I/O, CPU use, and warning logs for content that cannot be searched. Keep Canvas link extraction active.
src/vault-mcp/search/search-index.ts#L1615-L1728: Build PDF and text file lists, then read and extract them, only whenfileToolsEnabledis true.src/vault-mcp/search/file-watcher.ts#L88-L118: Pass the file-content capability intostartFileWatcher, then skip non-Canvas content reads when it is false.#!/bin/bash set -euo pipefail rg -n -C 4 --type=ts '\bstartFileWatcher\s*\(' src rg -n -C 4 --type=ts '\bfileToolsEnabled\b' src/vault-mcp🤖 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/vault-mcp/search/search-index.ts` around lines 1615 - 1728, In src/vault-mcp/search/search-index.ts lines 1615-1728, keep Canvas reading and link extraction unconditional, but only build PDF/text lists and perform PDF extraction or text reads when fileToolsEnabled is true. In src/vault-mcp/search/file-watcher.ts lines 88-118, pass the file-content capability into startFileWatcher and skip non-Canvas content reads when disabled; preserve Canvas processing in all cases.
🧹 Nitpick comments (6)
src/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts (3)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
buildEmptyStreamPdfbefore its call site.Line 20 calls
buildEmptyStreamPdf, which is aconstarrow function initialized at line 34. This works today because Vitest defers theitcallback until after the module body evaluates. It breaks with a TDZReferenceErrorif the call ever moves into thedescribebody or into collection-phase code.Move the helper above the
describeblock.🤖 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/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts` at line 20, Move the buildEmptyStreamPdf helper declaration above the describe block so it is initialized before any call site, while preserving its existing behavior and implementation.
5-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the Markdown reconstruction rules.
Both tests exercise only the trivial path.
reconstructPdfMarkdowninpdf.tsimplements four non-trivial rules that no test asserts:
- Heading levels from relative font sizes (
buildHeadingLevels, H1–H3 cap, smallest size stays body text).- Fenced code blocks from
fontFamily === "monospace", including the fence closed at page end.--- Page N ---separators for multi-page documents.- The deduplicated links footer and the
Links: Nheader field.A regression in any of these rules passes the current suite. Add fixtures with two font sizes, a monospace run, and two pages.
Do you want me to generate these fixtures and tests?
🤖 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/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts` around lines 5 - 30, Extend the extractPdfText tests around reconstructPdfMarkdown with fixtures covering two relative font sizes, a monospace run, and a two-page document. Assert H1–H3 heading assignment with the smallest font remaining body text, fenced code closure at page end, page separators, and the deduplicated links footer plus Links: N header field.
34-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPDF test fixtures have no shared, layer-neutral home. The shared root cause is that
buildMinimalPdflives undermcp-core/__tests__/, so leaf-layer tests reach upward into the protocol layer to use it, and a second fixture was hand-copied rather than derived from it. One shared fixture module fixes both sites.
src/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts#L34-L86:buildEmptyStreamPdfduplicates almost all ofbuildMinimalPdf— the header, the catalog, pages, page, and stream objects, the xref table, and the trailer. Only the content stream and the/Resourcesentry differ. Parameterize a single builder over the content stream and the font resource, then express both fixtures through it.src/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.ts#L8-L8: this import points at../../mcp-core/__tests__/pdf-fixture.js. Re-point it at the shared fixture module once the builder moves to a neutral location, such as a test-support folder that no production layer owns.🤖 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/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts` around lines 34 - 86, Move buildMinimalPdf into a shared, layer-neutral test-support fixture module and parameterize it by content stream and font resource so buildEmptyStreamPdf in src/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts uses the shared builder instead of duplicating PDF structure; preserve the differing /Resources and stream behavior. Update the import in src/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.ts#L8 to reference the new shared module rather than the mcp-core fixture, with no direct change needed beyond that import.src/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.ts (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fixture import path resolves correctly.
buildMinimalPdfis defined atsrc/vault-mcp/mcp-core/__tests__/pdf-fixture.ts. The relative specifier and the.jsextension are correct.One placement note: this leaf-layer test now depends on a fixture inside
mcp-core/__tests__/.pdf.test.tsdoes the same. See the consolidated comment on fixture placement.🤖 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/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.ts` at line 8, Keep the buildMinimalPdf import unchanged because its relative path and .js extension resolve correctly; no code change is required for this comment.src/vault-mcp/obsidian-markdown/pdf-engine.ts (1)
62-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
pdf-engine.tsbreaks theobsidian-markdown/folder contract. The shared root cause is one placement decision: a module that resolves package paths from disk and mutatesglobalThisnow lives in a folder that both the coding guidelines andAGENTS.mddefine as pure, filesystem-free parsers. Resolve the contradiction in one direction — either move the module or amend the stated contract.
src/vault-mcp/obsidian-markdown/pdf-engine.ts#L62-L89:resolvePdfjsAssetPathsreads thepdfjs-distpackage root throughcreateRequire(...).resolve, andinitializePdfEngineinstalls process-wideDOMMatrix,Path2D, andImageDataglobals. If the module stays here, confirm the ESLint layering rule permits it; otherwise move it toutils/under the "complete, standalone primitive" bar.AGENTS.md#L129-L129: this line listspdf-engine.tsunderobsidian-markdown/, while lines 180-182 of the same file state the folder holds "pure parsers/transforms" with "No fs, no SQLite, no MCP". If the placement stands, add an explicit carve-out for the PDF bootstrap in that paragraph, matching how the paragraph already carves outcanvas.tsandlinks.ts.As per coding guidelines: "Keep Obsidian/Markdown parsers pure: no filesystem, SQLite, or MCP dependencies."
🤖 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/vault-mcp/obsidian-markdown/pdf-engine.ts` around lines 62 - 89, Resolve the placement/contract mismatch for pdf-engine.ts: either move resolvePdfjsAssetPaths and initializePdfEngine to utils/ as a standalone primitive and update all references, or retain the module under obsidian-markdown only after confirming the layering rule permits its filesystem resolution and globalThis mutations. If retaining it, update AGENTS.md at the listed entry and pure-parser contract to explicitly carve out the PDF bootstrap alongside canvas.ts and links.ts; otherwise state that the AGENTS.md site requires no direct change beyond updating the file listing.Source: Coding guidelines
AGENTS.md (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
pdf-engine.tsentry conflicts with the folder contract stated later in this file.Line 129 places
pdf-engine.tsinobsidian-markdown/. Lines 180-182 of this same file define that folder as "pure parsers/transforms" with "No fs, no SQLite, no MCP".pdf-engine.tsresolves thepdfjs-distpackage root from disk and mutatesglobalThis.If the placement is intended, amend the
obsidian-markdown/layering paragraph to carve out the PDF bootstrap, the way the paragraph already carves outcanvas.tsfor JSON andlinks.tsfor namespace exports. See the consolidated comment.🤖 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 `@AGENTS.md` at line 129, Update the obsidian-markdown layering paragraph to explicitly carve out pdf-engine.ts as an allowed PDF bootstrap exception, alongside the existing canvas.ts and links.ts exceptions. Document that it may resolve pdfjs-dist from disk and mutate globalThis, while preserving the folder’s pure-parser/transform contract for all other modules.
🤖 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 `@AGENTS.md`:
- Line 130: Update the pdf.ts entry in AGENTS.md to document extractPdfText as
returning PdfTextResult, including its text and totalPages fields, instead of
describing a direct Uint8Array-to-string conversion.
In `@DOCKERHUB.md`:
- Line 69: Update the corresponding text/data-file description in README.md to
state that full-text indexing/search is limited to the first 100KB, while
preserving the separate claim that large files and logs can be read by line
range; then regenerate DOCKERHUB.md so it reflects the README change.
In `@src/vault-mcp/mcp-core/tools/asset-tools.ts`:
- Line 110: Update the search-coverage statement in the asset-tools description
to reflect that vault_search indexes PDFs and all supported text formats, while
retaining the existing markdown-note coverage and file-browsing/note-link
guidance where applicable.
In `@src/vault-mcp/search/search-index.ts`:
- Around line 989-995: Update the truncation logic near contentBytes and
truncatedContent to enforce MAX_INDEXED_CONTENT_BYTES by iterating Unicode code
points and accumulating each code point’s UTF-8 byte length, stopping before the
cap is exceeded. Preserve the full content when already within the limit, avoid
splitting surrogate pairs, and add a boundary test using non-ASCII characters.
---
Outside diff comments:
In `@src/vault-mcp/search/search-index.ts`:
- Around line 1615-1728: In src/vault-mcp/search/search-index.ts lines
1615-1728, keep Canvas reading and link extraction unconditional, but only build
PDF/text lists and perform PDF extraction or text reads when fileToolsEnabled is
true. In src/vault-mcp/search/file-watcher.ts lines 88-118, pass the
file-content capability into startFileWatcher and skip non-Canvas content reads
when disabled; preserve Canvas processing in all cases.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 129: Update the obsidian-markdown layering paragraph to explicitly carve
out pdf-engine.ts as an allowed PDF bootstrap exception, alongside the existing
canvas.ts and links.ts exceptions. Document that it may resolve pdfjs-dist from
disk and mutate globalThis, while preserving the folder’s pure-parser/transform
contract for all other modules.
In `@src/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.ts`:
- Line 8: Keep the buildMinimalPdf import unchanged because its relative path
and .js extension resolve correctly; no code change is required for this
comment.
In `@src/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts`:
- Line 20: Move the buildEmptyStreamPdf helper declaration above the describe
block so it is initialized before any call site, while preserving its existing
behavior and implementation.
- Around line 5-30: Extend the extractPdfText tests around
reconstructPdfMarkdown with fixtures covering two relative font sizes, a
monospace run, and a two-page document. Assert H1–H3 heading assignment with the
smallest font remaining body text, fenced code closure at page end, page
separators, and the deduplicated links footer plus Links: N header field.
- Around line 34-86: Move buildMinimalPdf into a shared, layer-neutral
test-support fixture module and parameterize it by content stream and font
resource so buildEmptyStreamPdf in
src/vault-mcp/obsidian-markdown/__tests__/pdf.test.ts uses the shared builder
instead of duplicating PDF structure; preserve the differing /Resources and
stream behavior. Update the import in
src/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.ts#L8 to reference the
new shared module rather than the mcp-core fixture, with no direct change needed
beyond that import.
In `@src/vault-mcp/obsidian-markdown/pdf-engine.ts`:
- Around line 62-89: Resolve the placement/contract mismatch for pdf-engine.ts:
either move resolvePdfjsAssetPaths and initializePdfEngine to utils/ as a
standalone primitive and update all references, or retain the module under
obsidian-markdown only after confirming the layering rule permits its filesystem
resolution and globalThis mutations. If retaining it, update AGENTS.md at the
listed entry and pure-parser contract to explicitly carve out the PDF bootstrap
alongside canvas.ts and links.ts; otherwise state that the AGENTS.md site
requires no direct change beyond updating the file listing.
🪄 Autofix
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: 4aeaf4ef-515d-45cf-9659-259f7162898f
📒 Files selected for processing (18)
.devin/wiki.jsonAGENTS.mdARCHITECTURE.mdDOCKERHUB.mdREADME.mdsrc/vault-mcp/mcp-core/__tests__/tool-definitions.test.tssrc/vault-mcp/mcp-core/tools/asset-tools.tssrc/vault-mcp/mcp-core/tools/search-tools.tssrc/vault-mcp/obsidian-markdown/__tests__/pdf-engine.test.tssrc/vault-mcp/obsidian-markdown/__tests__/pdf.test.tssrc/vault-mcp/obsidian-markdown/pdf-engine.tssrc/vault-mcp/obsidian-markdown/pdf.tssrc/vault-mcp/search/__tests__/file-watcher.test.tssrc/vault-mcp/search/__tests__/search-index.test.tssrc/vault-mcp/search/file-watcher.tssrc/vault-mcp/search/search-index.tssrc/vault-mcp/vault-operations/__tests__/asset-operations.test.tssrc/vault-mcp/vault-operations/asset-operations.ts
…h coverage
- truncateToUtf8ByteLimit: iterates Unicode code points to enforce the
100KB cap in UTF-8 bytes (string.slice truncated by UTF-16 code units,
so multi-byte content could exceed the byte limit)
- AGENTS.md: extractPdfText signature corrected to { text, totalPages }
- README + DOCKERHUB.md: 100KB indexing cap stated alongside
searchability claim
- asset-tools.ts: search coverage line updated to include canvas, PDF,
and text formats
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…it nitpicks - Rebuild pipeline: gate PDF and text file list building behind fileToolsEnabled (canvas stays unconditional for link extraction) - File watcher: check search.fileContentIndexingEnabled before reading non-canvas files for FTS — avoids wasted I/O when tables don't exist - AGENTS.md: document pdf-engine.ts as a noted exception to the obsidian-markdown/ pure-parser contract (createRequire + globalThis mutation, compatible lint profile) - pdf.test.ts: move buildEmptyStreamPdf above describe block (TDZ safety) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressing CodeRabbit review findings (review 4910580977): Actionable (4 inline threads — already resolved in prior passes): Outside-diff finding (1):
Nitpick findings (6 in review body):
🔍 ship-check · pr-monitor · claude-opus-4-6 |
Move pdf-fixture.ts from mcp-core/__tests__/ to obsidian-markdown/__tests__/ — the PDF parsers live in obsidian-markdown/, so the fixtures belong alongside them. Consolidate buildEmptyStreamPdf (was inlined in pdf.test.ts) into the shared fixture module. Update all 4 import sites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split the single-paragraph PDF description into structured subsections: bulleted list for text rendition features, paragraph for raw mode, and a labeled "Font independence" paragraph for the pdfjs engine explanation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- AGENTS.md pdf-engine.ts rationale: explain why it belongs with the PDF parsers (domain logic, same unit as pdf.ts) instead of citing lint rule compatibility - ARCHITECTURE.md: rename "Font independence" heading to "Why text works without system fonts" — plain language over jargon Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pre-existing: vault_list_files tool description omits .yaml/.yml from supported extensions
The Failure scenario: A user discovers Suggested fixNo code change needed for `vault_list_files` itself — fixing `vault_read_file`'s description (see previous finding) is the root fix, as `vault_list_files` cross-references `vault_read_file`. |
Scanned or image-only PDFs produce no indexable text and are silently skipped — note the limitation in ARCHITECTURE.md so readers don't assume all PDFs are searchable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hoist canvasLinks extraction before the transaction so the debug log retains the link count diagnostic for canvas files (lost during the generalization of upsertFileContent). Extract the PDF mapper callback into a named extractPdfContent function for readability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressing umm-actually PR-level finding: vault_list_files omits .yaml/.yml The bot's own analysis concludes "No code change needed for vault_list_files itself — fixing vault_read_file's description is the root fix." That fix was already applied ( Also fixed in this push:
🔍 ship-check · pr-monitor · claude-opus-4-6 |
|
Restore canvas link count in upsertFileContent debug log after generalization
The upsertFileContent debug log at line 1062 in search-index.ts previously emitted Failure scenario: A canvas board with file-type nodes is indexed. Previously the debug log showed the link count (e.g. Suggested fixCapture `canvasLinks.length` before the transaction and include it in the debug log conditionally (only for canvas files), or log it separately inside the canvas branch after the transaction: `logger.debug('indexed file content', { path: params.filePath, links: isCanvas ? canvasLinks.length : undefined, fts: Boolean(upsertFileContentStmt) })`. |
|
Addressing umm-actually finding: canvas link count in debug log Already fixed in 🔍 ship-check · pr-monitor · claude-opus-4-6 |
Remove wrapping parentheses around await mapWithConcurrency by splitting the result into a named intermediate (pdfResults) before filtering nulls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
vault_searchto index PDF and text file content (.txt, .csv, .json, .xml, .svg, .log, .yaml, .yml, .base) alongside canvas files from PR feat(search): canvas file content FTS + graph parity #424pdf-engine.tsfromutils/toobsidian-markdown/— PDF domain logic that fails the utils/ admission bar of "generic with zero domain knowledge"obsidian-markdown/pdf.tswithextractPdfText(Uint8Array → string)— manages pdfjs proxy lifecycle internally; callers pass bytes, get text.canvas-only guard inupsertFileContent, generalizing to all indexable file types with a 100KB content capINDEXABLE_TEXT_EXTENSIONS; PDF extraction usesmapWithConcurrency(4 slots) during rebuildTest plan
npm test— 2450 tests pass (5 new: 2 PDF extraction, 3 file content indexing)npm run lint— 0 errorsnpm run build— cleankind: "file"and correctextension🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation