Skip to content

FEATURE: automatically downsize large images #10

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
ShashankFC wants to merge 1 commit into
base: image-processing-optimization
Choose a base branch
from
Open

FEATURE: automatically downsize large images #10

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

@codeant-ai codeant-ai Bot Feb 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The client-side upload validation now uses a hardcoded 10MB limit for all uploads, ignoring per-type site settings like max_attachment_size_kb, so attachments larger than the configured limit will pass the browser check and only fail later on the server, leading to inconsistent behavior and confusing errors. You should keep the 10MB allowance only for images (which are downsized server-side) and continue to honor the site setting for attachments. [logic error]

Severity Level: Major ⚠️ 
- ⚠️ Composer uploads allow oversize attachments past browser validation.
- ⚠️ /uploads.json rejects attachments using max_attachment_size_kb server-side.
- ⚠️ Users see late failures and confusing size limit messages.
- ⚠️ Client ignores attachment limit despite upload-selector.js using it.
- ⚠️ utilities-test size expectations regress (`utilities-test.js.es6:52-61`).
Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB;
if (type === 'image') {
// allow larger original images; the server will downsize them
maxSizeKB = 10 * 1024; // 10MB
} else {
// still respect per-type limits for non-image uploads
maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];
}
Steps of Reproduction ✅ 
1. Open the composer and trigger an upload from the reply area; this uses the jQuery
fileupload binding in `app/assets/javascripts/discourse/views/composer.js.es6:333-344`,
where the `fileuploadsubmit` handler at line 339 calls
`Discourse.Utilities.validateUploadedFiles(data.files)`.

2. In `validateUploadedFiles` at
`app/assets/javascripts/discourse/lib/utilities.js:143-160`, upload a non-image file (e.g.
`document.pdf`) so that `type` is set to `'attachment'` by the logic at line 158
(`isAnImage(upload.name) ? 'image' : 'attachment'`), and the function forwards to
`validateUploadedFile(upload, type, bypassNewUserRestriction)` at line 160.

3. Configure the site so that `SiteSetting.max_attachment_size_kb` is significantly lower
than 10 MB (this setting is defined in `config/site_settings.yml:516` and exposed on the
client via `Discourse.SiteSettings.max_attachment_size_kb`, as used in
`app/assets/javascripts/discourse/controllers/upload-selector.js.es6:17` and the utilities
tests at `test/javascripts/lib/utilities-test.js.es6:80`), then attempt to upload an
attachment whose size in kB is greater than `max_attachment_size_kb` but less than or
equal to 10 * 1024.

4. Observe that on the client, `validateUploadedFile` in `utilities.js:163-190` uses the
hardcoded `var maxSizeKB = 10 * 1024;` at line 182 instead of the per-type site setting,
so the attachment passes the browser check and the upload proceeds to `/uploads.json`
(configured in `composer.js.es6:333-335`); the server-side
`UploadsController#create_upload` at `app/controllers/uploads_controller.rb:51-83` and
`Upload` model validations (`app/models/upload.rb:59-169` with
`Validators::UploadValidator`) then enforce the real limit and reject the file, causing a
late failure and inconsistent behavior between client-side validation and server-side
rules.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** app/assets/javascripts/discourse/lib/utilities.js
**Line:** 182:182
**Comment:**
	*Logic Error: The client-side upload validation now uses a hardcoded 10MB limit for all uploads, ignoring per-type site settings like `max_attachment_size_kb`, so attachments larger than the configured limit will pass the browser check and only fail later on the server, leading to inconsistent behavior and confusing errors. You should keep the 10MB allowance only for images (which are downsized server-side) and continue to honor the site setting for attachments.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

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

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={})
Comment on lines 145 to +149

@codeant-ai codeant-ai Bot Feb 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The OptimizedImage.downsize class method is defined twice with different parameter lists, and the second definition overwrites the first, so existing callers like OptimizedImage.downsize(path, path, 100, 100, opts) now pass too many arguments to the final method and will raise an ArgumentError at runtime. You should collapse these into a single downsize implementation that can handle both the old (from, to, width, height, opts) style and the new (from, to, dimensions, opts) style using a flexible argument list. [logic error]

Severity Level: Major ⚠️ 
- ❌ `Jobs::ResizeEmoji` crashes with ArgumentError whenever it runs.
- ⚠️ Custom emoji resize logic in `emoji.rb` job fails.
- ⚠️ Oversized custom emojis may remain unoptimized or too large.
- ⚠️ Background job queue polluted with failed `resize_emoji` jobs.
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={})
def self.downsize(from, to, *args)
dimensions = nil
opts = {}
# Support: downsize(from, to, "80%", opts)
if args.length == 2 && args[0].is_a?(String) && args[1].is_a?(Hash)
dimensions = args[0]
opts = args[1]
# Support: downsize(from, to, max_width, max_height, opts)
elsif args.length == 3
max_width, max_height, opts = args
dimensions = "#{max_width}x#{max_height}"
opts ||= {}
# Support: downsize(from, to, max_width, max_height)
elsif args.length == 2
max_width, max_height = args
dimensions = "#{max_width}x#{max_height}"
else
raise ArgumentError, "wrong number of arguments (#{args.length + 2} for 4..5)"
end
Steps of Reproduction ✅ 
1. Note that `OptimizedImage.downsize` is defined twice in
`app/models/optimized_image.rb:145-151`. In Ruby, the second definition (lines 149-151,
signature `(from, to, dimensions, opts={})`) overwrites the first definition `(from, to,
max_width, max_height, opts={})`.

2. Observe an existing caller in `app/jobs/regular/resize_emoji.rb:5-15`, where
`Jobs::ResizeEmoji#execute` calls `OptimizedImage.downsize(path, path, 100, 100, opts)`
(line 14), i.e., with 5 arguments: `from`, `to`, `max_width`, `max_height`, and `opts`.

3. With the PR's final file state, when the `ResizeEmoji` job runs (enqueued in
`app/models/emoji.rb:73` via `Jobs.enqueue(:resize_emoji, path: path)` after emoji
changes), Ruby dispatches to the *second* `downsize` definition, whose method signature is
`(from, to, dimensions, opts={})` and therefore expects at most 4 arguments.

4. At runtime, calling `OptimizedImage.downsize(path, path, 100, 100, opts)` now passes 5
arguments to a 4-parameter method, causing `ArgumentError: wrong number of arguments
(given 5, expected 3..4)` and the `Jobs::ResizeEmoji` background job fails whenever it
executes.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** app/models/optimized_image.rb
**Line:** 145:149
**Comment:**
	*Logic Error: The `OptimizedImage.downsize` class method is defined twice with different parameter lists, and the second definition overwrites the first, so existing callers like `OptimizedImage.downsize(path, path, 100, 100, opts)` now pass too many arguments to the final method and will raise an `ArgumentError` at runtime. You should collapse these into a single `downsize` implementation that can handle both the old `(from, to, width, height, opts)` style and the new `(from, to, dimensions, opts)` style using a flexible argument list.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

optimize("downsize", from, to, dimensions, opts)
end

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