Conversation
There was a problem hiding this comment.
Pull request overview
Implements blog post versioning so updates create immutable snapshots, enables restoring prior versions in the admin editor, and adds a UI to view diffs between a selected version and the current post.
Changes:
- Introduces
BlogPostVersiondomain entity + EF Core mapping/migration to persist version history. - Adds
IBlogPostVersionService/BlogPostVersionServiceto snapshot on save and restore prior versions. - Extends the admin editor to show version history, diff dialog, and restore actions (DiffPlex-powered).
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/LinkDotNet.Blog.UnitTests/Web/Features/Admin/BlogPostEditor/Components/CreateNewBlogPostTests.cs | Registers a version service fake for editor component tests. |
| tests/LinkDotNet.Blog.UnitTests/Domain/BlogPostVersionTests.cs | Adds unit tests validating snapshot field coverage and exclusions. |
| tests/LinkDotNet.Blog.IntegrationTests/Web/Features/Admin/BlogPostEditor/UpdateBlogPostPageTests.cs | Updates integration tests for versioned saves + toast message changes. |
| tests/LinkDotNet.Blog.IntegrationTests/Web/Features/Admin/BlogPostEditor/CreateNewBlogPostPageTests.cs | Adds version service stub wiring for create page tests. |
| tests/LinkDotNet.Blog.IntegrationTests/Web/Features/Admin/BlogPostEditor/BlogPostVersionServiceTests.cs | Adds integration coverage for save/restore/history behaviors. |
| src/LinkDotNet.Blog.Web/ServiceExtensions.cs | Registers IBlogPostVersionService in DI. |
| src/LinkDotNet.Blog.Web/LinkDotNet.Blog.Web.csproj | Adds DiffPlex dependency reference. |
| src/LinkDotNet.Blog.Web/Features/Admin/BlogPostEditor/UpdateBlogPostPage.razor | Uses version service when saving; wires restore callback into editor component. |
| src/LinkDotNet.Blog.Web/Features/Admin/BlogPostEditor/Services/IBlogPostVersionService.cs | Defines the versioning service contract. |
| src/LinkDotNet.Blog.Web/Features/Admin/BlogPostEditor/Services/BlogPostVersionService.cs | Implements snapshot-on-save, restore, and version history queries. |
| src/LinkDotNet.Blog.Web/Features/Admin/BlogPostEditor/Components/VersionDiffDialog.razor | Adds a modal to display field and content diffs between version/current. |
| src/LinkDotNet.Blog.Web/Features/Admin/BlogPostEditor/Components/CreateNewBlogPost.razor | Adds version history UI, diff, and restore interactions. |
| src/LinkDotNet.Blog.Infrastructure/Persistence/Sql/Mapping/BlogPostVersionConfiguration.cs | Adds EF mapping for BlogPostVersion. |
| src/LinkDotNet.Blog.Infrastructure/Persistence/Sql/BlogDbContext.cs | Adds DbSet<BlogPostVersion> + applies mapping. |
| src/LinkDotNet.Blog.Infrastructure/Migrations/BlogDbContextModelSnapshot.cs | Updates snapshot for the new entity/table. |
| src/LinkDotNet.Blog.Infrastructure/Migrations/20260424073229_AddBlogPostVersioning.cs | Adds migration creating the BlogPostVersions table + unique index. |
| src/LinkDotNet.Blog.Infrastructure/Migrations/20260424073229_AddBlogPostVersioning.Designer.cs | Migration designer for the new versioning table. |
| src/LinkDotNet.Blog.Domain/BlogPostVersion.cs | Introduces the version snapshot entity + CreateSnapshot. |
| Directory.Packages.props | Adds centralized DiffPlex version. |
Files not reviewed (1)
- src/LinkDotNet.Blog.Infrastructure/Migrations/20260424073229_AddBlogPostVersioning.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static IReadOnlyList<DiffDisplayLine> BuildContentDiff(string oldText, string newText) | ||
| { | ||
| var rawLines = InlineDiffBuilder.Diff(oldText, newText).Lines; | ||
| var visible = new bool[rawLines.Count]; | ||
|
|
||
| for (var i = 0; i < rawLines.Count; i++) | ||
| { | ||
| if (rawLines[i].Type == ChangeType.Unchanged) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var from = Math.Max(0, i - ContextLines); | ||
| var to = Math.Min(rawLines.Count - 1, i + ContextLines); | ||
| for (var j = from; j <= to; j++) | ||
| { | ||
| visible[j] = true; | ||
| } | ||
| } | ||
|
|
||
| var result = new List<DiffDisplayLine>(rawLines.Count); | ||
| var oldLine = 0; | ||
| var newLine = 0; | ||
| var pendingCollapse = 0; | ||
|
|
||
| for (var i = 0; i < rawLines.Count; i++) | ||
| { | ||
| var piece = rawLines[i]; | ||
|
|
||
| if (!visible[i] && piece.Type == ChangeType.Unchanged) | ||
| { | ||
| oldLine++; | ||
| newLine++; | ||
| pendingCollapse++; | ||
| continue; | ||
| } | ||
|
|
||
| if (pendingCollapse > 0) | ||
| { | ||
| result.Add(new DiffDisplayLine("", "", " ", "", "", IsCollapse: true, CollapseCount: pendingCollapse)); | ||
| pendingCollapse = 0; | ||
| } | ||
|
|
||
| string oldNum, newNum, prefix, cssClass; | ||
| switch (piece.Type) | ||
| { | ||
| case ChangeType.Deleted: | ||
| oldLine++; | ||
| oldNum = oldLine.ToString(); | ||
| newNum = ""; | ||
| prefix = "-"; | ||
| cssClass = "bg-danger-subtle"; | ||
| break; | ||
| case ChangeType.Inserted: | ||
| newLine++; | ||
| oldNum = ""; | ||
| newNum = newLine.ToString(); | ||
| prefix = "+"; | ||
| cssClass = "bg-success-subtle"; | ||
| break; | ||
| case ChangeType.Modified: | ||
| oldLine++; | ||
| newLine++; | ||
| oldNum = oldLine.ToString(); | ||
| newNum = newLine.ToString(); | ||
| prefix = "~"; | ||
| cssClass = "bg-warning-subtle"; | ||
| break; | ||
| default: | ||
| if (piece.Type == ChangeType.Unchanged) | ||
| { | ||
| oldLine++; | ||
| newLine++; | ||
| } | ||
|
|
||
| oldNum = piece.Type == ChangeType.Unchanged ? oldLine.ToString() : ""; | ||
| newNum = piece.Type == ChangeType.Unchanged ? newLine.ToString() : ""; | ||
| prefix = " "; | ||
| cssClass = ""; | ||
| break; | ||
| } | ||
|
|
||
| result.Add(new DiffDisplayLine(oldNum, newNum, prefix, piece.Text ?? "", cssClass)); | ||
| } | ||
|
|
||
| if (pendingCollapse > 0) | ||
| { | ||
| result.Add(new DiffDisplayLine("", "", " ", "", "", IsCollapse: true, CollapseCount: pendingCollapse)); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
BuildContentDiff always emits a final "collapse" line when there are no changes (all lines are Unchanged and visible[] stays false). That means contentLines.Count > 0 even when the content is identical, so the UI won't show the intended "Content is identical." message. Consider detecting the "no changes" case (e.g., if there are no non-Unchanged pieces) and returning an empty list.
| <ItemGroup Label="Code Analyzers"> | ||
| <PackageVersion Include="DiffPlex" Version="1.9.0" /> | ||
| <PackageVersion Include="IDisposableAnalyzers" Version="4.0.8" /> | ||
| <GlobalPackageReference Include="SonarAnalyzer.CSharp" Version="10.23.0.137933" PrivateAssets="All" IncludeAssets="Runtime;Build;Native;contentFiles;Analyzers" /> |
There was a problem hiding this comment.
DiffPlex is a runtime dependency used by the Web project, but its centrally-managed version was added under the "Code Analyzers" item group. Please move it under the "Web" group to keep package organization consistent with the rest of this file.
| public async ValueTask SaveNewVersionAsync(BlogPost currentBlogPost, BlogPost updatedBlogPost) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(currentBlogPost); | ||
| ArgumentNullException.ThrowIfNull(updatedBlogPost); | ||
|
|
||
| await StoreSnapshotAsync(currentBlogPost); | ||
|
|
||
| currentBlogPost.Update(updatedBlogPost); | ||
| await blogPostRepository.StoreAsync(currentBlogPost); | ||
| } |
There was a problem hiding this comment.
SaveNewVersionAsync stores the snapshot and updates the BlogPost in two separate operations/DbContexts without any transaction. If the blog post update fails after the snapshot is inserted (or vice versa), the version history can become inconsistent. Consider performing snapshot + blog post update in a single BlogDbContext transaction (or otherwise ensuring atomicity).
| // Reconstruct a transient BlogPost from the target version fields. | ||
| // ScheduledPublishDate is intentionally not restored. | ||
| var restored = BlogPost.Create( | ||
| targetVersion.Title, | ||
| targetVersion.ShortDescription, | ||
| targetVersion.Content, | ||
| targetVersion.PreviewImageUrl, | ||
| targetVersion.IsPublished, | ||
| targetVersion.UpdatedDate, | ||
| scheduledPublishDate: null, | ||
| targetVersion.Tags, | ||
| targetVersion.PreviewImageUrlFallback, | ||
| targetVersion.AuthorName); | ||
|
|
||
| currentBlogPost.Update(restored); |
There was a problem hiding this comment.
The service doc/comment says ScheduledPublishDate is "intentionally not restored", but the current implementation sets scheduledPublishDate: null and then calls currentBlogPost.Update(restored), which will actively clear any existing ScheduledPublishDate on the blog post. If the intent is to ignore the scheduled date from the version, preserve currentBlogPost.ScheduledPublishDate instead of forcing it to null.
| var maxVersion = await db.BlogPostVersions | ||
| .Where(v => v.BlogPostId == blogPost.Id) | ||
| .Select(v => (int?)v.VersionNumber) | ||
| .MaxAsync() ?? 0; | ||
|
|
||
| var snapshot = BlogPostVersion.CreateSnapshot(blogPost, maxVersion + 1); | ||
| await db.BlogPostVersions.AddAsync(snapshot); | ||
| await db.SaveChangesAsync(); |
There was a problem hiding this comment.
StoreSnapshotAsync derives the next VersionNumber via MaxAsync() + 1. With concurrent saves/restores for the same BlogPostId, two requests can compute the same max and then hit the unique index (BlogPostId, VersionNumber) causing a DbUpdateException. Consider adding concurrency handling (transaction with appropriate isolation, retry on unique constraint violation, or generating version numbers in the database).
e458b85 to
20e583a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/LinkDotNet.Blog.Infrastructure/Migrations/20260424073229_AddBlogPostVersioning.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await StoreSnapshotAsync(currentBlogPost); | ||
|
|
||
| currentBlogPost.Update(updatedBlogPost); | ||
| await blogPostRepository.StoreAsync(currentBlogPost); |
There was a problem hiding this comment.
Per issue requirements, ScheduledPublishDate should not be considered/versioned when updating a post. Currently SaveNewVersionAsync calls currentBlogPost.Update(updatedBlogPost), and BlogPost.Update copies ScheduledPublishDate, which means schedule-only changes will both (a) create a new version that doesn't contain scheduling info and (b) overwrite the live scheduled date. Consider preserving currentBlogPost.ScheduledPublishDate during updates (and/or skipping snapshot creation when only non-versioned fields like schedule changed).
| public async ValueTask RestoreVersionAsync(BlogPost currentBlogPost, BlogPostVersion targetVersion) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(currentBlogPost); | ||
| ArgumentNullException.ThrowIfNull(targetVersion); | ||
|
|
||
| // Snapshot the current state before overwriting it | ||
| await StoreSnapshotAsync(currentBlogPost); | ||
|
|
||
| // Reconstruct a transient BlogPost from the target version fields. | ||
| // ScheduledPublishDate is not versioned, so we preserve whatever schedule the | ||
| // current live post has — unless the version being restored is published (a | ||
| // published post cannot carry a scheduled date per the domain invariant). | ||
| var scheduledPublishDate = targetVersion.IsPublished ? null : currentBlogPost.ScheduledPublishDate; |
There was a problem hiding this comment.
RestoreVersionAsync accepts any BlogPostVersion instance, but doesn't validate that targetVersion.BlogPostId matches currentBlogPost.Id. Accidentally passing a version from a different post would overwrite the current post with unrelated content. Add a guard clause (and ideally a clear exception) to ensure the version belongs to the blog post being restored.
| <div class="list-group-item d-flex justify-content-between align-items-center py-2 px-3"> | ||
| <div class="me-2" style="min-width: 0;"> | ||
| <div class="fw-semibold small">v@(v.VersionNumber)</div> | ||
| <div class="text-muted" style="font-size: 0.72rem;">@v.CreatedAt.ToString("MMM dd, yyyy HH:mm")</div> |
There was a problem hiding this comment.
BlogPostVersion.CreatedAt is set using DateTime.UtcNow, but the version history list renders CreatedAt without converting to local time (unlike VersionDiffDialog, which calls ToLocalTime()). This will display UTC timestamps to users. Consider consistently applying ToLocalTime() (or clearly labeling the timezone) in the list view.
| <div class="text-muted" style="font-size: 0.72rem;">@v.CreatedAt.ToString("MMM dd, yyyy HH:mm")</div> | |
| <div class="text-muted" style="font-size: 0.72rem;">@v.CreatedAt.ToLocalTime().ToString("MMM dd, yyyy HH:mm")</div> |
20e583a to
5207506
Compare
Closes #483