FEATURE: automatically downsize large images by everettbu · Pull Request #1 · ai-code-review-evaluation/discourse-graphite

everettbu · 2025-07-26T19:36:33Z

Test 1

mfeuerstein

PR Review — approved

Reviewed 3 files. 0 high-severity issues found. Verdict: approved.

`app/assets/javascripts/discourse/lib/utilities.js` (low)

Reviewed app/assets/javascripts/discourse/lib/utilities.js — looks good

`app/models/optimized_image.rb` (low)

Reviewed app/models/optimized_image.rb — looks good

`app/controllers/uploads_controller.rb` (low)

Reviewed app/controllers/uploads_controller.rb — looks good

zach-source

Found 4 blocking issues.

critical app/models/optimized_image.rb:147 — Duplicate downsize definition — original 4-arg overload is silently replaced, breaking all existing callers
high app/controllers/uploads_controller.rb:55 — URL-sourced download limit hardcoded to 10 MB, ignoring SiteSetting.max_image_size_kb
high app/assets/javascripts/discourse/lib/utilities.js:182 — Client-side size check hardcoded to 10 MB, bypassing per-type site settings
medium app/assets/javascripts/discourse/lib/utilities.js:246 — 413 error handler shows hardcoded 10 MB instead of actual server limit

zach-source · 2026-07-29T02:39:06Z

  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


[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 · 2026-07-29T02:39:06Z

      # 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


[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 · 2026-07-29T02:39:06Z

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


[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 · 2026-07-29T02:39:06Z

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


[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

Method overloading, missing error handling, hardcoded upload limits.

zach-source · 2026-07-29T15:46:43Z

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



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 · 2026-07-29T15:46:43Z

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


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 · 2026-07-29T15:46:43Z

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


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 · 2026-07-29T15:46:43Z

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


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

Method shadowing causes parameter mismatch; resize loop proceeds even when images don't shrink; size limits hardcoded throughout, ignoring site settings — see inline.

zach-source · 2026-07-29T16:47:29Z


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


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.

zach-source · 2026-07-29T16:47:29Z

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


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:

zach-source · 2026-07-29T16:47:29Z

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


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 · 2026-07-29T16:47:29Z

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


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 · 2026-07-29T16:47:29Z

      # 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


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

zach-source

Method redefinition creates runtime errors; hard-coded limits override site settings; confusing error message. See inline.

zach-source · 2026-07-29T17:24:20Z

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



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.

zach-source · 2026-07-29T17:24:20Z

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


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 · 2026-07-29T17:24:20Z

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


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 · 2026-07-29T17:24:20Z

+    optimize("resize", from, to, "#{width}x#{height}", opts)
  end

  def self.downsize(from, to, max_width, max_height, opts={})


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

zach-source

Method collision and hardcoded limits flagged inline.

zach-source · 2026-07-29T18:02:38Z

  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


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 · 2026-07-29T18:02:38Z

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


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'];

zach-source · 2026-07-29T18:02:38Z

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


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;

FEATURE: automatically downsize large images

ffbaf8c

This was referenced Nov 14, 2025

FEATURE: automatically downsize large images ai-code-review-evaluations/graphite-discourse#10

Closed

FEATURE: automatically downsize large images ai-code-review-evaluations/graphite-discourse#20

Open

ShashankFC mentioned this pull request Jan 5, 2026

FEATURE: automatically downsize large images AI-Code-Review-Evals/graphite-discourse#1

Open

mfeuerstein reviewed Apr 10, 2026

View reviewed changes

khaliqgant mentioned this pull request May 15, 2026

[MSD benchmark] FEATURE: automatically downsize large images MSD-Benchmarks/discourse-graphite#1

Open

amznUjjwalpa mentioned this pull request May 21, 2026

FEATURE: automatically downsize large images CloudSmith-Agent-Benchmark/discourse-graphite__PR1#1

Open

amznUjjwalpa mentioned this pull request Jun 9, 2026

FEATURE: automatically downsize large images CloudSmith-Release-Safety/discourse-graphite__PR1__PR1#1

Open

zach-source suggested changes Jul 29, 2026

View reviewed changes

	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

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

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

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

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

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

Uh oh!

Conversation

everettbu commented Jul 26, 2025

Uh oh!

mfeuerstein left a comment

Choose a reason for hiding this comment

PR Review — approved

app/assets/javascripts/discourse/lib/utilities.js (low)

app/models/optimized_image.rb (low)

app/controllers/uploads_controller.rb (low)

Uh oh!

zach-source left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-source left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-source left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-source left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-source left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`app/assets/javascripts/discourse/lib/utilities.js` (low)

`app/models/optimized_image.rb` (low)

`app/controllers/uploads_controller.rb` (low)