Merge test into main#1739
Conversation
* fix: remove dead ArtistAgent code and GET /api/agents endpoint Co-authored-by: Cursor <cursoragent@cursor.com> * updating --------- Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes the ChangesArtistAgent Type Removal Refactoring
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 13 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Removes dead ArtistAgent code and API endpoint. Changes are mechanical (removing union types, simplifying conditionals) with no behavior change. CI green, AI found no issues. Low risk of breakage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/Sidebar/Modals/RenameModal.tsx (1)
36-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing label and
aria-describedbyfor the rename input.The input has
id="chatName"but no associated<label htmlFor="chatName">, and the error message on line 50 isn't linked back to the input. Screen reader users won't hear the field name nor the validation feedback. Quick wins:♻️ Proposed accessibility fix
<h2 className="text-xl font-semibold mb-5">Rename Chat</h2> <form onSubmit={handleSubmit}> <div className="mb-5"> + <label htmlFor="chatName" className="sr-only"> + Chat name + </label> <input type="text" id="chatName" className={`w-full px-4 py-3 border rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-500 transition-colors duration-200 text-base ${ error ? "border-red-500" : "border-border" }`} value={name} onChange={handleChange} onBlur={handleBlur} placeholder="Enter chat name" + aria-invalid={Boolean(error)} + aria-describedby={error ? "chatName-error" : undefined} /> <div className="h-5 mt-2"> {error && ( - <p className="text-red-500 text-sm animate-fadeIn">{error}</p> + <p id="chatName-error" className="text-red-500 text-sm animate-fadeIn" role="alert"> + {error} + </p> )} </div> </div>As per coding guidelines: "Provide clear labels and error messages in form components" and "Use
aria-describedbyfor error message associations in form components".🤖 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 `@components/Sidebar/Modals/RenameModal.tsx` around lines 36 - 53, The rename input with id "chatName" lacks an associated label and its error message isn't linked for assistive tech; add a <label htmlFor="chatName"> (e.g., "Chat name") adjacent to the input and give the error <p> an id (e.g., "chatName-error") then set aria-describedby={error ? "chatName-error" : undefined} on the input (update where value/name are managed in the component using the existing name, handleChange, handleBlur and error variables) so screen readers announce both the field label and validation feedback.
🧹 Nitpick comments (5)
components/Sidebar/RecentChats/ChatItem.tsx (2)
55-67: ⚡ Quick winDuplicates the same optimistic-payload check in
useCreateChat.This block performs the same
content.optimistic === truediscrimination thathooks/useCreateChat.tsxdoes (lines 33–48 there). Now that the type isConversation-only, both call sites would benefit from a shared typed predicate (e.g.,isOptimisticMemory(memory)exported fromlib/chat/). Consolidating eliminates the parallelas Record<string, unknown>/ inline content-shape casts and keeps the optimistic contract in one place.🤖 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 `@components/Sidebar/RecentChats/ChatItem.tsx` around lines 55 - 67, This duplicates the optimistic-memory check; extract a shared typed predicate (e.g., export function isOptimisticMemory(memory: unknown): memory is MemoryType) into lib/chat (name it isOptimisticMemory) and replace the inline logic in ChatItem.tsx (the isOptimisticChatItem computation over chatRoom.memories) and the analogous check in useCreateChat.tsx with calls to isOptimisticMemory to remove the repeated casts and centralize the optimistic payload shape check.
46-52: 💤 Low value
useEffectto mirror prop into state can be replaced byuseSyncExternalStore-free derivation, but state is needed here.Local note rather than a change request: the state is required only because
useCreateChatwrites back viasetDisplayNameafter the server returns the topic. If you ever remove that callback path (e.g., let React Query's refetched conversation drive the topic and just derivegetChatDisplayInfo(chatRoom).displayNameinline), you could drop both theuseStateand thisuseEffectand have a single source of truth from the query cache.🤖 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 `@components/Sidebar/RecentChats/ChatItem.tsx` around lines 46 - 52, Keep the local state and effect as written because displayName is mutated via setDisplayName by useCreateChat after server response: retain const [displayName, setDisplayName] = useState(getChatDisplayInfo(chatRoom).displayName) and the useEffect that updates it on chatRoom changes, and ensure the create-chat callback in useCreateChat calls setDisplayName to write the server topic back; if you later remove that callback path, replace both the useState and useEffect with a derived const displayName = getChatDisplayInfo(chatRoom).displayName to use a single source of truth.hooks/useCreateChat.tsx (1)
33-69: ⚡ Quick winType-narrowing is fine; consider extracting the optimistic-message lookup.
The
contentshape is asserted viaascasts in two places (lines 36–39 and 55–59) with overlapping inline types. Now thatchatRoomis strictlyConversation, this is a good moment to lift the optimistic-payload type into@/types/Chat(e.g.,OptimisticMemoryContent) and use a small typed predicate likegetOptimisticFirstMessage(memories). That would also letChatItem.tsx(which performs the sameoptimistic === truecheck at lines 55–67) share the predicate — DRY across the optimistic flow.Not a blocker for this PR; flagging because the union removal makes the helper trivial to extract now.
🤖 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 `@hooks/useCreateChat.tsx` around lines 33 - 69, Extract the optimistic-memory content shape into a typed alias (e.g., OptimisticMemoryContent) in `@/types/Chat` and implement a small typed predicate/helper (e.g., getOptimisticFirstMessage(memories) or isOptimisticMemory(memory): memory is OptimisticMemoryContent) that encapsulates the optimistic === true and parts[0].text checks; then replace the inline casts and duplicated logic in useCreateChat.tsx (the chatRoom.memories find that produces firstMessage and the later cast used for messageText) with that helper, and reuse the same helper in ChatItem.tsx to remove duplication.lib/chat/getChatRoomId.ts (1)
3-3: 💤 Low valueTrivial passthrough — consider inlining at call sites eventually.
With the union gone,
getChatRoomId(x)is justx.id. Not worth churning this PR over, but on a future cleanup pass this helper (and arguablygetChatDisplayInfo) could be inlined or fully removed since they no longer encapsulate any branching logic.🤖 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 `@lib/chat/getChatRoomId.ts` at line 3, getChatRoomId is a trivial passthrough (returns chatRoom.id) now that the union type is gone; remove the helper and replace its call sites with direct property access (use chatRoom.id) or inline it where used, and likewise consider removing getChatDisplayInfo if it also contains no branching—update imports/usages referencing getChatRoomId (and getChatDisplayInfo) to use the direct fields and delete the now-unnecessary functions.components/Sidebar/Modals/DeleteConfirmationModal.tsx (1)
50-60: ⚖️ Poor tradeoffSequential bulk-delete is intentional but worth confirming the UX trade-off.
The for-loop awaits each
deleteChatso the progress counter (current/total) updates step-by-step — good for transparency. Trade-off: deleting N chats takes N round trips serialized. For typical bulk sizes (a handful) this is fine; if users routinely select dozens, consider a small concurrency window (e.g.,p-limitstyle, 3–5 in flight) while still reporting progress. Not changing anything yet — just flagging.🤖 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 `@components/Sidebar/Modals/DeleteConfirmationModal.tsx` around lines 50 - 60, Current implementation in the DeleteConfirmationModal loops sequentially over chatsToDelete awaiting deleteChat each time which is simple but serializes network calls; to address the reviewer’s trade-off either keep as-is for small batches or implement a bounded concurrency executor (e.g., using p-limit or a simple worker pool) to run ~3–5 deleteChat calls in parallel while still updating setDeletingProgress as each promise settles and pushing failures into failedChats; ensure you use the same identifiers (chatsToDelete, deleteChat, setDeletingProgress, failedChats, chatCount) so progress increments on each completed delete and errors are collected and logged consistently.
🤖 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 `@lib/chat/getChatDisplayInfo.ts`:
- Around line 3-5: Update getChatDisplayInfo to use the same empty-topic
fallback as the UI: return displayName: item.topic || "Untitled Chat" in
getChatDisplayInfo, and remove the duplicate fallback logic in ChatItem (so
ChatItem uses the displayName from getChatDisplayInfo directly); target the
getChatDisplayInfo export and the ChatItem component's displayName usage to
consolidate the placeholder string.
---
Outside diff comments:
In `@components/Sidebar/Modals/RenameModal.tsx`:
- Around line 36-53: The rename input with id "chatName" lacks an associated
label and its error message isn't linked for assistive tech; add a <label
htmlFor="chatName"> (e.g., "Chat name") adjacent to the input and give the error
<p> an id (e.g., "chatName-error") then set aria-describedby={error ?
"chatName-error" : undefined} on the input (update where value/name are managed
in the component using the existing name, handleChange, handleBlur and error
variables) so screen readers announce both the field label and validation
feedback.
---
Nitpick comments:
In `@components/Sidebar/Modals/DeleteConfirmationModal.tsx`:
- Around line 50-60: Current implementation in the DeleteConfirmationModal loops
sequentially over chatsToDelete awaiting deleteChat each time which is simple
but serializes network calls; to address the reviewer’s trade-off either keep
as-is for small batches or implement a bounded concurrency executor (e.g., using
p-limit or a simple worker pool) to run ~3–5 deleteChat calls in parallel while
still updating setDeletingProgress as each promise settles and pushing failures
into failedChats; ensure you use the same identifiers (chatsToDelete,
deleteChat, setDeletingProgress, failedChats, chatCount) so progress increments
on each completed delete and errors are collected and logged consistently.
In `@components/Sidebar/RecentChats/ChatItem.tsx`:
- Around line 55-67: This duplicates the optimistic-memory check; extract a
shared typed predicate (e.g., export function isOptimisticMemory(memory:
unknown): memory is MemoryType) into lib/chat (name it isOptimisticMemory) and
replace the inline logic in ChatItem.tsx (the isOptimisticChatItem computation
over chatRoom.memories) and the analogous check in useCreateChat.tsx with calls
to isOptimisticMemory to remove the repeated casts and centralize the optimistic
payload shape check.
- Around line 46-52: Keep the local state and effect as written because
displayName is mutated via setDisplayName by useCreateChat after server
response: retain const [displayName, setDisplayName] =
useState(getChatDisplayInfo(chatRoom).displayName) and the useEffect that
updates it on chatRoom changes, and ensure the create-chat callback in
useCreateChat calls setDisplayName to write the server topic back; if you later
remove that callback path, replace both the useState and useEffect with a
derived const displayName = getChatDisplayInfo(chatRoom).displayName to use a
single source of truth.
In `@hooks/useCreateChat.tsx`:
- Around line 33-69: Extract the optimistic-memory content shape into a typed
alias (e.g., OptimisticMemoryContent) in `@/types/Chat` and implement a small
typed predicate/helper (e.g., getOptimisticFirstMessage(memories) or
isOptimisticMemory(memory): memory is OptimisticMemoryContent) that encapsulates
the optimistic === true and parts[0].text checks; then replace the inline casts
and duplicated logic in useCreateChat.tsx (the chatRoom.memories find that
produces firstMessage and the later cast used for messageText) with that helper,
and reuse the same helper in ChatItem.tsx to remove duplication.
In `@lib/chat/getChatRoomId.ts`:
- Line 3: getChatRoomId is a trivial passthrough (returns chatRoom.id) now that
the union type is gone; remove the helper and replace its call sites with direct
property access (use chatRoom.id) or inline it where used, and likewise consider
removing getChatDisplayInfo if it also contains no branching—update
imports/usages referencing getChatRoomId (and getChatDisplayInfo) to use the
direct fields and delete the now-unnecessary functions.
🪄 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: CHILL
Plan: Pro
Run ID: 47303af6-30eb-4fed-924c-93ece88e2dc2
📒 Files selected for processing (13)
app/api/agents/route.tscomponents/Sidebar/Modals/DeleteConfirmationModal.tsxcomponents/Sidebar/Modals/RenameModal.tsxcomponents/Sidebar/RecentChats/ChatItem.tsxcomponents/Sidebar/RecentChats/RecentChatList.tsxcomponents/Sidebar/RecentChats/useRecentChats.tshooks/useArtistAgents.tshooks/useConversations.tsxhooks/useCreateChat.tsxhooks/useRenameModal.tslib/chat/getChatDisplayInfo.tslib/chat/getChatRoomId.tslib/supabase/getArtistAgents.ts
💤 Files with no reviewable changes (3)
- hooks/useArtistAgents.ts
- lib/supabase/getArtistAgents.ts
- app/api/agents/route.ts
| export const getChatDisplayInfo = (item: Conversation) => ({ | ||
| displayName: item.topic || "Chat Analysis", | ||
| }); |
There was a problem hiding this comment.
Fallback string is inconsistent with ChatItem and now reads stale.
ChatItem.tsx (line 86) falls back to "Untitled Chat" when displayName is empty/whitespace, but this helper returns "Chat Analysis" when topic is falsy. Two issues:
- The "Analysis" wording was meaningful when this helper handled
ArtistAgent; with the union gone it no longer fits the conversation-only model. - Two different fallbacks exist for what is conceptually the same empty-topic state, so users will see different placeholders depending on which code path runs.
Suggest consolidating on one label (e.g., "Untitled Chat") and dropping the duplicate fallback in ChatItem.tsx.
♻️ Proposed fix
-export const getChatDisplayInfo = (item: Conversation) => ({
- displayName: item.topic || "Chat Analysis",
-});
+export const getChatDisplayInfo = (item: Conversation) => ({
+ displayName: item.topic?.trim() || "Untitled Chat",
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getChatDisplayInfo = (item: Conversation) => ({ | |
| displayName: item.topic || "Chat Analysis", | |
| }); | |
| export const getChatDisplayInfo = (item: Conversation) => ({ | |
| displayName: item.topic?.trim() || "Untitled Chat", | |
| }); |
🤖 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 `@lib/chat/getChatDisplayInfo.ts` around lines 3 - 5, Update getChatDisplayInfo
to use the same empty-topic fallback as the UI: return displayName: item.topic
|| "Untitled Chat" in getChatDisplayInfo, and remove the duplicate fallback
logic in ChatItem (so ChatItem uses the displayName from getChatDisplayInfo
directly); target the getChatDisplayInfo export and the ChatItem component's
displayName usage to consolidate the placeholder string.
2 participants
Summary
Promotes
test→main. Brings in:Test plan
testSummary by cubic
Removes the deprecated ArtistAgent flow and the GET /api/agents endpoint. Consolidates chat logic around Conversation only to reduce complexity.
/api/agents,lib/supabase/getArtistAgents, andhooks/useArtistAgents.Conversationonly; dropped union types andagentIdlogic.getChatDisplayInfo,getChatRoomId) to work withConversationonly.Written for commit a77ca7f. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Removed Features
Improvements