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
+20 −12
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

@zach-source zach-source Jul 29, 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.

[high] Client-side size check hardcoded to 10 MB, bypassing per-type site settings

The original code used Discourse.SiteSettings['max_' + type + '_size_kb'] to enforce per-type limits (image vs. attachment). The diff replaces this with a hardcoded 10 * 1024 (10 MB) for all types. This means: (1) non-image file types (attachments) that have a site setting smaller than 10 MB are no longer client-side validated — users see no error until the server rejects them; (2) the maxSizeKB value passed to the file_too_large i18n message will always show "10240 KB" regardless of the actual configured limit, confusing users.

Suggestion: Restore the dynamic lookup: var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb']; — or, if the intent is to allow large image uploads for server-side downsize, special-case only image types.

Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = type === 'image' ? 10 * 1024 : Discourse.SiteSettings['max_' + type + '_size_kb'];

@zach-source zach-source Jul 29, 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.

Hardcoded 10MB bypasses admin-configured limits for non-image uploads.

Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = (type === 'image') ? (10 * 1024) : Discourse.SiteSettings['max_' + type + '_size_kb'];
Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = (type === 'image') ? 10 * 1024 : Discourse.SiteSettings['max_' + type + '_size_kb'];

@zach-source zach-source Jul 29, 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.

Hardcoded 10 MB check ignores max_image_size_kb site setting. Sites with lower limits bypass validation entirely. Read the site setting instead:

Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];

@zach-source zach-source Jul 29, 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.

Hard-coded 10 MB override ignores per-file-type site settings. Admins' max_image_size_kb and max_attachment_size_kb now ignored. Restore:

Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];
Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'] || 10 * 1024;

@zach-source zach-source Jul 29, 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.

Hardcoded 10 MB limit replaces per-type server setting. Error message shows wrong limit, breaks client-side validation for non-image types. Use Discourse.SiteSettings['max_' + type + '_size_kb']:

Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];

@ron-x5labs ron-x5labs Aug 12, 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.

[blocking] Hardcoded 10 MB replaces per-type site settings and breaks an existing test

The original Discourse.SiteSettings['max_' + type + '_size_kb'] enforced admin-configured max_image_size_kb / max_attachment_size_kb per type. Replacing it with 10 * 1024 (10 MB) for all types means:

  1. Admin limits are bypassed client-side — attachments configured below 10 MB are no longer validated before upload, so users hit a server rejection with a contradictory limit shown.
  2. The error dialog always reports "10240 KB" regardless of the actual configured limit.

This also breaks the existing test test/javascripts/lib/utilities-test.js.es6:52 ("prevents files that are too big from being uploaded"), which sets max_image_size_kb = 5 and expects a 10 KB file to be rejected with max_size_kb: 5. With the hardcoded 10 * 1024 KB, that 10 KB file passes validation and both assertions fail.

Restore the dynamic lookup:

Suggested change
var maxSizeKB = 10 * 1024; // 10MB
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];

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

@zach-source zach-source Jul 29, 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.

[medium] 413 error handler shows hardcoded 10 MB instead of actual server limit

The 413 (Request Entity Too Large) handler is supposed to tell the user the configured maximum. By hardcoding 10 * 1024 here, any site with max_image_size_kb set differently will show the wrong limit in the error dialog. This is misleading: the server rejected the upload based on its configured limit, but the client reports a different number.

Suggestion: Restore var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;

Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;

@zach-source zach-source Jul 29, 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.

Error message shows hardcoded 10MB instead of actual admin-configured limit.

Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;
Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;

@zach-source zach-source Jul 29, 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.

Error message hardcoded to 10240 KB, ignoring actual max_image_size_kb setting. Users see wrong limit if configured differently. Restore the site-setting read:

Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;

@zach-source zach-source Jul 29, 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.

Hard-coded error message (10240 KB) hides actual server limit, confusing users if different. Use:

Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;
Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb || 10 * 1024;

@zach-source zach-source Jul 29, 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.

Hardcoded 10 MB limit replaces max_image_size_kb setting. Error message shows wrong limit if admin configured lower value. Use Discourse.SiteSettings.max_image_size_kb:

Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;

@ron-x5labs ron-x5labs Aug 12, 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.

[non-blocking] 413 handler shows a hardcoded 10 MB limit

A 413 (Request Entity Too Large) comes from the web server's client_max_body_size, which the max_image_size_kb setting description tells admins to keep in sync. Hardcoding 10 * 1024 here means the dialog reports a 10 MB limit even when the server is configured differently, misleading the user. Restore the site setting so the message matches reality:

Suggested change
var maxSizeKB = 10 * 1024; // 10 MB
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;

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

@zach-source zach-source Jul 29, 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.

[high] URL-sourced download limit hardcoded to 10 MB, ignoring SiteSetting.max_image_size_kb

The original code used SiteSetting.max_image_size_kb.kilobytes as the download size cap for API URL uploads. The diff replaces it with the literal 10.megabytes. This means admins who configure max_image_size_kb to a value smaller than 10 MB (e.g., 1 MB) will have that limit silently bypassed for API uploads — an API caller can now download and store images up to 10 MB regardless of site policy. It also means FileHelper.download fetches up to 10 MB before the downsize loop even runs, wasting bandwidth for non-image types that are not subject to the downsize path.

Suggestion: Keep using SiteSetting.max_image_size_kb.kilobytes as the download cap, or use [SiteSetting.max_image_size_kb.kilobytes, 10.megabytes].max if the intent is to allow downloading larger files for subsequent downsize.

Suggested change
tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil
tempfile = FileHelper.download(url, [SiteSetting.max_image_size_kb.kilobytes, 10.megabytes].max, "discourse-upload-#{type}") rescue nil

@zach-source zach-source Jul 29, 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.

Download cap hardcoded to 10 MB ignores max_image_size_kb setting. Wastes bandwidth on small-limit sites, truncates valid uploads on large-limit sites. Use proportional cap:

Suggested change
tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil
tempfile = FileHelper.download(url, [SiteSetting.max_image_size_kb.kilobytes * 5, 10.megabytes].max, "discourse-upload-#{type}") rescue nil

@ron-x5labs ron-x5labs Aug 12, 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.

[non-blocking] Download cap hardcoded to 10 MB bypasses max_image_size_kb

The original SiteSetting.max_image_size_kb.kilobytes bounded API URL downloads to the admin-configured limit. Replacing it with 10.megabytes means:

  • Sites with a low max_image_size_kb still fetch up to 10 MB per API upload before the downsize loop runs — wasted bandwidth and temp storage.
  • For non-image attachments (which skip the downsize loop entirely), this is the only pre-create_for bound, so a 10 MB fetch bypasses the admin's intended attachment policy until the validator rejects it.

If the intent is to allow headroom for images that will be downsized, derive the cap from the setting rather than a magic number, e.g. [SiteSetting.max_image_size_kb.kilobytes * 5, 10.megabytes].max.

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

@zach-source zach-source Jul 29, 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.

Return value ignored: if all downsize calls fail, loop exhausts attempts and uploads oversized file anyway.

          break unless OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
          attempt -= 1

@zach-source zach-source Jul 29, 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.

Image re-encoding can increase file size (format overhead). Loop runs 5 attempts regardless of shrinkage, then uploads oversized file. Track previous size and break if unchanged:

@ron-x5labs ron-x5labs Aug 12, 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.

[blocking] Downsize loop ignores failures and has no progress detection

OptimizedImage.downsize → convert_with returns false on ImageMagick failure ($?.exitstatus != 0) or when ImageOptim raises. This loop never checks the return value, so:

  • If convert/gifsicle is missing or fails, the loop spins 5 times doing nothing, then the unchanged oversized (or corrupt) file is passed to Upload.create_for on line 72 and rejected with a generic "file too large" error — no log explains that auto-resize failed.
  • Re-encoding an already-compressed image can increase size (format/metadata overhead); the loop has no way to detect a stall and will exhaust all 5 attempts, then upload an oversized file that the validator rejects.

Track the previous size, check the return value, and stop early:

attempt = 5
prev_size = tempfile.size
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
  ok = OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
  break if !ok || tempfile.size >= prev_size  # downsize failed or made no progress
  prev_size = tempfile.size
  attempt -= 1
end
Rails.logger.warn("Failed to downsize upload below max_image_size_kb after #{5 - attempt} attempts") if tempfile.size > SiteSetting.max_image_size_kb.kilobytes

OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)

@ron-x5labs ron-x5labs Aug 12, 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.

