refactor: extract JsonFileStore to eliminate triplicated JSON persistence logic

[Copilot]

• Create src/JsonFileStore.cs with generic static Load<T> and Save<T> methods
• Refactor NotificationHistory to use JsonFileStore
• Refactor ReviewRequestHistory to use JsonFileStore
• Refactor ReviewRequestService to use JsonFileStore
• Fix duplicate ex.Message in JsonFileStore log messages (Logger already appends it)
• Add per-file ConcurrentDictionary locking in JsonFileStore so Load and Save are both synchronized, preventing races between concurrent reads/writes
• Extract JsonSerializerOptions to a static readonly field to avoid per-call allocation
• Build verified (0 errors, 0 warnings)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Duplicate Code: JSON Persistence (Load/Save) Pattern Across Three Storage Classes</issue_title>
<issue_description>Analysis of recent commits — commit ebf44dd

Assignee: @copilot

Summary

Three storage classes implement nearly identical Load() and Save() methods for JSON file persistence, creating ~40 lines of essentially copy-pasted logic replicated three times.

Duplication Details

Pattern: Thread-safe JSON file persistence (Load + Save)

• Severity: High
• Occurrences: 3 classes, 6 methods
• Locations:
  • src/NotificationHistory.cs (Load: lines 18–31, Save: lines 35–49)
  • src/ReviewRequestHistory.cs (Load: lines 15–29, Save: lines 33–47)
  • src/ReviewRequestService.cs (Load: lines 69–83, Save: lines 86–100)

Duplicated Save() code sample (nearly identical across all three files):

lock (_lockObject)
{
    try
    {
        var options = new JsonSerializerOptions { WriteIndented = true };
        var json = JsonSerializer.Serialize(_data, options);
        File.WriteAllText(Constants.SomeFileName, json);
    }
    catch (Exception ex)
    {
        Logger.LogError($"Error saving ...", ex);
    }
}

Duplicated Load() code sample:

if (File.Exists(Constants.SomeFileName))
{
    try
    {
        var json = File.ReadAllText(Constants.SomeFileName);
        return JsonSerializer.Deserialize(T)(json) ?? new T();
    }
    catch (Exception ex)
    {
        Logger.LogError($"Error loading ...", ex);
    }
}
return new T();

Impact Analysis

• Maintainability: Any bug in the persistence logic (e.g., atomic write, encoding) must be fixed in three separate places.
• Bug Risk: High — inconsistent error message formats already exist (NotificationHistory omits ex.Message in the load error log, the others include it).
• Code Bloat: ~40 lines of near-duplicate code across three files.

Refactoring Recommendations

1. Extract a generic JsonFileStore(T) utility class

   • Create: src/JsonFileStore.cs
   • Expose T Load(string filePath, T defaultValue) and void Save(string filePath, T data) static/instance methods with built-in locking, serialization, and error handling.
   • Estimated effort: ~1–2 hours
   • Benefits: Single place to fix persistence bugs, consistent error logging, easy to extend (e.g., atomic writes via temp file)

2. Replace inline logic in all three classes with calls to the shared utility.

Implementation Checklist

• Review duplication findings
• Create src/JsonFileStore.cs with generic persistence helper
• Refactor NotificationHistory to use JsonFileStore
• Refactor ReviewRequestHistory to use JsonFileStore
• Refactor ReviewRequestService to use JsonFileStore
• Verify no functionality broken (manual smoke test)

Analysis Metadata

• Analyzed Files: 21 .cs files
• Detection Method: Pattern search across source files
• Commit: ebf44dd
• Analysis Date: 2026-03-17

Generated by Duplicate Code Detector · ◷

To install this agentic workflow, run

gh aw add github/gh-aw/.github/workflows/duplicate-code-detector.md@852cb06ad52958b402ed982b69957ffc57ca0619

Comments on the Issue (you are @copilot in this section)

• Fixes Duplicate Code: JSON Persistence (Load/Save) Pattern Across Three Storage Classes #155

---

📍 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 refactors JSON persistence by introducing a shared JsonFileStore helper and updating existing storage classes to use it, reducing duplicated load/save logic across the codebase.

Changes:

• Added JsonFileStore.Load<T> / JsonFileStore.Save<T> to centralize JSON file persistence and error handling.
• Refactored NotificationHistory, ReviewRequestHistory, and ReviewRequestService to use JsonFileStore instead of inline JSON/file I/O.

Reviewed changes

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

File	Description
src/JsonFileStore.cs	New centralized JSON load/save utility used by multiple storage classes.
src/NotificationHistory.cs	Replaces inline JSON persistence with JsonFileStore calls.
src/ReviewRequestHistory.cs	Replaces inline JSON persistence with JsonFileStore calls and keeps HashSet conversion.
src/ReviewRequestService.cs	Replaces inline JSON persistence with JsonFileStore calls for review request details storage.

