Fix: Session grouping — add Move to Group menu item#125
Fix: Session grouping — add Move to Group menu item#125davidortinau wants to merge 1 commit intomainfrom
Conversation
The SessionListItem context menu had Groups and OnMove parameters wired up but never rendered any UI to move sessions between groups. This adds: - A 'Move to…' submenu in the session context menu that lists all available groups (excluding the session's current group). Only shown when multiple groups exist. - Defensive auto-creation of SessionMeta in MoveSession() when the session hasn't been reconciled yet, preventing silent no-ops. - Submenu CSS styling (indented items, non-interactive label). - 6 new unit tests covering MoveSession behavior: existing meta update, auto-create meta, invalid group rejection, cross-group moves, move back to default, and StateChanged event firing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review this PR |
|
@davidortinau I've opened a new pull request, #126, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR adds the missing UI for moving sessions between groups, fixing a bug where users could create groups but had no way to move sessions into them.
Changes:
- Added "Move to..." submenu in the session context menu with group options
- Made
MoveSession()defensive by auto-creatingSessionMetaentries for sessions that haven't been reconciled yet - Added CSS styling for submenu display with proper indentation and hover states
- Added 6 comprehensive unit tests covering various move scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| SessionListItem.razor | Added "Move to..." submenu UI with group filtering and click handlers |
| SessionListItem.razor.css | Added CSS for .menu-submenu, .submenu-label, and .submenu-item |
| CopilotService.Organization.cs | Refactored MoveSession() to validate group first, auto-create missing SessionMeta, and always save/notify |
| SessionOrganizationTests.cs | Added MoveSessionTests class with 6 test cases covering edge cases |
| var meta = Organization.Sessions.FirstOrDefault(m => m.SessionName == sessionName); | ||
| if (meta != null && Organization.Groups.Any(g => g.Id == groupId)) | ||
| if (meta == null) | ||
| { | ||
| // Session exists but wasn't reconciled yet — create meta on the fly | ||
| meta = new SessionMeta { SessionName = sessionName, GroupId = groupId }; | ||
| Organization.Sessions.Add(meta); |
There was a problem hiding this comment.
There's a potential race condition here. Organization.Sessions is a List<SessionMeta> which is not thread-safe. This method can be called from both the UI thread (via Blazor event handlers in SessionSidebar.razor) and background threads (via WsBridgeServer.HandleOrganizationCommand which runs in the WebSocket receive loop).
The Add operation on line 178 could collide with concurrent reads/writes from other threads. Consider adding synchronization (e.g., lock statement) around the list access, or using a thread-safe collection. This same issue exists in other organization methods that modify Organization.Sessions and Organization.Groups.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@davidortinau I've opened a new pull request, #127, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Hey! 🎉 Fantastic job spotting this issue and getting a fix out the door. The "Move to Group" context menu was exactly the missing piece — groups could be created but sessions were stranded with no way to move them. Your instinct to make MoveSession() defensive and auto-create missing SessionMeta was spot-on engineering. Since this PR was opened, all of these improvements have landed in main through subsequent work. After rebasing on the latest origin/main, this branch produces a zero diff — every change here is already part of the codebase. Do, or do not — there is no diff. 🧘 This is being closed purely because the work is done, not because it wasn't valuable. Your contributions helped shape the direction here, and we'd love to see more. The Force of your PRs is strong. Keep them coming! 🚀 |
PureWeen
deleted the
fix/grouping-seems-broken-i-can-create-a-gro-20260216-2018
branch
Bug
Users can create groups but cannot move sessions into them.
Root Cause
SessionListItem.razor receives Groups and OnMove parameters but never renders a UI element to invoke the move.
Fix
Testing