Implement zip-based image delivery and improve code quality#21
Merged
Conversation
This change consolidates PDF conversion output into a single zip file, simplifying client integration and reducing API response complexity. Additionally, code quality improvements reduce technical debt and improve maintainability. Key Changes: - Implement zip-based delivery: All converted images are packaged into a single zip file and uploaded to the destination URL - Extract ZipBuilder class: Separate zip creation logic for better code organization (A-rated with 0 smells) - Improve code quality: Extract duplicate method calls across 6 modules, reducing complexity and eliminating 9 code smells - Upgrade PdfConverter: Improved from C to B rating in RubyCritic - Add documentation: Comprehensive comments for RetryError class Benefits: - Simpler client integration: Single zip download vs. multiple images - Improved code quality: RubyCritic score improved from 85.98 to 87.92 - Better maintainability: Reduced code duplication and complexity - Excellent test coverage: 99.83% line coverage, all 609 unit tests passing Technical Details: - Added rubyzip gem for zip file creation - Updated API response: 'images' field now returns single zip URL - Updated documentation: CLAUDE.md and README.md reflect new behavior - Extracted duplicate method calls in AwsConfig, RequestValidator, JwtAuthenticator, PdfConverter, and ImageUploader 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR successfully implements zip-based image delivery for the PDF conversion service, replacing the previous approach of uploading individual images with a single zip file upload. The changes include significant code quality improvements through extraction of duplicate code and creation of a dedicated ZipBuilder utility class.
Key Changes:
- Implemented zip file creation and upload workflow using the
rubyzipgem - Refactored duplicate variable references across multiple modules (AwsConfig, RequestValidator, JwtAuthenticator, PdfConverter, ImageUploader)
- Created new
ZipBuilderclass for zip file generation with comprehensive test coverage - Updated API response format from array of image URLs to single zip URL
- Enhanced documentation across README.md and CLAUDE.md with zip file usage instructions
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| prompts/completed/001-zip-images-stream-to-s3.md | Documents the implementation requirements and approach for the zip-based delivery feature |
| pdf_converter/lib/zip_builder.rb | New utility class for creating zip files in memory from image files with clean API |
| pdf_converter/spec/lib/zip_builder_spec.rb | Comprehensive test suite for ZipBuilder with 100% coverage of zip creation scenarios |
| pdf_converter/app/image_uploader.rb | Refactored to use ZipBuilder and upload single zip file instead of individual images |
| pdf_converter/spec/app/image_uploader_spec.rb | Updated tests to verify zip upload behavior instead of batch image uploads |
| pdf_converter/app.rb | Modified to handle zip URL instead of uploaded URLs array in response |
| pdf_converter/spec/app_spec.rb | Updated tests to expect zip_url instead of uploaded_urls in responses |
| pdf_converter/app/response_builder.rb | Changed success_response to accept zip_url parameter instead of uploaded_urls array |
| pdf_converter/spec/app/response_builder_spec.rb | Updated response builder tests for new zip-based response format |
| pdf_converter/spec/integration/localstack_integration_spec.rb | Updated integration tests to verify zip file upload to S3 |
| pdf_converter/lib/retry_handler.rb | Added documentation comments to RetryError class |
| pdf_converter/lib/aws_config.rb | Extracted ENV['AWS_ENDPOINT_URL'] to local variable to reduce duplication |
| pdf_converter/app/request_validator.rb | Extracted event['body'] and body['webhook'] to variables |
| pdf_converter/app/jwt_authenticator.rb | Extracted validation_result[:error] and ENV['AWS_ENDPOINT_URL'] to variables |
| pdf_converter/app/pdf_converter.rb | Extracted temp_pdf.path and page_index + 1 to variables for reuse |
| pdf_converter/Gemfile | Added rubyzip (~> 2.3) dependency |
| pdf_converter/Gemfile.lock | Locked rubyzip at version 2.4.1 |
| README.md | Updated with zip file delivery examples and destination URL guidance |
| CLAUDE.md | Updated API specification to reflect zip-based response format |
| .claude/settings.local.json | Added SlashCommand permission for running prompts |
| pdf_converter/spec/.rspec_status | Updated test execution results showing all 609 tests passing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements zip-based image delivery for PDF conversions and includes significant code quality improvements across the codebase.
🎯 Key Changes
Zip-Based Delivery
imagesfield now returns a single zip URL instead of an arrayCode Quality Improvements
ZipBuilderclass with A rating and 0 code smellsPdfConverterfrom C to B rating in RubyCriticRetryErrorclass📊 Metrics
Test Coverage:
Code Quality:
🔧 Technical Details
New Dependencies:
rubyzipgem (~> 2.3) for zip file creationAPI Changes:
imagesfield changed from array to string (single zip URL).zipfileNew Files:
pdf_converter/lib/zip_builder.rb- A-rated zip creation utilitypdf_converter/spec/lib/zip_builder_spec.rb- Comprehensive test suiteCode Quality Improvements:
ENV['AWS_ENDPOINT_URL']to variables (3 duplicates → 1)event['body']to variable (4 duplicates → 1)body['webhook']to variable (2 duplicates → 1)validation_result[:error]to variable (2 duplicates → 1)temp_pdf.pathto variable (2 duplicates → 1)page_index + 1topage_numbervariable (multiple duplicates → reused)📚 Documentation Updates
CLAUDE.mdAPI specification with zip file examplesREADME.mdwith zip-based usage instructions.zipfilesRetryErrorclass✅ Benefits
🧪 Test Plan
🤖 Generated with Claude Code