Discourse / PR #25

FEATURE: automatically downsize large images

discourse/discourse@ffbaf8c5
Golden recall: 100% Extra findings: 5

Golden Comments (3/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 The downsize method is defined twice. The second definition, which expects a single dimensions string parameter, overrides the first, which expected separate max_width and max_height parameters. This makes the original method unreachable and breaks existing code that calls it with separate width and height arguments. MEDIUM ✓ Found "Duplicate def self.downsize definition (line 145 vs 149) silently overrides the first, leaving only the 4-argument dimensions form. This breaks ResizeEmoji which calls with 4 positional args + opts, raising an ArgumentError at runtime" (app/models/optimized_image.rb L149)
#2 Hardcoding maxSizeKB = 10 * 1024 ignores Discourse.SiteSettings['max_' + type + '_size_kb'], so the client-side limit can diverge from server-side and per-type settings (also applies to the 413 handler below). LOW ✓ Found "Hardcoded 10 * 1024 replaces the configurable max_<type>_size_kb site setting. This ignores admin configuration, breaks the existing test that expects max_image_size_kb = 5 to reject a 10KB file, and also relaxes the limit for attachments, which the server does not downsize" (app/assets/javascripts/discourse/lib/utilities.js L182)
#3 Passing 80% as the dimensions can fail for animated GIFs when allow_animated_thumbnails is true, since the animated path uses gifsicle --resize-fit which expects WxH geometry, not a percentage; downsizing would then silently fail. MEDIUM ✓ Found "The controller passes a percentage string ("80%") as the dimensions argument. While acceptable for ImageMagick, the gifsicle path (--resize-fit) expects pixel dimensions, so animated GIFs will fail. The percentage also does not match the intended max-dimension threshold semantics" (app/controllers/uploads_controller.rb L67)

Supplementary Findings (5 findings)

Finding File Severity Legitimate?
The return value of OptimizedImage.downsize is ignored. If ImageMagick fails (corrupt/oversized image), the failure is silently swallowed, the loop re-runs on the broken file, and the oversized/invalid image is still uploaded without any error surfaced app/controllers/uploads_controller.rb:67 HIGH ✓ Real: a silent failure on the hot upload path means oversized or corrupt images still get uploaded
The in-place downsize (from == to) writes to the same file being read, which is unsafe for convert/gifsicle and may corrupt the tempfile if the tool truncates while still reading app/controllers/uploads_controller.rb:67 MEDIUM ✓ Real: writing to the file being read can corrupt the tempfile; the tool contract is violated
The fixed 80% pixel-shrink loop does not guarantee the file size falls below max_image_size_kb within 5 attempts; if it remains oversized, the file is uploaded with no warning or final check app/controllers/uploads_controller.rb:66 MEDIUM ✓ Real: 5 iterations of 80% give no convergence guarantee, and there is no final size check
SVG (and webp) are not excluded from the down-size branch. FileHelper.is_image? includes SVG, so oversized SVGs will be rasterized/re-encoded via ImageMagick, contradicting the existing SVG handling in OptimizedImage.create_for app/controllers/uploads_controller.rb:64 MEDIUM ✓ Real: SVGs pass is_image? and would be re-encoded, contradicting the existing SVG handling
The 413 error display now always reports "10240kb" instead of the configured max_image_size_kb, giving users misleading info about the actual server limit. Also duplicates the magic constant from line 182 app/assets/javascripts/discourse/lib/utilities.js:246 LOW ✓ Real UX nit: the hardcoded message misleads users about the actual server limit

Honest Analysis

FriendlyReviewer found all three golden bugs of this PR — recall 100%. The strongest catch is golden #1: the review does not merely note that downsize is defined twice, it pins the two definitions to lines 145 vs 149, identifies the surviving 4-argument form, and traces the runtime consequence (calls from ResizeEmoji raising ArgumentError). That is the kind of finding that prevents a broken production job. The hardcoded 10MB limit finding (golden #2) is also thorough: it names the broken test expectation and the attachment case the server does not downsize, which is a real behavioral divergence, not just a style nit.

The third golden is the weakest of the three matches. FriendlyReviewer does flag the percentage-vs---resize-fit mismatch on the gifsicle path, but as one 🟡 item among three findings stacked on the same line (L67): the gifsicle mismatch, the in-place resize, and the non-converging loop all share one line number. The golden's specific failure mode (animated GIFs silently failing to downsize) is found, but the review treats it as one concern in a cluster rather than calling out the animated-GIF path specifically. The finding is accurate; the targeting is coarser than the golden comment.

The five supplementary findings are all concentrated on the same two lines (L64-L67) of uploads_controller.rb plus the 413 message in utilities.js. The most substantive is the ignored return value of OptimizedImage.downsize — a silent failure that still uploads the oversized image, which is arguably the most production-relevant issue on this PR, and one the golden set does not contain. The SVG/webp exclusion and loop-convergence points are legitimate design gaps, and the 413 message is a minor but real UX regression. All five are defensible; none is a forced or invented finding.

The main caveat is that the review's verdict ("not production-ready, requires changes on both sides") rests entirely on the upload path — which is correct for this PR, but means the review's density is a product of the change being small and centralized. Within that scope, nothing important was missed.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
3 high, 4 medium, 1 low 1 issue

The MR adds an automatic image down-size loop on the server and relaxes the client-side upload size check to accommodate large images. However, the implementation has multiple blocking issues: a duplicate downsize method that breaks the ResizeEmoji job, client-side hardcoded 10MB limits that ignore site configuration and break existing tests, an unsafe in-place resize, ignored failure return values, and a non-converging shrink loop that can still upload oversized images. The feature is not production-ready and requires changes on both server and client sides.

These might need a close look

  • 🔴 app/models/optimized_image.rb (L149)
    Duplicate def self.downsize definition (line 145 vs 149) silently overrides the first, leaving only the 4-argument dimensions form. This breaks ResizeEmoji which calls with 4 positional args + opts, raising an ArgumentError at runtime.
  • 🔴 app/controllers/uploads_controller.rb (L67)
    The return value of OptimizedImage.downsize is ignored. If ImageMagick fails (corrupt/oversized image), the failure is silently swallowed, the loop re-runs on the broken file, and the oversized/invalid image is still uploaded without any error surfaced.
  • 🔴 app/assets/javascripts/discourse/lib/utilities.js (L182)
    Hardcoded 10 * 1024 replaces the configurable max_<type>_size_kb site setting. This ignores admin configuration, breaks the existing test that expects max_image_size_kb = 5 to reject a 10KB file, and also relaxes the limit for attachments, which the server does not downsize.

Worth checking

  • 🟡 app/controllers/uploads_controller.rb (L67)
    The controller passes a percentage string ("80%") as the dimensions argument. While acceptable for ImageMagick, the gifsicle path (--resize-fit) expects pixel dimensions, so animated GIFs will fail. The percentage also does not match the intended max-dimension threshold semantics.
  • 🟡 app/controllers/uploads_controller.rb (L67)
    The in-place downsize (from == to) writes to the same file being read, which is unsafe for convert/gifsicle and may corrupt the tempfile if the tool truncates while still reading.
  • 🟡 app/controllers/uploads_controller.rb (L66)
    The fixed 80% pixel-shrink loop does not guarantee the file size falls below max_image_size_kb within 5 attempts; if it remains oversized, the file is uploaded with no warning or final check.
  • 🟡 app/controllers/uploads_controller.rb (L64)
    SVG (and webp) are not excluded from the down-size branch. FileHelper.is_image? includes SVG, so oversized SVGs will be rasterized/re-encoded via ImageMagick, contradicting the existing SVG handling in OptimizedImage.create_for. Exclude SVG and confirm webp behavior.
Small things (take or leave)
  • 🔵 app/assets/javascripts/discourse/lib/utilities.js (L246)
    The 413 error display now always reports "10240kb" instead of the configured max_image_size_kb, giving users/misleading info about the actual server limit. Also duplicates the magic constant from line 182.

✅ Feature-level checklist

Not there yet

  • ❌ FEATURE: automatically downsize large images.
    The feature is not fully implemented: the server down-size loop can fail silently and still upload oversized images, SVG handling is inconsistent, the client-side limit change breaks configuration and test expectations, and the animated GIF path is broken. These issues prevent the feature from reliably meeting its intended behavior.