fix(group-chat): add participant remove button and allow agentId updates#718
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded participant removal UI and callbacks: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ParticipantCard
participant GroupChatPanel
participant MaestroAPI as window.maestro.groupChat
User->>ParticipantCard: Click "Remove"
ParticipantCard->>ParticipantCard: Show confirm/cancel
User->>ParticipantCard: Click "Confirm"
ParticipantCard->>ParticipantCard: set isRemoving = true
ParticipantCard->>GroupChatPanel: onRemove(participantName)
GroupChatPanel->>MaestroAPI: removeParticipant(groupChatId, participantName)
MaestroAPI-->>GroupChatPanel: Success/Failure
GroupChatPanel-->>ParticipantCard: resolve/reject
ParticipantCard->>ParticipantCard: clear isRemoving & confirmRemove
ParticipantCard->>User: Update UI (removed or reset)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR wires up a missing "Remove participant" UI for group chats — a two-step confirmation button on
Confidence Score: 4/5Safe to merge after fixing silent error swallowing in both remove-participant handlers. One P1 finding: errors from the IPC call are swallowed before reaching ParticipantCard, so failures are invisible to both the user and Sentry. The rest of the change — type extension, confirmation UI, moderator exclusion — is correct and low-risk. src/renderer/components/GroupChatParticipants.tsx and src/renderer/components/GroupChatRightPanel.tsx — identical handleRemoveParticipant handlers both need error re-throwing. Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant PC as ParticipantCard
participant GCP as GroupChatParticipants / GroupChatRightPanel
participant IPC as window.maestro.groupChat
participant BE as group-chat-storage (main)
User->>PC: Click "Remove"
PC->>PC: setConfirmRemove(true)
User->>PC: Click "Confirm"
PC->>PC: setIsRemoving(true)
PC->>GCP: onRemove(participantName)
GCP->>IPC: removeParticipant(groupChatId, participantName)
IPC->>BE: removeParticipant()
alt Success
BE-->>IPC: resolved
IPC-->>GCP: resolved
GCP-->>PC: resolved (onRemove returns)
PC->>PC: finally: setIsRemoving(false), setConfirmRemove(false)
Note over PC: Component unmounts (participant removed from state)
else Failure
BE-->>IPC: throws
IPC-->>GCP: throws
GCP->>GCP: catch: console.error only
GCP-->>PC: resolved (error swallowed!)
PC->>PC: finally: setIsRemoving(false), setConfirmRemove(false)
Note over PC: Remove button reappears — no user feedback
end
Reviews (1): Last reviewed commit: "fix(group-chat): add participant remove ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const handleRemoveParticipant = useCallback( | ||
| async (participantName: string) => { | ||
| try { | ||
| await window.maestro.groupChat.removeParticipant(groupChatId, participantName); | ||
| } catch (error) { | ||
| console.error(`Failed to remove participant ${participantName}:`, error); | ||
| } | ||
| }, | ||
| [groupChatId] |
There was a problem hiding this comment.
handleRemoveParticipant catches and swallows all errors, so if the IPC call fails the onRemove callback never rejects. ParticipantCard's handleRemove then completes normally, resets isRemoving and confirmRemove, and the Remove button reappears — but the user receives no indication that the operation failed. This also prevents Sentry from capturing unexpected backend failures (CLAUDE.md explicitly flags this pattern as "WRONG"). The same issue exists in GroupChatRightPanel.tsx at the equivalent handler.
The fix is to let unexpected errors re-throw (Sentry captures them automatically) and have ParticipantCard surface a failure state — or at minimum have onRemove not catch the error so the finally in handleRemove can be paired with a caught error that shows a toast/message.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/components/GroupChatRightPanel.tsx (1)
192-202: Consider extracting shared participant action handlers.Both
GroupChatRightPanel.tsxandGroupChatParticipants.tsxnow contain identicalhandleRemoveParticipantandhandleContextResetimplementations. These could be extracted into a shared hook (e.g.,useParticipantActions(groupChatId)) to reduce duplication.This is existing technical debt that predates this PR, so deferring is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/GroupChatRightPanel.tsx` around lines 192 - 202, Both components duplicate participant action handlers; extract them into a shared hook named useParticipantActions(groupChatId) that returns handleRemoveParticipant and handleContextReset so the logic is centralized. Move the try/catch logic that calls window.maestro.groupChat.removeParticipant(groupChatId, participantName) into the hook and implement handleContextReset there as well; then replace the inline handleRemoveParticipant and handleContextReset in GroupChatRightPanel and GroupChatParticipants with the hook's returned functions to remove duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/GroupChatRightPanel.tsx`:
- Around line 192-202: Both components duplicate participant action handlers;
extract them into a shared hook named useParticipantActions(groupChatId) that
returns handleRemoveParticipant and handleContextReset so the logic is
centralized. Move the try/catch logic that calls
window.maestro.groupChat.removeParticipant(groupChatId, participantName) into
the hook and implement handleContextReset there as well; then replace the inline
handleRemoveParticipant and handleContextReset in GroupChatRightPanel and
GroupChatParticipants with the hook's returned functions to remove duplication
and keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffd54000-f4b2-4cd4-8c29-098614155044
📒 Files selected for processing (4)
src/main/group-chat/group-chat-storage.tssrc/renderer/components/GroupChatParticipants.tsxsrc/renderer/components/GroupChatRightPanel.tsxsrc/renderer/components/ParticipantCard.tsx
|
Thanks for the contribution, @pedramamini! Clean fix — the two-step confirm UX on the Remove button is a nice touch, and wiring up the already-implemented backend was the right call. One thing to address before merge: Greptile flagged (and I agree) that the try {
await window.maestro.groupChat.removeParticipant(groupChatId, participantName);
} catch (error) {
console.error(`Failed to remove participant ${participantName}:`, error);
}Per our error handling guidelines in CLAUDE.md, unexpected errors should bubble up so Sentry can capture them. The user also gets zero feedback that the remove failed — the button just reappears silently. Please either:
Everything else looks good. The |
…tes (#717) When a user switches an agent's provider after adding it to a group chat, the participant becomes stale (old agentId) and cannot be removed because no remove UI existed despite the backend API being fully implemented. - Add Remove button with confirmation to ParticipantCard component - Wire up onRemove callback in GroupChatRightPanel and GroupChatParticipants - Add agentId to ParticipantUpdate type so stale participants can be updated Closes #717
…d from ParticipantUpdate - Remove try/catch in handleRemoveParticipant callbacks so failures reach Sentry instead of being silently swallowed via console.error - Remove unused 'agentId' from ParticipantUpdate type (no code path uses it to update stale agent IDs)
pedramamini
force-pushed
the
fix/717-group-chat-stale-provider-no-remove
branch
from
b1c325c to
b5e8d9a
Compare
1 participant
Summary
ParticipantCardso users can remove stale participants from group chatsonRemovecallback throughGroupChatRightPanelandGroupChatParticipantsto the existingremoveParticipantIPC handler (backend was already implemented, UI was missing)agentIdtoParticipantUpdatetype enabling programmatic updates to a participant's agent type when provider changesRoot Cause
When a user switches an agent's provider (e.g., Codex → Claude) after adding it to a group chat, the participant's
agentIdis snapshotted at creation time inmetadata.jsonand never updated. The stale reference causes the right panel to display the old provider, and since there was no UI to remove participants, users were stuck with no way to fix the group chat.Workaround (pre-fix)
Create a new group chat — it picks up the updated provider automatically.
Closes #717
Test plan
Summary by CodeRabbit