Show loading and error states for annotation and caption exports#1636
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExport downloads now use service handlers instead of generated links. The dialog adds shared loading and error UI, URL-based download handling, focused unit tests, and deterministic end-to-end download assertions. ChangesExport flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ExportSamples
participant ExportService
participant Browser
User->>ExportSamples: Click export button
ExportSamples->>ExportService: Start annotation or caption export
ExportService->>Browser: Trigger constructed download URL
Browser-->>User: Download export
ExportService-->>ExportSamples: Return success or error
ExportSamples-->>User: Update loading or error state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…sampling-and-export-image-annotations-export-download
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b1b284fc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const response = await exportCollectionAnnotations({ | ||
| path: { collection_id }, | ||
| query: { annotation_collection_id, export_format }, | ||
| parseAs: 'blob' |
There was a problem hiding this comment.
For large annotation or segmentation exports, this changes the old anchor-based browser download into fetch plus response.blob(), so the entire JSON/ZIP must be materialized in the tab before triggerDownloadBlob runs. Large YOLO or PASCAL VOC exports can now stall or exhaust browser memory instead of streaming directly through the browser download manager; keep a direct download path for large artifacts or use a streaming approach.
Useful? React with 👍 / 👎.
| const getFilename = (response: Response): string => | ||
| response.headers.get('content-disposition')?.match(/filename="?([^";]+)"?/)?.[1] ?? 'export'; | ||
|
|
||
| export const exportAnnotations = async ({ |
There was a problem hiding this comment.
ai_guidelines/frontend.md says API work should live in small hooks and that “we do not use a services folder”; adding this exported service keeps the request outside a hook and forces loading/error state to be split between the component and service. Consider a component-specific hook for this download mutation instead.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte (1)
136-146: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
exportErroris never reset, permanently disabling the samples export button after one failure.
handleExportonly setsexportErroron failure; nothing ever clears it back to''— not on retry, not on success, not when the export dialog reopens (contrast withhandleAnnotationExport/handleCaptionExport, which explicitly resetannotationExportError = ''at the start of each attempt). SinceerrorMessage = exportError || statErrorfeedsisSubmitDisabled(return !!errorMessage;), once a samples export fails once, the "Download" button in the samples tab becomes permanently disabled for the remainder of the session, with no way to recover short of a full page reload.🐛 Proposed fix
const handleExport = async () => { + exportError = ''; const response = await exportCollection({ collection_id: collectionId, includeFilter, excludeFilter, collectionFilter: $imageFilter }); if (response.error) { exportError = `Export failed: ${response.error}`; } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte` around lines 136 - 146, Reset exportError to an empty string at the start of handleExport before calling exportCollection, so retries and subsequent successful exports clear the prior failure state and allow the samples export button to recover.
🧹 Nitpick comments (2)
lightly_studio_view/src/lib/components/ExportSamples/ExportDownloadButton/ExportDownloadButton.svelte (1)
16-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winButton doesn't self-disable when
isLoadingis true.
disabledandisLoadingare independent props; the underlying<Button>only respects the passeddisabledvalue. Every current call site inExportSamples.sveltehappens to pass matching values, but nothing in this component enforces that contract — a future caller passingisLoading={true}withoutdisabled={true}would let users click through the spinner overlay and trigger duplicate export requests.Consider deriving the effective disabled state internally so the guarantee doesn't rely on caller discipline.
🛡️ Proposed fix
-<Button class="relative my-4 w-full" {disabled} {onclick} data-testid={testId}> +<Button class="relative my-4 w-full" disabled={disabled || isLoading} {onclick} data-testid={testId}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightly_studio_view/src/lib/components/ExportSamples/ExportDownloadButton/ExportDownloadButton.svelte` around lines 16 - 29, Derive an effective disabled state in ExportDownloadButton from disabled || isLoading, and pass that value to the underlying Button instead of disabled alone. Keep the existing props and loading-spinner rendering unchanged while ensuring isLoading always prevents clicks.lightly_studio_view/src/lib/services/exportAnnotations.ts (1)
22-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate error-handling logic and doubled "Error:" prefix in the resulting message.
exportAnnotationsandexportCaptionsrepeat identical try/catch/error-shape logic. Also,throw new Error(JSON.stringify(response.error))followed by'Export failed: ' + String(e)produces a message likeExport failed: Error: "Not Found"becauseString()on anErrorinstance prepends"Error: "(confirmed viaError.prototype.toString()semantics). This user-visible text (rendered byExportAnnotationError) reads awkwardly.Consider extracting a shared helper and using
e.message/e instanceof Error ? e.message : String(e)for a cleaner message.♻️ Proposed refactor
-const getFilename = (response: Response): string => - response.headers.get('content-disposition')?.match(/filename="?([^";]+)"?/)?.[1] ?? 'export'; +const getFilename = (response: Response): string => + response.headers.get('content-disposition')?.match(/filename="?([^";]+)"?/)?.[1] ?? 'export'; + +const runExport = async ( + call: () => Promise<{ data?: unknown; error?: unknown; response: Response }> +): Promise<ExportResult> => { + try { + const response = await call(); + if (response.error) throw new Error(JSON.stringify(response.error)); + if (!response.data) throw new Error('No data'); + triggerDownloadBlob(getFilename(response.response), response.data as Blob); + return {}; + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + return { error: `Export failed: ${message}` }; + } +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightly_studio_view/src/lib/services/exportAnnotations.ts` around lines 22 - 50, Extract the shared export request, validation, download, and error-result handling used by exportAnnotations and exportCaptions into a helper, then have both functions delegate to it. In the helper’s catch path, use the caught Error’s message when available, falling back to String(e), so the returned “Export failed” text contains only one error prefix while preserving existing response and filename behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte`:
- Around line 159-188: Guard the asynchronous state updates in
handleAnnotationExport and handleCaptionExport so completions from requests
started under a previous exportType cannot modify the current tab’s
annotationExportError or isAnnotationExporting. Capture the active tab identity
or per-call token before each request, and only apply the response state when it
still matches after awaiting; preserve the existing reset behavior in the
exportType effect.
In `@lightly_studio_view/src/lib/services/exportAnnotations.ts`:
- Around line 13-35: The annotation and caption export handlers do not pass the
active image filter, so their results ignore the current view. Update
handleAnnotationExport and handleCaptionExport to pass $imageFilter into
exportAnnotations and exportCaptions, and extend the corresponding service
parameter types and request query construction to forward that filter while
preserving existing export behavior.
---
Outside diff comments:
In `@lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte`:
- Around line 136-146: Reset exportError to an empty string at the start of
handleExport before calling exportCollection, so retries and subsequent
successful exports clear the prior failure state and allow the samples export
button to recover.
---
Nitpick comments:
In
`@lightly_studio_view/src/lib/components/ExportSamples/ExportDownloadButton/ExportDownloadButton.svelte`:
- Around line 16-29: Derive an effective disabled state in ExportDownloadButton
from disabled || isLoading, and pass that value to the underlying Button instead
of disabled alone. Keep the existing props and loading-spinner rendering
unchanged while ensuring isLoading always prevents clicks.
In `@lightly_studio_view/src/lib/services/exportAnnotations.ts`:
- Around line 22-50: Extract the shared export request, validation, download,
and error-result handling used by exportAnnotations and exportCaptions into a
helper, then have both functions delegate to it. In the helper’s catch path, use
the caught Error’s message when available, falling back to String(e), so the
returned “Export failed” text contains only one error prefix while preserving
existing response and filename behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4b64f812-8c0b-48de-ab9a-9c952d0e472b
📒 Files selected for processing (10)
lightly_studio_view/e2e/general/export-annotations.e2e-test.tslightly_studio_view/e2e/general/export-captions.e2e-test.tslightly_studio_view/e2e/general/export-segmentation-masks.e2e-test.tslightly_studio_view/src/lib/components/ExportSamples/ExportAnnotationError/ExportAnnotationError.sveltelightly_studio_view/src/lib/components/ExportSamples/ExportAnnotationError/ExportAnnotationError.test.tslightly_studio_view/src/lib/components/ExportSamples/ExportDownloadButton/ExportDownloadButton.sveltelightly_studio_view/src/lib/components/ExportSamples/ExportDownloadButton/ExportDownloadButton.test.tslightly_studio_view/src/lib/components/ExportSamples/ExportSamples.sveltelightly_studio_view/src/lib/services/exportAnnotations.test.tslightly_studio_view/src/lib/services/exportAnnotations.ts
💤 Files with no reviewable changes (3)
- lightly_studio_view/e2e/general/export-captions.e2e-test.ts
- lightly_studio_view/e2e/general/export-annotations.e2e-test.ts
- lightly_studio_view/e2e/general/export-segmentation-masks.e2e-test.ts
| $effect(() => { | ||
| if (exportType) { | ||
| annotationExportError = ''; | ||
| isAnnotationExporting = false; | ||
| } | ||
| }); | ||
|
|
||
| // | ||
| // YouTube-VIS video Segmentation mask export | ||
| // | ||
| const exportYoutubeVisSegmentationMaskURL = `${PUBLIC_LIGHTLY_STUDIO_API_URL}api/collections/${collectionId}/export/youtube-vis?ts=${Date.now()}&export_format=youtube_vis_segmentation`; | ||
| // Semantic segmentation export | ||
| // | ||
| const exportPascalVocURL = $derived( | ||
| `${PUBLIC_LIGHTLY_STUDIO_API_URL}api/collections/${collectionId}/export/annotations?ts=${Date.now()}&export_format=pascal_voc${annotationCollectionParam}` | ||
| ); | ||
| const handleAnnotationExport = async (export_format: ExportFormat) => { | ||
| annotationExportError = ''; | ||
| isAnnotationExporting = true; | ||
| const response = await exportAnnotations({ | ||
| collection_id: collectionId, | ||
| annotation_collection_id: effectiveAnnotationCollectionId ?? null, | ||
| export_format | ||
| }); | ||
| isAnnotationExporting = false; | ||
| if (response.error) { | ||
| annotationExportError = response.error; | ||
| } | ||
| }; | ||
|
|
||
| // | ||
| // Caption export | ||
| // | ||
| const exportCaptionsURL = `${PUBLIC_LIGHTLY_STUDIO_API_URL}api/collections/${collectionId}/export/captions?ts=${Date.now()}`; | ||
| const handleCaptionExport = async () => { | ||
| annotationExportError = ''; | ||
| isAnnotationExporting = true; | ||
| const response = await exportCaptions(collectionId); | ||
| isAnnotationExporting = false; | ||
| if (response.error) { | ||
| annotationExportError = response.error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale annotation/caption export state can leak across tabs after switching mid-request.
The $effect resets annotationExportError/isAnnotationExporting when exportType changes, but it doesn't cancel or ignore in-flight handleAnnotationExport/handleCaptionExport calls. If a user switches tabs while a request is pending, the earlier call's .then continuation still executes afterward and can set isAnnotationExporting = false or annotationExportError = response.error on the new, unrelated tab — showing a stale spinner/error for a request the user didn't initiate on that tab.
Consider guarding the state writes with a check that exportType (or a per-call token) hasn't changed since the request was issued.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte`
around lines 159 - 188, Guard the asynchronous state updates in
handleAnnotationExport and handleCaptionExport so completions from requests
started under a previous exportType cannot modify the current tab’s
annotationExportError or isAnnotationExporting. Capture the active tab identity
or per-call token before each request, and only apply the response state when it
still matches after awaiting; preserve the existing reset behavior in the
exportType effect.
| export const exportAnnotations = async ({ | ||
| collection_id, | ||
| annotation_collection_id, | ||
| export_format | ||
| }: { | ||
| collection_id: string; | ||
| annotation_collection_id: string | null; | ||
| export_format?: ExportFormat; | ||
| }): Promise<ExportResult> => { | ||
| try { | ||
| const response = await exportCollectionAnnotations({ | ||
| path: { collection_id }, | ||
| query: { annotation_collection_id, export_format }, | ||
| parseAs: 'blob' | ||
| }); | ||
| if (response.error) throw new Error(JSON.stringify(response.error)); | ||
| if (!response.data) throw new Error('No data'); | ||
| triggerDownloadBlob(getFilename(response.response), response.data as Blob); | ||
| return {}; | ||
| } catch (e) { | ||
| return { error: 'Export failed: ' + String(e) }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the generated SDK query types for these endpoints support a filter param
ast-grep run --pattern 'exportCollectionAnnotations' --lang typescript lightly_studio_view/src/lib/api/lightly_studio_local
rg -n -A5 'exportCollectionAnnotations|exportCollectionCaptions' lightly_studio_view/src/lib/api/lightly_studio_local/sdk.gen.ts lightly_studio_view/src/lib/api/lightly_studio_local/types.gen.tsRepository: lightly-ai/lightly-studio
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and symbols first.
git ls-files | rg 'exportAnnotations\.ts|exportCaptions\.ts|ExportSamples\.svelte|sdk\.gen\.ts|types\.gen\.ts|lightly_studio_local'
# Show compact outlines for the likely files once found.
for f in $(git ls-files | rg 'exportAnnotations\.ts|exportCaptions\.ts|ExportSamples\.svelte|sdk\.gen\.ts|types\.gen\.ts'); do
echo "=== $f ==="
ast-grep outline "$f" --view expanded | sed -n '1,220p'
done
# Search for export endpoint signatures and any filter-related params/usages.
rg -n -A6 -B3 'exportCollectionAnnotations|exportCollectionCaptions|collectionFilter|annotation_collection_id|export_format|handleAnnotationExport|handleCaptionExport' .Repository: lightly-ai/lightly-studio
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow down the relevant frontend files.
git ls-files | rg 'lightly_studio_view/src/lib/services/export(Annotations|Captions)\.ts|lightly_studio_view/src/lib/components/ExportSamples\.svelte|lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts'
# Show the actual export service implementations.
for f in lightly_studio_view/src/lib/services/exportAnnotations.ts lightly_studio_view/src/lib/services/exportCaptions.ts lightly_studio_view/src/lib/components/ExportSamples.svelte; do
echo "=== $f ==="
wc -l "$f"
ast-grep outline "$f" --view expanded | sed -n '1,220p'
done
# Inspect the specific call sites and parameters.
rg -n -A8 -B6 'handleAnnotationExport|handleCaptionExport|exportAnnotations\(|exportCaptions\(|collectionFilter|imageFilter|active view|activeView|filter' lightly_studio_view/src/lib/components/ExportSamples.svelte lightly_studio_view/src/lib/services/exportAnnotations.ts lightly_studio_view/src/lib/services/exportCaptions.ts
# Inspect generated client signatures for these endpoints.
for f in $(git ls-files | rg 'lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts$'); do
echo "=== $f ==="
rg -n -A10 -B4 'exportCollectionAnnotations|exportCollectionCaptions|collectionFilter|annotation_collection_id|export_format' "$f"
doneRepository: lightly-ai/lightly-studio
Length of output: 787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant source slices.
echo "=== lightly_studio_view/src/lib/services/exportAnnotations.ts ==="
cat -n lightly_studio_view/src/lib/services/exportAnnotations.ts | sed -n '1,140p'
echo "=== lightly_studio_view/src/lib/components/ExportSamples.svelte ==="
cat -n lightly_studio_view/src/lib/components/ExportSamples.svelte | sed -n '1,260p'
# Locate generated API files and inspect the endpoint signatures.
echo "=== generated api files ==="
git ls-files | rg 'lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts$'
for f in $(git ls-files | rg 'lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts$'); do
echo "=== $f ==="
rg -n -A8 -B4 'exportCollectionAnnotations|exportCollectionCaptions|annotation_collection_id|export_format|collectionFilter|filter' "$f"
doneRepository: lightly-ai/lightly-studio
Length of output: 2453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual export UI file(s) and related call sites.
git ls-files | rg 'ExportSamples\.svelte|exportAnnotations\.ts|exportCaptions\.ts|handleAnnotationExport|handleCaptionExport|collectionFilter|imageFilter|active view|activeView'
# Inspect the generated client files that define these endpoints.
for f in $(git ls-files | rg 'lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts$'); do
echo "=== $f ==="
rg -n -A12 -B6 'exportCollectionAnnotations|exportCollectionCaptions|annotation_collection_id|export_format|collectionFilter|filter' "$f"
doneRepository: lightly-ai/lightly-studio
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the actual frontend files involved.
echo "=== lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte ==="
cat -n lightly_studio_view/src/lib/components/ExportSamples/ExportSamples.svelte | sed -n '1,320p'
echo "=== lightly_studio_view/src/lib/services/exportAnnotations.ts ==="
cat -n lightly_studio_view/src/lib/services/exportAnnotations.ts | sed -n '1,220p'
# Locate the generated API directory and inspect endpoint signatures.
git ls-files | rg 'lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts$'
for f in $(git ls-files | rg 'lightly_studio_view/src/lib/api/.*/(sdk|types)\.gen\.ts$'); do
echo "=== $f ==="
rg -n -A12 -B6 'exportCollectionAnnotations|exportCollectionCaptions|annotation_collection_id|export_format|collectionFilter|filter' "$f"
doneRepository: lightly-ai/lightly-studio
Length of output: 19027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact generated API definitions and backend routes for these exports.
rg -n -A14 -B6 'exportCollectionAnnotations|exportCollectionCaptions' lightly_studio_view lightly_studio
# If the generated client files exist under a different path, list them.
git ls-files | rg 'sdk\.gen\.ts$|types\.gen\.ts$|api/.*/lightly_studio_local'Repository: lightly-ai/lightly-studio
Length of output: 23544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the generated SDK module behind '$lib/api/lightly_studio_local'.
fd -a 'lightly_studio_local' lightly_studio_view/src/lib/api
git ls-files | rg 'lightly_studio_view/src/lib/api/.*(lightly_studio_local|sdk\.gen\.ts|types\.gen\.ts|index\.ts)$'
# Inspect the export endpoint definitions and any query params they accept.
for f in $(git ls-files | rg 'lightly_studio_view/src/lib/api/.*(lightly_studio_local|sdk\.gen\.ts|types\.gen\.ts|index\.ts)$'); do
echo "=== $f ==="
rg -n -A12 -B6 'exportCollectionAnnotations|exportCollectionCaptions|collectionFilter|annotation_collection_id|export_format|filter' "$f"
doneRepository: lightly-ai/lightly-studio
Length of output: 282
Thread the active-view filter through annotation and caption exports. ExportSamples/ExportSamples.svelte already passes $imageFilter to sample export, but handleAnnotationExport/handleCaptionExport still call exportAnnotations/exportCaptions without any filter. Add the filter to these service calls so these exports follow the active view too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lightly_studio_view/src/lib/services/exportAnnotations.ts` around lines 13 -
35, The annotation and caption export handlers do not pass the active image
filter, so their results ignore the current view. Update handleAnnotationExport
and handleCaptionExport to pass $imageFilter into exportAnnotations and
exportCaptions, and extend the corresponding service parameter types and request
query construction to forward that filter while preserving existing export
behavior.
…d-export-image-annotations-export-download' of https://github.com/lightly-ai/lightly-studio into leonardo-lig-8988-figure-out-add-context-to-sampling-and-export-image-annotations-export-download
1 participant
What has changed and why?
This PR is a follow-up to #1632 and improves the export experience for annotations and captions.
Instead of relying on direct download links, annotation and caption exports are now triggered through frontend service calls. This makes it possible to keep the user in the dialog, surface export failures inline, and show progress while the request is running.
How has it been tested?
Manual test + unit tests
Did you update CHANGELOG.md?
Summary by CodeRabbit
href/target behavior.