refactor: extract JsonFilePersistence<T> to eliminate duplicated JSON persistence logic

[Copilot]

NotificationHistory and ReviewRequestHistory each independently implemented ~22 lines of identical JSON file persistence logic (lock, Load(), Save(), error handling) with minor inconsistencies in error message formatting.

Changes

• New src/JsonFilePersistence.cs — generic utility class encapsulating all file persistence concerns:
  • Single _lockObject guarding both Load() and Save() for thread-safe file I/O
  • Consistent error logging using the file path
• NotificationHistory — delegates to JsonFilePersistence<List<NotificationEntry>>; removes private Load() and inline Save() body
• ReviewRequestHistory — delegates to JsonFilePersistence<List<string>>; removes private Load() and inline Save() body; HashSet↔List conversion kept at the call sites

// Both history classes now follow this pattern instead of duplicating the logic
_persistence = new JsonFilePersistence<List<NotificationEntry>>(Constants.NotificationHistoryFileName);
_entries = _persistence.Load(new List<NotificationEntry>());

public void Save()
{
    lock (_lockObject)  // protects _entries from concurrent mutation
    {
        _persistence.Save(_entries);  // handles file I/O locking internally
    }
}

The history classes retain their own _lockObject to protect in-memory state during concurrent reads/writes — this is distinct from the _persistence-internal lock that guards file I/O.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Duplicate Code: JSON File-Based Persistence Pattern in History Classes</issue_title>
<issue_description>Analysis of commit ebf44dd

Assignee: @copilot

Summary

NotificationHistory and ReviewRequestHistory independently implement nearly identical JSON file-based persistence logic — including lock objects, Load(), and Save() methods with the same structure. This is a classic copy-paste duplication that violates DRY and will lead to divergence and inconsistent bug fixes.

Duplication Details

Pattern: JSON File Persistence (Load/Save with lock + error logging)

• Severity: Medium
• Occurrences: 2 classes, ~22 lines of structurally identical code per class
• Locations:
  • src/NotificationHistory.cs (lines 10, 18–50)
  • src/ReviewRequestHistory.cs (lines 8, 15–49)

Code Sample — NotificationHistory.cs (lines 10, 18–50):

private readonly object _lockObject = new object();

private List(NotificationEntry) Load()
{
    if (File.Exists(Constants.NotificationHistoryFileName))
    {
        try
        {
            var json = File.ReadAllText(Constants.NotificationHistoryFileName);
            return JsonSerializer.Deserialize(List<NotificationEntry)>(json) ?? new List(NotificationEntry)();
        }
        catch (Exception ex)
        {
            Logger.LogError($"Error loading notification history", ex);
        }
    }
    return new List(NotificationEntry)();
}

public void Save()
{
    lock (_lockObject)
    {
        try
        {
            var options = new JsonSerializerOptions { WriteIndented = true };
            var json = JsonSerializer.Serialize(_entries, options);
            File.WriteAllText(Constants.NotificationHistoryFileName, json);
        }
        catch (Exception ex)
        {
            Logger.LogError($"Error saving notification history", ex);
        }
    }
}

Code Sample — ReviewRequestHistory.cs (lines 8, 15–49):

private readonly object _lockObject = new object();

private HashSet(string) Load()
{
    if (File.Exists(Constants.ReviewRequestHistoryFileName))
    {
        try
        {
            var json = File.ReadAllText(Constants.ReviewRequestHistoryFileName);
            var list = JsonSerializer.Deserialize(List<string)>(json);
            return list != null ? new HashSet(string)(list) : new HashSet(string)();
        }
        catch (Exception ex)
        {
            Logger.LogError($"Error loading review request history: {ex.Message}", ex);
        }
    }
    return new HashSet(string)();
}

private void Save()
{
    lock (_lockObject)
    {
        try
        {
            var options = new JsonSerializerOptions { WriteIndented = true };
            var list = _seenRequestIds.ToList();
            var json = JsonSerializer.Serialize(list, options);
            File.WriteAllText(Constants.ReviewRequestHistoryFileName, json);
        }
        catch (Exception ex)
        {
            Logger.LogError($"Error saving review request history: {ex.Message}", ex);
        }
    }
}

Impact Analysis

• Maintainability: Any change to the persistence strategy (e.g., atomic writes, backup-before-overwrite) must be applied to both classes separately. Forgetting one leads to inconsistent behavior.
• Bug Risk: Minor differences already exist (e.g., error message format in LogError call differs between the two). Future bugs may be fixed in one class but not the other.
• Code Bloat: ~44 lines of near-identical logic spread across two files.

Refactoring Recommendations

1. Extract a generic JsonFilePersistence(T) utility class

   • Create: src/JsonFilePersistence.cs
   • Encapsulate the Load(T)() and Save(T)() logic with the file path, lock, and error handling
   • Both history classes delegate to this utility for file I/O
   • Estimated effort: ~1–2 hours
   • Benefits: Single place to improve persistence (atomic writes, error recovery, etc.)

2. Alternative: Abstract base class JsonFileHistory(TEntry, TCollection)

   • NotificationHistory and ReviewRequestHistory inherit from it
   • Base class provides Load(), Save(), _lockObject
   • Subclasses implement collection-specific logic (Add, MarkAsSeen, etc.)
   • Estimated effort: ~2–3 hours
   • Benefits: Stronger typing, enforces consistent interface

Implementation Checklist

• Review duplication findings
• Choose refactoring approach (utility class vs. base class)
• Create shared persistence abstraction
• Refactor NotificationHistory to use the shared abstraction
• Refactor ReviewRequestHistory to use the shared abstraction
• Verify no functionality broken

Analysis Metadata

• Analyzed Files: 21 .cs files
• Detection Method: Serena semantic code analysis + manual pattern comparison
• Commit: ebf44dd
• Analysis Date: 2026-03-18

Generated by [Duplicate Code Detec...

• Fixes Duplicate Code: JSON File-Based Persistence Pattern in History Classes #156

---

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

[Copilot]

Pull request overview

This PR removes duplicated JSON file persistence code from NotificationHistory and ReviewRequestHistory by extracting it into a shared generic helper, keeping the history classes focused on in-memory state management.

Changes:

• Added JsonFilePersistence<T> to encapsulate JSON load/save, file I/O locking, and consistent error logging.
• Refactored NotificationHistory to delegate persistence to JsonFilePersistence<List<NotificationEntry>>.
• Refactored ReviewRequestHistory to delegate persistence to JsonFilePersistence<List<string>>, keeping HashSet↔List conversion at the call sites.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/JsonFilePersistence.cs	Introduces shared generic JSON file load/save utility with internal locking and unified logging.
src/NotificationHistory.cs	Replaces bespoke JSON persistence with JsonFilePersistence<List<NotificationEntry>>.
src/ReviewRequestHistory.cs	Replaces bespoke JSON persistence with JsonFilePersistence<List<string>> and retains HashSet semantics in-memory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.