fix: conversation rename & delete dialog#18
Merged
Conversation
- Fix update_conversation_title using two-query pattern (supabase-py v2 does not support .select() chaining after .update()) - Add global unhandled exception handler in FastAPI to ensure 500 errors include CORS headers (previously ServerErrorMiddleware bypassed CORS) - Catch all exceptions in _get_jwks() not just httpx.HTTPError - Add try/except with logging in PATCH conversation route - Replace double-click delete confirm with a modal dialog (base-ui Dialog) - Add new Dialog UI component using @base-ui/react/dialog - Fix absolute .env path resolution in config.py - Update COMMANDS.md with correct PowerShell activation command
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR introduces YouTube video detection and routing, improves backend error handling, refactors the conversation update service, and adds a modal dialog pattern to the frontend. Changes span backend infrastructure (config, security, exceptions), agent orchestration (YouTube routing), chat features (conversation service), and frontend UI (dialog component and sidebar integration). ChangesCore Infrastructure Updates
YouTube Agent and Supervisor Routing
Chat Conversation Update Flow
Frontend Dialog and Sidebar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/features/chat/conversation_service.py (1)
239-276: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winThe "no
.select()chaining" comment is inaccurate — chaining is supported.The Supabase Python reference explicitly documents "Chain .select() after update() to return specific columns from the updated row(s)." This is the same pattern used by
.insert()calls elsewhere in this same module. Replacing the second round-trip with a chained.select(...)would halve the DB calls per rename and remove a benign race window where a concurrently deleted row makes a successful update appear as 404.♻️ Proposed simplification
- """Update a conversation's title. - - Returns the updated ConversationOut, or None if not found / not owned. - supabase-py v2 update() builder does not support .select() chaining, so we - execute the update first, then fetch the updated row in a separate query. - """ + """Update a conversation's title. + + Returns the updated ConversationOut, or None if not found / not owned. + """ client = await get_supabase_client() try: result = ( await client.table("conversations") .update({"title": title}) .eq("id", conversation_id) .eq("user_id", user_id) + .select("id, title, model_id, created_at, updated_at") .execute() ) except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught logger.exception( "update_conversation_title: update failed conversation_id=%s", conversation_id, ) raise if not result.data: return None - try: - fetch = ( - await client.table("conversations") - .select("id, title, model_id, created_at, updated_at") - .eq("id", conversation_id) - .eq("user_id", user_id) - .limit(1) - .execute() - ) - except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught - logger.exception( - "update_conversation_title: fetch failed conversation_id=%s", - conversation_id, - ) - raise - if not fetch.data: - return None - return ConversationOut.model_validate(fetch.data[0]) + return ConversationOut.model_validate(result.data[0])Please verify against the exact
supabase/postgrestversions pinned inbackend/pyproject.toml(or equivalent) — if a regression genuinely blocks.select()chaining there, please link the upstream issue in the comment instead of stating the chaining is unsupported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/features/chat/conversation_service.py` around lines 239 - 276, The comment incorrectly claims .select() chaining after .update() is unsupported; change update_conversation_title to use a single chained call on client.table("conversations") — i.e., call .update({"title": title}).eq("id", conversation_id).eq("user_id", user_id).select("id, title, model_id, created_at, updated_at").limit(1).execute() instead of performing a separate fetch, then validate the returned row with ConversationOut.model_validate(fetch.data[0]); also verify the pinned supabase/postgrest versions in pyproject.toml and, if chaining fails, replace the comment with a link to the upstream issue.frontend/features/chat/components/ConversationSidebar.tsx (1)
84-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep dialog open until deletion completes.
Closing the dialog immediately (line 85) before the mutation finishes hides the loading state and any potential errors from the user. On slow networks or if the deletion fails, the user sees no feedback.
🔄 Recommended fix to show loading state and handle errors
function handleDeleteConfirm() { - setDeleteOpen(false); deleteConv(conversation.id, { - onSuccess: () => onDeleted(conversation.id), + onSuccess: () => { + setDeleteOpen(false); + onDeleted(conversation.id); + }, + onError: (err) => { + console.error('Failed to delete conversation:', err); + // Optionally: show error message in dialog or toast + setDeleteOpen(false); + }, }); }Then update the dialog buttons to show loading state:
<DialogClose className='...'> Cancel </DialogClose> <button type='button' onClick={handleDeleteConfirm} + disabled={deleteLoading} - className='rounded-lg bg-[`#ff4444`]/90 px-4 py-2 text-sm font-medium text-white transition-colors hover:bg-[`#ff4444`] cursor-pointer' + className={[ + 'rounded-lg bg-[`#ff4444`]/90 px-4 py-2 text-sm font-medium text-white transition-colors hover:bg-[`#ff4444`] cursor-pointer', + deleteLoading ? 'opacity-50 cursor-not-allowed' : '', + ].join(' ')} > - Delete + {deleteLoading ? 'Deleting...' : 'Delete'} </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/features/chat/components/ConversationSidebar.tsx` around lines 84 - 89, The dialog is being closed immediately in handleDeleteConfirm which hides loading and errors; instead remove the immediate setDeleteOpen(false) and call deleteConv(conversation.id, { onSuccess: () => { setDeleteOpen(false); onDeleted(conversation.id); }, onError: (err) => { /* keep dialog open and surface err.message to user */ } });; update the dialog's action buttons to use the mutation's loading state (e.g., deleteConv.isLoading or a local isDeleting derived from the mutation) to show a spinner/disable buttons while deleting and display the mutation error message in the dialog when onError runs; keep references to handleDeleteConfirm, deleteConv, setDeleteOpen, and onDeleted to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/core/security.py`:
- Line 39: The except clause in backend.app.core.security.py currently uses a
redundant tuple "except (httpx.HTTPError, Exception) as exc"; change this to
either a single broad handler "except Exception as exc" or, preferably, a
narrower handler that only catches the intended httpx errors like "except
httpx.HTTPError as exc" (or add the project's lint suppression if a broad catch
is required). Update the except clause near the existing try/except block in
security.py (the line containing "except (httpx.HTTPError, Exception) as exc")
to use the chosen single exception form and keep the existing logging/handling
of exc unchanged.
In `@backend/app/features/chat/router.py`:
- Around line 119-127: The current except block in
backend/app/features/chat/router.py is catching all Exception and will convert
any raised HTTPException into a 500; change the error handling so HTTPException
is re-raised unchanged and only other exceptions are logged and wrapped into a
500. Concretely, inside the update_conversation handler (the block using
logger.exception and raising HTTPException("Failed to update conversation")),
either add an explicit except HTTPException: raise to propagate client errors,
then have a separate except Exception as exc that logs via logger.exception(...)
and raises the 500, or detect isinstance(exc, HTTPException) and re-raise before
wrapping; ensure you reference the existing logger.exception call and the raised
HTTPException detail message.
In `@frontend/components/ui/dialog.tsx`:
- Around line 11-73: The components DialogBackdrop, DialogPopup, DialogTitle and
DialogDescription still use React.forwardRef and assign displayName; update them
to the React 19 pattern by accepting ref as a normal prop in the component
signature (remove React.forwardRef wrappers), remove the now-unnecessary
displayName assignments, and pass the ref through to the underlying Dialog.*
elements via the ref prop; ensure prop types remain React.ElementRef<typeof
Dialog.Backdrop/Popup/Title/Description> and
React.ComponentPropsWithoutRef<typeof Dialog.Backdrop/Popup/Title/Description>
so type compatibility is preserved.
---
Outside diff comments:
In `@backend/app/features/chat/conversation_service.py`:
- Around line 239-276: The comment incorrectly claims .select() chaining after
.update() is unsupported; change update_conversation_title to use a single
chained call on client.table("conversations") — i.e., call .update({"title":
title}).eq("id", conversation_id).eq("user_id", user_id).select("id, title,
model_id, created_at, updated_at").limit(1).execute() instead of performing a
separate fetch, then validate the returned row with
ConversationOut.model_validate(fetch.data[0]); also verify the pinned
supabase/postgrest versions in pyproject.toml and, if chaining fails, replace
the comment with a link to the upstream issue.
In `@frontend/features/chat/components/ConversationSidebar.tsx`:
- Around line 84-89: The dialog is being closed immediately in
handleDeleteConfirm which hides loading and errors; instead remove the immediate
setDeleteOpen(false) and call deleteConv(conversation.id, { onSuccess: () => {
setDeleteOpen(false); onDeleted(conversation.id); }, onError: (err) => { /* keep
dialog open and surface err.message to user */ } });; update the dialog's action
buttons to use the mutation's loading state (e.g., deleteConv.isLoading or a
local isDeleting derived from the mutation) to show a spinner/disable buttons
while deleting and display the mutation error message in the dialog when onError
runs; keep references to handleDeleteConfirm, deleteConv, setDeleteOpen, and
onDeleted to locate and modify the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8759e42b-c89e-408d-869b-fa3d7e33a897
📒 Files selected for processing (10)
COMMANDS.mdbackend/app/agents/subagents/youtube_agent.pybackend/app/agents/supervisor.pybackend/app/core/config.pybackend/app/core/security.pybackend/app/features/chat/conversation_service.pybackend/app/features/chat/router.pybackend/app/main.pyfrontend/components/ui/dialog.tsxfrontend/features/chat/components/ConversationSidebar.tsx
- security.py: simplify redundant except tuple to bare 'except Exception' - router.py: re-raise HTTPException before generic except so client errors are not converted to 500 - dialog.tsx: migrate forwardRef components to React 19 ref-as-prop pattern, remove displayName assignments - conversation_service.py: improve comment, add link to upstream postgrest-py issue #394 explaining why two-query approach is required - ConversationSidebar: move setDeleteOpen(false) into onSuccess, add onError to surface delete errors in dialog, disable buttons while deleting
This was referenced May 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation