Refactor: eliminate triplicated JSON persistence, HTTP timing, and notify-locking patterns

[Copilot]

• Create src/JsonFileStore.cs generic thread-safe JSON file persistence helper
• Refactor NotificationHistory.cs to use JsonFileStore<T>
• Refactor ReviewRequestHistory.cs to use JsonFileStore<T>
• Refactor ReviewRequestService.cs to use JsonFileStore<T> and extract ExecuteAndNotify(Func<bool>) helper
• Refactor GitHubService.cs to extract SendTimedGetAsync(string url) helper
• Address review: use static shared JsonSerializerOptions in JsonFileStore<T> for both Load and Save
• Address review: use Stopwatch instead of DateTime.UtcNow deltas in SendTimedGetAsync
• Verify build passes (0 warnings, 0 errors)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Duplicate Code: JSON File Persistence Pattern Repeated Across History/Service Classes</issue_title>
<issue_description>Analysis of commit ebf44dd

Assignee: @copilot

Summary

Three classes independently implement nearly identical JSON file persistence logic (Load/Save with try/catch and a lock object), and one class repeats an HTTP timing/logging block across three methods. These duplications increase maintenance burden and risk inconsistent bug fixes.

---

Duplication #1 — JSON File Persistence (Most Significant)

Pattern: Thread-safe JSON Load/Save with identical structure

• Severity: High
• Occurrences: 3 classes
• Locations:
  • src/NotificationHistory.cs (lines 17–52, Load/Save methods)
  • src/ReviewRequestHistory.cs (lines 13–42, Load/Save methods)
  • src/ReviewRequestService.cs (lines 85–110, Load/Save methods)

Code Sample (nearly identical across all three):

private void Save()
{
    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);
        }
    }
}

private T Load()
{
    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();
}

Duplication #2 — HTTP Request Timing/Logging Block

Pattern: Repeated timing measurement around _httpClient.GetAsync

• Severity: Medium
• Occurrences: 3 methods in src/GitHubService.cs
• Locations:
  • GetCurrentUserAsync (~lines 57–65)
  • GetPendingReviewsAsync (~lines 110–119)
  • GetLatestReleaseAsync (~lines 245–253)

Code Sample:

var startTime = DateTime.UtcNow;
var response = await _httpClient.GetAsync(url);
var elapsed = (DateTime.UtcNow - startTime).TotalMilliseconds;
Logger.LogInfo($"HTTP Response: {(int)response.StatusCode} {response.StatusCode} | {elapsed:F0}ms | {url}");

Duplication #3 — notifyNeeded Locking Pattern

Pattern: bool notifyNeeded flag + post-lock observer notification

• Severity: Low-Medium
• Occurrences: 4 methods in src/ReviewRequestService.cs
• Locations:
  • AddOrUpdate (~lines 128–175)
  • MarkAsRead (~lines 177–188)
  • MarkAllAsRead (~lines 190–207)
  • RemoveStaleRequests (~lines 240–255)

Code Sample:

bool notifyNeeded = false;
lock (_lockObject)
{
    // ... mutate state ...
    notifyNeeded = true;
}
if (notifyNeeded)
{
    NotifyObservers();
}

---

Impact Analysis

• Maintainability: Any fix to the JSON serialization options, error message format, or file-writing strategy must be applied in 3 separate places. A missed update creates silent inconsistencies.
• Bug Risk: A bug in the Load error-handling (e.g., adding retry logic) would need to be replicated manually across all three classes.
• Code Bloat: ~60 lines of near-identical persistence logic could be reduced to a single generic utility.

---

Refactoring Recommendations

For Duplication #1 (JSON Persistence)

1. Extract a generic JsonFileStore(T) helper in src/JsonFileStore.cs:

   • Constructor accepts filePath and optionally maxEntries
   • Exposes T Load() and void Save(T data) with the shared try/catch and lock
   • Estimated effort: 2–3 hours
   • Benefits: Single place to improve serialization options, error handling, or add atomic writes

2. Refactor NotificationHistory, ReviewRequestHistory, and ReviewRequestService to delegate load/save to JsonFileStore(T)

For Duplication #2 (HTTP Timing)

1. Extract SendTimedGetAsync(string url) as a private helper in GitHubService.cs:
   • Wraps _httpClient.GetAsync, measures elapsed time, logs the result
   • Returns HttpResponseMessage
   • Estimated effort: 30 minutes

For Duplication #3 (notifyNeeded)

1. Extract ExecuteAndNotify(Action action) helper in ReviewRequestService.cs:
   • Runs action inside a lock, returns bool indicating change
   • Calls NotifyObservers() if change detected
   • Estimated effort: 1 hour

---

Implementation Checklist

• Review duplication findings
• Prioritize refactoring tasks (suggest: Implement Windows system tray bot for GitHub PR review monitoring with desktop notifications, logging, and proxy support #1 first, then 'net6.0-windows' is out of support #2, then sunzhuoshi/agent-super...

• Fixes Duplicate Code: JSON File Persistence Pattern Repeated Across History/Service Classes #153

---

📍 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

Refactors repeated persistence, HTTP timing/logging, and observer-notification locking patterns into shared helpers to reduce duplication across the app.

Changes:

• Introduces JsonFileStore<T> to centralize thread-safe JSON load/save for multiple history/service classes.
• Refactors ReviewRequestService to use ExecuteAndNotify(Func<bool>) to unify lock + conditional observer notifications.
• Refactors GitHubService to use SendTimedGetAsync(string url) to unify GET timing/logging.

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
src/JsonFileStore.cs	Adds generic JSON file persistence helper used by multiple components.
src/NotificationHistory.cs	Switches persistence to JsonFileStore<List<NotificationEntry>>.
src/ReviewRequestHistory.cs	Switches persistence to JsonFileStore<List<string>> and rebuilds HashSet from loaded list.
src/ReviewRequestService.cs	Delegates persistence to JsonFileStore and centralizes lock+notify via ExecuteAndNotify.
src/GitHubService.cs	Centralizes GET timing/logging via SendTimedGetAsync.

---

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

[Copilot]

JsonFileStore<T> creates a new JsonSerializerOptions instance on every Save call and doesn't apply any options on Load. Since this helper is meant to centralize persistence behavior, consider using a single shared JsonSerializerOptions field (and passing it to both Serialize and Deserialize) to avoid option drift and repeated allocations.

[Copilot]

SendTimedGetAsync measures elapsed time using DateTime.UtcNow deltas, which can be skewed by system clock adjustments (NTP/manual changes) and lead to inaccurate/negative durations. Prefer a monotonic timer like Stopwatch for elapsed timing measurements.

[sunzhuoshi]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Done in 9ce2cff:

• JsonFileStore<T>: replaced the per-call new JsonSerializerOptions with a static readonly field shared by both Serialize (Save) and Deserialize (Load), eliminating repeated allocations and option drift.
• SendTimedGetAsync: replaced the DateTime.UtcNow delta with Stopwatch.StartNew() / Stop() for monotonic, clock-skew-resistant elapsed timing.