Polish sidebar floating controls and session status bars

[Redth]

Summary

• refine the floating sidebar toolbar and status chips so they don’t obscure content and feel more integrated
• polish the bottom session status bar with cleaner grouping, separators, and right-aligned model/token/context info
• smooth the sidebar header fade and reduce the translucency of overlay pills for better readability while scrolling

Testing

• dotnet build -f net10.0-maccatalyst

[PureWeen]

🤖 Multi-Model Code Review — PR #485 R1

PR: Polish sidebar floating controls and session status bars
Diff: 469 lines, 7 files, 3 commits
Models: claude-opus-4.6, claude-sonnet-4.6, gpt-5.3-codex
CI: ⚠️ No checks reported on this branch

---

🔴 CRITICAL — Blank output for malformed diffs (ChatMessageItem.razor ~234–249)

Flagged by: Opus, Sonnet, Codex (3/3)

The TryExtractNumberedViewOutput fallback branch is unreachable due to the rendering logic structure. When _hasDiffOutput is true (so ShouldRenderDiffView returned true), the code enters the outer else if (_hasDiffOutput) block. Inside, if _parsedDiff.Count > 0 is false (malformed diff), there is no fallback — the output silently renders blank.

The TryExtractNumberedViewOutput branch exists as a peer else if or nested branch, but it can never succeed:

• If nested: _hasDiffOutput == true means ShouldRenderDiffView returned true, which means TryExtractNumberedViewOutput already returned false inside ShouldRenderDiffView (line 37 of DiffParser.cs). So the nested call will always return false → dead branch.
• If peer: The outer else if (_hasDiffOutput) already matched, so the peer else if is never evaluated → also unreachable.

Either way, when Parse() returns 0 files for a diff-like string, the user sees blank output instead of the raw content. This is a user-visible data-loss regression vs. the previous code which had an else { <pre>@TruncateResult(Content)</pre> } fallback.

Fix: The TryExtractNumberedViewOutput fallback should be a peer of the _hasDiffOutput block (not nested), and the _hasDiffOutput block needs its own raw-content else branch restored:

else if (_hasDiffOutput)
{
    var _parsedDiff = DiffParser.Parse(Message.Content);
    if (_parsedDiff.Count > 0)
    {
        <div class="action-output-diff"><DiffView RawDiff="@Message.Content" /></div>
    }
    else
    {
        <pre>@ChatMessageList.TruncateResult(Message.Content)</pre>
    }
}
else if (DiffParser.TryExtractNumberedViewOutput(Message.Content, out var _plainViewOutput))
{
    <pre>@ChatMessageList.TruncateResult(_plainViewOutput)</pre>
}
else
{
    ...
}

---

Items below consensus threshold (1/3 — not blocking)

• Events.cs unconditional normalization (Codex only): TryExtractNumberedViewOutput in the ToolExecutionComplete handler applies to all tools, not just view. However, the method only matches all-context-line self-diffs (no real additions/removals), which are definitively synthetic — false-positive risk is negligible.
• Dead guard in loop (Opus only): The Removed line check inside TryExtractNumberedViewOutput is unreachable since the early-return already filtered to context-only lines. Harmless but misleading.
• Double parse (Opus only): ShouldRenderDiffView → TryExtractNumberedViewOutput → Parse(), then razor calls Parse() again. Minor perf concern for large outputs.

---

Test Coverage

4 new tests cover ShouldRenderDiffView and TryExtractNumberedViewOutput happy paths. Missing: test for the blank-output scenario when Parse() returns 0 files but LooksLikeUnifiedDiff is true.

Verdict: ⚠️ Request Changes

Required: Fix the blank-output regression in ChatMessageItem.razor where malformed diff-like content silently disappears instead of showing raw text.

[Redth]

�� Multi-Model Code Review — PR #485 R3

PR: Polish sidebar floating controls and session status bars
Diff: 483 lines, 7 files, 5 commits
**Models:**claude-opus-4.6, claude-sonnet-4.6, gpt-5.3-codex
CI: ⚠️ No checks reported on this branch

---

Re-review status

Prior finding	Status
🔴 Blank output for malformed diffs in ChatMessageItem.razor	✅ FIXED
🟡 Events.cs normalization applied to all tools	✅ FIXED
🟡 BottomBarTooltipTests still expected .status-msgs	✅ FIXED

Findings

No remaining substantive issues found after the latest fixes.

• PolyPilot/Services/CopilotService.Events.cs now scopes the synthetic numbered-output normalization to plain file-read tools (view/read), eliminating the earlier risk of rewriting unrelated tool output.
• PolyPilot.Tests/BottomBarTooltipTests.cs now matches the new .log-msg-count markup, and the inline count also has its own tooltip title.
• PolyPilot/Models/DiffParser.cs now correctly handles multi-file standard unified diffs without diff --git headers and preserves hunk lines that begin with repeated ---/+++ content.

Consensus notes

• Opus: no significant issues found; prior R1/R2 findings are resolved.
• Sonnet: no significant issues found; confirmed the parser change is a correctness improvement.
• Codex: raised only a possible missing integration test for the event-handler normalization path. After adversarial review, the other models disagreed that this is a review-worthy issue because the behavior is already sufficiently covered by the new parser/unit tests and the Events.cs change is a thin, low-risk call site.

Test coverage

Coverage is good for the new parser behavior:

• Added regression tests for multi-file non-git unified diffs
• Added regression tests for hunk lines that look like file headers
• Updated tooltip coverage for the revised bottom-bar markup

No additional blocking test gaps remain for this PR.

Verdict: ✅ Approve

The previously reported issues are fixed, the re-review did not uncover any new consensus bugs, and the new code paths now have appropriate regression coverage.

[PureWeen]

🤖 Multi-Model Code Review — PR #485 R2

PR: Polish sidebar floating controls and session status bars
Diff: 456 lines, 7 files, 4 commits (1 new since R1)
Models: claude-opus-4.6, claude-sonnet-4.6, gpt-5.3-codex
CI: ⚠️ No checks reported on this branch

---

R1 Finding Status

R1 Finding	Status
🔴 Blank output for malformed diffs (ChatMessageItem.razor ~234-249)	✅ FIXED (3/3 confirm)

The TryExtractNumberedViewOutput branch is now a peer else if after the _hasDiffOutput block. Both paths verified:

• Malformed diff (LooksLikeUnifiedDiff=true, Parse()=empty) → inner else { <pre> } renders raw content ✅
• Synthetic view-tool self-diff → peer else if TryExtractNumberedViewOutput renders numbered text ✅
• Dead Removed guard removed from loop ✅
• New test Parse_MalformedDiffLikeMarkersSeparated_ReturnsEmptyWhileLookingDiffLike covers the edge case ✅

---

New R2 Findings

🟡 MODERATE — Events.cs normalization applies to all tools, not just view (lines 400–403)

Flagged by: Opus, Sonnet (2/3)

TryExtractNumberedViewOutput(resultStr, ...) runs on every ToolExecutionCompleteEvent, not scoped to view/read tools. If a bash command happens to output a synthetic self-diff (context-only, all lines identical), the diff headers would be silently discarded before storage.

Mitigating factor: TryExtractNumberedViewOutput only matches all-context-line diffs with valid parse results — an extremely narrow match. The risk is very low in practice, and the numbered format is more useful than a context-only diff for display. Not blocking, but scoping to view-related tool names would be a clean improvement.

🟡 MODERATE — Test breakage: .status-msgs class removed but test still expects it

Flagged by: Codex (1/3), verified by reviewer

PolyPilot.Tests/BottomBarTooltipTests.cs:64 asserts class="status-msgs" exists in ExpandedSessionView. The R2 diff replaces <span class="status-msgs"> with the count embedded in .log-label. Test ExpandedSessionView_MessageCount_HasTitle will fail.

Impact: Test suite regression. Easy fix — update the regex to match the new .log-msg-count / .log-label markup, or remove the assertion if the separate tooltip is no longer needed.

---

Items below consensus (informational only)

• direction: rtl on <select> — minor a11y concern (Sonnet only)
• Redundant double-parse per render (Opus only)
• CSS magic numbers for floating filter bar (Sonnet only)

---

Test Coverage

5 new tests cover ShouldRenderDiffView and TryExtractNumberedViewOutput including the malformed-diff edge case. Missing: update to BottomBarTooltipTests for removed .status-msgs class.

Verdict: ⚠️ Request Changes (minor)

The R1 CRITICAL is properly fixed. Two remaining items:

1. Required: Update BottomBarTooltipTests.ExpandedSessionView_MessageCount_HasTitle to match the new markup (broken test)
2. Optional: Scope TryExtractNumberedViewOutput normalization in Events.cs to view-related tools only

[Redth]

�� Multi-Model Code Review — PR #485 R3

PR: Polish sidebar floating controls and session status bars
Diff: 483 lines, 7 files, 5 commits
Models: claude-opus-4.6, claude-sonnet-4.6, gpt-5.3-codex
CI: ⚠️ No checks reported on this branch

---

Re-review status

Prior finding	Status
🔴 Blank output for malformed diffs in ChatMessageItem.razor	✅ FIXED
🟡 Events.cs normalization applied to all tools	✅ FIXED
🟡 BottomBarTooltipTests still expected .status-msgs	✅ FIXED

Findings

No remaining substantive issues found after the latest fixes.

• PolyPilot/Services/CopilotService.Events.cs now scopes the synthetic numbered-output normalization to plain file-read tools (view/read), eliminating the earlier risk of rewriting unrelated tool output.
• PolyPilot.Tests/BottomBarTooltipTests.cs now matches the new .log-msg-count markup, and the inline count also has its own tooltip title.
• PolyPilot/Models/DiffParser.cs now correctly handles multi-file standard unified diffs without diff --git headers and preserves hunk lines that begin with repeated ---/+++ content.

Consensus notes

• Opus: no significant issues found; prior R1/R2 findings are resolved.
• Sonnet: no significant issues found; confirmed the parser change is a correctness improvement.
• Codex: raised only a possible missing integration test for the event-handler normalization path. After adversarial review, the other models disagreed that this is a review-worthy issue because the behavior is already sufficiently covered by the new parser/unit tests and the Events.cs change is a thin, low-risk call site.

Test coverage

Coverage is good for the new parser behavior:

• Added regression tests for multi-file non-git unified diffs
• Added regression tests for hunk lines that look like file headers
• Updated tooltip coverage for the revised bottom-bar markup

No additional blocking test gaps remain for this PR.

Verdict: ✅ Approve

The previously reported issues are fixed, the re-review did not uncover any new consensus bugs, and the new code paths now have appropriate regression coverage.

[PureWeen]

🤖 Multi-Model Code Review — PR #485 R3

PR: Polish sidebar floating controls and session status bars
Diff: 567 lines, 8 files, 5 commits (1 new since R2)
Models: claude-opus-4.6, claude-sonnet-4.6, gpt-5.3-codex
CI: ⚠️ No checks reported on this branch
Tests: All 3,118 pass ✅ (verified by Opus sub-agent)

---

Prior Finding Status — All Fixed ✅

Finding	R1	R2	R3
🔴 Dead fallback branch (ChatMessageItem.razor)	CRITICAL	✅ FIXED	✅ Confirmed
🟡 Events.cs normalization scope too broad	—	NEW	✅ FIXED (scoped via IsPlainTextViewTool)
🟡 BottomBarTooltipTests regex breakage	—	NEW	✅ FIXED (updated to log-msg-count + title on span)

---

R3 Consensus Findings

🟢 MINOR — Double Parse() per render for synthetic view output

Flagged by: Opus, Sonnet (2/3)

ShouldRenderDiffView() → TryExtractNumberedViewOutput() → Parse() determines it's a synthetic self-diff (returns false). Then the peer else if TryExtractNumberedViewOutput() calls Parse() again. Two O(n) parses per render for the same content.

Mitigating factors: (1) Events.cs normalizes at write time, so replayed content usually won't trigger LooksLikeUnifiedDiff, meaning the Razor path rarely fires. (2) Only affects synthetic view/read results. (3) Not a correctness issue.

---

Below consensus threshold (informational)

Parse() hunk-interior --- false boundary (Sonnet only, 1/3): The old code had current == null && guard preventing --- detection inside an active hunk. The new code removed it, relying on the ---/+++/@@ triplet. A removed line with content -- old appears as --- old in diff, which could theoretically trigger a false file boundary if followed by +++ and @@. Extremely narrow edge case (requires -- →++ replacement at exact hunk boundary). Opus explicitly traced and deemed safe; Codex found nothing. The triplet requirement makes this practically unreachable.

---

Test Coverage

7 new tests cover ShouldRenderDiffView, TryExtractNumberedViewOutput, malformed diffs, standard unified multi-file parsing, and hunk-interior file-like headers. BottomBarTooltipTests updated. Good coverage.

Verdict: ✅ Approve

All prior findings (R1 CRITICAL + both R2 MODERATEs) are fixed. The only consensus finding is a minor double-parse performance nit. The Parse() improvements (triplet detection, narrowed skip-lines) are well-tested. Ship it.