Conversation
… done so far and provide feedback for Jules to continue.
There was a problem hiding this comment.
Pull Request Overview
This PR adds LZW compression/decompression, integrates it into the CLI, enhances progress reporting with more detailed metrics, and introduces a reusable buffer utility.
- Implements LZW algorithm in
compressor/lzwwith I/O wrappers supporting progress callbacks - Extends CLI and helper types to recognize the new
lzwalgorithm - Improves progress tracking (
ProgressInfo,UpdateProgress, andProgressTrackingReader) - Adds
ResizableBufferfor dynamic in-memory buffering
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/reader_progress.go | New ProgressTrackingReader to wrap an io.Reader and report byte-level progress |
| utils/progress.go | Expanded ProgressInfo and revamped UpdateProgress for batch and file-level metrics |
| utils/helper.go | Added LZW constant to the Algorithm enum |
| utils/cli.go | CLI parsing updated to accept lzw and improved error message |
| utils/buffer.go | New ResizableBuffer type with read/write/reset functionality |
| compressor/lzw/lzw.go | Core LZW encode/decode routines with bit-level writers/readers |
| compressor/lzw/io.go | File-level Zip/Unzip I/O functions, metadata framing, path safety checks, progress hooks |
Comments suppressed due to low confidence (4)
compressor/lzw/lzw.go:56
- The core
compressDatafunction is complex and untested; consider adding unit tests covering small and large inputs, empty streams, and dictionary boundary conditions.
func compressData(input io.Reader, output io.Writer) error {
utils/buffer.go:19
- Public methods on
ResizableBufferlack Go doc comments; adding brief descriptions forWrite,Len,Bytes,Reader, andResetwill improve API clarity.
func (rb *ResizableBuffer) Write(p []byte) (n int, err error) {
compressor/lzw/lzw.go:30
- [nitpick] The
writemethod onbitWritercould be renamed toWriteBitsorWriteCodeto avoid confusion withio.Writer.Write.
func (bw *bitWriter) write(code uint32, numBits uint) error {
utils/reader_progress.go:17
- [nitpick] Function
NewProgressReaderwraps aProgressTrackingReader; renaming it toNewProgressTrackingReaderwould better reflect the underlying type.
func NewProgressReader(r io.Reader, totalSize int64, onProgress func(readBytes int64, totalBytes int64)) io.Reader {
| // TODO: Report progress for skipped file if possible, e.g. | ||
| // if progressCallback != nil { utils.UpdateProgress(utils.ProgressInfo{ ... CurrentFileName: fileName, OriginalMessage: "Skipped unsafe file" ... }, progressCallback) } |
There was a problem hiding this comment.
There’s a TODO left in Unzip for reporting skipped-file progress; consider implementing or removing this stub to avoid incomplete code paths.
| // TODO: Report progress for skipped file if possible, e.g. | |
| // if progressCallback != nil { utils.UpdateProgress(utils.ProgressInfo{ ... CurrentFileName: fileName, OriginalMessage: "Skipped unsafe file" ... }, progressCallback) } | |
| // Report progress for skipped file if progressCallback is provided. | |
| if progressCallback != nil { | |
| utils.UpdateProgress(utils.ProgressInfo{ | |
| CurrentFileName: fileName, | |
| OriginalMessage: "Skipped unsafe file", | |
| PercentComplete: 0, // Skipped files do not contribute to progress percentage. | |
| }, progressCallback) | |
| } |
| fileName := string(fileNameBytes) | ||
| cleanFileName := filepath.Clean(fileName) | ||
|
|
||
| if strings.HasPrefix(cleanFileName, ".."+string(filepath.Separator)) || cleanFileName == ".." || strings.HasPrefix(cleanFileName, string(filepath.Separator)) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") { |
There was a problem hiding this comment.
[nitpick] The path sanitization rejects any backslash or colon, which might block valid Unix filenames; consider using filepath.Rel or filepath.IsAbs for a more robust check against directory traversal.
| if strings.HasPrefix(cleanFileName, ".."+string(filepath.Separator)) || cleanFileName == ".." || strings.HasPrefix(cleanFileName, string(filepath.Separator)) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") { | |
| relPath, err := filepath.Rel(outputDir, filepath.Join(outputDir, cleanFileName)) | |
| if err != nil || filepath.IsAbs(relPath) || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) { |
This pull request introduces the LZW compression algorithm to the project, along with enhancements to progress tracking and utility functionality. The most significant changes include the implementation of LZW compression and decompression, updates to the CLI and helper files to support the new algorithm, and improvements to the progress reporting system.
LZW Compression and Decompression Implementation:
compressor/lzw/io.gowithZipandUnzipfunctions to handle file compression and decompression using the LZW algorithm. These functions include detailed progress reporting and safety checks for file paths during decompression.compressor/lzw/lzw.gowith corecompressDataanddecompressDatafunctions implementing the LZW algorithm, including dictionary management and bit-level operations.CLI and Helper Updates:
utils/cli.goto support the LZW algorithm in the CLI by adding it as a valid option and improving error messages for unsupported algorithms.LZWto theAlgorithmtype inutils/helper.goto define it as a supported compression algorithm.Progress Tracking Enhancements:
ProgressInfoinutils/progress.goto include more granular details, such as file-specific and batch-level progress metrics.UpdateProgressinutils/progress.goto calculate progress based on bytes processed and construct detailed progress messages, including file names and sizes.Utility Improvements:
utils/buffer.gowith theResizableBuffertype to provide a dynamic buffer for handling compressed data efficiently.