-
-
Notifications
You must be signed in to change notification settings - Fork 4
Allow recording and saving audio #1836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e to how the slider works
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 📝 WalkthroughWalkthroughThis update introduces a comprehensive media file management feature across the backend and frontend. It adds new types, APIs, GraphQL queries, and TypeScript interfaces for handling file uploads, metadata, and queries. The backend supports file saving, uploading, and synchronization, while the frontend enables users to upload, validate, and manage audio files with real-time feedback and improved integration with .NET services. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 minutes Possibly related PRs
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (3)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
1579-1588
: Consider adding validation for MIME type parameter.The TypeToLinkedFolder method correctly maps MIME types to folders, but consider adding validation to handle null or empty MIME type strings gracefully.
private string TypeToLinkedFolder(string mimeType) { + if (string.IsNullOrEmpty(mimeType)) return "Others"; return mimeType switch { { } s when s.StartsWith("audio/") => AudioVisualFolder, {} s when s.StartsWith("video/") => AudioVisualFolder, { } s when s.StartsWith("image/") => "Pictures", _ => "Others" }; }
backend/FwLite/MiniLcm/Media/UploadFileResponse.cs (1)
5-29
: Consider adding factory methods for better API clarityThe multiple constructors with different validation rules could be confusing for consumers. Consider adding static factory methods for clearer intent:
public record UploadFileResponse { + public static UploadFileResponse Success(MediaUri mediaUri, bool savedToLexbox) + => new(mediaUri, savedToLexbox); + + public static UploadFileResponse Failure(UploadFileResult result) + => new(result); + + public static UploadFileResponse Error(string errorMessage) + => new(errorMessage); + public UploadFileResponse(MediaUri mediaUri, bool savedToLexbox) { MediaUri = mediaUri; Result = savedToLexbox ? UploadFileResult.SavedToLexbox : UploadFileResult.SavedLocally; }This would make the API usage more explicit and self-documenting.
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (1)
109-133
: Consider adding support for additional audio formatsThe MIME type mapping could include more formats for better compatibility:
case 'audio/m4a': return 'm4a'; + case 'audio/flac': + return 'flac'; + case 'audio/opus': + return 'opus'; + case 'audio/3gpp': + case 'audio/3gpp2': + return '3gp'; default: return 'audio';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
backend/FwHeadless/LexboxFwDataMediaAdapter.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MediaFileTests.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
(2 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs
(2 hunks)backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs
(3 hunks)backend/FwLite/FwLiteShared/Sync/SyncService.cs
(4 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs
(6 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
(2 hunks)backend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.cs
(1 hunks)backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
(3 hunks)backend/FwLite/MiniLcm/IMiniLcmReadApi.cs
(1 hunks)backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs
(2 hunks)backend/FwLite/MiniLcm/Media/FileMetadata.cs
(1 hunks)backend/FwLite/MiniLcm/Media/MediaFile.cs
(1 hunks)backend/FwLite/MiniLcm/Media/MediaUri.cs
(2 hunks)backend/FwLite/MiniLcm/Media/ReadFileResponse.cs
(1 hunks)backend/FwLite/MiniLcm/Media/UploadFileResponse.cs
(1 hunks)backend/LexBoxApi/GraphQL/LexQueries.cs
(1 hunks)backend/Testing/FwHeadless/MediaFileServiceTests.cs
(1 hunks)backend/harmony
(1 hunks)frontend/schema.graphql
(9 hunks)frontend/viewer/src/lib/components/audio/AudioDialog.svelte
(4 hunks)frontend/viewer/src/lib/components/audio/audio-editor.svelte
(1 hunks)frontend/viewer/src/lib/components/field-editors/audio-input.svelte
(4 hunks)frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte
(1 hunks)frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte
(3 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.ts
(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IReadFileResponseJs.ts
(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts
(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IMediaFile.ts
(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IUploadFileResponse.ts
(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/MediaFileType.ts
(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/UploadFileResult.ts
(1 hunks)frontend/viewer/src/lib/services/service-provider-dotnet.ts
(4 hunks)
🧠 Learnings (15)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs (2)
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/MiniLcm/Media/ReadFileResponse.cs (1)
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
backend/Testing/FwHeadless/MediaFileServiceTests.cs (1)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
frontend/viewer/src/lib/components/audio/audio-editor.svelte (3)
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/FwLiteShared/Sync/SyncService.cs (2)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte (3)
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: hahn-kev
PR: #1827
File: frontend/viewer/src/lib/i18n/LocalizationPicker.svelte:12-15
Timestamp: 2025-07-17T05:09:27.809Z
Learning: In Svelte 5, the bind:value directive supports function binding syntax where you can pass two functions separated by a comma: bind:value={getter, setter}
. For example, bind:value={() => $locale, l => setLanguage(l)}
is valid syntax where the first function is the getter and the second is the setter. This is not a JavaScript comma operator issue - it's a specific Svelte 5 compiler feature for two-way binding.
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (3)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (3)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
frontend/viewer/src/lib/components/field-editors/audio-input.svelte (3)
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (2)
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: myieye
PR: #1802
File: frontend/viewer/src/project/NewEntryButton.svelte:36-36
Timestamp: 2025-07-04T17:00:57.368Z
Learning: In this codebase, $props.id()
(Svelte rune) automatically returns a unique identifier per component instance, so components using it do not require an explicit id
prop from parents.
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (2)
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
🧬 Code Graph Analysis (4)
backend/FwHeadless/LexboxFwDataMediaAdapter.cs (2)
backend/FwHeadless/Media/MediaFileService.cs (2)
MediaFile
(72-89)MediaFile
(91-94)backend/LexCore/Entities/MediaFile.cs (1)
MediaFile
(8-41)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IMediaFile.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts (1)
ILcmFileMetadata
(6-12)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (3)
backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs (2)
MediaUri
(31-38)MediaUri
(40-43)backend/FwLite/MiniLcm/Media/MediaUri.cs (4)
MediaUri
(12-16)MediaUri
(18-21)MediaUri
(23-29)MediaUri
(47-52)backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs (1)
MediaUri
(15-15)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.ts (2)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts (1)
ILcmFileMetadata
(6-12)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IUploadFileResponse.ts (1)
IUploadFileResponse
(8-13)
🧰 Additional context used
🧠 Learnings (15)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs (2)
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/MiniLcm/Media/ReadFileResponse.cs (1)
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
backend/Testing/FwHeadless/MediaFileServiceTests.cs (1)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
frontend/viewer/src/lib/components/audio/audio-editor.svelte (3)
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/FwLiteShared/Sync/SyncService.cs (2)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte (3)
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: hahn-kev
PR: #1827
File: frontend/viewer/src/lib/i18n/LocalizationPicker.svelte:12-15
Timestamp: 2025-07-17T05:09:27.809Z
Learning: In Svelte 5, the bind:value directive supports function binding syntax where you can pass two functions separated by a comma: bind:value={getter, setter}
. For example, bind:value={() => $locale, l => setLanguage(l)}
is valid syntax where the first function is the getter and the second is the setter. This is not a JavaScript comma operator issue - it's a specific Svelte 5 compiler feature for two-way binding.
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (3)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (3)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
frontend/viewer/src/lib/components/field-editors/audio-input.svelte (3)
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (2)
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: myieye
PR: #1802
File: frontend/viewer/src/project/NewEntryButton.svelte:36-36
Timestamp: 2025-07-04T17:00:57.368Z
Learning: In this codebase, $props.id()
(Svelte rune) automatically returns a unique identifier per component instance, so components using it do not require an explicit id
prop from parents.
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (2)
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
🧬 Code Graph Analysis (4)
backend/FwHeadless/LexboxFwDataMediaAdapter.cs (2)
backend/FwHeadless/Media/MediaFileService.cs (2)
MediaFile
(72-89)MediaFile
(91-94)backend/LexCore/Entities/MediaFile.cs (1)
MediaFile
(8-41)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IMediaFile.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts (1)
ILcmFileMetadata
(6-12)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (3)
backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs (2)
MediaUri
(31-38)MediaUri
(40-43)backend/FwLite/MiniLcm/Media/MediaUri.cs (4)
MediaUri
(12-16)MediaUri
(18-21)MediaUri
(23-29)MediaUri
(47-52)backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs (1)
MediaUri
(15-15)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.ts (2)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts (1)
ILcmFileMetadata
(6-12)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IUploadFileResponse.ts (1)
IUploadFileResponse
(8-13)
⏰ 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). (8)
- GitHub Check: Build API / publish-api
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build UI / publish-ui
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: frontend
- GitHub Check: Analyze (csharp)
🔇 Additional comments (51)
backend/harmony (1)
1-1
: Confirm intentional Harmony submodule bumpOnly the submodule SHA has changed. Please double-check that:
- The targeted commit
4305ac0d3…
is the intended one (e.g., includes the new audio-handling APIs).- CI/jobs that fetch submodules run
git submodule update --init --recursive
after this merge.- Any dependent version constraints or release notes are updated.
If all good, no further action needed.
backend/FwLite/FwDataMiniLcmBridge.Tests/MediaFileTests.cs (1)
4-4
: Import addition looks correctThe new
using MiniLcm.Media;
directive is required now thatMediaUri
and friends were moved. No further issues spotted here.backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateDictionaryProxy.cs (1)
4-4
: Necessary namespace added
MediaUri
is now defined inMiniLcm.Media
; this import is therefore essential. 👍backend/Testing/FwHeadless/MediaFileServiceTests.cs (1)
11-11
: Good catchBringing in
MiniLcm.Media
keeps the tests compiling against the moved types. Looks good.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IReadFileResponseJs.ts (1)
6-6
: Path fix acknowledgedThe updated relative import matches the new location of
ReadFileResult
. ✔backend/FwLite/MiniLcm/Media/ReadFileResponse.cs (1)
3-3
: Namespace relocation verified – no stale references foundI searched the entire repository for
MiniLcm.Models.ReadFileResponse
andMiniLcm.Models.ReadFileResult
and found no lingering references. All usages have been updated to the newMiniLcm.Media
namespace.backend/FwHeadless/LexboxFwDataMediaAdapter.cs (1)
7-9
: LGTM! Proper namespace imports for media functionality.The addition of
using MiniLcm.Media;
and the alias for MediaFile properly support the media handling functionality while avoiding naming conflicts betweenMiniLcm.Media.MediaFile
andLexCore.Entities.MediaFile
.backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs (1)
2-2
: LGTM! Required import for MediaUri usage.The
using MiniLcm.Media;
directive is necessary to support theMediaUri
type used in the interface method signatures.backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (1)
4-4
: LGTM! Essential import for MediaUri type.The
using MiniLcm.Media;
import is required for theGetFileStream(MediaUri mediaUri)
method signature to compile correctly.frontend/viewer/src/lib/components/audio/audio-editor.svelte (3)
12-12
: LGTM! Proper externalization of file name.Adding
name
as a required prop allows parent components to control the displayed file name, which is more flexible than deriving it internally from the audio blob.
16-16
: LGTM! Consistent prop destructuring.The destructuring correctly includes the new
name
prop alongside existing props.
31-34
: LGTM! Appropriate conditional rendering.The conditional display of the file name prevents showing empty or undefined names in the UI, which provides a better user experience.
backend/FwLite/MiniLcm/Media/FileMetadata.cs (1)
3-7
: LGTM! Well-designed metadata record.The
LcmFileMetadata
record is well-structured with:
- Appropriate required properties (
Filename
,MimeType
)- Sensible optional metadata (
Author
,UploadDate
)- Correct use of
DateTimeOffset
for timezone-aware timestamps- Immutable record type providing value equality semantics
backend/FwLite/MiniLcm/Media/MediaFile.cs (1)
1-6
: LGTM! Clean and well-structured record definition.The
MediaFile
record follows modern C# best practices with immutable positional parameters. The implementation is concise and serves as an effective data transfer object for the media management system.frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IUploadFileResponse.ts (1)
6-13
: LGTM! Well-structured upload response interface.The interface design is logical with appropriate required/optional property distinctions:
result
is required (every response needs a result status)errorMessage
andmediaUri
are optional (context-dependent)The type structure aligns well with typical file upload response patterns.
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/MediaFileType.ts (1)
6-13
: LGTM! Comprehensive media file type enumeration.The enum covers the essential media file categories with clear, descriptive string values. The inclusion of "Other" as a fallback and the focus on "Audio" aligns well with the PR's audio recording objectives.
backend/LexBoxApi/GraphQL/LexQueries.cs (1)
271-279
: LGTM! Well-designed GraphQL query with appropriate security controls.The
MediaFiles
query follows established patterns in the codebase and includes:
- Proper admin-only access restriction via
[AdminRequired]
- Comprehensive query capabilities (pagination, filtering, sorting, projection)
- Clean, minimal implementation consistent with other queries
The implementation appropriately leverages the GraphQL decorators for functionality while maintaining security boundaries.
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IMediaFile.ts (1)
6-12
: LGTM! Well-structured media file interface with comprehensive data model.The interface effectively represents media files with:
- Required
uri
for file location/identification- Required
metadata
providing essential file information (filename, mimeType, author, uploadDate)The structure ensures data completeness while supporting the media management functionality introduced in this PR.
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (2)
2-2
: LGTM!The addition of the
MiniLcm.Media
namespace import is appropriate for supporting the new media file functionality.
125-135
: Well-designed interface extension.The
SaveFile
method follows established patterns in the interface:
- Consistent async/await signature
- Clear XML documentation
- Sensible default implementation returning
NotSupported
- Appropriate parameter types for stream and metadata
This provides a clean contract for media file operations across different implementations.
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/UploadFileResult.ts (1)
1-14
: Well-structured enum for upload results.The
UploadFileResult
enum provides comprehensive coverage of file upload scenarios:
- Success cases for different storage targets (
SavedLocally
,SavedToLexbox
)- Appropriate error cases (
TooBig
,NotSupported
,AlreadyExists
,Error
)- String-based values facilitate serialization between backend and frontend
The generated code follows proper conventions with ESLint disabled for auto-generated files.
frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte (1)
53-53
: Proper two-way binding for audio inputs.The change from one-way prop to two-way binding (
bind:audioId
) with anonchange
handler aligns theAudioInput
usage with the text input pattern above. This enables reactive updates when audio files are uploaded or changed, maintaining consistency across input types.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.ts (2)
21-22
: Appropriate type imports for media functionality.The imports for
IUploadFileResponse
andILcmFileMetadata
are correctly added to support the newsaveFile
method.
68-68
: Well-designed saveFile method signature.The method provides good flexibility with the stream parameter accepting multiple types (
Blob | ArrayBuffer | Uint8Array
) to accommodate different frontend upload scenarios. The return type correctly aligns with the upload response interface.frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts (1)
6-12
: Well-structured file metadata interface.The
ILcmFileMetadata
interface appropriately defines essential file metadata:
- Required fields (
filename
,mimeType
) for core file identification- Optional fields (
author
,uploadDate
) for additional context- Consistent string typing throughout
- Follows TypeScript naming conventions
This provides proper typing support for the media file upload functionality.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (2)
20-20
: LGTM - Clean namespace import for media types.The addition of the
MiniLcm.Media
namespace import supports the new media file handling functionality.
703-715
: Well-implemented file save method with proper error handling.The
SaveFile
method correctly:
- Uses the injected
lcmMediaService
for file operations- Constructs a
MediaUri
with the project's server ID (with sensible fallback to "lexbox.org")- Provides comprehensive error handling with logging
- Returns appropriate success/error responses
The implementation follows established patterns in the codebase and integrates well with the existing media infrastructure.
backend/FwLite/FwLiteShared/Sync/SyncService.cs (4)
7-7
: LGTM - Required dependency import.Adding the
LcmCrdt.MediaServer
namespace import to support the new media upload functionality.
30-30
: LGTM - Proper dependency injection.The
LcmMediaService
dependency is correctly added to the constructor to support media upload operations during sync.
86-86
: Good placement of media upload in sync flow.Calling
UploadPendingMedia()
before the main data synchronization ensures that media files are uploaded when the connection is available, which is the optimal timing for this operation.
146-156
: Excellent error handling that preserves sync robustness.The
UploadPendingMedia
method implementation is well-designed:
- Exceptions are caught and logged without disrupting the main sync process
- This ensures that media upload failures don't prevent data synchronization
- The error logging provides sufficient information for debugging
This approach maintains the reliability of the sync process while adding new functionality.
backend/FwLite/LcmCrdt/MediaServer/IMediaServerClient.cs (2)
11-17
: Well-designed multipart upload API with clear documentation.The
UploadFile
method is properly implemented:
- Correct Refit attributes for multipart file upload
- Clear comment explaining the string fileId workaround for Refit limitations
- Sensible optional parameters for metadata (author, filename)
- Appropriate use of query parameter for projectId
The API design follows RESTful conventions and handles the technical constraints well.
20-20
: Simple and appropriate response record.The
MediaUploadFileResponse
record provides a clean way to return the uploaded file's GUID from the server.backend/FwLite/MiniLcm/Media/MediaUri.cs (3)
1-4
: LGTM - Proper namespace organization and JSON support.The addition of JSON serialization imports and the move to the
MiniLcm.Media
namespace properly organizes the media-related types and enables JSON serialization support.
6-6
: Correct JSON converter attribute.The
JsonConverter
attribute properly specifies the custom converter class forMediaUri
serialization.
45-58
: Well-implemented JSON converter with robust null handling.The
MediaUriJsonConverter
is correctly implemented:
Read
method handles null values gracefully by returningMediaUri.NotFound
Write
method uses the existingToString()
method for consistency- Follows standard JSON converter patterns
- Provides reliable serialization/deserialization for the
MediaUri
typeThis ensures that
MediaUri
objects can be properly serialized in API responses and configuration files.backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
1545-1577
: LGTM! Well-structured file saving implementation with proper error handling.The SaveFile method implementation is solid with good separation of concerns:
- Proper async/await usage with stream handling
- Comprehensive error handling for directory creation and file operations
- Appropriate use of Path.Combine for cross-platform compatibility
- Correct disposal pattern with
await using
frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (3)
23-23
: Good type relaxation to support optional audio values.The change from
IRichString
toIRichString | undefined
is appropriate since audio fields can be empty or undefined.
45-53
: Well-implemented audio state management function.The setAudioId function properly handles both setting and clearing audio IDs, correctly updates the value object, and triggers the onchange callback appropriately.
78-78
: Excellent use of Svelte's two-way binding pattern.The binding pattern
bind:audioId={() => getAudioId(value[ws.wsId]), audioId => setAudioId(audioId, ws.wsId)}
follows Svelte best practices for custom two-way binding with getter/setter functions.backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (2)
349-349
: Appropriate file size limit for web uploads.The 10MB limit is reasonable for audio files and prevents abuse while allowing typical use cases.
352-366
: Robust JS interop implementation with proper resource management.The SaveFile method implementation is excellent:
- Size validation before processing prevents resource waste
- Proper async stream handling with disposal
- Error logging for disposal failures without breaking the main flow
- Correct delegation to the underlying API
backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs (2)
40-43
: Clean separation of URI generation logic.The PathToUri method provides a focused, single-responsibility function for generating MediaUri from file paths. The implementation is clean and reusable.
31-38
: Improved cache management with on-demand updates.The updated MediaUriFromPath method correctly handles new files by updating the cache after URI generation, ensuring the cache stays synchronized with the file system.
frontend/viewer/src/lib/components/field-editors/audio-input.svelte (4)
51-51
: Correct import path after namespace reorganization.The updated import path properly reflects the movement of ReadFileResult to the MiniLcm.Media namespace.
22-25
: Good handling of edge case with Infinity duration.Converting Infinity to NaN is appropriate for audio elements that haven't fully loaded or have streaming content where duration isn't determinable.
60-64
: Well-designed callback interface for reactive updates.The optional onchange callback with a sensible default allows parent components to react to audio ID changes while maintaining backward compatibility.
197-205
: Robust error handling with proper resource cleanup.The try-finally pattern ensures URL revocation and cleanup happens even if the onchange callback fails, preventing memory leaks.
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
58-58
: ArrayBuffer support verified across modern browsersArrayBuffer is natively supported in all modern desktop and mobile browsers (Chrome 7+, Firefox 4+, Edge 12+, Safari 5.1+, IE 10+, Opera 12.1+), so including it in the
IJSStreamReference
union type is safe. No changes required.frontend/schema.graphql (2)
182-191
:sizeInBytes
should beLong
, notInt
GraphQL
Int
is 32-bit signed (≈2 GB max). High-quality audio or video easily exceeds that. Use the existingLong
scalar already declared in the schema and update the related filter/sort inputs.- sizeInBytes: Int + sizeInBytes: LongAlso update:
- sizeInBytes: IntOperationFilterInput + sizeInBytes: LongOperationFilterInputand add a corresponding
LongOperationFilterInput
(patterned afterIntOperationFilterInput
).
508-509
: Double-check authorization scope formediaFiles
queryThe new
mediaFiles
field is@authorize(policy: "AdminRequiredPolicy")
, yet the PR description says it is “for testing purposes only.”
If this query is not meant for production, consider:
- Hiding it behind a compile-time
#if DEBUG
or feature flag similar to the UI flag already mentioned.- Adding a comment with a removal TODO to avoid accidentally exposing it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes needed here. Some were caught by CodeRabbit, but it missed the Path.Combine
usage that should have been Path.Join
since that wasn't among the changed lines of this PR. But the two that it caught (1034 typo that should have been 1024, and File.Overwrite that should have been File.Create) are both real issues. I would have caught the File.Overwrite issue myself but I might have missed the 1034 typo.
…ated file in case of an error recording the file record in the db
@rmunn I've handled all your suggestions. For the Path join stuff I actually refactored the |
…edia uri is not NotFound
…dicate that you can read length
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
1551-1583
: Use named parameters for boolean arguments.For better readability, consider using named parameters when passing boolean values, as suggested in past reviews.
- return new UploadFileResponse(mediaAdapter.MediaUriFromPath(fullPath, Cache), savedToLexbox: false, newResource: false); + return new UploadFileResponse(mediaAdapter.MediaUriFromPath(fullPath, Cache), savedToLexbox: false, newResource: false);Wait, I see the named parameters are already being used. The code is already following the best practice.
1585-1594
: Add consistent spacing in switch expression.For consistency with other patterns, add a space after the closing brace.
private string TypeToLinkedFolder(string mimeType) { return mimeType switch { { } s when s.StartsWith("audio/") => AudioVisualFolder, - { } s when s.StartsWith("video/") => AudioVisualFolder, + { } s when s.StartsWith("video/") => AudioVisualFolder, { } s when s.StartsWith("image/") => "Pictures", _ => "Others" }; }
🧹 Nitpick comments (3)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs (1)
21-26
: Add null check or use pattern matching for safer type casting.The direct cast to
FwDataMiniLcmApi
could throw anInvalidCastException
. Consider using pattern matching for safer casting:public override async Task InitializeAsync() { await base.InitializeAsync(); - var projectFolder = ((FwDataMiniLcmApi)Api).Cache.LangProject.LinkedFilesRootDir; + if (Api is not FwDataMiniLcmApi fwDataApi) + throw new InvalidOperationException("Expected FwDataMiniLcmApi instance"); + var projectFolder = fwDataApi.Cache.LangProject.LinkedFilesRootDir; Directory.CreateDirectory(projectFolder); }backend/FwLite/MiniLcm.Tests/MediaTestsBase.cs (1)
109-121
: Update or remove misleading comment.The comment mentions "simulating a small image or audio file" but the test creates a 25MB file, which is not small. This appears to be copy-pasted from the previous test.
[Fact] public async Task FileOperations_FileTooLarge_FailsGracefully() { - // Arrange - Create test binary data (simulating a small image or audio file) + // Arrange - Create test binary data exceeding the size limit var originalBytes = new byte[1024 * 1024 * 25]; // 25 MB var random = new Random(42); // Use fixed seed for reproducible tests random.NextBytes(originalBytes);backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs (1)
29-38
: Update outdated comment to reflect absolute path requirement.The comment states paths should be relative, but the implementation requires absolute paths within LinkedFilesRootDir.
- //path is expected to be relative to the LinkedFilesRootDir + // path must be an absolute path within the LinkedFilesRootDir public MediaUri MediaUriFromPath(string path, LcmCache cache)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/FwHeadless/LexboxFwDataMediaAdapter.cs
(2 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/FwDataTestsKernel.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/ProjectLoaderFixture.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MediaFileTests.cs
(4 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
(4 hunks)backend/FwLite/FwDataMiniLcmBridge/LexEntryFilterMapProvider.cs
(1 hunks)backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs
(3 hunks)backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs
(3 hunks)backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs
(5 hunks)backend/FwLite/LcmCrdt.Tests/MiniLcmTests/MediaTests.cs
(1 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
(2 hunks)backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs
(5 hunks)backend/FwLite/MiniLcm.Tests/MediaTestsBase.cs
(1 hunks)backend/FwLite/MiniLcm/Media/MediaFile.cs
(1 hunks)backend/FwLite/MiniLcm/Media/ReadFileResponse.cs
(2 hunks)backend/FwLite/MiniLcm/Media/StreamUtils.cs
(1 hunks)backend/FwLite/MiniLcm/Media/UploadFileResponse.cs
(1 hunks)backend/Testing/FwHeadless/MediaFileServiceTests.cs
(4 hunks)frontend/viewer/src/lib/components/audio/AudioDialog.svelte
(4 hunks)frontend/viewer/src/lib/components/field-editors/audio-input.svelte
(4 hunks)frontend/viewer/src/lib/in-memory-api-service.ts
(2 hunks)frontend/viewer/svelte.config.js
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/viewer/svelte.config.js
- backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/ProjectLoaderFixture.cs
- backend/FwLite/MiniLcm/Media/StreamUtils.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/FwLite/MiniLcm/Media/MediaFile.cs
- backend/Testing/FwHeadless/MediaFileServiceTests.cs
- backend/FwLite/FwDataMiniLcmBridge/Media/IMediaAdapter.cs
- backend/FwHeadless/LexboxFwDataMediaAdapter.cs
- backend/FwLite/FwDataMiniLcmBridge.Tests/MediaFileTests.cs
- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
- frontend/viewer/src/lib/components/field-editors/audio-input.svelte
- frontend/viewer/src/lib/components/audio/AudioDialog.svelte
- backend/FwLite/MiniLcm/Media/UploadFileResponse.cs
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1537
File: frontend/viewer/src/SvelteUxProjectView.svelte:151-153
Timestamp: 2025-03-12T06:32:08.277Z
Learning: When reviewing PRs where files have been moved or code has been relocated from one file to another, focus the review on actual modifications to the code rather than raising issues about pre-existing code patterns that were simply relocated.
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
backend/FwLite/LcmCrdt.Tests/MiniLcmTests/MediaTests.cs (1)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs (1)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
backend/FwLite/FwDataMiniLcmBridge/LexEntryFilterMapProvider.cs (1)
Learnt from: hahn-kev
PR: #1698
File: backend/FwLite/LcmCrdt/Data/Filtering.cs:25-35
Timestamp: 2025-06-13T09:25:37.958Z
Learning: In backend/FwLite/LcmCrdt/Data/Filtering.cs FtsFilter
, the &&
combination between the FTS MATCH
result and the SearchValue
fallback is intentional to maintain existing search behavior; any future change to use ||
(or another approach) will be considered later.
frontend/viewer/src/lib/in-memory-api-service.ts (3)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:20:03.010Z
Learning: The user wants to share the 10MB file size limit constant between C# and TypeScript using Reinforced.Typings to ensure consistency and prevent hardcoding discrepancies like the current typo (1034 vs 1024).
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.367Z
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.
backend/FwLite/MiniLcm/Media/ReadFileResponse.cs (2)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs (5)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
Learnt from: hahn-kev
PR: #1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Learnt from: hahn-kev
PR: #1753
File: backend/FwHeadless/Services/SyncHostedService.cs:212-235
Timestamp: 2025-06-16T04:10:03.163Z
Learning: In the FwHeadless project, using hard-coded "crdt" strings for CRDT project names is acceptable and doesn't cause issues with project name collisions in their current architecture, though the team acknowledges that using constants instead of magic strings would be a better practice.
backend/FwLite/MiniLcm.Tests/MediaTestsBase.cs (1)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
backend/FwLite/FwDataMiniLcmBridge/Media/LocalMediaAdapter.cs (1)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/FwDataTestsKernel.cs (1)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (5)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:20:03.010Z
Learning: The user wants to share the 10MB file size limit constant between C# and TypeScript using Reinforced.Typings to ensure consistency and prevent hardcoding discrepancies like the current typo (1034 vs 1024).
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.367Z
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.
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (4)
Learnt from: rmunn
PR: #1836
File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140
Timestamp: 2025-07-22T09:19:27.272Z
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.
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Learnt from: hahn-kev
PR: #1698
File: backend/FwLite/LcmCrdt/FullTextSearch/UpdateEntrySearchTableInterceptor.cs:44-46
Timestamp: 2025-06-13T09:33:32.009Z
Learning: In this codebase (C# 11/12), the team intentionally uses the list pattern is []
to check if a collection is empty; suggestions replacing it with Count == 0
are unnecessary.
Learnt from: hahn-kev
PR: #1698
File: backend/FwLite/LcmCrdt/Data/Filtering.cs:25-35
Timestamp: 2025-06-13T09:25:37.958Z
Learning: In backend/FwLite/LcmCrdt/Data/Filtering.cs FtsFilter
, the &&
combination between the FTS MATCH
result and the SearchValue
fallback is intentional to maintain existing search behavior; any future change to use ||
(or another approach) will be considered later.
🧬 Code Graph Analysis (2)
backend/FwLite/FwDataMiniLcmBridge/LexEntryFilterMapProvider.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
Func
(820-827)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (2)
ILexEntry
(156-164)PickText
(145-149)
frontend/viewer/src/lib/in-memory-api-service.ts (2)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/ILcmFileMetadata.ts (1)
ILcmFileMetadata
(6-12)frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Media/IUploadFileResponse.ts (1)
IUploadFileResponse
(8-13)
🪛 GitHub Check: GHA integration tests / dotnet
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
[warning] 822-822:
The variable 'v' is assigned but its value is never used
🪛 GitHub Check: Build FwHeadless / publish-fw-headless
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
[warning] 822-822:
The variable 'v' is assigned but its value is never used
🪛 GitHub Check: Build API / publish-api
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
[warning] 822-822:
The variable 'v' is assigned but its value is never used
⏰ 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). (1)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (21)
backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/FwDataTestsKernel.cs (1)
14-15
: LGTM! Proper absolute path configuration for media tests.The configuration correctly sets up the projects folder for media file handling with an absolute path, and the comment clearly explains why this is needed (for media, not in-memory projects).
backend/FwLite/MiniLcm/Media/ReadFileResponse.cs (2)
3-3
: LGTM! Proper namespace organization for media types.Moving to the
MiniLcm.Media
namespace aligns with the media-focused organization of types in this PR.
5-5
: LGTM! Proper implementation of disposal pattern.The implementation correctly follows the disposal pattern:
- Both
IAsyncDisposable
andIDisposable
interfaces are implemented- Null-conditional operators safely handle cases where
Stream
is nullValueTask.CompletedTask
is the appropriate return for completed async operationsThis ensures proper resource cleanup when
ReadFileResponse
contains a stream.Also applies to: 27-35
backend/FwLite/LcmCrdt.Tests/MiniLcmTests/MediaTests.cs (1)
1-18
: LGTM! Well-structured test class for media functionality.The test class properly:
- Extends
MediaTestsBase
to inherit comprehensive media test cases- Initializes the fixture with a specific project name for media testing
- Implements proper async lifecycle management with correct disposal order
- Provides concrete implementation for the abstract base test class
This ensures the media functionality is thoroughly tested in the LcmCrdt context.
backend/FwLite/FwDataMiniLcmBridge/LexEntryFilterMapProvider.cs (1)
31-32
: LGTM! Important null safety improvement.Adding the null check for
entry.LexemeFormOA
prevents potential null reference exceptions. This aligns with the usage pattern seen elsewhere in the codebase (e.g.,FwDataMiniLcmApi.cs
line 825) whereLexemeFormOA
is accessed with null-conditional operators, confirming it can be null.frontend/viewer/src/lib/in-memory-api-service.ts (3)
28-28
: LGTM! Proper imports for media file handling types.The added imports for
ReadFileResult
,ILcmFileMetadata
,IUploadFileResponse
, andUploadFileResult
are necessary for implementing the new media file methods.Also applies to: 30-33
355-355
: LGTM! More graceful handling for unsupported operations.Returning
{result: ReadFileResult.NotSupported}
instead of throwing an error allows callers to handle this gracefully, which is appropriate for a mock/in-memory API service.
358-360
: LGTM! Consistent stub implementation for media file saving.The
saveFile
method properly follows the interface signature and returnsUploadFileResult.NotSupported
, maintaining consistency with thegetFileStream
method's approach for unsupported operations in the in-memory API.backend/FwLite/MiniLcm.Tests/MediaTestsBase.cs (1)
140-156
: Fix parameter assignment that doesn't affect caller.The assignment to
originalBytes
parameter on line 144 doesn't affect the caller's variable since it's not passed by reference. This could cause confusion if callers expect the array to be populated.-private async Task<UploadFileResponse> SaveTestFile(string fileName, byte[]? originalBytes = null, string mimeType = "application/octet-stream") +private async Task<UploadFileResponse> SaveTestFile(string fileName, byte[]? originalBytes = null, string mimeType = "application/octet-stream") { const string author = "Test Author"; var uploadDate = DateTimeOffset.UtcNow; - if (originalBytes is null) Random.Shared.NextBytes(originalBytes = new byte[1024]); + originalBytes ??= new byte[1024]; + if (originalBytes.Length == 1024 && originalBytes.All(b => b == 0)) + { + Random.Shared.NextBytes(originalBytes); + } var metadata = new LcmFileMetadata(fileName, mimeType, author, uploadDate);Or simpler:
- if (originalBytes is null) Random.Shared.NextBytes(originalBytes = new byte[1024]); + if (originalBytes is null) + { + originalBytes = new byte[1024]; + Random.Shared.NextBytes(originalBytes); + }⛔ Skipped due to learnings
Learnt from: rmunn PR: sillsdev/languageforge-lexbox#1836 File: backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs:120-140 Timestamp: 2025-07-22T09:19:27.272Z 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.
Learnt from: rmunn PR: sillsdev/languageforge-lexbox#1836 File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25 Timestamp: 2025-07-22T09:20:03.010Z Learning: The user wants to share the 10MB file size limit constant between C# and TypeScript using Reinforced.Typings to ensure consistency and prevent hardcoding discrepancies like the current typo (1034 vs 1024).
backend/FwLite/LcmCrdt/MediaServer/LcmMediaService.cs (8)
13-20
: Constructor enhancements look good!The addition of
IServerHttpClientProvider
andILogger<LcmMediaService>
follows proper dependency injection patterns and enables better connectivity management and logging throughout the service.
72-81
: Improved error handling for file downloads.Good addition of explicit error checking and descriptive exception messages. This will help with debugging download failures.
83-89
: Clean extraction of client creation logic.Good refactoring to extract the media server client creation into a reusable helper method.
90-105
: Excellent file handling improvements!The switch from
File.OpenWrite
toFile.Create
addresses the critical file truncation issue, and the addition ofEnsureUnique
prevents accidental overwrites. These changes ensure data integrity.
111-122
: Well-implemented file upload method.The async upload implementation properly handles multipart file uploads with appropriate metadata.
123-152
: Robust file saving implementation with proper error handling.Excellent implementation that:
- Prevents duplicate files by checking existence first
- Uses
File.Create
for proper file truncation- Includes comprehensive error handling with cleanup
- Only attempts remote upload when online
- Provides clear feedback about whether a new resource was created
153-168
: Simple and effective unique filename generation.Good implementation that handles filename conflicts by appending numeric suffixes while preserving file extensions.
169-174
: Clear implementation for uploading pending resources.The method correctly checks online status before attempting uploads. Note that the return value indicates whether upload was attempted (when online), not necessarily whether all uploads succeeded.
backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs (4)
2-2
: LGTM! Necessary import for media cleanup functionality.The new using directive is required for accessing
LcmMediaService
in the cleanup logic.
43-46
: Good backward compatibility approach.The delegation to the new overloaded method maintains compatibility for existing tests while enabling customizable project names.
48-48
: Well-implemented parameterization for flexible testing.The new overloaded method correctly uses the
projectName
parameter in bothCrdtProject
andProjectData
initialization, replacing hardcoded values and enabling more flexible test scenarios.Also applies to: 60-60, 73-73
131-132
: Proper test isolation through media cache cleanup.The addition of media cache directory cleanup ensures test isolation and prevents leftover files from affecting subsequent tests. The existence check and recursive deletion are appropriate.
Important
recording/uploading audio is still hidden behind the dev flag until we handle file conversion.
Implements: