feat:inline media players for audio/video - #1677
Conversation
…h for media playing
There was a problem hiding this comment.
Pull request overview
Adds inline playback for audio/video attachments (authenticated blob fetch → object URLs) in both the Report Manager attachment field and the Detail View Activity section, and extends the existing ImageModal to support fullscreen video rendering.
Changes:
- Introduces
fetchFileAsObjectUrlFromUrl()utility to fetch media as a blob and create an object URL. - Replaces download-only UX for saved audio/video attachments with inline players (and fullscreen video in Activity section).
- Adds styling + i18n strings and expands test coverage for the new media behaviors.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/file/index.js | Adds fetchFileAsObjectUrlFromUrl() helper for authenticated blob fetching + object URL creation. |
| src/utils/file/index.test.js | Adds unit test coverage for the new object-URL helper. |
| src/ReportManager/DetailsSection/SchemaForm/formElements/Attachment/index.js | Adds play/close UX and inline <audio>/<video> rendering for saved media attachments. |
| src/ReportManager/DetailsSection/SchemaForm/formElements/Attachment/index.test.js | Updates tests to expect play/close behavior and loading spinner for media. |
| src/ReportManager/DetailsSection/SchemaForm/formElements/Attachment/styles.module.scss | Adds layout + player styling and a loading spinner for inline media. |
| src/DetailViewComponents/ActivitySection/index.js | Expands “Expand All” behavior to include audio/video attachments (not just images). |
| src/DetailViewComponents/ActivitySection/index.test.js | Adds test ensuring Expand All expands audio/video attachment collapses. |
| src/DetailViewComponents/ActivitySection/styles.module.scss | Adds media preview sizing styles and a loading spinner animation. |
| src/DetailViewComponents/ActivitySection/AttachmentListItem/index.js | Adds audio/video preview rendering, authenticated media fetch via object URLs, and fullscreen video modal support. |
| src/DetailViewComponents/ActivitySection/AttachmentListItem/index.test.js | Adds extensive tests for audio/video preview, loading state, and fullscreen video modal update flow. |
| src/ImageModal/index.js | Adds mediaType prop and renders <video> when mediaType === 'video'. |
| src/ImageModal/index.test.js | New tests for video rendering, error state, and download behavior. |
| src/ImageModal/styles.module.scss | Extends sizing rules to apply to video as well as img. |
| public/locales/en-US/reports.json | Adds play/close/player ARIA labels for SchemaForm attachments. |
| public/locales/es/reports.json | Same as above (es). |
| public/locales/fr/reports.json | Same as above (fr). |
| public/locales/ne-NP/reports.json | Same as above (ne-NP). |
| public/locales/pt/reports.json | Same as above (pt). |
| public/locales/sw/reports.json | Same as above (sw). |
| public/locales/en-US/details-view.json | Adds audio/video preview alt strings for Activity section attachment previews. |
| public/locales/es/details-view.json | Same as above (es). |
| public/locales/fr/details-view.json | Same as above (fr). |
| public/locales/ne-NP/details-view.json | Same as above (ne-NP). |
| public/locales/pt/details-view.json | Same as above (pt). |
| public/locales/sw/details-view.json | Same as above (sw). |
| useEffect(() => { | ||
| if (isPlayerOpen && isMedia && attachment.originalUrl && !mediaFetchPromiseRef.current) { | ||
| mediaFetchPromiseRef.current = fetchFileAsObjectUrlFromUrl(attachment.originalUrl).then((objectUrl) => { | ||
| setMediaObjectUrl(objectUrl); | ||
| }); | ||
| } | ||
| }, [attachment.originalUrl, isMedia, isPlayerOpen]); |
| useEffect(() => () => { | ||
| if (mediaObjectUrl) { | ||
| URL.revokeObjectURL(mediaObjectUrl); | ||
| } | ||
| }, [mediaObjectUrl]); |
| const ensureMediaObjectUrl = useCallback(() => { | ||
| if (!mediaFetchPromiseRef.current) { | ||
| mediaFetchPromiseRef.current = fetchFileAsObjectUrlFromUrl(attachment.url).then((objectUrl) => { | ||
| setMediaObjectUrl(objectUrl); | ||
|
|
||
| return objectUrl; | ||
| }); | ||
| } | ||
|
|
||
| return mediaFetchPromiseRef.current; | ||
| }, [attachment.url]); |
| {!error && mediaType === 'video' && <video | ||
| aria-label={title} | ||
| controls | ||
| onError={setImageError} | ||
| onLoadedData={setImageLoaded} | ||
| ref={imageRef} | ||
| src={src} | ||
| style={{ display: loaded ? 'block' : 'none' }} | ||
| />} | ||
|
|
||
| {!error && mediaType === 'image' && <img |
luixlive
left a comment
luixlive
left a comment
There was a problem hiding this comment.
Some feedback from me and some from Claude. Candidly, some of these changes feel like a patch over our implementation for images, with hardcoded conditions and complex flows with several useEffects and different fetching and error branches. I feel like it would be cleaner to have specific methods and components per file type so the code is easier to understand, which also makes it less error-prone and easier to test. Of course we can have shared files to avoid repetition.
| "collapseOpenButtonLabel": "Collapse", | ||
| "collapseOpenButtonTitle": "Expanded", | ||
| "imagePreviewAlt": "{{fileName}} preview", | ||
| "videoPreviewAlt": "{{fileName}} video preview", |
There was a problem hiding this comment.
nit: Sort the keys alphabetically so it's easier to search for stuff 👍
| }); | ||
| } | ||
|
|
||
| return mediaFetchPromiseRef.current; |
There was a problem hiding this comment.
Why is this returning the ref? The places from where it is called are not storing it, and the refs are anyway accessible from anywhere. It's a bit misleading.
| {!loaded && <LoadingOverlay />} | ||
| {!loaded && !fetchError && <LoadingOverlay />} | ||
|
|
||
| {!showError && mediaType === 'video' && <video |
There was a problem hiding this comment.
This component is called ImageModal. The clean solution here would be to either have a different specific modal for video (if worth it), or renaming this one to a more generic name.
|
|
||
| const onShowImageFullScreen = useCallback((event) => { | ||
| const mediaFetchPromiseRef = useRef(null); | ||
| const pendingModalIdRef = useRef(null); |
There was a problem hiding this comment.
From Claude:
A single pendingModalIdRef is overwritten on every fullscreen click, so opening the same video's fullscreen modal twice in quick succession leaves the first modal permanently stuck showing the loading spinner.
|
|
||
| useEffect(() => () => { | ||
| if (mediaObjectUrl) { | ||
| URL.revokeObjectURL(mediaObjectUrl); |
There was a problem hiding this comment.
From Claude:
The object URL created for video/audio playback is revoked on component unmount, but a fullscreen ImageModal referencing that same URL lives in global Redux modal state and isn't closed when the detail view unmounts.
|
|
||
| $cardExpansionTransitionTime: 300ms; | ||
|
|
||
| @keyframes mediaLoadingSpin { |
There was a problem hiding this comment.
From Claude:
A new hand-rolled CSS spinner (mediaLoadingSpin keyframes + .mediaLoadingSpinner) duplicates the existing LoadingOverlay component that this same PR already uses one file away.
| ? <span className={styles.error}>{t('playerErrorLabel')}</span> | ||
| : mediaObjectUrl | ||
| ? (attachment.fileType === 'audio' | ||
| ? <audio |
There was a problem hiding this comment.
Here we have like 3 nested ternary operators, which makes it hard to reason about. We should abstract some of this code into separate components or methods.
[draft]