Extract shared JSON persistence logic into JsonPersistence helper

[Copilot]

• Explore repo structure and understand the duplicate persistence code
• Create src/JsonPersistence.cs static helper class with shared Load<T> and Save<T> methods
• Refactor NotificationHistory.cs to use JsonPersistence
• Refactor ReviewRequestHistory.cs to use JsonPersistence
• Verify build succeeds (0 warnings, 0 errors)
• Address PR review: clarify JsonPersistence thread-safety doc
• Address PR review: remove redundant null-coalesce in ReviewRequestHistory.Load()
• Address PR review: make JsonPersistence.Save atomic (write to .tmp, then File.Move with overwrite)
• Address PR review: remove redundant lock from ReviewRequestHistory.Save()
• Address PR review: update class-level doc to reflect atomic write (readers see old or new file, not partial)
• Address PR review: clean up .tmp file in finally block if File.Move fails
• Address PR review: split NotificationHistory.Save() into private SaveUnlocked() + public Save() wrapper to eliminate redundant re-entrant lock from Add()

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Duplicate Code: JSON Persistence Pattern in NotificationHistory and ReviewRequestHistory</issue_title>
<issue_description>Analysis of commit ebf44dd

Assignee: @copilot

Summary

NotificationHistory and ReviewRequestHistory both implement nearly identical thread-safe JSON file persistence logic, including a _lockObject, a Load() method, and a Save() method. This structural duplication (~25 lines each) violates DRY and means any fix to the persistence layer must be applied twice.

Duplication Details

Pattern: Thread-safe JSON file load/save with lock and error handling

• Severity: Medium
• Occurrences: 2 classes
• Locations:
  • src/NotificationHistory.cs (lines 10, 19–44)
  • src/ReviewRequestHistory.cs (lines 8, 12–43)

Code Sample — NotificationHistory.cs:

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 (structurally identical):

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 persistence logic (e.g., switching from plain JSON to encrypted storage, adding atomic writes, changing error handling) must be applied in two places.
• Bug Risk: The two Save() implementations are already inconsistent: NotificationHistory does not include ex.Message in the log message string, while ReviewRequestHistory does — a symptom of copy-paste drift.
• Code Bloat: ~50 lines of near-identical boilerplate spread across two files.

Refactoring Recommendations

1. Extract a generic JsonFileStore(T) base class or utility

   • Path suggestion: src/JsonFileStore.cs
   • It would accept a file path and provide generic Load() / Save() with locking and error handling.
   • Both NotificationHistory and ReviewRequestHistory would delegate to it.
   • Estimated effort: 1–2 hours
   • Benefits: Single point of change for persistence logic, eliminates inconsistencies.

2. Alternatively: extract a static JsonPersistence helper

   • Provide TryLoad(T)(string path, out T value) and TrySave(T)(string path, T value) statics.
   • Lightweight; no inheritance required.

Implementation Checklist

• Review duplication findings
• Decide on refactoring approach (base class vs. static helper)
• Implement JsonFileStore(T) or equivalent
• Refactor NotificationHistory to use it
• Refactor ReviewRequestHistory to use it
• Verify no functionality broken
• Update tests if applicable

Analysis Metadata

• Analyzed Files: src/NotificationHistory.cs, src/ReviewRequestHistory.cs
• Detection Method: Serena semantic code analysis + pattern search
• Commit: ebf44dd
• Analysis Date: 2026-03-16

