FEATURE: automatically downsize large images #1
Conversation
WalkthroughThe changes standardize file size limits to a fixed 10 MB in both JavaScript and server-side Ruby code, eliminating dynamic site setting lookups. In image upload processing, a new downsizing loop is introduced to shrink oversized images automatically. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Server
participant OptimizedImage
User->>Frontend: Uploads file
Frontend->>Frontend: Validate file size (10 MB limit)
alt File size valid
Frontend->>Server: Send file
Server->>Server: If image and size > max, downsize up to 5 times
loop Downsizing
Server->>OptimizedImage: downsize("WIDTHxHEIGHT")
OptimizedImage-->>Server: Return downsized image
end
Server->>Frontend: Upload success/failure
else File size invalid
Frontend->>User: Show error (max 10 MB)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@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: 4
🧹 Nitpick comments (1)
app/controllers/uploads_controller.rb (1)
55-55: Fix rescue modifier style and ensure consistent error handling.The rescue modifier should be avoided for better readability and error handling.
- 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 file from URL: #{url}, error: #{e.message}") + tempfile = nil + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/discourse/lib/utilities.js(2 hunks)app/controllers/uploads_controller.rb(1 hunks)app/models/optimized_image.rb(1 hunks)
🧰 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)
🪛 RuboCop (1.76.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)
🪛 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)
🔇 Additional comments (3)
app/models/optimized_image.rb (2)
142-142: Good refactoring to standardize dimension parameter handling.The change to use dimension strings consistently across the API is a clean simplification that reduces parameter count and standardizes the interface.
153-156: Clean updates to support the new dimension string format.The changes to
optimizemethod and instruction method calls properly handle the new dimension string parameter format.app/controllers/uploads_controller.rb (1)
55-55: Good consistency with frontend 10MB limit.The hardcoded 10MB limit for URL downloads aligns well with the frontend validation changes, ensuring consistent behavior across client and server.
| // check file size | ||
| var fileSizeKB = file.size / 1024; | ||
| var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb']; | ||
| var maxSizeKB = 10 * 1024; // 10MB |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of hardcoding the file size limit.
Replacing the dynamic site setting with a hardcoded 10MB limit removes configurability. This could be problematic for deployments that need different size limits.
If this change is intentional to standardize limits, ensure it's documented and that server-side limits are also updated consistently. Otherwise, consider keeping the dynamic lookup:
- var maxSizeKB = 10 * 1024; // 10MB
+ var maxSizeKB = (type === 'image') ?
+ Discourse.SiteSettings.max_image_size_kb :
+ Discourse.SiteSettings.max_attachment_size_kb;📝 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.
| var maxSizeKB = 10 * 1024; // 10MB | |
| var maxSizeKB = (type === 'image') ? | |
| Discourse.SiteSettings.max_image_size_kb : | |
| Discourse.SiteSettings.max_attachment_size_kb; |
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/lib/utilities.js at line 182, the file size
limit is hardcoded to 10MB, removing configurability. To fix this, revert to
using the dynamic site setting for the file size limit instead of a fixed value,
ensuring the limit can be configured per deployment. If the hardcoded limit is
intentional, add documentation explaining this decision and verify that
server-side limits are updated to match.
| // entity too large, usually returned from the web server | ||
| case 413: | ||
| var maxSizeKB = Discourse.SiteSettings.max_image_size_kb; | ||
| var maxSizeKB = 10 * 1024; // 10 MB |
There was a problem hiding this comment.
Fix variable declaration scope in switch case.
The variable declaration inside the switch case should be wrapped in a block to prevent potential access issues from other cases.
case 413:
- var maxSizeKB = 10 * 1024; // 10 MB
- bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
- return;
+ {
+ var maxSizeKB = 10 * 1024; // 10 MB
+ bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
+ return;
+ }📝 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.
| 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; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[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)
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/lib/utilities.js at line 246, the variable
maxSizeKB is declared directly inside a switch case without a block, which can
cause scope leakage or conflicts with other cases. To fix this, wrap the
variable declaration inside curly braces {} within the switch case to create a
block scope, ensuring maxSizeKB is scoped only to that case.
| # 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.
💡 Verification agent
🧩 Analysis chain
Verify the downsizing implementation and add error handling.
The automatic image downsizing feature looks good, but there are a few concerns to address:
- Verify that
OptimizedImage.downsizeaccepts "80%" as a dimension parameter - Add validation that downsizing actually reduces file size
- Handle potential downsizing failures
Consider adding error handling and size validation:
# 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
+ original_size = tempfile.size
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
- OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ success = OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ break unless success
+
+ # Ensure the file size actually decreased
+ new_size = File.size(tempfile.path)
+ break if new_size >= tempfile.size
+ tempfile.size = new_size
+
attempt -= 1
end
end🏁 Script executed:
#!/bin/bash
# Check if OptimizedImage.downsize supports percentage dimensions
ast-grep --pattern 'def $_ downsize($$$)
$$$
end'
# Also check the downsize_instructions method to see how it handles percentages
ast-grep --pattern 'def $_ downsize_instructions($$$)
$$$
end'Length of output: 121
🏁 Script executed:
#!/bin/bash
set -e
# Locate where OptimizedImage is defined
rg -n "class OptimizedImage"
# As a fallback, check for module definitions
rg -n "module OptimizedImage"
# Search for any method definitions named 'downsize'
rg -n -C2 "def downsize"
# Also check for 'downsize_instructions' definitions
rg -n -C2 "def downsize_instructions"
# List files and contexts where 'OptimizedImage.downsize' is referenced
rg -n -C2 "OptimizedImage\.downsize"Length of output: 145
🏁 Script executed:
#!/bin/bash
set -e
# Inspect the downsize and downsize_instructions method definitions
rg -n -C5 "def downsize" app/models/optimized_image.rb
rg -n -C5 "def downsize_instructions" app/models/optimized_image.rbLength of output: 65
🏁 Script executed:
#!/bin/bash
set -e
# Search for class method definitions for downsize
rg -n -C3 "def self\.downsize"
# Search for any occurrence of downsize in the model, to catch alternate definitions
rg -n -C3 "downsize" app/models/optimized_image.rb
# Search for class method definitions for downsize_instructions
rg -n -C3 "def self\.downsize_instructions"
# Search for any occurrence of downsize_instructions in the model
rg -n -C3 "downsize_instructions" app/models/optimized_image.rbLength of output: 3486
🏁 Script executed:
#!/bin/bash
set -e
# Show the full implementation of the optimize method to determine its return behavior
rg -n -C20 "def self\.optimize" app/models/optimized_image.rbLength of output: 1401
Add error handling and size validation for image downsizing
I’ve confirmed that OptimizedImage.downsize(..., "80%", …) maps to the def self.downsize(from, to, dimensions, opts={}) overload and ultimately uses convert_with, which returns true/false based on success. To ensure we don’t loop infinitely or accept a failed or no-op resize, please update the controller loop as follows:
• Track and update the temp file’s size after each attempt
• Break out if downsize returns false or yields no further reduction
Suggested diff for app/controllers/uploads_controller.rb (around lines 63–70):
# 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
+ original_size = tempfile.size
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
- OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ success = OptimizedImage.downsize(
+ tempfile.path, tempfile.path,
+ "80%",
+ allow_animation: SiteSetting.allow_animated_thumbnails
+ )
+ break unless success
+
+ new_size = File.size(tempfile.path)
+ break if new_size >= tempfile.size
+ tempfile.size = new_size
attempt -= 1
end
endThis ensures the loop exits on failures and only continues while actual downsizing occurs.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/uploads_controller.rb around lines 63 to 70, the image
downsizing loop lacks error handling and does not update the tempfile size after
each attempt, risking infinite loops or accepting failed resizes. Modify the
loop to capture the return value of OptimizedImage.downsize and break if it
returns false, indicating failure. Also, after each successful downsize, refresh
the tempfile size to check if further downsizing is needed, exiting the loop if
no size reduction occurs.
| def self.downsize(from, to, max_width, max_height, opts={}) | ||
| optimize("downsize", from, to, max_width, max_height, opts) | ||
| optimize("downsize", from, to, "#{max_width}x#{max_height}", opts) | ||
| end | ||
|
|
||
| def self.optimize(operation, from, to, width, height, opts={}) | ||
| dim = dimensions(width, height) | ||
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end |
There was a problem hiding this comment.
Critical: Duplicate method definitions create ambiguity.
Ruby will resolve the duplicate downsize method definitions by using the last one defined (line 149), which could break existing code that calls downsize with separate max_width and max_height parameters.
Choose one of these solutions:
Option 1 (Recommended): Remove the first method and update all callers
- def self.downsize(from, to, max_width, max_height, opts={})
- optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
- end
-
def self.downsize(from, to, dimensions, opts={})
optimize("downsize", from, to, dimensions, opts)
endOption 2: Use different method names
def self.downsize(from, to, max_width, max_height, opts={})
optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
end
- def self.downsize(from, to, dimensions, opts={})
+ def self.downsize_with_dimensions(from, to, dimensions, opts={})
optimize("downsize", from, to, dimensions, opts)
end📝 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.
| def self.downsize(from, to, max_width, max_height, opts={}) | |
| optimize("downsize", from, to, max_width, max_height, opts) | |
| optimize("downsize", from, to, "#{max_width}x#{max_height}", opts) | |
| end | |
| def self.optimize(operation, from, to, width, height, opts={}) | |
| dim = dimensions(width, height) | |
| def self.downsize(from, to, dimensions, opts={}) | |
| optimize("downsize", from, to, dimensions, opts) | |
| end | |
| def self.downsize(from, to, dimensions, opts={}) | |
| optimize("downsize", from, to, dimensions, opts) | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[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)
🤖 Prompt for AI Agents
In app/models/optimized_image.rb around lines 145 to 151, there are two
conflicting definitions of the downsize method causing ambiguity. To fix this,
remove the first downsize method that takes max_width and max_height as separate
parameters, and update all calls to downsize to use the second method that
accepts a single dimensions string parameter instead. This will ensure
consistent method usage and prevent unexpected behavior.
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
2 participants
Test 1
Summary by CodeRabbit
New Features
Refactor
Chores