Update SIL.Harmony to rc.234 and rename CrdtConfig to HarmonyConfig - #2482
Update SIL.Harmony to rc.234 and rename CrdtConfig to HarmonyConfig#2482hahn-kev-bot wants to merge 7 commits into
Conversation
The new Harmony release renames CrdtConfig to SIL.Harmony.Config.HarmonyConfig and changes several APIs. This adapts LexBox to compile and run against it: - Add a `global using CrdtConfig = SIL.Harmony.Config.HarmonyConfig;` shim to each project that references CrdtConfig, so existing references keep compiling (transitional; a full rename can follow). - ConfigureCrdt no longer reads config.JsonSerializerOptions directly — Harmony now freezes the ChangeTypeListBuilder the first time the options are built, which threw "ChangeTypeListBuilder is frozen" before AddRemoteResourceEntity. Attach the ExampleSentence translation modifier via the new deferred config.ConfigureJsonOptions hook instead. - ChangeTypes now yields RegisteredChangeType descriptors; project to .Type in AllChangeTypes and ConfigRegistrationTests. - IChange polymorphism moved out of the type-info resolver into an internal JsonConverter Harmony only adds to its own JsonSerializerOptions. Add MakeLcmCrdtExternalJsonOptions, which copies those converters onto the Web-cased sync options, and use it for the Refit client. Without it, decoding a server changes response threw "Deserialization of interface or abstract types is not supported. Type 'SIL.Harmony.Changes.IChange'". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lists - Add LcmCrdtKernel.AllRegisteredChanges() returning Harmony's RegisteredChangeType descriptors directly; AllChangeTypes() now just projects .Type off it. - ChangeTypesCodeGenerator and HistoryService.ListActivityChangeTypes now read the authoritative $type discriminator from RegisteredChangeType.Discriminator instead of re-deriving it, so the generated list and activity keys can't drift from what the serializer actually writes. - Remove HistoryService.GetChangeTypeKeyFromType, which existed only to share that now-unnecessary derivation. Generated TS is unchanged (discriminators identical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the transitional `global using CrdtConfig = HarmonyConfig` shims with the real rename now that we build against the renamed SIL.Harmony type: - Rename every CrdtConfig type reference to HarmonyConfig and add the `using SIL.Harmony.Config;` import the alias was masking. - Rename the corresponding `crdtConfig` variables/parameters/fields (and their doc comments) to `harmonyConfig`. LcmCrdtConfig — an unrelated local type — is left untouched, including WebTroubleshootingService's `crdtConfig` which holds an LcmCrdtConfig. - Delete the nine HarmonyConfigAlias.cs shim files. Purely mechanical; no behavior change. Generated TypeScript is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe PR updates Harmony package versions, replaces FwLite’s CrdtConfig usage with HarmonyConfig across service wiring and JSON handling, adds registered-change enumeration, updates change-type consumers, and aligns tests and harnesses with the new configuration shape. HarmonyConfig migration
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/FwLiteWeb/FwLiteWebKernel.cs`:
- Around line 33-35: The web and debugger sync JSON configuration only copies
Harmony’s type resolver, omitting its converters needed for IChange
polymorphism. Update the JsonOptions configuration in FwLiteWebKernel.cs and the
web-options setup in FakeSyncSource.cs to reuse MakeLcmCrdtExternalJsonOptions()
or copy its JsonSerializerOptions.Converters, and add web/debugger coverage for
a sync payload containing ChangeEntity<IChange>.
🪄 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 Plus
Run ID: 30347e29-b886-440e-88a7-b68949850979
📒 Files selected for processing (24)
backend/Directory.Packages.propsbackend/FwLite/FwLiteMaui/FwLiteMauiKernel.csbackend/FwLite/FwLiteProjectSync/ProjectSnapshotService.csbackend/FwLite/FwLiteShared/FwLiteSharedKernel.csbackend/FwLite/FwLiteShared/Services/FwLiteJson.csbackend/FwLite/FwLiteShared/TypeGen/ChangeTypesCodeGenerator.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/FwLiteWeb/FwLiteWebKernel.csbackend/FwLite/LcmCrdt.Tests/ConfigRegistrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests.csbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.csbackend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.csbackend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.csbackend/FwLite/LcmCrdt.Tests/TestJsonOptions.csbackend/FwLite/LcmCrdt/CrdtProjectsService.csbackend/FwLite/LcmCrdt/Data/SetupCollationInterceptor.csbackend/FwLite/LcmCrdt/HistoryService.csbackend/FwLite/LcmCrdt/Json.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/MediaServer/LcmMediaService.csbackend/FwLite/LcmCrdt/SnapshotAtCommitService.csbackend/FwLite/LcmDebugger/FakeSyncSource.csbackend/Testing/FwHeadless/Services/SyncWorkerTestHarness.cs
| services.AddOptions<JsonOptions>().PostConfigure<IOptions<HarmonyConfig>>((jsonOptions, harmonyConfig) => | ||
| { | ||
| jsonOptions.SerializerOptions.TypeInfoResolver = crdtConfig.Value.MakeLcmCrdtExternalJsonTypeResolver(); | ||
| jsonOptions.SerializerOptions.TypeInfoResolver = harmonyConfig.Value.MakeLcmCrdtExternalJsonTypeResolver(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AGENTS files =="
git ls-files | grep 'AGENTS\.md$' | sed -n '1,80p'
echo "== Locate relevant files =="
fd -a 'FwLiteWebKernel\.cs|FakeSyncSource\.cs|HarmonyConfig\.*|LcmCrdtKernel\.*|JsonOptions' . | sed 's#^\./##'
echo "== Search relevant symbols =="
rg -n "MakeLcmCrdtExternalJsonTypeResolver|MakeLcmCrdtExternalJsonOptions|JsonSerializerOptions|Change.*Converter|IChange|RegisterJsonConverters|Converters" -S .Repository: sillsdev/languageforge-lexbox
Length of output: 45814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend/FwLite/AGENTS.md =="
cat -n backend/FwLite/AGENTS.md
echo "== backend/FwLite/FwLiteWeb/FwLiteWebKernel.cs =="
cat -n backend/FwLite/FwLiteWeb/FwLiteWebKernel.cs
echo "== backend/FwLite/LcmDebugger/FakeSyncSource.cs =="
cat -n backend/FwLite/LcmDebugger/FakeSyncSource.cs
echo "== backend/FwLite/LcmCrdt/Json.cs relevant section =="
cat -n backend/FwLite/LcmCrdt/Json.cs | sed -n '230,275p'
echo "== ChangeSerializationTests relevant section =="
cat -n backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs | sed -n '1,170p'
echo "== Find sync/web tests involving JsonOptions/IChange =="
rg -n "IOptions<JsonOptions>|JsonOptions|IChange|ChangeEntity<IChange>|SyncState|ServerCommit|FromJsonFile" backend/FwLite/FwLiteWeb backend/FwLite/LcmCrdt.Tests backend/FwLite/FwLiteProjectSync.Tests -SRepository: sillsdev/languageforge-lexbox
Length of output: 43916
🏁 Script executed:
#!/bin/bash
set -euo pipefail
dotnet --version 2>/dev/null || true
dotnet --list-sdks 2>/dev/null || true
git ls-files | rg '(\.(csproj|props|targets|slnf|json)$)' | sed -n '1,120p'
echo "== target frameworks and package versions =="
rg -n "<TargetFramework|<TargetFrameworks|<Harmony|SIL\.Harmony|PackageReference" backend/FwLite -S
echo "== project files containing FwLiteWeb and LcmDebugger =="
cat -n backend/FwLite/FwLiteWeb/FwLiteWeb.csproj 2>/dev/null || true
cat -n backend/FwLite/LcmDebugger/LcmDebugger.csproj 2>/dev/null || trueRepository: sillsdev/languageforge-lexbox
Length of output: 50386
Use Harmony’s full Web JSON options when parsing sync payloads.
JsonOptions and FakeSyncSource build Web options but only assign Harmony’s type resolver, so ChangeEntity<IChange> change fields still deserialize without Harmony’s IChange polymorphism converter. Use MakeLcmCrdtExternalJsonOptions() and/or copy JsonSerializerOptions.Converters, and add web/debugger coverage for a sync payload containing IChange.
📍 Affects 2 files
backend/FwLite/FwLiteWeb/FwLiteWebKernel.cs#L33-L35(this comment)backend/FwLite/LcmDebugger/FakeSyncSource.cs#L46-L51
🤖 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/FwLiteWeb/FwLiteWebKernel.cs` around lines 33 - 35, The web
and debugger sync JSON configuration only copies Harmony’s type resolver,
omitting its converters needed for IChange polymorphism. Update the JsonOptions
configuration in FwLiteWebKernel.cs and the web-options setup in
FakeSyncSource.cs to reuse MakeLcmCrdtExternalJsonOptions() or copy its
JsonSerializerOptions.Converters, and add web/debugger coverage for a sync
payload containing ChangeEntity<IChange>.
CodeRabbit flagged that FwLiteWebKernel's JsonOptions and FakeSyncSource built Web options with only Harmony's type resolver, so ChangeEntity<IChange> fields in sync payloads deserialized without Harmony's IChange polymorphism converter (the same break the Refit client hit). - FwLiteWebKernel: copy both the resolver and Harmony's converters from MakeLcmCrdtExternalJsonOptions onto the ASP.NET SerializerOptions. - FakeSyncSource.FromJsonFile and TestJsonOptions.External: use MakeLcmCrdtExternalJsonOptions instead of a resolver-only options object. - Add a regression test round-tripping every change type as IChange through the external options. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Just a few thoughts. Looks good 👍
| var externalOptions = harmonyConfig.Value.MakeLcmCrdtExternalJsonOptions(); | ||
| jsonOptions.SerializerOptions.TypeInfoResolver = externalOptions.TypeInfoResolver; | ||
| foreach (var converter in externalOptions.Converters) | ||
| jsonOptions.SerializerOptions.Converters.Add(converter); |
There was a problem hiding this comment.
I wonder if this should maybe happen inside or in a sibling method of MakeLcmCrdtExternalJsonOptions? It's not obvious here what things from MakeLcmCrdtExternalJsonOptions need to be copied over, but it is obvious where MakeLcmCrdtExternalJsonOptions is defined.
| /// </remarks> | ||
| public static JsonSerializerOptions MakeLcmCrdtExternalJsonOptions(this HarmonyConfig config) | ||
| { | ||
| var options = new JsonSerializerOptions(JsonSerializerDefaults.Web) |
There was a problem hiding this comment.
I just learned about JsonSerializerOptions.MakeReadOnly().
It's probably not really worth anything caching and reusing a read-only copy of these options is it? It looks like only the RefitSettings take the whole object as is, so it presumably wouldn't cost us more memory, but...the benefit is just a tiny bit of startup time I guess?
| /// server response throws "Deserialization of interface or abstract types is not supported. Type | ||
| /// 'SIL.Harmony.Changes.IChange'". | ||
| /// </remarks> | ||
| public static JsonSerializerOptions MakeLcmCrdtExternalJsonOptions(this HarmonyConfig config) |
There was a problem hiding this comment.
Are we gonna later rename LcmCrdt => LcmHarmony? 🙃
| public static IReadOnlyList<RegisteredChangeType> AllRegisteredChanges() | ||
| { | ||
| var harmonyConfig = new HarmonyConfig(); | ||
| ConfigureCrdt(harmonyConfig); |
There was a problem hiding this comment.
Somewhat unrelated to your changes, but:
ConfigureCrdt() looks like a fairly heavy method.
It's invoked for building the list of change types on the history page.
Is it a cache candidate?
There was a problem hiding this comment.
oh, well there we should just be injecting HarmonyConfig and using that since it's already setup in an IOC conatiner. This is really mostly a helper for tests.
| } | ||
|
|
||
| public static void ConfigureCrdt(CrdtConfig config, bool addRemoteResourceEntity = true) | ||
| public static void ConfigureCrdt(HarmonyConfig config, bool addRemoteResourceEntity = true) |
There was a problem hiding this comment.
I suppose there are a bazillion things you could consider renaming.
This is just one example and you don't want to chase them all down, so just rename what you feel like renaming. Crdt is probably still the correct term in some contexts.
| public static void ConfigureCrdt(HarmonyConfig config, bool addRemoteResourceEntity = true) | |
| public static void ConfigureHarmony(HarmonyConfig config, bool addRemoteResourceEntity = true) |
3 participants
Update harmony to the latest version and use the new APIs for getting types.
AI summary
Updates SIL.Harmony
0.2.1-rc.225→rc.234and adapts LexBox to the renamed/reworked config API. Uses the released package only — none of the unreleasedreduce-sync-workprojection changes.Harmony API changes handled
CrdtConfigwas renamed toSIL.Harmony.Config.HarmonyConfig. Renamed all references (types, variables, fields, doc comments) and added theusing SIL.Harmony.Config;import. The unrelatedLcmCrdtConfigtype is untouched.ChangeTypeListBuildernow freezes the first time its JSON options are built, soConfigureCrdtno longer readsJsonSerializerOptionsmid-configuration (that threw"ChangeTypeListBuilder is frozen"beforeAddRemoteResourceEntity). The ExampleSentence translation modifier is attached via the new deferredHarmonyConfig.ConfigureJsonOptionshook instead.HarmonyConfig.ChangeTypesnow yieldsRegisteredChangeTypedescriptors. AddedLcmCrdtKernel.AllRegisteredChanges();AllChangeTypes()projects.Typeoff it.IChangepolymorphism moved out of the type-info resolver into an internalJsonConverterHarmony only adds to its own options. AddedMakeLcmCrdtExternalJsonOptions(), which copies Harmony's converters onto the Web-cased sync options used by the Refit client. Without it, decoding a server changes response throws"Deserialization of interface or abstract types is not supported. Type 'SIL.Harmony.Changes.IChange'".Cleanups
ChangeTypesCodeGeneratorandHistoryService.ListActivityChangeTypesnow use the authoritativeRegisteredChangeType.Discriminatorinstead of re-deriving the$type; removed the now-unusedHistoryService.GetChangeTypeKeyFromType.Generated TypeScript is unchanged (discriminators identical).
Test plan
FwLiteOnly.slnf,FwHeadless,LexBoxApi,Testing,FwLiteMauiLcmCrdt.Testsconfig / serialization / entity-copy suites pass (86 passed, 1 skipped)FwLiteProjectSync.Tests) — recommend confirming in CI