fix: impl _final method instead hack pipe event #114
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZipUncompressStream
participant yauzl
User->>ZipUncompressStream: write(chunk)
ZipUncompressStream->>ZipUncompressStream: _write(chunk, _encoding, callback)
ZipUncompressStream->>User: callback()
User->>ZipUncompressStream: end()
ZipUncompressStream->>ZipUncompressStream: _final(callback)
ZipUncompressStream->>yauzl: fromBuffer(buffer, options, [YAUZL_CALLBACK])
yauzl-->>ZipUncompressStream: (err, zipFile)
alt err
ZipUncompressStream->>User: _finalCallback(err)
else success
ZipUncompressStream->>User: _finalCallback(null)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @fengmk2, 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 PR refactors the ZipUncompressStream to correctly handle stream termination and error propagation by implementing the _final method. This change replaces a less robust approach that relied on the pipe event, leading to more reliable and standard stream behavior for uncompressing zip files.
Highlights
- Stream Refactoring: Replaced the custom pipe event handling logic in ZipUncompressStream with a standard Node.js _final method implementation for proper stream termination.
- Error and Completion Handling: Centralized error and stream completion callbacks within ZipUncompressStream to ensure consistent behavior, especially when dealing with yauzl events.
- Debugging Improvements: Integrated util.debuglog for enhanced debugging capabilities within the ZipUncompressStream, providing better visibility into stream operations.
- Build Configuration: Updated .gitignore to include pnpm-lock.yaml, reflecting changes in dependency management practices.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ZIP uncompression stream implementation to use the proper Node.js stream _final method instead of a hacky pipe event listener. The change improves the stream's lifecycle management by implementing the standard writable stream interface.
- Replaces pipe event hack with proper
_finalmethod implementation - Adds debug logging throughout the uncompression process
- Updates error handling to use callback-based approach consistent with stream patterns
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/zip/uncompress_stream.js | Main refactoring to implement _final method and replace pipe event handling with proper stream lifecycle |
| test/zip/uncompress_stream.test.js | Minor formatting change (blank line addition) |
There was a problem hiding this comment.
Code Review
This pull request refactors the ZipUncompressStream to correctly implement the Node.js Writable stream interface by using _write and _final methods. This replaces a previous implementation that relied on a 'pipe' event listener hack. The changes make the stream handling more robust and idiomatic. My review includes one suggestion to remove a stale comment that is no longer valid after this refactoring.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/zip/uncompress_stream.js (2)
42-48:_finalCallbackoverwritten later – can lose debug/error emit wrapper.
When_finalruns it replaces_finalCallbackwith the rawcallback, so subsequent errors (e.g.openReadStreamfailure) skip the debug log and emit path defined here.
Wrap the stream’scallbackinstead of wholesale replacing the property to keep consistent logging/emission behaviour.
74-80: Possible back-pressure / type-safety issue in_write.
•callback()is invoked immediately after buffering; large archives will still accumulate unbounded RAM, but at least honourstream.Writableback-pressure by waiting for high-water-mark drain signals (defercallbackuntilthis.pushequivalent).
•debug('write size: %d', chunk.length)assumeschunkis aBuffer; if a string is written the%dformatter will coerce incorrectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)lib/zip/uncompress_stream.js(4 hunks)test/zip/uncompress_stream.test.js(1 hunks)
🔇 Additional comments (3)
.gitignore (1)
10-10: Ignore entry looks good.
Addingpnpm-lock.yamlis a harmless housekeeping update.test/zip/uncompress_stream.test.js (1)
24-24: No behavioural change detected.
Blank-line addition is purely cosmetic – nothing further.lib/zip/uncompress_stream.js (1)
81-86: Guard empty-input for piped usage.
If nothing was written beforeend(),Buffer.concat([])yields an empty buffer andyauzl.fromBufferwill synchronously error. Detect empty buffer and fail fast with a clear message.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/release.yml (1)
9-13: Add back the NPM_TOKEN secret so npm publish can authenticateWithout explicitly passing
NPM_TOKENinto the reusable workflow, semantic-release will not have the credentials needed to publish to npm and will fail.Please update
.github/workflows/release.ymlto include:jobs: release: name: NPM uses: node-modules/github-actions/.github/workflows/npm-release.yml@master secrets: GIT_TOKEN: ${{ secrets.GIT_TOKEN }} + NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
🧹 Nitpick comments (1)
.github/workflows/nodejs.yml (1)
3-11: Optional: add concurrency to reduce duplicate CI runs across push/pull_request/merge_group.Helps cancel superseded runs on the same ref.
Apply:
name: CI on: push: branches: [ master ] pull_request: branches: [ master ] merge_group: +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/nodejs.yml(1 hunks).github/workflows/release.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/nodejs.yml (2)
10-11: Addition of merge_group trigger: LGTM.This correctly enables runs from GitHub’s merge queue. Ensure merge queue is enabled on the repo/org; otherwise this event won’t fire.
18-18: Node.js 24 is supported by the reusable workflowThe
versioninput is treated as a free-form string and converted to a matrix byaction_repo/scripts/test/index.js, with no hard-coded whitelist. In the Test job we useactions/setup-node@v4withcheck-latest: true, so any valid Node.js release—including 24—will be fetched even if it’s not preinstalled on runners.No further changes needed.
.github/workflows/release.yml (1)
9-10: Reusable npm-release workflow path verifiedThe referenced workflow at
node-modules/github-actions/.github/workflows/npm-release.yml@masterexists and is publicly accessible. No further action is needed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #114 +/- ##
==========================================
+ Coverage 97.76% 97.99% +0.23%
==========================================
Files 19 19
Lines 1028 1048 +20
Branches 273 277 +4
==========================================
+ Hits 1005 1027 +22
+ Misses 23 21 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/zip/index.test.js (1)
29-29: Prefer removing commented-out code or using a toggleable debug helperLeaving commented-out logs around causes drift. Either remove them entirely or switch to a conditional debug helper so you can enable logs locally without code changes.
Option A — remove the dead lines:
- // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log('dest', destFile); - // console.log(destDir);Option B — keep them but behind a debug helper:
- // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log('dest', destFile); + debug('dest', destFile); - // console.log(destDir); + debug(destDir);Add this small helper near the top of the file (or import from a shared test util):
// test/util/debug.js (recommended) or inline in this file const DEBUG = process.env.TEST_DEBUG === '1'; function debug(...args) { if (DEBUG) console.log(...args); }Also applies to: 40-40, 73-73, 85-85, 98-98, 109-109, 120-120, 130-130, 282-282, 293-293
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
test/gzip/file_stream.test.js(4 hunks)test/gzip/index.test.js(3 hunks)test/tar/file_stream.test.js(2 hunks)test/tar/index.test.js(12 hunks)test/tar/stream.test.js(10 hunks)test/tgz/file_stream.test.js(1 hunks)test/tgz/index.test.js(8 hunks)test/tgz/stream.test.js(10 hunks)test/zip/file_stream.test.js(1 hunks)test/zip/index.test.js(10 hunks)test/zip/stream.test.js(10 hunks)test/zip/uncompress_stream.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (10)
- test/zip/file_stream.test.js
- test/tgz/file_stream.test.js
- test/gzip/file_stream.test.js
- test/tar/file_stream.test.js
- test/gzip/index.test.js
- test/tar/stream.test.js
- test/tgz/stream.test.js
- test/tar/index.test.js
- test/tgz/index.test.js
- test/zip/stream.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/zip/uncompress_stream.test.js
🔇 Additional comments (1)
test/zip/index.test.js (1)
29-29: Silencing noisy test logs is a good cleanupCommenting out these console logs keeps CI output clean and focused on failures. Looks good.
Also applies to: 40-40, 73-73, 85-85, 98-98, 109-109, 120-120, 130-130, 282-282, 293-293
[skip ci] ## 2.0.0 (2025-08-09) * fix: impl _final method instead hack pipe event (#114) ([ba52b7b](ba52b7b)), closes [#114](#114) * feat: remove `pump` and raise minimum node to v18 (#113) ([8ac1164](8ac1164)), closes [#113](#113) ### BREAKING CHANGE * Drop Node.js < 18 support Node.js 4 is long EOL, so this PR raises it to 18 as per comment. Node 10 has built-in `stream.pipeline` and `fs.mkdir` with `recursive` option, which can be used instead of `pump` (they are the same). Also 3 tests fail for me but they fail on main branch as well (timeout) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Replaced the external stream piping utility with the native Node.js stream API throughout the codebase and tests. * Updated documentation examples to reflect the new usage of the native stream API. * Replaced external directory creation utilities with native Node.js directory creation methods using recursive options. * Removed obsolete dependencies and increased the minimum required Node.js version to 18.0.0. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
Chores
.gitignoreto excludepnpm-lock.yamlfrom version control.Refactor
Style