Allow renaming and delete of (almost) all sessions.#222
Allow renaming and delete of (almost) all sessions.#222cyan-ember wants to merge 2 commits intodaggerhashimoto:masterfrom
Conversation
📝 WalkthroughWalkthroughThis change blocks deletion of top-level main agent sessions by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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/features/sessions/SessionNode.tsx (1)
61-82:⚠️ Potential issue | 🟠 MajorMemoization comparator must check handler presence changes.
The
arePropsEqualfunction ignores the optional handler props (onAbort,onDelete,onStartRename), but lines 146–149 derive action button visibility directly from whether these handlers exist. WhenSessionListconditionally passes handlers based on parent configuration (lines 228–229), toggling handler availability won't trigger a re-render, leaving stale buttons or missing actions until an unrelated prop changes.Add handler presence comparisons to the memo predicate:
Suggested fix
prev.renameValue === next.renameValue && prev.granularStatus === next.granularStatus && - prev.compact === next.compact + prev.compact === next.compact && + Boolean(prev.onAbort) === Boolean(next.onAbort) && + Boolean(prev.onDelete) === Boolean(next.onDelete) && + Boolean(prev.onStartRename) === Boolean(next.onStartRename)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sessions/SessionNode.tsx` around lines 61 - 82, arePropsEqual currently omits optional handler props so changes to onAbort, onDelete, or onStartRename won't trigger re-renders and action button visibility stays stale; update arePropsEqual (which compares SessionNodeProps) to also compare presence (and/or reference) of prev.onAbort vs next.onAbort, prev.onDelete vs next.onDelete, and prev.onStartRename vs next.onStartRename so toggling those handlers in SessionList updates SessionNode rendering.
🧹 Nitpick comments (2)
src/features/sessions/SessionList.tsx (1)
56-57: Drop the now-deadisRootAgentfield fromdeleteTarget.After the Line 141 early return, this state is never populated for top-level agent sessions, so the field is always
falseand no longer read. Removing it will make the delete-dialog state contract easier to follow.Suggested cleanup
- const [deleteTarget, setDeleteTarget] = useState<{ key: string; label: string; descendantCount: number; isRootAgent: boolean } | null>(null); + const [deleteTarget, setDeleteTarget] = useState<{ key: string; label: string; descendantCount: number } | null>(null);setDeleteTarget({ key, label, descendantCount: targetNode ? countDescendants(targetNode) : 0, - isRootAgent: false, });Also applies to: 147-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sessions/SessionList.tsx` around lines 56 - 57, The state type for deleteTarget contains a now-unused isRootAgent field; remove isRootAgent from the useState declaration for deleteTarget and from any places that set or read it (e.g., calls to setDeleteTarget, delete-dialog props/state consumers and any destructuring that expects isRootAgent). Update the type to { key: string; label: string; descendantCount: number } | null, remove any logic branches that reference isRootAgent, and ensure deleteBlockedTarget usage remains unchanged.src/features/sessions/SessionList.test.tsx (1)
78-90: Also assert that deletion stays blocked.This currently proves the informational dialog opens, but not that the destructive callback stayed untouched. Please keep the
onDeletemock in a variable and assert it is not called after the click sequence, otherwise a regression that both opens the modal and still deletes would pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sessions/SessionList.test.tsx` around lines 78 - 90, The test opens the informational dialog but doesn't assert the delete callback wasn't invoked; change the inline onDelete mock to a named mock variable (e.g., const onDelete = vi.fn()) passed into renderSessionList, then after firing the actions assert that onDelete was not called (expect(onDelete).not.toHaveBeenCalled()) to ensure deletion remains blocked; update references in this test to use the named onDelete mock and keep the existing assertions that the dialog and link are present.
🤖 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/features/sessions/SessionNode.tsx`:
- Around line 61-82: arePropsEqual currently omits optional handler props so
changes to onAbort, onDelete, or onStartRename won't trigger re-renders and
action button visibility stays stale; update arePropsEqual (which compares
SessionNodeProps) to also compare presence (and/or reference) of prev.onAbort vs
next.onAbort, prev.onDelete vs next.onDelete, and prev.onStartRename vs
next.onStartRename so toggling those handlers in SessionList updates SessionNode
rendering.
---
Nitpick comments:
In `@src/features/sessions/SessionList.test.tsx`:
- Around line 78-90: The test opens the informational dialog but doesn't assert
the delete callback wasn't invoked; change the inline onDelete mock to a named
mock variable (e.g., const onDelete = vi.fn()) passed into renderSessionList,
then after firing the actions assert that onDelete was not called
(expect(onDelete).not.toHaveBeenCalled()) to ensure deletion remains blocked;
update references in this test to use the named onDelete mock and keep the
existing assertions that the dialog and link are present.
In `@src/features/sessions/SessionList.tsx`:
- Around line 56-57: The state type for deleteTarget contains a now-unused
isRootAgent field; remove isRootAgent from the useState declaration for
deleteTarget and from any places that set or read it (e.g., calls to
setDeleteTarget, delete-dialog props/state consumers and any destructuring that
expects isRootAgent). Update the type to { key: string; label: string;
descendantCount: number } | null, remove any logic branches that reference
isRootAgent, and ensure deleteBlockedTarget usage remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfbc67c6-87ef-4014-8210-15ff50ccf864
📒 Files selected for processing (3)
src/features/sessions/SessionList.test.tsxsrc/features/sessions/SessionList.tsxsrc/features/sessions/SessionNode.tsx
|
This addresses a real problem, thank you for introducing this. However there is a minor point I'd like to discuss before we go ahead with this:
In my view the ideal behaviour should be preventing deletes for only so if non-main top-level roots should remain deletable (which I think is desirable for most users) this would be too broad for that. What do you think ? |
|
Yeah, I was thinking about that too. Here's where I ended up. The Agent panel is operating both as a session list and an Agent list (and I think it does that in an elegant way). But when you click on the agent::main session its a bit ambiguous on whether you want to do an agent-operation or a session-operation if you click delete, you can interpret it either as an agent-delete or as a session-delete (of the agent::main session)
So here's where it gets a little weird. If you delete an agent::main session without deleting the agent, then the gateway will simply spawn a new session next time you talk to the agent. Which ends up having the same effect as if you used /reset initially rather than delete and then a re-message (although the mechanism is different). The question is, how do you talk to that agent? If it's bound to a channel like telegram, you can message it through the channel to spawn a new session, but what should Nerve show in the Agent panel at that point (where there is an agent, but no session), and how would Nerve route a message to that session-less agent to spawn a new session (not actually sure on this point, but I assume it must be possible). So here's where I ended up on how I think the Nerve UI can best abstract what's going on in the gateway when doing deletes. If you're deleting
I wasn't ready to tackle deleting agents, so this PR only goes half way handles the session half, and then on *:main sessions pops a modal to educate on /reset (since there's already a button) and delete (by linking to documentation). It felt like a good initial step, but I think the right full solution would be what I think you're suggesting and enable agent deletes as well. So perhaps the next PR after this (or along side it) should update that education modal and turn it into a two button modal with, "do yo want to (a) reset the session, or (b) delete the agent." |
|
You're right, my previous comment was from a narrow angle view. The current PR only really handles the session-deletion side, not the broader agent-deletion case and those are not the same thing. I agree with your framing here: for If you don't want to blow-up the scope of this work, my suggestion would be to keep this PR scoped to the session/transcript side and merge it as the foundation without changing current behavior around root session deletion until that follow-up is properly designed. But if you want to keep pushing and build the full agent deletion path here, I'm fine with that too. Let me know which direction you want to take. Also, thank you for the work here. This was needed, and I appreciate you stepping in and thinking it through. |
2 participants
What
Makes rename and delete controls consistently available for all sessions in the Agent panel, while blocking deletion of main agent sessions with a clearer explanatory modal.
Why
Agents panel currently hides rename/delete for some session types even though the gateway broadly supports session labeling and deletion. This makes direct-channel sessions and ACP sessions feel inconsistent in the agent panel. Main agent sessions are still not deletable, but that exception is better handled with an explicit explanation than by hiding the control.
How
Extends the existing session action visibility so all session rows can show rename/delete controls in the agent panel, including direct-channel, ACP, and cron-backed sessions.
Intercepts delete clicks for main agent sessions and shows a modal explaining that:
Adds focused regression tests covering:
Manual testing
Summary by CodeRabbit
Bug Fixes
Tests