Repository navigation
fix(source-control): keep huge change sets responsive - #9477
brennanb2025 merged 7 commits into
Conversation
|
Electron before/after proof for the exact 12,000-file fixture: https://htmlpreview.github.io/?https://gist.githubusercontent.com/OrcaWin/6647ab6a294ab47bc1898334076e66b6/raw/orca-7991-electron-proof.html The artifact embeds both temporary screenshots and their SHA-256 hashes; neither the screenshots nor fixture are committed to the repository. The measured IPC boundary changed from 10,000 entries / 850,222 bytes to 1,000 entries / 85,221 bytes, and deleting the fixture followed by Retry verified recovery. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change streams Git status output into an incremental porcelain parser, enabling early termination and capped status metadata. Unmerged records now count toward limits and preserve output order across main and relay status paths. Shared subprocess-tree termination handles streamed Git and agent execution cleanup. Capped metadata propagates through IPC, runtime, polling, submodule, and editor state. Source Control adds retry and truncated-submodule UI states, with end-to-end coverage for payload size, responsiveness, recovery, and cache behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
🧹 Nitpick comments (2)
src/relay/git-stdout-stream.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplicitly type
childand use optional chaining for stream access.To ensure type safety and prevent implicit
anyerrors under strict TypeScript configurations, consider explicitly typingchildasChildProcessand adding optional chaining tostdoutandstderr(since Node types them asReadable | null).♻️ Proposed refactor
Update the import and variable declaration:
-import { spawn } from 'node:child_process' +import { spawn, type ChildProcess } from 'node:child_process'- let child + let child: ChildProcess try {Add optional chaining in
cleanupand during listener setup:const cleanup = (): void => { - child.stdout.off('data', onStdoutData) - child.stderr.off('data', onStderrData) + child.stdout?.off('data', onStdoutData) + child.stderr?.off('data', onStderrData) child.off('error', onError) child.off('close', onClose) options.signal?.removeEventListener('abort', onAbort)- child.stdout.on('data', onStdoutData) - child.stderr.on('data', onStderrData) + child.stdout?.on('data', onStdoutData) + child.stderr?.on('data', onStderrData) child.on('error', onError) child.on('close', onClose)Also applies to: 42-42, 65-69, 135-138
src/relay/agent-exec-handler.ts (1)
237-252: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider using
StringDecoderinstead ofchunk.toString('utf-8').Decoding arbitrary stream chunks with
.toString('utf-8')can corrupt multi-byte characters if they happen to be split across buffer boundaries. Since the newgit-stdout-stream.tsfile elegantly solves this by usingnode:string_decoder, consider adopting the same approach here for robustness.♻️ Proposed refactor
Add the import at the top of the file:
import { StringDecoder } from 'node:string_decoder'Instantiate the decoders and replace
.toString()in the data handlers:+ const stdoutDecoder = new StringDecoder('utf8') + const stderrDecoder = new StringDecoder('utf8') + const onStdoutData = (chunk: Buffer): void => { stdoutBytes += chunk.byteLength if (stdoutBytes > MAX_OUTPUT_BYTES) { terminateRelaySubprocessTree(child) return } - stdout += chunk.toString('utf-8') + stdout += stdoutDecoder.write(chunk) } const onStderrData = (chunk: Buffer): void => { stderrBytes += chunk.byteLength if (stderrBytes > MAX_OUTPUT_BYTES) { terminateRelaySubprocessTree(child) return } - stderr += chunk.toString('utf-8') + stderr += stderrDecoder.write(chunk) }(Note: Ensure you call
.end()on both decoders during cleanup if the final trailing bytes are needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04668d81-4542-437b-8f3e-9522f2b30020
📒 Files selected for processing (20)
src/main/git/status-porcelain-parser.test.tssrc/main/git/status.tssrc/main/ipc/filesystem.test.tssrc/main/providers/ssh-git-provider.test.tssrc/main/runtime/rpc/methods/git.test.tssrc/relay/agent-exec-handler.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/relay/git-handler.tssrc/relay/git-stdout-stream.test.tssrc/relay/git-stdout-stream.tssrc/relay/subprocess-tree-termination.tssrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/useGitStatusPolling.test.tssrc/renderer/src/store/slices/editor.test.tssrc/shared/git-status-limit.tssrc/shared/git-status-porcelain-parser.tssrc/shared/git-uncommitted-line-stats.test.tstests/e2e/large-file-count-fixtures.tstests/e2e/source-control-large-file-count.spec.ts
…urce-control-freeze-repro # Conflicts: # src/relay/git-handler-status-ops.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
24-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate translated Windows release guidance.
The main README now points Windows users to the latest installer and removes the RC warning, but
docs/readme/README.fr.mdLine 220 still recommends pinned RCv1.4.147-rc.3. Update the translated documentation in the same change so localized users are not directed to stale or conflicting builds.Also applies to: 212-216
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a18d9335-c6ea-4d21-9636-a54ae5c8b422
📒 Files selected for processing (3)
README.mdsrc/main/git/status.tssrc/main/ipc/filesystem.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/git/status.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/git/status.ts (1)
308-315: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSimplify the unmerged limit logic to robustly handle skipped conflicts.
If
parseUnmergedEntryreturnsnull(e.g., for a submodule conflict), it skips that entry. BecauseunmergedLimitis pre-calculated and slices the array upfront, skipping an entry means we might return fewer thanlimittotal entries, even if there are additional unused unmerged lines in the parser that could have filled the gap.Checking
entries.lengthinside the loop simplifies the logic and ensures we fill up to the exactlimit.
(Note: You may want to apply this same simplification tosrc/relay/git-handler-status-ops.ts, which uses the identical pattern).♻️ Proposed refactor
- // Why: cap conflict lookups too so a conflict-heavy merge cannot bypass the status bound. - const unmergedLimit = didHitLimit - ? Math.max(0, limit - entries.length) - : parser.unmergedLines.length - for (const line of parser.unmergedLines.slice(0, unmergedLimit)) { + // Why: cap conflict lookups too so a conflict-heavy merge cannot bypass the status bound. + for (const line of parser.unmergedLines) { + if (didHitLimit && entries.length >= limit) { + break + } const unmergedEntry = await parseUnmergedEntry(worktreePath, line) if (unmergedEntry) { entries.push(unmergedEntry)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73541675-44a1-4b36-a99e-e4542ec78c11
📒 Files selected for processing (7)
src/main/git/status-porcelain-parser.test.tssrc/main/git/status.test.tssrc/main/git/status.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/shared/git-status-porcelain-parser.tstests/e2e/source-control-large-file-count.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/relay/git-handler-status-ops.ts
- tests/e2e/source-control-large-file-count.spec.ts
- src/relay/git-handler-status-ops.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/relay/git-handler-status-ops.ts (1)
42-61: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer asynchronous file system checks to avoid blocking the event loop.
Node.js
existsSyncis synchronous and blocks the thread. Since this function is alreadyasyncand runs on a relay server that handles concurrent requests, it is better practice to use asynchronous file system operations (e.g.,fs.promises.access) to check for file existence without blocking the event loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a199157b-2c51-4a3e-92ff-2903ac69a203
📒 Files selected for processing (29)
src/main/git/runner-command-exec.test.tssrc/main/git/runner.tssrc/main/git/status-porcelain-parser.test.tssrc/main/git/status.test.tssrc/main/git/status.tssrc/relay/agent-exec-handler.test.tssrc/relay/git-handler-status-ops.test.tssrc/relay/git-handler-status-ops.tssrc/relay/git-handler.tssrc/relay/git-stdout-stream.test.tssrc/relay/git-stdout-stream.tssrc/relay/subprocess-tree-termination.test.tssrc/relay/subprocess-tree-termination.tssrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/source-control-submodule-expansion.test.tssrc/renderer/src/components/right-sidebar/source-control-submodule-expansion.tssrc/renderer/src/components/right-sidebar/source-control-too-many-changes-banner.test.tsxsrc/renderer/src/components/right-sidebar/useSourceControlSubmoduleStatus.test.tsxsrc/renderer/src/components/right-sidebar/useSourceControlSubmoduleStatus.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/store/slices/editor.test.tssrc/renderer/src/store/slices/editor.tssrc/shared/git-status-limit.test.tssrc/shared/git-status-limit.tssrc/shared/git-status-porcelain-parser.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/relay/git-handler.ts
- src/relay/git-handler-status-ops.test.ts
* fix(source-control): keep huge change sets responsive * Fix cancellation and retry handling for capped status * Harden capped status for conflict-heavy repositories * Harden capped status recovery and cancellation * fix(source-control): preserve capped status correctness * fix(source-control): translate submodule status at render time --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
Summary
didHitLimitsignalELI5
A branch with thousands of changes made Orca carry and render a giant list all at once, briefly freezing the window even though only a capped list was shown. Orca now stops reading as soon as it has the capped result, sends only that bounded result across process and host boundaries, pauses automatic refresh churn, and lets the user retry explicitly after cleaning up files.
Electron evidence
Exact fixture in both runs: one committed file plus exactly 12,000 untracked TypeScript files in 120 directories.
The exact Electron flow passed twice after the fix, including a fresh rebuild. Removing all 12,000 untracked files and choosing Retry cleared capped mode, proving explicit recovery.
Validation
typecheck:nodetypecheck:webTrade-off / user-visible change
Repositories with more than 1,000 changes now enter capped mode earlier, show the first 1,000 entries, pause automatic watcher and safety refreshes, and expose Retry. The smaller bound is what keeps renderer projection and cross-host transfer responsive; explicit Retry preserves recovery after cleanup.
Fixes #7991