Discourse / PR #1

FEATURE: automatically downsize large images

marchugon/discourse-cursor-fr-benchmark#5
Golden recall: 66.7% Extra findings: 9

Golden Comments (2/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 downsize defined twice in optimized_image.rb — the second expects a single dimensions parameter (String), the first expected max_width, max_height separately. The second overloads the first, making the original call inaccessible. MEDIUM ✓ Found FriendlyReviewer found the double definition and identified that it breaks app/jobs/regular/resize_emoji.rb (passes 4 args to the method that now accepts only 3). Classified HIGH (more severe than golden's Medium).
#2 maxSizeKB = 10 * 1024 hardcoded ignores Discourse.SiteSettings['max_' + type + '_size_kb'] → no admin configurability, client and server limits can diverge (also applies to the 413 handler) LOW ✓ Found FriendlyReviewer found all 3 occurrences (utilities.js L185, L248 + uploads_controller.rb L55) — 1 more than the golden reference. Classified HIGH × 3 (more severe than golden's Low).
#3 "80%" passed as dimensions for downsizing can fail for animated GIFs when allow_animated_thumbnails is true — the animated path uses gifsicle --resize-fit which expects WxH geometry, not a percentage MEDIUM ✗ Missed FriendlyReviewer did not mention the GIF-specific bug with allow_animated_thumbnails and gifsicle --resize-fit.

Supplementary Findings (14 findings)

Finding File Severity Legitimate?
Duplicate downsize method: lines 168-170 dead code, L172 silently overrides optimized_image.rb:168 HIGH ✓ Covers golden #1 (with caller identified)
Breaking change: ResizeEmoji passes 4 args, the surviving method takes only 3 → ArgumentError runtime optimized_image.rb:172 HIGH ✓ Covers golden #1 (breaking change caller)
In-place file modification: source = destination → data loss if convert crashes uploads_controller.rb:68 HIGH ✓ Data loss legitimate
Hardcoded download limit 10MB bypasses max_image_size_kb admin setting uploads_controller.rb:55 HIGH ✓ Covers golden #2 (bypasses admin config)
Hardcoded 10MB client-side removes admin configurability for images + attachments utilities.js:185 HIGH ✓ Covers golden #2 (removes configurability)
Hardcoded 10MB in the 413 handler → '10240 KB' misleading if server has a different limit utilities.js:248 HIGH ✓ Covers golden #2 (misleading error message)
Existing tests using SiteSettings will fail with the hardcoded limit utilities.js:185 HIGH ✓ Test impact legitimate
Return value of OptimizedImage.downsize not checked → silent failures uploads_controller.rb:68 MEDIUM ✓ Silent failures
No post-loop size validation → oversized files accepted after 5 iterations uploads_controller.rb:69 MEDIUM ✓ Oversized files accepted
SVG files rasterized by the downsizing loop uploads_controller.rb:67 MEDIUM ✓ SVGs incorrectly rasterized
Variable content_type not initialized when API URL path is taken → passed nil uploads_controller.rb:56 MEDIUM ✓ nil passed as content_type
dimensions method removed from optimized_image.rb → verify external plugins optimized_image.rb:156 LOW ⚠ Preventative
Cosmetic inconsistency: '10 MB' vs '10MB' between comments utilities.js:249 LOW 💅 Cosmetic
Pre-existing: file.size undefined → size check bypassed utilities.js:163 LOW ✓ Bug legitimate

Honest Analysis

FriendlyReviewer detected 2 out of 3 golden bugs in this PR. The duplicate downsize method (golden #1) was found with a more complete diagnosis than the benchmark itself — FriendlyReviewer not only flagged the double definition but also identified that ResizeEmoji passes 4 arguments to a method that now only accepts 3, causing a runtime ArgumentError. This is a clear case of the reviewer tracing impact beyond the immediate file.

The hardcoded 10MB limit (golden #2) was also found across all 3 occurrences (JS frontend + Ruby backend), compared to only 2 in the golden reference. FriendlyReviewer correctly identified that this removes admin configurability. However, FriendlyReviewer classified this as HIGH across all occurrences, whereas the benchmark rates it as a LOW-severity issue. The severity inflation is notable — findings that the benchmark considers minor are regularly escalated by the reviewer.

The missed golden bug (GIF animation with allow_animated_thumbnails and gifsicle) is genuinely subtle: "80%" as a resize dimension works for ImageMagick's convert but not for gifsicle --resize-fit which expects WxH geometry. This is a domain-specific bug that requires understanding both the codebase configuration and the tooling behavior. Its absence is understandable.

Beyond the golden set, FriendlyReviewer produced 11 additional HIGH and MEDIUM findings (plus 3 LOW items excluded from scoring). Several are noteworthy: the in-place file modification risk (data loss if convert crashes), the hardcoded limit bypassing admin site settings, the unverified return value of downsize, and SVG files being needlessly rasterized through the downsizing loop. Almost all are legitimate concerns.

The LOW-severity findings are mostly acceptable noise. The dimensions method removal finding is purely preventative — it warns about potential external plugin breakage without evidence of actual impact. The cosmetic inconsistency between '10 MB' and '10MB' in comments is minor. The pre-existing file.size undefined bug is real but non-critical — it was already broken before this PR.

Overall, FriendlyReviewer delivers solid coverage on this PR with 2/3 golden bugs found. The main weaknesses are the GIF animation blind spot and a tendency to over-classify severity — findings are frequently pushed to HIGH when the benchmark considers them Medium or Low. The strength is breadth: the reviewer explores all affected files and finds issues the golden set does not cover.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
7 high, 4 medium, 3 low

The code review identified several critical issues that must be addressed before merging. The server-side changes in optimized_image.rb introduce a duplicate method definition that silently overrides the first, creating a breaking change for the ResizeEmoji job which will crash at runtime. The uploads controller modifies temp files in-place risking data loss on failure, bypasses admin-configurable size limits with a hardcoded 10MB, and does not validate the return value of the downsizing operation or the final file size after the loop. Additionally, SVG files will be incorrectly rasterized. On the client side, the JavaScript changes replace dynamic site settings with hardcoded 10MB limits, removing admin configurability, causing test failures, and potentially misleading users with incorrect error messages. All findings require attention, with the breaking change and data loss risk being the highest priority.

These might need a close look

  • 🔴 app/models/optimized_image.rb (L168)
    Duplicate downsize method definition: lines 168-170 are dead code as the second definition (line 172) silently overrides the first. Remove the first definition.
  • 🔴 app/models/optimized_image.rb (L172)
    Breaking change: The surviving downsize method accepts only 3 positional arguments but app/jobs/regular/resize_emoji.rb:14 passes 4 positional arguments, causing an ArgumentError at runtime. Either restore the old 4-arg signature or update all callers.
  • 🔴 app/controllers/uploads_controller.rb (L68)
    In-place file modification risk: Using the same file as source and destination can lead to data loss if convert crashes. Use an intermediate temp file and move on success.
  • 🔴 app/controllers/uploads_controller.rb (L55)
    Hardcoded download size limit of 10MB bypasses admin-configured max_image_size_kb. Use a configurable limit or at least respect the site setting.
  • 🔴 app/assets/javascripts/discourse/lib/utilities.js (L185)
    Hardcoded 10MB limit replaces dynamic site setting lookups for both images and attachments. This removes admin configurability and gives attachments a 10MB limit (server does not downsize attachments). Use separate configurable limits or keep site settings for attachments.
  • 🔴 app/assets/javascripts/discourse/lib/utilities.js (L248)
    Hardcoded 10MB limit in HTTP 413 error handler displays '10240 KB' even if server limit differs, misleading users. Source from server configuration or shared constant.
  • 🔴 app/assets/javascripts/discourse/lib/utilities.js (L185)
    Existing tests that rely on site settings will fail because the hardcoded limit always allows files up to 10MB. Tests at test/javascripts/lib/utilities-test.js.es6 must be updated.

Worth checking

  • 🟡 app/controllers/uploads_controller.rb (L68)
    Return value of OptimizedImage.downsize is not checked; failures are silently ignored. Check the return and abort/log on failure.
  • 🟡 app/controllers/uploads_controller.rb (L69)
    No post-loop size validation: after 5 iterations, oversized files are accepted. Add a final check and reject if still over the limit.
  • 🟡 app/controllers/uploads_controller.rb (L67)
    SVG files will be rasterized by the downsizing loop. Exclude SVGs with a check like && !filename.match?(/\.svg$/i).
  • 🟡 app/controllers/uploads_controller.rb (L56)
    Uninitialized local variable content_type when API URL path is taken; passed as nil to Upload.create_for. Initialize before the conditional or detect from downloaded file.

Small things (take or leave)

  • 🔵 app/models/optimized_image.rb (L156)
    The dimensions class method has been removed. Verify no external plugins depend on it.
  • 🔵 app/assets/javascripts/discourse/lib/utilities.js (L249)
    Minor inconsistency: comment uses '10 MB' with space but line 185 uses '10MB' without space. Use consistent formatting.
  • 🔵 app/assets/javascripts/discourse/lib/utilities.js (L163)
    Pre-existing issue: if file.size is undefined, the size check is bypassed. Consider adding a guard.