Conversation
There was a problem hiding this comment.
Pull request overview
Adds a version history feature for entries (password notes) so users can view prior versions in the entry Details page, while keeping existing stored data backward-compatible via a migration step.
Changes:
- Introduces
EntryHistoryRecordand adds aHistorycollection toEntry. - Persists history on create/update and migrates existing entries to include a synthetic “pre-history” record.
- Renders an expandable “Change history” timeline in the Details view with supporting CSS styling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wwwroot/css/site.css | Adds styles for the history timeline UI in the entry Details view. |
| Views/Entries/Details.cshtml | Adds an expandable history section that lists versions sorted by change timestamp. |
| Services/JsonDataStore.cs | Writes history records on create/update, migrates older entries, and adjusts JSON deserialization options. |
| Models/EntryHistoryRecord.cs | New model representing a single historical version snapshot. |
| Models/Entry.cs | Adds History collection to the entry model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public string Title { get; set; } = ""; | ||
| public string Details { get; set; } = ""; | ||
| public string? Users { get; set; } | ||
| public List<EntryHistoryRecord> History { get; set; } = new(); |
There was a problem hiding this comment.
History is declared as a non-nullable List<EntryHistoryRecord>, but other code paths treat it as potentially null (e.g., History ??= ..., History == null, and Model.History ?? ...). With nullable reference types enabled, this will produce warnings and makes the nullability contract unclear. Consider either making History nullable (List<EntryHistoryRecord>?) to match the persisted/deserialized reality, or enforce non-nullability by removing the null checks and ensuring migration/deserialization always produces a non-null list (including handling "History": null in stored JSON).
| public List<EntryHistoryRecord> History { get; set; } = new(); | |
| public List<EntryHistoryRecord>? History { get; set; } = new(); |
| entry.History ??= new List<EntryHistoryRecord>(); | ||
| entry.History.Add(new EntryHistoryRecord | ||
| { |
There was a problem hiding this comment.
entry.History ??= ... assumes History can be null, but Entry.History is currently non-nullable. With <Nullable>enable</Nullable>, this will trigger warnings and obscures the intended nullability contract. Align this by either making Entry.History nullable or by removing the null-coalescing and relying on a guaranteed non-null list after deserialization/migration.
| entry.History ??= new List<EntryHistoryRecord>(); | |
| entry.History.Add(new EntryHistoryRecord | |
| { | |
| var history = entry.History; | |
| if (history == null) | |
| { | |
| history = new List<EntryHistoryRecord>(); | |
| entry.History = history; | |
| } | |
| history.Add(new EntryHistoryRecord | |
| { |
No description provided.