Add API for downloading media file metadata #1930
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a JS-invokable MediaFilesService across backend and frontend: new service enum and provider wiring, LcmMediaService APIs for listing/uploading/downloading resources and fetching file metadata, TypeGen exports, TypeScript interfaces, service registry/context updates, and a new frontend dialog and sidebar integration for managing media file transfers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
| @@ -8,6 +8,9 @@ public interface IMediaServerClient | |||
| [Get("/api/media/{fileId}")] | |||
| Task<HttpResponseMessage> DownloadFile(Guid fileId); | |||
|
|
|||
| [Get("/api/media/metadata/{fileId}")] | |||
| Task<HttpResponseMessage> GetFileMetadata(Guid fileId); | |||
There was a problem hiding this comment.
It probably makes more sense to just use the return type of LcmFileMetadata here, and the request library will handle errors for us. DownloadFile was a special case.
There was a problem hiding this comment.
Got it, thanks. That will make it easier, I won't have to jump through quite as many hoops. (It was just a couple of hoops, but still).
There was a problem hiding this comment.
Haven't gotten around to this one yet, BTW. I currently don't expose a GetFileMetadata call in the JsInvokable API (just downloading and uploading all files, or a selected list of files), so perhaps I should delete this? Although come to think of it, if we want to let users pick and choose files to download selectively then they're going to need to know at least the filename of the file, so maybe this one is needed after all.
Will eventually add API to do this in batch form for all media files in the project, then we can (e.g.) download metadata immediately and download file contents on-demand, or download and cache all file contents prior to going offline.
rmunn
force-pushed
the
feat/media-file-metadata-apis
branch
from
4fbcf0e to
b16bb5d
Compare
Will be used in frontend to let the API call a simpler method if the "Select All" checkbox is checked, but we still need a method to download a selected list of resources in case the user wants to omit some.
This will be used if the frontend UI shows the user a list of available resources and he chooses to select only some of them rather than all.
Will be used in media files dialog (to be created).
Copy and paste error here
Was calling wrong remote API
|
I've made no attempt to fix lint errors or anything else just yet. @hahn-kev, if you want to wait until this is mergeable before reviewing it, that's fine. I just marked it ready for review because I figured you might want to give it a quick lookover to verify that it's the right direction, and to let me know any other API calls that might be needed (I can already see that GetFileMetadata is going to be needed after all, so my deliberately omitting it from the JsInvokable will turn out to be the wrong decision. I'll implement that next). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
157-165: Dispose MediaFilesService DotNetObjectReference to avoid leaks.In Cleanup you dispose
HistoryServiceandSyncService, but notMediaFilesService. This will leak a JS interop handle.Add:
if (SyncService is not null) { SyncService.Dispose(); } + if (MediaFilesService is not null) + { + MediaFilesService.Dispose(); + }Also consider nulling the field after disposal for clarity.
Also applies to: 197-198
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
190-193: Potential ArgumentNullException when server omits Content-Disposition filename.If filename is null, Path.GetFileName(filename) throws. Fall back to remoteId before calling Path.GetFileName.
Apply this diff:
- filename = Path.GetFileName(filename); - var localPath = Path.Combine(projectResourceCachePath, filename ?? remoteId); + // If server didn't specify a filename, fall back to remoteId + var safeFilename = filename is null ? remoteId : Path.GetFileName(filename); + var localPath = Path.Combine(projectResourceCachePath, safeFilename);While here: good job using File.Create below — that aligns with the prior learning to avoid File.OpenWrite/truncation issues.
♻️ Duplicate comments (1)
backend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.cs (1)
11-13: Return a typed model instead of HttpResponseMessage for GetFileMetadata (prevents disposal leaks and simplifies callers)Returning HttpResponseMessage forces call sites to handle status-code checks, JSON deserialization, and disposal. In LcmMediaService.GetFileMetadata (see provided snippet), the HttpResponseMessage isn’t disposed, which can leak connections. Refit can deserialize to LcmFileMetadata and throw ApiException on non-success, eliminating boilerplate and the disposal footgun.
Align with the prior review guidance to use the typed return (this was discussed earlier), and let the request library handle errors. Also keep DownloadFile as HttpResponseMessage since it’s a streaming scenario.
Apply this focused diff here:
- [Get("/api/media/metadata/{fileId}")] - Task<HttpResponseMessage> GetFileMetadata(Guid fileId); + [Get("/api/media/metadata/{fileId}")] + Task<LcmFileMetadata> GetFileMetadata(Guid fileId);Follow-up (outside this file):
- Update LcmMediaService.GetFileMetadata to call mediaClient.GetFileMetadata(fileId) directly and return the result; remove manual IsSuccessStatusCode checks and ReadFromJsonAsync; no disposal needed.
- Ensure the correct using for LcmFileMetadata’s namespace is present in this file.
Optional (nice-to-have):
- Consider adding a CancellationToken parameter to long-running calls in this interface in a future pass for batch operations.
🧹 Nitpick comments (18)
frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/ILocalResource.ts (1)
6-10: Confirm ID semantics and path normalization across platformsPlease verify that:
- ILocalResource.id uses the same format and identity domain as IRemoteResource.id (likely a GUID string) to enable reliable matching and deduping.
- localPath is normalized consistently (e.g., forward slashes vs. backslashes, case sensitivity on Windows vs. POSIX) before comparison or display to avoid false “pending” states.
frontend/viewer/src/lib/services/service-provider.ts (1)
19-20: Wiring looks correct; consider adding a convenience accessor for consistency
- The import and LexboxServiceRegistry mapping for MediaFilesService are correct.
- If you don’t plan to exclusively use the dedicated wrapper in media-files-service.ts, consider adding a convenience accessor here (e.g., useMediaFilesService) for parity with other service helpers.
Example helper (outside the changed lines, only if you want it centralized here):
export function useMediaFilesService(): IMediaFilesServiceJsInvokable { return window.lexbox.ServiceProvider.getService(DotnetService.MediaFilesService); }Also applies to: 32-33
frontend/viewer/src/project/ProjectSidebar.svelte (1)
108-116: Gate Media Files UI on actual service availability (not only features.sync).Currently the dialog and button render only when
features.syncis true. If the media service becomes available in non-sync contexts (e.g., FWData local-only scenarios), the UI will stay hidden despite being usable. Safer gating is to additionally (or instead) checkprojectContext.mediaFilesServiceso the button only appears when the backend service is wired, preventing runtime “service not available” errors if the dialog gets opened elsewhere.If you agree, you can wrap just the media dialog/button with a service-availability check:
{#if features.sync} - <MediaFilesDialog bind:this={mediaFilesDialog} /> + {#if projectContext.mediaFilesService} + <MediaFilesDialog bind:this={mediaFilesDialog} /> + {/if} <SyncDialog bind:this={syncDialog} {syncStatus} /> <Sidebar.MenuItem> - <Sidebar.MenuButton onclick={() => mediaFilesDialog?.open()} class="justify-between"> + {#if projectContext.mediaFilesService} + <Sidebar.MenuButton onclick={() => mediaFilesDialog?.open()} class="justify-between"> <div class="flex items-center gap-2"> <Icon icon="i-mdi-folder"/> <span>{$t`Media Files`}</span> </div> </Sidebar.MenuButton> + {/if}Optional: consider showing a small badge/count for pending transfers when available; align with how sync status uses a dot indicator.
frontend/viewer/src/lib/project-context.svelte.ts (1)
9-11: ProjectContext wiring for MediaFilesService is consistent and symmetric with existing services.Imports, setup, private field, and getter all follow the established pattern for optional services (history/sync). This will make UI gating straightforward.
Optionally add a convenience boolean (e.g.,
get hasMediaFilesService(): boolean) to simplify template conditions in multiple components.Also applies to: 24-24, 50-50, 89-91, 104-105
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
118-122: Avoid duplicate/conflicting export configs for MediaFilesServiceJsInvokable.You export MediaFilesServiceJsInvokable here with
.WithPublicMethods(), and again later via the genericserviceTypesexport with.AlwaysReturnPromise().OnlyJsInvokable(). Double configuration can be confusing and may yield inconsistent return types if ordering changes.Prefer a single source of truth. Two options:
- Option A (recommended): remove the dedicated export here and rely on the
serviceTypesexport below.- Option B: keep the dedicated export but match the semantics (
AlwaysReturnPromise().OnlyJsInvokable()), and exclude MediaFilesService from the later bulk export.Apply Option A (remove the dedicated export):
- builder.ExportAsInterface<MediaFilesServiceJsInvokable>() - // .WithPublicMethods(b => b.AlwaysReturnPromise().OnlyJsInvokable()); - .WithPublicMethods(); - // TODO: Does MediaFilesServiceJsInvokable need the AlwaysReturnPromise().OnlyJsInvokable() setup that MiniLcmJsInvokable needs?If you prefer Option B instead, change to:
- builder.ExportAsInterface<MediaFilesServiceJsInvokable>() - // .WithPublicMethods(b => b.AlwaysReturnPromise().OnlyJsInvokable()); - .WithPublicMethods(); + builder.ExportAsInterface<MediaFilesServiceJsInvokable>() + .WithPublicMethods(b => b.AlwaysReturnPromise().OnlyJsInvokable());…and then filter it out from
serviceTypesbefore bulk export.frontend/viewer/src/project/MediaFilesDialog.svelte (3)
6-28: Clean up unused imports/vars and fix lint errors.ESLint flagged several unused symbols and issues. Remove or use them. Minimal cleanup:
- import { Icon, PingingIcon } from '$lib/components/ui/icon'; - import { t, plural } from 'svelte-i18n-lingui'; + import { Icon } from '$lib/components/ui/icon'; + import { t } from 'svelte-i18n-lingui'; - import { formatDate } from '$lib/components/ui/format'; - import { delay } from '$lib/utils/time'; - import { cn } from '$lib/utils'; - import {useFeatures} from '$lib/services/feature-service'; - import {SyncStatus} from '$lib/dotnet-types/generated-types/LexCore/Sync/SyncStatus'; - import type {IPendingCommits} from '$lib/dotnet-types/generated-types/FwLiteShared/Sync/IPendingCommits'; - import LoginButton from '$lib/auth/LoginButton.svelte'; + // (removed unused imports) - import {ProjectSyncStatusEnum} from '$lib/dotnet-types/generated-types/LexCore/Sync/ProjectSyncStatusEnum'; - import {ProjectSyncStatusErrorCode} from '$lib/dotnet-types/generated-types/LexCore/Sync/ProjectSyncStatusErrorCode'; @@ - const { - syncStatus = SyncStatus.Success - }: {syncStatus?: SyncStatus} = $props(); + // (removed unused prop until needed) @@ - const features = useFeatures(); @@ - let server = $derived(projectContext.server); - let loading = $state(false); + let loading = $state(false); @@ - const serverName = $derived(server?.displayName ?? projectContext.projectData?.serverId ?? 'unknown'); + // (removed unused serverName) @@ - function onLoginStatusChange(status: 'logged-in' | 'logged-out') { - if (status === 'logged-in') { - onOpen(); - } - } + // (removed unused onLoginStatusChange; add later when wiring login events)If you prefer to keep pluralization, switch the labels to use
plural()instead of removing that import. I can provide a localized variant.Also applies to: 30-32, 35-41, 47-47, 108-112
132-142: Optional: pluralize counts for i18n and clarity.If you keep the
pluralimport, improve the labels:- {pendingDownloadCount ?? '?'} files to download + {plural(pendingDownloadCount, { + one: $t`${pendingDownloadCount} file to download`, + other: $t`${pendingDownloadCount} files to download`, + })} @@ - {pendingUploadCount ?? '?'} files to upload + {plural(pendingUploadCount, { + one: $t`${pendingUploadCount} file to upload`, + other: $t`${pendingUploadCount} files to upload`, + })}
58-70: Consider error handling on initial load.
onOpen()swallows any failure in the try/finally. If either list call rejects, the dialog will just clear the spinner. Recommend adding a catch to show a notification, and keep the dialog open:async function onOpen(): Promise<void> { loading = true; - try { + try { let remotePromise = service.resourcesPendingDownload(); let localPromise = service.resourcesPendingUpload(); [localFiles, remoteFiles] = await Promise.all([ localPromise, remotePromise, ]); + } catch (error) { + AppNotification.error($t`Failed to load media status.` + '\n' + (error as Error).message); } finally { loading = false; } }Assumes
AppNotification.errorexists similarly to.promise(). If not, I can wire an alternative.frontend/viewer/src/lib/services/media-files-service.ts (1)
6-12: Thin wrapper is fine; consider a non-throwing accessor for FwData contexts.Current
useMediaFilesService()throws if the service isn’t available. If the dialog or code is included for FwData projects (where remote isn’t wired), a non-throwingmaybeUseMediaFilesService()could improve composability:export function useMediaFilesService() { const projectContext = useProjectContext(); if (!projectContext.mediaFilesService) { throw new Error('MediaFilesService not available in the current project context'); } return new MediaFilesService(projectContext); } + +export function maybeUseMediaFilesService() { + const projectContext = useProjectContext(); + return projectContext.mediaFilesService ? new MediaFilesService(projectContext) : undefined; +}Optional, depending on intended usage scope of the dialog.
If you’d like, I can scan call sites of
useMediaFilesService()to confirm they’re gated by CRDT-only UI.Also applies to: 14-44
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (9)
56-68: Download-on-demand is sane; consider minimal diagnostics.Logic is correct and side-effect free when offline. Consider a debug-level log when you actually attempt a download (to aid support/debugging), and optionally pass a CancellationToken in the future.
70-83: Make batch download resilient to per-item failures (don’t fail the whole loop).A single failing file (e.g., 404, transient network) currently aborts the whole batch. Catch per-item exceptions, log, and continue.
Apply this diff:
public async Task DownloadAllResources() { var connectionStatus = await httpClientProvider.ConnectionStatus(); if (connectionStatus == ConnectionStatus.Online) { var resources = await ResourcesPendingDownload(); foreach (var resource in resources) { if (resource.RemoteId is null) continue; - await resourceService.DownloadResource(resource.Id, this); + try + { + await resourceService.DownloadResource(resource.Id, this); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Skipping failed download for ResourceId={ResourceId} RemoteId={RemoteId}", resource.Id, resource.RemoteId); + } } } // TODO: Gracefully handle other connection statuses, e.g. "not logged in" }
90-97: Mirror batch-download resilience for selected resources.Same rationale as DownloadAllResources: catch/log per-item exceptions so one failure doesn’t cancel the rest.
Apply this diff:
public async Task DownloadResources(IEnumerable<RemoteResource> resources) { foreach (var resource in resources) { - await DownloadResourceIfNeeded(resource.Id); + try + { + await DownloadResourceIfNeeded(resource.Id); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Skipping failed selective download for ResourceId={ResourceId}", resource.Id); + } } }
98-105: Pre-flight file existence and continue-on-error for uploads.When uploading a user-selected subset, local files may have been deleted or moved. Guard and continue.
Apply this diff:
public async Task UploadResources(IEnumerable<LocalResource> resources) { foreach (var resource in resources) { - await ((IRemoteResourceService)this).UploadResource(resource.Id, resource.LocalPath); + try + { + if (!File.Exists(resource.LocalPath)) + { + logger.LogWarning("Skipping upload: missing LocalPath for ResourceId={ResourceId} Path={Path}", resource.Id, resource.LocalPath); + continue; + } + await ((IRemoteResourceService)this).UploadResource(resource.Id, resource.LocalPath); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Skipping failed upload for ResourceId={ResourceId}", resource.Id); + } } }
141-162: Safer error reporting and typed failures for GetFileMetadata.Two tweaks:
- Use HttpRequestException with StatusCode for non-success, which callers can pattern-match.
- Truncate the error-body preview to avoid logging very large responses.
Apply this diff:
- if (!response.IsSuccessStatusCode) - { - throw new Exception($"Failed to retrieve metadata for file {fileId}: {response.StatusCode} {response.ReasonPhrase}"); - } + if (!response.IsSuccessStatusCode) + { + string contentPreview = string.Empty; + try { contentPreview = await response.Content.ReadAsStringAsync(); } catch { /* ignore */ } + if (contentPreview.Length > 2048) contentPreview = contentPreview[..2048] + "…"; + throw new System.Net.Http.HttpRequestException( + $"Failed to retrieve metadata for file {fileId}: {(int)response.StatusCode} {response.ReasonPhrase}. Body: {contentPreview}", + null, + response.StatusCode); + } @@ - if (metadata is null) + if (metadata is null) { // Try to get content into error message, but if buffering not enabled for this request, give up - var content = ""; + var content = ""; try { - content = await response.Content.ReadAsStringAsync(); + content = await response.Content.ReadAsStringAsync(); + if (content.Length > 2048) content = content[..2048] + "…"; } catch { } // Oh well, we tried throw new Exception($"Failed to retrieve metadata for file {fileId}: response was in incorrect format. {content}"); }Note: Nice job using ReadFromJsonAsync here; the new using in Line 10 covers this. Also, per the retrieved learning, elsewhere in this class you’re already using File.Create (avoids the OpenWrite truncation bug) — good.
70-83: Optional: bounded parallelism for large batches.Serial loops are fine to start; if projects often have many media files, consider a small degree of concurrency (e.g., 4) with SemaphoreSlim to improve throughput while avoiding server overload.
I can provide a drop-in helper to run the loops with bounded concurrency if desired.
Also applies to: 90-97, 98-105
114-114: Tiny nit: message spacing in FileNotFoundException."Unable to find the file with Id" + fileId lacks a space before the Guid. Consider interpolated string.
- throw new FileNotFoundException("Unable to find the file with Id" + fileId, localResource.LocalPath); + throw new FileNotFoundException($"Unable to find the file with Id {fileId}", localResource.LocalPath);
164-173: Follow-up needed to support NotFound mapping.To enable the GetFileStream 404 mapping above, adjust RequestMediaFile to throw HttpRequestException with StatusCode on failure (instead of a plain Exception).
Outside-selected-range snippet (for clarity):
private async Task<(Stream? stream, string? filename)> RequestMediaFile(Guid fileId) { var mediaClient = await MediaServerClient(); var response = await mediaClient.DownloadFile(fileId); if (!response.IsSuccessStatusCode) { var preview = ""; try { preview = await response.Content.ReadAsStringAsync(); } catch { } if (preview.Length > 2048) preview = preview[..2048] + "…"; throw new System.Net.Http.HttpRequestException( $"Failed to download file {fileId}: {(int)response.StatusCode} {response.ReasonPhrase}. Body: {preview}", null, response.StatusCode); } return (await response.Content.ReadAsStreamAsync(), response.Content.Headers.ContentDisposition?.FileName?.Replace("\"", "")); }I can wire this and adjust tests if you want it in this PR.
114-139: Optional Refactor: Handle HTTP 404 as ReadFileResult.NotFoundReadFileResult enum already includes a
NotFoundmember, so mapping server “404 Not Found” errors toReadFileResult.NotFoundwill make the API more predictable for callers—allowing UI code to distinguish “file missing” from “offline” and other errors.• File:
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
Lines: 114–139Apply these changes around each
DownloadResourceIfNeeded(fileId)call:- var localResource = await DownloadResourceIfNeeded(fileId); + LocalResource? localResource; + try + { + localResource = await DownloadResourceIfNeeded(fileId); + } + catch (System.Net.Http.HttpRequestException ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) + { + return new ReadFileResponse(ReadFileResult.NotFound, $"File {fileId} not found on server."); + } @@ - // Try again, maybe earlier failure was a blip - localResource = await DownloadResourceIfNeeded(fileId); + // Try again, maybe earlier failure was a blip + try + { + localResource = await DownloadResourceIfNeeded(fileId); + } + catch (System.Net.Http.HttpRequestException ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) + { + return new ReadFileResponse(ReadFileResult.NotFound, $"File {fileId} not found on server."); + } @@ - // One more attempt to download again, maybe the cache was cleared - localResource = await DownloadResourceIfNeeded(fileId); + // One more attempt to download again, maybe the cache was cleared + try + { + localResource = await DownloadResourceIfNeeded(fileId); + } + catch (System.Net.Http.HttpRequestException ex) when (ex.StatusCode == System.Net.HttpStatusCode.NotFound) + { + return new ReadFileResponse(ReadFileResult.NotFound, $"File {fileId} not found on server."); + }You’ll also need to ensure that your HTTP layer (
RequestMediaFile()or similar) throwsHttpRequestExceptionwithStatusCodeset toHttpStatusCode.NotFoundwhen the server returns 404.This refactor is optional but recommended to improve API consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs(2 hunks)backend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.cs(1 hunks)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs(3 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(3 hunks)backend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.cs(1 hunks)backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs(4 hunks)frontend/viewer/src/DotnetProjectView.svelte(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectScope.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/ILocalResource.ts(1 hunks)frontend/viewer/src/lib/project-context.svelte.ts(5 hunks)frontend/viewer/src/lib/services/media-files-service.ts(1 hunks)frontend/viewer/src/lib/services/service-provider.ts(2 hunks)frontend/viewer/src/project/MediaFilesDialog.svelte(1 hunks)frontend/viewer/src/project/ProjectSidebar.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-14T12:50:25.135Z
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1906
File: frontend/viewer/src/lib/components/ui/dialog-shared/dialog-shared-root.svelte:3-3
Timestamp: 2025-08-14T12:50:25.135Z
Learning: In the dialog-shared-root.svelte component, the module-level `openDialogs` state is intentionally shared across all component instances to coordinate dialog stacking and overlay behavior across the entire application. This enables proper z-index management where newer dialogs appear on top and only the bottom dialog shows its overlay.
Applied to files:
frontend/viewer/src/project/ProjectSidebar.sveltefrontend/viewer/src/project/MediaFilesDialog.svelte
📚 Learning: 2025-07-22T09:19:37.386Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Applied to files:
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs
📚 Learning: 2025-07-22T09:19:27.281Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.281Z
Learning: In the LcmMediaService class (backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs), File.OpenWrite should not be used for saving files because it doesn't truncate existing files, which causes data corruption when re-uploading files smaller than the original. Use File.Create instead to ensure proper file truncation.
Applied to files:
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
🧬 Code graph analysis (10)
frontend/viewer/src/lib/services/media-files-service.ts (4)
frontend/viewer/src/lib/project-context.svelte.ts (2)
useProjectContext(37-39)ProjectContext(40-141)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts (1)
IMediaFilesServiceJsInvokable(9-17)frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/IRemoteResource.ts (1)
IRemoteResource(6-11)frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/ILocalResource.ts (1)
ILocalResource(6-10)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts (2)
frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/IRemoteResource.ts (1)
IRemoteResource(6-11)frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/ILocalResource.ts (1)
ILocalResource(6-10)
backend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.cs (1)
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (16)
Task(23-26)Task(28-31)Task(33-36)Task(43-49)Task(51-54)Task(56-68)Task(70-83)Task(85-88)Task(90-96)Task(98-104)Task(112-139)Task(141-162)Task(164-173)Task(175-180)Task(182-197)Task(203-213)
backend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.cs (3)
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
LcmMediaService(14-267)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (4)
JSInvokable(36-40)JSInvokable(42-47)JSInvokable(49-85)JSInvokable(87-117)backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (1)
Task(67-82)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
backend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.cs (1)
MediaFilesServiceJsInvokable(7-44)
frontend/viewer/src/lib/project-context.svelte.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts (1)
IMediaFilesServiceJsInvokable(9-17)
frontend/viewer/src/lib/services/service-provider.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts (1)
IMediaFilesServiceJsInvokable(9-17)
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (2)
frontend/viewer/src/lib/services/media-files-service.ts (1)
MediaFilesService(14-44)backend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.cs (1)
MediaFilesServiceJsInvokable(7-44)
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
backend/FwLite/MiniLcm/Media/ReadFileResponse.cs (2)
ReadFileResponse(7-12)ReadFileResponse(14-19)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (4)
backend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.cs (1)
MediaFilesServiceJsInvokable(7-44)frontend/viewer/src/lib/project-context.svelte.ts (3)
syncService(86-88)mediaFilesService(89-91)historyService(83-85)backend/FwLite/FwLiteShared/Sync/SyncService.cs (1)
SyncService(23-240)frontend/viewer/src/lib/services/media-files-service.ts (1)
MediaFilesService(14-44)
🪛 GitHub Check: ESLint Report Analysis
frontend/viewer/src/project/MediaFilesDialog.svelte
[failure] 7-7: frontend/viewer/src/project/MediaFilesDialog.svelte#L7
[@typescript-eslint/no-unused-vars] 'PingingIcon' is defined but never used.
[failure] 9-9: frontend/viewer/src/project/MediaFilesDialog.svelte#L9
[@typescript-eslint/no-unused-vars] 'plural' is defined but never used.
[failure] 13-13: frontend/viewer/src/project/MediaFilesDialog.svelte#L13
[@typescript-eslint/no-unused-vars] 'formatDate' is defined but never used.
[failure] 16-16: frontend/viewer/src/project/MediaFilesDialog.svelte#L16
[@typescript-eslint/no-unused-vars] 'delay' is defined but never used.
[failure] 17-17: frontend/viewer/src/project/MediaFilesDialog.svelte#L17
[@typescript-eslint/no-unused-vars] 'cn' is defined but never used.
[failure] 20-20: frontend/viewer/src/project/MediaFilesDialog.svelte#L20
[@typescript-eslint/no-unused-vars] 'IPendingCommits' is defined but never used.
[failure] 21-21: frontend/viewer/src/project/MediaFilesDialog.svelte#L21
[@typescript-eslint/no-unused-vars] 'LoginButton' is defined but never used.
[failure] 23-23: frontend/viewer/src/project/MediaFilesDialog.svelte#L23
[@typescript-eslint/no-unused-vars] 'ProjectSyncStatusEnum' is defined but never used.
[failure] 24-24: frontend/viewer/src/project/MediaFilesDialog.svelte#L24
[@typescript-eslint/no-unused-vars] 'ProjectSyncStatusErrorCode' is defined but never used.
[failure] 30-30: frontend/viewer/src/project/MediaFilesDialog.svelte#L30
[@typescript-eslint/no-unused-vars] 'syncStatus' is assigned a value but never used.
[failure] 35-35: frontend/viewer/src/project/MediaFilesDialog.svelte#L35
[@typescript-eslint/no-unused-vars] 'features' is assigned a value but never used.
[failure] 41-41: frontend/viewer/src/project/MediaFilesDialog.svelte#L41
[@typescript-eslint/no-unused-vars] 'loading' is assigned a value but never used.
[failure] 47-47: frontend/viewer/src/project/MediaFilesDialog.svelte#L47
[@typescript-eslint/no-unused-vars] 'serverName' is assigned a value but never used.
[failure] 78-78: frontend/viewer/src/project/MediaFilesDialog.svelte#L78
[@typescript-eslint/require-await] Async function 'downloadAll' has no 'await' expression.
[failure] 93-93: frontend/viewer/src/project/MediaFilesDialog.svelte#L93
[@typescript-eslint/require-await] Async function 'uploadAll' has no 'await' expression.
[failure] 108-108: frontend/viewer/src/project/MediaFilesDialog.svelte#L108
[@typescript-eslint/no-unused-vars] 'onLoginStatusChange' is defined but never used.
[failure] 110-110: frontend/viewer/src/project/MediaFilesDialog.svelte#L110
[@typescript-eslint/no-floating-promises] Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build API / publish-api
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (16)
frontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/ILocalResource.ts (1)
6-10: LGTM: Generated interop contract looks correctInterface shape matches intended usage (upload APIs expect ILocalResource[]). No issues from a typing standpoint.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts (1)
15-16: LGTM: Added MediaFilesService enum memberThe new enum value slots cleanly into the existing service list and will flow through SERVICE_KEYS consumers.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectScope.ts (1)
19-20: LGTM: Project scope now exposes mediaFilesServiceThe optional DotNet object aligns with how other services are surfaced. Ensure consumers null-check this property in projects without media support.
frontend/viewer/src/project/ProjectSidebar.svelte (1)
19-19: LGTM: Import and instance binding for MediaFilesDialog look correct.Import path matches colocated component;
bind:this={mediaFilesDialog}mirrors existing dialog patterns. Based on our prior learning about the shared dialog root and stacking behavior, this should integrate cleanly with global dialog stacking.Also applies to: 56-56
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (1)
49-50: LGTM: MediaFilesService enum and type mapping added consistently.
DotnetService.MediaFilesService => typeof(MediaFilesServiceJsInvokable)and the enum entry are aligned with the new interop surface. No issues spotted.Confirm intention: MediaFilesService is project-scoped (via ProjectServicesProvider) and not exported in
ExportedServices(global). If that’s correct, no change needed toExportedServices. If you intended a global service, add it there as well.Also applies to: 107-108
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (3)
24-24: LGTM: Added SIL.Harmony.Resource import.Required for exporting LocalResource/RemoteResource types.
88-91: LGTM: Exposing LcmFileMetadata, RemoteResource, and LocalResource to TS.These are the right public types for the media transfer APIs; property-export filters guard against computed-only getters.
153-159: Sanity check: Bulk export includes MediaFilesServiceJsInvokable with Promise-returning, JS-invokable-only methods.This ensures consistent TS signatures (Promises) and limits the surface to
[JSInvokable]members. Good guardrails for interop.After TypeGen, confirm the generated
IMediaFilesServiceJsInvokablemethods all returnPromise<...>and no non-invokable members leaked.frontend/viewer/src/DotnetProjectView.svelte (1)
16-16: LGTM: Properly proxied MediaFilesService and passed into ProjectContext.
wrapInProxy(projectScope.mediaFilesService, DotnetService.MediaFilesService)mirrors history/sync wiring and keeps the TS surface consistent with other services.Also applies to: 56-60, 65-66
frontend/viewer/src/project/MediaFilesDialog.svelte (1)
49-52: LGTM: open/close lifecycle and delayed clear.Watching the query param and delaying clear for the close animation is a good touch.
Also applies to: 72-75
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.ts (1)
9-17: Interface surface looks consistent with the invokable; generated status noted.Methods align with the backend wrapper. No issues on types for Local/Remote resources.
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
70-75: LGTM: wiring new MediaFilesService into CRDT scope.Service creation via ActivatorUtilities and storage in
ProjectScopeare consistent with the existing pattern.Also applies to: 175-183, 195-198
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (4)
10-10: Good add: enables Content.ReadFromJsonAsync.The import is necessary for ReadFromJsonAsync used below. All good.
28-31: Thin delegator reads well.ResourcesPendingDownload is a clear pass-through to the resource service. No concerns.
33-36: Symmetric delegator looks good.ResourcesPendingUpload mirrors the download counterpart cleanly.
85-88: Upload-all wrapper is clear.Delegates to UploadResources, which keeps a single code path. Good.
| public class MediaFilesServiceJsInvokable(LcmMediaService mediaService) | ||
| { | ||
| [JSInvokable] | ||
| public async Task<RemoteResource[]> ResourcesPendingDownload() | ||
| { | ||
| return await mediaService.ResourcesPendingDownload(); | ||
| } | ||
|
|
||
| [JSInvokable] | ||
| public async Task<LocalResource[]> ResourcesPendingUpload() | ||
| { | ||
| return await mediaService.ResourcesPendingUpload(); | ||
| } | ||
|
|
||
| [JSInvokable] | ||
| public async Task DownloadAllResources() | ||
| { | ||
| await mediaService.DownloadAllResources(); | ||
| } | ||
|
|
||
| [JSInvokable] | ||
| public async Task UploadAllResources() | ||
| { | ||
| await mediaService.UploadAllResources(); | ||
| } | ||
|
|
||
| [JSInvokable] | ||
| public async Task DownloadResources(IEnumerable<RemoteResource> resources) | ||
| { | ||
| await mediaService.DownloadResources(resources); | ||
| } | ||
|
|
||
| [JSInvokable] | ||
| public async Task UploadResources(IEnumerable<LocalResource> resources) | ||
| { | ||
| await mediaService.UploadResources(resources); | ||
| } | ||
| } |
There was a problem hiding this comment.
Expose metadata APIs through JS interop to meet PR objectives.
Per PR objectives, the backend should provide an API to fetch file metadata. LcmMediaService has GetFileMetadata(Guid fileId) and AllResources() but the invokable does not expose them, so the frontend can’t call them.
Add wrappers:
public class MediaFilesServiceJsInvokable(LcmMediaService mediaService)
{
+ [JSInvokable]
+ public async Task<HarmonyResource[]> AllResources()
+ {
+ return await mediaService.AllResources();
+ }
+
[JSInvokable]
public async Task<RemoteResource[]> ResourcesPendingDownload()
{
return await mediaService.ResourcesPendingDownload();
}
@@
public async Task UploadResources(IEnumerable<LocalResource> resources)
{
await mediaService.UploadResources(resources);
}
+
+ [JSInvokable]
+ public async Task<LcmFileMetadata> GetFileMetadata(Guid fileId)
+ {
+ return await mediaService.GetFileMetadata(fileId);
+ }
}You’ll also need to export HarmonyResource and LcmFileMetadata via TypeGen so the TS interface gains these signatures, and update the generated IMediaFilesServiceJsInvokable plus the frontend wrapper accordingly. I can draft the TypeGen config changes if helpful.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class MediaFilesServiceJsInvokable(LcmMediaService mediaService) | |
| { | |
| [JSInvokable] | |
| public async Task<RemoteResource[]> ResourcesPendingDownload() | |
| { | |
| return await mediaService.ResourcesPendingDownload(); | |
| } | |
| [JSInvokable] | |
| public async Task<LocalResource[]> ResourcesPendingUpload() | |
| { | |
| return await mediaService.ResourcesPendingUpload(); | |
| } | |
| [JSInvokable] | |
| public async Task DownloadAllResources() | |
| { | |
| await mediaService.DownloadAllResources(); | |
| } | |
| [JSInvokable] | |
| public async Task UploadAllResources() | |
| { | |
| await mediaService.UploadAllResources(); | |
| } | |
| [JSInvokable] | |
| public async Task DownloadResources(IEnumerable<RemoteResource> resources) | |
| { | |
| await mediaService.DownloadResources(resources); | |
| } | |
| [JSInvokable] | |
| public async Task UploadResources(IEnumerable<LocalResource> resources) | |
| { | |
| await mediaService.UploadResources(resources); | |
| } | |
| } | |
| public class MediaFilesServiceJsInvokable(LcmMediaService mediaService) | |
| { | |
| [JSInvokable] | |
| public async Task<HarmonyResource[]> AllResources() | |
| { | |
| return await mediaService.AllResources(); | |
| } | |
| [JSInvokable] | |
| public async Task<RemoteResource[]> ResourcesPendingDownload() | |
| { | |
| return await mediaService.ResourcesPendingDownload(); | |
| } | |
| [JSInvokable] | |
| public async Task<LocalResource[]> ResourcesPendingUpload() | |
| { | |
| return await mediaService.ResourcesPendingUpload(); | |
| } | |
| [JSInvokable] | |
| public async Task DownloadAllResources() | |
| { | |
| await mediaService.DownloadAllResources(); | |
| } | |
| [JSInvokable] | |
| public async Task UploadAllResources() | |
| { | |
| await mediaService.UploadAllResources(); | |
| } | |
| [JSInvokable] | |
| public async Task DownloadResources(IEnumerable<RemoteResource> resources) | |
| { | |
| await mediaService.DownloadResources(resources); | |
| } | |
| [JSInvokable] | |
| public async Task UploadResources(IEnumerable<LocalResource> resources) | |
| { | |
| await mediaService.UploadResources(resources); | |
| } | |
| [JSInvokable] | |
| public async Task<LcmFileMetadata> GetFileMetadata(Guid fileId) | |
| { | |
| return await mediaService.GetFileMetadata(fileId); | |
| } | |
| } |
| let remoteFiles = $state<IRemoteResource[]>([]); | ||
| let localFiles = $state<ILocalResource[]>([]); | ||
| let pendingUploadCount = $derived(localFiles?.length ?? 0); | ||
| let pendingDownloadCount = $derived(remoteFiles?.length ?? 0); | ||
| let server = $derived(projectContext.server); | ||
| let loading = $state(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t mutate $derived; make counters const and drive them from the lists.
pendingUploadCount/pendingDownloadCount should be read-only derived signals. Remove assignments and declare as const:
- let remoteFiles = $state<IRemoteResource[]>([]);
- let localFiles = $state<ILocalResource[]>([]);
- let pendingUploadCount = $derived(localFiles?.length ?? 0);
- let pendingDownloadCount = $derived(remoteFiles?.length ?? 0);
- let server = $derived(projectContext.server);
+ let remoteFiles = $state<IRemoteResource[]>([]);
+ let localFiles = $state<ILocalResource[]>([]);
+ const pendingUploadCount = $derived(localFiles.length);
+ const pendingDownloadCount = $derived(remoteFiles.length);And remove any later writes to those counters (handled by the refactors above).
Also applies to: 88-89, 103-104
🧰 Tools
🪛 GitHub Check: ESLint Report Analysis
[failure] 41-41: frontend/viewer/src/project/MediaFilesDialog.svelte#L41
[@typescript-eslint/no-unused-vars] 'loading' is assigned a value but never used.
🤖 Prompt for AI Agents
In frontend/viewer/src/project/MediaFilesDialog.svelte around lines 36 to 41
(and also check lines 88-89 and 103-104), pendingUploadCount and
pendingDownloadCount are being created as mutable $derived signals and later
assigned to; change them to read-only const derived values driven from
localFiles.length and remoteFiles.length (e.g., const pendingUploadCount =
derived(() => localFiles?.length ?? 0) and const pendingDownloadCount =
derived(() => remoteFiles?.length ?? 0)), remove any subsequent
assignments/writes to these counters elsewhere in the file (including the
referenced lines 88-89 and 103-104) so counts are computed reactively from the
lists instead of being mutated.
| let loadingDownload = $state(false); | ||
| async function downloadAll() { | ||
| try { | ||
| const downloadPromise = service.downloadAllResources(); | ||
| const count = pendingDownloadCount; // Break reactivity before we set pending count to 0 | ||
| AppNotification.promise(downloadPromise, { | ||
| loading: $t`Downloading files from remote...`, | ||
| success: $t`${count} files downloaded.`, | ||
| error: (error) => $t`Failed to download files.` + '\n' + (error as Error).message, | ||
| }); | ||
| } finally { | ||
| pendingDownloadCount = 0; | ||
| loadingDownload = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix race/UX bugs in downloadAll(): await the promise, set loading, and refresh counts instead of mutating $derived.
- The function is marked async but has no await; finally runs immediately, zeroing counts before work finishes.
- You’re assigning to a $derived signal (
pendingDownloadCount = 0), which is read-only by design. - No loading flag is set to true, so the spinner never shows.
Apply this refactor to make the flow deterministic and reactive:
- let loadingDownload = $state(false);
- async function downloadAll() {
- try {
- const downloadPromise = service.downloadAllResources();
- const count = pendingDownloadCount; // Break reactivity before we set pending count to 0
- AppNotification.promise(downloadPromise, {
- loading: $t`Downloading files from remote...`,
- success: $t`${count} files downloaded.`,
- error: (error) => $t`Failed to download files.` + '\n' + (error as Error).message,
- });
- } finally {
- pendingDownloadCount = 0;
- loadingDownload = false;
- }
- }
+ let loadingDownload = $state(false);
+ async function downloadAll() {
+ loadingDownload = true;
+ try {
+ const count = pendingDownloadCount; // snapshot before list refresh
+ const downloadPromise = service.downloadAllResources();
+ AppNotification.promise(downloadPromise, {
+ loading: $t`Downloading files from remote...`,
+ success: $t`${count} files downloaded.`,
+ error: (error) => $t`Failed to download files.` + '\n' + (error as Error).message,
+ });
+ await downloadPromise;
+ // Refresh lists instead of mutating derived counters
+ remoteFiles = await service.resourcesPendingDownload();
+ } finally {
+ loadingDownload = false;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let loadingDownload = $state(false); | |
| async function downloadAll() { | |
| try { | |
| const downloadPromise = service.downloadAllResources(); | |
| const count = pendingDownloadCount; // Break reactivity before we set pending count to 0 | |
| AppNotification.promise(downloadPromise, { | |
| loading: $t`Downloading files from remote...`, | |
| success: $t`${count} files downloaded.`, | |
| error: (error) => $t`Failed to download files.` + '\n' + (error as Error).message, | |
| }); | |
| } finally { | |
| pendingDownloadCount = 0; | |
| loadingDownload = false; | |
| } | |
| } | |
| let loadingDownload = $state(false); | |
| async function downloadAll() { | |
| loadingDownload = true; | |
| try { | |
| const count = pendingDownloadCount; // snapshot before list refresh | |
| const downloadPromise = service.downloadAllResources(); | |
| AppNotification.promise(downloadPromise, { | |
| loading: $t`Downloading files from remote...`, | |
| success: $t`${count} files downloaded.`, | |
| error: (error) => $t`Failed to download files.` + '\n' + (error as Error).message, | |
| }); | |
| await downloadPromise; | |
| // Refresh lists instead of mutating derived counters | |
| remoteFiles = await service.resourcesPendingDownload(); | |
| } finally { | |
| loadingDownload = false; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: ESLint Report Analysis
[failure] 78-78: frontend/viewer/src/project/MediaFilesDialog.svelte#L78
[@typescript-eslint/require-await] Async function 'downloadAll' has no 'await' expression.
| let loadingUpload = $state(false); | ||
| async function uploadAll() { | ||
| try { | ||
| const uploadPromise = service.uploadAllResources(); | ||
| const count = pendingUploadCount; // Break reactivity before we set pending count to 0 | ||
| AppNotification.promise(uploadPromise, { | ||
| loading: $t`Uploading files to remote...`, | ||
| success: $t`${count} files uploaded.`, | ||
| error: (error) => $t`Failed to upload files.` + '\n' + (error as Error).message, | ||
| }); | ||
| } finally { | ||
| pendingUploadCount = 0; | ||
| loadingUpload = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the same await/refresh logic for uploadAll().
Same issues as downloadAll(): no await, premature zeroing of a $derived value, spinner never shows. Refactor:
- let loadingUpload = $state(false);
- async function uploadAll() {
- try {
- const uploadPromise = service.uploadAllResources();
- const count = pendingUploadCount; // Break reactivity before we set pending count to 0
- AppNotification.promise(uploadPromise, {
- loading: $t`Uploading files to remote...`,
- success: $t`${count} files uploaded.`,
- error: (error) => $t`Failed to upload files.` + '\n' + (error as Error).message,
- });
- } finally {
- pendingUploadCount = 0;
- loadingUpload = false;
- }
- }
+ let loadingUpload = $state(false);
+ async function uploadAll() {
+ loadingUpload = true;
+ try {
+ const count = pendingUploadCount;
+ const uploadPromise = service.uploadAllResources();
+ AppNotification.promise(uploadPromise, {
+ loading: $t`Uploading files to remote...`,
+ success: $t`${count} files uploaded.`,
+ error: (error) => $t`Failed to upload files.` + '\n' + (error as Error).message,
+ });
+ await uploadPromise;
+ localFiles = await service.resourcesPendingUpload();
+ } finally {
+ loadingUpload = false;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let loadingUpload = $state(false); | |
| async function uploadAll() { | |
| try { | |
| const uploadPromise = service.uploadAllResources(); | |
| const count = pendingUploadCount; // Break reactivity before we set pending count to 0 | |
| AppNotification.promise(uploadPromise, { | |
| loading: $t`Uploading files to remote...`, | |
| success: $t`${count} files uploaded.`, | |
| error: (error) => $t`Failed to upload files.` + '\n' + (error as Error).message, | |
| }); | |
| } finally { | |
| pendingUploadCount = 0; | |
| loadingUpload = false; | |
| } | |
| } | |
| let loadingUpload = $state(false); | |
| async function uploadAll() { | |
| loadingUpload = true; | |
| try { | |
| const count = pendingUploadCount; | |
| const uploadPromise = service.uploadAllResources(); | |
| AppNotification.promise(uploadPromise, { | |
| loading: $t`Uploading files to remote...`, | |
| success: $t`${count} files uploaded.`, | |
| error: (error) => $t`Failed to upload files.` + '\n' + (error as Error).message, | |
| }); | |
| await uploadPromise; | |
| localFiles = await service.resourcesPendingUpload(); | |
| } finally { | |
| loadingUpload = false; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: ESLint Report Analysis
[failure] 93-93: frontend/viewer/src/project/MediaFilesDialog.svelte#L93
[@typescript-eslint/require-await] Async function 'uploadAll' has no 'await' expression.
🤖 Prompt for AI Agents
In frontend/viewer/src/project/MediaFilesDialog.svelte around lines 92 to 106,
uploadAll currently starts the upload but doesn’t await it, causing the finally
block to run immediately (clearing pendingUploadCount and hiding the spinner) —
mirror the downloadAll fix: set loadingUpload = true before starting, capture
pendingUploadCount into a local count, then await the notification wrapper
(await AppNotification.promise(uploadPromise, {...})) so the function pauses
while upload runs; only after the await (or in finally after the awaited call)
set pendingUploadCount = 0 and loadingUpload = false to preserve reactivity and
show the spinner for the duration of the upload.
| <Dialog bind:open={openQueryParam.current}> | ||
| <DialogContent class="sm:min-h-80 sm:min-w-96 grid-rows-[auto_1fr] items-center"> | ||
| <DialogHeader> | ||
| <DialogTitle>{$t`Download Files`}</DialogTitle> | ||
| </DialogHeader> | ||
| {#if loadingDownload || loadingUpload} | ||
| <Loading class="place-self-center size-10" /> | ||
| {:else} | ||
| <div in:fade | ||
| class="grid grid-rows-[auto] grid-cols-[1fr_7fr_1fr] gap-y-6 gap-x-8" | ||
| > | ||
| <!-- TODO: Make icon(s) pulse while downloading, perhaps show progress in notification... --> | ||
| <!-- TODO: Detect not-logged-in status and provide login button similar to sync dialog --> | ||
| <div class="col-span-1 text-center"> | ||
| <Icon icon="i-mdi-folder" class="size-10" /> | ||
| </div> | ||
| <div class="text-center content-center"> | ||
| {pendingDownloadCount ?? '?'} files to download | ||
| </div> | ||
| <div class="content-center text-center"> | ||
| <Button onclick={downloadAll}>DL</Button> | ||
| </div> | ||
| <div class="col-span-1 text-center"> | ||
| <Icon icon="i-mdi-folder" class="size-10" /> | ||
| </div> | ||
| <div class="text-center content-center"> | ||
| {pendingUploadCount ?? '?'} files to upload | ||
| </div> | ||
| <div class="content-center text-center"> | ||
| <Button onclick={uploadAll}>UL</Button> | ||
| </div> | ||
| </div> | ||
| {/if} | ||
| </DialogContent> | ||
| </Dialog> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix Svelte event bindings and show spinner during initial load.
- Use
on:clickfor Svelte component events instead ofonclick. - Include
loadingin the condition so the spinner shows whileonOpen()is running. - Disable buttons while working.
- {#if loadingDownload || loadingUpload}
+ {#if loading || loadingDownload || loadingUpload}
<Loading class="place-self-center size-10" />
{:else}
@@
- <div class="content-center text-center">
- <Button onclick={downloadAll}>DL</Button>
+ <div class="content-center text-center">
+ <Button on:click={downloadAll} disabled={loading || loadingDownload}>DL</Button>
</div>
@@
- <div class="content-center text-center">
- <Button onclick={uploadAll}>UL</Button>
+ <div class="content-center text-center">
+ <Button on:click={uploadAll} disabled={loading || loadingUpload}>UL</Button>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Dialog bind:open={openQueryParam.current}> | |
| <DialogContent class="sm:min-h-80 sm:min-w-96 grid-rows-[auto_1fr] items-center"> | |
| <DialogHeader> | |
| <DialogTitle>{$t`Download Files`}</DialogTitle> | |
| </DialogHeader> | |
| {#if loadingDownload || loadingUpload} | |
| <Loading class="place-self-center size-10" /> | |
| {:else} | |
| <div in:fade | |
| class="grid grid-rows-[auto] grid-cols-[1fr_7fr_1fr] gap-y-6 gap-x-8" | |
| > | |
| <!-- TODO: Make icon(s) pulse while downloading, perhaps show progress in notification... --> | |
| <!-- TODO: Detect not-logged-in status and provide login button similar to sync dialog --> | |
| <div class="col-span-1 text-center"> | |
| <Icon icon="i-mdi-folder" class="size-10" /> | |
| </div> | |
| <div class="text-center content-center"> | |
| {pendingDownloadCount ?? '?'} files to download | |
| </div> | |
| <div class="content-center text-center"> | |
| <Button onclick={downloadAll}>DL</Button> | |
| </div> | |
| <div class="col-span-1 text-center"> | |
| <Icon icon="i-mdi-folder" class="size-10" /> | |
| </div> | |
| <div class="text-center content-center"> | |
| {pendingUploadCount ?? '?'} files to upload | |
| </div> | |
| <div class="content-center text-center"> | |
| <Button onclick={uploadAll}>UL</Button> | |
| </div> | |
| </div> | |
| {/if} | |
| </DialogContent> | |
| </Dialog> | |
| <Dialog bind:open={openQueryParam.current}> | |
| <DialogContent class="sm:min-h-80 sm:min-w-96 grid-rows-[auto_1fr] items-center"> | |
| <DialogHeader> | |
| <DialogTitle>{$t`Download Files`}</DialogTitle> | |
| </DialogHeader> | |
| {#if loading || loadingDownload || loadingUpload} | |
| <Loading class="place-self-center size-10" /> | |
| {:else} | |
| <div in:fade | |
| class="grid grid-rows-[auto] grid-cols-[1fr_7fr_1fr] gap-y-6 gap-x-8" | |
| > | |
| <!-- TODO: Make icon(s) pulse while downloading, perhaps show progress in notification... --> | |
| <!-- TODO: Detect not-logged-in status and provide login button similar to sync dialog --> | |
| <div class="col-span-1 text-center"> | |
| <Icon icon="i-mdi-folder" class="size-10" /> | |
| </div> | |
| <div class="text-center content-center"> | |
| {pendingDownloadCount ?? '?'} files to download | |
| </div> | |
| <div class="content-center text-center"> | |
| <Button on:click={downloadAll} disabled={loading || loadingDownload}>DL</Button> | |
| </div> | |
| <div class="col-span-1 text-center"> | |
| <Icon icon="i-mdi-folder" class="size-10" /> | |
| </div> | |
| <div class="text-center content-center"> | |
| {pendingUploadCount ?? '?'} files to upload | |
| </div> | |
| <div class="content-center text-center"> | |
| <Button on:click={uploadAll} disabled={loading || loadingUpload}>UL</Button> | |
| </div> | |
| </div> | |
| {/if} | |
| </DialogContent> | |
| </Dialog> |
🤖 Prompt for AI Agents
In frontend/viewer/src/project/MediaFilesDialog.svelte around lines 115 to 149,
the event bindings and loading behavior are incorrect: replace the DOM-style
onclick attributes with Svelte event bindings (use on:click for the downloadAll
and uploadAll Button components), update the conditional that shows the spinner
to include the top-level loading flag (use loading || loadingDownload ||
loadingUpload) so the spinner appears while onOpen() runs, and disable the DL/UL
buttons while any loading is active (set the Button disabled when loading ||
loadingDownload || loadingUpload) to prevent duplicate actions.
This allows us to display a list of filenames, and possibly author and/or upload dates, for the user to be able to select which file(s) to download or upload.
This allows us to provide checkboxes, which have to have string (rather than object) values in Svelte, to select files to download.
Also would allow selecting files to upload, but there's no way in the UI to add a file to the project without it being automatically uploaded, so this is harder to test.
| @@ -112,6 +116,10 @@ private static void ConfigureMiniLcmTypes(ConfigurationBuilder builder) | |||
| .FlattenHierarchy() | |||
| .WithPublicProperties() | |||
| .WithPublicMethods(b => b.AlwaysReturnPromise().OnlyJsInvokable()); | |||
| builder.ExportAsInterface<MediaFilesServiceJsInvokable>() | |||
| // .WithPublicMethods(b => b.AlwaysReturnPromise().OnlyJsInvokable()); | |||
| .WithPublicMethods(); | |||
There was a problem hiding this comment.
ues we should always use OnlyJsInvokable for services which are executed over js like this, and in this case AlwaysReturnPromise is a good idea too.
| @@ -50,25 +129,44 @@ public async Task DeleteResource(Guid fileId) | |||
| /// <exception cref="FileNotFoundException"></exception> | |||
| public async Task<ReadFileResponse> GetFileStream(Guid fileId) | |||
| { | |||
| var localResource = await resourceService.GetLocalResource(fileId); | |||
| var localResource = await DownloadResourceIfNeeded(fileId); | |||
There was a problem hiding this comment.
I don't like that we're not calling DownloadResourceIfNeeded 3 different times, if we want to implement retrying then that should go somewhere else, not in this method as it makes it much more messy and hard to follow.
| <SyncDialog bind:this={syncDialog} {syncStatus} /> | ||
| <Sidebar.MenuItem> | ||
| <Sidebar.MenuButton onclick={() => mediaFilesDialog?.open()} class="justify-between"> |
There was a problem hiding this comment.
this should be dev only since it'll be merged in before it's finished
There was a problem hiding this comment.
My intent was that this dialog would be deleted before merging, as I've given no consideration to UI as I wrote it, only to testing out the API calls. That's also why I wasn't planning on solving any of the lint errors, since the idea was to delete the dialog once the API was reviewed and approved, and the lint errors would be solved by deleting the file.
WIP, ready for review. Will fix #1835 when done.
Now have a MediaFilesService that is JS invokable, and can get a list of pending resources (local or server), download/upload all pending resources from/to the server, and also download/upload a selected list of pending resources (so that we can eventually give the user UI to select only a subset of files to download).
Also have a very barebones test UI that I have made no effort to pretty up, but which allows downloading all remote files that aren't local, or uploading all local files that aren't remote.
Screenshot of test UI:
To test: