fix: Re-check PR badge when session becomes active

[PureWeen]

When clicking a session in the sidebar or switching expanded views, invalidate the PrLinkService cache and re-fetch if no PR was previously found. This ensures PR badges appear promptly after creating a PR or switching branches.

Changes:

• SessionListItem: Track IsActive transitions; on activation, invalidate cache + re-fetch if no PR cached
• ExpandedSessionView: Re-check on session switch if no PR cached

No change to SessionCard (dashboard) — the 5-minute cache TTL handles that naturally.

[PureWeen]

🔍 Multi-Model Code Review — PR #469 (R1)

PR: fix: Re-check PR badge when session becomes active
Review type: R1 (first review)
Models: Claude Opus 4.6, Claude Sonnet 4.6, GPT-5.3-Codex
CI Status: ⚠️ No CI checks reported on this branch

---

Consensus Findings (2+ of 3 models agree)

🔴 CRITICAL — ExpandedSessionView: Fetch storm on every render when no PR exists

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

File: PolyPilot/Components/ExpandedSessionView.razor, ~lines 689–696 (new else if (_prUrl == null) block)

The new else if (_prUrl == null) branch has no activation-transition guard (unlike SessionListItem's becameActive check). OnParametersSet fires on every parent re-render — including every AssistantMessageDeltaEvent during streaming (~10–20×/sec). When no PR exists for the repo:

1. _prUrl stays null after fetch (no PR found)
2. dir == _lastPrCheckedDir (same session)
3. Every render: Invalidate(dir) clears the cache → FetchPrUrlAsync(dir) spawns a new gh pr view process
4. Before the 15-second gh process completes, dozens more are spawned

This creates an unbounded process-spawning storm for the common "no PR" case during streaming responses. The 5-minute cache TTL is defeated because Invalidate() is called before every fetch.

Note: SessionListItem.razor does NOT have this problem — the becameActive edge detector (IsActive && !_wasActive) correctly limits re-fetch to a single event on activation.

Fix: Add an edge guard similar to SessionListItem, e.g.:

• Track a _prFetchInFlight boolean to prevent concurrent fetches
• Or add a timestamp debounce (_lastPrCheckTime)
• Or restrict to an explicit session-switch signal

---

🟡 MODERATE — No fetch-in-flight guard allows overlapping process spawns

Flagged by: Opus, Sonnet | Not flagged by: Codex (flagged as minor concurrent fetch concern)

Files: Both ExpandedSessionView.razor and SessionListItem.razor, FetchPrUrlAsync calls

Neither component has a _fetchInFlight guard. For SessionListItem, the becameActive edge detection provides adequate protection in practice. For ExpandedSessionView, the missing guard directly amplifies the storm above. Even without the storm, a slow first fetch (15s timeout) can overlap with a second triggered fetch, causing a race where the last-to-complete wins on _prUrl. Each concurrent pair spawns OS processes (git rev-parse + gh pr view).

Fix: Add _prFetchInFlight boolean guard:

private bool _prFetchInFlight;
private async Task FetchPrUrlAsync(string dir)
{
    if (_prFetchInFlight) return;
    _prFetchInFlight = true;
    try { /* existing logic */ }
    finally { _prFetchInFlight = false; }
}

---

Items NOT reaching consensus (1 of 3 models only)

Finding	Flagged by	Why excluded
Stale PR badge race on rapid session switch	Opus only	Valid concern (captured-session check missing in FetchPrUrlAsync), but Sonnet/Codex didn't flag it
Cross-component cache invalidation redundancy	Sonnet only	Design smell, not a bug
Fire-and-forget exception handling	All 3 (as pre-existing)	Pre-existing pattern, not introduced by this PR

---

Verification Summary

Check	Result
SessionListItem becameActive guard	✅ Correctly implements edge detection
ExpandedSessionView activation guard	❌ Missing — fires on every render
PrLinkService.Invalidate() exists	✅ Already in codebase
Fire-and-forget safety	✅ Service-level catch-all exists (pre-existing)

---

Recommended Action

⚠️ Request changes — The ExpandedSessionView fetch storm (🔴 CRITICAL) is a real regression that will spawn hundreds of gh processes during streaming for any session without a PR. This must be fixed before merging.

Specific ask:

1. Add a _prFetchInFlight guard to ExpandedSessionView.FetchPrUrlAsync to prevent overlapping spawns
2. Add an edge-detection guard (similar to SessionListItem's becameActive) so the _prUrl == null re-check only fires once per session switch, not on every render cycle

[PureWeen]

🔄 R2 Multi-Model Re-Review — PR #469

PR: fix: Re-check PR badge when session becomes active
Models: Claude Opus 4.6 ✅, Claude Sonnet 4.6 ✅, GPT-5.3-Codex ✅
CI Status: ⚠️ No checks configured
New commits since R1: ❌ None — R1 findings are NOT addressed

---

R1 Findings — Status

🔴 CRITICAL — ExpandedSessionView fetch storm (3/3 models confirm STILL PRESENT)

File: PolyPilot/Components/ExpandedSessionView.razor ~lines 689–696

The else if (_prUrl == null) branch fires on every render via OnParametersSet. During streaming responses (~10-20 re-renders/sec), when no PR exists for the repo:

1. _prUrl stays null (no PR found)
2. Invalidate(dir) clears the cache before each fetch
3. FetchPrUrlAsync(dir) spawns a new gh pr view process
4. Before the 15s gh process completes, dozens more are spawned

This is unbounded process spawning. The 5-minute cache TTL is defeated because Invalidate() is called on every render.

SessionListItem.razor does NOT have this problem — the becameActive edge detector correctly limits re-fetch to one event per activation transition.

Fix needed: Add an activation-transition guard to ExpandedSessionView (e.g., track session identity changes or a _hasRetried flag).

---

🟡 MODERATE — No fetch-in-flight guard (3/3 models confirm STILL PRESENT)

Files: ExpandedSessionView.razor and SessionListItem.razor

Neither component has a _prFetchInFlight boolean to prevent overlapping FetchPrUrlAsync calls. For SessionListItem, the becameActive guard provides adequate protection. For ExpandedSessionView, the missing guard directly amplifies the storm.

Fix needed: Add _prFetchInFlight guard to FetchPrUrlAsync.

---

🟡 MODERATE — Invalidate() defeats cache + cross-component blast (2/3: Sonnet, Codex)

NEW finding — PrLinkService.Invalidate(dir) is called before every re-fetch in both components. Since PrLinkService is a singleton, invalidation in one component busts the cache for all components watching the same directory. Combined with the ExpandedSessionView storm, this means the cache is permanently defeated during streaming.

Fix: Remove Invalidate() calls, or only call on genuine user actions (session switch, not re-renders).

---

Recommended Action: ⚠️ Request Changes

The CRITICAL fetch storm from R1 is unaddressed. This PR will cause unbounded gh pr view process spawning for any session without an associated PR during streaming responses. Minimum fix:

1. Add a transition guard to ExpandedSessionView (like SessionListItem's becameActive)
2. Add _prFetchInFlight guard to prevent concurrent fetches
3. Remove or debounce Invalidate() calls

---

R2 re-review with 3/3 models. Previous: R1. No new commits since R1.

[PureWeen]

🔍 PR #469 — R3 Multi-Model Re-Review (Post-Fix)

Models: Claude Opus 4.6, Claude Sonnet 4.6, GPT-5.3 Codex | CI: No checks configured

Previous Findings Status

#	Finding	R1/R2 Severity	R3 Status	Models
1	Fetch storm — FetchPrUrlAsync on every render	🔴 CRITICAL	✅ Fixed	3/3
2	Invalidate() defeats 5-min cache TTL	🟡 MODERATE	✅ Fixed	3/3
3	Cross-component blast radius from singleton	🟡 MODERATE	✅ Fixed	3/3

New Findings (R3)

#	Severity	Finding	Models	Status
4	🟡 MODERATE	Stale _prUrl shown for wrong session on same-dir switch	Sonnet 🔴, Codex, Opus 🟢 (3/3)	✅ Fixed in 2nd commit
5	🟢 MINOR	Out-of-order async race in FetchPrUrlAsync	Sonnet, Codex (2/3)	✅ Fixed in 2nd commit

Fix Details

Commit 1 (71ca4e2b6): Activation guard + remove Invalidate()

• ExpandedSessionView: Added _lastExpandedSessionId to track session identity; sessionSwitched && _prUrl == null gates re-fetch to session transitions only
• SessionListItem: Added becameActive edge detection (IsActive && !_wasActive); removed PrLinkService.Invalidate() call
• Both components now respect the 5-minute cache TTL

Commit 2 (1e14508ee): Stale state + async race fix

• Clear _prUrl/_prLabel when sessionSwitched is true, preventing Session A's PR badge from persisting to Session B (same-dir case)
• Capture Session.SessionId before await in FetchPrUrlAsync; discard stale results if session changed during the async operation

Test Results

✅ All 3111 tests pass

Verdict: ✅ Approve — All original findings fixed. R3 findings addressed in 2nd commit. Clean, minimal changes with correct edge-trigger patterns matching existing codebase conventions.

[PureWeen]

🔍 PR #469 — R4 Performance-Focused Multi-Model Review

Models: Claude Opus 4.6, Claude Sonnet 4.6, GPT-5.3 Codex | CI: No checks configured | Focus: Performance

Previous Findings (R1–R3)

All 5 findings from R1–R3 remain ✅ Fixed — unanimous 3/3.

Performance Analysis (3/3 Consensus on All Points)

Concern	Verdict	Detail
Hot-path cost (20 renders/sec)	✅ O(1), zero allocs	~3 boolean/string checks per render, all short-circuit. ~60ns/sec added.
Fetch storm prevention	✅ Impossible by construction	sessionSwitched is true for exactly 1 render per switch. becameActive is edge-triggered. During streaming: all guards evaluate false.
Render loop risk	✅ Clean dead-end	FetchPrUrlAsync → StateHasChanged → OnParametersSet → all guards false → no re-fetch.
N-instance scaling (100+ sessions)	✅ Negligible	Per-instance cost: 2 bool reads + 1 write. At 100×20Hz = 2,000 calls/sec → ~500μs total.
GUID string comparison	✅ Immaterial	Same reference on repeated renders → pointer equality short-circuit. Worst case: 36-char compare at 20Hz = ~2μs/sec.
Memory / subprocess lifetime	✅ Bounded	At most 1-2 tasks outstanding (only on session switch). gh pr view has 10s timeout + proper cleanup in finally.
Cache coherence	✅ Respected	Invalidate() fully removed. 5-min TTL cache consulted on all paths.

Pre-Existing Observation (Not a Regression)

🟢 MINOR: Double subprocess on cold cache (3/3 flagged as pre-existing)

• ExpandedSessionView and SessionListItem can both fire FetchPrUrlAsync for the same directory simultaneously on session switch
• Both miss cache → 2 gh pr view subprocesses instead of 1
• Bounded to exactly 2, once per session switch, once per 5-min TTL window
• Not introduced by this PR — the old code had the same race. A Lazy<Task> per-key in PrLinkService would eliminate it, but the complexity isn't warranted for a once-per-click event.

Test Results

✅ All 3111 tests pass

Verdict: ✅ Approve — Performance Clean

The PR is well-structured from a performance perspective. The hot path (streaming at 20Hz) adds zero meaningful overhead. Fetch storms are structurally impossible. All guards are one-shot edge-triggered. The 5-min cache is fully respected. No regressions, no render loops, no memory concerns.