Repository navigation
fix(ai-vault-search): own buffered transcript rows before they pin the parent - #22563
Conversation
…e parent #22378 had the right fix: capped search rows are slices of the transcript, so the writer has to own the text before buffering it. Use the shared copier, and land the heap regression that fails while a slice still keeps the tool output alive. Co-authored-by: fancivez <fancivez@gmail.com>
![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
session-search-index-writer.ts: reuse the shared string-owner helper — the inlineBuffer.from(row.text, 'utf16le').toString('utf16le')detach is replaced byownRetainedString(row.text), which performs the same V8SlicedStringdetach, skips the copy for strings under the sliced-string threshold, and falls back to the code-unit block copier whereBufferis absent.session-search-index-writer.test.ts: forced-GC retention test — 70 tool rows with ~2 MiB of discarded output capped to 3072 chars each, asserting retained heap stays under 16 MiB and that every capped row remains searchable after commit.
The helper is equivalent to the inline copy for every input this path produces: the main process always has Buffer, and the helper's round-trip probe already covers lone surrogates and NUL, so code units survive byte-for-byte. The new test is real coverage rather than theatre — without the copy each 3072-char slice pins its full multi-megabyte parent, so the retained-heap bound fails. --expose-gc is pinned in config/vitest.config.ts, matching the retention-test convention across the repo. No wire, SSH, path, or platform surface is touched.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthrough
Merge Risk: ⚪ Minimal · up to The search and sidebar changes preserve their intended behavior, and the regression test is supported by the standard test setup. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
The toolchain banner reads detectedWorktreesByRepo, and the real store always has that map. The sidebar test left it out, so a rerender with a repo crashed before the scan assertion.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Sidebar.test.tsx: sidebar harness gains the worktree listing map — the mocked store insetSidebarStatenow suppliesdetectedWorktreesByRepo: {}.LocalGitToolchainScanBannerreadss.detectedWorktreesByRepoand indexes it by repo id, so tests that seedreposwith ids would otherwise dereferenceundefined. The fixture matches the shape every other renderer test harness uses, and the suite passes locally (6/6).
This delta is test-harness-only; the memory-ownership change in session-search-index-writer.ts was reviewed previously (review 5297597363) and is unchanged. The fixture addition is a reasonable green-CI fix, though it is unrelated to the indexing fix in the PR title.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Agent Session History indexing already stopped holding onto huge transcript lines, but the person who first fixed that was not named on the merge. This records their fix the way the rest of the app copies those strings, and adds the test that proves the big lines are actually released.
What Changed
Before, the search writer made its own UTF-16 copy of each buffered row. That copy did release the parent transcript, which is why large Codex logs stopped crashing the indexer. It was the same idea as the open pull request, written a second time, and that pull request's author was not on the commit.
After, the writer uses the shared string-copy helper the rest of the app already uses for this, and a regression test buffers 70 large tool rows, checks that the retained heap stays small, then confirms every capped row is still searchable.
Why
The open pull request was right. A capped tool row is a slice of the original message, so keeping the short slice keeps the whole message alive. Copying through the existing helper is the same detach the merged code did inline, and it skips the copy for strings that are already too short to pin a parent. The new test fails if that copy is removed: retained heap stays about 280 MiB instead of under 16 MiB.
Linked Issue
Follow-up to #22296, which is already fixed. Credits #22378.
Visual Proof
N/A — indexing memory only, no screen or interaction change.
Testing
pnpm test src/main/ai-vault-search/session-search-index-writer.test.ts— 9 passed, including the new retention test.oxlintandoxfmt --checkare clean on the two files.AI Disclosure
Review
The helper preserves JavaScript code units, including unpaired surrogates, which is what the inline copy was doing. Search text, caps, and commit behavior are unchanged. No SSH, remote wire, or path changes. The writer runs on the execution host for local session history.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Co-authored-by: fancivez fancivez@gmail.com