perf(history): limit parsing to recent entries - #247
Conversation
Read JSONL history from the end and stop once enough valid entries are found. Closes #206
|
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:
📝 WalkthroughWalkthroughChangesHistory loading and corruption reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant loadEntries
participant HistoryFile
participant JSONParser
Caller->>loadEntries: request recent entries
loadEntries->>HistoryFile: read chunks from file end
HistoryFile-->>loadEntries: chunk bytes
loadEntries->>JSONParser: parse reconstructed rows
JSONParser-->>loadEntries: valid entries or corruption details
loadEntries-->>Caller: requested entries and warnings
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 `@tests/history-corruption.test.mjs`:
- Around line 140-154: Add coverage in the loadEntries test suite for the
recently scanned rows fallback: create a history fixture larger than one 64KB
chunk, place a corrupted row within the portion scanned before loadEntries(1)
stops, and assert warnCorruptedHistory emits the generic recently scanned rows
wording when totalLineCount remains undefined. Keep the existing assertions for
returned entries and warning behavior where applicable.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 481b2a4e-7e1a-4cb2-a7f6-536e1ffcd951
📒 Files selected for processing (2)
src/history/store.tstests/history-corruption.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-innode:testfor test files intests/andtests/e2e/; do not use Jest or Mocha.
Write test assertions withnode:assert/strictin test files undertests/andtests/e2e/.
Files:
tests/history-corruption.test.mjs
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
The project is ESM-only: use
.jsextensions in local imports and keepverbatimModuleSyntax-compatible TypeScript imports (for example,import typefor type-only imports).
Files:
src/history/store.ts
src/{history,llm}/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Recent commit history should be stored as JSONL, analyzed by
buildProfile()for commit-style patterns, and injected into prompts as style guidance.
Files:
src/history/store.ts
🔇 Additional comments (4)
src/history/store.ts (3)
15-15: LGTM!
274-287: LGTM!
240-243: 🎯 Functional CorrectnessNo change needed: chunks already overlap by 3 bytes before decode.
The read loop subtracts
HISTORY_READ_CHUNK_SIZE - 3, so at most a 3-byte incomplete UTF-8 lead can start in the previous chunk and be completed from the next raw read. The boundary is not decoded twice, and the newline-only tail case cannot split a multi-byte leading byte.> Likely an incorrect or invalid review comment.tests/history-corruption.test.mjs (1)
140-154: LGTM! Confirmed the assertions match the traced chunked-read behavior for this fixture.
| test('loadEntries stops parsing once it has enough recent valid entries', async () => { | ||
| await withIsolatedHistory( | ||
| [ | ||
| '{invalid old line', | ||
| validEntry('fix: keep older valid entry', '2026-06-01T00:00:00Z'), | ||
| validEntry('feat: keep newest valid entry', '2026-06-01T00:00:01Z'), | ||
| ], | ||
| async ({ warnings }) => { | ||
| const entries = await loadEntries(1); | ||
|
|
||
| assert.deepEqual(entries.map((entry) => entry.message), ['feat: keep newest valid entry']); | ||
| assert.deepEqual(warnings, []); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider covering the "recently scanned rows" fallback path.
This test only exercises the case where a corrupted row is skipped entirely because it's never scanned (stop happens before reaching it). It doesn't cover the case where a corrupted row is scanned but the read stops before reaching the physical start of the file (a real multi-chunk history), which is when totalLineCount stays undefined and warnCorruptedHistory should fall back to the generic "recently scanned rows" wording. If that scenario isn't covered elsewhere in this file, consider adding a fixture large enough to span multiple 64KB chunks with a corrupted row inside the scanned range.
🤖 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 `@tests/history-corruption.test.mjs` around lines 140 - 154, Add coverage in
the loadEntries test suite for the recently scanned rows fallback: create a
history fixture larger than one 64KB chunk, place a corrupted row within the
portion scanned before loadEntries(1) stops, and assert warnCorruptedHistory
emits the generic recently scanned rows wording when totalLineCount remains
undefined. Keep the existing assertions for returned entries and warning
behavior where applicable.
Add test to ensure that when loadEntries performs a partial scan and encounters a corrupted entry at the end of the history, it correctly returns only valid entries, reports a single warning, and uses a generic location message referencing "recently scanned rows" instead of a specific file location.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d4f04ffa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const chunkEnd = position; | ||
| position = Math.max(0, chunkEnd - HISTORY_READ_CHUNK_SIZE - 3); | ||
| const bytesToRead = chunkEnd - position; | ||
| const buffer = Buffer.allocUnsafe(bytesToRead); | ||
| const { bytesRead } = await history.read(buffer, 0, bytesToRead, position); | ||
| const lines = (buffer.toString('utf-8', 0, bytesRead) + remainder).split('\n'); |
There was a problem hiding this comment.
When a multibyte character in a JSONL row straddles one of these backward-read boundaries, decoding each chunk independently replaces the character fragments with U+FFFD; subtracting three extra bytes only moves the boundary and does not align it or create overlap. The row still parses, so loadEntries() silently returns a corrupted commit message (for example, an emoji split at the boundary becomes replacement characters), which can distort both displayed history and the learned style profile injected into prompts. Carry raw boundary bytes between reads or use a streaming UTF-8 decoder instead.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
The previous implementation split chunks by converting to string and concatenating a remainder, which could break multi-byte UTF-8 code points. This caused corrupted characters (e.g., replacement character U+FFFD) in commit messages. Changed to operate on Buffer objects, splitting on newline bytes (0x0a) and keeping the remainder as a Buffer. This ensures multi-byte sequences are never split across chunk boundaries. Added a test that verifies a long history row containing multi-byte characters is correctly loaded without corruption.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3378cac2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bytesToRead = chunkEnd - position; | ||
| const buffer = Buffer.allocUnsafe(bytesToRead); | ||
| const { bytesRead } = await history.read(buffer, 0, bytesToRead, position); | ||
| const chunk = Buffer.concat([buffer.subarray(0, bytesRead), remainder]); |
There was a problem hiding this comment.
When a history row contains a large diff, every backward-read iteration without a newline copies the entire accumulated remainder again through Buffer.concat, making the scan quadratic in the row size. The CLI can persist diffs approaching its 100 MB Git buffer limit, so loading a recent entry can copy tens of gigabytes and stall every subsequent profile build; retain the chunks without concatenating them repeatedly, or assemble the row only after locating its boundary.
AGENTS.md reference: AGENTS.md:L62-L63
Useful? React with 👍 / 👎.
Refactor the backward reading logic in `loadEntries` to collect remainder chunks in an array (`remainderChunks`) instead of concatenating them with each new chunk. This avoids repeated `Buffer.concat` calls and reduces memory allocations, improving performance when reading large history files. Update the UTF-8 split test to use a 2MB message and verify that only one `Buffer.concat` call is made during the entire read operation.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a971b6318
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| position = Math.max(0, chunkEnd - HISTORY_READ_CHUNK_SIZE - 3); | ||
| const bytesToRead = chunkEnd - position; | ||
| const buffer = Buffer.allocUnsafe(bytesToRead); | ||
| const { bytesRead } = await history.read(buffer, 0, bytesToRead, position); | ||
| const chunk = buffer.subarray(0, bytesRead); |
There was a problem hiding this comment.
When FileHandle.read() returns fewer bytes than requested, as permitted on some network or FUSE filesystems, position has already moved backward by the full bytesToRead, while only bytesRead bytes are retained. The unread suffix is then permanently skipped, which can merge JSONL fragments, discard otherwise valid recent entries, or report them as corrupted. Continue reading until the requested interval is filled, or update the cursor based on the bytes actually read.
Useful? React with 👍 / 👎.
…stem The previous `history.read` call in `loadEntries` could return fewer bytes than requested, potentially causing incomplete chunk reads. Added `readHistoryChunk` which retries reading from the unread offset until all requested bytes are read or EOF is reached. Updated `loadEntries` to use the new function and exported it for testing. Added a unit test to verify retry behavior.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
1 participant
Summary
Reads JSONL history from the end and stops after collecting the requested number of valid entries.
Changes
Testing
npm run buildnode --test tests/history-corruption.test.mjsRelated
Closes #206
Summary by CodeRabbit
Bug Fixes
Tests