[non-blocking] In-place downsize can corrupt the sole upload copy on a mid-write crash

OptimizedImage.downsize(tempfile.path, tempfile.path, ...) uses the same path as both source and destination; convert_with invokes an external convert/gifsicle process that writes directly to that path. If the process is killed or the disk fills mid-write, the downloaded tempfile — the only copy of the user's upload — is left truncated or partially written, and the return false if $?.exitstatus != 0 check can't undo the damage. Downsize to a temporary output file and atomically rename on success.

@ron-x5labs ron-x5labs Aug 12, 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.

[non-blocking] ImageOptim.new is re-instantiated on every convert_with call

convert_with (optimized_image.rb:164) constructs ImageOptim.new each invocation, which performs on-disk binary discovery (jpegoptim, optipng, …). This 5-iteration loop triggers up to 5 redundant discovery scans per oversized upload. Memoize the ImageOptim instance (e.g. a class-level @@image_optim ||= ImageOptim.new) or accept it as a parameter.

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={})

@zach-source zach-source Jul 29, 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.

Dead-code method—unreachable since line 148 redefines downsize with a different arity (3 args vs 4 args). Delete:

optimize("downsize", from, to, max_width, max_height, opts)
optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
end

@zach-source zach-source Jul 29, 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.

[critical] Duplicate downsize definition — original 4-arg overload is silently replaced, breaking all existing callers

Ruby uses the last definition when the same method name appears twice. The diff adds a new 3-arg def self.downsize(from, to, dimensions, opts={}) but the prior def self.downsize(from, to, max_width, max_height, opts={}) was already rewritten in the same hunk (line 143) to delegate to optimize with "#{max_width}x#{max_height}". The net result is two self.downsize definitions: the one at ~143 (4-arg) and the new one at ~147 (3-arg). In Ruby the 3-arg version wins, so any caller still passing downsize(from, to, width, height) — including the now-modified resize-path callers anywhere else in the codebase — will silently pass a numeric max_width as dimensions, producing a malformed ImageMagick argument and a silent false return from convert_with.

Suggestion: Remove the duplicate 4-arg downsize definition entirely; keep only the 3-arg form and update all call sites to pre-format the dimension string before calling.

@zach-source zach-source Jul 29, 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.

Two downsize methods with same name — Ruby uses last definition, breaking old 4-arg callers. Delete the old definition and update all callers to pre-format dimensions as "WIDTHxHEIGHT" string (or percentage like "80%").


@zach-source zach-source Jul 29, 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.

Ruby method overloading: second downsize replaces the first. All four-arg callers will fail silently.

Remove the old definition entirely; keep only the new 3-argument version.

@zach-source zach-source Jul 29, 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.

Duplicate downsize method—line 145 already defines it. Ruby keeps only the last definition (line 148), making the 4-arg version unreachable and breaking existing callers. Remove lines 145–147.

def self.optimize(operation, from, to, width, height, opts={})
dim = dimensions(width, height)
def self.downsize(from, to, dimensions, opts={})

@zach-source zach-source Jul 29, 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.

Method downsize defined twice — 3-arg version shadows the 4-arg version at line 145. Remove the redundant 4-arg method (lines 145–147) since both convert dimensions the same way.

@ron-x5labs ron-x5labs Aug 12, 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.

[blocking] Duplicate downsize definition shadows the 5-arg version, breaking existing callers

Ruby has no method overloading by arity — the second def self.downsize(from, to, dimensions, opts={}) (4 params) completely replaces the 5-param def self.downsize(from, to, max_width, max_height, opts={}) defined just above it. The last definition wins.

The existing caller app/jobs/regular/resize_emoji.rb:14 invokes OptimizedImage.downsize(path, path, 100, 100, opts) with 5 positional arguments, which now raises ArgumentError: wrong number of arguments (given 5, expected 3..4) and crashes every Jobs::ResizeEmoji run.

Fix: keep a single method that accepts either form, e.g.

def self.downsize(from, to, *args)
  opts = args.last.is_a?(Hash) ? args.pop : {}
  dims = args.size == 2 ? "#{args[0]}x#{args[1]}" : args[0]
  optimize("downsize", from, to, dims, opts)
end

…or update resize_emoji.rb (and any other 5-arg callers) to pass a pre-formatted dimensions string and delete the dead 5-param definition.

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