Sign up
View stargazersSign up

perf(server): sort the working-tree file list once per status read#22499

merged2
main
GitHub fork: r11/h3-h2
opened12d agobyOonly21mil
1 files changed
View on GitHubTwo-way sync enabledPR last imported 11h ago

Problem

Each local status read in GitVcsDriverCore.ts builds the working-tree file list in two steps. It turns the numstat entries into file rows and sorts them with toSorted. Then it appends the paths that have no numstat (untracked files and the original path of a rename) and sorts the whole list again with sort. The second sort decides the final order, so the first one only costs an extra array copy and its comparisons on every status read.

Change

Drop the first .toSorted((a, b) => a.path.localeCompare(b.path)). The final files.sort(...) after the append is unchanged. Without the trailing call, the formatter puts .map( on the same line as Array.from(...), so the hunk re-indents five lines. With whitespace ignored, the only change is the removed toSorted line.

The returned list is the same for every input. The final sort uses the same comparator and is stable. Rows that compare equal keep their relative order on both revisions, numstat rows in map order and then the appended paths in the order they were added.

Scope and approval

No issue. This is a small server-side cleanup with no change to what status returns, so server, web, desktop and mobile behavior stay the same. One production file, 5 additions and 7 deletions, and no new test.

Verification

Run on Linux.

The returned list does not change, so a test of it passes on main too. Existing tests already guard the order this PR must keep. commits selected files from a nested project directory and reports the linked worktree as the root of a nested directory in it in GitVcsDriverCore.test.ts expect an untracked path ahead of tracked ones. With the final files.sort(...) removed, both fail. An earlier revision added a test that counted sort passes with an Array.prototype.sort spy. It mirrored the implementation and repeated the cases above, so it was dropped.

To size the saved work, I added temporary Array.prototype.sort and String.prototype.localeCompare spies to a status test, ran it on both revisions and did not commit it. Main makes 9 comparisons between the five paths and this branch makes 7. The returned file list and totals have the same SHA-256 on both.

Main makes 2 sort passes and 9 comparisons, this branch 1 pass and 7, with the same output hash

Also run on the branch:

  • vp test run src/vcs/GitVcsDriverCore.test.ts src/vcs/GitVcsDriver.test.ts src/vcs/VcsStatusBroadcaster.test.ts in apps/server: 3 files, 301/301 passed.
  • tsc-rs --noEmit in apps/server: exit 0, no errors.
  • vp fmt --check and vp lint on the changed file: format passes. Lint shows one older warning on a line this PR does not touch.

Not checked:

  • macOS and Windows test runs.
  • A wall-clock measurement. This PR claims only the removed work: one array copy and one sort of the numstat rows per status read.
  • Repository-wide tests and CI.

Prior PRs: none found that remove this sort.

Generated with Claude Opus 5.5 in T3 Code Nightly

Checks

21 successful · 14 skipped
All checks passed
9188d5f

Added 1 commit

  • perf(server): sort the working-tree file list once per status read0ff2742
CcoderabbitaiBot12d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: da650b54-b819-4835-83a2-165b1667474d
📥 Commits

Reviewing files that changed from the base of the PR and between ca140c435f38612656dd655fc42e2f78efbfb4c9 and 0ff2742e22978ab41464346fdf5d71d5962928f8.

📒 Files selected for processing: 2
  • apps/server/src/vcs/GitVcsDriverCore.test.ts
  • apps/server/src/vcs/GitVcsDriverCore.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.

Walkthrough

The status reader now sorts the complete changed-file list after adding files without numstat data. A regression test checks alphabetical ordering, aggregate insertion and deletion totals, and that sorting occurs once.

Changes: Changed-file list

Layer / File(s)Summary
Build and validate the changed-file list
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts		The status reader builds the initial file list without sorting, then sorts after adding files without numstat entries. The regression test checks alphabetical order, aggregate totals, and that sorting occurs once.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: juliusmarminge

Merge Risk: External image Minimal · up to 0ff27

The working-tree file list remains ordered with its totals preserved. No issue identified here needs resolution before merge.

Architecture risk: External image Low · up to 0ff27

The change affects 1 system.

Changed systems: apps/server

Architecture concerns No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.test.ts: Added the vi test utility import for mocking and spy assertions.
  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.test.ts: Added a status regression test covering modified text, binary, and new files without numstat data. It verifies alphabetical file ordering, correct aggregate counts, and that only one sort operation processes the returned file list.
  • observed — Modified behavior in apps/server/src/vcs/GitVcsDriverCore.ts: Replaced the .toSorted() call on the initial file-stat mapping with an unsorted map; the complete file list is still sorted later after zero-stat changed files are appended.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main change: sorting the working-tree file list once per status read. It uses an appropriate conventional commit format.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Description checkPassedThe description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the behavior, scope rationale, implementation change, focused test results, commands …
Full details: Description check

Explanation

The description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the behavior, scope rationale, implementation change, focused test results, commands run, observed outcomes, limitations, and agent attribution. It also explains why no issue or new test was used.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Added 1 commit

  • test(server): drop the sort-count test that existing status tests already cover9188d5f
Mmaria1d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified unchanged head 9188d5f14bb414dd929adf2df0ebf630708eb294 on linux with the real dev server, repository playwright, /usr/bin/google-chrome, and codex gpt-6.1-sol through the local proxy. projects, threads, files, edits, and commits were created through the app. no mocks or fixtures. only one dev server ran at a time.

reproduced the redundant sort on github/main (5732bbee39) first. non-pausing inspector logpoints in the running servers observed both sort calls on main and only the final sort on this head. the returned file list and totals matched byte for byte (sha256 c858f4af6277f42f2ec9f73f601fd9869a03faa5710c4cda2d207e640051c8cb). no source instrumentation was added.

the client preserved ordering and counts for modified text, binary, untracked files, and a staged rename. checked the status-backed commit picker, expanded diffs, refresh, and a real app commit returning to clean +0/-0. the picker reports +2/-1; the diff also counts the untracked line and reports +3/-1 on both revisions.

blacksmith: the three existing git driver/status broadcaster test files passed, 301/301 tests. all 28 reported pr checks are successful. no code changes were needed.

main, matching repo data:

main status picker with five sorted paths

pr, matching repo data:

pr status picker with the same five paths and totals

pr diff expansion and refresh in the real client

short runtime transcript

Not verified

native desktop, ios/android, macos/windows, relay/tunnel connections, other providers, and high-volume or wall-clock performance. this linux pass verifies removed sort work and preserved behavior; it does not claim a measured speedup.