feat(chat): session-scoped sidebar rename + archive-on-delete#1763
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (6)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR refactors chat deletion from direct row deletion to session archiving, migrates chat updates to session-scoped endpoints, and threads session IDs through modal workflows. The old ChangesSession-scoped chat operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
💡 Codex ReviewLine 11 in 65a5558  Update active-chat detection for session routes
When a user clicks a sidebar row, this now navigates to ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Repoints sidebar rename and delete at the session-scoped api so they
operate on the same `chats` / `sessions` rows the workflow surface
persists into — the previous handlers hit the legacy `rooms`-shaped
endpoints and silently no-op'd on workflow chats.
- Rename: `PATCH /api/chats { chatId, topic }` → `PATCH /api/sessions/{sid}/chats/{cid} { title }`. `useRenameModal` and `updateChat` thread `sessionId` from the `Conversation` row and switch the wire field to `title`.
- Delete: switch from row-delete to session archive (`PATCH /api/sessions/{sid} { status: "archived" }`). The api filters archived sessions out of `GET /api/chats` (recoupable/api#630), so the chat disappears from the sidebar; archive also runs the existing `stopSandboxOnArchive` lifecycle, and the whole thing stays reversible from the admin side.
- Sidesteps the previous "Cannot delete the only chat in a session" 400, which was a real footgun once every workflow chat ended up in a 1-chat session.
- Removes the now-unused `lib/chats/deleteChat.ts`; adds `lib/sessions/archiveSession.ts` for the new wire call.
If a session ever has >1 chat, archiving from one row will hide the
others too. Every sidebar row maps 1:1 to its own session today
(each `POST /api/sessions` mints a session + single chat), so this is
a no-op in practice — worth knowing before any future flow creates
additional chats inside an existing session.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arpitgupta1214
force-pushed
the
feat/sidebar-delete-rename-session-scoped
branch
from
b2f6efe to
8187b1a
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 14 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the reported issue is low-to-moderate severity (4/10) and appears limited to a specific deletion flow rather than broad app behavior.
- In
components/Sidebar/Modals/DeleteConfirmationModal.tsx, deleting chats that share a session can trigger repeated archive attempts on the same session, which may surface misleading "Failed to delete" errors for some rows. - User impact is mainly confusing feedback during multi-chat deletion, not clear data corruption or a systemic regression from the summary provided.
- Pay close attention to
components/Sidebar/Modals/DeleteConfirmationModal.tsx- repeated session archiving in the loop can cause false failure messages.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="components/Sidebar/Modals/DeleteConfirmationModal.tsx">
<violation number="1" location="components/Sidebar/Modals/DeleteConfirmationModal.tsx:68">
P2: When multiple chats share a session, the loop archives the same session repeatedly. Subsequent calls for an already-archived session likely fail, producing confusing "Failed to delete" errors for rows that were effectively archived. Consider deduplicating by `sessionId` before iterating.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| try { | ||
| await deleteChat(chat.id); | ||
| await deleteChat({ sessionId: chat.sessionId }); |
There was a problem hiding this comment.
P2: When multiple chats share a session, the loop archives the same session repeatedly. Subsequent calls for an already-archived session likely fail, producing confusing "Failed to delete" errors for rows that were effectively archived. Consider deduplicating by sessionId before iterating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Sidebar/Modals/DeleteConfirmationModal.tsx, line 68:
<comment>When multiple chats share a session, the loop archives the same session repeatedly. Subsequent calls for an already-archived session likely fail, producing confusing "Failed to delete" errors for rows that were effectively archived. Consider deduplicating by `sessionId` before iterating.</comment>
<file context>
@@ -52,15 +65,18 @@ const DeleteConfirmationModal = ({ isOpen, onClose, chatRoom, chatRooms, onDelet
try {
- await deleteChat(chat.id);
+ await deleteChat({ sessionId: chat.sessionId });
} catch (chatError) {
- console.error(`Error deleting chat ${chat.topic || "Chat"}:`, chatError);
</file context>
Preview verification — live test on
|
| Step | Result |
|---|---|
| Click "Chat options" → "Rename" on existing chat | Modal opens with current title pre-filled ✅ |
| Edit title, click "Rename" | PATCH /api/sessions/{sid}/chats/{cid} body {"title":"..."} → 200 ✅ |
| Sidebar updates | Row shows new title, bubbles to top by updatedAt ✅ |
Wire shape: session-scoped PATCH path, body uses title (not legacy topic).
Delete flow (now fully working with api#630 merged)
| Step | Result |
|---|---|
| Click "Chat options" → "Delete" | Confirmation modal opens with chat title quoted ✅ |
| Click "Delete" | PATCH /api/sessions/{sid} body {"status":"archived"} → 200, response shows status: "archived", lifecycleState: "archived" ✅ |
| Sandbox teardown | lifecycleState: "archived" in response confirms stopSandboxOnArchive fired ✅ |
| Auto-refetch | GET /api/chats?artist_account_id=... re-fires immediately after the PATCH ✅ |
| Row removal | api#630 filters the archived session out; row vanishes from sidebar in ~1s (14 → 13 chats) ✅ |
Reversibility check
Tested via curl: PATCH /api/sessions/{sid} {"status":"running"} → next GET /api/chats returns the chat again. So "delete" is a soft-delete from the user's perspective but recoverable from the admin side, matching the PR body's design.
Recommendation
All four substantive claims of #1763 verified end-to-end. Recommend merge.
2 participants
Replaces chat#1758, which was auto-closed when its base branch (
feat/sidebar-canonical-urls) was deleted alongside the chat#1756 merge. Same code, retargeted attest.Two related changes to the sidebar's chat actions, now that chat#1756 has shipped session-scoped chat listings:
Rename — session-scoped PATCH
The rename flow was still hitting the legacy
PATCH /api/chatsshape ({ chatId, topic }), which only touched theroomstable. Repoint atPATCH /api/sessions/{sid}/chats/{cid}with{ title }so the canonical chat row is updated.updateChat({ sessionId, chatId, title })→PATCH /api/sessions/{sid}/chats/{cid}.useRenameModalsendstitle(wastopic).Delete — archive the owning session
Instead of removing the chat row, the "Delete chat" action now archives the owning session via
PATCH /api/sessions/{sid} { status: "archived" }. The api filters chats from archived sessions out ofGET /api/chats(api#630), so the row disappears from the sidebar; archive also triggersstopSandboxOnArchiveso we tear down running sandboxes for chats the user said they're done with. Reversible from the admin side.This also sidesteps the
400 Cannot delete the only chat in a sessionguard that blocks chat-row deletes when a session has a single chat — archive has no such constraint.lib/sessions/archiveSession.ts→PATCH /api/sessions/{sid} { status: "archived" }.useDeleteChatcalls it (drops thechatIdarg).DeleteConfirmationModalno longer passeschatId.Caveat
If a session ever has >1 chat, archiving from one row hides the others too. Today every sidebar row maps 1:1 to its own session (
POST /api/sessionsmints a fresh session per chat), so this is a theoretical concern. Will need to revisit if/when sessions ever hold multiple chats.Summary by cubic
Session-scope the sidebar: renaming now updates the canonical chat row, and “Delete chat” archives the owning session so chats disappear and sandboxes stop.
New Features
PATCH /api/sessions/{sid}/chats/{cid}with{ title }(threadssessionId; switches fromtopic→title).PATCH /api/sessions/{sid}with{ status: "archived" }; avoids the “Cannot delete the only chat in a session” 400, is reversible, and triggers sandbox teardown.useDeleteChatnow takes{ sessionId }.lib/chats/deleteChat.ts; addedlib/sessions/archiveSession.ts.Migration
GET /api/chats.Written for commit 8187b1a. Summary will update on new commits.
Summary by CodeRabbit