fix(chat): delete trailing messages before regenerate using transportChatId#1787
fix(chat): delete trailing messages before regenerate using transportChatId#1787pradipthaadhi wants to merge 16 commits into
Conversation
#1765) * refactor(chat): update chat components to use chatId instead of roomId 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. * refactor(chat): integrate sessionId and update chat path utilities 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. * fix(chat): ensure newRoomId is treated as a string in URL handling 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. * refactor(Header, OrganizationProvider): remove usePathname and replace 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. * refactor(chat): split chatPaths per SRP, drop dead legacy branch, simplify useParams - Remove dead `/chat/` branch from isActiveChatRoomPath — this PR makes /chat/{id} 404, so treating it as an active chat room contradicted the PR's own intent and could never fire in production. - Split lib/chat/chatPaths.ts into one-function-per-file (getChatPath, getChatUrl, isActiveChatRoomPath) per repo SRP convention; mirror tests. - Simplify useVercelChat to `const { chatId } = useParams<{ chatId?: string }>()` (KISS), consistent with chat.tsx. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Sweets Sweetman <sweetmantech@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#1767) (#1774) * feat(chat): defer new-chat bootstrap wait from spinner to send button (#1767) New chats landing on `/` or `/chat` blocked the whole view on a full-screen spinner while `POST /api/sessions` + `POST /api/sandbox` resolved (~12s cold, dominated by sandbox provisioning). Render `<Chat>` immediately with a typeable input instead and provision in parallel; the Send button stays disabled with a "Preparing your workspace…" cue until the api-minted `sessionId` + `chatId` land, so the workflow transport never fires without the ids its validator requires. - NewChatBootstrap: render <Chat> eagerly with a stable placeholder chat id (survives the preparing → ready transition); drop the spinner. - Thread `sessionId?`, `workflowChatId`, `isBootstrapPreparing` through Chat → VercelChatProvider → useVercelChat (provider kept inline). - useVercelChat: `transportChatId = workflowChatId ?? id` drives the transport, optimistic conversation, and URL; only load persisted history on a canonical route so no spinner flashes over the input during the ready transition. - ChatInput: gate Send on `isBootstrapPreparing`; input stays typeable. - useChatTransport / useMessageLoader: sessionId optional + guard. - useCreateArtistTool: guard the one context sessionId consumer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(chat): send live sessionId/chatId from workflow transport (#1767) The new-chat flow mounts <Chat> (and useChat) during provisioning with sessionId undefined + a placeholder chatId. useChat captures the transport from that mount render and does not swap to the rebuilt instance when the ids later resolve — so the first send POSTed `sessionId: undefined` to /api/chat/workflow and got a 400 (the URL rewrote correctly because it reads the live prop, not the transport). Read the ids from refs inside transport.body() so a single stable transport instance always sends the values current as of the request, regardless of useChat's stale capture. Removed chatId/sessionId from the memo deps since rebuilding the instance never helped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(chat): replace preparing-text cue with a workspace status dot (#1767) Swap the flashing "Preparing your workspace…" toolbar text for a stoplight status dot in the top-right of the chat input: red = off (provisioning failed / unavailable), yellow = provisioning, green = ready. Hover shows context via the shadcn tooltip. - New `WorkspaceStatusIndicator` component in its own file (SRP) — a pure display component driven by a `status` prop, built on the existing shadcn-based `common/Tooltip`, so it's open for reuse without change. - Thread `workspaceStatus: "off" | "provisioning" | "ready"` through Chat → VercelChatProvider → context (replacing the `isBootstrapPreparing` boolean); Send is gated while it's not `ready`. - NewChatBootstrap now derives the status and renders `<Chat>` even on a provisioning error (red dot + disabled Send, input still visible) instead of swapping in a full-screen error view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(chat): simplify provisioning tooltip copy (#1767) Shorten the yellow/provisioning workspace-status tooltip to "Preparing your workspace". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(chat): drop legacy GET /api/chats/{id}/artist caller (#1767)
* fix(chat): reactive conversations cache + user-scoped session query
* refactor(chat): drop dead /chat/:id artist cache fallback (post-#1765)
The conversations-cache fallback in useArtistFromChat only existed for the
legacy /chat/:id deep link (chatId but no sessionId). That route was
removed in #1765 — every chat is now reached via the canonical
/sessions/{sessionId}/chats/{chatId} URL, so sessionId is always present.
Reduce to the session path: GET /api/sessions/{sessionId} -> session.artistId
-> select. Removes useSyncExternalStore reactive-cache machinery,
findArtistIdInConversationsCache (+ its test), and the unused chatId param.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(chat): validate getSessionById response with zod (PR #1768 review)
Replace type-casting with boundary zod validation, matching the
getConversations convention. Drops the `as GetSessionByIdResponse`/
`as GetSessionByIdErrorResponse` casts in favor of safeParse/parse.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(chat): extract shared safeJsonParse into lib/api (PR #1768 review)
Replace the locally-defined safeJsonParse in getSessionById with a
shared helper in lib/api/, alongside the other HTTP helpers. DRY/SRP.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Sweets Sweetman <sweetmantech@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… and hooks This commit updates the MessageEditor component and the useVercelChat hook to utilize transportChatId instead of the previous chatId. This change enhances the consistency of chat-related API calls and aligns with the new session-scoped architecture, ensuring that the correct chat identifier is used for operations like deleting trailing messages.
#1781) * fix(chat): persist selected model before send so the workflow bills it The chat-workflow path bills the model it reads from chats.model_id at request time. The picker selection was never written there, so every new chat (and any model change) billed the chats.model_id default instead of the chosen model. Before sending, await a PATCH of the selected model to chats.model_id when it changed for the active chat (shouldPersistChatModel guards redundant writes). The chat row already exists from new-chat bootstrap, so this covers the first turn race-free without an api change. Reuses buildPatchChatBody + updateChat(modelId) (originally from #1779). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(chat): extract model-persist-before-send into usePersistSelectedModel Addresses OCP/SRP review feedback on #1781: instead of inlining the persist-before-send block in useVercelChat, encapsulate it in a dedicated usePersistSelectedModel hook (owns the lastPersisted ref + guard + PATCH). useVercelChat now just calls `await persistSelectedModel()` before send. Behavior unchanged; pure decision logic still covered by shouldPersistChatModel tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(chat): persist selected model on retry/edit + append paths Addresses review feedback on #1781: - Retry/Edit call reload() → regenerate (and append() calls sendMessage) bypassing handleSubmit, so the selected model was not persisted before those sends — the regenerated turn could bill the previous/default model. Now await persistSelectedModel() in handleReload and append too. - Clarify updateChat JSDoc: it patches title and/or modelId (not just rename). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Step 3 of the /api/chat/workflow → /api/chat rename (chat#1767), against
the merged docs contract (docs#235) and the api endpoint (api#645, on test).
useChatTransport now POSTs to ${baseUrl}/api/chat instead of
/api/chat/workflow. Comment-only refs in VercelChatProvider updated too.
⚠️ Deploy coordination: requires api#645 (which removes /api/chat/workflow)
to be live in the same environment. Must deploy together with the api
promotion and the open-agents repoint (step 4), or chat 404s.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ete-transport-chat-id
…e editor and hooks" This reverts commit 9f34b11.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dd3f14a9d
ℹ️ 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".
| await deleteTrailingMessagesApi({ | ||
| chatId: transportChatId, | ||
| fromMessageId: lastMessage.id, | ||
| accessToken, |
There was a problem hiding this comment.
Avoid deleting again after edited-user retries
When a user edits an earlier message, MessageEditor has already called deleteTrailingMessages from that user message before invoking reload() in the same onSuccess callback. Because this hook's messages value is still from the pre-edit render at that point, lastMessage can still be the old assistant response, so this added delete attempts to remove a message that was just deleted and returns before regenerate() if the API reports it as missing. This makes edit-and-resubmit fail in the edited-message path rather than regenerating from the updated user message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 3/5
- There is a concrete regression risk in
hooks/useVercelChat.ts: if the delete API call fails, thecatchreturns early andregenerate()does not run, which can break the edit-and-resubmit flow. - Given the medium severity (6/10) and solid confidence (7/10), this is more than a cosmetic issue and can affect user-visible behavior, so the merge risk is moderate.
- Pay close attention to
hooks/useVercelChat.ts- ensure delete failures do not blockregenerate()in the reload/resubmit path.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…t-id Revert "refactor(chat): replace chatId with transportChatId in message editor and hooks"
1dd3f14 to
9a9bdc7
Compare
9a9bdc7 to
057674d
Compare
|
Warning Review limit reached
More reviews will be available in 48 minutes and 3 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 selected for processing (5)
📝 WalkthroughWalkthroughThe PR threads ChangesTransport Chat ID Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
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)
hooks/useVercelChat.ts (1)
305-342: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRefactor to use existing
useDeleteTrailingMessageshook instead of direct API call.This implementation duplicates the logic from
useDeleteTrailingMessages(lines 230-243), violating the DRY principle. The hook already handles access token retrieval, API invocation, and crucially, local state updates viaonSuccesscallback.Current issues:
- Code duplication - Access token logic and API call pattern are reimplemented
- State management gap - After the delete API succeeds (line 321), the local
messagesarray still contains the deleted assistant message untilregenerate()completes. The hook'sonSuccessproperly cleans state first.- Inconsistent pattern -
handleSendMessage(line 377) uses the hook, buthandleReloadbypasses it- Performance concern - Including
messagesin the dependency array (line 340) recreates this callback on every message change♻️ Proposed refactor using the existing hook
const handleReload = useCallback(async () => { const headers = await getHeaders(); await persistSelectedModel(); - - const lastMessage = messages.at(-1); - if (lastMessage?.role === "assistant") { - const accessToken = await getAccessToken(); - if (!accessToken) { - toast.error("Please sign in to regenerate."); - return; - } - - try { - await deleteTrailingMessagesApi({ - chatId: transportChatId, - fromMessageId: lastMessage.id, - accessToken, - }); - } catch (error) { - console.error("Failed to delete trailing messages before regenerate:", error); - toast.error("Failed to regenerate. Please try again."); - return; - } - } - + + const lastMessage = messages.at(-1); + if (lastMessage?.role === "assistant") { + try { + await deleteTrailingMessages({ + chatId: transportChatId, + fromMessageId: lastMessage.id, + }); + } catch (error) { + console.error("Failed to delete trailing messages before regenerate:", error); + toast.error("Failed to regenerate. Please try again."); + return; + } + } + await regenerate({ body: chatRequestBody, headers }); }, [ getHeaders, - getAccessToken, + deleteTrailingMessages, regenerate, chatRequestBody, persistSelectedModel, - messages, transportChatId, ]);Note: Removed
messagesfrom dependency array since it would cause unnecessary callback recreation. The latestmessageswill be captured at call time.As per coding guidelines, write minimal code and avoid unnecessary abstractions during implementation; the existing hook should be leveraged instead of duplicating its 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 `@hooks/useVercelChat.ts` around lines 305 - 342, Refactor handleReload to call the existing useDeleteTrailingMessages hook instead of calling deleteTrailingMessagesApi directly: keep the persistSelectedModel and headers logic (getHeaders, persistSelectedModel, regenerate) but replace the accessToken/API call block with the hook's mutate function (from useDeleteTrailingMessages) so the hook can obtain the token, call the API, and run its onSuccess to update local messages before calling regenerate; remove messages from the handleReload dependency array and ensure you pass transportChatId and lastMessage.id (or fromMessageId) to the hook's mutate call so state is cleaned first and the regenerate({ body: chatRequestBody, headers }) call happens after the hook signals success.Source: Coding guidelines
🤖 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/useVercelChat.ts`:
- Around line 305-342: Refactor handleReload to call the existing
useDeleteTrailingMessages hook instead of calling deleteTrailingMessagesApi
directly: keep the persistSelectedModel and headers logic (getHeaders,
persistSelectedModel, regenerate) but replace the accessToken/API call block
with the hook's mutate function (from useDeleteTrailingMessages) so the hook can
obtain the token, call the API, and run its onSuccess to update local messages
before calling regenerate; remove messages from the handleReload dependency
array and ensure you pass transportChatId and lastMessage.id (or fromMessageId)
to the hook's mutate call so state is cleaned first and the regenerate({ body:
chatRequestBody, headers }) call happens after the hook signals success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 332d97fb-87f1-4735-828c-9a8dfaa541ad
📒 Files selected for processing (3)
components/VercelChat/message-editor.tsxhooks/useVercelChat.tsproviders/VercelChatProvider.tsx
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Requires human review: This change modifies the core chat regeneration logic by adding conditional server-side trailing message deletion and introducing a new API error class, which carries significant risk of message loss or unexpected behavior and requires careful human review.
Re-trigger cubic
Summary by CodeRabbit