Extract shared JsonSerializerOptions to eliminate duplicate allocations#774
Extract shared JsonSerializerOptions to eliminate duplicate allocations#774
Conversation
Replace 11 inline `new JsonSerializerOptions { WriteIndented = true }`
allocations across 7 files with a shared `JsonDefaults.Indented` static
readonly field. Each inline allocation bypassed System.Text.Json's internal
caching; the shared instance is reused for every serialization call.
New file: PolyPilot/Models/JsonDefaults.cs
Modified: CopilotService.Events.cs, CopilotService.Organization.cs,
CopilotService.Persistence.cs, CopilotService.cs,
RepoManager.cs, ScheduledTaskService.cs, ConnectionSettings.cs
Test csproj: added Compile Include for JsonDefaults.cs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔍 Design-Level Findings (outside diff)🟡 MODERATE — Incomplete refactor: two inline allocations remain (Flagged by 2/3 reviewers) The PR description states all 11 inline
Both are identical 🟢 MINOR — Pre-existing duplicate static instance (Flagged by 2/3 reviewers)
private static readonly JsonSerializerOptions SaveOptions = new() { WriteIndented = true };This is now a semantic duplicate of One additional finding (type metadata cache growth from shared options in
|
|
/review |
|
✅ Expert Code Review completed successfully! |
Expert Code Review — PR #774PR: Extract shared Methodology3 independent reviewers with adversarial consensus (Reviewer 1: deep reasoning, Reviewer 2: pattern matching, Reviewer 3: alternative perspective). Findings
Discarded Findings
Verified Safe (3/3 agree)
CI Status
Test Coverage
SummaryClean mechanical refactor. All 11 converted call sites are correct. The 3 missed sites (findings #2–#4) are pre-existing and outside the diff — not regressions — but would complete the consolidation goal. Finding #1 (
Warning
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary — PR #774: Extract shared JsonSerializerOptions
The core refactor is correct: JsonSerializerOptions is a pure BCL type with no platform API dependencies, so static readonly is safe here (no TypeInitializationException risk per project conventions). Thread safety after the first freeze is guaranteed by System.Text.Json. All 3 reviewers confirmed zero behavioral regressions, no data loss risks, and no race conditions.
Findings
🟡 MODERATE — MakeReadOnly() not called (3/3 reviewers)
File: PolyPilot/Models/JsonDefaults.cs:7 (inline comment above)
readonly prevents field reassignment only — not mutation of the object. The instance is fully mutable until first use. Any future code adding a converter or changing a property before the first serialization would silently corrupt global serialization behavior; after freeze it would throw an opaque InvalidOperationException far from the actual mutation site. Fix: call opts.MakeReadOnly() before returning from a factory method (see inline comment for full fix).
🟢 MINOR — One migration site missed (3/3 reviewers)
File: PolyPilot/Services/CopilotService.Events.cs ~line 3596 (outside diff)
A zero-idle diagnostic capture path still uses new System.Text.Json.JsonSerializerOptions { WriteIndented = true } and was not converted. Not a regression, but leaves the PR's stated goal incomplete and will confuse future maintainers about whether the omission was intentional.
Fix: var json = System.Text.Json.JsonSerializer.Serialize(capture, JsonDefaults.Indented);
i️ Informational — Dead #if branch in ConnectionSettings.cs (pre-existing, not introduced)
After this PR both the IOS || ANDROID and #else branches contain identical JsonSerializer.Serialize(this, JsonDefaults.Indented) calls. The structural #if/#else split around the serialization line is pre-existing dead weight that could be collapsed, but this is outside scope for this PR.
Warning
⚠️ Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
api.nuget.orgdc.services.visualstudio.comlearn.microsoft.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.nuget.org"
- "dc.services.visualstudio.com"
- "learn.microsoft.com"See Network Configuration for more information.
Generated by Expert Code Review for issue #774 · ● 13.6M
|
|
||
| internal static class JsonDefaults | ||
| { | ||
| internal static readonly JsonSerializerOptions Indented = new() { WriteIndented = true }; |
There was a problem hiding this comment.
🟡 MODERATE — Shared instance not hardened against mutation (3/3 reviewers)
readonly only prevents reassignment of the field reference; it does not prevent mutation of the object's properties or Converters list. JsonSerializerOptions is fully mutable from construction until the first JsonSerializer.Serialize() call, at which point System.Text.Json internally freezes it.
Concrete failing scenario: Any future code that calls JsonDefaults.Indented.Converters.Add(...) or sets .PropertyNamingPolicy etc. before the first serialization will silently corrupt every consumer in the assembly. After freeze, the same attempt throws InvalidOperationException from deep inside System.Text.Json with no stack frame pointing back here — a hard-to-diagnose crash.
All current call sites are pure reads so there is no immediate regression, but the field is internal (accessible throughout the assembly) with no compile-time guard.
Fix: Call MakeReadOnly() immediately after construction (.NET 7+, this project targets .NET 10):
internal static class JsonDefaults
{
internal static readonly JsonSerializerOptions Indented = CreateIndented();
private static JsonSerializerOptions CreateIndented()
{
var opts = new JsonSerializerOptions { WriteIndented = true };
opts.MakeReadOnly();
return opts;
}
}This causes any mutation attempt to throw immediately and deterministically at the mutation site rather than producing mysterious downstream failures.
1 participant
Replace 11 inline
new JsonSerializerOptions { WriteIndented = true }allocations across 7 files with a sharedJsonDefaults.Indentedstatic readonly field.Each inline call allocated a new
JsonSerializerOptionsinstance on every invocation, bypassing System.Text.Json's internal caching. The sharedstatic readonlyfield is safe here —JsonSerializerOptionsis a pure BCL type with no platform API dependencies.Changes
PolyPilot/Models/JsonDefaults.cs—internal static classwithIndentedfieldCopilotService.Events.cs(line 1496)CopilotService.Organization.cs(lines 458, 3059)CopilotService.Persistence.cs(lines 159, 1461, 1557)CopilotService.cs(line 1966)RepoManager.cs(line 359)ScheduledTaskService.cs(line 493)ConnectionSettings.cs(lines 334, 336)<Compile Include>forJsonDefaults.csVerification
dotnet build PolyPilot/PolyPilot.csproj -f net10.0-maccatalyst✅ 0 errorsdotnet test PolyPilot.Tests/PolyPilot.Tests.csproj✅ 3565 passed, 0 failedZero behavioral change — pure mechanical refactor.