Replace group-based sidebar with flat session list + worktree items (#818)#819
Conversation
🧪 Integration Test Report — PR #819
❌ Integration test failures |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 6 findings identified (6 moderate). 4 posted as inline comments; 2 additional findings are outside the diff and included in the summary comment below.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
Generated by Expert Code Review · ● 48.6M
| public IReadOnlyList<AgentSessionInfo> GetFlatSessions() | ||
| { | ||
| var metas = Organization.Sessions.ToDictionary(m => m.SessionName); | ||
| var allSessions = GetAllSessions().ToList(); |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 reviewers · Codespace sessions appear twice in sidebar
GetAllSessions() returns all sessions including those in codespace groups. RebuildRenderedGroups() puts these into _flatSessionList AND separately into _codespaceGroups. The template then renders codespace sessions twice: once in the flat "Sessions" list and once in the "Codespaces" section below.
Suggestion: Either filter codespace-group sessions out of the flat list, or stop rendering them in the separate codespace section. For example, in RebuildRenderedGroups:
var codespaceGroupIds = _codespaceGroups.Select(c => c.Group.Id).ToHashSet();
_flatSessionList = ApplyStatusFilter(flat
.Where(s => {
var m = CopilotService.GetSessionMeta(s.Name);
return m == null || !codespaceGroupIds.Contains(m.GroupId);
}).ToList());| return allSessions | ||
| .OrderByDescending(s => metas.TryGetValue(s.Name, out var m) && m.IsPinned) | ||
| .ThenBy(s => UrgencyScore(s)) | ||
| .ThenBy(s => DateTimeOffset.MaxValue - s.LastUpdatedAt) |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers · GetFlatSessions() ignores Organization.SortMode
This line hardcodes recency sort (LastUpdatedAt), but the sidebar sort dropdown (A–Z, Created, Manual) is still rendered and calls ApplySort(). With this change, the sort dropdown becomes a no-op — the flat list always sorts by last-active regardless of the user's selection.
GetOrganizedSessions() uses ApplySort(s, metas) which respects SortMode. This method should do the same.
Suggestion: Replace .ThenBy(s => DateTimeOffset.MaxValue - s.LastUpdatedAt) with .ThenBy(s => ApplySort(s, metas)), or disable/remove the sort controls if recency-only is intentional.
| foreach (var meta in Organization.Sessions) | ||
| { | ||
| if (!string.IsNullOrEmpty(meta.WorktreeId)) | ||
| activeWorktreeIds.Add(meta.WorktreeId); |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 reviewers · GetUnoccupiedWorktrees() misses group-level WorktreeId
This only checks SessionMeta.WorktreeId, but worktrees can also be claimed at the group level via SessionGroup.WorktreeId and SessionGroup.CreatedWorktreeIds (used by multi-agent groups before individual session metas are reconciled). A worktree owned by a multi-agent group could appear as "unoccupied", leading to a duplicate session in an already-claimed worktree.
Suggestion: Also collect group-level worktree IDs:
foreach (var group in Organization.Groups)
{
if (!string.IsNullOrEmpty(group.WorktreeId))
activeWorktreeIds.Add(group.WorktreeId);
if (group.CreatedWorktreeIds != null)
foreach (var id in group.CreatedWorktreeIds)
activeWorktreeIds.Add(id);
}| { | ||
| @if (!showGroupHeaders) | ||
| <div class="section-header"><span class="section-header-label">Sessions</span></div> | ||
| @foreach (var session in flatSessions) |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 reviewers (after follow-up) · Multi-agent orchestration controls lost
The flat list renders all sessions identically via SessionListItem. The old group-based rendering had dedicated multi-agent UI: orchestrator headers, worker tree visibility, dispatch/reflect controls, phase badges, and the mode picker. These are all gone in the flat layout. _groupPhases is still tracked (line ~1594) but never rendered.
If multi-agent teams remain a supported feature, orchestration controls need to be surfaced somewhere (e.g., inline on orchestrator sessions, or a separate section). If this is an intentional simplification for #818, consider documenting it as a known regression and fully removing the dead _groupPhases tracking code.
Cross-Platform Verification — PR #819Build Results
✅ All platforms verified Previous Review HistoryFound 0 Triggered by: verify-build run |
🧪 Integration Test Report — PR #819
✅ All platforms passed |
Review-Fix Loop — Round 1 of 3Findings addressed: 9 of 10
Tests✅ All 3661 tests pass Next stepsExpert review round 2 and verify-build dispatched. Warning
|
Cross-Platform Verification — PR #819Build Results
✅ All platforms verified Previous Review HistoryFound 0 Triggered by: verify-build run |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review — Round 2: 5 findings (3 moderate, 2 moderate previously reported)
Three independent reviewers analyzed this PR's interaction with the full codebase. A round 1 review already identified 4 inline findings (SortMode, codespace double-render, group-level worktrees, multi-agent UI loss). This round surfaces 3 additional findings not covered in round 1.
New findings (this round)
| # | Severity | Consensus | File | Line | Finding |
|---|---|---|---|---|---|
| 1 | 🟡 MODERATE | 3/3 | Dashboard.razor |
385 | GetFlatSessions() called directly in render loop — uncached, allocates dict+list+sort every StateHasChanged() |
| 2 | 🟡 MODERATE | 3/3 | SessionSidebar.razor |
2106 | TOCTOU race in CreateSessionForWorktree — worktree occupancy not re-checked between render and click |
| 3 | 🟡 MODERATE | 3/3 | Dashboard.razor |
3405 | JsSwitchToSessionByIndex still uses GetOrganizedSessions() but grid renders flat — ⌘+1..9 activate wrong session (outside diff — posted in summary comment) |
Round 1 findings confirmed (4 existing inline comments)
| Severity | Line | Finding | Status |
|---|---|---|---|
| 🟡 MODERATE | Organization.cs:1381 |
GetFlatSessions() ignores SortMode — sort dropdown is a no-op |
Addressed in PR #821 |
| 🟡 MODERATE | Organization.cs:1361 |
Codespace sessions double-rendered (flat list + codespace section) | Addressed in PR #821 |
| 🟡 MODERATE | Organization.cs:1395 |
GetUnoccupiedWorktrees() misses group-level WorktreeId/CreatedWorktreeIds |
Addressed in PR #821 |
| 🟡 MODERATE | SessionSidebar.razor:877 |
Multi-agent orchestration controls fully removed | Intentional per #818 |
What's working well
- ✅
IsOrchestratorWorkersync correctly replicated inGetFlatSessions()—NeedsAttentionsuppression works for workers - ✅
CreateSessionForWorktree()follows the establishedisCreatingguard + try/catch/finally pattern - ✅ Thread safety:
GetFlatSessions()only called from UI-thread contexts (render,RebuildRenderedGroups) - ✅ Status filter properly applied to flat list in sidebar via
ApplyStatusFilter()
CI / Test Status
- PR #821 reports all 3661 tests pass after addressing round 1 findings
- The new
GetFlatSessions()andGetUnoccupiedWorktrees()have no unit test coverage yet
Discarded findings (1/3 consensus — noted in summary comment)
- Dashboard orphan
{literal at line 268 - O(N×G) group lookup performance in
GetFlatSessions() - Dead
GroupRenderModel/BuildGroupRenderModelcode
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
api.nuget.orgdc.services.visualstudio.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"See Network Configuration for more information.
Generated by Expert Code Review · ● 101.1M
| createError = null; | ||
| try | ||
| { | ||
| var wt = RepoManager.Worktrees.FirstOrDefault(w => w.Id == worktreeId); |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers · TOCTOU race: worktree occupancy not re-checked at click time
_unoccupiedWorktrees is computed at RebuildRenderedGroups() time (render). When the user clicks a worktree item, CreateSessionForWorktree looks up the worktree from RepoManager.Worktrees — the worktree still exists physically, so wt != null passes. But between render and click, another code path (e.g., QuickCreateSessionForRepo, a remote bridge sync, or another client) could have already created a session for this worktree.
CreateSessionWithWorktreeAsync will then create a second session bound to the same worktree, potentially causing conflicting git operations (two sessions writing to the same working directory).
Additionally, GetUnoccupiedWorktrees() checks Organization.Sessions (the reconciled, disk-synced list), but newly created sessions live in _sessions first — ReconcileOrganization() may not have run yet. So even at render time, recently occupied worktrees can appear as "unoccupied."
Suggestion: Re-check occupancy before proceeding:
private async Task CreateSessionForWorktree(string worktreeId)
{
if (isCreating) return;
isCreating = true;
createError = null;
try
{
// Re-check occupancy at click time
var currentUnoccupied = CopilotService.GetUnoccupiedWorktrees();
if (!currentUnoccupied.Any(w => w.Id == worktreeId))
throw new InvalidOperationException("Worktree is already in use");
var wt = RepoManager.Worktrees.FirstOrDefault(w => w.Id == worktreeId);
// ...And in GetUnoccupiedWorktrees(), also check live _sessions for WorktreeId to catch not-yet-reconciled sessions.
| @foreach (var session in visibleSessions) | ||
| { | ||
| <div class="session-grid" style="grid-template-columns: repeat(@_gridColumns, 1fr)"> | ||
| @foreach (var session in CopilotService.GetFlatSessions()) |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers · GetFlatSessions() called directly in render loop without caching
GetFlatSessions() has no cache — every call allocates a new Dictionary<string, SessionMeta> (from Organization.Sessions.ToDictionary()), a new List<AgentSessionInfo> (from GetAllSessions().ToList()), does an O(N×G) Organization.Groups.FirstOrDefault() scan per session for IsOrchestratorWorker sync, and then a three-key LINQ sort. This is called inline in the Blazor render tree, meaning it runs on every StateHasChanged() cycle.
StateHasChanged() has 15+ call sites including streaming deltas, tool events, and the 5-second heartbeat. With 30+ sessions, this creates significant repeated allocation pressure.
By contrast, the sidebar correctly pre-computes once in RebuildRenderedGroups() and stores the result in _flatSessionList. The old GetOrganizedSessions() had a HashCode.Combine cache that short-circuited when nothing changed.
Suggestion: Pre-compute the flat session list in SafeRefreshAsync and store it in a field (matching the sidebar pattern), or add a cache to GetFlatSessions() using the same _organizedSessionsCacheKey approach:
// In Dashboard.razor:
private List<AgentSessionInfo> _dashboardFlatSessions = new();
// In SafeRefreshAsync or equivalent:
_dashboardFlatSessions = CopilotService.GetFlatSessions().ToList();
// In template:
`@foreach` (var session in _dashboardFlatSessions)|
Commit pushed:
|
Review-Fix Loop — Round 2 of 3Findings addressed: 3 of 3
Tests✅ All 3661 tests pass Next stepsExpert review round 3 and verify-build dispatched. Warning
|
Cross-Platform Verification — PR #819Build Results
✅ All platforms verified Previous Review HistoryFound 6 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Expert Code Review: 3 findings identified (1 moderate, 2 minor). 2 posted as inline comments; 1 outside-diff finding included in the summary comment below.
Generated by Expert Code Review · ● 84.9M
| } | ||
|
|
||
| @if (sessions.Any() || CopilotService.HasMultipleGroups) | ||
| else |
There was a problem hiding this comment.
🟢 MINOR · 2/2 reviewers · Empty "Sessions" header when only codespace sessions exist
When a user has only codespace sessions: sessions.Any() is true (codespace sessions are in GetAllSessions()), but flatSessions is empty (codespace sessions are filtered out by RebuildRenderedGroups). With _statusFilter == All, this else branch renders the "Sessions" header with an empty @foreach body — a bare label with nothing under it.
Suggestion: Guard this branch:
else if (flatSessions.Any())
{
<div class="section-header">...Sessions...</div>
`@foreach` (var session in flatSessions) { ... }
}| } | ||
| catch (Exception ex) | ||
| { | ||
| createError = ex.Message; |
There was a problem hiding this comment.
🟢 MINOR · 2/2 reviewers · createError invisible on worktree creation failure
createError is set here on failure, but it's only rendered inside CreateSessionForm components (visible when showCreateSessionForm is true). Since CreateSessionForWorktree doesn't set that flag, the error is invisible to the user — only logged to Console.WriteLine. The user sees the spinner disappear with no explanation.
Suggestion: Add an error banner near the worktree section, or use the existing quickCreateError/quickCreateErrorRepoId pattern. Note: this follows the same pattern as QuickCreateSessionForFolder et al., so it's a pre-existing convention rather than a new regression.
|
Commit pushed:
|
Review-Fix Loop — Round 3 of 3Findings addressed: 3 of 3
Fixes applied
Test results✅ 3660/3661 passed (1 flaky Review dispatchWarning
|
Implements #818: - Replace group headers/collapse/expand with flat recency-sorted session list - Add unoccupied worktree items below sessions - Remove Group and Multi-Agent Team from create popover - Remove Move to submenu from session context menu - Add GetFlatSessions() and GetUnoccupiedWorktrees() to CopilotService - Keep codespace groups, persisted sessions, and external sessions intact Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ard, keyboard shortcut alignment - Cache GetFlatSessions() in _flatSessions field populated in RefreshState(), replacing uncached call in render loop (Dashboard.razor:385) - Add TOCTOU re-check in CreateSessionForWorktree to verify worktree occupancy at click time (SessionSidebar.razor:2106) - Update JsSwitchToSessionByIndex to iterate _flatSessions instead of GetOrganizedSessions(), aligning keyboard shortcuts with visible grid ordering (Dashboard.razor:3405) Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d, worktree error display - Fix JsCycleExpandedSession to use _flatSessions (sorted) instead of sessions (unsorted) so tab-cycling matches visible grid order - Guard empty Sessions header when only codespace sessions exist - Surface worktree creation errors in the sidebar worktree section Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bc502e2 to
4dfb324
Compare
Cross-Platform Verification — PR #819Build Results
✅ All platforms verified Previous Review HistoryFound 6 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
🧪 Integration Test Report — PR #819
✅ All platforms passed |
This comment has been minimized.
This comment has been minimized.
Expert Code Review — PR #819Methodology: 3 independent reviewers with adversarial consensus (2 follow-up rounds for disputed findings) 5 findings posted: 3 moderate, 2 minor — 3 as inline comments, 2 outside-diff findings below. Outside-diff findings
Discarded findings (1/3 consensus — single reviewer only)
CI Status
Test Coverage
|
There was a problem hiding this comment.
Review Summary
Reviewed the full diff and traced callers/callees across SessionSidebar.razor, SessionListItem.razor, Dashboard.razor, and CopilotService.Organization.cs. Findings exclude items already reported in previous reviews.
1 actionable finding:
| # | Severity | Finding | Consensus |
|---|---|---|---|
| 1 | 🟡 MODERATE | "Reset Conversation" button silently broken — OnClearHistory no longer wired after ClearSessionHistory method was removed |
Verified in source |
| 2 | 🟢 MINOR | Dead code: GroupRenderModel + BuildGroupRenderModel in sidebar |
— |
| 3 | 🟢 MINOR | Dead code in Dashboard: group creation methods/state now unreachable | — |
| 4 | 🟢 MINOR | Stray bare { } block in Dashboard toolbar after conditional removal |
— |
The moderate finding (#1) is a functional regression — a menu item renders but clicking it does nothing. The minor findings are dead code that should be cleaned up but have no runtime impact.
Generated by Expert Code Review · ● 41.6M
| <SessionListItem Session="session" | ||
| Meta="meta" | ||
| CompactMode="IsDesktopRailMode" | ||
| IsActive="@(session.Name == CopilotService.ActiveSessionName)" | ||
| IsCompleted="@completedSessions.Contains(session.Name)" | ||
| IsMultiAgentMember="isMultiAgentMember" | ||
| IsRenaming="@(renamingSession == session.Name)" | ||
| IsMenuOpen="@(openMenuSession == session.Name)" | ||
| ShortcutIndex="idx" | ||
| Groups="CopilotService.Organization.Groups" | ||
| UsageInfo="@(usageBySession.TryGetValue(session.Name, out var usg) ? usg : null)" | ||
| OnSelect="() => SelectSession(sName)" | ||
| OnClose="() => CloseSession(sName)" | ||
| OnCloseWithOptions="(opts) => CloseSessionWithOptions(sName, opts.DeleteWorktree, opts.DeleteBranch)" | ||
| OnPin="(pinned) => { CopilotService.PinSession(sName, pinned); }" | ||
| OnMove="(groupId) => CopilotService.MoveSession(sName, groupId)" | ||
| OnStartRename="() => StartRename(sName)" | ||
| OnCommitRename="CommitRename" | ||
| OnToggleMenu="() => ToggleSessionMenu(sName)" | ||
| OnCloseMenu="() => { openMenuSession = null; }" | ||
| OnReportBug="() => OpenBugReportForSession(sName)" | ||
| OnFixWithCopilot="() => OpenFixItForSession(sName)" | ||
| OnAnalyze="() => AnalyzeSessionEfficiency(sName)" | ||
| OnContinueInNewSession="() => ContinueInNewSession(sName)" /> |
There was a problem hiding this comment.
🟡 MODERATE — "Reset Conversation" silently broken
OnClearHistory is not wired here (nor in the codespace <SessionListItem> at line 1136). SessionListItem still renders the "🧹 Reset Conversation" button (line 269) which invokes OnClearHistory, but since no handler is passed, EventCallback.Empty makes it a silent no-op.
The ClearSessionHistory method that previously provided the handler was removed at the bottom of this diff.
Fix: Either re-add the handler and wire OnClearHistory="() => ClearSessionHistory(sName)", or remove the button from SessionListItem.razor if the feature is intentionally deprecated.
| @onclick:stopPropagation="true" title="More options">⋯</button> | ||
| <div class="toolbar-controls @(toolbarMenuOpen ? "open" : "")"> | ||
| @if (isAddingDashGroup) | ||
| { |
There was a problem hiding this comment.
🟢 MINOR · 3/3 reviewers
Stray { renders as literal text in toolbar. The @if (isAddingDashGroup) block was removed but its opening { was left behind. In Razor, a bare { without a preceding @ keyword is emitted as literal HTML — users see a visible { character in the Dashboard toolbar between the ⋯ button and the Grid controls.
Fix: Delete this line.
| var sName = session.Name; | ||
| var sessionGroup = meta != null ? CopilotService.Organization.Groups.FirstOrDefault(g => g.Id == meta.GroupId) : null; | ||
| var isMultiAgentMember = sessionGroup is { IsMultiAgent: true }; | ||
| <SessionListItem Session="session" |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 reviewers
"Reset Conversation" silently broken. The new <SessionListItem> in the flat list no longer binds OnClearHistory. The old group-based rendering passed OnClearHistory="() => ClearSessionHistory(sName)", and the ClearSessionHistory method was removed entirely (line 814 in old file). The menu item still renders in SessionListItem.razor but the default EventCallback no-ops — clicking "🧹 Reset Conversation" does nothing.
Scenario: User right-clicks a session → clicks "Reset Conversation" → nothing happens, no error shown.
Fix: Either re-add ClearSessionHistory and wire OnClearHistory on both flat-list and codespace SessionListItem instances, or hide the menu item in SessionListItem.razor when the callback is unbound.
| } | ||
| catch (Exception ex) | ||
| { | ||
| createError = ex.Message; |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers (after follow-up)
Worktree error contaminates session creation form. The catch block sets both createError and _worktreeCreateError. Since createError is bound to CreateSessionForm via CreateError="@createerror" (lines 205, 624), a worktree creation failure leaks into the unrelated session creation form.
Scenario: User clicks a worktree item → creation fails → user later opens the + session creation form → sees stale "Worktree is already in use" error from the prior worktree click.
Fix: Remove createError = ex.Message; from this catch block. _worktreeCreateError is the correct error channel for the worktree UI.
|
Implements #818: Replace the group-based sidebar with a flat, recency-sorted session list and unoccupied worktree items.
Changes
SessionSidebar.razor
_regularGroupModels/_showGroupHeadersgroup-based rendering with_flatSessionListbacked byCopilotService.GetFlatSessions()— sessions are rendered in a single flat list sorted by recencyCopilotService.GetUnoccupiedWorktrees(). Clicking creates a new session for that worktreeCreateSessionForWorktreemethod for the worktree click handlerSessionListItem.razor (pre-existing change)
CopilotService.Organization.cs (pre-existing change)
GetFlatSessions()— returns all sessions sorted by recency (pinned first, then by last activity)GetUnoccupiedWorktrees()— returns worktrees not currently occupied by any active sessionDashboard.razor (pre-existing change)
Testing
_showGroupHeaders,_regularGroupModels)Closes #818
Warning
The following domain was blocked by the firewall during workflow execution:
192.0.2.1To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.