Begin modeling pictures in MiniLcm#2300
Conversation
📝 WalkthroughWalkthroughAdds a ChangesPicture Support on Senses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
github-actions
Bot
added
the
📦 Lexbox
C# Unit Tests165 tests 165 ✅ 21s ⏱️ Results for commit 0891da0. ♻️ This comment has been updated with latest results. |
|
@CodeRabbit - Please examine how MediaUri is handled in audio files, then suggest how to handle the MediaUri property in the Picture class. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Search for MediaUri in the codebase ==="
rg -n "MediaUri" --type cs -C 3
echo ""
echo "=== Search for audio-related model files ==="
fd -e cs -i "audio" --exec echo {}
echo ""
echo "=== Search for Picture model files ==="
fd -e cs -i "picture" --exec echo {}
echo ""
echo "=== Search for ExampleSentence or audio model definitions ==="
rg -n "class.*Audio\|AudioFile\|Sound" --type cs -C 5Length of output: 45536 🏁 Script executed: #!/bin/bash
echo "=== Find MiniLcm model files ==="
fd -e cs . --search-path "MiniLcm" | head -60
echo ""
echo "=== Search for all MediaUri occurrences ==="
rg -rn "MediaUri" -C 5Length of output: 57201 🏁 Script executed: #!/bin/bash
echo "=== Search for PictureFileRA (LCM picture file property) ==="
rg -n "PictureFileRA\|InternalPath\|PictureFolder\|Pictures folder\|TypeToLinkedFolder" --type cs -C 3
echo ""
echo "=== Look at UpdatePictureProxy in full ==="
cat backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdatePictureProxy.cs
echo ""
echo "=== Look at CreatePictureChange in full ==="
cat backend/FwLite/LcmCrdt/Changes/CreatePictureChange.cs
echo ""
echo "=== Look at PictureSync in full ==="
cat backend/FwLite/MiniLcm/SyncHelpers/PictureSync.cs
echo ""
echo "=== Search for TypeToLinkedFolder and AudioVisualFolder constant ==="
rg -n "TypeToLinkedFolder\|AudioVisualFolder\|PictureFolder\|Pictures\b" --type cs -C 2Length of output: 5543 🏁 Script executed: #!/bin/bash
echo "=== Find AudioVisualFolder and TypeToLinkedFolder in FwDataMiniLcmApi.cs ==="
grep -n "AudioVisual\|TypeToLinked\|PictureFile\|InternalPath\|Pictures\|LinkedFiles\|CmFile\|cmFile" \
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs | head -60
echo ""
echo "=== View lines around the folder/type mapping function ==="
grep -n "TypeToLinkedFolder\|AudioVisualFolder\|const string\|private.*Folder" \
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
echo ""
echo "=== Grep for picture file path references in FW LCM ==="
rg -n "PictureFile\|CmFile\|InternalPath\|\.PictureFilePath\|picture\..*[Ff]ile\|\.ImageFile" \
backend/FwLite/FwDataMiniLcmBridge/ -C 2
echo ""
echo "=== Check what LCM picture interface looks like ==="
rg -rn "PictureFileRA\|ICmPicture\|CmPicture" --type cs -C 3 | head -80Length of output: 3346 🏁 Script executed: #!/bin/bash
echo "=== LcmToPicture method context ==="
grep -n "LcmToPicture\|private.*Picture\|FromLcmPicture\|ToMiniLcmPicture\|ICmPicture" \
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
echo ""
echo "=== View lines 795-830 of FwDataMiniLcmApi.cs (picture mapping) ==="
sed -n '795,835p' backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
echo ""
echo "=== View lines 1940-1960 (TypeToLinkedFolder) ==="
sed -n '1940,1960p' backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
echo ""
echo "=== Search for ICmPicture file-path related properties in LCM NuGet/dlls ==="
rg -rn "\.PictureFile\|picture\.\|ICmPicture\b" --type cs backend/FwLite/ -C 2 | head -60
echo ""
echo "=== Check the SIL LCModel ICmPicture or CmPicture in dlls/packages ==="
find . -name "*.cs" | xargs grep -l "ICmPicture" 2>/dev/null | head -10
echo ""
echo "=== View LfClassicMiniLcmApi picture mapping (line ~350) ==="
grep -n "picture\.\|Picture\|MediaUri\|CmFile\|InternalPath" backend/LfClassicData/LfClassicMiniLcmApi.cs | head -30Length of output: 4911 🏁 Script executed: #!/bin/bash
echo "=== Search for ICmFile usage anywhere ==="
rg -rn "ICmFile\|PictureFileRA\|InternalPath\|\.PicturePath\b" --type cs -C 3 | head -60
echo ""
echo "=== Look for how FW code sets picture file path - CreatePicture context ==="
sed -n '1810,1850p' backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
echo ""
echo "=== Search for Pictures folder const or PictureFolder ==="
rg -rn "const.*[Pp]icture\|PictureFolder\b" --type cs -C 2
echo ""
echo "=== Look for nuget packages that contain LCModel ==="
find . -name "*.csproj" | xargs grep -l "LCModel\|liblcm\|SIL.LCModel" 2>/dev/null | head -5
echo ""
echo "=== Check any existing usages that access picture.PictureFile ==="
rg -rn "picture\." --type cs backend/FwLite/FwDataMiniLcmBridge/ -C 1 | head -40Length of output: 5205
How
|
| Step | Audio | Picture |
|---|---|---|
| FW storage | MultiString[audioWs] = relative filename |
ICmPicture.PictureFileRA.InternalPath = relative path |
| Subfolder | LinkedFilesRootDir/AudioVisual/ |
LinkedFilesRootDir/Pictures/ |
| Read (FW→MiniLcm) | mediaAdapter.MediaUriFromPath(fullPath) |
same |
| Write (MiniLcm→FW) | mediaAdapter.PathFromMediaUri() → update string |
same, but find/create ICmFile, set PictureFileRA |
| NotFound guard | UpdateDictionaryProxy.ShouldSet skips write |
SetLcmPictureFile returns early if MediaUri.NotFound |
|
Current code is partly working, but I haven't yet told Entity Framework how to map a Picture object into the DB. Which results in this exception when I run a sync: System.InvalidOperationException: The property 'Picture.MediaUri' could not be mapped because it is of type 'MediaUri', which is not a supported primitive type or a valid entity type. Either explicitly map this property, or ignore it using the '[NotMapped]' attribute or by using 'EntityTypeBuilder.Ignore' in 'OnModelCreating'.
at Microsoft.EntityFrameworkCore.Infrastructure.ModelValidator.ValidatePropertyMapping(IConventionTypeBase structuralType, IConventionModel model, IDiagnosticsLogger`1 logger)
at Microsoft.EntityFrameworkCore.Infrastructure.ModelValidator.ValidatePropertyMapping(IModel model, IDiagnosticsLogger`1 logger)
at Microsoft.EntityFrameworkCore.Infrastructure.ModelValidator.Validate(IModel model, IDiagnosticsLogger`1 logger)
at Microsoft.EntityFrameworkCore.Infrastructure.RelationalModelValidator.Validate(IModel model, IDiagnosticsLogger`1 logger)
at Microsoft.EntityFrameworkCore.Sqlite.Infrastructure.Internal.SqliteModelValidator.Validate(IModel model, IDiagnosticsLogger`1 logger)
at Microsoft.EntityFrameworkCore.Infrastructure.ModelRuntimeInitializer.Initialize(IModel model, Boolean designTime, IDiagnosticsLogger`1 validationLogger)
at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.CreateModel(DbContext context, ModelCreationDependencies modelCreationDependencies, Boolean designTime)
at Microsoft.EntityFrameworkCore.Infrastructure.ModelSource.GetModel(DbContext context, ModelCreationDependencies modelCreationDependencies, Boolean designTime)
at Microsoft.EntityFrameworkCore.Internal.DbContextServices.CreateModel(Boolean designTime)
at Microsoft.EntityFrameworkCore.Internal.DbContextServices.get_Model()
at Microsoft.EntityFrameworkCore.Infrastructure.EntityFrameworkServicesBuilder.<>c.<TryAddCoreServices>b__8_4(IServiceProvider p)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSiteMain(ServiceCallSite callSite, TArgument argument)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine.<>c__DisplayClass2_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
at Microsoft.EntityFrameworkCore.Infrastructure.EntityFrameworkRelationalServicesBuilder.<>c.<TryAddCoreServices>b__3_3(IServiceProvider p)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.DynamicServiceProviderEngine.<>c__DisplayClass2_0.<RealizeService>b__0(ServiceProviderEngineScope scope)
at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
at Microsoft.EntityFrameworkCore.Infrastructure.Internal.InfrastructureExtensions.GetService(IInfrastructure`1 accessor, Type serviceType)
at Microsoft.EntityFrameworkCore.Infrastructure.Internal.InfrastructureExtensions.GetService[TService](IInfrastructure`1 accessor)
at Microsoft.EntityFrameworkCore.Infrastructure.AccessorExtensions.GetService[TService](IInfrastructure`1 accessor)
at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.get_Dependencies()
at Microsoft.EntityFrameworkCore.Infrastructure.DatabaseFacade.Microsoft.EntityFrameworkCore.Storage.IDatabaseFacadeDependenciesAccessor.get_Dependencies()
at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.GetFacadeDependencies(DatabaseFacade databaseFacade)
at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.CloseConnectionAsync(DatabaseFacade databaseFacade)
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in /src/backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 181
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in /src/backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 194
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in /src/backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 194
at FwHeadless.Services.SyncWorker.SetupCrdtProject(String crdtFile, IProjectLookupService projectLookupService, Guid projectId, CrdtProjectsService projectsService, String projectFolder, Guid fwProjectId, String lexboxUrl) in /src/backend/FwHeadless/Services/SyncHostedService.cs:line 371
at FwHeadless.Services.SyncWorker.ExecuteSync(CancellationToken stoppingToken, Boolean onlyHarmony) in /src/backend/FwHeadless/Services/SyncHostedService.cs:line 188
at FwHeadless.Services.SyncWorker.ExecuteSync(CancellationToken stoppingToken, Boolean onlyHarmony) in /src/backend/FwHeadless/Services/SyncHostedService.cs:line 276
at FwHeadless.Services.SyncHostedService.ExecuteAsync(CancellationToken stoppingToken) in /src/backend/FwHeadless/Services/SyncHostedService.cs:line 33 |
|
@CodeRabbit - Look at the exception in #2300 (comment) and look at the Entity Framework Core documentation. Summarize how to map the MediaUri property, and provide links to the documentation explaining how to map properties in EF Core. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Find MediaUri type definition ==="
rg -n "class MediaUri\|struct MediaUri\|record MediaUri" --type cs -C 5
echo ""
echo "=== Find existing EF Core value converters in LcmCrdt ==="
rg -n "ValueConverter\|HasConversion\|OnModelCreating\|EntityTypeBuilder\|modelBuilder" --type cs backend/FwLite/LcmCrdt/ -C 3 | head -100
echo ""
echo "=== Find DbContext in LcmCrdt ==="
fd -e cs . backend/FwLite/LcmCrdt/ | xargs grep -l "DbContext\|OnModelCreating" 2>/dev/null
echo ""
echo "=== Check if MediaUri implements any interface or has implicit conversion ==="
rg -n "implicit\|explicit\|operator\|ToString\|Parse\|TryParse" --type cs -l | xargs grep -l "MediaUri" 2>/dev/nullLength of output: 3199 🌐 Web query:
Length of output: 20413 🏁 Script executed: #!/bin/bash
echo "=== MediaUri definition - search all file types ==="
rg -rn "MediaUri" --type cs | grep -i "class\|record\|struct" | head -20
echo ""
echo "=== Find the file that defines MediaUri ==="
rg -rn "MediaUri" --type cs -l
echo ""
echo "=== Read LcmCrdtDbContext.cs ==="
cat backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs
echo ""
echo "=== Read MiniLcm MediaUri source file ==="
fd -e cs "MediaUri" | head -5Length of output: 6757 🏁 Script executed: #!/bin/bash
echo "=== Read MediaUri.cs ==="
cat backend/FwLite/MiniLcm/Media/MediaUri.csLength of output: 1864
The fixAdd a converter class and register it in // In LcmCrdtDbContext.ConfigureConventions:
protected override void ConfigureConventions(ModelConfigurationBuilder builder)
{
builder.Properties<MultiString>()
.HaveColumnType("jsonb")
.HaveConversion<MultiStringDbConverter>();
builder.Properties<RichString?>()
.HaveColumnType("jsonb")
.HaveConversion<RichStringDbConverter>();
builder.Properties<RichMultiString>()
.HaveColumnType("jsonb")
.HaveConversion<RichMultiStringDbConverter>();
builder.Properties<WritingSystemId>()
.HaveConversion<WritingSystemIdConverter>();
// Add this:
builder.Properties<MediaUri>()
.HaveConversion<MediaUriConverter>();
}
// Add this converter class alongside the others:
private class MediaUriConverter() : ValueConverter<MediaUri, string>(
uri => uri.ToString(),
str => new MediaUri(str));
Relevant EF Core documentation
After adding the converter, you'll also need to create a new EF migration ( |
|
@CodeRabbit - When I run Please analyze the port-forwarding setup for the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Find Taskfile for fw-lite-web ==="
fd -i "Taskfile" | head -20
echo ""
echo "=== Search for fw-lite-web in Taskfiles ==="
rg -n "fw-lite-web\|fw_lite_web" --type yaml -C 5
echo ""
echo "=== Search for port 3050 references ==="
rg -rn "3050" -C 3 | head -60
echo ""
echo "=== Search for port forwarding or devtunnel setup ==="
rg -rn "devtunnel\|port.forward\|portforward\|forward.*3050\|3050.*forward" -C 5 | head -60Length of output: 28033 🏁 Script executed: #!/bin/bash
echo "=== Search for OIDC/auth configuration with localhost ==="
rg -rn "localhost:3050\|localhost.*3050\|3050.*localhost" -C 3 | head -80
echo ""
echo "=== Search for OidcAuthority or Authority config ==="
rg -rn "Authority\|OidcAuthority\|OpenIdConnect\|oidc" --type cs -C 3 | head -80
echo ""
echo "=== Search for auth login endpoint ==="
rg -rn "api/auth/login\|AuthController\|login.*controller" --type cs -C 5 | head -60Length of output: 637 🏁 Script executed: #!/bin/bash
echo "=== Root Taskfile.yml - search for fw-lite-web or related ==="
grep -n "fw-lite-web\|fw_lite_web\|fw-lite\|fwlite\|5137\|3050\|https-proxy\|devtunnel\|forward" Taskfile.yml | head -60
echo ""
echo "=== backend/FwLite/Taskfile.yml - full content ==="
cat backend/FwLite/Taskfile.yml
echo ""
echo "=== frontend/https-proxy/vite.config.ts - full content ==="
cat frontend/https-proxy/vite.config.ts
echo ""
echo "=== backend/Taskfile.yml - search for fw-lite or 3050 or 5137 ==="
grep -n "fw-lite\|3050\|5137\|https-proxy\|devtunnel\|forward" backend/Taskfile.yml | head -40Length of output: 10948 🏁 Script executed: #!/bin/bash
echo "=== Search for 5137 in all files ==="
rg -rn "5137" -C 2 | head -60
echo ""
echo "=== Search for lexbox server URL config in appsettings or launchSettings ==="
fd -e json "appsettings\|launchSettings" backend/FwLite/ | head -20
fd -e json "appsettings\|launchSettings" backend/FwLite/ | xargs grep -l "localhost\|lexbox\|3050\|Authority\|oidc\|auth" 2>/dev/null | head -10
echo ""
echo "=== Search for auth login in FwLite web project ==="
rg -rn "login\|auth\|oidc\|Authority" backend/FwLite/FwLiteWeb/ -C 3 | head -80
echo ""
echo "=== Find FwLiteWeb project structure ==="
fd . backend/FwLite/FwLiteWeb/ | head -40Length of output: 14226 🏁 Script executed: #!/bin/bash
echo "=== AuthRoutes.cs ==="
cat backend/FwLite/FwLiteWeb/Routes/AuthRoutes.cs
echo ""
echo "=== Program.cs ==="
cat backend/FwLite/FwLiteWeb/Program.cs
echo ""
echo "=== appsettings.json files in FwLiteWeb ==="
fd -e json "appsettings" backend/FwLite/FwLiteWeb/ | xargs -I{} sh -c 'echo "--- {} ---"; cat {}'
echo ""
echo "=== frontend Taskfile.yml - fw-lite-infr task ==="
grep -n "fw-lite-infr\|https-proxy\|3050\|lexbox" frontend/Taskfile.yml -A 10 | head -60
echo ""
echo "=== FwLiteWebConfig.cs ==="
cat backend/FwLite/FwLiteWeb/FwLiteWebConfig.cs
echo ""
echo "=== ServerRedirectUrlProvider.cs ==="
cat backend/FwLite/FwLiteWeb/Services/ServerRedirectUrlProvider.csLength of output: 9618 🏁 Script executed: #!/bin/bash
echo "=== Find AuthService and AuthConfig ==="
fd -e cs "AuthService\|AuthConfig\|OAuthService" backend/FwLite/ | head -10
echo ""
echo "=== AuthConfig.cs ==="
fd -e cs "AuthConfig" backend/FwLite/ | xargs cat
echo ""
echo "=== AuthService.cs ==="
fd -e cs "AuthService" backend/FwLite/ | xargs cat
echo ""
echo "=== OAuthService.cs ==="
fd -e cs "OAuthService" backend/FwLite/ | xargs cat
echo ""
echo "=== UrlContext.cs ==="
cat backend/FwLite/FwLiteWeb/UrlContext.cs
echo ""
echo "=== Search for ValidateAuthority or ValidIssuers or MSAL config ==="
rg -rn "ValidateAuthority\|ValidIssuers\|PublicClientApplicationBuilder\|WithAuthority\|OidcWellKnown\|openid-configuration" --type cs -C 5 | head -80
echo ""
echo "=== Root Taskfile lines 113-120 (fw-lite-web task definition) ==="
sed -n '100,130p' Taskfile.ymlLength of output: 22755 🏁 Script executed: #!/bin/bash
echo "=== Find OAuthClientFactory and OAuthClient ==="
fd -e cs "OAuthClient" backend/FwLite/ | head -10
echo ""
echo "=== OAuthClientFactory.cs ==="
fd -e cs "OAuthClientFactory" backend/FwLite/ | xargs cat
echo ""
echo "=== OAuthClient.cs ==="
fd -e cs -x echo {} backend/FwLite/ | xargs grep -l "OAuthClient" | grep -v "Factory\|Service\|Test" | head -5 | xargs cat
echo ""
echo "=== Search for PublicClientApplication or WithAuthority in FwLiteShared ==="
rg -rn "PublicClientApplication\|WithAuthority\|BuildApplication\|IPublicClientApplication" --type cs backend/FwLite/ -C 5 | head -80
echo ""
echo "=== Search for ValidateIssuer or authority in appsettings for the local lexbox ==="
fd -e json "appsettings" backend/ | xargs grep -l "localhost\|3050\|3000\|authority\|Authority" 2>/dev/null | head -5
echo ""
echo "=== Check if local lexbox server config has ASPNETCORE_FORWARDEDHEADERS ==="
fd -i "docker-compose" . | xargs grep -l "lexbox\|3000\|3050" 2>/dev/null | head -5
fd "docker-compose" . | head -10Length of output: 60547
Root CauseThe data flow is: The chain of events that causes the mismatch:
Fix OptionsOption A — Fix in the vite proxy (self-contained, no server changes needed)Intercept the OIDC discovery response and rewrite import { gunzipSync } from 'zlib';
// Inside the lexboxServer ProxyOptions configure callback, after existing code:
proxy.on('proxyRes', (proxyRes, req, res) => {
if (!req.url?.includes('openid-configuration')) return;
const chunks: Buffer[] = [];
const originalWrite = res.write.bind(res);
const originalEnd = res.end.bind(res);
// Intercept the streamed response
proxyRes.on('data', (chunk: Buffer) => chunks.push(chunk));
proxyRes.on('end', () => {
const body = Buffer.concat(chunks).toString('utf-8');
const rewritten = body.replaceAll(
/"issuer"\s*:\s*"http:\/\//g,
'"issuer":"https://'
);
const newBody = Buffer.from(rewritten, 'utf-8');
res.setHeader('Content-Length', newBody.length);
originalWrite(newBody);
originalEnd();
});
});
A simpler alternative using vite's built-in transformer hook (if supported by your vite version): // In vite.config.ts server.proxy options:
'/': {
...lexboxServer,
// Add a response rewriter for OIDC discovery only
rewrite: (path) => path,
}Option B — Fix in the local lexbox server (correct architectural fix)Enable environment:
ASPNETCORE_FORWARDEDHEADERS_ENABLED: "true"This makes ASP.NET Core automatically enable forwarded headers, so the lexbox server generates Or in {
"ForwardedHeaders": {
"ForwardedHeaders": "XForwardedFor, XForwardedProto"
}
}Option C — Fix in
|
C# FwHeadless Unit Tests48 tests 48 ✅ 15s ⏱️ Results for commit 0891da0. ♻️ This comment has been updated with latest results. |
|
@CodeRabbit - Syncing pictures using commit 3a729a7 produced the following exception: System.NotSupportedException: type JsonPatchChange`1 does not support NewEntity, because it inherits from EditChange, this means it must be called with a from an existing entity, not a newly generated one
at SIL.Harmony.Changes.EditChange`1.NewEntity(Commit commit, IChangeContext context) in backend/harmony/src/SIL.Harmony/Changes/EditChange.cs:line 8
at SIL.Harmony.Changes.Change`1.SIL.Harmony.Changes.IChange.NewEntity(Commit commit, IChangeContext context) in backend/harmony/src/SIL.Harmony/Changes/Change.cs:line 51
at SIL.Harmony.SnapshotWorker.ApplyCommitChanges(SortedSet`1 commits) in backend/harmony/src/SIL.Harmony/SnapshotWorker.cs:line 78
at SIL.Harmony.SnapshotWorker.UpdateSnapshots(SortedSet`1 commits) in backend/harmony/src/SIL.Harmony/SnapshotWorker.cs:line 54
at SIL.Harmony.DataModel.UpdateSnapshots(CrdtRepository repo, SortedSet`1 commitsToApply) in backend/harmony/src/SIL.Harmony/DataModel.cs:line 217
at SIL.Harmony.DataModel.SIL.Harmony.ISyncable.AddRangeFromSync(IEnumerable`1 commits) in backend/harmony/src/SIL.Harmony/DataModel.cs:line 159
at SIL.Harmony.DataModel.SIL.Harmony.ISyncable.AddRangeFromSync(IEnumerable`1 commits) in backend/harmony/src/SIL.Harmony/DataModel.cs:line 161
at SIL.Harmony.DataModel.SIL.Harmony.ISyncable.AddRangeFromSync(IEnumerable`1 commits) in backend/harmony/src/SIL.Harmony/DataModel.cs:line 161
at SIL.Harmony.SyncHelper.SyncWith(ISyncable localModel, ISyncable remoteModel, JsonSerializerOptions serializerOptions) in backend/harmony/src/SIL.Harmony/SyncHelper.cs:line 42
at SIL.Harmony.DataModel.SyncWith(ISyncable remoteModel) in backend/harmony/src/SIL.Harmony/DataModel.cs:line 377
at FwLiteShared.Sync.SyncService.ExecuteSync(Boolean skipNotifications) in backend/FwLite/FwLiteShared/Sync/SyncService.cs:line 89
at FwLiteShared.Projects.CombinedProjectsService.<>c.<<DownloadProject>b__16_1>d.MoveNext() in backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs:line 202
--- End of stack trace from previous location ---
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 175
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 191
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 194
at LcmCrdt.CrdtProjectsService.CreateProject(CreateProjectRequest request) in backend/FwLite/LcmCrdt/CrdtProjectsService.cs:line 194
at FwLiteShared.Projects.CombinedProjectsService.<>c__DisplayClass16_0.<<DownloadProject>b__0>d.MoveNext() in backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs:line 196
--- End of stack trace from previous location ---
at FwLiteShared.Projects.CombinedProjectsService.DownloadProject(ProjectModel project) in backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs:line 196If possible, analyse why NewEntity is being called on a |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PictureSync.cs ==="
fd -e cs "PictureSync" | xargs cat
echo ""
echo "=== CreatePictureChange.cs ==="
fd -e cs "CreatePictureChange" | xargs cat
echo ""
echo "=== Search for where pictures are synced from FW to CRDT ==="
rg -n "CreatePicture\|AddPicture\|picture.*sync\|sync.*picture" --type cs -i backend/FwLite/ -C 3 | head -80
echo ""
echo "=== How other entity syncs look - e.g. EntrySync ==="
fd -e cs "EntrySync" backend/FwLite/ | xargs cat 2>/dev/null | head -100
echo ""
echo "=== How other entity syncs look - e.g. SenseSync ==="
fd -e cs "SenseSync" backend/FwLite/ | xargs cat 2>/dev/null | head -100Length of output: 10820 🏁 Script executed: #!/bin/bash
echo "=== Find CrdtMiniLcmApi picture-related methods ==="
fd -e cs "CrdtMiniLcmApi" backend/FwLite/ | xargs grep -n "Picture\|picture" | head -60
echo ""
echo "=== Find CreatePicture and UpdatePicture implementations in LcmCrdt ==="
rg -n "CreatePicture\|UpdatePicture\|DeletePicture\|MovePicture" --type cs backend/FwLite/LcmCrdt/ -C 5
echo ""
echo "=== Find how JsonPatchChange is created/used ==="
rg -n "JsonPatchChange\b" --type cs backend/FwLite/ -C 3
echo ""
echo "=== CrdtFwdataProjectSyncService - picture related sync code ==="
rg -n "Picture\|picture" --type cs backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs -C 3
echo ""
echo "=== Check UpdatePicture in LcmCrdt and what change type it creates ==="
rg -n "async.*UpdatePicture\|Task.*UpdatePicture" --type cs backend/FwLite/ -C 10Length of output: 14639
Root cause
// Overload 1 — takes an UpdateObjectInput, creates JsonPatchChange<Picture>
public async Task<Picture> UpdatePicture(Guid entryId, Guid senseId, Guid pictureId, UpdateObjectInput<Picture> update)
{
var patchChange = new JsonPatchChange<Picture>(pictureId, jsonPatch); // ← EditChange, no NewEntity support
await AddChange(patchChange);
...
}
// Overload 2 — takes before/after Pictures, calls PictureSync.Sync (single-picture version)
public async Task<Picture> UpdatePicture(Guid entryId, Guid senseId, Picture before, Picture after, ...)
{
await PictureSync.Sync(entryId, senseId, before, after, api ?? this); // ← ONLY calls UpdatePicture, NEVER CreatePicture
...
}The single-picture The sync code on the FW headless server is taking a path that calls the "before/after Picture" Where to lookThe FW headless sync must have new code (in this PR) in either // CORRECT — uses DiffOrderable, calls CreatePicture for new pictures
await PictureSync.Sync(entryId, senseId, beforeSense.Pictures, afterSense.Pictures, api);…it's going through something like: // WRONG — single-picture overload, always emits JsonPatchChange, never CreatePicture
foreach (var (before, after) in matchedPictures)
await api.UpdatePicture(entryId, senseId, before, after);
// or
await PictureSync.Sync(entryId, senseId, beforePicture, afterPicture, api);Suggested fixOption A — In // In SenseSync.Sync, alongside the ExampleSentenceSync call:
changes += await PictureSync.Sync(entryId,
beforeSense.Id,
beforeSense.Pictures,
afterSense.Pictures,
api);Option B — If the FW sync has a custom loop in The key invariant to enforce: 🧠 Learnings used |
Compiles, but some logic (most notably MediaUri handling) is incomplete
Next up will be writing tests to make sure we can properly two-way sync a picture from FW Classic, i.e. that the MediaUri handling is correct.
|
Rebased on top of current |
|
Huh. Look commit 2ce7ec8 and, specifically, at the DB migration. Why did it add the HomographNumber property? I might have had a local DB that's not up-to-date. I might have to delete that migration and create a new one. |
|
Force-pushed to get rid of commit 2ce7ec8 so the migration won't add a duplicate HomographNumber column. |
MiniLcmApiWriteNormalizationWrapper is new on develop branch since I started this PR, so I didn't have the Picture CRUD API calls for it.
Now that `pictures` is a required property of ISense, the demo data shape needs to change to include it as well. No actual pictures in the demo yet, that can come later.
Since Picture is no longer a Harmony top-level type (i.e. one that's stored in its own DB table), CreateChange and DeleteChange are no longer applicable. We had changed CreateChange but missed DeleteChange earlier. Also renamed the CreatePictureChange file to match the class name.
Two more places needed to be updated so ISense will typecheck.
|
@CodeRabbit fullreview |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
backend/FwLite/MiniLcm/Models/Picture.cs (1)
5-7: ⚡ Quick winOutdated TODO comment.
The TODO at lines 5-6 asks whether to implement
IOrderable, but line 7 already implements it. The comment should be removed or updated to reflect the current implementation status.🧹 Suggested cleanup
-// TODO: Determine whether FW allows users to add multiple pictures to a single sense, and whether users can then reorder pictures -// If so, implement IOrderable public class Picture : IObjectWithId<Picture>, IOrderable🤖 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/MiniLcm/Models/Picture.cs` around lines 5 - 7, The TODO comment in the Picture class is outdated. The comment questions whether to implement IOrderable, but the very next line shows the Picture class already implements IOrderable. Remove the TODO comment entirely since the implementation decision has already been made and is reflected in the class definition.backend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.cs (1)
12-20: 💤 Low valueOrder assignment is immediately overwritten in ApplyChange.
Line 16 assigns
Order = picture.Order, but line 35 inApplyChangerecomputes it viaOrderPicker.PickOrder(entity.Pictures, Between). The constructor assignment is misleading since the value is never used—ApplyChange always determines the final order.Consider removing line 16 or adding a comment explaining that the initial order is overwritten during application.
🤖 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/Changes/CreateSensePictureChange.cs` around lines 12 - 20, The Order property is being assigned from picture.Order in the CreateSensePictureChange constructor on line 16, but this value is immediately overwritten in the ApplyChange method when OrderPicker.PickOrder is called. This makes the constructor assignment misleading and unnecessary. Either remove the Order assignment from the constructor, or add a comment explaining that the initial order value is only temporary and will be recalculated during ApplyChange when the actual order positioning is determined.
🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 1833-1837: The GetPicture method is missing ownership validation
for the entryId and senseId parameters. Currently it retrieves a picture by
global ID without verifying that the picture actually belongs to the specified
entry and sense. Add validation after retrieving the picture via
PictureRepository.TryGetObject to confirm the lcmPicture belongs to the provided
entryId and senseId before calling FromLcmPicture, similar to how
update/move/delete operations enforce these boundary checks. Apply the same
ownership validation to the related picture creation path in the method around
lines 1857-1868 where only the sense existence is checked.
In `@backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.cs`:
- Around line 52-56: The Pictures property getter and setter in the
UpdateSenseProxy class both throw NotImplementedException, which causes runtime
crashes when update operations attempt to access this property. Instead of
throwing NotImplementedException for both the get and set accessors of the
Pictures property, implement them to either support the update operation
properly or throw a more meaningful exception that cleanly rejects the update
with proper context about why pictures cannot be updated through this proxy.
In `@backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs`:
- Around line 250-251: The removePictureChange variable is created but never
emitted in the yield statement, and instead setPictureOrderChange is being
yielded a second time. Change the yield statement for ChangeWithDependencies to
use removePictureChange instead of the duplicate setPictureOrderChange to ensure
the RemoveSensePictureChange is properly tested and avoid potential
duplicate-key failures when the changes are collected into a dictionary.
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 879-888: The UpdatePicture method incorrectly uses
JsonPatchChange<Picture> which assumes Picture is a standalone CRDT entity, but
Picture is actually a JSON-stored value within the Sense entity. Create a new
UpdateSensePictureChange class that extends EditChange<Sense> (following the
pattern of CreateSensePictureChange, RemoveSensePictureChange, and
ReorderSensePictureChange). This new class should accept the picture ID, sense
ID, and JsonPatch operations, then in its ApplyChange method locate the picture
in sense.Pictures by ID and apply the patch operations to it. Replace the
JsonPatchChange<Picture> instantiation in the UpdatePicture method with an
instance of this new UpdateSensePictureChange class.
- Around line 862-870: The CreatePicture method returns the input picture
parameter directly, but CreateSensePictureChange may generate a new ID when
picture.Id is Guid.Empty and computes the Order value from the Between position
during ApplyChange, resulting in stale values being returned to the caller.
Follow the pattern established in CreateExampleSentence by fetching the picture
from the repository after adding the change via AddChange to ensure the returned
picture contains the correct Id and Order values that were computed during the
change application. You may need to capture the ID generated by
CreateSensePictureChange and use it to retrieve the picture from the repository.
In `@backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs`:
- Around line 106-108: The PictureListDbConverter class deserializes JSON
strings but the migration sets a default value of empty string, which is invalid
JSON and will throw a JsonException. The null-coalescing operator only handles
null returns, not exceptions. Add an explicit check for empty or null strings in
the deserialization lambda of the ValueConverter (the second parameter that
currently has the JsonSerializer.Deserialize call) before attempting
deserialization, similar to the pattern used in LcmCrdtKernel.cs, or
alternatively change the migration default value in file
20260617073518_PicturesStoredAsJsonOnSenses.cs from empty string to the valid
JSON array "[]".
In
`@backend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.cs`:
- Around line 13-18: The AddColumn call for the Pictures column in the migration
has an invalid default value of an empty string which is not valid JSON and will
cause JsonSerializer.Deserialize to fail when PictureListDbConverter attempts to
process existing rows. Change the defaultValue parameter from an empty string to
a valid empty JSON array representation to ensure the deserializer receives
syntactically correct JSON.
In `@backend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.cs`:
- Around line 445-451: The NormalizePicture method at line 445 is not being
called within the NormalizeSense method, which is the primary entry point for
normalizing sense data. To fix this, integrate NormalizePicture into the
NormalizeSense method by ensuring that any Picture objects in the Sense.Pictures
collection are normalized using the NormalizePicture helper. This will ensure
picture captions are properly normalized when senses are written through the
normalization flow.
- Around line 458-461: The UpdatePicture method currently uses NormalizePatch
which normalizes all string fields including mediaUri, which conflicts with the
design pattern established in NormalizePicture that explicitly avoids mutating
media identifiers. Instead of using NormalizePatch, apply a selective
normalization approach that excludes the mediaUri field from normalization,
similar to how NormalizePicture handles Picture objects to preserve media
identifiers.
In `@backend/LfClassicData/LfClassicMiniLcmApi.cs`:
- Around line 450-459: The GetPicture method is not discoverable because the
sense projection in this adapter does not expose the Sense.Pictures collection,
preventing clients from enumerating picture IDs through normal sense read flows.
Locate the sense projection or query mapping configuration used when fetching
senses, and ensure that the Sense.Pictures collection is included in the
projection so clients can access picture IDs and subsequently call the
GetPicture method with valid picture identifiers.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.cs`:
- Around line 12-20: The Order property is being assigned from picture.Order in
the CreateSensePictureChange constructor on line 16, but this value is
immediately overwritten in the ApplyChange method when OrderPicker.PickOrder is
called. This makes the constructor assignment misleading and unnecessary. Either
remove the Order assignment from the constructor, or add a comment explaining
that the initial order value is only temporary and will be recalculated during
ApplyChange when the actual order positioning is determined.
In `@backend/FwLite/MiniLcm/Models/Picture.cs`:
- Around line 5-7: The TODO comment in the Picture class is outdated. The
comment questions whether to implement IOrderable, but the very next line shows
the Picture class already implements IOrderable. Remove the TODO comment
entirely since the implementation decision has already been made and is
reflected in the class definition.
🪄 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: 1a0eaee0-11d3-4424-856a-85b1f6c8e0b9
📒 Files selected for processing (31)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdatePictureProxy.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.csbackend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt/Changes/CreateSenseChange.csbackend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/RemoveSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/ReorderSensePictureChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/LcmCrdt/OrderPicker.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Models/Picture.csbackend/FwLite/MiniLcm/Models/Sense.csbackend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.csbackend/FwLite/MiniLcm/SyncHelpers/PictureSync.csbackend/FwLite/MiniLcm/SyncHelpers/SenseSync.csbackend/LfClassicData/Entities/Entry.csbackend/LfClassicData/LfClassicMiniLcmApi.csfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ISense.tsfrontend/viewer/src/lib/sandbox/Sandbox.sveltefrontend/viewer/src/lib/utils.tsfrontend/viewer/src/project/demo/demo-entry-data.ts
| public override List<Picture> Pictures | ||
| { | ||
| get => throw new NotImplementedException(); | ||
| set => throw new NotImplementedException(); | ||
| } |
There was a problem hiding this comment.
Pictures proxy accessors hard-fail on update paths.
Line 54 and Line 55 throw NotImplementedException, so any UpdateSense patch touching /pictures will crash at runtime instead of being handled or rejected cleanly.
🤖 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/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.cs`
around lines 52 - 56, The Pictures property getter and setter in the
UpdateSenseProxy class both throw NotImplementedException, which causes runtime
crashes when update operations attempt to access this property. Instead of
throwing NotImplementedException for both the get and set accessors of the
Pictures property, implement them to either support the update operation
properly or throw a more meaningful exception that cleanly rejects the update
with proper context about why pictures cannot be updated through this proxy.
Since pictures aren't a top-level Harmony object, they need a dedicated change type in order to update them; can't just use `JsonPatch<Picture>` because Harmony will look for the picture via its ID (alone, no senseId) and when it doesn't find it, try to call NewEntity() which fails. So now the JsonPatchDocument is carried in an UpdateSensePictureChange, which also records the senseId so that it can retrieve the sense and get the picture from the sense. Then the JsonPatch is correctly applied.
The NormalizePatch method will get special handling for MediaUri values, to ensure that they remain exactly as found in the .fwdata file and are not accidentally changed.
No LfClassicMediaAdapter yet, so this won't be able to find the files, but at least the Sense objects returned will have Pictures lists that are non-empty. Later we'll create a LfClassicMediaAdapter to find the picture files in Language Forge.
A jsonb property that's a list should have default value "[]".
Need to check if this is correct, because it might be a chicken-and-egg situation where the AddChange won't have actually applied the change yet.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Changes/UpdateSensePictureChange.cs`:
- Around line 28-30: The UpdateSensePictureChange class currently applies a
patch to a picture found only by PictureId without checking if it has been
soft-deleted. After the FirstOrDefault call that retrieves the picture by
PictureId, add an additional null check to verify that the picture's DeletedAt
property is null before calling Patch.ApplyTo. If the picture's DeletedAt is not
null, the method should return without applying the patch, preventing mutations
to soft-deleted picture records.
🪄 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: 6fb0470f-da98-4579-b826-9238606e508a
📒 Files selected for processing (8)
backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt/Changes/UpdateSensePictureChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.csbackend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.csbackend/LfClassicData/LfClassicMiniLcmApi.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.cs
- backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs
- backend/LfClassicData/LfClassicMiniLcmApi.cs
- backend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.cs
- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
Updates on deleted pictures, like other entities, should become no-ops.
|
@CodeRabbit fullreview |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs (1)
21-21: ⚡ Quick winAlign
CreateSenseChangepicture payload with materialization behavior.Line 21 and Line 36 persist
Pictureson the change, but Line 51 always materializesSense.Picturesas empty. That creates a misleading/ignored payload path and extra commit bloat. Prefer removingPicturesfromCreateSenseChange(since picture creation is already handled byCreateSensePictureChange) to keep one source of truth.Suggested diff
public CreateSenseChange(Sense sense, Guid entryId) : base(sense.Id == Guid.Empty ? Guid.NewGuid() : sense.Id) { sense.Id = EntityId; EntryId = entryId; Order = sense.Order; Definition = sense.Definition; SemanticDomains = sense.SemanticDomains; Gloss = sense.Gloss; PartOfSpeechId = sense.PartOfSpeech?.Id ?? sense.PartOfSpeechId; - Pictures = sense.Pictures; } @@ public Guid? PartOfSpeechId { get; set; } public IList<SemanticDomain>? SemanticDomains { get; set; } -public List<Picture>? Pictures { get; set; }Also applies to: 36-36, 51-51
🤖 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/Changes/CreateSenseChange.cs` at line 21, Remove the Pictures property assignments from the CreateSenseChange class to align with its materialization behavior. Specifically, delete the `Pictures = sense.Pictures;` assignments (which appear at lines 21 and 36 in the provided context) since the materialization at line 51 always creates an empty Pictures collection, and picture creation is already handled separately by the CreateSensePictureChange class. This eliminates the misleading payload and reduces unnecessary commit bloat by maintaining a single source of truth for picture persistence.backend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.cs (1)
565-583: The path matching inNormalizePicturePatchis actually safe. The codebase does not apply a camelCase naming policy to JSON Patch operations. Patches useSystemTextJsonPatchwith the defaultJsonSerializerOptions(noPropertyNamingPolicyconfigured), which preserves PascalCase property names. Picture patch paths will consistently be/MediaUri, not/mediaUri, so the exact-case string comparison correctly identifies all MediaUri updates.That said, if you want to make the code more robust to future naming policy changes, you could still extract the path comparison to a helper method using
StringComparison.OrdinalIgnoreCase. This is optional but recommended as defensive programming.🤖 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/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.cs` around lines 565 - 583, To make the code more robust to future naming policy changes in the NormalizePicturePatch method, extract the path comparison logic into a helper method that uses case-insensitive string comparison. Create a private helper method that checks if a given operation path matches the MediaUri property name using StringComparison.OrdinalIgnoreCase, then replace all occurrences of the exact-case string equality checks (op.Path == $"/{nameof(Picture.MediaUri)}") throughout NormalizePicturePatch with calls to this new helper method to ensure consistent and resilient path matching.
🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 1792-1805: The ValidateOwnership method is performing both
sense-level and entry-level ownership checks, but sense-owned objects like
pictures should not be validated against the caller-supplied entryId since
senses can be legitimately re-parented to different entries. Remove the entry
ownership validation (the check for sense.Entry.Guid != entryId), remove the
entryId parameter from the ValidateOwnership method signature, and update all
call sites (at lines 1876, 1913, and 1928) to remove the entryId argument being
passed to ValidateOwnership, keeping only the picture and senseId parameters.
- Around line 1833-1837: The GetPicture method lacks a sense ownership
validation, allowing any picture with a valid ID to be returned regardless of
whether it belongs to the specified senseId. After retrieving the lcmPicture
from PictureRepository.TryGetObject, add a security check to verify that the
picture's owner is an ILexSense instance and that the owner's Guid matches the
provided senseId parameter before calling FromLcmPicture. If the ownership check
fails, return null instead of the mapped picture to prevent unauthorized access
to unrelated pictures.
In `@backend/FwLite/LcmCrdt/Changes/ReorderSensePictureChange.cs`:
- Around line 15-19: In the ReorderSensePictureChange class, the FirstOrDefault
query that searches for the picture by PictureId only checks the picture ID but
does not verify that the picture has not been soft-deleted. Add a DeletedAt is
null check to the LINQ condition in the FirstOrDefault call to ensure that
soft-deleted pictures are skipped during the reordering operation, preventing
mutation of deleted entities.
In `@backend/FwLite/LcmCrdt/Changes/UpdateSensePictureChange.cs`:
- Around line 26-31: In the ApplyChange method of UpdateSensePictureChange
class, after calling Patch.ApplyTo(picture), the entity.Pictures collection is
not re-sorted even though the patch can modify the Order property of the
picture. Add code after the Patch.ApplyTo(picture) call to re-sort the
entity.Pictures collection to preserve ordering invariants and maintain
consistent state.
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 862-870: The CreatePicture method calls AddChange with a
CreateSensePictureChange that can generate a new ID when the input picture.Id is
Guid.Empty, but then attempts to read back using the original picture.Id
parameter which may not match the actual created ID. After calling AddChange,
capture the ID that was actually generated and used by the change (either by
checking if the picture object was mutated with the new ID or by returning the
ID from AddChange), then use that generated ID in the GetPicture call instead of
the original picture.Id parameter.
- Around line 885-888: The UpdatePicture method needs to validate that the
jsonPatch from update.Patch does not contain any patch operations that modify
the Order field, as UpdateSensePictureChange does not re-sort sense.Pictures and
can break ordering invariants. Before creating the UpdateSensePictureChange with
jsonPatch, check if any patch operations target the Order field and reject the
request if found, directing the caller to use MovePicture instead for reordering
operations.
- Around line 862-914: Add ownership validation in the picture write methods
CreatePicture, both UpdatePicture overloads, MovePicture, and DeletePicture to
ensure the sense with the provided senseId actually belongs to the entry with
the provided entryId before calling AddChange(...). This can be accomplished by
fetching the sense from the repository and verifying that sense.EntryId matches
the entryId parameter, throwing NotFoundException if the sense does not exist or
does not belong to the specified entry. This validation should occur before any
write changes are applied to maintain route contract consistency and prevent
mismatched ID operations.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs`:
- Line 21: Remove the Pictures property assignments from the CreateSenseChange
class to align with its materialization behavior. Specifically, delete the
`Pictures = sense.Pictures;` assignments (which appear at lines 21 and 36 in the
provided context) since the materialization at line 51 always creates an empty
Pictures collection, and picture creation is already handled separately by the
CreateSensePictureChange class. This eliminates the misleading payload and
reduces unnecessary commit bloat by maintaining a single source of truth for
picture persistence.
In `@backend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.cs`:
- Around line 565-583: To make the code more robust to future naming policy
changes in the NormalizePicturePatch method, extract the path comparison logic
into a helper method that uses case-insensitive string comparison. Create a
private helper method that checks if a given operation path matches the MediaUri
property name using StringComparison.OrdinalIgnoreCase, then replace all
occurrences of the exact-case string equality checks (op.Path ==
$"/{nameof(Picture.MediaUri)}") throughout NormalizePicturePatch with calls to
this new helper method to ensure consistent and resilient path matching.
🪄 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: ce8f808b-37b0-4370-8041-b7d0b44c9008
📒 Files selected for processing (32)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdatePictureProxy.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.csbackend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt/Changes/CreateSenseChange.csbackend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/RemoveSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/ReorderSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/UpdateSensePictureChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260617073518_PicturesStoredAsJsonOnSenses.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/LcmCrdt/OrderPicker.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Models/Picture.csbackend/FwLite/MiniLcm/Models/Sense.csbackend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.csbackend/FwLite/MiniLcm/SyncHelpers/PictureSync.csbackend/FwLite/MiniLcm/SyncHelpers/SenseSync.csbackend/LfClassicData/Entities/Entry.csbackend/LfClassicData/LfClassicMiniLcmApi.csfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ISense.tsfrontend/viewer/src/lib/sandbox/Sandbox.sveltefrontend/viewer/src/lib/utils.tsfrontend/viewer/src/project/demo/demo-entry-data.ts
The .HasDefaultValue is expecting a C# value, it's .HasDefaultValueSql that takes a string in SQL syntax.
Pictures belong to a sense, but that sense might move to a different owning entry. So the pictures should validate the sense that owns them, but not the entry that owns the sense.
If a picture has already been deleted, its ReorderSensePictureChange should become a no-op.
This ensures that if the picture was created via code that assigns an empty GUID and waits to see what ID will come back from the database, the new GUID (which is created in CreateSensePictureChange) will be found and the correct Picture object will be returned.
|
@CodeRabbit fullreview |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 1874-1883: The UpdatePictureProxy class used in the update.Apply()
call lacks persistence mapping for picture ordering, causing /Order operations
to silently succeed without reordering the sense picture sequence. Add explicit
field mapping for picture ordering in the UpdatePictureProxy class to properly
persist order changes to the LCM data structure, or implement a guard that fails
fast when order updates are attempted, ensuring that order changes are either
properly handled or explicitly rejected rather than silently ignored.
- Around line 1831-1835: In the GetPicture method, the ownership validation is
being performed on lcmPicture before checking if it is actually null. The
TryGetObject call on line 1833 may fail to find an object, leaving lcmPicture as
null, but ValidateOwnership is still called with this null value which can cause
a crash. Add a null check for lcmPicture immediately after the TryGetObject
call, and return null early if lcmPicture is null, before calling
ValidateOwnership.
In `@backend/FwLite/FwLiteProjectSync.Tests/DoubleDiffTests.cs`:
- Around line 10-51: Add comprehensive round-trip tests for picture sync
operations in FwLiteProjectSync.Tests to verify bidirectional consistency.
Create tests that validate picture operations (create, update, move, delete)
work correctly when syncing the Sense model's Pictures collection in both
directions, ensuring the sync infrastructure handles picture changes
consistently between local and remote states, similar to how the existing
DoubleDiff tests validate double value changes.
In `@backend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.cs`:
- Around line 22-31: The MediaUri property in the CreateSensePictureChange class
is declared as non-nullable but has no initializer, violating the nullable
reference type contract and causing a CS8618 compilation error. To fix this, add
an initializer to the MediaUri property declaration (such as = default), change
it to be nullable by using MediaUri?, or mark it as required. Follow the pattern
used in UpdateSensePictureChange.cs where properties are properly initialized
with their declarations.
In `@backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs`:
- Around line 102-104: The MediaUriDbConverter class lacks defensive null and
empty checks in its deserialization logic, which could cause exceptions during
materialization. Modify the deserialization lambda in MediaUriDbConverter to
check if the input json parameter is null or empty before attempting
deserialization, and return MediaUri.NotFound as a safe fallback in those cases.
This approach aligns with the existing MediaUri convention and is consistent
with other converters in the codebase like RichStringDbConverter and
PictureListDbConverter that include similar safeguards.
In `@backend/FwLite/LcmCrdt/LcmCrdtKernel.cs`:
- Around line 226-229: The HasConversion configurations for both the Pictures
and SemanticDomains properties in LcmCrdtKernel.cs are missing null/empty JSON
payload protection that exists in the LinqToDB mapping at line 143. Add a
string.IsNullOrEmpty check in the deserialization lambda for both the Pictures
property conversion (around lines 226-229) and the SemanticDomains property
conversion (around lines 222-225) to handle empty or null JSON values gracefully
by returning a new empty list instead of attempting deserialization that would
fail on malformed data.
In `@backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs`:
- Line 19: The ProductVersion annotation in the LcmCrdtDbContextModelSnapshot.cs
file has been updated to 10.0.8, but this EF Core upgrade was already applied in
a prior commit and should not be included in the current PR. Revert the
ProductVersion value in the modelBuilder.HasAnnotation call back to its original
version so that this PR contains only picture-support-related changes and
excludes the unrelated EF Core version upgrade.
In `@backend/FwLite/MiniLcm/Models/Picture.cs`:
- Around line 11-12: The MediaUri property in the Picture class is declared as
non-nullable but is not initialized or marked as required, causing CS8618
compilation errors. Fix this by adding the `required` keyword to the MediaUri
property declaration to enforce initialization at construction time, making it
consistent with other mandatory properties like WritingSystem and
ComplexFormComponent in the codebase.
In `@backend/FwLite/MiniLcm/SyncHelpers/PictureSync.cs`:
- Around line 40-44: The code is currently diffing Picture.MediaUri by calling
ToString() directly on the URI objects without resolving them to their canonical
form first, as indicated by the TODO comment. To fix this, before calling
SimpleStringDiff.GetStringDiff with nameof(Picture.MediaUri), resolve
beforePicture.MediaUri and afterPicture.MediaUri to their canonical
representations (rather than using ToString() directly), then pass the canonical
string values to the GetStringDiff method to ensure consistent and correct patch
operations.
In `@backend/LfClassicData/LfClassicMiniLcmApi.cs`:
- Around line 354-362: The ToPicture method hardcodes MediaUri.NotFound
regardless of whether the picture has a persisted filename, causing it to ignore
the FileName property from the Entities.Picture object. Replace the hardcoded
MediaUri.NotFound assignment with logic that checks if picture.FileName exists
and constructs an appropriate MediaUri from it; only use MediaUri.NotFound as a
fallback when no filename is present.
🪄 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: 7a6c0e6e-bd9e-48f2-b739-94f24198b77f
📒 Files selected for processing (34)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdatePictureProxy.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.csbackend/FwLite/FwLiteProjectSync.Tests/DoubleDiffTests.csbackend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt/Changes/CreateSenseChange.csbackend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/RemoveSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/ReorderSensePictureChange.csbackend/FwLite/LcmCrdt/Changes/UpdateSensePictureChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260619022528_AddPicturesColumnToSenses.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260619022528_AddPicturesColumnToSenses.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/LcmCrdt/OrderPicker.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Models/Picture.csbackend/FwLite/MiniLcm/Models/Sense.csbackend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.csbackend/FwLite/MiniLcm/SyncHelpers/DoubleDiff.csbackend/FwLite/MiniLcm/SyncHelpers/PictureSync.csbackend/FwLite/MiniLcm/SyncHelpers/SenseSync.csbackend/LfClassicData/Entities/Entry.csbackend/LfClassicData/LfClassicMiniLcmApi.csfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IPicture.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ISense.tsfrontend/viewer/src/lib/sandbox/Sandbox.sveltefrontend/viewer/src/lib/utils.tsfrontend/viewer/src/project/demo/demo-entry-data.ts
| public Task<Picture?> GetPicture(Guid entryId, Guid senseId, Guid id) | ||
| { | ||
| PictureRepository.TryGetObject(id, out var lcmPicture); | ||
| ValidateOwnership(lcmPicture, entryId, senseId); | ||
| return Task.FromResult(lcmPicture is null ? null : FromLcmPicture(senseId, lcmPicture)); |
There was a problem hiding this comment.
Null-check lcmPicture before ownership validation in GetPicture.
Line 1834 validates ownership before checking whether TryGetObject actually found a picture, so unknown IDs can crash instead of returning null.
Suggested fix
public Task<Picture?> GetPicture(Guid entryId, Guid senseId, Guid id)
{
- PictureRepository.TryGetObject(id, out var lcmPicture);
- ValidateOwnership(lcmPicture, entryId, senseId);
- return Task.FromResult(lcmPicture is null ? null : FromLcmPicture(senseId, lcmPicture));
+ PictureRepository.TryGetObject(id, out var lcmPicture);
+ if (lcmPicture is null) return Task.FromResult<Picture?>(null);
+ ValidateOwnership(lcmPicture, entryId, senseId);
+ return Task.FromResult<Picture?>(FromLcmPicture(senseId, lcmPicture));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Task<Picture?> GetPicture(Guid entryId, Guid senseId, Guid id) | |
| { | |
| PictureRepository.TryGetObject(id, out var lcmPicture); | |
| ValidateOwnership(lcmPicture, entryId, senseId); | |
| return Task.FromResult(lcmPicture is null ? null : FromLcmPicture(senseId, lcmPicture)); | |
| public Task<Picture?> GetPicture(Guid entryId, Guid senseId, Guid id) | |
| { | |
| PictureRepository.TryGetObject(id, out var lcmPicture); | |
| if (lcmPicture is null) return Task.FromResult<Picture?>(null); | |
| ValidateOwnership(lcmPicture, senseId); | |
| return Task.FromResult<Picture?>(FromLcmPicture(senseId, lcmPicture)); | |
| } |
🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs` around lines 1831
- 1835, In the GetPicture method, the ownership validation is being performed on
lcmPicture before checking if it is actually null. The TryGetObject call on line
1833 may fail to find an object, leaving lcmPicture as null, but
ValidateOwnership is still called with this null value which can cause a crash.
Add a null check for lcmPicture immediately after the TryGetObject call, and
return null early if lcmPicture is null, before calling ValidateOwnership.
| var lcmPicture = PictureRepository.GetObject(pictureId); | ||
| ValidateOwnership(lcmPicture, entryId, senseId); | ||
| UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Update picture", | ||
| "Revert picture", | ||
| Cache.ServiceLocator.ActionHandler, | ||
| () => | ||
| { | ||
| var updateProxy = new UpdatePictureProxy(lcmPicture, this); | ||
| update.Apply(updateProxy); | ||
| }); |
There was a problem hiding this comment.
UpdatePicture patch flow can silently ignore /Order updates.
update.Apply(new UpdatePictureProxy(...)) has no persistence mapping for picture ordering, so /Order ops may report success without reordering the sense picture sequence.
Suggested guard (fail fast until order is explicitly mapped)
public Task<Picture> UpdatePicture(Guid entryId,
Guid senseId,
Guid pictureId,
UpdateObjectInput<Picture> update)
{
var lcmPicture = PictureRepository.GetObject(pictureId);
ValidateOwnership(lcmPicture, entryId, senseId);
+ if (update.Patch.Operations.Any(op => op.Path == $"/{nameof(Picture.Order)}"))
+ throw new NotSupportedException("Use MovePicture to change picture order.");
UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Update picture",
"Revert picture",
Cache.ServiceLocator.ActionHandler,
() =>
{
var updateProxy = new UpdatePictureProxy(lcmPicture, this);
update.Apply(updateProxy);
});As per coding guidelines: “Map new fields to/from LCM (FieldWorks) data structures in FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs.”
🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs` around lines 1874
- 1883, The UpdatePictureProxy class used in the update.Apply() call lacks
persistence mapping for picture ordering, causing /Order operations to silently
succeed without reordering the sense picture sequence. Add explicit field
mapping for picture ordering in the UpdatePictureProxy class to properly persist
order changes to the LCM data structure, or implement a guard that fails fast
when order updates are attempted, ensuring that order changes are either
properly handled or explicitly rejected rather than silently ignored.
Source: Coding guidelines
| [Fact] | ||
| public void DiffEmptyDoublesDoNothing() | ||
| { | ||
| double? before = null; | ||
| double? after = null; | ||
| var result = DoubleDiff.GetDoubleDiff<Placeholder>("test", before, after); | ||
| result.Should().BeEmpty(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void DiffOneToEmptyAddsOne() | ||
| { | ||
| double? before = null; | ||
| var after = 1.5; | ||
| var result = DoubleDiff.GetDoubleDiff<Placeholder>("test", before, after); | ||
| result.Should().BeEquivalentTo([ | ||
| new Operation<Placeholder>("add", "/test", null, 1.5) | ||
| ]); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void DiffOneToTwoReplacesOne() | ||
| { | ||
| var before = 1.5; | ||
| var after = 2.5; | ||
| var result = DoubleDiff.GetDoubleDiff<Placeholder>("test", before, after); | ||
| result.Should().BeEquivalentTo([ | ||
| new Operation<Placeholder>("replace", "/test", null, 2.5) | ||
| ]); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void DiffNoneToOneRemovesOne() | ||
| { | ||
| var before = 1.5; | ||
| double? after = null; | ||
| var result = DoubleDiff.GetDoubleDiff<Placeholder>("test", before, after); | ||
| result.Should().BeEquivalentTo([ | ||
| new Operation<Placeholder>("remove", "/test", null) | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate candidate sync tests for picture-related flows in FwLiteProjectSync.Tests
fd -t f --glob '*Tests.cs' backend/FwLite/FwLiteProjectSync.Tests
# Inspect references to picture/sense sync operations in that test project
rg -n --type cs -C3 '\b(PictureSync|SenseSync\.Sync|CreatePicture|UpdatePicture|MovePicture|DeletePicture)\b' backend/FwLite/FwLiteProjectSync.TestsRepository: sillsdev/languageforge-lexbox
Length of output: 1053
🏁 Script executed:
# Search for "picture" more broadly (case-insensitive) in test files
rg -i -n 'picture' backend/FwLite/FwLiteProjectSync.Tests --type cs
# Check what SenseSync and related changes actually involve
rg -i -n 'sensesync|picture' backend/FwLite/FwLiteProjectSync --type cs | head -50
# Check if there are any test files specifically for picture or sense sync
fd -i picture backend/FwLite/FwLiteProjectSync.Tests
fd -i sense backend/FwLite/FwLiteProjectSync.TestsRepository: sillsdev/languageforge-lexbox
Length of output: 2471
🏁 Script executed:
# Check what tests actually exist for picture sync or sense sync in the test project
rg -i 'picture|sense' backend/FwLite/FwLiteProjectSync.Tests --type cs
# Look at EntrySyncTests.cs to understand the scope of what's being tested
head -100 backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs
# Check SyncTests.cs for picture/sense coverage
rg -i 'picture|sense' backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs -A 2 -B 2Repository: sillsdev/languageforge-lexbox
Length of output: 21000
🏁 Script executed:
# Check if Picture is a property of Sense or ExampleSentence
rg -n 'Pictures|Picture' backend/FwLite/FwLiteProjectSync.Tests --type cs -A 2
# Check the Sense model to understand its structure
fd -t f 'Sense.cs' backend --exec head -50 {} \;
# Check ExampleSentence model structure
fd -t f 'ExampleSentence.cs' backend --exec head -50 {} \;
# Search for any test files mentioning or testing pictures anywhere
find backend/FwLite/FwLiteProjectSync.Tests -name '*.cs' -exec grep -l -i 'picture' {} \;Repository: sillsdev/languageforge-lexbox
Length of output: 3247
Add picture sync round-trip tests to FwLiteProjectSync.Tests.
Picture operations (create, update, move, delete) are implemented in the sync infrastructure and the Sense model includes a Pictures collection, but there are no sync round-trip tests for picture operations in FwLiteProjectSync.Tests. Per coding guidelines, add targeted picture sync tests to verify bidirectional consistency between sync implementations.
🤖 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/FwLiteProjectSync.Tests/DoubleDiffTests.cs` around lines 10 -
51, Add comprehensive round-trip tests for picture sync operations in
FwLiteProjectSync.Tests to verify bidirectional consistency. Create tests that
validate picture operations (create, update, move, delete) work correctly when
syncing the Sense model's Pictures collection in both directions, ensuring the
sync infrastructure handles picture changes consistently between local and
remote states, similar to how the existing DoubleDiff tests validate double
value changes.
Source: Coding guidelines
| [JsonConstructor] | ||
| private CreateSensePictureChange(Guid entityId) : base(entityId) | ||
| { | ||
| } | ||
|
|
||
| public Guid PictureId { get; set; } | ||
| public double Order { get; set; } | ||
| public RichMultiString? Caption { get; set; } | ||
| public MediaUri MediaUri { get; set; } | ||
| public BetweenPosition? Between { get; set; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find backend -name "CreateSensePictureChange.cs" -type fRepository: sillsdev/languageforge-lexbox
Length of output: 132
🏁 Script executed:
fd -t f "CreateSensePictureChange.cs" backend/Repository: sillsdev/languageforge-lexbox
Length of output: 132
🏁 Script executed:
cat -n backend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.csRepository: sillsdev/languageforge-lexbox
Length of output: 1941
🏁 Script executed:
# Check for .csproj file in the directory to see nullable settings
find backend/FwLite/LcmCrdt -name "*.csproj" | head -5Repository: sillsdev/languageforge-lexbox
Length of output: 111
🏁 Script executed:
# Check if there's a global Directory.Build.props that sets nullable
find backend -name "Directory.Build.props" -o -name "*.csproj" | grep -E "(Directory\.Build|FwLite.*csproj)" | head -5Repository: sillsdev/languageforge-lexbox
Length of output: 339
🏁 Script executed:
cat -n backend/Directory.Build.propsRepository: sillsdev/languageforge-lexbox
Length of output: 2901
🏁 Script executed:
cat -n backend/FwLite/LcmCrdt/LcmCrdt.csprojRepository: sillsdev/languageforge-lexbox
Length of output: 2011
🏁 Script executed:
# Let's also look at other change classes to see how they handle the JSON constructor pattern
ls -la backend/FwLite/LcmCrdt/Changes/ | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 1441
🏁 Script executed:
# Check a few other change classes to understand the pattern
rg "JsonConstructor" backend/FwLite/LcmCrdt/Changes/ -A 3 -B 1 | head -60Repository: sillsdev/languageforge-lexbox
Length of output: 4366
🏁 Script executed:
# Check UpdateSensePictureChange.cs to see if it has the same issue
cat -n backend/FwLite/LcmCrdt/Changes/UpdateSensePictureChange.csRepository: sillsdev/languageforge-lexbox
Length of output: 1655
🏁 Script executed:
# Search for null-forgiving operators or pragmas in CreateSensePictureChange
rg "MediaUri" backend/FwLite/LcmCrdt/Changes/CreateSensePictureChange.cs -B 2 -A 2Repository: sillsdev/languageforge-lexbox
Length of output: 550
🏁 Script executed:
# Check if MediaUri type has a default value or special handling
rg "class MediaUri|struct MediaUri" backend/Repository: sillsdev/languageforge-lexbox
Length of output: 388
🏁 Script executed:
cat -n backend/FwLite/MiniLcm/Media/MediaUri.cs | head -40Repository: sillsdev/languageforge-lexbox
Length of output: 1498
MediaUri property violates nullable reference type contract with no initializer.
The private JSON constructor does not initialize MediaUri, and the property has no initializer despite being non-nullable. With <WarningsAsErrors>Nullable</WarningsAsErrors> enabled, this should cause a CS8618 compilation error. Either initialize the property in its declaration (e.g., = default), make it nullable (MediaUri?), or mark it as required. See UpdateSensePictureChange.cs line 24 for the correct pattern: public JsonPatchDocument<Picture> Patch { get; init; } = new();
🤖 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/Changes/CreateSensePictureChange.cs` around lines 22 -
31, The MediaUri property in the CreateSensePictureChange class is declared as
non-nullable but has no initializer, violating the nullable reference type
contract and causing a CS8618 compilation error. To fix this, add an initializer
to the MediaUri property declaration (such as = default), change it to be
nullable by using MediaUri?, or mark it as required. Follow the pattern used in
UpdateSensePictureChange.cs where properties are properly initialized with their
declarations.
Source: Coding guidelines
| private class MediaUriDbConverter() : ValueConverter<MiniLcm.Media.MediaUri, string>( | ||
| mul => JsonSerializer.Serialize(mul, (JsonSerializerOptions?)null), | ||
| json => JsonSerializer.Deserialize<MiniLcm.Media.MediaUri>(json, (JsonSerializerOptions?)null)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the LcmCrdtDbContext.cs file around the specified lines
cat -n backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs | sed -n '95,115p'Repository: sillsdev/languageforge-lexbox
Length of output: 1057
🏁 Script executed:
# Let's also check the full context of the MediaUriDbConverter and surrounding code
cat -n backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs | sed -n '95,130p'Repository: sillsdev/languageforge-lexbox
Length of output: 1057
🏁 Script executed:
# Check how MediaUri is defined in the MiniLcm.Media namespace
find . -type f -name "*.cs" | xargs rg -l "class MediaUri|record MediaUri" | head -5Repository: sillsdev/languageforge-lexbox
Length of output: 5122
🏁 Script executed:
# Look for usage of MediaUri in the codebase
rg "MediaUri" --type cs -A 2 -B 2 | head -50Repository: sillsdev/languageforge-lexbox
Length of output: 4955
🏁 Script executed:
# Check if there are other ValueConverters in the file for comparison
rg "ValueConverter" backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs -B 2 -A 3Repository: sillsdev/languageforge-lexbox
Length of output: 1561
🏁 Script executed:
# Check if MediaUri is used in model configurations and whether it's nullable
rg "Media\.MediaUri" backend/FwLite --type cs -B 2 -A 2 | head -40Repository: sillsdev/languageforge-lexbox
Length of output: 1245
🏁 Script executed:
# Examine the MediaUri definition
cat -n backend/FwLite/MiniLcm/Media/MediaUri.csRepository: sillsdev/languageforge-lexbox
Length of output: 2185
🏁 Script executed:
# Check if MediaUri is nullable in the Picture model or used elsewhere
rg "MediaUri" backend/FwLite/LcmCrdt --type cs -B 1 -A 1 | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 2502
🏁 Script executed:
# Check the Picture model definition to see if MediaUri is nullable
cat -n backend/FwLite/MiniLcm/Models/Picture.cs | head -50Repository: sillsdev/languageforge-lexbox
Length of output: 1285
🏁 Script executed:
# Check how RichStringDbConverter handles null - compare with MediaUriDbConverter
cat -n backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs | sed -n '80,100p'Repository: sillsdev/languageforge-lexbox
Length of output: 1031
🏁 Script executed:
# Check if there are any tests or error handling patterns for MediaUri deserialization
rg "MediaUri.*Deserialize|MediaUri.*try|MediaUri.*catch" --type cs -B 2 -A 2Repository: sillsdev/languageforge-lexbox
Length of output: 577
🏁 Script executed:
# Look for any existing error cases or known issues with MediaUri
rg "MediaUri\.NotFound" --type cs -B 2 -A 2 | head -40Repository: sillsdev/languageforge-lexbox
Length of output: 3280
🏁 Script executed:
# Check if JsonSerializer options are explicitly configured anywhere for MediaUri
rg "JsonSerializerOptions|JsonConverter" backend/FwLite/LcmCrdt --type cs -B 1 -A 1 | head -50Repository: sillsdev/languageforge-lexbox
Length of output: 4947
🏁 Script executed:
# Verify whether nullable MediaUri ever appears in the codebase (e.g., MediaUri?)
rg "MediaUri\?" --type cs | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 163
🏁 Script executed:
# Check if JsonSerializer.Deserialize with null options respects [JsonConverter] attribute
python3 << 'PY'
# According to .NET documentation, when options is null,
# JsonSerializer uses the default settings
# Default settings DO respect [JsonConverter] attributes on the type
# Let's verify this by checking if there are any JSON deserialization failures in tests
import subprocess
result = subprocess.run(
["rg", "JsonException|InvalidOperationException", "backend/FwLite", "--type", "cs", "-l"],
capture_output=True,
text=True
)
print("Files with JsonException handling:")
print(result.stdout)
PYRepository: sillsdev/languageforge-lexbox
Length of output: 3680
🏁 Script executed:
# Look at how MediaUri is actually used - does it ever need to be null/NotFound?
rg "Picture.*MediaUri|MediaUri.*NotFound" --type cs -B 1 -A 1 | head -40Repository: sillsdev/languageforge-lexbox
Length of output: 3744
🏁 Script executed:
# Check database constraints - is MediaUri column nullable in the schema?
find . -name "*Migration*" -type f -name "*.cs" | xargs rg -l "MediaUri" | head -3Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check if there's a pattern of null-checking before deserialization in similar contexts
rg "string.IsNullOrWhiteSpace.*Deserialize|string.IsNullOrEmpty.*Deserialize" --type cs -B 1 -A 1Repository: sillsdev/languageforge-lexbox
Length of output: 881
🏁 Script executed:
# Verify what happens when JsonSerializer.Deserialize is called with null/empty string on a type with [JsonConverter]
cat -n backend/FwLite/MiniLcm/Media/MediaUri.cs | sed -n '45,58p'Repository: sillsdev/languageforge-lexbox
Length of output: 648
🏁 Script executed:
# Check if there are any actual database issues or test failures related to MediaUri
rg "MediaUri.*null|MediaUri.*empty" backend/FwLite --type cs -B 2 -A 2 | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 644
🏁 Script executed:
# Check what happens when Picture is loaded from database with bad MediaUri data
rg "Picture\s" backend/FwLite/LcmCrdt --type cs -A 3 -B 1 | grep -A 3 "MediaUri" | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check if MediaUri in Picture could ever be stored as null in the database
rg "HasColumnName|HasColumnType" backend/FwLite/LcmCrdt --type cs -B 3 -A 3 | grep -A 5 -B 5 "MediaUri"Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check EF Core ValueConverter behavior - does it properly use [JsonConverter] attributes?
# Let's look at how RichStringDbConverter handles this differently
cat -n backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs | sed -n '75,92p'Repository: sillsdev/languageforge-lexbox
Length of output: 872
🏁 Script executed:
# Verify if there's documentation or comments about why MediaUriDbConverter doesn't have null-checks
cat -n backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs | sed -n '98,109p'Repository: sillsdev/languageforge-lexbox
Length of output: 852
🏁 Script executed:
# Check if there are any actual issues filed or test cases that would break with null/empty MediaUri
rg "test.*MediaUri|Test.*MediaUri" --type cs -i -lRepository: sillsdev/languageforge-lexbox
Length of output: 132
Add defensive null/empty checks to MediaUriDbConverter.
The converter currently deserializes without guarding against null or empty payloads, inconsistent with similar converters in the codebase (e.g., RichStringDbConverter includes try-catch; PictureListDbConverter uses ?? new()). While MediaUri's [JsonConverter] attribute handles null gracefully by returning MediaUri.NotFound, the ValueConverter may not properly delegate when the input is null/empty, potentially throwing during materialization.
Consider returning MediaUri.NotFound as a safe fallback rather than throwing, which aligns with the existing MediaUri convention:
private class MediaUriDbConverter() : ValueConverter<MiniLcm.Media.MediaUri, string>(
mul => JsonSerializer.Serialize(mul, (JsonSerializerOptions?)null),
- json => JsonSerializer.Deserialize<MiniLcm.Media.MediaUri>(json, (JsonSerializerOptions?)null));
+ json => string.IsNullOrWhiteSpace(json)
+ ? MediaUri.NotFound
+ : JsonSerializer.Deserialize<MiniLcm.Media.MediaUri>(json, (JsonSerializerOptions?)null)
+ ?? MediaUri.NotFound);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private class MediaUriDbConverter() : ValueConverter<MiniLcm.Media.MediaUri, string>( | |
| mul => JsonSerializer.Serialize(mul, (JsonSerializerOptions?)null), | |
| json => JsonSerializer.Deserialize<MiniLcm.Media.MediaUri>(json, (JsonSerializerOptions?)null)); | |
| private class MediaUriDbConverter() : ValueConverter<MiniLcm.Media.MediaUri, string>( | |
| mul => JsonSerializer.Serialize(mul, (JsonSerializerOptions?)null), | |
| json => string.IsNullOrWhiteSpace(json) | |
| ? MediaUri.NotFound | |
| : JsonSerializer.Deserialize<MiniLcm.Media.MediaUri>(json, (JsonSerializerOptions?)null) | |
| ?? MediaUri.NotFound); |
🤖 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/LcmCrdtDbContext.cs` around lines 102 - 104, The
MediaUriDbConverter class lacks defensive null and empty checks in its
deserialization logic, which could cause exceptions during materialization.
Modify the deserialization lambda in MediaUriDbConverter to check if the input
json parameter is null or empty before attempting deserialization, and return
MediaUri.NotFound as a safe fallback in those cases. This approach aligns with
the existing MediaUri convention and is consistent with other converters in the
codebase like RichStringDbConverter and PictureListDbConverter that include
similar safeguards.
| builder.Property(s => s.Pictures) | ||
| .HasColumnType("jsonb") | ||
| .HasConversion(list => JsonSerializer.Serialize(list, (JsonSerializerOptions?)null), | ||
| json => JsonSerializer.Deserialize<List<Picture>>(json, (JsonSerializerOptions?)null) ?? new()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me find and read the LcmCrdtKernel.cs file
fd "LcmCrdtKernel.cs" --type fRepository: sillsdev/languageforge-lexbox
Length of output: 113
🏁 Script executed:
# Read the relevant sections of the file
cat -n backend/FwLite/LcmCrdt/LcmCrdtKernel.cs | sed -n '135,155p'Repository: sillsdev/languageforge-lexbox
Length of output: 1867
🏁 Script executed:
# Read lines around 226-229
cat -n backend/FwLite/LcmCrdt/LcmCrdtKernel.cs | sed -n '220,235p'Repository: sillsdev/languageforge-lexbox
Length of output: 1181
Make Sense.Pictures and Sense.SemanticDomains conversions tolerant to empty payloads (parity with line 143).
Lines 143-145 include a string.IsNullOrEmpty(json) check before deserializing List<Picture> in the LinqToDB mapping schema, but the Entity Framework conversions at lines 226-229 (Pictures) and 222-225 (SemanticDomains) lack this protection. This inconsistency can cause deserialization exceptions at read time when encountering null or empty JSON values from legacy or malformed data.
Suggested fix
builder.Property(s => s.Pictures)
.HasColumnType("jsonb")
.HasConversion(list => JsonSerializer.Serialize(list, (JsonSerializerOptions?)null),
- json => JsonSerializer.Deserialize<List<Picture>>(json, (JsonSerializerOptions?)null) ?? new());
+ json => string.IsNullOrWhiteSpace(json)
+ ? new()
+ : JsonSerializer.Deserialize<List<Picture>>(json, (JsonSerializerOptions?)null) ?? new());
builder.Property(s => s.SemanticDomains)
.HasColumnType("jsonb")
.HasConversion(list => JsonSerializer.Serialize(list, (JsonSerializerOptions?)null),
- json => JsonSerializer.Deserialize<List<SemanticDomain>>(json, (JsonSerializerOptions?)null) ?? new());
+ json => string.IsNullOrWhiteSpace(json)
+ ? new()
+ : JsonSerializer.Deserialize<List<SemanticDomain>>(json, (JsonSerializerOptions?)null) ?? new());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| builder.Property(s => s.Pictures) | |
| .HasColumnType("jsonb") | |
| .HasConversion(list => JsonSerializer.Serialize(list, (JsonSerializerOptions?)null), | |
| json => JsonSerializer.Deserialize<List<Picture>>(json, (JsonSerializerOptions?)null) ?? new()); | |
| builder.Property(s => s.Pictures) | |
| .HasColumnType("jsonb") | |
| .HasConversion(list => JsonSerializer.Serialize(list, (JsonSerializerOptions?)null), | |
| json => string.IsNullOrWhiteSpace(json) | |
| ? new() | |
| : JsonSerializer.Deserialize<List<Picture>>(json, (JsonSerializerOptions?)null) ?? new()); | |
| builder.Property(s => s.SemanticDomains) | |
| .HasColumnType("jsonb") | |
| .HasConversion(list => JsonSerializer.Serialize(list, (JsonSerializerOptions?)null), | |
| json => string.IsNullOrWhiteSpace(json) | |
| ? new() | |
| : JsonSerializer.Deserialize<List<SemanticDomain>>(json, (JsonSerializerOptions?)null) ?? new()); |
🤖 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/LcmCrdtKernel.cs` around lines 226 - 229, The
HasConversion configurations for both the Pictures and SemanticDomains
properties in LcmCrdtKernel.cs are missing null/empty JSON payload protection
that exists in the LinqToDB mapping at line 143. Add a string.IsNullOrEmpty
check in the deserialization lambda for both the Pictures property conversion
(around lines 226-229) and the SemanticDomains property conversion (around lines
222-225) to handle empty or null JSON values gracefully by returning a new empty
list instead of attempting deserialization that would fail on malformed data.
| { | ||
| #pragma warning disable 612, 618 | ||
| modelBuilder.HasAnnotation("ProductVersion", "9.0.16"); | ||
| modelBuilder.HasAnnotation("ProductVersion", "10.0.8"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check EF Core package versions in all csproj files
fd -e csproj -x grep -H "Microsoft.EntityFrameworkCore" {} \; | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 705
🏁 Script executed:
#!/bin/bash
# Check the actual ProductVersion in the snapshot file
echo "=== ProductVersion in ModelSnapshot ==="
sed -n '19p' backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs
# Check the csproj files for explicit version numbers
echo "=== LcmCrdt.csproj EntityFrameworkCore packages ==="
grep -A 1 "Microsoft.EntityFrameworkCore" backend/FwLite/LcmCrdt/LcmCrdt.csproj | grep -E "Version|Include"
# Also check the Directory.Build.props for centralized versioning
echo "=== Directory.Build.props (if exists) ==="
if [ -f "Directory.Build.props" ]; then
grep -i "entityframework" Directory.Build.props
fi
# Check for any global package references
echo "=== backend/Directory.Build.props ==="
if [ -f "backend/Directory.Build.props" ]; then
grep -i "entityframework" backend/Directory.Build.props
fiRepository: sillsdev/languageforge-lexbox
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Check ComplexProperty signature at line 554
echo "=== ComplexProperty signature at line 554 ==="
sed -n '550,560p' backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csRepository: sillsdev/languageforge-lexbox
Length of output: 655
🌐 Web query:
EF Core 10 ComplexProperty API signature changes from EF Core 9
💡 Result:
EF Core 10: the ComplexProperty “API signature changes from EF Core 9” that matter are not about the delegate/lambda overloads changing; rather, EF Core 10 introduces breaking behavior in complex-type column/property naming and some related convention signatures. 1) Complex property mapping behavior changed (breaking) - EF Core 10 uniquifies complex type column names by appending a number when the same column name would otherwise collide on the table. [1] - EF Core 10 also uses the full path for nested complex type property column names (e.g., EntityType.Complex.NestedComplex.Property maps to Complex_NestedComplex_Property). [1] Impact: code that relied on prior column naming (e.g., raw SQL, manual projections, migrations/hand-written column names, or expectations about generated column names) may need adjustment. 2) EF Core 10 complex-type column naming improvements are documented as breaking changes The EF Core 10 breaking changes list explicitly calls out: - “Complex type column names are now uniquified” [1] - “Nested complex type properties use full path in column names” [1] 3) ComplexProperty builder signatures: generally still available via EntityTypeBuilder.ComplexProperty and nested configuration builders - EF Core 10 docs show the primary ComplexProperty configuration API on EntityTypeBuilder (for mapping complex properties, optionally to JSON via ToJson). [2] - EF Core 10 docs also show configuration APIs like ComplexPropertyBuilder.Property(...) for configuring properties inside a complex type. [3] These indicate the modeling entry points still exist, but the important migration concerns are the schema/column naming changes above. What to do when upgrading EF Core 9 -> 10 - Re-check any code/migrations that depend on specific generated column names for complex and nested complex types; EF Core 10 may now generate different names because of uniquification and full-path naming. [1] - Validate custom conventions if you wrote infrastructure-level conventions related to discriminator property sets (a separate breaking change in EF Core 10 is that IDiscriminatorPropertySetConvention’s signature changed). This is not a ComplexProperty API itself, but it is another common “signature change” encountered during upgrades. [1] Direct answer to the phrasing in your question - There isn’t a single obvious “ComplexProperty method signature changed” item called out in the EF Core 10 breaking changes docs; instead, EF Core 10’s breaking change for complex types is the generated column naming behavior. [1] If you share the exact EF Core 9 ComplexProperty overload/signature you’re using (e.g., expression vs string-based, nested types, ToJson vs non-JSON, or EF Core 9-specific custom builder/convention code), I can tell you precisely what (if anything) you need to change and map it to the EF Core 10 docs.
Citations:
- 1: https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-10.0/breaking-changes
- 2: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.metadata.builders.entitytypebuilder.complexproperty?view=efcore-10.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.metadata.builders.complexpropertybuilder.property?view=efcore-10.0
🏁 Script executed:
#!/bin/bash
# Check git history for previous ProductVersion values in this file
echo "=== Git log for ProductVersion changes ==="
git log --oneline -n 20 -- backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs
echo "=== Check if there's a previous snapshot or revision ==="
git show HEAD~1:backend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.cs 2>/dev/null | grep -A 2 "ProductVersion" | head -5Repository: sillsdev/languageforge-lexbox
Length of output: 1481
🏁 Script executed:
#!/bin/bash
# Check if EntityFrameworkCore packages have explicit versions in csproj
echo "=== Full EntityFrameworkCore section from LcmCrdt.csproj ==="
sed -n '/<ItemGroup>/,/<\/ItemGroup>/p' backend/FwLite/LcmCrdt/LcmCrdt.csproj | grep -A 1 -B 1 "EntityFrameworkCore"
# Check for version management files
echo "=== Check global.json for SDK version ==="
if [ -f "global.json" ]; then
cat global.json
fiRepository: sillsdev/languageforge-lexbox
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about the EF Core upgrade in recent commits
echo "=== Recent commits mentioning EF or EntityFramework ==="
git log --all --oneline --grep -i "ef\|entityframework" -n 10 2>/dev/null || echo "No matching commits found"
echo "=== Check for PR description or branch info ==="
git log --format="%B" -n 1 | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Check the ".NET 10 packages bump" commit to understand the upgrade
echo "=== Details of 'Bump pretty much ALL packages for .NET 10' commit ==="
git show --stat b1ab8210 | head -50
echo "=== Check if EF Core version was explicitly updated in that commit ==="
git show b1ab8210 -- "*csproj" | grep -i "entityframework" | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 3135
🏁 Script executed:
#!/bin/bash
# Check the PR `#2264` for context about the upgrade
echo "=== Check for any mention of EF Core 10 in commits around package bumping ==="
git log --format="%H %s" --all | grep -i "bump\|package" | head -10
# Get more details on the actual version bumps
echo "=== Check Directory.Build.props or paket files for version management ==="
fd -e "paket\|props" -x grep -l "EntityFrameworkCore\|dotnet-version" {} \;Repository: sillsdev/languageforge-lexbox
Length of output: 1098
Remove EF Core version from this PR—it was already upgraded in a prior commit.
The ProductVersion change to 10.0.8 occurred in commit #2264 ("Bump pretty much ALL packages for .NET 10"), which is a separate upgrade from the current PR. This snapshot file should not have been modified by the current changes. The ComplexProperty call at line 554 is not an API signature change—it's standard EF Core 10 migration snapshot syntax. Verify that only picture-support-related modifications are included in this PR.
🤖 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/Migrations/LcmCrdtDbContextModelSnapshot.cs` at line
19, The ProductVersion annotation in the LcmCrdtDbContextModelSnapshot.cs file
has been updated to 10.0.8, but this EF Core upgrade was already applied in a
prior commit and should not be included in the current PR. Revert the
ProductVersion value in the modelBuilder.HasAnnotation call back to its original
version so that this PR contains only picture-support-related changes and
excludes the unrelated EF Core version upgrade.
| public virtual MediaUri MediaUri { get; set; } | ||
| public virtual RichMultiString Caption { get; set; } = []; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
proj="$(fd -i 'LcmCrdt.csproj' backend | head -n1)"
dotnet build "$proj" -warnaserrorRepository: sillsdev/languageforge-lexbox
Length of output: 120
🏁 Script executed:
cd backend/FwLite/MiniLcm/Models && cat -n Picture.csRepository: sillsdev/languageforge-lexbox
Length of output: 1285
🏁 Script executed:
# Check the project file for nullable reference type settings
cat -n backend/FwLite/MiniLcm/MiniLcm.csproj | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 1023
🏁 Script executed:
# Look at other MiniLcm model files to see how they handle non-nullable properties
fd -t f '\.cs$' backend/FwLite/MiniLcm/Models/ -exec basename {} \; | head -10Repository: sillsdev/languageforge-lexbox
Length of output: 225
🏁 Script executed:
# Correct fd command to list model files
fd -t f '\.cs$' backend/FwLite/MiniLcm/Models/Repository: sillsdev/languageforge-lexbox
Length of output: 1045
🏁 Script executed:
# Check other model files to see initialization patterns
for file in $(fd -t f '\.cs$' backend/FwLite/MiniLcm/Models/ | head -5); do
echo "=== $(basename $file) ==="
grep -n "public virtual.*{ get; set; }" "$file" | head -5
doneRepository: sillsdev/languageforge-lexbox
Length of output: 861
🏁 Script executed:
# Check for nullable directive in the MiniLcm project
grep -r "nullable" backend/FwLite/MiniLcm/ --include="*.cs" --include="*.csproj" | head -10Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Look at the MediaUri class definition
fd -t f '\.cs$' backend/FwLite/MiniLcm/ -exec grep -l "class MediaUri\|record MediaUri" {} \;Repository: sillsdev/languageforge-lexbox
Length of output: 299
🏁 Script executed:
# Check the parent directory for Directory.Build.props or similar global settings
cat -n backend/Directory.Build.props 2>/dev/null || echo "File not found"Repository: sillsdev/languageforge-lexbox
Length of output: 2901
🏁 Script executed:
# Find MediaUri definition
rg "class MediaUri|record MediaUri|struct MediaUri" backend/FwLite/MiniLcm/Repository: sillsdev/languageforge-lexbox
Length of output: 255
🏁 Script executed:
# Check how Picture is instantiated/initialized in the codebase
rg "new Picture" backend/FwLite/ -B 2 -A 3 | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 2384
🏁 Script executed:
# Check Sense.cs to see another model pattern
cat -n backend/FwLite/MiniLcm/Models/Sense.cs | head -40Repository: sillsdev/languageforge-lexbox
Length of output: 1914
🏁 Script executed:
# Check if Picture has any constructor that initializes MediaUri
rg "Picture()" backend/FwLite/ -B 3 -A 5Repository: sillsdev/languageforge-lexbox
Length of output: 50385
🏁 Script executed:
# Check if MediaUri has a default/empty constructor
cat -n backend/FwLite/MiniLcm/Media/MediaUri.cs | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 1230
🏁 Script executed:
# Check if Picture has any constructor that might initialize MediaUri
cat -n backend/FwLite/MiniLcm/Models/Picture.csRepository: sillsdev/languageforge-lexbox
Length of output: 1285
🏁 Script executed:
# Check how other similar value types are initialized in the codebase
rg "public virtual.*{ get; set; } = " backend/FwLite/MiniLcm/Models/ --type cs | head -15Repository: sillsdev/languageforge-lexbox
Length of output: 206
🏁 Script executed:
# Check how Picture instances are created in tests
rg "new Picture\s*{" backend/FwLite/LcmCrdt.Tests/ -A 5 | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 167
🏁 Script executed:
# Check how Picture instances are created in implementation
rg "new Picture\s*{" backend/FwLite/LcmCrdt/ -A 5 | head -30Repository: sillsdev/languageforge-lexbox
Length of output: 167
🏁 Script executed:
# Check if there are any nullable suppression directives used for MediaUri
rg "MediaUri\?" backend/FwLite/MiniLcm/Models/Picture.csRepository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Look at how other non-nullable properties without initializers are handled
rg "public virtual.*\{ get; set; \}" backend/FwLite/MiniLcm/Models/Entry.csRepository: sillsdev/languageforge-lexbox
Length of output: 832
🏁 Script executed:
# Verify all Picture creations set MediaUri explicitly
rg "new Picture" backend/FwLite -A 10 | grep -E "(new Picture|MediaUri =)" | head -20Repository: sillsdev/languageforge-lexbox
Length of output: 871
🏁 Script executed:
# Check if any code relies on MediaUri being uninitialized/null
rg "MediaUri\s*==" backend/FwLite -B 2 -A 2Repository: sillsdev/languageforge-lexbox
Length of output: 55
🏁 Script executed:
# Check if the test file compiles or if there's a nullable warning about MediaUri
rg "new Picture.*Caption" backend/FwLite/LcmCrdt.Tests/ -A 1 -B 1Repository: sillsdev/languageforge-lexbox
Length of output: 456
🏁 Script executed:
# Examine what properties are marked as required in the codebase
rg "public virtual required" backend/FwLite/MiniLcm/Models/Repository: sillsdev/languageforge-lexbox
Length of output: 944
Initialize or mark MediaUri as required.
MediaUri is declared non-nullable but not initialized, which violates nullable reference type constraints enabled globally with WarningsAsErrors. The test case in UseChangesTests.cs creates Picture without setting MediaUri, which triggers CS8618 errors. Either mark the property as required (consistent with other mandatory properties in the codebase like WritingSystem, ComplexFormComponent) to enforce initialization at construction, or initialize it with a default value.
🤖 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/MiniLcm/Models/Picture.cs` around lines 11 - 12, The MediaUri
property in the Picture class is declared as non-nullable but is not initialized
or marked as required, causing CS8618 compilation errors. Fix this by adding the
`required` keyword to the MediaUri property declaration to enforce
initialization at construction time, making it consistent with other mandatory
properties like WritingSystem and ComplexFormComponent in the codebase.
Source: Coding guidelines
| // TODO: Determine if MediaUri.ToString() is correct here | ||
| patchDocument.Operations.AddRange(SimpleStringDiff.GetStringDiff<Picture>( | ||
| nameof(Picture.MediaUri), | ||
| beforePicture.MediaUri.ToString(), | ||
| afterPicture.MediaUri.ToString())); |
There was a problem hiding this comment.
Resolve MediaUri canonical mapping before emitting patch ops.
DiffToUpdate currently diffs Picture.MediaUri via ToString() while the code itself flags uncertainty. In this PR context, MediaUri is explicitly the non-trivial mapping; patching raw stringified values risks persisting non-canonical media identifiers and causing sync drift across layers.
🤖 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/MiniLcm/SyncHelpers/PictureSync.cs` around lines 40 - 44, The
code is currently diffing Picture.MediaUri by calling ToString() directly on the
URI objects without resolving them to their canonical form first, as indicated
by the TODO comment. To fix this, before calling SimpleStringDiff.GetStringDiff
with nameof(Picture.MediaUri), resolve beforePicture.MediaUri and
afterPicture.MediaUri to their canonical representations (rather than using
ToString() directly), then pass the canonical string values to the GetStringDiff
method to ensure consistent and correct patch operations.
| private static MiniLcm.Models.Picture ToPicture(Guid senseId, Entities.Picture picture) | ||
| { | ||
| return new MiniLcm.Models.Picture | ||
| { | ||
| Id = picture.Guid, | ||
| Caption = ToRichMultiString(picture.Caption), | ||
| // TODO: Create a LfClassicMediaAdapter that can create MediaUris for LF URLs, then use that rather than returning NotFound | ||
| MediaUri = MediaUri.NotFound, | ||
| }; |
There was a problem hiding this comment.
ToPicture drops persisted file linkage by always returning MediaUri.NotFound.
Line 361 ignores Entities.Picture.FileName, so GetPicture can never return a usable media reference even when LF has one persisted.
🤖 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/LfClassicData/LfClassicMiniLcmApi.cs` around lines 354 - 362, The
ToPicture method hardcodes MediaUri.NotFound regardless of whether the picture
has a persisted filename, causing it to ignore the FileName property from the
Entities.Picture object. Replace the hardcoded MediaUri.NotFound assignment with
logic that checks if picture.FileName exists and constructs an appropriate
MediaUri from it; only use MediaUri.NotFound as a fallback when no filename is
present.
1 participant
WIP for #2232 and #2233.
MiniLcm API for picture support now contains the skeleton of what it needs to do, but one major feature is still lacking: correct MediaUri handling (which is the heart of the feature). It's not nearly as direct a translation from FW Classic to FW Lite, so I've implemented the easy stuff first. MediaUri handling will come next.