FEATURE: automatically downsize large images#10
Open
ShashankFC wants to merge 1 commit into
Open
Conversation
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:S
Sequence DiagramShows the new flow where oversized image uploads are automatically downsampled on the server before being stored and thumbnail jobs are enqueued — the core change in this PR. sequenceDiagram
participant Client
participant Server
participant ImageOptimizer
participant Storage
Client->>Server: POST upload (file or URL)
Server->>Server: Detect image && size > max → repeat downsize attempts
Server->>ImageOptimizer: OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation)
ImageOptimizer->>ImageOptimizer: Run convert/gifsicle + ImageOptim (resize & optimize)
ImageOptimizer-->>Server: downsized tempfile
Server->>Storage: Create Upload and store file
Storage-->>Server: Upload saved (enqueue thumbnail job)
Generated by CodeAnt AI |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // 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.
Suggestion: The client-side upload validation now uses a hardcoded 10MB limit for all uploads, ignoring per-type site settings like max_attachment_size_kb, so attachments larger than the configured limit will pass the browser check and only fail later on the server, leading to inconsistent behavior and confusing errors. You should keep the 10MB allowance only for images (which are downsized server-side) and continue to honor the site setting for attachments. [logic error]
Severity Level: Major ⚠️
- ⚠️ Composer uploads allow oversize attachments past browser validation.
- ⚠️ /uploads.json rejects attachments using max_attachment_size_kb server-side.
- ⚠️ Users see late failures and confusing size limit messages.
- ⚠️ Client ignores attachment limit despite upload-selector.js using it.
- ⚠️ utilities-test size expectations regress (`utilities-test.js.es6:52-61`).
Suggested change
| var maxSizeKB = 10 * 1024; // 10MB | |
| var maxSizeKB; | |
| if (type === 'image') { | |
| // allow larger original images; the server will downsize them | |
| maxSizeKB = 10 * 1024; // 10MB | |
| } else { | |
| // still respect per-type limits for non-image uploads | |
| maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb']; | |
| } | |
Steps of Reproduction ✅
1. Open the composer and trigger an upload from the reply area; this uses the jQuery
fileupload binding in `app/assets/javascripts/discourse/views/composer.js.es6:333-344`,
where the `fileuploadsubmit` handler at line 339 calls
`Discourse.Utilities.validateUploadedFiles(data.files)`.
2. In `validateUploadedFiles` at
`app/assets/javascripts/discourse/lib/utilities.js:143-160`, upload a non-image file (e.g.
`document.pdf`) so that `type` is set to `'attachment'` by the logic at line 158
(`isAnImage(upload.name) ? 'image' : 'attachment'`), and the function forwards to
`validateUploadedFile(upload, type, bypassNewUserRestriction)` at line 160.
3. Configure the site so that `SiteSetting.max_attachment_size_kb` is significantly lower
than 10 MB (this setting is defined in `config/site_settings.yml:516` and exposed on the
client via `Discourse.SiteSettings.max_attachment_size_kb`, as used in
`app/assets/javascripts/discourse/controllers/upload-selector.js.es6:17` and the utilities
tests at `test/javascripts/lib/utilities-test.js.es6:80`), then attempt to upload an
attachment whose size in kB is greater than `max_attachment_size_kb` but less than or
equal to 10 * 1024.
4. Observe that on the client, `validateUploadedFile` in `utilities.js:163-190` uses the
hardcoded `var maxSizeKB = 10 * 1024;` at line 182 instead of the per-type site setting,
so the attachment passes the browser check and the upload proceeds to `/uploads.json`
(configured in `composer.js.es6:333-335`); the server-side
`UploadsController#create_upload` at `app/controllers/uploads_controller.rb:51-83` and
`Upload` model validations (`app/models/upload.rb:59-169` with
`Validators::UploadValidator`) then enforce the real limit and reject the file, causing a
late failure and inconsistent behavior between client-side validation and server-side
rules.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/assets/javascripts/discourse/lib/utilities.js
**Line:** 182:182
**Comment:**
*Logic Error: The client-side upload validation now uses a hardcoded 10MB limit for all uploads, ignoring per-type site settings like `max_attachment_size_kb`, so attachments larger than the configured limit will pass the browser check and only fail later on the server, leading to inconsistent behavior and confusing errors. You should keep the 10MB allowance only for images (which are downsized server-side) and continue to honor the site setting for attachments.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
145
to
+149
| 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 | ||
|
|
||
| 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.
Suggestion: The OptimizedImage.downsize class method is defined twice with different parameter lists, and the second definition overwrites the first, so existing callers like OptimizedImage.downsize(path, path, 100, 100, opts) now pass too many arguments to the final method and will raise an ArgumentError at runtime. You should collapse these into a single downsize implementation that can handle both the old (from, to, width, height, opts) style and the new (from, to, dimensions, opts) style using a flexible argument list. [logic error]
Severity Level: Major ⚠️
- ❌ `Jobs::ResizeEmoji` crashes with ArgumentError whenever it runs.
- ⚠️ Custom emoji resize logic in `emoji.rb` job fails.
- ⚠️ Oversized custom emojis may remain unoptimized or too large.
- ⚠️ Background job queue polluted with failed `resize_emoji` jobs.
Suggested change
| 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 | |
| def self.optimize(operation, from, to, width, height, opts={}) | |
| dim = dimensions(width, height) | |
| def self.downsize(from, to, dimensions, opts={}) | |
| def self.downsize(from, to, *args) | |
| dimensions = nil | |
| opts = {} | |
| # Support: downsize(from, to, "80%", opts) | |
| if args.length == 2 && args[0].is_a?(String) && args[1].is_a?(Hash) | |
| dimensions = args[0] | |
| opts = args[1] | |
| # Support: downsize(from, to, max_width, max_height, opts) | |
| elsif args.length == 3 | |
| max_width, max_height, opts = args | |
| dimensions = "#{max_width}x#{max_height}" | |
| opts ||= {} | |
| # Support: downsize(from, to, max_width, max_height) | |
| elsif args.length == 2 | |
| max_width, max_height = args | |
| dimensions = "#{max_width}x#{max_height}" | |
| else | |
| raise ArgumentError, "wrong number of arguments (#{args.length + 2} for 4..5)" | |
| end | |
Steps of Reproduction ✅
1. Note that `OptimizedImage.downsize` is defined twice in
`app/models/optimized_image.rb:145-151`. In Ruby, the second definition (lines 149-151,
signature `(from, to, dimensions, opts={})`) overwrites the first definition `(from, to,
max_width, max_height, opts={})`.
2. Observe an existing caller in `app/jobs/regular/resize_emoji.rb:5-15`, where
`Jobs::ResizeEmoji#execute` calls `OptimizedImage.downsize(path, path, 100, 100, opts)`
(line 14), i.e., with 5 arguments: `from`, `to`, `max_width`, `max_height`, and `opts`.
3. With the PR's final file state, when the `ResizeEmoji` job runs (enqueued in
`app/models/emoji.rb:73` via `Jobs.enqueue(:resize_emoji, path: path)` after emoji
changes), Ruby dispatches to the *second* `downsize` definition, whose method signature is
`(from, to, dimensions, opts={})` and therefore expects at most 4 arguments.
4. At runtime, calling `OptimizedImage.downsize(path, path, 100, 100, opts)` now passes 5
arguments to a 4-parameter method, causing `ArgumentError: wrong number of arguments
(given 5, expected 3..4)` and the `Jobs::ResizeEmoji` background job fails whenever it
executes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/models/optimized_image.rb
**Line:** 145:149
**Comment:**
*Logic Error: The `OptimizedImage.downsize` class method is defined twice with different parameter lists, and the second definition overwrites the first, so existing callers like `OptimizedImage.downsize(path, path, 100, 100, opts)` now pass too many arguments to the final method and will raise an `ArgumentError` at runtime. You should collapse these into a single `downsize` implementation that can handle both the old `(from, to, width, height, opts)` style and the new `(from, to, dimensions, opts)` style using a flexible argument list.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
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
CodeAnt-AI Description
Enforce a 10 MB client/server upload cap and automatically downsize images that exceed site limits
What Changed
Impact
✅ Fewer rejected uploads for remotely fetched images✅ Larger local images accepted after automatic resizing✅ Clearer file-size error messages showing a 10 MB limit💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.