Generated by [Duplicate Code Detector](https://github.com/sunzhuoshi/agent-supervisor/actio...

• Fixes Duplicate Code: JSON Persistence Pattern in NotificationHistory and ReviewRequestHistory #154

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[Copilot]

Pull request overview

This PR refactors duplicated JSON file persistence logic used by NotificationHistory and ReviewRequestHistory into a shared JsonPersistence helper to centralize load/save behavior and logging.

Changes:

• Added src/JsonPersistence.cs with shared generic Load<T> / Save<T> helpers.
• Refactored NotificationHistory to delegate JSON persistence to JsonPersistence.
• Refactored ReviewRequestHistory to delegate JSON persistence to JsonPersistence.

Reviewed changes

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

File	Description
src/JsonPersistence.cs	Introduces shared JSON load/save helpers with centralized serialization options and error logging.
src/NotificationHistory.cs	Removes in-class JSON persistence and uses JsonPersistence for loading/saving entries.
src/ReviewRequestHistory.cs	Removes in-class JSON persistence and uses JsonPersistence for loading/saving request IDs.

---

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

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied both review changes in commit 2fd9063:

1. JsonPersistence.cs doc — Replaced the misleading "safe to call concurrently" wording with a clear note that thread-safety must be managed by callers, and that concurrent reads during a Save can observe partial file writes.
2. ReviewRequestHistory.cs:16 — Removed the redundant ?? new List<string>() null-coalesce; JsonPersistence.Load<T> always returns a non-null value.

[Copilot]

Pull request overview

This PR removes duplicated JSON file load/save logic from NotificationHistory and ReviewRequestHistory by introducing a shared JsonPersistence helper, keeping persistence behavior consistent across both history stores.

Changes:

• Added JsonPersistence static helper with generic Load<T> / Save<T> wrappers around System.Text.Json.
• Refactored NotificationHistory to delegate JSON persistence to JsonPersistence.
• Refactored ReviewRequestHistory to delegate JSON persistence to JsonPersistence.

Reviewed changes

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

File	Description
src/JsonPersistence.cs	Adds shared JSON load/save implementation and logging context.
src/NotificationHistory.cs	Replaces inline JSON persistence with JsonPersistence calls.
src/ReviewRequestHistory.cs	Replaces inline JSON persistence with JsonPersistence calls.

---

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

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied both changes in commit ccf4371:

1. JsonPersistence.Save — atomic write: Now serializes to a sibling .tmp file first, then calls File.Move(..., overwrite: true) to atomically replace the target. This eliminates the partial-write window and prevents readers from ever observing truncated JSON.
2. ReviewRequestHistory.Save — remove redundant lock: Since Save() is private and all callers (MarkAsSeen, MarkMultipleAsSeen) already hold _lockObject, the inner lock was redundant. Removed it and added a // Must be called while holding _lockObject. comment to document the invariant.

[Copilot]

Pull request overview

This PR reduces duplication in the history persistence layer by extracting shared JSON load/save behavior into a reusable JsonPersistence helper and refactoring history classes to delegate persistence to it.

Changes:

• Added JsonPersistence helper with generic Load<T> and atomic Save<T> (write .tmp then replace).
• Refactored NotificationHistory to use JsonPersistence for load/save.
• Refactored ReviewRequestHistory to use JsonPersistence and removed its internal save lock (documenting lock expectations).

Reviewed changes

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

File	Description
src/JsonPersistence.cs	Introduces shared JSON load/save helpers and centralizes logging and atomic write behavior.
src/NotificationHistory.cs	Replaces inline JSON persistence with JsonPersistence calls.
src/ReviewRequestHistory.cs	Replaces inline JSON persistence with JsonPersistence calls and adjusts locking approach.

---

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

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all three changes in commit 6962c67:

1. JsonPersistence.cs class doc — Updated to accurately reflect the atomic write: readers will see either the previous or the new file, never partial content, while still noting that callers should use a lock to avoid logical races.
2. JsonPersistence.Save temp file cleanup — Added a finally block that sets tempPath = null after a successful File.Move, and best-effort deletes the .tmp file in the finally if it was not moved (i.e., on failure).
3. NotificationHistory redundant lock — Extracted a private SaveUnlocked() (called under _lockObject) and kept the public Save() as a lock-acquiring wrapper. Add() now calls SaveUnlocked() directly, eliminating the re-entrant lock acquisition.

[Copilot]

Pull request overview

This PR reduces duplicated JSON file persistence code by introducing a shared JsonPersistence helper and refactoring history classes to use it, including atomic writes for safer on-disk updates.

Changes:

• Added JsonPersistence helper with shared Load<T>/Save<T> methods and atomic save via .tmp + overwrite move.
• Refactored NotificationHistory to delegate JSON load/save to JsonPersistence and avoid redundant/re-entrant locking during Add().
• Refactored ReviewRequestHistory to delegate JSON load/save to JsonPersistence and rely on the existing external lock.

Reviewed changes

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

File	Description
src/JsonPersistence.cs	Introduces centralized JSON load/save with shared serializer options and atomic save semantics.
src/NotificationHistory.cs	Switches persistence to JsonPersistence and splits Save()/SaveUnlocked() to prevent re-entrant locking from Add().
src/ReviewRequestHistory.cs	Switches persistence to JsonPersistence and removes internal locking from Save() (callers already hold the lock).

---

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