Skip to content

Conversation

@hcfw007
Copy link
Contributor

@hcfw007 hcfw007 commented Apr 15, 2025

重新开了一个PR,因为在 juzibot 的 repo 上开了一个 main-to-origin 分支专门用于合并,以避免解决冲突时意外 update 了 juzibot 的 main 分支。

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @hcfw007, 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!

Summary of Changes

This pull request updates the file-box package to version 1.7.16 and introduces several enhancements and fixes. Key changes include the addition of environment variables for configuring HTTP request and response timeouts, disabling slice downloading, and setting HTTP chunk size. The update also includes a fix for handling errors during stream writing and improvements to HTTP header processing and chunk downloading. Additionally, the package name has been changed to @juzi/file-box.

Highlights

  • Environment Variables Configuration: Introduces environment variables FILEBOX_HTTP_REQUEST_TIMEOUT, FILEBOX_HTTP_RESPONSE_TIMEOUT, NO_SLICE_DOWN, and HTTP_CHUNK_SIZE to allow users to configure HTTP request timeouts, disable slice downloading, and set HTTP chunk size.
  • Error Handling Improvement: Fixes an issue where errors during stream writing were not properly handled, ensuring that the write stream is closed before rejecting the promise.
  • HTTP Header Processing Enhancement: Improves HTTP header processing by including headers and proxy URL when making HTTP HEAD requests, allowing for more flexible and configurable requests.
  • Chunk Downloading Enhancement: Adds chunk downloading functionality to download files in multiple parts, improving reliability and supporting breakpoint re-downloading.
  • Package Name Change: Changes the package name from file-box to @juzi/file-box.

Changelog

Click here to see the changelog
  • .vscode/settings.json
    • Removed the file.
  • README.md
    • Added documentation for the new environment variables.
    • Updated the history section to reflect the changes in version 1.7.
  • package.json
    • Updated the package name to @juzi/file-box.
    • Updated the package version to 1.7.16.
    • Added new dependencies and devDependencies, including @types/node, @typescript-eslint/eslint-plugin, eslint-plugin-n, https-proxy-agent, tap, and typescript.
    • Modified the test script to include a timeout.
  • src/config.ts
    • Added constants for HTTP request timeout, HTTP response timeout, disabling slice downloading, HTTP chunk size, and ready retry, configurable via environment variables.
  • src/file-box.ts
    • Added READY_RETRY import from ./config.js.
    • Added headers property to the FileBox constructor.
    • Added url getter to return the remote URL.
    • Added proxyUrl property and useProxyUrl method to set a proxy URL.
    • Modified the ready method to retry on failure.
    • Modified the _syncUrlMetadata method to include headers and proxy URL when making HTTP HEAD requests.
    • Modified the _transformUrlToStream method to include headers and proxy URL when creating the HTTP stream.
    • Modified the pipe method to improve error handling.
    • Added error handling to the write stream in pipe method.
  • src/misc.spec.ts
    • Added a test case for httpStream to verify chunk downloading.
  • src/misc.ts
    • Added imports for various modules, including assert, crypto, events, fs, http, https, os, path, stream, and url.
    • Added constants for HTTP chunk size, HTTP request timeout, HTTP response timeout, and disabling slice downloading.
    • Modified the httpHeadHeader function to include headers and proxy URL when making HTTP HEAD requests.
    • Modified the httpStream function to support chunk downloading and proxy URLs.
    • Added functions for downloading files in chunks, setting proxy options, and converting streams to buffers.
    • Refactored httpStream to use fetch and downloadFileInChunks for improved reliability and flexibility.
  • src/mod.ts
    • Exported all members from './misc.js'.
  • src/pure-functions/sized-chunk-transformer.ts
    • Corrected a typo in Buffer.concat.
  • src/urn-registry/uniform-resource-name-registry.ts
    • Corrected typos in array destructuring.
  • src/version.ts
    • Removed an extra blank line.
  • tests/fixtures/smoke-testing.ts
    • Updated the import path for file-box to @juzi/file-box.
  • tests/network-timeout.spec.ts
    • Added a new test file to verify network timeout functionality.
  • tsconfig.json
    • Set noUnusedLocals to false.
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 is currently in preview and 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 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.


A file in a box,
Across the network it flies,
Data finds its home.

Footnotes

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request introduces several updates and enhancements to the file-box library, including environment variable configurations, improved HTTP handling with proxy support and chunk downloading, and dependency updates. The changes seem well-structured and address potential network timeout issues. However, some areas require closer examination and potential refinement.

Summary of Findings

  • Error Handling in Chunk Downloading: The chunk downloading logic includes retry mechanisms, but the error handling could be more robust to ensure temporary file cleanup and consistent error reporting.
  • Redundant Protocol Check: The protocol check in httpStream might be redundant as it's already performed in fetch. Consider removing the check in httpStream to simplify the logic.
  • Inconsistent Naming: The naming of the environment variable FILE_BOX_READY_RETRY in src/config.ts and its usage in src/file-box.ts is inconsistent. Standardize the naming for clarity.