---

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

[Copilot]

Logger.LogError(message, ex) already appends ex.Message to message when ex is provided (see Logger.LogError). Including {ex.Message} in the message here results in duplicated text in logs. Consider logging "Error loading {filePath}" (and similarly for saving) and rely on Logger to append exception details.

[Copilot]

Logger.LogError already appends ex.Message when an exception is passed. Building the message as "Error saving {filePath}: {ex.Message}" will duplicate the exception message in the final log entry. Use a message without ex.Message and pass ex so the logger can format details consistently.

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in 0bf0083. Removed {ex.Message} from both the Load and Save log messages in JsonFileStore.cs — Logger.LogError already appends : {ex.Message}\n{ex.StackTrace} when an exception is provided, so it was being duplicated in the final log entry.

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

These changes were already applied in 0bf0083. Both Load and Save log messages in JsonFileStore.cs no longer include {ex.Message} — the logger formats the exception details consistently on its own.

[Copilot]

Pull request overview

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

---

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

[Copilot]

The XML doc note about synchronization is only on Save(), but Load() can also race with concurrent Save() calls (e.g., read while another thread truncates/writes), leading to JsonException and loss of persisted state. Either extend the doc to cover both methods or centralize synchronization inside JsonFileStore (e.g., per-file lock) so callers can’t accidentally use it unsafely.

Suggested change

	namespace AgentSupervisor
	{
	internal static class JsonFileStore
	{
	public static T Load<T>(string filePath, T defaultValue)
	{
	if (File.Exists(filePath))
	{
	try
	{
	var json = File.ReadAllText(filePath);
	return JsonSerializer.Deserialize<T>(json) ?? defaultValue;
	}
	catch (Exception ex)
	{
	Logger.LogError($"Error loading {filePath}", ex);
	}
	}
	return defaultValue;
	}
	/// <summary>
	/// Serializes <paramref name="data"/> to JSON and writes it to <paramref name="filePath"/>.
	/// Callers are responsible for synchronization when concurrent access is possible.
	/// </summary>
	public static void Save<T>(string filePath, T data)
	{
	try
	{
	var options = new JsonSerializerOptions { WriteIndented = true };
	var json = JsonSerializer.Serialize(data, options);
	File.WriteAllText(filePath, json);
	}
	catch (Exception ex)
	{
	Logger.LogError($"Error saving {filePath}", ex);
	}
	using System.Collections.Concurrent;
	namespace AgentSupervisor
	{
	internal static class JsonFileStore
	{
	private static readonly ConcurrentDictionary<string, object> FileLocks = new();
	private static object GetFileLock(string filePath)
	{
	// Use full path to ensure a single lock per physical file.
	var fullPath = System.IO.Path.GetFullPath(filePath);
	return FileLocks.GetOrAdd(fullPath, _ => new object());
	}
	public static T Load<T>(string filePath, T defaultValue)
	{
	var fileLock = GetFileLock(filePath);
	lock (fileLock)
	{
	if (File.Exists(filePath))
	{
	try
	{
	var json = File.ReadAllText(filePath);
	return JsonSerializer.Deserialize<T>(json) ?? defaultValue;
	}
	catch (Exception ex)
	{
	Logger.LogError($"Error loading {filePath}", ex);
	}
	}
	}
	return defaultValue;
	}
	/// <summary>
	/// Serializes <paramref name="data"/> to JSON and writes it to <paramref name="filePath"/>.
	/// This method coordinates access with <see cref="Load{T}(string, T)"/> so concurrent
	/// reads and writes for the same file path are synchronized.
	/// </summary>
	public static void Save<T>(string filePath, T data)
	{
	var fileLock = GetFileLock(filePath);
	lock (fileLock)
	{
	try
	{
	var options = new JsonSerializerOptions { WriteIndented = true };
	var json = JsonSerializer.Serialize(data, options);
	File.WriteAllText(filePath, json);
	}
	catch (Exception ex)
	{
	Logger.LogError($"Error saving {filePath}", ex);
	}
	}

[Copilot]

JsonSerializerOptions is allocated on every Save() call. Since this helper centralizes JSON persistence, consider using a static readonly JsonSerializerOptions instance to avoid repeated allocations and keep formatting consistent across all callers.

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in 6efa1b2. Two changes applied to JsonFileStore.cs:

1. Added a ConcurrentDictionary<string, object> for per-file locks — both Load and Save now acquire the lock for the resolved full path, preventing read/write races between concurrent callers.
2. Extracted JsonSerializerOptions to a static readonly field to avoid a new allocation on every Save() call.