Stop the activity detail view crashing on opaque changes

backend/FwLite/LcmCrdt.Tests/ActivityChangeInfoResolverCoverageTests.cs

@@ -55,6 +55,28 @@ public async Task EveryChangeEntityType_ResolvesASubject_OrIsIntentionallyDegrad
         }
     }
 
+    [Fact]
+    public async Task EveryChange_OpensADetailContext_LinkedToTheListRowsOwningEntry()
+    {
+        await DataModel.AddChanges(ClientId, UseChangesTests.AllChangesInDependencyOrder());
+        var activities = await Service.ProjectActivity(0, take: 1000, new ActivityQuery());
+
+        using var _ = new AssertionScope();
+        foreach (var activity in activities)
+        {
+            foreach (var change in activity.Changes)
+            {
+                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;
+
+                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 (HistoryService.OwningEntryId)");
+            }
+        }
+    }
+
     // Entity type name without the CLR generic-arity suffix, so a generic entity (RemoteResource<TMetadata>)
     // 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

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()
     {

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();

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), 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
+{
+    protected override MiniLcmApiFixture CreateFixture() => MiniLcmApiFixture.Create(configureServices: services =>
+        services.Configure<HarmonyConfig>(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();
+    }
+}

backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs

@@ -1,6 +1,8 @@
 using SIL.Harmony.Config;
 using System.Diagnostics;
 using LcmCrdt.Changes;
+using LinqToDB;
+using LinqToDB.EntityFrameworkCore;
 using Meziantou.Extensions.Logging.Xunit;
 using Microsoft.Data.Sqlite;
 using Microsoft.EntityFrameworkCore;
@@ -16,6 +18,7 @@ public class MiniLcmApiFixture : IAsyncLifetime, IAsyncDisposable
 {
     private readonly bool _seedWs = true;
     private readonly Guid? _projectId;
+    private readonly Action<IServiceCollection>? _configureServices;
     // fixtures run in parallel and share project codes, so each gets its own folder (media cache) to delete on dispose
     private readonly string _projectPath = Path.GetFullPath($"fixture-{Guid.NewGuid():N}");
     private AsyncServiceScope _services;
@@ -36,15 +39,16 @@ public MiniLcmApiFixture()
     {
     }
 
-    public static MiniLcmApiFixture Create(bool seedWs = true, Guid? projectId = null)
+    public static MiniLcmApiFixture Create(bool seedWs = true, Guid? projectId = null, Action<IServiceCollection>? 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<IServiceCollection>? configureServices = null)
     {
         _seedWs = seedWs;
         _projectId = projectId;
+        _configureServices = configureServices;
     }
 
     public async Task InitializeAsync()
@@ -66,14 +70,24 @@ public async Task InitializeAsync(string projectName, Guid? projectId = null)
             }
         }
 