Merge Readiness

The pull request includes significant improvements and new features. However, the identified issues related to error handling, redundant checks, and naming inconsistencies should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Addressing the critical and high severity comments is highly recommended before merging.

Comment on lines +186 to +189
void rm(tmpFile, { force: true })
writeStream.close()
throw new Error(`Download file with chunk failed! ${err.message}`, { cause: err })
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The temporary file is removed only when the retry count exceeds the limit. If an error occurs during chunk download but retries are still available, the temporary file should be cleaned up to avoid accumulating orphaned files. Additionally, consider logging the error that caused the retry.

      const err = error as Error
      console.error(`Download file with chunk failed (attempt ${retries}): ${err.message}`)
      if (--retries <= 0) {
        void rm(tmpFile, { force: true })
        writeStream.close()
        throw new Error(`Download file with chunk failed! ${err.message}`, { cause: err })

export const HTTP_CHUNK_SIZE = Number(process.env['FILEBOX_HTTP_CHUNK_SIZE'])
|| 1024 * 512

export const READY_RETRY = Number(process.env['FILE_BOX_READY_RETRY']) || 3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The environment variable name FILE_BOX_READY_RETRY is inconsistent with other variable names (using underscores instead of camel case). Consider renaming it to FILEBOX_READY_RETRY to maintain consistency.

Suggested change
export const READY_RETRY = Number(process.env['FILE_BOX_READY_RETRY']) || 3
export const READY_RETRY = Number(process.env['FILEBOX_READY_RETRY']) || 3

Comment on lines +637 to +640
tryCount ++
if (tryCount >= READY_RETRY) {
throw e
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The retry mechanism is implemented, but consider adding a delay between retries to avoid overwhelming the server. Also, logging the error before retrying would be helpful for debugging.

      } catch (e) {
        tryCount ++
        console.error(`FileBox: ready() failed (attempt ${tryCount}/${READY_RETRY}): ${e}`)
        await new Promise(resolve => setTimeout(resolve, 1000)) // Add a 1-second delay
        if (tryCount >= READY_RETRY) {
          throw e
        }

Comment on lines 387 to +390
md5: obj.md5,
name: obj.name,
size: obj.size,
headers: obj.headers,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a type definition for the headers property in the FileBoxOptionsUrl interface to ensure type safety when accessing it.

        fileBox = this.fromUrl(obj.url, {
          md5: obj.md5,
          name: obj.name,
          size: obj.size,
          headers: obj.headers,
        })

@huan
Copy link
Owner

huan commented Apr 27, 2025

This is a huge PR, and it’s an anti-pattern.

I’d really appreciate it if you could follow the same collaboration best practices we use in the Wechaty community: create an issue first, break down the new features into the smallest possible pieces, describe each feature in its own issue, and open a separate PR for each one.

Thank you!

@huan huan requested a review from Copilot April 27, 2025 07:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR moves merging activity to a dedicated main-to-origin branch to avoid inadvertently updating the main branch in the juzibot repository, while also introducing several refactorings and enhancements.

  • Updated network timeout tests and smoke testing fixtures
  • Significant refactoring in network handling (src/misc.ts) including support for proxy settings and chunked downloads
  • Documentation, configuration, and workflow adjustments to reflect these changes

Reviewed Changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/network-timeout.spec.ts Added tests for HTTP request/response timeout scenarios with sinon fake timers
tests/fixtures/smoke-testing.ts Adjusted import to use the new module location
src/version.ts Minor cleanup related to version management
src/mod.ts Exported misc module to surface additional utility functions
src/misc.ts Refactored HTTP streaming, added proxy support, and introduced chunked download logic
src/misc.spec.ts Added test verifying HTTP streaming in chunked mode
src/file-box.ts Enhanced proxy support and ready method retry logic
src/config.ts Updated environment variable definitions and defaults
README.md Updated documentation regarding environment variables and deprecations
.github/workflows/npm.yml Updated NPM publishing workflow token
Files not reviewed (2)
  • .vscode/settings.json: Language not supported
  • package.json: Language not supported
Comments suppressed due to low confidence (3)

src/misc.ts:50

  • [nitpick] The error message uses the already decremented 'REDIRECT_TTL' value, which may be confusing. Consider storing the original TTL value and reflecting that in the error message.
throw new Error(`ttl expired! too many(>${REDIRECT_TTL}) 302 redirection.`)

src/config.ts:15

  • [nitpick] The environment variable 'FILE_BOX_READY_RETRY' is inconsistent with the naming convention used for other variables (e.g. 'FILEBOX_HTTP_REQUEST_TIMEOUT'). Consider renaming it for consistency.
export const READY_RETRY = Number(process.env['FILE_BOX_READY_RETRY']) || 3

src/misc.ts:125

  • Consider propagating the error from the request's error event instead of simply destroying the request to avoid losing important error details.
.on('error', () => { req.destroy() })

@hcfw007 hcfw007 closed this Apr 27, 2025
@hcfw007 hcfw007 deleted the main-to-origin branch May 22, 2025 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants