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

frontend/src/components/tasks/partials/Attachments.vue

@@ -170,36 +170,39 @@
 		</Modal>
 
 		<ImageLightbox
-			v-if="attachmentImageBlobUrl !== null"
-			:key="attachmentImageBlobUrl"
-			:blob-url="attachmentImageBlobUrl"
-			:alt="attachmentImageAlt"
-			@close="closeImageLightbox"
+			v-if="preview?.kind === 'image'"
+			:key="preview.blobUrl"
+			:blob-url="preview.blobUrl"
+			:alt="preview.name"
+			@close="closePreview"
 		/>
 
 		<!-- Attachment PDF modal -->
 		<Modal
-			:enabled="attachmentPdfBlobUrl !== null"
+			:enabled="preview?.kind === 'pdf'"
 			:wide="true"
 			:aria-label="$t('misc.pdfPreview')"
-			@close="closePdfPreview"
+			@close="closePreview"
 		>
 			<iframe
-				v-if="attachmentPdfBlobUrl"
-				:src="attachmentPdfBlobUrl"
+				v-if="preview?.kind === 'pdf'"
+				:src="preview.blobUrl"
 				class="pdf-preview-iframe"
 			/>
 		</Modal>
 
 		<!-- Attachment video modal -->
 		<Modal
-			:enabled="attachmentVideoLoading || attachmentVideoBlobUrl !== null"
+			:enabled="previewLoading || preview?.kind === 'video'"
 			:wide="true"
 			:aria-label="$t('misc.videoPreview')"
-			@close="closeVideoPreview"
+			@close="closePreview"
 		>
-			<Loading v-if="attachmentVideoLoading" />
-			<div v-else-if="attachmentVideoFailed">
+			<Loading v-if="previewLoading" />
+			<div
+				v-else-if="previewFailed"
+				class="video-preview-error"
+			>
 				<p>{{ $t('misc.videoLoadFailed') }}</p>
 				<XButton
 					icon="download"
@@ -210,9 +213,9 @@
 				</XButton>
 			</div>
 			<video
-				v-else-if="attachmentVideoBlobUrl"
-				:src="attachmentVideoBlobUrl"
-				:aria-label="attachmentVideoName"
+				v-else-if="preview?.kind === 'video'"
+				:src="preview.blobUrl"
+				:aria-label="preview.name"
 				class="video-preview"
 				controls
 				playsinline
@@ -223,7 +226,7 @@
 </template>
 
 <script setup lang="ts">
-import {ref, shallowReactive, computed, watch, onMounted, onBeforeUnmount, type Ref, type ComponentPublicInstance} from 'vue'
+import {ref, shallowReactive, computed, watch, onMounted, onBeforeUnmount, type ComponentPublicInstance} from 'vue'
 import {useDropZone} from '@vueuse/core'
 
 import User from '@/components/misc/User.vue'
@@ -232,7 +235,7 @@ import Loading from '@/components/misc/Loading.vue'
 import BaseButton from '@/components/base/BaseButton.vue'
 
 import AttachmentService from '@/services/attachment'
-import {canPreviewAudio, canPreviewImage, canPreviewPdf, canPreviewVideo} from '@/models/attachment'
+import {canPreviewAudio, canPreviewImage, previewKind, type PreviewKind} from '@/models/attachment'
 import {getDisplayName} from '@/models/user'
 import type {IAttachment} from '@/modelTypes/IAttachment'
 import type {ITask} from '@/modelTypes/ITask'
@@ -461,66 +464,53 @@ async function deleteAttachment() {
 	}
 }
 
-const attachmentImageBlobUrl = ref<string | null>(null)
-const attachmentImageAlt = ref('')
-const attachmentPdfBlobUrl = ref<string | null>(null)
-const attachmentVideoBlobUrl = ref<string | null>(null)
-const attachmentVideoName = ref('')
-const attachmentVideoLoading = ref(false)
-const attachmentVideoFailed = ref(false)
-let previewRequestToken = 0
-
-function replaceBlobUrl(target: Ref<string | null>, blobUrl: string | null) {
-	if (target.value !== null) {
-		URL.revokeObjectURL(target.value)
-	}
-	target.value = blobUrl
+interface Preview {
+	kind: PreviewKind
+	blobUrl: string
+	name: string
 }
 
-function closeImageLightbox() {
-	replaceBlobUrl(attachmentImageBlobUrl, null)
-	attachmentImageAlt.value = ''
-}
+const preview = ref<Preview | null>(null)
+const previewLoading = ref(false)
+const previewFailed = ref(false)
+let previewRequestToken = 0
 
-function closePdfPreview() {
-	replaceBlobUrl(attachmentPdfBlobUrl, null)
+function replacePreview(next: Preview | null) {
+	if (preview.value !== null) {
+		URL.revokeObjectURL(preview.value.blobUrl)
+	}
+	preview.value = next
 }
 
-function closeVideoPreview() {
+function closePreview() {
 	// an in-flight blob must not re-open the dismissed modal
 	previewRequestToken++
-	replaceBlobUrl(attachmentVideoBlobUrl, null)
-	attachmentVideoName.value = ''
-	attachmentVideoLoading.value = false
-	attachmentVideoFailed.value = false
+	replacePreview(null)
+	previewLoading.value = false
+	previewFailed.value = false
 }
 
 // a detached <video> can still fire error after its blob url was revoked
 function onVideoError(e: Event) {
-	if ((e.target as HTMLVideoElement).src !== attachmentVideoBlobUrl.value) {
+	if ((e.target as HTMLVideoElement).src !== preview.value?.blobUrl) {
 		return
 	}
-	attachmentVideoFailed.value = true
+	previewFailed.value = true
 }
 
 function downloadVideoPreview() {
-	const blobUrl = attachmentVideoBlobUrl.value
-	if (blobUrl === null) {
+	const current = preview.value
+	if (current === null) {
 		return
 	}
 
 	// downloadBlob revokes the url itself, so hand over ownership before closing
-	attachmentVideoBlobUrl.value = null
-	downloadBlob(blobUrl, attachmentVideoName.value)
-	closeVideoPreview()
+	preview.value = null
+	downloadBlob(current.blobUrl, current.name)
+	closePreview()
 }
 
-onBeforeUnmount(() => {
-	previewRequestToken++
-	closeImageLightbox()
-	closePdfPreview()
-	closeVideoPreview()
-})
+onBeforeUnmount(closePreview)
 
 const audioPlayers = new Map<IAttachment['id'], AudioPreviewInstance>()
 
@@ -539,19 +529,19 @@ async function viewOrDownload(attachment: IAttachment) {
 		return
 	}
 
-	if (!canPreviewImage(attachment) && !canPreviewPdf(attachment) && !canPreviewVideo(attachment)) {
+	const kind = previewKind(attachment)
+	if (kind === null) {
 		downloadAttachment(attachment)
 		return
 	}
 
-	const isVideo = canPreviewVideo(attachment)
-	closeVideoPreview()
+	closePreview()
 
 	previewRequestToken++
 	const requestToken = previewRequestToken
 
 	// only video is big enough that the full-buffer wait reads as a dead click
-	attachmentVideoLoading.value = isVideo
+	previewLoading.value = kind === 'video'
 
 	try {
 		const blobUrl = await attachmentService.getBlobUrl(attachment)
@@ -560,22 +550,11 @@ async function viewOrDownload(attachment: IAttachment) {
 			URL.revokeObjectURL(blobUrl)
 			return
 		}
-		attachmentVideoLoading.value = false
-		if (canPreviewImage(attachment)) {
-			replaceBlobUrl(attachmentImageBlobUrl, blobUrl)
-			attachmentImageAlt.value = attachment.file.name
-		} else if (isVideo) {
-			replaceBlobUrl(attachmentVideoBlobUrl, blobUrl)
-			attachmentVideoName.value = attachment.file.name
-		} else if (canPreviewPdf(attachment)) {
-			replaceBlobUrl(attachmentPdfBlobUrl, blobUrl)
-		} else {
-			URL.revokeObjectURL(blobUrl)
-			downloadAttachment(attachment)
-		}
+		previewLoading.value = false
+		replacePreview({kind, blobUrl, name: attachment.file.name})
 	} catch (e) {
 		if (requestToken === previewRequestToken) {
-			attachmentVideoLoading.value = false
+			previewLoading.value = false
 		}
 		error(e)
 	}
@@ -806,6 +785,21 @@ defineExpose({
 	display: block;
 }
 
+// unlike the video and iframe branches, the error state has no opaque media of its own to sit on
+.video-preview-error {
+	max-inline-size: 25rem;
+	margin: 0 auto;
+	padding: 2rem;
+	border-radius: $radius;
+	background: var(--white);
+	color: var(--text);
+	text-align: center;
+
+	p {
+		margin-block-end: 1rem;
+	}
+}
+
 .is-task-cover {
 	background: var(--primary);
 	color: var(--white);

frontend/src/components/tasks/partials/FilePreview.vue

@@ -28,6 +28,17 @@
 		/>
 	</div>
 
+	<!-- Video icon -->
+	<div
+		v-else-if="isVideo"
+		class="icon-wrapper"
+	>
+		<Icon
+			size="6x"
+			icon="play"
+		/>
+	</div>
+
 	<!-- Fallback -->
 	<div
 		v-else
@@ -44,7 +55,7 @@
 import {computed, ref, shallowReactive, watchEffect} from 'vue'
 import AttachmentService, {PREVIEW_SIZE} from '@/services/attachment'
 import type {IAttachment} from '@/modelTypes/IAttachment'
-import {canPreviewAudio, canPreviewImage, canPreviewPdf} from '@/models/attachment'
+import {canPreviewAudio, canPreviewImage, canPreviewPdf, canPreviewVideo} from '@/models/attachment'
 
 const props = defineProps<{
 	modelValue?: IAttachment
@@ -54,6 +65,7 @@ const attachmentService = shallowReactive(new AttachmentService())
 const blobUrl = ref<string | undefined>(undefined)
 const isPdf = computed(() => props.modelValue && canPreviewPdf(props.modelValue))
 const isAudio = computed(() => props.modelValue && canPreviewAudio(props.modelValue))
+const isVideo = computed(() => props.modelValue && canPreviewVideo(props.modelValue))
 
 watchEffect(async () => {
 	if (props.modelValue && canPreviewImage(props.modelValue)) {

frontend/src/models/attachment.test.ts

@@ -1,6 +1,6 @@
 import {describe, it, expect} from 'vitest'
 
-import {canPreviewAudio, canPreviewImage, canPreviewPdf, canPreviewVideo} from './attachment'
+import {canPreviewAudio, canPreviewImage, canPreviewPdf, canPreviewVideo, previewKind} from './attachment'
 import type {IAttachment} from '@/modelTypes/IAttachment'
 
 function attachment(name: string, mime: string): IAttachment {
@@ -58,35 +58,53 @@ describe('canPreviewAudio', () => {
 })
 
 describe('canPreviewVideo', () => {
-	it('previews a real mp4', () => {
+	it('previews an mp4', () => {
 		expect(canPreviewVideo(attachment('clip.mp4', 'video/mp4'))).toBe(true)
 	})
 
-	it('previews a real webm', () => {
+	it('previews a webm', () => {
 		expect(canPreviewVideo(attachment('clip.webm', 'video/webm'))).toBe(true)
 	})
 
-	it('refuses text bytes disguised as an mp4', () => {
-		expect(canPreviewVideo(attachment('evil.mp4', 'text/plain'))).toBe(false)
+	it('previews a container without a known suffix', () => {
+		expect(canPreviewVideo(attachment('clip.mkv', 'video/x-matroska'))).toBe(true)
 	})
 
-	it('refuses a video mime without a video suffix', () => {
-		expect(canPreviewVideo(attachment('clip.txt', 'video/mp4'))).toBe(false)
+	it('previews an ogg regardless of the file name', () => {
+		expect(canPreviewVideo(attachment('19-40-43', 'video/ogg'))).toBe(true)
 	})
 
 	it('matches the mime case-insensitively', () => {
-		expect(canPreviewVideo(attachment('clip.MP4', 'VIDEO/MP4'))).toBe(true)
+		expect(canPreviewVideo(attachment('clip.mp4', 'VIDEO/MP4'))).toBe(true)
+	})
+
+	it('refuses text bytes disguised as an mp4', () => {
+		expect(canPreviewVideo(attachment('evil.mp4', 'text/plain'))).toBe(false)
 	})
 
 	it('refuses audio ogg', () => {
 		expect(canPreviewVideo(attachment('song.ogg', 'audio/ogg'))).toBe(false)
 	})
+})
+
+describe('previewKind', () => {
+	it('maps an image to image', () => {
+		expect(previewKind(attachment('pic.png', 'image/png'))).toBe('image')
+	})
+
+	it('maps a pdf to pdf', () => {
+		expect(previewKind(attachment('doc.pdf', 'application/pdf'))).toBe('pdf')
+	})
+
+	it('maps an mp4 to video', () => {
+		expect(previewKind(attachment('clip.mp4', 'video/mp4'))).toBe('video')
+	})
 
-	it('previews video ogg', () => {
-		expect(canPreviewVideo(attachment('clip.ogg', 'video/ogg'))).toBe(true)
+	it('returns null for a plain text file', () => {
+		expect(previewKind(attachment('notes.txt', 'text/plain'))).toBeNull()
 	})
 
-	it('refuses a video mime with an unsupported .mkv suffix', () => {
-		expect(canPreviewVideo(attachment('clip.mkv', 'video/x-matroska'))).toBe(false)
+	it('returns null for audio, which does not use the blob preview path', () => {
+		expect(previewKind(attachment('memo.mp3', 'audio/mpeg'))).toBeNull()
 	})
 })

frontend/src/models/attachment.ts

@@ -7,7 +7,6 @@ import type { IAttachment } from '@/modelTypes/IAttachment'
 
 export const SUPPORTED_IMAGE_SUFFIX = ['.jpeg', '.jpg', '.png', '.bmp', '.gif']
 export const SUPPORTED_PDF_SUFFIX = ['.pdf']
-export const SUPPORTED_VIDEO_SUFFIX = ['.mp4', '.webm', '.ogg', '.ogv', '.mov', '.m4v']
 
 export function canPreviewImage(attachment: IAttachment): boolean {
 	const mime = attachment.file.mime.toLowerCase()
@@ -28,10 +27,24 @@ export function canPreviewAudio(attachment: IAttachment): boolean {
 	return attachment.file.mime.toLowerCase().startsWith('audio/')
 }
 
+// No suffix allowlist, for the same reason as audio: a <video> element neither parses HTML nor executes script, so the sniffed mime is the whole boundary.
 export function canPreviewVideo(attachment: IAttachment): boolean {
-	const mime = attachment.file.mime.toLowerCase()
-	return SUPPORTED_VIDEO_SUFFIX.some((suffix) => attachment.file.name.toLowerCase().endsWith(suffix))
-		&& mime.startsWith('video/')
+	return attachment.file.mime.toLowerCase().startsWith('video/')
+}
+
+export type PreviewKind = 'image' | 'pdf' | 'video'
+
+export function previewKind(attachment: IAttachment): PreviewKind | null {
+	if (canPreviewImage(attachment)) {
+		return 'image'
+	}
+	if (canPreviewPdf(attachment)) {
+		return 'pdf'
+	}
+	if (canPreviewVideo(attachment)) {
+		return 'video'
+	}
+	return null
 }
 
 export default class AttachmentModel extends AbstractModel<IAttachment> implements IAttachment {