Skip to content

refactor(attachments): collapse blob previews into one state, gate video on mime - #3657

Merged
kolaente merged 4 commits into
mainfrom
feat-video-preview-followup
Aug 29, 2026
Merged

refactor(attachments): collapse blob previews into one state, gate video on mime#3657
kolaente merged 4 commits into
mainfrom
feat-video-preview-followup

Conversation

@tink-bot

Copy link
Copy Markdown
Collaborator

Follow-up to #3491, which merged before these could land.

#3491 shipped video attachment previews. A review pass on it surfaced four design questions that were deliberately deferred to the maintainer rather than decided mid-PR; this implements the answers.

Changes

1. Gate video previews on mime alone (985cb0c74)

Drops SUPPORTED_VIDEO_SUFFIX. This matches canPreviewAudio, which landed on main in the meantime with the reasoning already spelled out: a <video> element neither parses HTML nor executes script, so the server-sniffed mime is the whole boundary. The allowlist only rejected playable containers without buying any safety.

Verified: .mkv (server mime video/matroska) and a completely extension-less mp4 (video/mp4) now preview and decode, where they previously downloaded. text/plain named evil.mp4 still downloads.

2. Play icon for video rows (57d2fe91a)

Video was the only previewable type still rendering the generic file icon, so the thumbnail disagreed with the click behaviour. Mirrors the isAudio/volume-high branch.

3. Collapse the blob previews into one state (2e9507e89)

Image, PDF and video each carried their own ref, close function, modal and onBeforeUnmount line — seven refs and three close functions between them. A previewKind() helper now drives both the guard and the assignment, so at most one preview is structurally representable.

This also removes a latent hazard and fixes a real bug:

  • The old dispatch selected PDF by elimination (a bare else), so a fourth preview type added to the guard would have routed its blob into the same-origin pdf-preview-iframe. With one kind value there is no elimination branch left.
  • Previously closeVideoPreview() cleared only the video refs, so opening a video while an image lightbox was open left both mounted, with the video audible underneath. Now impossible by construction.

Audio is untouched — it uses the separate inline per-row AudioPreview player.

4. Panel behind the video error state (2820bf734)

The video and iframe branches sit on their own opaque media; the error branch had none, so white text and the download button floated over the dimmed attachment rows.

Testing

pnpm lint, pnpm lint:styles and pnpm build pass. 43 unit tests pass across 5 files, including main's AudioPreview suite; previewKind has its own coverage and the canPreviewVideo cases were updated for the mime-only gate. No new vue-tsc errors (the 4 in these files are pre-existing and only line-shifted).

Driven end-to-end against a local build with Playwright across 13 flows: image lightbox (zoom/pan/keyboard) and PDF iframe unregressed; exactly one preview surface mounted at every animation frame across both switch directions; blob revoked in the same frame as the incoming click; ESC-mid-fetch and unmount-mid-fetch both end with the late blob created and revoked in the same millisecond and no modal resurrection; spinner confirmed video-only (147ms on a 64MB mp4, zero frames on an 18MB PDF); ProRes .mov still shows the failure message and its Download button produces a byte-identical file; audio still bypasses the modal entirely.

Notes

Reviewed and runtime-tested with Claude Code.

Drops SUPPORTED_VIDEO_SUFFIX to match canPreviewAudio: a <video> element
neither parses HTML nor executes script, so the server-sniffed mime is the
whole boundary. The allowlist only rejected playable containers (.mkv,
.avi, extension-less uploads) without buying any safety.
Video was the only previewable type still rendering the generic file icon,
so the thumbnail disagreed with the click behaviour.
Image, PDF and video each carried their own ref, close function, modal and
unmount line. A previewKind() helper now drives both the guard and the
assignment, so at most one preview is representable and there is no
by-elimination branch left to route a new type into the PDF iframe.

Audio stays on its inline per-row player, untouched.
The video and iframe branches sit on their own opaque media; the error
branch had nothing, so white text and the download button floated over the
dimmed attachment rows behind the modal.
@kolaente
kolaente enabled auto-merge (rebase) August 29, 2026 15:40
@github-actions github-actions Bot added area/attachments File attachments on tasks: upload, preview, download area/frontend Frontend-only code or UI work area/internal-code Internal refactoring, cleanup, code-quality work labels Aug 29, 2026
@github-actions

Copy link
Copy Markdown

Preview Deployment

Preview deployments for this PR are available at:

URL Tag Commit
https://pr-3657.preview.vikunja.dev ghcr.io/go-vikunja/vikunja:pr-3657 latest
https://sha-2820bf73448e0d9496c2bbc20b022517da08989c.preview.vikunja.dev ghcr.io/go-vikunja/vikunja:sha-2820bf73448e0d9496c2bbc20b022517da08989c 2820bf7

The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the pr-3657 image — the preview picks up the new version on restart. The per-commit URLs point to a specific version and will not change.

Run locally with Docker
docker pull ghcr.io/go-vikunja/vikunja:pr-3657
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-3657

Last updated for commit 2820bf7

@github-actions github-actions Bot added the auto-merge PR has GitHub auto-merge enabled label Aug 29, 2026
@kolaente
kolaente merged commit df258f8 into main Aug 29, 2026
42 checks passed
@kolaente
kolaente deleted the feat-video-preview-followup branch August 29, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/attachments File attachments on tasks: upload, preview, download area/frontend Frontend-only code or UI work area/internal-code Internal refactoring, cleanup, code-quality work auto-merge PR has GitHub auto-merge enabled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants