Media manager#2358
Open
hahn-kev wants to merge 35 commits into
Open
Conversation
- Introduced MediaFileDetail component for displaying file details and handling file downloads. - Implemented GetFileStream method in MediaFilesServiceJsInvokable to retrieve file streams. - Updated ILcmFileMetadata to include SizeInBytes property. - Enhanced IMediaFilesServiceJsInvokable interface with getFileStream method.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds file size and metadata (author, upload date, MIME type) to media files end-to-end: backend storage/sync/upload flows, a typed ChangesMedia File Metadata Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
the
💻 FW Lite
Contributor
Contributor
C# FwHeadless Unit Tests48 tests 48 ✅ 18s ⏱️ Results for commit e19ea6f. ♻️ This comment has been updated with latest results. |
Contributor
C# Unit Tests165 tests 165 ✅ 20s ⏱️ Results for commit e19ea6f. ♻️ This comment has been updated with latest results. |
Collaborator
|
You're fast ;). It's a little funny how much the right panel looks like an email 😄 I'm wondering how users will find things? Or maybe they don't actually want to? What are the real use cases for this?
If that's all, then, this is probably fine. Maybe we just need a filter for "Unused media" i.e. orphaned stuff. Nit: can we make the resource "names" align vertically e.g. by either stacking the cloud and local icons vertically instead of horizontally or putting them in the second row? Maybe like the part of speech badge 🤔 or maybe put the icons into the "Local and remote" text? I'm guessing the "Download all" is actually for making stuff available offline, rather than saving a copy of everything somewhere on your device right? "Downloading" sounds like "Saving" to me. "Make all media available offline" is a bit long for a button text 😆. Maybe we just change it to "Download all..." (i.e. with 3-dots), so users know it opens a dialog and the dialog then has an explanation and submit button? The GDocs UI for making things available offline is a bit weird. I first clicked a button in this overlay to make my doc "ready for offline use": After that I was able to open, edit and comment on the document while offline. When I click it, it changes to: I see no differences in behaviour after enabling that second option. |
Collaborator
Author
|
Another use case I could see is removing files locally (to free up space), not sure how realistic that is though. On android there's a distinction between cache and data, we should probably figure that out to download stuff into cache. Yes you noticed another terminology issue we stumbled in to. I think download in this context could mean either, export so I can use it for whatever, or download to my device from the server. So we should probably just not use the word download. What if we just used the term "Load" and "Load all" for the current "Download all" button which loads for offline use. We currently use "Save As" for getting files out of the app, I think that can stay as is. 
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…d code to pull in metadata
…just icons so they dont shift the layout
github-actions
Bot
added
the
📦 Lexbox
Co-authored-by: Cursor <cursoragent@cursor.com>
4 tasks
Co-authored-by: Cursor <cursoragent@cursor.com>
The media-manager branch adopted Harmony's generic IRemoteResourceService<LcmFileMetadata> API but was still pinned to rc.202, which caused CS0308 build failures in CI.
Follows the SIL.Harmony rc.211 bump that made remote resources generic on metadata.
Resolve conflicts in CRDT regression snapshots and i18n catalogs, regenerating DataModelSnapshotTests verified files after merge. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (1)
151-164: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDispose
MediaFilesServicein the cleanup callback to prevent a DotNetObjectReference leak.
HistoryService?.Dispose()andSyncService?.Dispose()are called during cleanup, butMediaFilesServiceis not.DotNetObjectReferenceholds a GCHandle that must be explicitly disposed; omitting it leaks memory for every CRDT project that is opened and closed.🛡️ Proposed fix
HistoryService?.Dispose(); SyncService?.Dispose(); + MediaFilesService?.Dispose(); MiniLcm.Value.Dispose();🤖 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 `@backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs` around lines 151 - 164, The cleanup callback in ProjectServicesProvider is disposing HistoryService and SyncService but omits MediaFilesService, which leaves its DotNetObjectReference-backed GCHandle undisposed. Update the cleanup logic in the async Defer callback to also dispose MediaFilesService alongside the other project-scoped services, keeping the disposal order consistent with HistoryService, SyncService, MiniLcm, and serviceScope.
♻️ Duplicate comments (8)
frontend/viewer/src/locales/fr.po (2)
1275-1277: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Loaded file".Same issue as flagged in
es.po:msgid "Loaded file"has nomsgstrline before the next comment block, unlikeen.po.🐛 Proposed fix
#: src/lib/media-manager/MediaManagerView.svelte msgid "Loaded file" +msgstr "" #. Accessible label on the activity view loading spinner while filtered commits are fetched.🤖 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 `@frontend/viewer/src/locales/fr.po` around lines 1275 - 1277, The French PO entry for Loaded file is missing its msgstr block, which breaks the gettext entry structure. Update the locale entry around msgid "Loaded file" in fr.po to include a proper msgstr line before the next comment block, matching the pattern used in en.po and the other locale files. Keep the surrounding MediaManagerView.svelte translation entry intact and ensure the PO syntax remains valid.
1661-1663: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Offline, unable to open file".Same defect as flagged in
es.po: nomsgstrline followsmsgid "Offline, unable to open file".🐛 Proposed fix
#: src/lib/media-manager/MediaFileDetail.svelte msgid "Offline, unable to open file" +msgstr "" #. Sort option in activity view — earliest commit first.🤖 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 `@frontend/viewer/src/locales/fr.po` around lines 1661 - 1663, The French locale entry for the MediaFileDetail string is missing the required msgstr line after msgid "Offline, unable to open file", which breaks PO syntax. Update the affected translation entry in fr.po so it follows the same structure as the other locale files, using the msgid/msgstr pair for "Offline, unable to open file" and keeping the surrounding comment/entry formatting consistent.frontend/viewer/src/locales/ko.po (2)
1661-1663: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Offline, unable to open file".Same defect as flagged in other locale files: no
msgstrline followsmsgid "Offline, unable to open file".🐛 Proposed fix
#: src/lib/media-manager/MediaFileDetail.svelte msgid "Offline, unable to open file" +msgstr "" #. Sort option in activity view — earliest commit first.🤖 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 `@frontend/viewer/src/locales/ko.po` around lines 1661 - 1663, The PO entry for the MediaFileDetail.svelte string is malformed because msgid "Offline, unable to open file" is not followed by a msgstr line. Update the ko.po translation entry so it includes the missing msgstr line immediately after that msgid, matching the structure used by the surrounding locale entries and keeping the PO syntax valid.
1275-1277: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Loaded file".Same issue as flagged in other locale files:
msgid "Loaded file"has nomsgstrline before the next comment block.🐛 Proposed fix
#: src/lib/media-manager/MediaManagerView.svelte msgid "Loaded file" +msgstr "" #. Accessible label on the activity view loading spinner while filtered commits are fetched.🤖 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 `@frontend/viewer/src/locales/ko.po` around lines 1275 - 1277, The PO entry for “Loaded file” is missing its msgstr line, which breaks the locale file structure. In ko.po, update the translation block for the msgid tied to MediaManagerView so it includes a proper msgstr before the next comment block, matching the format used by the other locale files and preserving valid PO syntax.frontend/viewer/src/locales/ms.po (2)
1275-1277: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Loaded file".Same issue as flagged in other locale files:
msgid "Loaded file"has nomsgstrline before the next comment block.🐛 Proposed fix
#: src/lib/media-manager/MediaManagerView.svelte msgid "Loaded file" +msgstr "" #. Accessible label on the activity view loading spinner while filtered commits are fetched.🤖 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 `@frontend/viewer/src/locales/ms.po` around lines 1275 - 1277, The ms.po entry for “Loaded file” is missing its required msgstr line, which breaks PO syntax and leaves the next comment block attached to the wrong entry. Update the translation entry associated with msgid "Loaded file" in this locale file by adding the missing msgstr line before the following comment block, matching the same structure used by the other locale files and keeping the entry aligned with the MediaManagerView.svelte accessibility label.
1661-1663: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Offline, unable to open file".Same defect as flagged in other locale files: no
msgstrline followsmsgid "Offline, unable to open file". Since this same pattern repeats identically across all 5 non-English locale files for exactly these two strings, it's likely a bug in the extraction/Crowdin-sync tooling for this PR rather than a one-off typo — worth checking the generation pipeline in addition to patching these files.🐛 Proposed fix
#: src/lib/media-manager/MediaFileDetail.svelte msgid "Offline, unable to open file" +msgstr "" #. Sort option in activity view — earliest commit first.🤖 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 `@frontend/viewer/src/locales/ms.po` around lines 1661 - 1663, The PO entry for the “Offline, unable to open file” string is missing its msgstr, breaking syntax in this locale file. Update the affected translation entry in the locales generated from MediaFileDetail.svelte so every msgid is immediately followed by a valid msgstr line, and verify the same pattern in the other non-English locale files for this exact string pair. Also check the extraction/Crowdin-sync pipeline that produced these locale files to prevent the missing msgstr from recurring.frontend/viewer/src/locales/id.po (2)
1661-1663: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Offline, unable to open file".Same defect as flagged in other locale files: no
msgstrline followsmsgid "Offline, unable to open file".🐛 Proposed fix
#: src/lib/media-manager/MediaFileDetail.svelte msgid "Offline, unable to open file" +msgstr "" #. Sort option in activity view — earliest commit first.🤖 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 `@frontend/viewer/src/locales/id.po` around lines 1661 - 1663, Add the missing msgstr entry for the "Offline, unable to open file" translation in the id.po locale so the PO block is syntactically complete; update the MediaFileDetail.svelte-related entry to include a proper msgstr line immediately after the msgid, matching the pattern used by the other locale files and keeping the surrounding activity-view comment intact.
1275-1277: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
msgstrline breaks PO syntax for "Loaded file".Same issue as flagged in
es.po/fr.po:msgid "Loaded file"has nomsgstrline before the next comment block.🐛 Proposed fix
#: src/lib/media-manager/MediaManagerView.svelte msgid "Loaded file" +msgstr "" #. Accessible label on the activity view loading spinner while filtered commits are fetched.🤖 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 `@frontend/viewer/src/locales/id.po` around lines 1275 - 1277, The PO entry for "Loaded file" is missing its msgstr line, which breaks the syntax before the next comment block. Update the locale entry in the id.po translation file so the msgid "Loaded file" is immediately followed by a proper msgstr line, matching the pattern used in the other locale files and keeping the surrounding comment for the loading spinner entry intact.
🧹 Nitpick comments (2)
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
208-217: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
GetFileMetadatafetches metadata from the server but never persists it locally.When
resourceexists butMetadatais null, every call hits the server. TheAddMissingMetadatamethod (line 42) exists for exactly this purpose but isn't called here. Consider caching the fetched metadata to avoid repeated remote calls.♻️ Proposed refactor
public async Task<LcmFileMetadata> GetFileMetadata(Guid fileId) { var resource = await resourceService.GetResource(fileId); if (resource is not { Metadata: not null }) { var mediaClient = await MediaServerClient(); - return await mediaClient.GetFileMetadata(fileId); + var metadata = await mediaClient.GetFileMetadata(fileId); + if (resource is not null) + { + await AddMissingMetadata(resource, metadata); + } + return metadata; } return resource.Metadata; }🤖 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 `@backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs` around lines 208 - 217, GetFileMetadata currently falls back to MediaServerClient when resource.Metadata is null, but it returns the remote metadata without saving it locally, so repeated calls keep hitting the server. Update LcmMediaService.GetFileMetadata to use AddMissingMetadata after fetching from the media server, so the retrieved LcmFileMetadata is cached on the resource before returning it.backend/FwHeadless/Media/MediaFileService.cs (1)
116-131: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider isolating per-file errors during sync.
AddMissingMetadata,AddExistingRemoteResource, andDeleteResourceare awaited sequentially per file with no try/catch; a single problematic resource (e.g., missing local file, server error) would abort the whole sync loop and leave remaining files unsynced for that pass.🤖 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 `@backend/FwHeadless/Media/MediaFileService.cs` around lines 116 - 131, The sync loop in MediaFileService currently awaits AddMissingMetadata, AddExistingRemoteResource, and DeleteResource without any per-resource error handling, so one failure can abort the rest of the pass. Wrap the per-file work in the sync routine that processes lcmResources and existingDbFile entries with try/catch so each resource is handled independently, log or record the failure with the relevant resource id/path, and continue to the next item instead of stopping the whole loop.
🤖 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 `@backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs`:
- Around line 285-297: The test setup in UseChangesTests is reusing the same
entity id for both the pending-upload chain and the create/delete branch, which
makes the dependency graph order-dependent. Update the create/delete branch to
use its own distinct entity id in the local variables around
createRemoteResourceChange and DeleteRemoteResourceChange, while keeping the
pending-upload flow on createRemoteResourcePendingUploadChange.EntityId. Make
sure the yielded ChangeWithDependencies entries still reference the correct
entity-specific ids and dependencies so CanAddAllChangeTypes() exercises
independent chains.
In `@backend/FwLite/LcmCrdt.Tests/ConfigRegistrationTests.cs`:
- Around line 17-23: The exclusion list in ConfigRegistrationTests is using the
wrong delete type for remote resources; update the test data to exclude
DeleteRemoteResourceChange<LcmFileMetadata> instead of
DeleteChange<RemoteResource<LcmFileMetadata>>. Make the change in the collection
that includes JsonPatchChange and DeleteChange entries so it matches the
concrete type actually used by the remote-resource delete path.
In `@backend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.cs`:
- Around line 27-39: `FileMetadata.SizeInBytes` is still using `int?`, which no
longer matches the widened server contract and can break large file uploads.
Update the `FileMetadata` DTO in `IMediaServerClient` to use `long?`, then
propagate the same type through `LcmMediaService.UploadResource` and any related
`LcmFileMetadata` mapping so `response.Metadata?.SizeInBytes` is handled as a
long consistently.
In `@frontend/viewer/src/lib/media-manager/MediaFileDetail.svelte`:
- Around line 202-231: The Save As flow in loadBlobForSave and saveAs can fail
silently when mediaFilesService is unavailable. Add user-facing feedback before
returning undefined in loadBlobForSave, and ensure saveAs surfaces that failure
path instead of just exiting; use the existing AppNotification.error pattern and
keep the fix centered on the loadBlobForSave/saveAs logic in MediaFileDetail.
- Around line 81-100: The metadata resolver in MediaFileDetail.svelte is
swallowing service fetch failures and never falling back to the synced
file.metadata value, which can leave metadata.current undefined for remote-only
files. Update the resource callback tied to file.id, file.remote,
file.localPath, and mediaFilesService to prefer file.metadata as a fallback (or
initial value) before/after calling service.getFileMetadata(), matching the
behavior used by MediaManagerView.svelte’s displayName logic. Keep the
localPath-based guess as the last fallback so the UI can still render a
meaningful state when the network lookup fails.
- Around line 142-168: The preview loader in MediaFileDetail’s resource callback
can outlive a superseded run and create an unreleased blob URL. Thread the
resource abort signal through the preview-loading path by updating the
loadFileBlob call inside the preview resource to accept and honor cancellation,
or ensure the URL is revoked in the loader’s cleanup when the request is
abandoned. Use the preview resource and loadFileBlob symbols to locate the
affected flow.
In `@frontend/viewer/src/lib/media-manager/MediaManagerView.svelte`:
- Around line 29-51: Extract the duplicated media helper logic into a shared
media-manager utility module and use it from MediaManagerView.svelte and
MediaFileDetail.svelte. Move the LocationStatus type, locationLabels map, and
the filename-resolution helper currently in displayName() into a common helper
(for example a media-utils module), then update both components to import and
call the shared symbols instead of maintaining separate copies. Keep the shared
helper as the single source of truth for metadata filename handling, localPath
fallback, and status labels.
In `@frontend/viewer/src/locales/es.po`:
- Around line 1275-1277: The Spanish PO catalog is missing the translation entry
for the “Loaded file” msgid, leaving the block malformed. Update the entry in
the es.po locale file by adding the missing msgstr for the msgid "Loaded file"
before the next #:/#. comment block, keeping the PO structure consistent with
the surrounding entries.
- Around line 1661-1663: The PO entry for the MediaFileDetail string is
malformed because the translation block for msgid "Offline, unable to open file"
is missing its msgstr line. Update this entry in the locale file so it includes
a proper msgstr for that msgid, keeping the surrounding comment and metadata
intact, and verify other nearby entries in the same section remain correctly
formatted.
In `@frontend/viewer/src/locales/sw.po`:
- Around line 1661-1663: The PO entry for the “Offline, unable to open file”
string is incomplete because it is missing its msgstr line, causing the next
translation entry to be parsed inside the same block. In the sw locale file, add
the missing empty msgstr for the msgid "Offline, unable to open file" before the
following “Oldest first” entry, and make the same correction in the vi locale
file. Use the existing PO entry structure around the affected msgid/msgstr pairs
to restore a valid translation block.
- Around line 1275-1277: The PO entries are malformed because the `msgid` lines
for `Loaded file` and `Offline, unable to open file` are missing their
corresponding `msgstr ""` lines, causing adjacent entries to merge. Update the
affected entries in the `sw.po` locale file so each `msgid` is immediately
followed by an empty `msgstr` placeholder, keeping the existing entry structure
intact and preventing translation compilation errors.
In `@frontend/viewer/src/locales/vi.po`:
- Around line 1661-1663: The PO entry for the msgid “Offline, unable to open
file” is missing its msgstr field, causing the following entry to be parsed as
part of the same block. Update the vi.po localization entry near the
MediaFileDetail.svelte translation so that the msgid has a corresponding empty
msgstr line before the next comment or msgid starts.
- Around line 1275-1277: The vi.po translation entries for the strings handled
by MediaManagerView.svelte are malformed because they end at msgid without a
matching msgstr. Add the missing msgstr "" line for both “Loaded file” and
“Offline, unable to open file” in frontend/viewer/src/locales/vi.po so each PO
entry is complete and parseable.
---
Outside diff comments:
In `@backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs`:
- Around line 151-164: The cleanup callback in ProjectServicesProvider is
disposing HistoryService and SyncService but omits MediaFilesService, which
leaves its DotNetObjectReference-backed GCHandle undisposed. Update the cleanup
logic in the async Defer callback to also dispose MediaFilesService alongside
the other project-scoped services, keeping the disposal order consistent with
HistoryService, SyncService, MiniLcm, and serviceScope.
---
Duplicate comments:
In `@frontend/viewer/src/locales/fr.po`:
- Around line 1275-1277: The French PO entry for Loaded file is missing its
msgstr block, which breaks the gettext entry structure. Update the locale entry
around msgid "Loaded file" in fr.po to include a proper msgstr line before the
next comment block, matching the pattern used in en.po and the other locale
files. Keep the surrounding MediaManagerView.svelte translation entry intact and
ensure the PO syntax remains valid.
- Around line 1661-1663: The French locale entry for the MediaFileDetail string
is missing the required msgstr line after msgid "Offline, unable to open file",
which breaks PO syntax. Update the affected translation entry in fr.po so it
follows the same structure as the other locale files, using the msgid/msgstr
pair for "Offline, unable to open file" and keeping the surrounding
comment/entry formatting consistent.
In `@frontend/viewer/src/locales/id.po`:
- Around line 1661-1663: Add the missing msgstr entry for the "Offline, unable
to open file" translation in the id.po locale so the PO block is syntactically
complete; update the MediaFileDetail.svelte-related entry to include a proper
msgstr line immediately after the msgid, matching the pattern used by the other
locale files and keeping the surrounding activity-view comment intact.
- Around line 1275-1277: The PO entry for "Loaded file" is missing its msgstr
line, which breaks the syntax before the next comment block. Update the locale
entry in the id.po translation file so the msgid "Loaded file" is immediately
followed by a proper msgstr line, matching the pattern used in the other locale
files and keeping the surrounding comment for the loading spinner entry intact.
In `@frontend/viewer/src/locales/ko.po`:
- Around line 1661-1663: The PO entry for the MediaFileDetail.svelte string is
malformed because msgid "Offline, unable to open file" is not followed by a
msgstr line. Update the ko.po translation entry so it includes the missing
msgstr line immediately after that msgid, matching the structure used by the
surrounding locale entries and keeping the PO syntax valid.
- Around line 1275-1277: The PO entry for “Loaded file” is missing its msgstr
line, which breaks the locale file structure. In ko.po, update the translation
block for the msgid tied to MediaManagerView so it includes a proper msgstr
before the next comment block, matching the format used by the other locale
files and preserving valid PO syntax.
In `@frontend/viewer/src/locales/ms.po`:
- Around line 1275-1277: The ms.po entry for “Loaded file” is missing its
required msgstr line, which breaks PO syntax and leaves the next comment block
attached to the wrong entry. Update the translation entry associated with msgid
"Loaded file" in this locale file by adding the missing msgstr line before the
following comment block, matching the same structure used by the other locale
files and keeping the entry aligned with the MediaManagerView.svelte
accessibility label.
- Around line 1661-1663: The PO entry for the “Offline, unable to open file”
string is missing its msgstr, breaking syntax in this locale file. Update the
affected translation entry in the locales generated from MediaFileDetail.svelte
so every msgid is immediately followed by a valid msgstr line, and verify the
same pattern in the other non-English locale files for this exact string pair.
Also check the extraction/Crowdin-sync pipeline that produced these locale files
to prevent the missing msgstr from recurring.
---
Nitpick comments:
In `@backend/FwHeadless/Media/MediaFileService.cs`:
- Around line 116-131: The sync loop in MediaFileService currently awaits
AddMissingMetadata, AddExistingRemoteResource, and DeleteResource without any
per-resource error handling, so one failure can abort the rest of the pass. Wrap
the per-file work in the sync routine that processes lcmResources and
existingDbFile entries with try/catch so each resource is handled independently,
log or record the failure with the relevant resource id/path, and continue to
the next item instead of stopping the whole loop.
In `@backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs`:
- Around line 208-217: GetFileMetadata currently falls back to MediaServerClient
when resource.Metadata is null, but it returns the remote metadata without
saving it locally, so repeated calls keep hitting the server. Update
LcmMediaService.GetFileMetadata to use AddMissingMetadata after fetching from
the media server, so the retrieved LcmFileMetadata is cached on the resource
before returning it.
🪄 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: 5af5ec3b-46f7-48ae-ba67-3b3fe8ee4fef
📒 Files selected for processing (52)
LexBox.slnbackend/Directory.Packages.propsbackend/FwHeadless/Controllers/MediaFileController.csbackend/FwHeadless/Media/MediaFileService.csbackend/FwHeadless/Models/MediaFileModels.csbackend/FwLite/FwLiteShared/Services/FwLiteProvider.csbackend/FwLite/FwLiteShared/Services/MediaFilesServiceJsInvokable.csbackend/FwLite/FwLiteShared/Services/ProjectServicesProvider.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.legacy.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/ConfigRegistrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.legacy.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectBaseModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectWithIdModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.csbackend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.csbackend/FwLite/LcmCrdt/MediaServer/LcmMediaService.csbackend/FwLite/LcmCrdt/Migrations/20260617054015_AddRemoteResourceMetadataColumn.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260617054015_AddRemoteResourceMetadataColumn.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/MiniLcm/Media/FileMetadata.csbackend/LexCore/Entities/MediaFile.csbackend/Testing/FwHeadless/MediaFileTests.csfrontend/viewer/src/DotnetProjectView.sveltefrontend/viewer/src/ShadcnProjectView.sveltefrontend/viewer/src/lib/components/field-editors/audio-input.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.tsfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMediaFilesServiceJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectScope.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.tsfrontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/IHarmonyResource.tsfrontend/viewer/src/lib/dotnet-types/generated-types/SIL/Harmony/Resource/IRemoteResource.tsfrontend/viewer/src/lib/media-manager/MediaFileDetail.sveltefrontend/viewer/src/lib/media-manager/MediaManagerView.sveltefrontend/viewer/src/lib/services/service-provider.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/ProjectSidebar.sveltefrontend/viewer/src/project/project-context.svelte.ts
…and file loading processes. Introduce throwIfAborted function to manage abort scenarios, ensuring graceful handling of aborted requests. Update resource loading logic to utilize abort signals, improving responsiveness during file operations.
Replace the dependency-only $effect with runed watch so clearing stale metadata/preview on row change passes lint. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lets users view media associated with the project. Manually trigger downloading all media or upload pending.
This has highlighted an issue with our current strategy/design. Right now the metadata for media files only lives on lexbox, if the file isn't downloaded we don't even know the file name or type. It would be really nice to store the metadata with the resource, so that gets synced between clients automatically. This may require a harmony change, but there could be other ways to do this.