Skip to content

Thumbnails followups - #3793

Merged
aduffeck merged 7 commits into
opencloud-eu:mainfrom
aduffeck:thumbnails-followups
Oct 26, 2026
Merged

aduffeck merged 7 commits into
opencloud-eu:mainfrom
aduffeck:thumbnails-followups

Conversation

@aduffeck

Copy link
Copy Markdown
Member

Improve and harden the new push-based thumbnail generator.

A box like 0x2048 passed the check, and the aspect ratio of a very wide source then sized the other side without a bound.
The old default list reached 7680x4320, which made webdav ask the generator for boxes it refuses.
With both the old and the new name set, webdav and the thumbnails service picked different values.
Without it the user provider searched with an empty tenant.
The cache keys changed, so the old files were never read again but stayed mixed in with the new ones. With the cache in its own directory the old one can be deleted.
@aduffeck
aduffeck requested a review from dschmidt October 23, 2026 12:32
@codacy-production

codacy-production Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity

Metric Results
Complexity 5

View in Codacy

🟢 Coverage 88.64% diff coverage

Metric Results
Coverage variation Report missing for 38845621
Diff coverage ✅ 88.64% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (3884562) Report Missing Report Missing Report Missing
Head commit (9c15a7e) 92415 24921 26.97%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3793) 44 39 88.64%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@butonic butonic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@dschmidt

Copy link
Copy Markdown
Contributor

Two things I'd like changed, the rest looks good to me:

  1. 0x0 still escapes the new guard. freeAxis only checks the limits when resize is true. For 0x0, autoAxis returns the source size with resize == false, so the check is skipped and the image comes back at its own size, up to the 7680x7680 input limit, well past the 2048 output bound. Both axes free is still a free axis. One check in freeAxis regardless of resize covers all three paths.

  2. One sentence in MIGRATION.md for the files/ mount. With the root mounted it doesn't matter whether the subfolder is files or cache, so the change itself is fine. But someone who mounted exactly <root>/files, or copied cache_dir: .../thumbnails/files from the previous migration example, now silently writes the cache next to the mount. Telling them to point WEBDAV_THUMBNAILS_CACHE_DIR at the mount covers it.

Optional: in the gif path (push.go, serveAnimation) every error from resizeGIF is mapped to 422, while the still image path uses errors.Is(err, errBoxTooLarge). Same check in both would keep a future error from resizeGIF from turning into a silent 422.

0x0 returns the source at its own size, which skipped the check and let anything up to the input limit through.
@aduffeck
aduffeck merged commit 600e1fa into opencloud-eu:main Oct 26, 2026
60 checks passed
@aduffeck
aduffeck deleted the thumbnails-followups branch October 26, 2026 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants