FEATURE: automatically downsize large images

[akshayutture-augment]

Test 1

---

Replicated from ai-code-review-evaluation/discourse-greptile#1

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete

I've reviewed the PR that adds automatic downsizing for large images. Here's my detailed feedback:

---

Summary

This PR implements automatic image downsizing functionality to handle large uploads by iteratively reducing them to meet size constraints. The changes modify the upload validation logic and refactor the OptimizedImage model methods.

---

Critical Issues

1. Hardcoded Size Limits in JavaScript (app/assets/javascripts/discourse/lib/utilities.js:182, 246)

• Problem: The PR replaces dynamic site settings with hardcoded 10MB limits
• Original: Discourse.SiteSettings['max_' + type + '_size_kb'] and Discourse.SiteSettings.max_image_size_kb
• Changed to: 10 * 1024 (hardcoded)
• Impact: This removes administrator configurability and creates inconsistency with server-side settings
• Recommendation: Keep the dynamic site settings or ensure consistency across client/server

2. Potential Infinite Loop Risk (app/controllers/uploads_controller.rb:64-70)

• Problem: The downsize loop has a fixed attempt limit but no validation that progress is being made
• Code:

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

• Issue: If downsizing fails or doesn't reduce file size sufficiently, the loop will exhaust attempts without useful error handling
• Recommendation:
  • Check if file size actually decreased after each iteration
  • Log warnings when downsizing doesn't make progress
  • Consider returning an error if all attempts fail to meet the size requirement

3. File Mutation Safety (app/controllers/uploads_controller.rb:67)

• Problem: Downsizing modifies the tempfile in-place (tempfile.path, tempfile.path)
• Risk: If the operation fails mid-way, the original file is corrupted with no backup
• Recommendation: Either:
  • Create a backup before starting iterations, or
  • Use a separate temp file for output and only replace on success

---

Code Quality Issues

4. Duplicate Method Definition (app/models/optimized_image.rb:145-151)

• Problem: downsize method is defined twice with different signatures:
  • Line 145: downsize(from, to, max_width, max_height, opts={})
  • Line 149: downsize(from, to, dimensions, opts={})
• Impact: The first definition is completely overridden and unreachable
• Recommendation: Remove the duplicate. The refactoring should have removed the old signature entirely.

5. Removed Helper Method (app/models/optimized_image.rb)

• The dimensions helper method was removed, and its logic was inlined
• While this simplifies the code, it's a breaking change if any external code or plugins were calling OptimizedImage.dimensions(width, height)
• Recommendation: Verify no external dependencies exist, or add a deprecation warning

---

Design Concerns

6. Fixed Reduction Percentage

• The downsize operation uses a hardcoded "80%" reduction
• For very large images, 80% per iteration might be too aggressive (quality loss) or too conservative (many iterations needed)
• Suggestion: Consider using adaptive sizing based on the ratio of current size to target size

7. Missing Error Feedback

• If all 5 downsizing attempts fail, the code silently continues to create the upload
• Users won't know their image was too large and couldn't be reduced
• Recommendation: Return a meaningful error message when automatic downsizing fails

8. No Dimension Validation

• The code only checks file size, not image dimensions
• Very large resolution images might still cause memory issues during processing
• Suggestion: Consider validating/limiting maximum pixel dimensions as well

---

Testing Recommendations

1. Edge Cases:

   • Test with images exactly at the size limit
   • Test with images that cannot be compressed (already optimized)
   • Test with corrupt/invalid image files
   • Test with animated GIFs (code accounts for this, but needs verification)

2. Performance:

   • Measure downsize operation time for various image sizes
   • Test concurrent uploads to ensure no resource exhaustion

3. Integration:

   • Verify the JavaScript validation still works correctly with hardcoded limits
   • Test API uploads (line 55) work correctly with the new logic

---

Positive Aspects

✅ Good: The iterative downsizing approach with attempt limits prevents infinite loops
✅ Good: Preserves animation settings via SiteSetting.allow_animated_thumbnails
✅ Good: Simplification of the optimize method signature is cleaner
✅ Good: Guards against empty files with tempfile.size > 0 check

---

Recommendations Priority

Must Fix:

1. Remove duplicate downsize method definition (Line 149)
2. Revert hardcoded size limits in JavaScript or ensure consistency

Should Fix:
3. Add progress validation in downsize loop
4. Add error handling/messaging when downsizing fails
5. Implement file mutation safety (backup or separate output file)

Nice to Have:
6. Adaptive reduction percentage
7. Dimension validation
8. Better logging of downsize operations

---

[github-actions]

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.