Repository navigation
Bound file-search inventories and release abandoned Markdown data - #25369
Conversation
📝 WalkthroughWalkthroughFile and directory listings now enforce retention limits, and SSH directory reads and transfers use bounded, cancellable enumeration. Markdown discovery uses shared collection logic and dedicated local, relay, and SSH routes. SSH response streams and Git grep records have capacity limits. Renderer Markdown snapshots and completions are scoped to the active workspace and Monaco model. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The PR adds limits to file and Markdown listings and SSH operations. The remaining issues are narrow edge cases: inconsistent Windows paths for aliased folders, possible capacity errors on bounded Git listings, budget accounting after duplicate names, and duplicate error toasts. It is mergeable with owner awareness or quick follow-ups. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Individual listings gain useful memory limits and failure cleanup. However, the new remote Markdown discovery path bypasses the existing coordination that prevents overlapping whole-workspace scans. Switching workspaces can therefore leave several bounded scans running together, potentially degrading other operations on the same remote connection. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The Git grep record cap and its tests implement [ Full details: Docstring CoverageExplanation Docstring coverage is 7.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 54 functions across 62 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 3
🧹 Nitpick comments (1)
src/renderer/src/components/editor/useMarkdownDocuments.ts (1)
100-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale snapshot can remain visible when the scope has no path.
refreshMarkdownDocumentsreturns early whenworktreeIdorworktreePathis missing. The effect cleanup still incrementsrequestRef. The snapshot key then no longer matchesscopeKey, so the hook returns an empty list. This behavior is correct.The error path has a small redundancy. Two separate
if (requestRef.current === requestId)checks guard the toast and thesetSnapshotcall. Merge them into one block.♻️ Proposed merge
if (requestRef.current === requestId) { toast.error(err instanceof Error ? err.message : 'Failed to list Markdown documents.') - } - if (requestRef.current === requestId) { setSnapshot({ key: scopeKey, documents: [] }) }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
054bbba0-463e-41db-885c-7bd43a0f9b94
📒 Files selected for processing (69)
src/main/ipc/filesystem-list-files.test.tssrc/main/ipc/filesystem-list-files.tssrc/main/ipc/filesystem-markdown-document-listing.test.tssrc/main/ipc/filesystem/filesystem-read-handlers.tssrc/main/ipc/markdown-documents-ripgrep.test.tssrc/main/ipc/markdown-documents.tssrc/main/providers/filesystem-markdown-listing.tssrc/main/providers/filesystem-provider-contract.tssrc/main/providers/sftp-directory-test-fixture.tssrc/main/providers/ssh-directory-legacy-compatibility.test.tssrc/main/providers/ssh-directory-listing.tssrc/main/providers/ssh-directory-streaming-real.test.tssrc/main/providers/ssh-directory-streaming.test.tssrc/main/providers/ssh-file-listing.tssrc/main/providers/ssh-filesystem-download.test.tssrc/main/providers/ssh-filesystem-download.tssrc/main/providers/ssh-filesystem-provider-download-folder.test.tssrc/main/providers/ssh-filesystem-provider-sftp.tssrc/main/providers/ssh-filesystem-provider.test.tssrc/main/providers/ssh-filesystem-provider.tssrc/main/providers/ssh-listing-compatibility.test.tssrc/main/providers/ssh-markdown-document-listing.tssrc/main/providers/ssh-sftp-directory-close.tssrc/main/providers/ssh-sftp-filesystem-channel.tssrc/main/providers/ssh-sftp-filesystem-provider.test.tssrc/main/providers/ssh-sftp-filesystem-provider.tssrc/main/runtime/runtime-file-commands-constructor.tssrc/main/runtime/runtime-file-commands-search-runtime-files.tssrc/main/runtime/runtime-file-listing-producer-budgets.test.tssrc/main/runtime/runtime-file-target-execution-host.test.tssrc/main/ssh/ssh-directory-transfer-budget.tssrc/main/ssh/ssh-git-response-pending-retention.test.tssrc/main/ssh/ssh-git-response-stream-reader.tssrc/main/ssh/ssh-listing-response-retention.test.tssrc/main/ssh/ssh-response-payload.tssrc/main/ssh/ssh-response-pending-frames.test.tssrc/main/ssh/ssh-response-pending-frames.tssrc/main/ssh/system-ssh-file-transfer.tssrc/main/ssh/system-ssh-upload-plan-budget.test.tssrc/relay/dispatcher-rpc-routing.tssrc/relay/filesystem-capacity-error-wire.test.tssrc/relay/fs-directory-listing.tssrc/relay/fs-file-listing-paths.tssrc/relay/fs-handler-git-fallback.tssrc/relay/fs-handler-git-search-capacity.test.tssrc/relay/fs-handler-git-search-real-capacity.test.tssrc/relay/fs-handler-git-search.tssrc/relay/fs-handler-list-files-ignored.test.tssrc/relay/fs-handler-list-files.tssrc/relay/fs-handler.tssrc/relay/fs-markdown-document-listing.tssrc/relay/protocol.tssrc/renderer/src/components/editor/markdown-document-worktree-path-selector.test.tssrc/renderer/src/components/editor/markdown-document-worktree-path-selector.tssrc/renderer/src/components/editor/monaco-markdown-doc-completions-retention.test.tssrc/renderer/src/components/editor/monaco-markdown-doc-completions.tssrc/renderer/src/components/editor/rich-markdown-image-insert.test.tssrc/renderer/src/components/editor/use-monaco-editor-decorations.tssrc/renderer/src/components/editor/useMarkdownDocuments.tssrc/renderer/src/runtime/runtime-legacy-inventory-budget.test.tssrc/renderer/src/runtime/runtime-legacy-inventory-budget.tssrc/renderer/src/runtime/runtime-legacy-quick-open-inventory-expiry.test.tssrc/renderer/src/runtime/runtime-legacy-quick-open-inventory.tssrc/shared/directory-listing-budget.tssrc/shared/file-inventory-budget.test.tssrc/shared/file-inventory-budget.tssrc/shared/git-grep-record-limit.tssrc/shared/markdown-document-paths.tssrc/shared/node-markdown-document-listing.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| let retainedBytes = budget.record([sourceDir, destinationDir], depth) | ||
| try { | ||
| for await (const entry of readDirectoryEntriesViaSftp(sftp, sourceDir, { signal })) { | ||
| const localName = sanitizeLocalDownloadFilename(entry.filename) | ||
| retainedBytes += budget.record([sourceDir, destinationDir, entry.filename, localName], depth) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count only recorded entries when you release the budget.
budget.record increments entries once per call. Line 102 records the directory, and Line 106 records each entry. Line 144 releases usedLocalNames.size + 1 entries. The two counts are different in two cases:
- Line 106 can succeed, and then Line 107 can throw for a duplicate local name. In that case the entry is recorded but not added to
usedLocalNames. The release under-counts by one. - Line 106 adds the bytes for an entry. If Line 113 (
classifySftpEntry) throws after Line 110, the release count is correct. The ordering is still fragile.
If the release under-counts, the next sibling transfer in the same run starts with a higher count. The first error ends the whole run, so the practical impact is small. Track a local counter that increments on each successful record call, and release that counter.
Proposed fix
- let retainedBytes = budget.record([sourceDir, destinationDir], depth)
+ let retainedBytes = budget.record([sourceDir, destinationDir], depth)
+ let recordedEntries = 1
try {
for await (const entry of readDirectoryEntriesViaSftp(sftp, sourceDir, { signal })) {
const localName = sanitizeLocalDownloadFilename(entry.filename)
retainedBytes += budget.record([sourceDir, destinationDir, entry.filename, localName], depth)
+ recordedEntries += 1
...
- budget.release(retainedBytes, usedLocalNames.size + 1)
+ budget.release(retainedBytes, recordedEntries)| if (signal?.aborted) { | ||
| return Promise.reject(fileListingCancellationError(signal)) | ||
| } | ||
| const inventoryBudget = new FileInventoryBudget() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the inventory budget only to unbounded Git listings.
listFilesWithGit creates the budget even when maxResults is set. The ripgrep paths in fs-handler-list-files.ts and filesystem-list-files.ts create it only for unbounded listings. A bounded Git scan has early stops based only on directFileCandidates. Directory placeholders, gitlinks and excluded paths do not advance that count, but processPath still records them. In a large tree, a bounded Quick Open request can therefore throw FileInventoryCapacityError. It should return its first N results instead. The ignored-pass catch also rethrows this error, so the primary results are lost too.
Proposed fix
- const inventoryBudget = new FileInventoryBudget()
+ const inventoryBudget = maxResults === undefined ? new FileInventoryBudget() : null- inventoryBudget.record(path)
+ inventoryBudget?.record(path)| ...prev, | ||
| [worktreeId]: [] | ||
| })) | ||
| toast.error(err instanceof Error ? err.message : 'Failed to list Markdown documents.') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Repeated toasts on every refresh failure.
refreshMarkdownDocuments runs on every effect trigger (viewMode change, activeFile.id change) and after each save. If the listing keeps failing, the user sees a new error toast each time. Pass a stable id to toast.error so sonner deduplicates the toast.
🔧 Proposed fix
- toast.error(err instanceof Error ? err.message : 'Failed to list Markdown documents.')
+ toast.error(err instanceof Error ? err.message : 'Failed to list Markdown documents.', {
+ id: 'markdown-document-list-error'
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toast.error(err instanceof Error ? err.message : 'Failed to list Markdown documents.') | |
| toast.error(err instanceof Error ? err.message : 'Failed to list Markdown documents.', { | |
| id: 'markdown-document-list-error' | |
| }) |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Aggregate memory budgets for file search — new shared
FileInventoryBudget,DirectoryListingBudget,DirectoryTransferBudget, andgit-grep-record-limitenforce retained-byte / entry / depth ceilings while data is produced. Capacity failures kill the producer, clear partial state, and surface typed error codes (-33009..-33011) across the relay envelope. - Markdown discovery split from generic listing — a dedicated
fs.listMarkdownDocumentsRPC andlistMarkdownDocumentsprovider method enumerate Markdown under their own 20k-document / 8 MiB budget, preserving the existing ignore policy and folder-workspace alias paths. Older relays fall back to a 20 001-sentinellistFilesand fail rather than report a Markdown prefix as complete. - SSH response retention —
SshResponsePayloadandSshResponsePendingFramesreplace the growableparts[]and frame-only backlog with byte-bounded, release-on-settle accumulation; sentinel metadata is validated before any chunks are retained, and old plain replies are byte-validated. - SFTP directory ownership —
readDirectoryEntriesViaSftpstreams directory packets, owns the remote handle, bounds CLOSE, and retires a channel that fails to close so the next read reopens a fresh one. - Renderer release paths — Monaco completion documents are keyed by model identity (
WeakMap) so abandoned editors release their snapshots;useMarkdownDocumentskeys its snapshot by scope and surfaces listing failures; the legacy Quick Open inventory is decoded against the byte budget and LRU-evicted by aggregate bytes.
I read the complete diff (all 69 files). Byte/chunk retention, frame-drop bookkeeping, the listSshFiles sentinel check, the folder-alias path mapping, and the SFTP close/abort boundaries all hold up against their tests — no actionable defects found.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
7910655 to
17066ec
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the 12 commits at 17066ec3be, focused on what changed since the prior pullfrog review (79106554d4) after the branch was rebased onto current main.
- Removed the dead Markdown walker — deleted
shared/node-markdown-document-discovery.tsand its test (the fs-walk enumerator superseded by the ripgrep collector), plus the now-unusedmaxVisitedEntries/maxDepth/visitedEntries/visitMarkdownDocumentListingEntry/retainMarkdownRelativePathfrommarkdown-document-listing-limits.ts. No dangling references remain. - Fixed transfer-budget release accounting —
downloadDirectoryTreeinssh-filesystem-download.tsnow tracksretainedEntries(root plus one per recorded entry) instead ofusedLocalNames.size + 1, which under-counted on the duplicate-local-name abort path and leakedDirectoryTransferBudgetentries. - Hardened the SFTP download tests —
withSftpDirectoryHandlesfixture usage across the suite, a release-accounting assertion on the duplicate-name case, andit.each(['EOF', 'CLOSE'])final-abort cases proving no empty destination is created. - Cleanup — consolidated the
MarkdownDocumentimport infilesystem-provider-contract.ts; reverted the incidentalrich-markdown-image-insert.test.tschange (dropped the "Align image insertion test" commit).
The retainedEntries change is a genuine fix: on the duplicate-local-name throw the extra entry is already charged to the budget, so the old usedLocalNames.size + 1 released one fewer entry than it recorded. The added assertion fails against the old accounting.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the single commit at 972632804a since the prior pullfrog review (17066ec3be).
- Localized the Markdown listing-failure fallback —
useMarkdownDocumentsnow routes the non-Errortoast text throughtranslate('auto.components.editor.useMarkdownDocuments.listFailed', …)at settlement time, with the newen.jsonkey; realErrormessages still pass through unchanged, and pseudo-localized locales now get the fallback pseudo-localized too. - Added regression tests — a settlement-time localization test that fails against the previous hardcoded fallback, plus cases preserving real error detail and ignoring superseded/unmounted failures.
The navigation suite (17 tests) passes, and verify:localization-extraction, verify:localization-catalogs, and verify:localization-coverage all pass with the new key.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
9726328 to
d4eeb8c
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the two commits at d4eeb8c931 since the prior pullfrog review (972632804a). Both are test-only changes to src/relay/fs-search-line-fragments.test.ts; no production code changed in this range.
- Rewired the relay search fixture to real byte streams —
createProcess()now returns a mocked child whosestdoutis a realPassThrough, andrun()writesBufferchunks instead of emitting pre-decoded strings. This exercises both decoder boundaries production actually uses:searchWithRgcallsstdout.setEncoding('utf-8')(string chunks), whilesearchWithGitGrepconsumes rawBufferchunks. - Fragmented UTF-8 coverage — the Unicode case writes one byte per chunk, so multi-byte sequences now cross chunk boundaries through the real decoder rather than arriving pre-assembled. The stale
setEncodingspy assertion was correctly dropped since it no longer reflects the harness. - Dropped the
ChildProcesscast —createProcess()no longer imports or casts toChildProcess(stdoutisReadable | null, which cannot be.write()-n); the return type is inferred.
I ran the suite (6 tests pass) and pnpm tc:node (clean). The prior reviews' concerns were already retired; no open Pullfrog threads remain on the PR.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
d4eeb8c to
2f2058f
Compare
Credits @OrcaWin for the original bounded-record proposal.
Apply mobile limits before transport, list Markdown through its semantic producer, and retain complete directory results only within explicit capacity budgets. Stream SFTP directory packets and reject oversized transfer plans before reporting success; preserve narrow older-peer fallbacks and validate streamed response retention.
Keep one current document snapshot per consumer and attach completion candidates to the actual editor model lifetime. Resolve folder workspace roots through existing workspace identities so their previews and completions receive the same authorized listing as worktrees.
Leave unqualified inventories complete beyond 20,000 files. Charge retained paths and serialized bytes at local and relay producers, reject oversized full inventories explicitly, and bound old-peer response reassembly; keep caller-specific mobile and explicit result limits.
Send CLOSE even after cancellation, stop waiting after five seconds without an acknowledgement, and ignore late callbacks. Preserve the original capacity or cancellation error and close handles returned after an aborted OPENDIR.
70112b5 to
d06a8fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
33328970-42e3-4806-824e-c50cef367404
📥 Commits
Reviewing files that changed from the base of the PR and between 972632804accc257c9eea941e2de7dd63197ab71 and a4f286b.
📒 Files selected for processing (18)
src/main/ipc/filesystem-markdown-document-listing.test.tssrc/main/ipc/filesystem/filesystem-read-handlers.tssrc/main/ipc/markdown-documents-ripgrep.test.tssrc/main/ipc/markdown-documents.tssrc/main/providers/filesystem-markdown-listing.tssrc/main/providers/ssh-filesystem-provider.tssrc/main/providers/ssh-listing-compatibility.test.tssrc/main/providers/ssh-markdown-document-listing.tssrc/main/ripgrep/bundled-ripgrep-path.test.tssrc/main/ripgrep/bundled-ripgrep-stop.test.tssrc/main/ripgrep/bundled-ripgrep-stop.tssrc/relay/fs-handler.tssrc/relay/fs-markdown-document-launch.test.tssrc/relay/fs-markdown-document-listing.test.tssrc/relay/fs-markdown-document-listing.tssrc/relay/fs-search-line-fragments.test.tssrc/renderer/src/i18n/locales/en.jsonsrc/shared/node-markdown-document-listing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/i18n/locales/en.json
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| return rootPath === args.rootPath | ||
| ? documents | ||
| : markdownDocumentsFromRelativePaths( | ||
| args.rootPath, | ||
| documents.map((document) => document.relativePath) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Rebuild aliased-root document paths with joinSearchRoot.
collectMarkdownDocuments sets each filePath with joinSearchRoot(rootPath, relativePath). This keeps native Windows separators. On the alias branch, this handler rebuilds each document with markdownDocumentsFromRelativePaths(args.rootPath, ...). That helper builds filePath as ${normalizedRoot}/${relativePath}.
Assume a Windows request root differs from the resolved root. Possible causes are a junction or symlink alias, different casing, or a trailing separator. The handler then returns mixed paths such as C:\repo/docs/README.md. That path does not match the native path the collector returns for the same file on the non-alias branch. Renderer code that compares paths can then treat one file as two documents. One example is Markdown link navigation that opens a duplicate tab.
The relay fallback in src/relay/fs-handler.ts (Lines 141-144) already corrects this case. It remaps filePath through joinSearchRoot after the same helper call. Use the same remap here.
🐛 Proposed fix
return rootPath === args.rootPath
? documents
- : markdownDocumentsFromRelativePaths(
- args.rootPath,
- documents.map((document) => document.relativePath)
- )
+ : markdownDocumentsFromRelativePaths(
+ args.rootPath,
+ documents.map((document) => document.relativePath)
+ ).map((document) => ({
+ ...document,
+ filePath: joinSearchRoot(args.rootPath, document.relativePath)
+ }))Import joinSearchRoot from ../../../shared/text-search-paths.
ELI5
Part 1 of 3; base
main, followed by #25370 and #25371.Large listings fail clearly at an explicit memory limit. Markdown links and completion find documents beyond the ordinary file-list page.
What Changed
Accounts for directory entries, transfer plans, inventory retention, response queues and pending connections while data arrives. Uses a filtered Markdown producer on current hosts and the existing bounded full-inventory path on older hosts.
Preserves Windows drive/UNC path identity and historical SSH hidden-directory, ignore-file, unreadable-subdirectory and missing-ripgrep behavior. WSL failure and cancellation use the existing process-tree terminator.
Why
Producer-side accounting avoids allocating an oversized list before rejecting it. Reusing existing discovery and process ownership preserves established host behavior and prevents a second fallback policy.
Linked Issue
Fixes #17592.
Visual Proof
Markdown link activation previously remained on Source; the corrected path opens Target. Completion insertion and loaded content were asserted in the hidden app.
Before: link activation stays on Source
After: Target document opens
Testing
Bounds-layer
pnpm tcand six focused suites (50 tests) passed. Combined-stack verification passed 88 suites (888 tests, one existing skip), full typecheck and changed-code quality checks. Real ripgrep regressions cover Windows path identity, SSH ignore policy, a 25,002-source-file tree, missing binaries and unreadable subdirectories.Real Windows/Ubuntu 24.04 WSL probes passed abort, timeout and invalid-output cleanup. macOS rendered Markdown navigation passed four flows; current/older production SSH handlers passed nine client/host pairings including a complete 23,000-file inventory.
Two hidden native Windows Electron checks also passed on final head
e44c29d019e0: linked-folder refresh through retargeting and Quick Open rendered filename/path results.All tests/apps used
ORCA_BACKGROUND_LAUNCH=1. Fresh hosted CI on the rewritten heads is queued; merge remains gated on required checks.Review
A fresh independent reviewer rated the final combined source
e44c29d019e0low risk, with no unresolved concrete blocker after adversarial review, compatibility repairs, real SSH latency checks and native WSL cleanup evidence. Their focused stable-source rerun passed 20 suites / 155 tests. Counts above overlap and are not summed.The final production TypeScript is 58 net lines smaller than the previous candidate; additional tests preserve the behavior that the simplification must keep.
Agent skill upstream boundary
Notes
Legacy complete inventories retain a 64 MiB admission limit; Markdown discovery retains 20,000 documents / 8 MiB metadata. Oversized data produces an explicit error. These are retained-data limits, not total process RSS guarantees.
Checklist