refactor(chat): update chat components to use chatId instead of roomId#1765
refactor(chat): update chat components to use chatId instead of roomId#1765ahmednahima0-beep wants to merge 5 commits into
Conversation
This commit removes the InstantChatRoom component and updates various hooks and components to replace references of roomId with chatId. The changes ensure that chat functionality is aligned with the new session-scoped architecture, improving consistency across the chat system. Additionally, the sessionId is now required in several components to streamline the chat transport process.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 33 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR completes a migration from the legacy ChangesSession-scoped routing migration
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 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Line 297 in 3cb190d
Update chat-route guards for session URLs
This now moves new chats onto /sessions/${sessionId}/chats/${id}, but the existing organization-change guard still only recognizes pathname?.startsWith("/chat/") in providers/OrganizationProvider.tsx:58. In the scenario where a user changes organization while viewing one of these new session URLs, the guard no longer navigates away, and subsequent sends in the same chat use the newly selected organizationId from chatRequestBody, mixing org context into an existing conversation.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
![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 7 files
Confidence score: 2/5
- There is a clear user-facing regression risk: removing
app/chat/[roomId]/page.tsxmeans/chat/{id}can 404 while the app still generates and navigates to those URLs. - Because this issue is high severity (8/10) with high confidence (8/10) and affects core navigation flows, merge risk is high until routing and link generation are aligned.
- Pay close attention to
app/chat/[roomId]/page.tsx,useCreateArtistTool, andgenerateTxtFileEmail- they currently appear to disagree on whether/chat/{id}is a valid destination.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This commit enhances the chat components by introducing the `sessionId` in various hooks and updating the URL handling to utilize new utility functions for generating chat paths. The changes improve the consistency and maintainability of chat navigation, ensuring that the application correctly reflects the session-scoped architecture.
![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 (2)
hooks/useVercelChat.ts (1)
282-290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip persisted-history loading for route-less chats.
A fresh chat opened from
/or/chatstill passesidintouseMessageLoader, so it shows the loading state and makes a history request before the first message even though there is no persistedchatIdyet. Gate this path onrouteChatId, not justid.Proposed fix
+ const persistedChatId = routeChatId ? id : undefined; + const { isLoading: isMessagesLoading, hasError } = useMessageLoader( sessionId, - messages.length === 0 ? id : undefined, + messages.length === 0 ? persistedChatId : undefined, userId, setMessages, ); // Only show loading state when fetching persisted history for an existing chat. - const isLoading = isMessagesLoading && !!id && messages.length === 0; + const isLoading = + isMessagesLoading && !!persistedChatId && messages.length === 0;🤖 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/useVercelChat.ts` around lines 282 - 290, The loading and history fetch are being triggered for route-less chats because useMessageLoader is passed id; change the call and gating to use the route-specific identifier (routeChatId) instead of id: call useMessageLoader(sessionId, messages.length === 0 ? routeChatId : undefined, userId, setMessages) and compute isLoading as isMessagesLoading && !!routeChatId && messages.length === 0 so the persisted-history path only runs when a routed chatId exists; update references to use routeChatId in this block (isMessagesLoading, isLoading, and the ternary argument).hooks/useCreateArtistTool.ts (1)
21-88:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPrevent this effect from replaying the same copy workflow.
Once
finallyresetsisProcessingtofalse, this effect is eligible to run again with the sameresult, which can callcopyMessages(...)andrefetchConversations()multiple times for one artist creation. SincecopyMessagesis a write, that makes the migration non-idempotent and can duplicate/corrupt the new chat state. A stable “already handled this result” guard is needed here instead of relying onisProcessingalone.Suggested fix
-import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; @@ export function useCreateArtistTool(result: CreateArtistResult) { const { status, id, sessionId } = useVercelChatContext(); @@ const [isProcessing, setIsProcessing] = useState(false); const [isSuccess, setIsSuccess] = useState(false); const [error, setError] = useState<string | null>(null); + const processedResultKeyRef = useRef<string | null>(null); useEffect(() => { @@ + const resultKey = `${result.artist?.account_id ?? ""}:${result.newRoomId ?? ""}`; + const shouldSkip = !result.artist || !result.artist.account_id || isProcessing || - !isFinishedStreaming; + !isFinishedStreaming || + processedResultKeyRef.current === resultKey; if (shouldSkip) { return; } const processCreateArtistResult = async () => { try { + processedResultKeyRef.current = resultKey; setIsProcessing(true); @@ if (success) { @@ setIsSuccess(true); } else { console.error("Failed to copy messages"); setError("Failed to copy messages to the new artist"); + processedResultKeyRef.current = null; } } else { setIsSuccess(true); } } catch (error) { console.error("Error in useCreateArtistTool:", error); setError(error instanceof Error ? error.message : "Unknown error"); + processedResultKeyRef.current = null; } finally { setIsProcessing(false); } };🤖 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/useCreateArtistTool.ts` around lines 21 - 88, The effect can re-run and re-apply the copy workflow because isProcessing is reset in finally; add a stable "already handled" guard (e.g., a useRef or state like processedNewRoomIdRef / handledRoomIds) and check that before calling processCreateArtistResult so the same result.newRoomId is only processed once; inside processCreateArtistResult set that guard (mark result.newRoomId as handled) immediately after a successful copy or when you decide not to copy, and use the guard in the effect's shouldSkip logic (alongside result.newRoomId, id, status) to prevent re-processing the same newRoomId even after isProcessing flips to false. Ensure the guard keys on result.newRoomId (or a unique result identifier) and that refetchConversations/copyMessages are only invoked when the guard indicates the newRoomId hasn't been handled.
🧹 Nitpick comments (2)
hooks/useChatTransport.ts (1)
35-48: ⚡ Quick winDuplicate
getAccessToken()calls within the same request cycle.Both
headers()(line 36) andbody()(line 40) independently callgetAccessToken(). WhenDefaultChatTransportbuilds a request, both callbacks are invoked, resulting in two token fetches per message. Consider fetching once and sharing the result.♻️ Proposed refactor to fetch token once
const transport = useMemo( - () => - new DefaultChatTransport({ + () => { + let cachedTokenPromise: Promise<string | null> | null = null; + const getToken = () => { + if (!cachedTokenPromise) { + cachedTokenPromise = getAccessToken().catch(() => null); + } + return cachedTokenPromise; + }; + + return new DefaultChatTransport({ api: `${baseUrl}/api/chat/workflow`, headers: async (): Promise<Record<string, string>> => { - const accessToken = await getAccessToken().catch(() => null); + const accessToken = await getToken(); return accessToken ? { Authorization: `Bearer ${accessToken}` } : {}; }, body: async () => { - const recoupAccessToken = await getAccessToken().catch(() => null); + const recoupAccessToken = await getToken(); const body: { sessionId: string; chatId: string; recoupAccessToken?: string; } = { sessionId, chatId }; if (recoupAccessToken) body.recoupAccessToken = recoupAccessToken; return body; }, - }), + }); + }, [baseUrl, chatId, sessionId, getAccessToken], );🤖 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/useChatTransport.ts` around lines 35 - 48, The headers() and body() callbacks both call getAccessToken(), causing duplicate token fetches; modify useChatTransport so the token is fetched once per request and shared by both callbacks (e.g., call getAccessToken() once before constructing the headers/body functions or store its Promise in a local variable) and then have headers() and body() reference that shared result; update references in this file for the headers, body, getAccessToken, and DefaultChatTransport request construction so only a single getAccessToken() invocation occurs per message.components/Sidebar/RecentChats/useRecentChats.ts (1)
23-44: ⚡ Quick winDerive
activeChatIdfrom Next navigation state instead of mirroring it in local state.
activeChatIdis fully route-derived here, so the extra state/effect pair pluswindow.location.pathnameadds another sync path and ties sidebar highlighting toconversationsrefreshes. ReadingusePathname()directly keeps this in lockstep with navigation and trims the bookkeeping.♻️ Proposed simplification
-import { useParams } from "next/navigation"; +import { useParams, usePathname } from "next/navigation"; @@ const isMobile = useMobileDetection(); const params = useParams(); + const pathname = usePathname(); @@ - const [activeChatId, setActiveChatId] = useState<string | null>( - typeof params?.chatId === "string" ? params.chatId : null, - ); - - useEffect(() => { - const updateActiveChatId = () => { - const match = window.location.pathname.match( - /\/sessions\/[^/]+\/chats\/([^/]+)/, - ); - - if (match?.[1]) { - setActiveChatId(match[1]); - return; - } - - setActiveChatId( - typeof params?.chatId === "string" ? params.chatId : null, - ); - }; - - updateActiveChatId(); - }, [params, conversations]); + const activeChatId = + pathname?.match(/\/sessions\/[^/]+\/chats\/([^/]+)/)?.[1] ?? + (typeof params?.chatId === "string" ? params.chatId : null);As per coding guidelines, "Write minimal code - use only the absolute minimum code needed".
🤖 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/useRecentChats.ts` around lines 23 - 44, activeChatId is derived from the route so remove the local state and effect (activeChatId, setActiveChatId and the useEffect that reads window.location.pathname) and instead compute it from Next's usePathname() inside useRecentChats; parse the pathname with the same regex (/\/sessions\/[^/]+\/chats\/([^/]+)/) to extract the id, fall back to params?.chatId if not present, and remove the conversations dependency and any window references so the sidebar highlighting follows navigation state directly.
🤖 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.
Outside diff comments:
In `@hooks/useCreateArtistTool.ts`:
- Around line 21-88: The effect can re-run and re-apply the copy workflow
because isProcessing is reset in finally; add a stable "already handled" guard
(e.g., a useRef or state like processedNewRoomIdRef / handledRoomIds) and check
that before calling processCreateArtistResult so the same result.newRoomId is
only processed once; inside processCreateArtistResult set that guard (mark
result.newRoomId as handled) immediately after a successful copy or when you
decide not to copy, and use the guard in the effect's shouldSkip logic
(alongside result.newRoomId, id, status) to prevent re-processing the same
newRoomId even after isProcessing flips to false. Ensure the guard keys on
result.newRoomId (or a unique result identifier) and that
refetchConversations/copyMessages are only invoked when the guard indicates the
newRoomId hasn't been handled.
In `@hooks/useVercelChat.ts`:
- Around line 282-290: The loading and history fetch are being triggered for
route-less chats because useMessageLoader is passed id; change the call and
gating to use the route-specific identifier (routeChatId) instead of id: call
useMessageLoader(sessionId, messages.length === 0 ? routeChatId : undefined,
userId, setMessages) and compute isLoading as isMessagesLoading && !!routeChatId
&& messages.length === 0 so the persisted-history path only runs when a routed
chatId exists; update references to use routeChatId in this block
(isMessagesLoading, isLoading, and the ternary argument).
---
Nitpick comments:
In `@components/Sidebar/RecentChats/useRecentChats.ts`:
- Around line 23-44: activeChatId is derived from the route so remove the local
state and effect (activeChatId, setActiveChatId and the useEffect that reads
window.location.pathname) and instead compute it from Next's usePathname()
inside useRecentChats; parse the pathname with the same regex
(/\/sessions\/[^/]+\/chats\/([^/]+)/) to extract the id, fall back to
params?.chatId if not present, and remove the conversations dependency and any
window references so the sidebar highlighting follows navigation state directly.
In `@hooks/useChatTransport.ts`:
- Around line 35-48: The headers() and body() callbacks both call
getAccessToken(), causing duplicate token fetches; modify useChatTransport so
the token is fetched once per request and shared by both callbacks (e.g., call
getAccessToken() once before constructing the headers/body functions or store
its Promise in a local variable) and then have headers() and body() reference
that shared result; update references in this file for the headers, body,
getAccessToken, and DefaultChatTransport request construction so only a single
getAccessToken() invocation occurs per message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42a932ec-46de-45ea-8784-9ed2893c56c0
⛔ Files ignored due to path filters (1)
lib/chat/__tests__/chatPaths.test.tsis excluded by!**/*.test.*and included bylib/**
📒 Files selected for processing (12)
app/chat/[roomId]/page.tsxcomponents/Header/Artist.tsxcomponents/Sidebar/RecentChats/useRecentChats.tscomponents/VercelChat/chat.tsxhooks/useChatTransport.tshooks/useCreateArtistTool.tshooks/useMessageLoader.tshooks/useVercelChat.tslib/chat/chatPaths.tslib/email/generateTxtFileEmail.tsproviders/OrganizationProvider.tsxproviders/VercelChatProvider.tsx
💤 Files with no reviewable changes (1)
- app/chat/[roomId]/page.tsx
This commit updates the `useCreateArtistTool` hook to explicitly cast `result.newRoomId` to a string when generating the chat path. This change prevents potential type-related issues and ensures consistent URL formatting. Additionally, it adds an import statement for testing utilities in the chat paths test file, enhancing test coverage and maintainability.
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Resolve Artist.tsx: keep test hard-nav behavior, use isActiveChatRoomPath.
…e with window.location.pathname This commit removes the usePathname hook from both Artist.tsx and OrganizationProvider.tsx, replacing it with direct access to window.location.pathname. This change simplifies the code and maintains the intended navigation behavior without relying on Next.js's router for pathname management.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
1 participant
This commit removes the InstantChatRoom component and updates various hooks and components to replace references of roomId with chatId. The changes ensure that chat functionality is aligned with the new session-scoped architecture, improving consistency across the chat system. Additionally, the sessionId is now required in several components to streamline the chat transport process.
Summary by cubic
Refactored chat to use session-scoped
chatIdroutes, requiresessionIdacross the stack, and remove the legacy transport. Centralized URL helpers for navigation and emails; kept hard-nav behavior in Artist and fixed new-chat URL handling.Refactors
app/chat/[roomId]/page.tsx; replacedroomIdwithchatIdacross hooks/components; sidebar derives the active chat from/sessions/:sessionId/chats/:chatId.sessionIdis now required inChat,useVercelChat,useChatTransport,useMessageLoader, andVercelChatProvider; context exposessessionId.POST /api/chat/workflow, injectingsessionId,chatId, and the bearer token; legacy/api/chatpath removed.lib/chat/chatPaths(getChatPath,getChatUrl,isActiveChatRoomPath) with tests; used for URL updates, email CTAs, and route guards. ReplacedusePathnamewithwindow.location.pathnameand hard-nav viawindow.location.hrefinHeader/ArtistandOrganizationProviderto avoid Next router desync; Recent Chats parsing updated for session-scoped paths.newRoomIdtostringinuseCreateArtistTool.Migration
sessionIdandchatIdtoVercelChatProviderandChat.roomIdparams/routes tochatId; use/sessions/[sessionId]/chats/[chatId].getChatPath/getChatUrlfor links and history updates; remove any dependency on the legacy/api/chattransport.generateTxtFileEmailcalls to passsessionIdalong with the conversation id.Written for commit 7233350. Summary will update on new commits.
Summary by CodeRabbit