Skip to content

FEATURE: automatically downsize large images #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
everettbu wants to merge 1 commit into
base: image-processing-optimization
Choose a base branch
from
Open

FEATURE: automatically downsize large images #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/assets/javascripts/discourse/lib/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ Discourse.Utilities = {

// check file size
var fileSizeKB = file.size / 1024;
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];
var maxSizeKB = 10 * 1024; // 10MB

@coderabbitai coderabbitai Bot Jul 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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.

if (fileSizeKB > maxSizeKB) {
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return false;
Expand Down Expand Up @@ -243,7 +243,7 @@ Discourse.Utilities = {

// 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

@coderabbitai coderabbitai Bot Jul 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.

bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return;

Expand Down
11 changes: 10 additions & 1 deletion app/controllers/uploads_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,23 @@ def create_upload(type, file, url)
begin
# API can provide a URL
if file.nil? && url.present? && is_api?
tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil
tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil
filename = File.basename(URI.parse(url).path)
else
tempfile = file.tempfile
filename = file.original_filename
content_type = file.content_type
end

# 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
Comment on lines +63 to +70

@coderabbitai coderabbitai Bot Jul 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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:

  1. Verify that OptimizedImage.downsize accepts "80%" as a dimension parameter
  2. Add validation that downsizing actually reduces file size
  3. 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.rb

Length 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.rb

Length 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.rb

Length 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
       end

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


upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)

if upload.errors.empty? && current_user.admin?
Expand Down
17 changes: 8 additions & 9 deletions app/models/optimized_image.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,24 @@ def self.downsize_instructions_animated(from, to, dimensions, opts={})
end

def self.resize(from, to, width, height, opts={})
optimize("resize", from, to, width, height, opts)
optimize("resize", from, to, "#{width}x#{height}", opts)
end

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
Comment on lines 145 to +151

@coderabbitai coderabbitai Bot Jul 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

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)
   end

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

Suggested change
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.


def self.optimize(operation, from, to, dimensions, opts={})
method_name = "#{operation}_instructions"
method_name += "_animated" if !!opts[:allow_animation] && from =~ /\.GIF$/i
instructions = self.send(method_name.to_sym, from, to, dim, opts)
instructions = self.send(method_name.to_sym, from, to, dimensions, opts)
convert_with(instructions, to)
end

def self.dimensions(width, height)
"#{width}x#{height}"
end

def self.convert_with(instructions, to)
`#{instructions.join(" ")} &> /dev/null`
return false if $?.exitstatus != 0
Expand Down