fix: worktree sessions showing Ungrouped toast when parent is grouped#740
fix: worktree sessions showing Ungrouped toast when parent is grouped#740pedramamini merged 2 commits intoRunMaestro:rcfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughNormalize falsy Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / UI"
participant Component as "QuickActionsModal / SessionList"
participant Store as "Session Store"
participant Groups as "Groups Store"
participant Notifier as "Toast / History"
User->>Component: initiate move (groupId may be falsy)
Component->>Store: request sessions & normalize groupId (undefined if falsy)
Component->>Store: setSessions(updatedSessions with normalized groupId)
Store-->>Store: update target session and sessions where parentSessionId == target.id
Store->>Groups: request group by effectiveGroupId (session.groupId or parent's groupId)
Groups-->>Notifier: provide group name (or 'Ungrouped')
Component->>Notifier: emit completion/toast with resolved group name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/session/useGroupManagement.ts (1)
129-143:⚠️ Potential issue | 🔴 CriticalTwo handlers lack worktree child sync in move-to-group paths — inconsistent group assignment possible.
Lines 133-138 correctly sync children for drag/drop operations. However,
src/renderer/hooks/session/useSessionCrud.ts:468andsrc/renderer/components/SessionList/SessionList.tsx:334both update only the parent session without syncing direct worktree children. This creates inconsistent behavior: dragging a parent to a group syncs children, but moving via the context menu or group creation flows does not.Required fixes for worktree child parity
In
src/renderer/hooks/session/useSessionCrud.ts:468:setSessions((prev) => - prev.map((s) => (s.id === pendingMoveToGroupSessionId ? { ...s, groupId } : s)) + prev.map((s) => { + if (s.id === pendingMoveToGroupSessionId) return { ...s, groupId }; + if (s.parentSessionId === pendingMoveToGroupSessionId) return { ...s, groupId }; + return s; + }) );In
src/renderer/components/SessionList/SessionList.tsx:334:setSessions((prev) => - prev.map((s) => (s.id === sessionId ? { ...s, groupId: groupId || undefined } : s)) + prev.map((s) => { + if (s.id === sessionId) return { ...s, groupId: groupId || undefined }; + if (s.parentSessionId === sessionId) return { ...s, groupId: groupId || undefined }; + return s; + }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useGroupManagement.ts` around lines 129 - 143, The drag/drop handler handleDropOnGroup updates both the moved session and its direct worktree children; replicate that parity in the other move-to-group code paths: in the session update routine inside useSessionCrud (the function that assigns a new groupId to a session) and in the context-menu/group-move handler in SessionList.tsx, when setting a session's groupId also iterate the sessions state and update any session whose parentSessionId === movedSessionId to the same groupId (use the same setSessions(prev => prev.map(...)) pattern used in handleDropOnGroup and clear any dragging state as appropriate).
🧹 Nitpick comments (2)
src/renderer/hooks/batch/useBatchHandlers.ts (1)
216-225: Extract effective-group resolution into a local helper to prevent drift.The fallback logic is duplicated in two callbacks; centralizing it will keep toast attribution consistent.
Refactor sketch
+function resolveEffectiveGroupName( + sessionId: string, + sessions: { id: string; groupId?: string; parentSessionId?: string }[], + groups: { id: string; name: string }[] +): string { + const session = sessions.find((s) => s.id === sessionId); + const effectiveGroupId = + session?.groupId || + (session?.parentSessionId + ? sessions.find((s) => s.id === session.parentSessionId)?.groupId + : undefined); + return groups.find((g) => g.id === effectiveGroupId)?.name || 'Ungrouped'; +}Then use
resolveEffectiveGroupName(info.sessionId, currentSessions, currentGroups)in bothonCompleteandonPRResult.Also applies to: 517-524
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchHandlers.ts` around lines 216 - 225, Extract the duplicated fallback logic that computes effectiveGroupId/group name into a local helper function named e.g. resolveEffectiveGroupName(sessionId, currentSessions, currentGroups) that returns the resolved group (or null); move the block that looks up session via currentSessions, checks session.groupId or session.parentSessionId, and finds the group in currentGroups into that helper, then replace the inline logic in both onComplete and onPRResult callbacks to call resolveEffectiveGroupName(info.sessionId, currentSessions, currentGroups) so toast attribution is determined in one place and stays consistent.src/renderer/components/QuickActionsModal.tsx (1)
317-322: Normalize “no group” toundefinedbefore writing session state.This keeps grouping state canonical and avoids mixing
''andundefinedas ungrouped sentinels.Proposed cleanup
const handleMoveToGroup = (groupId: string) => { + const normalizedGroupId = groupId || undefined; const updatedSessions = sessions.map((s) => { - if (s.id === activeSessionId) return { ...s, groupId }; + if (s.id === activeSessionId) return { ...s, groupId: normalizedGroupId }; // Also update worktree children to keep groupId in sync - if (s.parentSessionId === activeSessionId) return { ...s, groupId }; + if (s.parentSessionId === activeSessionId) return { ...s, groupId: normalizedGroupId }; return s; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/QuickActionsModal.tsx` around lines 317 - 322, Normalize the "no group" sentinel by converting '' to undefined before writing session state: compute a normalizedGroupId (groupId === '' ? undefined : groupId) and use that in the sessions.map update (the block that returns { ...s, groupId }) so both the active session and any worktree children (checked via parentSessionId and activeSessionId) get undefined for ungrouped instead of ''. Update any other writes that set groupId in this component to use the normalized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/hooks/session/useGroupManagement.ts`:
- Around line 129-143: The drag/drop handler handleDropOnGroup updates both the
moved session and its direct worktree children; replicate that parity in the
other move-to-group code paths: in the session update routine inside
useSessionCrud (the function that assigns a new groupId to a session) and in the
context-menu/group-move handler in SessionList.tsx, when setting a session's
groupId also iterate the sessions state and update any session whose
parentSessionId === movedSessionId to the same groupId (use the same
setSessions(prev => prev.map(...)) pattern used in handleDropOnGroup and clear
any dragging state as appropriate).
---
Nitpick comments:
In `@src/renderer/components/QuickActionsModal.tsx`:
- Around line 317-322: Normalize the "no group" sentinel by converting '' to
undefined before writing session state: compute a normalizedGroupId (groupId ===
'' ? undefined : groupId) and use that in the sessions.map update (the block
that returns { ...s, groupId }) so both the active session and any worktree
children (checked via parentSessionId and activeSessionId) get undefined for
ungrouped instead of ''. Update any other writes that set groupId in this
component to use the normalized value.
In `@src/renderer/hooks/batch/useBatchHandlers.ts`:
- Around line 216-225: Extract the duplicated fallback logic that computes
effectiveGroupId/group name into a local helper function named e.g.
resolveEffectiveGroupName(sessionId, currentSessions, currentGroups) that
returns the resolved group (or null); move the block that looks up session via
currentSessions, checks session.groupId or session.parentSessionId, and finds
the group in currentGroups into that helper, then replace the inline logic in
both onComplete and onPRResult callbacks to call
resolveEffectiveGroupName(info.sessionId, currentSessions, currentGroups) so
toast attribution is determined in one place and stays consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6da63be3-a8b3-49de-930b-7b0f15ca5581
📒 Files selected for processing (5)
src/renderer/components/QuickActionsModal.tsxsrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/hooks/batch/useBatchHandlers.tssrc/renderer/hooks/session/useGroupManagement.tssrc/renderer/hooks/wizard/useWizardHandlers.ts
Greptile SummaryFixes worktree child sessions incorrectly showing "Ungrouped >" in toast notifications when their parent was moved to a group after the child was already created. The fix takes a two-pronged approach: proactively sync all children's Confidence Score: 5/5Safe to merge — fix is correct, two-pronged, and covers all toast notification call sites No P0 or P1 issues found. The proactive groupId sync on parent moves handles future reparenting correctly, and the reactive effectiveGroupId fallback in all five notification handlers covers sessions created before the parent was grouped. Verified that all notifyToast call sites with a group: property use the new pattern. The one minor inconsistency (Quick Actions 'No Group' sets groupId to '' while drag-and-drop sets it to undefined) is a pre-existing pattern that both resolve to falsy and behave identically. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User moves parent session to a group] --> B{Action type}
B -- Drag-and-drop --> C[handleDropOnGroup / handleDropOnUngrouped]
B -- Quick Actions --> D[handleMoveToGroup]
C --> E[Update parent groupId]
D --> E
E --> F[Also update all sessions where parentSessionId === movedSessionId]
F --> G[Children groupId now in sync]
H[Toast notification fires for a session] --> I{session.groupId set?}
I -- Yes --> J[Use session.groupId directly]
I -- No --> K{session.parentSessionId set?}
K -- Yes --> L[Look up parent session's groupId]
K -- No --> M[Show 'Ungrouped']
L --> N{Parent groupId found?}
N -- Yes --> O[Show parent's group name]
N -- No --> M
Reviews (1): Last reviewed commit: "fix: worktree sessions showing Ungrouped..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/hooks/batch/useBatchHandlers.ts (1)
38-55: Consider extractingresolveGroupNameto a shared utility.This same logic exists inline in
useAgentListeners.ts(lines 419-427). Extracting to a shared utility (e.g.,src/renderer/utils/sessionUtils.ts) would eliminate duplication and ensure consistency if the fallback logic ever needs to change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchHandlers.ts` around lines 38 - 55, Extract the duplicate resolveGroupName logic into a shared utility function (e.g., sessionUtils) and replace the inline implementations in both useBatchHandlers (function resolveGroupName) and useAgentListeners (the identical fallback block) with an import of that utility; ensure the utility accepts (sessionId, sessions, groups) and returns the same string fallback ('Ungrouped') so callers continue to work without other changes.
🤖 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/hooks/batch/useBatchHandlers.ts`:
- Around line 38-55: Extract the duplicate resolveGroupName logic into a shared
utility function (e.g., sessionUtils) and replace the inline implementations in
both useBatchHandlers (function resolveGroupName) and useAgentListeners (the
identical fallback block) with an import of that utility; ensure the utility
accepts (sessionId, sessions, groups) and returns the same string fallback
('Ungrouped') so callers continue to work without other changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0617a3d-3f0e-42b6-a343-74eab1e02e3d
📒 Files selected for processing (4)
src/renderer/components/QuickActionsModal.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/hooks/batch/useBatchHandlers.tssrc/renderer/hooks/session/useSessionCrud.ts
|
Thank you for this contribution, @scriptease! Great catch on the worktree child sessions not inheriting their parent's group. The two-pronged approach (proactive sync on group moves + reactive fallback in toast/notification handlers) is solid and covers all the code paths comprehensively. The CI failures (lint-and-format, test) are pre-existing on the LGTM — approving! |
Sync worktree children groupId when parent is moved to a group, and fall back to parent group in notification group name resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lize sentinel, extract helper - useSessionCrud handleGroupCreated: propagate groupId to worktree children - SessionList handleMoveToGroup: propagate groupId to worktree children - QuickActionsModal: normalize empty string groupId to undefined - useBatchHandlers: extract resolveGroupName helper for toast attribution Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
scriptease
force-pushed
the
worktree-toast
branch
from
e1cacdd to
6218490
Compare
3 participants
When moving a session with children (or git work trees) to a group after the child is already created, it doesn't get a groupId, so it would show toasts with "Ungrouped > " even though they are technically grouped by the parent session.
Summary by CodeRabbit