feat: CodeMirror 6 side-by-side diff editor

[PureWeen]

CodeMirror 6 Diff Editor

Adds a side-by-side diff editor using CodeMirror 6, integrated into the existing DiffView component.

Features

• Side-by-side MergeView with syntax highlighting for C#, JS/TS, Python, CSS, HTML, JSON, XML/XAML, Markdown, Bash
• Table/Editor toggle per diff file — switch between existing table view and CodeMirror
• Collapsed unchanged regions (like GitHub PR view)
• Line numbers on both sides with gutter change markers
• Custom dark theme matching PolyPilot's palette
• Search (Ctrl+F), fold gutters, bracket matching
• Pre-built 661KB bundle — works offline, no CDN dependency

Technical Details

• CodeMirror 6 bundle built from codemirror-src/ via esbuild (reproducible)
• DiffParser.ReconstructOriginal/ReconstructModified extract both sides from parsed diffs
• Instance lifecycle managed via JS Map + IAsyncDisposable
• 3 new tests for reconstruct methods
• Also fixes pre-existing ChangeModelAsync test compilation errors (missing named param)

Verified

• Build: 0 errors
• Tests: 3132/3132 pass
• MauiDevFlow: C# syntax highlighting, diff colors, collapsed regions all confirmed via screenshots

[PureWeen]

🔍 Multi-Reviewer Code Review — PR #507 (Re-review)

CodeMirror 6 Side-by-Side Diff Editor

3 independent reviewers re-analyzed this PR. Tests: ✅ 3141/3141 pass. CI: ⚠️ No checks configured on this branch.
No code changes since prior review — all previous findings remain unaddressed.

---

Previous Findings — Status

🔴 #1 — Fire-and-forget DisposeAllCmInstancesAsync + synchronous Clear() leaks all JS instances — ❌ STILL PRESENT

File: PolyPilot/Components/DiffView.razor lines 106–108, 176–179
Confirmed by: All 3 reviewers

_ = DisposeAllCmInstancesAsync();   // starts iterating _cmInstances.Values
_viewModes.Clear();
_cmInstances.Clear();               // invalidates enumerator mid-await

The fire-and-forget task iterates _cmInstances.Values. After the first await JS.InvokeVoidAsync(...) yields, _cmInstances.Clear() runs synchronously → InvalidOperationException from the enumerator on MoveNext(). Only the first instance (at best) is disposed; all others leak permanently. The exception is an unobserved faulted task.

---

🔴 #2 — Stale InitCodeMirrorForFile writes orphaned instance ID after parameter update — ❌ STILL PRESENT

File: PolyPilot/Components/DiffView.razor lines 152–157
Confirmed by: All 3 reviewers

No generation counter. If OnParametersSet fires a new diff while JS.InvokeAsync<int>("...createMergeView") is in-flight, the stale completion writes _cmInstances[fileIdx] = staleId into the freshly-cleared dictionary. The orphaned JS instance is never disposed.

---

🟡 #3 — Double-init creates orphaned CM instance on rapid toggle — ❌ STILL PRESENT

File: PolyPilot/Components/DiffView.razor lines 113–118
Confirmed by: All 3 reviewers

No guard against re-entering InitCodeMirrorForFile for the same fileIdx. Two concurrent OnAfterRenderAsync calls can each create a CM instance — whichever writes _cmInstances[fileIdx] last wins; the other is orphaned in the JS instances Map forever.

New detail (2/3 reviewers): On the JS side, createMergeView does container.innerHTML = '' which destroys the old DOM but does NOT call instances.get(oldId).mergeView.destroy() — the old CM instance's ViewPlugin subscriptions, MutationObservers, and event listeners remain live in the JS heap. This is a reliable memory leak per toggle-pair.

---

🟡 #4 — Fire-and-forget DisposeCmInstance in SetViewMode races with DisposeAsync — ❌ STILL PRESENT

File: PolyPilot/Components/DiffView.razor line 124
Confirmed by: All 3 reviewers

DisposeCmInstance calls _cmInstances.Remove(fileIdx) then awaits JS interop. If DisposeAsync → DisposeAllCmInstancesAsync is simultaneously iterating _cmInstances.Values, the Remove increments the version counter → InvalidOperationException on the enumerator.

New detail (2/3 reviewers): SetViewMode(Table) also does not remove the _pendingEditorInit entry, so a rapid Editor→Table toggle can still execute a stale init in the next OnAfterRenderAsync.

---

🟡 #5 — ReconstructOriginal/Modified omit inter-hunk gap lines — ❌ STILL PRESENT

File: PolyPilot/Models/DiffParser.cs lines 324–354
Confirmed by: All 3 reviewers

Multi-hunk diffs concatenate without gap markers. For a 500-line file with hunks at lines 1–5 and 490–495, the CM MergeView shows 10 lines with wrong line numbers. The multi-hunk test uses Assert.Contains per-line and passes while the gap-filling bug remains.

---

🟡 #6 — Pre-built 661KB bundle without integrity verification — ❌ STILL PRESENT

File: PolyPilot/wwwroot/index.html line 1447
Confirmed by: All 3 reviewers

No SRI hash, no CI reproducibility check.

---

🟢 #7 — O(n²) Files.IndexOf(file) in render loop — ❌ STILL PRESENT

File: PolyPilot/Components/DiffView.razor line 15
Confirmed by: All 3 reviewers

---

🟢 #8 — Bare catch {} blocks suppress all diagnostics — ⚠️ PARTIALLY FIXED

File: PolyPilot/Components/DiffView.razor lines 161, 169, 178
Confirmed by: All 3 reviewers

One catch block gained a comment explaining intent; dispose catches remain completely silent without logging.

---

New Findings (this re-review)

🟡 NEW-1 — DisposeAllCmInstancesAsync post-loop Clear() wipes newly-added valid instances

File: PolyPilot/Components/DiffView.razor line 180
Flagged by: 2/3 reviewers

Scenario: Fire-and-forget DisposeAllCmInstancesAsync is suspended between iterations. Meanwhile, OnAfterRenderAsync writes _cmInstances[newIdx] = freshId for a new diff. When the dispose loop finishes, _cmInstances.Clear() on line 180 removes freshId without ever calling JS dispose(freshId). This is distinct from findings #1 and #2 — it's about the post-loop Clear() wiping legitimately new entries.

---

ℹ️ Discarded Findings (1/3 only)

• No _disposed guard in DisposeAsync — Real concern in theory, but Blazor's sync context serializes lifecycle methods. Not confirmed by other reviewers.
• Trailing empty-string line parsed as context — Potential parser edge case, not confirmed by other reviewers.
• No test coverage for JS lifecycle bugs — Valid observation but more an acknowledgment than a code defect.

---

Recommendation: ⚠️ Request Changes

No code changes have been made since the initial review. All 8 previous findings remain unaddressed (7 fully, 1 partially). The two 🔴 CRITICAL findings (fire-and-forget disposal race + stale init writes) will cause JS-side memory leaks every time a new diff is loaded. One new 🟡 MODERATE finding was discovered.

Specific asks before merge:

1. Fix the fire-and-forget disposal: snapshot IDs before clearing, or use OnParametersSetAsync + await
2. Add a generation counter to InitCodeMirrorForFile to guard against stale writes
3. Add a double-init guard (check _cmInstances.ContainsKey before creating)
4. Remove _pendingEditorInit entries on Table toggle; consolidate disposal through OnAfterRenderAsync
5. Replace Files.IndexOf(file) with indexed for loop
6. Add at minimum catch (Exception ex) { Console.Error.WriteLine(...) } to catch blocks