Repository navigation
FEAT: Share image, audio, and video previews with dataset details - #3251
Julio César Suástegui (juliosuas) wants to merge 10 commits into
Conversation
Co-authored-by: Julio César Suástegui <juliosuas@users.noreply.github.com>
Roman Lutz (romanlutz)
left a comment
Roman Lutz (romanlutz)
left a comment
There was a problem hiding this comment.
Three media preview issues to address:
Cross-origin recovery links open in a new tab. Retry refetches a signed display URL. Tall images stay inside the preview.
A retry replaces only that member's display URL, so other playback keeps its place. The new URL goes through the same allowlist as the first load, and a failed refresh stays on screen.
A retry can still be in flight when the example is closed or replaced. Keep that response from writing its display URL, or its failure, onto the example that is open now.
| const response = await datasetsApi.getSeedExample(exampleId, selectionKey) | ||
| const refreshed = response.members.find((item) => item.id === memberId) | ||
| setDetail((current) => { | ||
| if (current == null || current.example_id !== response.example_id || refreshed == null) return current |
There was a problem hiding this comment.
🔴 Must fix (P2): ignore a refresh after its example has been closed.
Unlike the initial detail request, this refresh is not cancelled or invalidated when the selection changes. Checking example_id is not enough when the same example is reopened.
I reproduced this sequence: start Retry, close the example while that request is pending, then reopen it. The new detail loads ?sig=newly-opened, but when the earlier Retry completes, it overwrites that URL with ?sig=older-refresh. That can restore a stale signature or change the source of media the user has already reopened. The same deferred-response check passes for the initial detail load, which uses an AbortController.
Please bind refreshes to the same request lifecycle as the detail load and abort or ignore them after closing, switching examples, or changing runtime. Check that the request is still current before updating state or returning its URL, and add a close/reopen test with a delayed refresh.
There was a problem hiding this comment.
The refresh follows the same abort lifecycle as the detail load now. Closing the example, switching, or changing runtime aborts it, so a late response can't replace sig=newly-opened with sig=older-refresh. Retrying a second member doesn't cancel one already running either.
A refresh now follows the same request as the open example, so closing it, switching examples, or changing runtime aborts that request before its URL can be applied. Retrying one member no longer cancels a refresh already running for another.
This branch has not been deployed
2 participants
Description
Closes #2749
Dataset example details can now preview image, audio, and video members using the same component as chat. I pulled
ImageWithSpinnerandMediaWithFallbackout ofMessageList.tsxinto a sharedMediaPreview, and chat passes the same props/classes as before, so it renders the same way.In dataset details, nothing is fetched until you click "Show … preview". Then the image uses
loading="lazy", audio/video usepreload="none", nothing autoplays, and hiding or unmounting the preview pauses playback. If a load fails, you get a message with Retry and Download.Backend side: the seed detail response now includes
value_urlon each member. It comes fromresolve_client_media_url_async, a thin public wrapper over the existing_resolve_and_sign_media_async, so local files go through/api/mediaand blob URLs get signed the same way as in chat.valuestays the stable stored reference andvalue_urlis just the display URL. Only the detail endpoint resolves URLs. The list endpoint doesn't touch them, so browsing the table never signs or downloads anything.On the frontend,
mapMediaUrlinutils/media.tsdecides whether a member is a preview, download-only, missing, or not media. It never turns a bare filesystem path into a browser URL, it uses the same inline allowlist asroutes/media.py, and SVG/HTML/other active types are never rendered.A few things I wasn't sure about, happy to change any of them:
binary_pathmembers are download-only, no preview./api/mediait's still downloadable since the route serves it as an attachment.value_url(that's what the existing resolver does) and the UI shows "File is missing." Returningnullmight be cleaner.preload="none", a broken file or an unsupported codec in an allowed container only fails once you hit play. Expired and inaccessible links share one generic error message for now.Screenshots
Dataset detail with media members
Image preview
Audio preview
Video preview
Tests and Documentation
MediaPreviewandmapMediaUrl, plus the dataset detail).MessageList.test.tsxis unchanged and passes 63/63, same as on main.tests/unit/infraflaked while the machine was loaded; that folder passes 398/398 on its own, same as main, and the PR doesn't touch it.)@seededagainst the real backend: 88/88. No retries.datasets.spec.tsrepeated 20x with--fail-on-flaky-tests: 200/200, and@seeded20x: 40/40 (before merging main). It checks there are no/api/mediarequests before you click, no autoplay, and that broken/SVG members don't break the table.