Skip to content

feat: recreate thumbnails#10

Open
SeduroDotCom wants to merge 1 commit into
masterfrom
mr/recreate-thumbnails
Open

feat: recreate thumbnails#10
SeduroDotCom wants to merge 1 commit into
masterfrom
mr/recreate-thumbnails

Conversation

@SeduroDotCom

Copy link
Copy Markdown
Contributor

@urs-van-binsbergen urs-van-binsbergen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested ✅

Little detail: typo in the commit comment in webapp:

Submodule webapp 94bd7bc..db117b0:
  > chore: udpate dl

@nimaai nimaai 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.

good. the only concerns:

  1. Non-atomic recreation window (main risk): recreate_thumbnail_files! deletes the old Preview rows and their files before calling create_previews!. If
    create_previews! raises (as tested in "when thumbnail recreation fails"), the media file is left with zero previews until the action is retried successfully.
    For an admin tool this is probably an acceptable trade-off (old files must be removed to reuse filenames), but it's worth confirming that's intentional —
    e.g. document it in the flash/error message or the linked issue, since a transient conversion failure now destroys working thumbnails rather than leaving
    them in place.
  2. Narrow rescue in delete_thumbnail_files: only Errno::ENOENT, Errno::EROFS are swallowed. Other IO errors (e.g. Errno::EACCES on permission-restricted
    storage) will propagate, delete no more files, but the preview rows for not-yet-processed files have already been deleted in the same transaction —
    consistent with point 1, just flagging the same class of issue surfaces here too.

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