FEATURE: automatically downsize large images#10
Conversation
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/assets/javascripts/discourse/lib/utilities.js">
<violation number="1" location="app/assets/javascripts/discourse/lib/utilities.js:182">
P1: Hardcoded `10 * 1024` replaces the configurable `Discourse.SiteSettings['max_' + type + '_size_kb']`. This removes admin control over upload size limits and loses the type-specific differentiation between image and attachment max sizes (the `type` parameter is now unused). If the server-side setting is different from 10MB, the client and server will disagree on allowed file sizes, leading to confusing UX where the client accepts a file the server rejects (or vice versa). Restore the dynamic site setting lookup.</violation>
<violation number="2" location="app/assets/javascripts/discourse/lib/utilities.js:246">
P1: Hardcoded `10 * 1024` replaces `Discourse.SiteSettings.max_image_size_kb` in the 413 error handler. The error message shown to users will always report 10240 KB as the max size, even if the server's actual limit is different. This creates a misleading user experience. Restore the dynamic site setting lookup.</violation>
</file>
<file name="app/controllers/uploads_controller.rb">
<violation number="1" location="app/controllers/uploads_controller.rb:55">
P1: The hardcoded `10.megabytes` download limit replaces the configurable `SiteSetting.max_image_size_kb.kilobytes` and applies to **all** file types (not just images). Non-image files uploaded via URL can now bypass the admin-configured size limit. Consider keeping the original setting-based limit for non-image files, or using a separate limit that's only applied when the file is detected as an image.</violation>
</file>
<file name="app/models/optimized_image.rb">
<violation number="1" location="app/models/optimized_image.rb:149">
P0: Duplicate `downsize` method definition — the second overrides the first, breaking existing callers. In Ruby, redefining a method with the same name silently replaces the previous definition. The 4-arg version (`from, to, max_width, max_height, opts`) is overwritten by the 3-arg version (`from, to, dimensions, opts`), so `resize_emoji.rb`'s call with separate width/height args (`downsize(path, path, 100, 100, opts)`) will raise `ArgumentError` at runtime.
Remove the first `downsize` and update all callers to pre-format dimensions, or combine both signatures into a single method that detects whether it received a string or numeric args.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| def self.optimize(operation, from, to, width, height, opts={}) | ||
| dim = dimensions(width, height) | ||
| def self.downsize(from, to, dimensions, opts={}) |
There was a problem hiding this comment.
P0: Duplicate downsize method definition — the second overrides the first, breaking existing callers. In Ruby, redefining a method with the same name silently replaces the previous definition. The 4-arg version (from, to, max_width, max_height, opts) is overwritten by the 3-arg version (from, to, dimensions, opts), so resize_emoji.rb's call with separate width/height args (downsize(path, path, 100, 100, opts)) will raise ArgumentError at runtime.
Remove the first downsize and update all callers to pre-format dimensions, or combine both signatures into a single method that detects whether it received a string or numeric args.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/models/optimized_image.rb, line 149:
<comment>Duplicate `downsize` method definition — the second overrides the first, breaking existing callers. In Ruby, redefining a method with the same name silently replaces the previous definition. The 4-arg version (`from, to, max_width, max_height, opts`) is overwritten by the 3-arg version (`from, to, dimensions, opts`), so `resize_emoji.rb`'s call with separate width/height args (`downsize(path, path, 100, 100, opts)`) will raise `ArgumentError` at runtime.
Remove the first `downsize` and update all callers to pre-format dimensions, or combine both signatures into a single method that detects whether it received a string or numeric args.</comment>
<file context>
@@ -139,25 +139,24 @@ def self.downsize_instructions_animated(from, to, dimensions, opts={})
- def self.optimize(operation, from, to, width, height, opts={})
- dim = dimensions(width, height)
+ def self.downsize(from, to, dimensions, opts={})
+ optimize("downsize", from, to, dimensions, opts)
+ end
</file context>
| // 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 |
There was a problem hiding this comment.
P1: Hardcoded 10 * 1024 replaces Discourse.SiteSettings.max_image_size_kb in the 413 error handler. The error message shown to users will always report 10240 KB as the max size, even if the server's actual limit is different. This creates a misleading user experience. Restore the dynamic site setting lookup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/lib/utilities.js, line 246:
<comment>Hardcoded `10 * 1024` replaces `Discourse.SiteSettings.max_image_size_kb` in the 413 error handler. The error message shown to users will always report 10240 KB as the max size, even if the server's actual limit is different. This creates a misleading user experience. Restore the dynamic site setting lookup.</comment>
<file context>
@@ -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;
</file context>
| // check file size | ||
| var fileSizeKB = file.size / 1024; | ||
| var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb']; | ||
| var maxSizeKB = 10 * 1024; // 10MB |
There was a problem hiding this comment.
P1: Hardcoded 10 * 1024 replaces the configurable Discourse.SiteSettings['max_' + type + '_size_kb']. This removes admin control over upload size limits and loses the type-specific differentiation between image and attachment max sizes (the type parameter is now unused). If the server-side setting is different from 10MB, the client and server will disagree on allowed file sizes, leading to confusing UX where the client accepts a file the server rejects (or vice versa). Restore the dynamic site setting lookup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/lib/utilities.js, line 182:
<comment>Hardcoded `10 * 1024` replaces the configurable `Discourse.SiteSettings['max_' + type + '_size_kb']`. This removes admin control over upload size limits and loses the type-specific differentiation between image and attachment max sizes (the `type` parameter is now unused). If the server-side setting is different from 10MB, the client and server will disagree on allowed file sizes, leading to confusing UX where the client accepts a file the server rejects (or vice versa). Restore the dynamic site setting lookup.</comment>
<file context>
@@ -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
if (fileSizeKB > maxSizeKB) {
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
</file context>
| var maxSizeKB = 10 * 1024; // 10MB | |
| var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb']; |
| # 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 |
There was a problem hiding this comment.
P1: The hardcoded 10.megabytes download limit replaces the configurable SiteSetting.max_image_size_kb.kilobytes and applies to all file types (not just images). Non-image files uploaded via URL can now bypass the admin-configured size limit. Consider keeping the original setting-based limit for non-image files, or using a separate limit that's only applied when the file is detected as an image.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/uploads_controller.rb, line 55:
<comment>The hardcoded `10.megabytes` download limit replaces the configurable `SiteSetting.max_image_size_kb.kilobytes` and applies to **all** file types (not just images). Non-image files uploaded via URL can now bypass the admin-configured size limit. Consider keeping the original setting-based limit for non-image files, or using a separate limit that's only applied when the file is detected as an image.</comment>
<file context>
@@ -52,14 +52,23 @@ def create_upload(type, file, url)
# 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
</file context>
2 participants
Test 1
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#1