FEATURE: automatically downsize large images

[zaibkhan]

No description provided.

[codoki-pr-intelligence]

Codoki PR Review

Summary: Prevent in-place image overwrite, keep API arity, align limits
What’s good: Server-side downsizing before persisting uploads is a solid approach to keep images within site limits and avoid user friction; integrating with existing OptimizedImage pipeline is the right place.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Compatibility — downsize method override breaks 4-arg callers	…/models/optimized_image.rb This redefinition overrides the previous downsize signature (max_width, max_height, opts) and will break existing 4-arg callers at runtime (e.g., NoMethodError when opts is an Integer). To maintain backward compatibility and support the new dimensions string, collapse both into a single varargs method.
High	Correctness — In-place downsizing uses same path for input/output	…/controllers/uploads_controller.rb Converting in place (same input and output path) risks failure or corruption, especially for animated GIFs (gifsicle can't read and write the same file). It can also waste attempts if the conversion fails repeatedly. Write to a temp output and atomically replace the original on success; break or clean up on failure.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: Avoid in-place conversion on the same temp file path and preserve backward-compatible method signatures; also align client-side size checks/messages with server/site settings to prevent user confusion.
• Testing: Please add tests for: 1) OptimizedImage.downsize accepting both (max_width, max_height) and 'dimensions' forms, 2) UploadsController image downsizing loop for animated GIFs (ensuring it doesn't corrupt the tempfile and respects attempt limits), and 3) client/server alignment for size validation (e.g., images at 9MB pass client and are downsized server-side; 12MB are rejected).
• Documentation: Document the new effective client-side image size allowance (10MB) and clarify how server-side downsizing behaves for animated GIFs and static images, including the max attempts behavior and which setting governs final size.
• Compatibility: The new OptimizedImage.downsize definition overrides the previous 4-arg method and can break existing callers at runtime. Provide a single varargs method to maintain backward compatibility with both signatures.
• Performance: The downsizing loop may run up to 5 external conversions per upload, which can be CPU-heavy. Consider breaking early on failed conversion, exponential reduction, or lowering attempts. Also, avoid repeated work for animated GIFs where in-place conversion will fail.
• Open questions: Are there any external callers of OptimizedImage.downsize using the 4-arg signature (max_width, max_height, opts)? Do we intend to allow 10MB for image uploads across the board (including attachments), or only to facilitate server-side downsizing for images?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Client
    participant UploadsController
    participant FileHelper
    participant OptimizedImage
    Client->>UploadsController: create_upload(type, file, url)
    alt API URL path
        UploadsController->>FileHelper: download(url, 10MB, "discourse-upload-#{type}")
    end
    loop while size > max and attempts
        UploadsController->>OptimizedImage: downsize(from, to, "80%", allow_animation)
        alt animated GIF
            OptimizedImage->>OptimizedImage: downsize_instructions_animated()
        else static image
            OptimizedImage->>OptimizedImage: downsize_instructions()
        end
        OptimizedImage->>OptimizedImage: convert_with(instructions, to)
    end
    UploadsController-->>Client: Upload.create_for(...) result

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: This redefinition overrides the previous downsize signature (max_width, max_height, opts) and will break existing 4-arg callers at runtime (e.g., NoMethodError when opts is an Integer). To maintain backward compatibility and support the new dimensions string, collapse both into a single varargs method.

Suggested change

	def self.downsize(from, to, dimensions, opts={})
	def self.downsize(from, to, *args)
	opts = args.last.is_a?(Hash) ? args.pop : {}
	dimensions = args.length == 1 ? args.first : "#{args[0]}x#{args[1]}"
	optimize("downsize", from, to, dimensions, opts)
	end

[codoki-pr-intelligence]

⚠️ High: Converting in place (same input and output path) risks failure or corruption, especially for animated GIFs (gifsicle can't read and write the same file). It can also waste attempts if the conversion fails repeatedly. Write to a temp output and atomically replace the original on success; break or clean up on failure.

Suggested change

	OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
	tmp_out = "#{tempfile.path}.downsized"
	if OptimizedImage.downsize(tempfile.path, tmp_out, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
	FileUtils.mv(tmp_out, tempfile.path)
	else
	FileUtils.rm_f(tmp_out)
	break
	end

[codoki-pr-intelligence]

🔷 Medium: Hard-coding 10MB for all uploads diverges from site settings and per-type limits (images vs attachments), leading to user-visible mismatches (client allows, server rejects). Use 10MB only for images to enable server-side downsizing and keep attachments aligned with settings.