-        var services = new ServiceCollection()
+        var serviceCollection = new ServiceCollection()
             .AddTestLcmCrdtClient()
             .Configure<LcmCrdtConfig>(config => config.ProjectPath = _projectPath)
             .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<LcmCrdtDbContext>(builder => builder
+                .EnableServiceProviderCaching(false)
+                .UseLinqToDB(linq2db => linq2db.AddCustomOptions(options => options.UseDisableQueryCache(true))),
+                ServiceLifetime.Scoped);
+        }
+        var services = serviceCollection.BuildServiceProvider();
         _services = services.CreateAsyncScope();
         if (inMemory)
         {

backend/FwLite/LcmCrdt/HistoryService.cs

@@ -310,6 +310,11 @@ public async Task<ChangeContext> LoadChangeContext(Guid commitId, int changeInde
             .FirstOrDefaultAsync()
             ?? throw new InvalidOperationException($"Change {changeIndex} not found in commit {commitId}");
 
+        // 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, []);
+
         // 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)
@@ -361,31 +366,50 @@ private async Task<Entry> GetCurrentOrLatestEntry(Guid entryId)
 
     private async IAsyncEnumerable<Guid> GetAffectedEntryIds(ChangeEntity<IChange> changeEntity)
     {
-        if (changeEntity.Change.EntityType == typeof(Entry))
+        if (changeEntity.Change.EntityType == typeof(ComplexFormComponent))
         {
-            yield return changeEntity.EntityId;
+            var cfc = await dataModel.GetLatest<ComplexFormComponent>(changeEntity.EntityId)
+                ?? throw NotFoundException.ForType<ComplexFormComponent>(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<Sense>(changeEntity.EntityId)
-                ?? throw NotFoundException.ForType<Sense>(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<Guid?> OwningEntryId(Type? entityType, Guid entityId)
+    {
+        if (entityType == typeof(Entry)) return entityId;
+        if (entityType == typeof(Sense))
         {
-            var example = await dataModel.GetLatest<ExampleSentence>(changeEntity.EntityId)
-                ?? throw NotFoundException.ForType<ExampleSentence>(changeEntity.EntityId);
-            var sense = await dataModel.GetLatest<Sense>(example.SenseId)
-                ?? throw NotFoundException.ForType<Sense>(example.SenseId);
-            yield return sense.EntryId;
+            var sense = await dataModel.GetLatest<Sense>(entityId) ?? throw NotFoundException.ForType<Sense>(entityId);
+            return sense.EntryId;
         }
-        else if (changeEntity.Change.EntityType == typeof(ComplexFormComponent))
+        if (entityType == typeof(ExampleSentence))
         {
-            var cfc = await dataModel.GetLatest<ComplexFormComponent>(changeEntity.EntityId)
-                ?? throw NotFoundException.ForType<ComplexFormComponent>(changeEntity.EntityId);
-            yield return cfc.ComplexFormEntryId;
-            yield return cfc.ComponentEntryId;
+            var example = await dataModel.GetLatest<ExampleSentence>(entityId) ?? throw NotFoundException.ForType<ExampleSentence>(entityId);
+            return await OwningEntryId(typeof(Sense), example.SenseId);
+        }
+        if (entityType == typeof(CommentThread))
+        {
+            var thread = await dataModel.GetLatest<CommentThread>(entityId) ?? throw NotFoundException.ForType<CommentThread>(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<UserComment>(entityId) ?? throw NotFoundException.ForType<UserComment>(entityId);
+            return await OwningEntryId(typeof(CommentThread), comment.CommentThreadId);
         }
+        return null;
     }
 
     public static string ChangesNameHelper(IReadOnlyList<ActivityChange> changes)

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}
 
-    <label class={cn(
-      'w-fit flex items-center gap-2 border rounded p-2 px-4 bg-secondary/25',
-      currentEntity && 'cursor-pointer')}>
-      <FormatRelativeDate date={activity.timestamp}
-                          live
-                          actualDateOptions={{ dateStyle: 'medium', timeStyle: 'short' }}/>
-      <Switch disabled={!currentEntity} bind:checked={selectedShowCurrent} class="text-destructive" />
-      {#if currentEntity}
-        <span>{$t`Current version`}</span>
-      {:else}
-        <span class="text-destructive">{$t`Deleted`}</span>
-      {/if}
-    </label>
+    {#if hasCurrentVersion}
+      <label class={cn(
+        'w-fit flex items-center gap-2 border rounded p-2 px-4 bg-secondary/25',
+        currentEntity && 'cursor-pointer')}>
+        <FormatRelativeDate date={activity.timestamp}
+                            live
+                            actualDateOptions={{ dateStyle: 'medium', timeStyle: 'short' }}/>
+        <Switch disabled={!currentEntity} bind:checked={selectedShowCurrent} class="text-destructive" />
+        {#if currentEntity}
+          <span>{$t`Current version`}</span>
+        {:else}
+          <span class="text-destructive">{$t`Deleted`}</span>
+        {/if}
+      </label>
+    {/if}
   </div>
 {/if}