perf(core): Optimize sortPaths with decorate-sort-undecorate pattern#1235
perf(core): Optimize sortPaths with decorate-sort-undecorate pattern#1235
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a series of targeted performance optimizations across key file processing functions within the core module. By strategically employing more efficient data structures and algorithms, the changes aim to significantly reduce execution times and resource consumption in path sorting, file tree generation, and security-related file filtering, particularly when dealing with larger datasets. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughOptimization and refactoring of file-handling utilities: sortPaths now precomputes split path components and uses an improved comparator with directory prioritization; fileTreeGenerate introduces an O(1) lookup map for child nodes and refactors string construction with helper functions; filterOutUntrustedFiles replaces linear search with Set-based membership check. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Deploying repomix with
|
| Latest commit: |
f38828a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://51445554.repomix.pages.dev |
| Branch Preview URL: | https://claude-upbeat-haslett.repomix.pages.dev |
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance optimizations across sortPaths, generateFileTree, and filterOutUntrustedFiles functions. The changes leverage efficient data structures and algorithms, such as pre-computing path splits for sorting, using Map for O(1) child lookups in tree generation, and Set for O(1) lookup of suspicious files. The refactoring to use array join() for string concatenation in tree stringification also contributes to better performance. The benchmark results provided clearly demonstrate the substantial speedups achieved by these optimizations. The code is clean, well-structured, and directly addresses the identified performance bottlenecks, making it a valuable improvement to the codebase.
Code ReviewOverall: Clean, well-benchmarked performance PR. All three optimizations are algorithmically sound and the benchmark numbers are impressive. The changes are minimal in scope and maintain behavioral compatibility. filePathSort.ts — Pre-split optimizationThe decorate-sort-undecorate pattern (Schwartzian transform) is the right approach here. Avoiding Minor: In-place reorder creates an intermediate arrayLines 31-34 allocate a fileTreeGenerate.ts — Map lookup + array join + sort dedupThree wins in one:
Observation: optional chaining on nodeMapLine 48: filterOutUntrustedFiles.ts — Set lookupTextbook Potential risks
VerdictLooks good to merge. No bugs or behavioral changes detected. 🤖 Generated with Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1235 +/- ##
=======================================
Coverage 87.18% 87.18%
=======================================
Files 115 115
Lines 4322 4324 +2
Branches 1002 1002
=======================================
+ Hits 3768 3770 +2
Misses 554 554 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/file/fileTreeGenerate.ts (1)
42-50: Optional chaining could mask invariant violations.The
nodeMap?.usage on lines 43 and 48 silently handlesundefined, but ifnodeMapis ever undefined, the child would still be added tocurrentNode.children(line 47) while the lookup map would not be updated—breaking the O(1) optimization without any visible error.The invariant (every node has an entry in
childLookup) should hold given the initialization at lines 16 and 50, but a non-null assertion or explicit check would surface bugs faster if the invariant is ever violated.🔧 Optional: Add defensive assertion
const nodeMap = childLookup.get(currentNode); - let child = nodeMap?.get(part); + if (!nodeMap) { + throw new Error(`Missing childLookup entry for node: ${currentNode.name}`); + } + let child = nodeMap.get(part); if (!child) { child = createTreeNode(part, !isLastPart || isDirectory); currentNode.children.push(child); - nodeMap?.set(part, child); + nodeMap.set(part, child); childLookup.set(child, new Map<string, TreeNode>()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/file/fileTreeGenerate.ts` around lines 42 - 50, The optional chaining on nodeMap (nodeMap?.get and nodeMap?.set) can hide invariant violations where childLookup lacks an entry for currentNode; change this to an explicit check/assertion: retrieve nodeMap using childLookup.get(currentNode) and if it's undefined throw or assert with a clear message referencing childLookup and currentNode, so you fail fast instead of silently skipping the map update; then proceed to create the child with createTreeNode and call nodeMap.set(part, child) and childLookup.set(child, new Map<string, TreeNode>()) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/file/fileTreeGenerate.ts`:
- Around line 42-50: The optional chaining on nodeMap (nodeMap?.get and
nodeMap?.set) can hide invariant violations where childLookup lacks an entry for
currentNode; change this to an explicit check/assertion: retrieve nodeMap using
childLookup.get(currentNode) and if it's undefined throw or assert with a clear
message referencing childLookup and currentNode, so you fail fast instead of
silently skipping the map update; then proceed to create the child with
createTreeNode and call nodeMap.set(part, child) and childLookup.set(child, new
Map<string, TreeNode>()) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44012b1e-23a0-4969-bafd-5e6a3805e03e
📒 Files selected for processing (3)
src/core/file/filePathSort.tssrc/core/file/fileTreeGenerate.tssrc/core/security/filterOutUntrustedFiles.ts
Code Reviewalready left a thorough review covering the key points. Adding a few incremental observations below. AI Bot Inline CommentsNo inline review comments from bots — nothing to evaluate. Incremental observationsfileTreeGenerate.ts — placement The refactoring moves to the public API boundary (, ), which runs it once per call instead of recursively at every level. One thing to be aware of: if a caller ever reuses the same filePathSort.ts — For very large repos (e.g., 100k files), filterOutUntrustedFiles.ts Clean and correct. The Overall: The optimizations are algorithmically sound, benchmarks are compelling, and the implementation matches the intent. The 🤖 Generated with Claude Code |
4aabb71 to
315494a
Compare
Follow-up ReviewPrevious Claude review was thorough and covered all key points. One incremental update:
No new issues found. Good to merge. 🤖 Generated with Claude Code |
Follow-up Review (3rd pass)Previous Claude reviews covered the algorithmic improvements thoroughly. The New observation: module-level
|
Code ReviewOverall: Looks good. Clean, well-motivated optimization with solid benchmarks. What's Good
Minor ObservationsDetails
Premortem AnalysisDetails
VerdictApprove. The optimization is well-targeted, the implementation is clean, and the benchmark data is convincing. The only suggested follow-up is updating the PR title to reflect the final scope. 🤖 Generated with Claude Code |
e14a9b2 to
482e0ab
Compare
Pre-compute path.split() once per path before sorting, avoiding O(N log N) repeated string allocations during comparisons. Benchmark: 10,000 files 65ms → 11ms (6x speedup). Co-Authored-By: Claude Opus 4.6 (1M context) <koukun0120@gmail.com>
482e0ab to
d9fa509
Compare
Code ReviewOverall, the PR Scope vs. Content MismatchThe PR title and description mention three optimizations (sortPaths, generateFileTree, filterOutUntrustedFiles), but the diff only contains changes to Test Change Weakens AssertionDetailsThe test change in expect(result.filePaths).toEqual(mockFileStructure);with: expect(result.filePaths).toEqual(expect.arrayContaining(mockFileStructure));
expect(result.filePaths).toHaveLength(mockFileStructure.length);This no longer verifies ordering — it only checks that all elements are present. Since Behavioral Change: No Longer Mutates InputDetailsThe original implementation used Minor: Object Allocation OverheadDetailsThe decorated array creates an object SummaryThe core optimization is clean and well-motivated. Main action items:
🤖 Generated with Claude Code |
Replace weak arrayContaining assertion with exact toEqual using the correct sorted order, so the test verifies both content and sort behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <koukun0120@gmail.com>
Code ReviewOverall: Clean, well-motivated optimization. The decorate-sort-undecorate pattern is the right approach here and the benchmark numbers are convincing. A couple of observations: PR Description vs. Actual ChangesThe PR body describes three optimizations ( Code QualityThe implementation in Behavioral note: in-place mutation removedThe original code used Test ChangeThe test assertion change from No Issues Found
LGTM 👍 🤖 Generated with Claude Code |
…bility Mock data and expected sort order now use path.sep instead of hardcoded '/' separators. On Windows, path.sep is '\' so sortPaths splits differently, producing a different sort order. Co-Authored-By: Claude Opus 4.6 (1M context) <koukun0120@gmail.com>
Code Review (Follow-up)A previous review already covered the algorithmic analysis well. Two additional observations: PR description vs actual changesThe PR description and benchmarks reference three optimizations ( Public API mutation semantics
VerdictLooks good. The core optimization is clean and correct, and the test improvements (exact sort-order assertion + 🤖 Generated with Claude Code |
Summary
Optimize three performance-critical functions in the file processing pipeline:
sortPaths(filePathSort.ts): Pre-computepath.split()results before sorting, avoiding O(N log N) repeated string split allocations in sort comparisonsgenerateFileTree(fileTreeGenerate.ts): UseMapfor O(1) child node lookup instead ofArray.find()O(N) scan; usearray.push()+join()instead of string concatenation in tree stringification; eliminate redundant recursive sortingfilterOutUntrustedFiles(filterOutUntrustedFiles.ts): UseSet.has()for O(1) lookup instead ofArray.some()O(M) linear scanBenchmark Results (median, 50 iterations)
sortPaths— 6-7x fastergenerateTreeString— 1.3x faster at scaleTree construction only (Map vs Array.find):
filterOutUntrustedFiles— up to 30x fasterChecklist
npm run testnpm run lint🤖 Generated with Claude Code