From 4f307f029b3ab1695cef6c125b1040e7003e0fdd Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 23 Oct 2026 16:50:29 +0200 Subject: [PATCH 1/5] Stop the activity detail view crashing on opaque changes An opaque (unregistered) change has no snapshot when it's a create (the entity was never materialized on this client), so LoadChangeContext threw ArgumentNullException from GetAtCommit. Return an empty context for opaque changes instead: "Unknown" name, no preview, no affected entries. Opaque edits get the empty context too; their snapshot would show the entity state excluding the very change being viewed. The tests opt into UnknownChangeHandling.Fallback through a new fixture hook. Production keeps the default Throw for now, so unknown change types fail sync outright and never reach the activity view. Co-Authored-By: Claude Opus 5.5 --- .../HistoryServiceActivityTestsBase.cs | 4 +- .../HistoryServiceOpaqueChangeTests.cs | 67 +++++++++++++++++++ .../FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs | 25 +++++-- backend/FwLite/LcmCrdt/HistoryService.cs | 5 ++ 4 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs diff --git a/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTestsBase.cs b/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTestsBase.cs index 5b06820bc7..793bd854ef 100644 --- a/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTestsBase.cs +++ b/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTestsBase.cs @@ -20,10 +20,12 @@ public abstract class HistoryServiceActivityTestsBase : IAsyncLifetime, IAsyncDi public async Task InitializeAsync() { - _fixture = MiniLcmApiFixture.Create(); + _fixture = CreateFixture(); await _fixture.InitializeAsync(); } + protected virtual MiniLcmApiFixture CreateFixture() => MiniLcmApiFixture.Create(); + public async Task DisposeAsync() => await _fixture.DisposeAsync(); async ValueTask IAsyncDisposable.DisposeAsync() => await DisposeAsync(); diff --git a/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs b/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs new file mode 100644 index 0000000000..3547da5708 --- /dev/null +++ b/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs @@ -0,0 +1,67 @@ +using System.Text.Json; +using Microsoft.Extensions.DependencyInjection; +using SIL.Harmony.Changes; +using SIL.Harmony.Config; + +namespace LcmCrdt.Tests; + +// With UnknownChangeHandling.Fallback, a change type from a newer client that this client doesn't +// have registered deserializes as OpaqueChange (null EntityType, apply skipped) — the activity view +// must tolerate it. Production still uses the default Throw (sync fails instead); only this fixture +// opts into Fallback, covering the activity view for when it's turned on. +public class HistoryServiceOpaqueChangeTests : HistoryServiceActivityTestsBase +{ + protected override MiniLcmApiFixture CreateFixture() => MiniLcmApiFixture.Create(configureServices: services => + services.Configure(config => config.UnknownChangeHandling = UnknownChangeHandling.Fallback)); + + private static OpaqueChange UnknownChange(Guid entityId) + { + using var doc = JsonDocument.Parse( + $$"""{"$type":"ChangeFromTheFuture","EntityId":"{{entityId}}","SomeValue":7}"""); + return new OpaqueChange + { + TypeName = "ChangeFromTheFuture", + EntityId = entityId, + RawJson = doc.RootElement.Clone() + }; + } + + [Fact] + public async Task ProjectActivity_ToleratesOpaqueChanges() + { + var entryId = await CreateEntry("apple"); + await DataModel.AddChange(ClientId, UnknownChange(entryId), Meta()); + + var activities = await Service.ProjectActivity(); + + var activity = activities.Should().ContainSingle(a => a.Changes.Any(c => c.Entity.Change is OpaqueChange)).Subject; + activity.ChangeName.Should().Be("Unknown"); + activity.Changes.Single().Info.Subject.Should().BeNull(); + } + + [Fact] + public async Task LoadChangeContext_ToleratesOpaqueEditOfExistingEntity() + { + var entryId = await CreateEntry("apple"); + var commit = await DataModel.AddChange(ClientId, UnknownChange(entryId), Meta()); + + var context = await Service.LoadChangeContext(commit.Id, 0); + + context.ChangeName.Should().Be("Unknown"); + context.Snapshot.Should().BeNull(); + context.AffectedEntries.Should().BeEmpty(); + } + + [Fact] + public async Task LoadChangeContext_ToleratesOpaqueCreateOfUnknownEntity() + { + // An opaque create: the entity was never materialized on this client, so it has no snapshots. + var commit = await DataModel.AddChange(ClientId, UnknownChange(Guid.NewGuid()), Meta()); + + var context = await Service.LoadChangeContext(commit.Id, 0); + + context.ChangeName.Should().Be("Unknown"); + context.Snapshot.Should().BeNull(); + context.AffectedEntries.Should().BeEmpty(); + } +} diff --git a/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs b/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs index 06b3e33c0a..1e0b4c2711 100644 --- a/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs +++ b/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs @@ -2,6 +2,8 @@ using System.Diagnostics; using LcmCrdt.Changes; using LcmCrdt.MediaServer; +using LinqToDB; +using LinqToDB.EntityFrameworkCore; using Meziantou.Extensions.Logging.Xunit; using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; @@ -17,6 +19,7 @@ public class MiniLcmApiFixture : IAsyncLifetime, IAsyncDisposable { private readonly bool _seedWs = true; private readonly Guid? _projectId; + private readonly Action? _configureServices; private AsyncServiceScope _services; private LcmCrdtDbContext? _crdtDbContext; private SqliteConnection? _keepAliveConnection; @@ -35,15 +38,16 @@ public MiniLcmApiFixture() { } - public static MiniLcmApiFixture Create(bool seedWs = true, Guid? projectId = null) + public static MiniLcmApiFixture Create(bool seedWs = true, Guid? projectId = null, Action? configureServices = null) { - return new MiniLcmApiFixture(seedWs, projectId); + return new MiniLcmApiFixture(seedWs, projectId, configureServices); } - private MiniLcmApiFixture(bool seedWs = true, Guid? projectId = null) + private MiniLcmApiFixture(bool seedWs = true, Guid? projectId = null, Action? configureServices = null) { _seedWs = seedWs; _projectId = projectId; + _configureServices = configureServices; } public async Task InitializeAsync() @@ -65,13 +69,22 @@ public async Task InitializeAsync(string projectName, Guid? projectId = null) } } - var services = new ServiceCollection() + var serviceCollection = new ServiceCollection() .AddTestLcmCrdtClient() .AddLogging(builder => builder.AddDebug() .AddProvider(new LateXUnitLoggerProvider(this)) .AddFilter("LinqToDB", LogLevel.Trace) - .SetMinimumLevel(LogLevel.Error)) - .BuildServiceProvider(); + .SetMinimumLevel(LogLevel.Error)); + if (_configureServices is not null) + { + _configureServices(serviceCollection); + // Hook config like HarmonyConfig gets baked into the EF model and linq2db's compiled queries, both + // cached process-wide (first fixture wins). Opt this fixture out of both caches so its config applies. + serviceCollection.AddDbContextFactory(builder => builder + .EnableServiceProviderCaching(false) + .UseLinqToDB(linq2db => linq2db.AddCustomOptions(options => options.UseDisableQueryCache(true)))); + } + var services = serviceCollection.BuildServiceProvider(); _services = services.CreateAsyncScope(); if (inMemory) { diff --git a/backend/FwLite/LcmCrdt/HistoryService.cs b/backend/FwLite/LcmCrdt/HistoryService.cs index 0678b2ae0c..f9f5ef31c7 100644 --- a/backend/FwLite/LcmCrdt/HistoryService.cs +++ b/backend/FwLite/LcmCrdt/HistoryService.cs @@ -310,6 +310,11 @@ public async Task LoadChangeContext(Guid commitId, int changeInde .FirstOrDefaultAsync() ?? throw new InvalidOperationException($"Change {changeIndex} not found in commit {commitId}"); + // An OpaqueChange is a change type this client doesn't have registered: we can't tell what it did, + // and an opaque create has no snapshot to load (the entity was never materialized here). + if (change.Change is OpaqueChange) + return new ChangeContext(change, null, null, []); + // These branches are independent read-only lookups that each open their own // DbContext (via the repository factories), so run them concurrently. //todo needs optimizing: when a sync pruned this change's snapshot, this replays every commit since the entity's last one (median 22k on a 100k-commit project) From e2ed6c507f56c59923403b0627812f329eff8a2a Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 23 Oct 2026 16:49:27 +0200 Subject: [PATCH 2/5] Check every change type opens in the activity detail view Runs the full change catalogue through LoadChangeContext: no change may throw, and the detail context must link the owning entry the list row names. Comment types are allowlisted since the preview has no comment layout. Co-Authored-By: Claude Opus 5.5 --- ...ActivityChangeInfoResolverCoverageTests.cs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs b/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs index 953adf2cf5..a6015904eb 100644 --- a/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs @@ -55,6 +55,46 @@ public async Task EveryChangeEntityType_ResolvesASubject_OrIsIntentionallyDegrad } } + // Entity types whose list row names an owning entry that the detail view doesn't link to + private static readonly Dictionary DetailOmitsOwningEntry = new() + { + ["CommentThread"] = "The detail preview has no comment layout; linking the entry would show it as deleted.", + ["UserComment"] = "The detail preview has no comment layout; linking the entry would show it as deleted.", + }; + + [Fact] + public async Task EveryChange_LoadsADetailContext_LinkedToTheListRowsOwningEntry() + { + await DataModel.AddChanges(ClientId, UseChangesTests.AllChangesInDependencyOrder()); + var activities = await Service.ProjectActivity(0, take: 1000, new ActivityQuery()); + + var omitted = new HashSet(); + using var _ = new AssertionScope(); + foreach (var activity in activities) + { + foreach (var change in activity.Changes) + { + var name = EntityTypeName(change.Entity.Change.EntityType); + var changeName = change.Entity.Change.GetType().Name; + var context = (await Service.Awaiting(s => s.LoadChangeContext(activity.CommitId, change.Entity.Index)) + .Should().NotThrowAsync($"the detail view must open for {changeName}")).Which; + if (context is null || change.Info.OwningEntryId is not { } owningEntryId) continue; + + if (DetailOmitsOwningEntry.ContainsKey(name)) + { + if (context.AffectedEntries.All(e => e.Id != owningEntryId)) omitted.Add(name); + continue; + } + context.AffectedEntries.Select(e => e.Id).Should().Contain(owningEntryId, + $"the list row for {changeName} names an owning entry, so the detail view should link to it; " + + $"handle '{name}' in HistoryService.GetAffectedEntryIds or list it in {nameof(DetailOmitsOwningEntry)} with a reason"); + } + } + + omitted.Should().BeEquivalentTo(DetailOmitsOwningEntry.Keys, + $"each type in {nameof(DetailOmitsOwningEntry)} must be exercised and still omit its owning entry"); + } + // Entity type name without the CLR generic-arity suffix, so a generic entity (RemoteResource) // reads as "RemoteResource". The resolved entity types are all non-generic, so stripping is harmless there. // EntityType is null for opaque/unknown changes; none are expected in these fixtures, so bucket them From 8c4926f02835110370fea028d0e055e20ead8434 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 23 Oct 2026 17:12:01 +0200 Subject: [PATCH 3/5] Fail the detail-coverage allowlist as soon as any change links its entry Previously an allowlisted type passed if any one of its changes still omitted the owning entry, hiding a change that had started linking it. Co-Authored-By: Claude Opus 5.5 --- .../ActivityChangeInfoResolverCoverageTests.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs b/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs index a6015904eb..df6dbee070 100644 --- a/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs @@ -82,7 +82,9 @@ public async Task EveryChange_LoadsADetailContext_LinkedToTheListRowsOwningEntry if (DetailOmitsOwningEntry.ContainsKey(name)) { - if (context.AffectedEntries.All(e => e.Id != owningEntryId)) omitted.Add(name); + context.AffectedEntries.Select(e => e.Id).Should().NotContain(owningEntryId, + $"'{name}' is listed in {nameof(DetailOmitsOwningEntry)} but {changeName} now links its owning entry; remove it from the list"); + omitted.Add(name); continue; } context.AffectedEntries.Select(e => e.Id).Should().Contain(owningEntryId, @@ -92,7 +94,7 @@ public async Task EveryChange_LoadsADetailContext_LinkedToTheListRowsOwningEntry } omitted.Should().BeEquivalentTo(DetailOmitsOwningEntry.Keys, - $"each type in {nameof(DetailOmitsOwningEntry)} must be exercised and still omit its owning entry"); + $"each type in {nameof(DetailOmitsOwningEntry)} must be exercised"); } // Entity type name without the CLR generic-arity suffix, so a generic entity (RemoteResource) From 452779c59d967cd46e97397a0bf1fe13d4fef409 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 24 Oct 2026 18:29:56 +0000 Subject: [PATCH 4/5] Match the opaque fixture's DbContext factory lifetime to the kernel's The second AddDbContextFactory call defaulted to Singleton while LcmCrdtKernel registers it Scoped, so it added a root-scoped options descriptor beside the scoped one. Also explains why opaque edits get an empty detail context, not just opaque creates. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V3AhdPwmhYofmWwThhKavX --- .../FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs | 2 +- backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs | 3 ++- backend/FwLite/LcmCrdt/HistoryService.cs | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs b/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs index 3547da5708..53d29bcba5 100644 --- a/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/HistoryServiceOpaqueChangeTests.cs @@ -6,7 +6,7 @@ namespace LcmCrdt.Tests; // With UnknownChangeHandling.Fallback, a change type from a newer client that this client doesn't -// have registered deserializes as OpaqueChange (null EntityType, apply skipped) — the activity view +// have registered deserializes as OpaqueChange (null EntityType, apply skipped), and the activity view // must tolerate it. Production still uses the default Throw (sync fails instead); only this fixture // opts into Fallback, covering the activity view for when it's turned on. public class HistoryServiceOpaqueChangeTests : HistoryServiceActivityTestsBase diff --git a/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs b/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs index 1e0b4c2711..f007d94e37 100644 --- a/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs +++ b/backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs @@ -82,7 +82,8 @@ public async Task InitializeAsync(string projectName, Guid? projectId = null) // cached process-wide (first fixture wins). Opt this fixture out of both caches so its config applies. serviceCollection.AddDbContextFactory(builder => builder .EnableServiceProviderCaching(false) - .UseLinqToDB(linq2db => linq2db.AddCustomOptions(options => options.UseDisableQueryCache(true)))); + .UseLinqToDB(linq2db => linq2db.AddCustomOptions(options => options.UseDisableQueryCache(true))), + ServiceLifetime.Scoped); } var services = serviceCollection.BuildServiceProvider(); _services = services.CreateAsyncScope(); diff --git a/backend/FwLite/LcmCrdt/HistoryService.cs b/backend/FwLite/LcmCrdt/HistoryService.cs index f9f5ef31c7..86ed08713c 100644 --- a/backend/FwLite/LcmCrdt/HistoryService.cs +++ b/backend/FwLite/LcmCrdt/HistoryService.cs @@ -310,8 +310,8 @@ public async Task LoadChangeContext(Guid commitId, int changeInde .FirstOrDefaultAsync() ?? throw new InvalidOperationException($"Change {changeIndex} not found in commit {commitId}"); - // An OpaqueChange is a change type this client doesn't have registered: we can't tell what it did, - // and an opaque create has no snapshot to load (the entity was never materialized here). + // Opaque create: no snapshot, the entity was never materialized here. Opaque edit: the snapshot + // predates the change being viewed, so it would mislead. Show nothing for both. if (change.Change is OpaqueChange) return new ChangeContext(change, null, null, []); From 5f7b6bf04f19d35c008d05cecbbd47ece08c25fa Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 24 Oct 2026 18:30:12 +0000 Subject: [PATCH 5/5] Link comment changes in the activity detail view to the commented entry The activity list already groups comment-thread and user-comment changes under the entry they're about, but the detail view had no affected entry for them. The coverage test papered over that with an allowlist; it now just asserts that every change's detail links the entry its list row names. The preview showed "Deleted" for any snapshot type it can't find inside the affected entry, so the current-version toggle is now limited to the types it can (Entry, Sense, ExampleSentence); the timestamp it also carried is already shown by ActivityItem. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01V3AhdPwmhYofmWwThhKavX --- ...ActivityChangeInfoResolverCoverageTests.cs | 24 +-------- .../HistoryServiceActivityTests.cs | 14 +++++ backend/FwLite/LcmCrdt/HistoryService.cs | 53 +++++++++++++------ .../activity/ActivityItemChangePreview.svelte | 30 ++++++----- 4 files changed, 69 insertions(+), 52 deletions(-) diff --git a/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs b/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs index df6dbee070..826b31bcb6 100644 --- a/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs @@ -55,46 +55,26 @@ public async Task EveryChangeEntityType_ResolvesASubject_OrIsIntentionallyDegrad } } - // Entity types whose list row names an owning entry that the detail view doesn't link to - private static readonly Dictionary DetailOmitsOwningEntry = new() - { - ["CommentThread"] = "The detail preview has no comment layout; linking the entry would show it as deleted.", - ["UserComment"] = "The detail preview has no comment layout; linking the entry would show it as deleted.", - }; - [Fact] - public async Task EveryChange_LoadsADetailContext_LinkedToTheListRowsOwningEntry() + public async Task EveryChange_OpensADetailContext_LinkedToTheListRowsOwningEntry() { await DataModel.AddChanges(ClientId, UseChangesTests.AllChangesInDependencyOrder()); var activities = await Service.ProjectActivity(0, take: 1000, new ActivityQuery()); - var omitted = new HashSet(); using var _ = new AssertionScope(); foreach (var activity in activities) { foreach (var change in activity.Changes) { - var name = EntityTypeName(change.Entity.Change.EntityType); var changeName = change.Entity.Change.GetType().Name; var context = (await Service.Awaiting(s => s.LoadChangeContext(activity.CommitId, change.Entity.Index)) .Should().NotThrowAsync($"the detail view must open for {changeName}")).Which; if (context is null || change.Info.OwningEntryId is not { } owningEntryId) continue; - if (DetailOmitsOwningEntry.ContainsKey(name)) - { - context.AffectedEntries.Select(e => e.Id).Should().NotContain(owningEntryId, - $"'{name}' is listed in {nameof(DetailOmitsOwningEntry)} but {changeName} now links its owning entry; remove it from the list"); - omitted.Add(name); - continue; - } context.AffectedEntries.Select(e => e.Id).Should().Contain(owningEntryId, - $"the list row for {changeName} names an owning entry, so the detail view should link to it; " + - $"handle '{name}' in HistoryService.GetAffectedEntryIds or list it in {nameof(DetailOmitsOwningEntry)} with a reason"); + $"the list row for {changeName} names an owning entry, so the detail view should link to it (HistoryService.OwningEntryId)"); } } - - omitted.Should().BeEquivalentTo(DetailOmitsOwningEntry.Keys, - $"each type in {nameof(DetailOmitsOwningEntry)} must be exercised"); } // Entity type name without the CLR generic-arity suffix, so a generic entity (RemoteResource) diff --git a/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs b/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs index 9e8ac62767..7404020144 100644 --- a/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs @@ -18,6 +18,20 @@ public async Task ListActivityAuthors_ReturnsDistinctAuthorsWithCounts() authors.Should().Contain(a => a.AuthorId == "bob-id" && a.AuthorName == "Bob" && a.CommitCount == 1); } + [Fact] + public async Task LoadChangeContext_LinksACommentOnASenseToItsEntry() + { + var entryId = await CreateEntry("apple"); + var senseId = await CreateSense(entryId, "fruit", 1); + var threadId = await AddCommentThread(senseId, SubjectType.Sense); + await AddComment(threadId, "looks wrong"); + var commit = (await Service.ProjectActivity()).Single(a => a.ChangeName == "Create user comment").CommitId; + + var context = await Service.LoadChangeContext(commit, 0); + + context.AffectedEntries.Should().ContainSingle().Which.Id.Should().Be(entryId); + } + [Fact] public async Task ListActivityChangeTypes_IncludesCreateEntry() { diff --git a/backend/FwLite/LcmCrdt/HistoryService.cs b/backend/FwLite/LcmCrdt/HistoryService.cs index 86ed08713c..3bf4ca6336 100644 --- a/backend/FwLite/LcmCrdt/HistoryService.cs +++ b/backend/FwLite/LcmCrdt/HistoryService.cs @@ -366,31 +366,50 @@ private async Task GetCurrentOrLatestEntry(Guid entryId) private async IAsyncEnumerable GetAffectedEntryIds(ChangeEntity changeEntity) { - if (changeEntity.Change.EntityType == typeof(Entry)) + if (changeEntity.Change.EntityType == typeof(ComplexFormComponent)) { - yield return changeEntity.EntityId; + var cfc = await dataModel.GetLatest(changeEntity.EntityId) + ?? throw NotFoundException.ForType(changeEntity.EntityId); + yield return cfc.ComplexFormEntryId; + yield return cfc.ComponentEntryId; } - else if (changeEntity.Change.EntityType == typeof(Sense)) + else if (await OwningEntryId(changeEntity.Change.EntityType, changeEntity.EntityId) is { } entryId) { - var sense = await dataModel.GetLatest(changeEntity.EntityId) - ?? throw NotFoundException.ForType(changeEntity.EntityId); - yield return sense.EntryId; + yield return entryId; } - else if (changeEntity.Change.EntityType == typeof(ExampleSentence)) + } + + // Must agree with ActivityChangeInfoResolver's OwningEntryId, which the activity list shows for the same change. + private async Task OwningEntryId(Type? entityType, Guid entityId) + { + if (entityType == typeof(Entry)) return entityId; + if (entityType == typeof(Sense)) { - var example = await dataModel.GetLatest(changeEntity.EntityId) - ?? throw NotFoundException.ForType(changeEntity.EntityId); - var sense = await dataModel.GetLatest(example.SenseId) - ?? throw NotFoundException.ForType(example.SenseId); - yield return sense.EntryId; + var sense = await dataModel.GetLatest(entityId) ?? throw NotFoundException.ForType(entityId); + return sense.EntryId; } - else if (changeEntity.Change.EntityType == typeof(ComplexFormComponent)) + if (entityType == typeof(ExampleSentence)) { - var cfc = await dataModel.GetLatest(changeEntity.EntityId) - ?? throw NotFoundException.ForType(changeEntity.EntityId); - yield return cfc.ComplexFormEntryId; - yield return cfc.ComponentEntryId; + var example = await dataModel.GetLatest(entityId) ?? throw NotFoundException.ForType(entityId); + return await OwningEntryId(typeof(Sense), example.SenseId); + } + if (entityType == typeof(CommentThread)) + { + var thread = await dataModel.GetLatest(entityId) ?? throw NotFoundException.ForType(entityId); + return thread.SubjectType switch + { + SubjectType.Entry => await OwningEntryId(typeof(Entry), thread.SubjectId), + SubjectType.Sense => await OwningEntryId(typeof(Sense), thread.SubjectId), + SubjectType.ExampleSentence => await OwningEntryId(typeof(ExampleSentence), thread.SubjectId), + _ => null, + }; + } + if (entityType == typeof(UserComment)) + { + var comment = await dataModel.GetLatest(entityId) ?? throw NotFoundException.ForType(entityId); + return await OwningEntryId(typeof(CommentThread), comment.CommentThreadId); } + return null; } public static string ChangesNameHelper(IReadOnlyList changes) diff --git a/frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte b/frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte index 86e5ea6386..79d6ad40f7 100644 --- a/frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte +++ b/frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte @@ -30,6 +30,8 @@ const viewService = useViewService(); const affectedEntry = $derived(context.affectedEntries.length === 1 ? context.affectedEntries[0] : undefined); + // Only these snapshot types can be found again inside the affected entry, so only they get the current-version toggle + const hasCurrentVersion = $derived(['Entry', 'Sense', 'ExampleSentence'].includes(context.entityType)); let currentEntity = $derived.by(() => { if (!affectedEntry || !context.snapshot) return undefined; @@ -102,19 +104,21 @@ {@render entryButton(entry)} {/if} - + {#if hasCurrentVersion} + + {/if} {/if}