FEATURE: automatically downsize large images#1
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughFixed 10 MB client-side upload validation limit and implemented server-side image downscaling. Added OptimizedImage.downsize() method and refactored the optimize() API to accept a single dimensions parameter instead of separate width/height values. Changes
Sequence DiagramsequenceDiagram
participant User
participant UploadsController as Controller
participant OptimizedImage
participant FileSystem as File System
User->>UploadsController: Upload/download image
UploadsController->>UploadsController: Check if image exceeds<br/>max_image_size_kb
alt Image exceeds limit
loop Up to 5 iterations
UploadsController->>OptimizedImage: downsize(file, dimensions)<br/>with 80% quality
OptimizedImage->>FileSystem: Resize image in-place
OptimizedImage-->>UploadsController: Updated file size
UploadsController->>UploadsController: Check if within limit
end
end
UploadsController->>FileSystem: Save final image
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In @app/controllers/uploads_controller.rb:
- Line 72: The call to Upload.create_for uses tempfile.size which may be stale
after the downsizing loop; replace that argument with the actual on-disk size by
calling File.size(tempfile.path) (and ensure the tempfile is flushed/rewound if
needed) when invoking Upload.create_for(current_user.id, tempfile, filename,
File.size(tempfile.path), content_type: content_type, image_type: type) so the
stored size reflects the post-downsize file; locate the call to
Upload.create_for in the uploads controller and update the size parameter
accordingly.
🧹 Nitpick comments (2)
app/assets/javascripts/discourse/lib/utilities.js (1)
245-248: Wrap switch case declaration in a block to prevent scope leakage.The
var maxSizeKBdeclaration inside thecase 413block can be accessed by other switch clauses due to JavaScript's hoisting behavior. Wrap in a block for proper scoping.♻️ Proposed fix
// entity too large, usually returned from the web server - case 413: - var maxSizeKB = 10 * 1024; // 10 MB + case 413: { + var maxSizeKB = 10 * 1024; // 10 MB bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); return; + }app/controllers/uploads_controller.rb (1)
54-56: Avoid rescue modifier — exceptions are silently swallowed.The
rescue nilmodifier hides all download failures, making debugging difficult. Use explicit error handling to log failures.♻️ Proposed fix
if file.nil? && url.present? && is_api? - tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil + begin + tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") + rescue => e + Rails.logger.warn("Failed to download upload from URL #{url}: #{e.message}") + tempfile = nil + end filename = File.basename(URI.parse(url).path)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/discourse/lib/utilities.jsapp/controllers/uploads_controller.rbapp/models/optimized_image.rb
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/uploads_controller.rb (1)
app/models/optimized_image.rb (2)
downsize(145-147)downsize(149-151)
🪛 Biome (2.1.2)
app/assets/javascripts/discourse/lib/utilities.js
[error] 246-246: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 RuboCop (1.82.1)
app/controllers/uploads_controller.rb
[convention] 55-55: Avoid using rescue in its modifier form.
(Style/RescueModifier)
app/models/optimized_image.rb
[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.
(Lint/DuplicateMethods)
🔇 Additional comments (4)
app/models/optimized_image.rb (3)
141-143: LGTM!The
resizemethod correctly constructs the dimensions string and delegates tooptimize.
153-158: LGTM!The
optimizemethod signature update to accept a unifieddimensionsparameter is clean and the dynamic method dispatch for animated vs non-animated instructions works correctly.
145-151: Critical: Duplicate method definitions — seconddownsizeoverwrites the first.Ruby does not support method overloading. The second
downsizedefinition (lines 149-151) completely replaces the first one (lines 145-147). Any code calling the 4-argument signaturedownsize(from, to, max_width, max_height, opts)will fail with anArgumentError. Verify that all call sites use only the dimensions-string signature to confirm this change is intentional.app/assets/javascripts/discourse/lib/utilities.js (1)
182-186: Hard-coded upload limit removes per-type flexibility.The change from
Discourse.SiteSettings['max_' + type + '_size_kb']to a fixed 10,240 KB limit removes the ability to configure different size thresholds for images versus attachments. While this aligns with the server-side 10 MB fixed limit, consider verifying that no existing administrator configurations depend on per-type limits and that the old settings have been fully deprecated.
| # allow users to upload large images that will be automatically reduced to allowed size | ||
| if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | ||
| attempt = 5 | ||
| while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes | ||
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | ||
| attempt -= 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does ImageMagick convert -resize accept percentage values like "80%"?
💡 Result:
Yes. -resize accepts percentage values (e.g. magick input.png -resize 80% output.png). [1][2]
Note: on Windows batch files you must escape % by doubling it (use %%). [1]
🏁 Script executed:
# Search for OptimizedImage.downsize implementation
fd -type f -name "*.rb" | xargs rg -l "OptimizedImage" | head -20Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 226
🏁 Script executed:
# Find the downsize method definition
rg "def downsize" -A 15Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 67
Improve error handling and file size tracking in the downsize loop.
The downsizing logic has several issues:
-
No error handling: If
OptimizedImage.downsizefails, the loop continues without logging or indication of failure. -
File size not refreshed after modification:
tempfile.sizemay not reflect the updated file size after in-place modification. UseFile.size(tempfile.path)to get the current file size on disk. -
In-place file modification: Using the same path for source and destination carries inherent risk if the operation fails mid-write, potentially corrupting the file.
The "80%" dimension string is a valid ImageMagick resize format and will correctly reduce dimensions by 20%.
🐛 Proposed fix
# allow users to upload large images that will be automatically reduced to allowed size
if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
- while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
- OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ while attempt > 0 && File.size(tempfile.path) > SiteSetting.max_image_size_kb.kilobytes
+ unless OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ Rails.logger.warn("Failed to downsize image #{filename}, attempt #{6 - attempt}")
+ break
+ end
attempt -= 1
end
end| end | ||
| end | ||
|
|
||
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) |
There was a problem hiding this comment.
File size passed to Upload.create_for may be stale after downsizing.
After the downsizing loop modifies the file, tempfile.size may not reflect the new size. Use File.size(tempfile.path) to get the accurate post-downsize file size.
🐛 Proposed fix
- upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)
+ upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) | |
| upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type) |
🤖 Prompt for AI Agents
In @app/controllers/uploads_controller.rb at line 72, The call to
Upload.create_for uses tempfile.size which may be stale after the downsizing
loop; replace that argument with the actual on-disk size by calling
File.size(tempfile.path) (and ensure the tempfile is flushed/rewound if needed)
when invoking Upload.create_for(current_user.id, tempfile, filename,
File.size(tempfile.path), content_type: content_type, image_type: type) so the
stored size reflects the post-downsize file; locate the call to
Upload.create_for in the uploads controller and update the size parameter
accordingly.
2 participants
Test 1
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#1