FEATURE: automatically downsize large images

[ShashankFC]

Test 1nn---nReplicated from ai-code-review-evaluation/discourse-greptile#1

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 26s —— View job

---

Code Review Complete

Tasks

• Read repository guidelines (CLAUDE.md)
• Review changed files and git diff
• Analyze code for issues and improvements
• Provide comprehensive review feedback

---

Summary

This PR implements automatic downsizing of large images before upload processing. The feature allows users to upload images larger than the configured max_image_size_kb limit, automatically reducing them in an iterative loop until they meet the size requirement.

Issues Found

🔴 Critical: Hardcoded File Size Limits

Location: app/assets/javascripts/discourse/lib/utilities.js:182 and app/assets/javascripts/discourse/lib/utilities.js:246

The PR hardcodes the file size limit to 10 * 1024 (10MB) in the JavaScript validation, replacing the dynamic Discourse.SiteSettings values:

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

Problem: This breaks the dynamic site setting behavior. The JavaScript now enforces a fixed 10MB limit regardless of the server-side SiteSetting.max_image_size_kb configuration.

Impact:

• Site admins can no longer configure upload limits through settings
• Creates inconsistency: JS enforces 10MB while server-side respects SiteSetting.max_image_size_kb
• If server is configured for 5MB limit, users will pass JS validation but fail server-side validation
• If server is configured for 20MB limit, JS will incorrectly reject valid uploads

Recommendation: Revert these JavaScript changes and keep the dynamic site settings. The automatic downsizing feature on the server side (added in uploads_controller.rb:63-70) already handles large images gracefully.

Fix hardcoded limits →

---

🟡 Moderate: Duplicate Method Definition

Location: app/models/optimized_image.rb:145-151

The PR introduces two downsize methods with different signatures:

def self.downsize(from, to, max_width, max_height, opts={})
  optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
end

def self.downsize(from, to, dimensions, opts={})
  optimize("downsize", from, to, dimensions, opts)
end

Problem: In Ruby, the second method definition overwrites the first. The 4-parameter version (max_width, max_height) is effectively dead code.

Impact:

• The refactoring works because the new implementation in uploads_controller.rb:67 calls with a string dimension "80%"
• However, any existing code calling downsize(from, to, width, height) with integers will fail since that signature no longer exists
• This is a breaking API change

Recommendation: Remove the first downsize method definition (lines 145-147) if it's intentionally replaced, or rename one of them to avoid confusion. Check for any callers expecting the 4-parameter signature.

Fix duplicate method →

---

🟢 Minor: Missing Error Handling

Location: app/controllers/uploads_controller.rb:63-70

The automatic downsizing loop lacks error handling:

while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
  OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
  attempt -= 1
end

Concerns:

1. No error handling if OptimizedImage.downsize fails
2. No validation that file size actually decreased after downsize operation
3. Could infinite loop if downsize fails silently (though attempt counter provides safety)
4. No logging when downsizing occurs or when max attempts are reached

Recommendation: Add error handling and logging:

if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
  original_size = tempfile.size
  attempt = 5
  while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
    previous_size = tempfile.size
    if OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
      tempfile.rewind # Ensure file pointer is reset
      # Check if size actually decreased
      if tempfile.size >= previous_size
        Rails.logger.warn("Image downsizing did not reduce file size for #{filename}")
        break
      end
    else
      Rails.logger.error("Failed to downsize image #{filename}")
      break
    end
    attempt -= 1
  end
  Rails.logger.info("Downsized image #{filename} from #{original_size} to #{tempfile.size} bytes") if tempfile.size < original_size
end

---

🟢 Minor: Code Quality

Location: app/models/optimized_image.rb:141-143, 145-147

The refactoring improves code quality by:

• ✅ Removing the unnecessary dimensions helper method
• ✅ Simplifying the optimize method signature
• ✅ Making dimension formatting more explicit at call sites

This is good, but the duplicate method issue noted above needs resolution.

---

Positive Aspects

1. User Experience: The automatic downsizing feature significantly improves UX by accepting large images instead of rejecting them outright
2. Safety: The 5-attempt limit prevents infinite loops
3. Flexibility: The "80%" dimension parameter is a good starting point for iterative reduction
4. Respects Settings: Server-side implementation correctly uses SiteSetting.allow_animated_thumbnails

---

Recommendations

1. High Priority: Revert the JavaScript hardcoded limits and restore dynamic site settings
2. Medium Priority: Resolve the duplicate downsize method definition
3. Low Priority: Add error handling and logging to the downsizing loop
4. Testing: Verify behavior with:
   • Images slightly over the limit (1 downsize iteration)
   • Images significantly over the limit (multiple iterations)
   • Animated GIFs
   • Various image formats (PNG, JPEG, WebP)
   • Images that can't be downsized (already highly compressed)

---

| Branch: large-image-processing