feat!: use async and batch stage commands #208
Conversation
🦋 Changeset detectedLatest commit: f3ed526 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes refactor the codebase to use asynchronous file operations and batch staging of files. New batch Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant prettyQuick
participant Ignorer
participant FileProcessor
participant SCM
CLI->>prettyQuick: Call with options (onStageFiles)
prettyQuick->>Ignorer: await createIgnorer()
prettyQuick->>FileProcessor: await processFiles()
FileProcessor->>FileProcessor: Process files asynchronously
FileProcessor-->>prettyQuick: Return files to stage
prettyQuick->>CLI: Call onStageFiles(files)
prettyQuick->>SCM: await stageFiles(files)
SCM->>SCM: Batch files, run add in chunks
SCM-->>prettyQuick: Staging done
prettyQuick-->>CLI: Return result
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/createIgnorer.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs src/index.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs src/processFiles.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors file staging commands to use asynchronous and batched processing for both Mercurial and Git, replacing synchronous and single-file staging functions with new, more scalable alternatives.
- Introduce a new onStageFiles hook in PrettyQuickOptions.
- Replace stageFile with asynchronous, batched stageFiles functions in both hg and git modules.
- Update file operations to use fs/promises and adjust CLI and index integration accordingly.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Added the onStageFiles hook to the PrettyQuickOptions interface. |
| src/scms/hg.ts | Removed stageFile and introduced batched async stageFiles implementation. |
| src/scms/git.ts | Removed stageFile and introduced batched async stageFiles implementation. |
| src/processFiles.ts | Refactored to use async file operations with fs/promises. |
| src/index.ts | Integrated the onStageFiles hook and updated staging logic in the main flow. |
| src/createMatcher.ts | Removed an extraneous blank line for clarity. |
| src/createIgnorer.ts | Updated to use fs/promises with asynchronous file reading and tryFile check. |
| src/cli.mts | Added logging for onStageFiles and updated the invocation style for main. |
commit: |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #208 +/- ##
==========================================
- Coverage 90.58% 87.11% -3.48%
==========================================
Files 9 9
Lines 170 194 +24
Branches 41 44 +3
==========================================
+ Hits 154 169 +15
- Misses 16 25 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to f4f656a in 1 minute and 22 seconds. Click for details.
- Reviewed
277lines of code in8files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/createIgnorer.ts:15
- Draft comment:
Consider adding error handling around the file read operation. If readFile fails (e.g. due to permission issues), it might cause an unhandled rejection. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. src/processFiles.ts:21
- Draft comment:
Using Promise.all to process all files concurrently can be a performance risk in very large repositories. Consider throttling concurrency to avoid overwhelming system resources. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. src/scms/git.ts:88
- Draft comment:
The batch staging logic uses a fixed chunk size of 100 files. It might be beneficial to make this value configurable in case repositories have significantly more or fewer files. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. src/index.ts:112
- Draft comment:
In the staging step, if an error occurs while staging files, only a generic 'STAGE_FAILED' flag is added. Consider logging the caught error to aid debugging. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_6oydEOglWmNiu2Nu
You can customize 
src/cli.mts
Outdated
| console.log(`🔍 Examining ${picocolors.bold(file)}.`) | ||
| }, | ||
|
|
||
| onStageFiles() { |
There was a problem hiding this comment.
The onStageFiles callback is defined without any parameters, but later it’s invoked with an array of files. For consistency with the expected signature (onStageFiles(files: string[])), update the callback to accept a parameter.
| onStageFiles() { | |
| onStageFiles(files) { |
Co-authored-by: Cameron Hessler <cameronhessler123@gmail.com>
![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
♻️ Duplicate comments (1)
src/cli.mts (1)
54-56:⚠️ Potential issueFix callback parameter to match interface signature.
The
onStageFilescallback is defined without parameters, but according to the interface insrc/types.ts, it should accept afiles: string[]parameter.Apply this diff to fix the parameter mismatch:
- onStageFiles() { + onStageFiles(files) { console.log(`🏗️ Staging changed files.`) },
🧹 Nitpick comments (1)
src/index.ts (1)
110-117: Well-implemented batch staging with proper error handling.The batch staging implementation correctly:
- Invokes the callback before staging
- Handles staging failures gracefully
- Uses try-catch to capture any staging errors
Consider the performance implications for very large changesets. The current implementation stages all files at once, but for repositories with hundreds of changed files, you might want to consider chunking the staging operations:
if (filesToStage.length > 0) { try { await onStageFiles?.(filesToStage) - await scm.stageFiles(directory, filesToStage) + // Stage in chunks of 100 to avoid command line length limits + const CHUNK_SIZE = 100 + for (let i = 0; i < filesToStage.length; i += CHUNK_SIZE) { + const chunk = filesToStage.slice(i, i + CHUNK_SIZE) + await scm.stageFiles(directory, chunk) + } } catch { failReasons.add('STAGE_FAILED') } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/great-panthers-shout.md(1 hunks)src/cli.mts(2 hunks)src/createIgnorer.ts(1 hunks)src/createMatcher.ts(0 hunks)src/index.ts(5 hunks)src/processFiles.ts(2 hunks)src/scms/git.ts(1 hunks)src/scms/hg.ts(1 hunks)src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/createMatcher.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/scms/hg.ts (1)
src/scms/git.ts (1)
stageFiles(87-104)
src/scms/git.ts (1)
src/scms/hg.ts (3)
getUnstagedChangedFiles(56-56)getChangedFiles(39-54)stageFiles(58-75)
src/index.ts (1)
src/createIgnorer.ts (1)
createIgnorer(9-22)
🔇 Additional comments (13)
src/types.ts (1)
19-19: LGTM! Well-designed callback interface.The new
onStageFilescallback method follows the established pattern of other callbacks in the interface and correctly supports both synchronous and asynchronous implementations..changeset/great-panthers-shout.md (1)
1-6: Appropriate major version bump for breaking changes.The changeset correctly identifies this as a major version update, which is appropriate given the introduction of async operations and batch staging that could affect existing integrations.
src/cli.mts (2)
78-82: Good error handling for staging failures.The addition of
STAGE_FAILEDerror handling provides useful feedback to users when batch staging fails, advising them to manually stage Prettier changes.
88-88: Proper async function invocation.Using
void main()is the correct way to invoke a top-level async function, ensuring any unhandled promise rejections are properly handled.src/index.ts (3)
28-28: Correctly added callback parameter.The
onStageFilesparameter is properly added to the function signature, matching the interface definition insrc/types.ts.
43-47: Consistent async implementation.The
createIgnorercalls are correctly updated to useawait, which is consistent with the function being made async as shown in the relevant code snippets fromsrc/createIgnorer.ts.
83-94: Efficient batch collection approach.The change from immediate individual staging to collecting files in
filesToStagearray is a good optimization that will reduce the number of SCM operations.src/scms/hg.ts (1)
58-75: LGTM! Well-implemented batch staging with proper chunking.The
stageFilesfunction correctly implements batching to avoid command line argument limits. The logic is consistent with the Git implementation and handles the chunking efficiently.One consideration: if any batch fails, the entire operation will fail due to the
awaitin the loop. This provides fail-fast behavior which is likely desired for staging operations.src/createIgnorer.ts (2)
9-22: LGTM! Clean async conversion of the ignore file reader.The conversion to async is well-executed. The function signature and logic remain intact while properly adopting async/await patterns for file operations.
15-15:Details
❓ Verification inconclusive
Verify that
tryFilebehaves equivalently tofs.existsSyncfor this use case.The replacement of
fs.existsSync(file)withtryFile(file)needs verification. While both check for file existence, they may have different behaviors or return types that could affect the conditional logic.
🏁 Script executed:
#!/bin/bash # Description: Verify the behavior and signature of tryFile function from @pkgr/core # Expected: Documentation or source code showing tryFile behavior matches fs.existsSync # Check the tryFile function documentation/source rg -A 10 -B 5 "export.*tryFile" # Also check for any existing usage patterns of tryFile in the codebase rg "tryFile" -A 3 -B 3Length of output: 755
🏁 Script executed:
#!/bin/bash set -e # Step 1: Retrieve the dist tarball URL for @pkgr/core TARBALL_URL=$(npm view @pkgr/core dist.tarball) echo "Using tarball: $TARBALL_URL" # Step 2: List files in the tarball to locate source of tryFile echo "---- Listing matching paths ----" curl -sL "$TARBALL_URL" | tar -tz | grep -i 'tryFile' # Step 3: Extract and display the implementation files for tryFile echo "---- tryFile source (src) ----" curl -sL "$TARBALL_URL" | tar -xzO package/src/tryFile.* 2>/dev/null || echo "No src/tryFile.* found" echo echo "---- tryFile source (lib/dist) ----" curl -sL "$TARBALL_URL" | tar -xzO package/lib/tryFile.* 2>/dev/null || curl -sL "$TARBALL_URL" | tar -xzO package/dist/index.js | sed -n '1,200p' # If the above fails, you can fall back on inspecting the full index.js or main entry pointLength of output: 431
Confirm
tryFilematchesfs.existsSyncsemanticsOur automated inspection of the published
@pkgr/corepackage didn’t surface thetryFileimplementation, so it’s unclear whether it synchronously returns a boolean exactly likefs.existsSync. Please verify that:
tryFile(filePath)returnstrueif the file exists andfalseotherwise (no thrown errors).- It’s a synchronous check (not an async function or returning a Promise).
- There are no side-effects beyond existence checking.
If any of these differ, adjust the code or replace
tryFilewith a suitable equivalent.src/processFiles.ts (1)
37-37: Correct use ofreturninstead ofcontinuein the mapped function.The change from
continuetoreturnis correct for the new Promise.all(map()) structure, ascontinueis not valid within a mapped function.src/scms/git.ts (2)
84-85: Clean simplification ofgetUnstagedChangedFiles.Converting to an arrow function with implicit return is a nice simplification that maintains the same functionality while reducing code verbosity.
87-104: Excellent consistency with the Hg implementation.The
stageFilesfunction implementation is identical to the one insrc/scms/hg.ts, which ensures consistent behavior across different SCM systems. The batching logic properly handles command line argument limits and provides good error propagation.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f3ed526 in 47 seconds. Click for details.
- Reviewed
288lines of code in9files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/great-panthers-shout.md:1
- Draft comment:
Changeset formatting looks good. Ensure the version bump and changelog notes align with the project guidelines. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/cli.mts:54
- Draft comment:
The new onStageFiles callback is a nice addition for logging batch staging operations. Consider enhancing error logging (perhaps by exposing caught error details) to aid debugging if staging fails. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/createIgnorer.ts:15
- Draft comment:
Switching to async file system operations improves performance. Verify that every consumer of createIgnorer now awaits its result. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/index.ts:110
- Draft comment:
Batch staging is implemented correctly by collecting files and handling them in groups. It might help to log the error details in the catch block when staging fails, to ease troubleshooting. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/processFiles.ts:21
- Draft comment:
Refactoring file processing with Promise.all and async/await is a good performance improvement. Confirm that any individual file errors are managed as intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/scms/git.ts:88
- Draft comment:
Batch staging in git.ts is correctly splitting files into chunks of 100. Consider if additional logging or error handling per batch might be beneficial for debugging large commits. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. src/scms/hg.ts:58
- Draft comment:
The batched staging logic for Mercurial mirrors that of Git. It works well, though you might consider a guard check for empty batches to be extra safe. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. src/types.ts:19
- Draft comment:
The PrettyQuickOptions interface now includes onStageFiles. Ensure that downstream implementations correctly handle this new callback. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_ULiCK80JFFDtN8P5
You can customize
fix #100
close #151
Important
Introduce asynchronous and batch processing for file staging with improved error handling and a new callback for custom actions.
index.ts,scms/git.ts, andscms/hg.ts.onStageFilescallback inPrettyQuickOptionsintypes.tsfor custom actions during staging.cli.mts.createIgnorer.tsandprocessFiles.ts.createMatcher.ts.This description was created by
for f3ed526. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor