feat: migrate trailing message deletes to dedicated endpoint#1620
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes the Next.js DELETE route handler for deleting trailing messages and refactors the deletion flow from Supabase-based to a client-API-backed approach. It introduces a new Changes
Sequence DiagramsequenceDiagram
participant Editor as Message Editor
participant Hook as useDeleteTrailingMessages
participant Privy as Privy Auth
participant API as Client API
participant Chat as Chat State
Editor->>Hook: deleteTrailingMessages({ chatId, fromMessageId })
Hook->>Privy: getAccessToken()
Privy-->>Hook: accessToken
Hook->>API: DELETE /endpoint<br/>{ chatId, fromMessageId, accessToken }
API-->>Hook: success / error
alt Success
Hook->>Chat: onSuccess callback
Chat->>Chat: setMessages (remove from messageId)
Chat->>Chat: setMode("view")
Chat->>Chat: reload()
else Error
Hook-->>Editor: throw error
end
Hook-->>Editor: isDeletingTrailingMessages state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b3dc8792
ℹ️ 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".
| * @param id The ID of the memory to delete messages after | ||
| * @returns A promise that resolves when the operation is complete | ||
| */ | ||
| export async function clientDeleteTrailingMessages({ |
There was a problem hiding this comment.
Removing clientDeleteTrailingMessages here breaks the build because components/VercelChat/message-editor.tsx still imports @/lib/messages/clientDeleteTrailingMessages in this same commit. That leaves an unresolved module reference in the chat UI path, so the editor flow cannot compile/run until the caller is updated to the new API helper.
Useful? React with 👍 / 👎.
| if (earliestFailedUserMessageId) { | ||
| const successfulDeletion = await clientDeleteTrailingMessages({ | ||
| id: earliestFailedUserMessageId, | ||
| if (earliestFailedUserMessageId && accessToken) { |
There was a problem hiding this comment.
handleDeleteTrailingMessages now gates cleanup on accessToken, but useAccessToken is async and can be null during early retries. In that state this function skips deletion and still clears hasChatApiError, so the failed trailing message remains in local state and the next resend proceeds with an inconsistent transcript instead of the intended cleanup-first retry behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/VercelChat/message-editor.tsx (1)
40-73:⚠️ Potential issue | 🟠 MajorBlock submit when trailing-delete preconditions fail, and handle delete errors explicitly.
At Line 42, the delete step is skipped when
id/accessTokenis unavailable, but the function still updates local messages and exits edit mode. Also, a thrown network/parsing error from the awaited delete call will break the flow mid-submit.💡 Proposed fix
const handleSubmit = useCallback(async () => { setIsSubmitting(true); - - if (id && accessToken) { - await deleteTrailingMessages({ - chatId: id, - fromMessageId: message.id, - accessToken, - }); - } + try { + if (!id || !accessToken) return; + const deleted = await deleteTrailingMessages({ + chatId: id, + fromMessageId: message.id, + accessToken, + }); + if (!deleted) return; - // `@ts-expect-error` todo: support UIMessage in setMessages - setMessages((messages) => { - const index = messages.findIndex((m) => m.id === message.id); + // `@ts-expect-error` todo: support UIMessage in setMessages + setMessages((messages) => { + const index = messages.findIndex((m) => m.id === message.id); - if (index !== -1) { - const updatedMessage = { - ...message, - content: draftContent, - parts: [{ type: "text", text: draftContent }], - }; + if (index !== -1) { + const updatedMessage = { + ...message, + content: draftContent, + parts: [{ type: "text", text: draftContent }], + }; - return [...messages.slice(0, index), updatedMessage]; - } + return [...messages.slice(0, index), updatedMessage]; + } - return messages; - }); + return messages; + }); - setMode("view"); + setMode("view"); - // Only reload if the content has actually changed - if (text !== draftContent) { - reload(); + if (text !== draftContent) { + reload(); + } + } finally { + setIsSubmitting(false); } }, [id, accessToken, message.id, text, draftContent, setMessages, setMode, reload]);As per coding guidelines, "Implement built-in security practices for authentication and data handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/VercelChat/message-editor.tsx` around lines 40 - 73, The submit flow should not proceed when delete preconditions are missing and must handle errors from deleteTrailingMessages; update the function so that before mutating local state (setMessages) or switching mode (setMode) you verify id and accessToken and if either is missing, abort by setting setIsSubmitting(false) and returning early, and wrap the await deleteTrailingMessages(...) call in a try/catch to catch network/parsing errors—on catch, call setIsSubmitting(false), surface/log the error, and return without updating messages or calling setMode("view"); keep reload() behavior only after successful delete and state updates. Ensure you reference the existing symbols: deleteTrailingMessages, setIsSubmitting, setMessages, setMode, reload, id, accessToken, and message.id.hooks/useVercelChat.ts (1)
262-297:⚠️ Potential issue | 🟠 MajorOnly clear chat API error after successful trailing cleanup, and abort resend otherwise.
At Line 285,
hasChatApiErroris reset even when deletion is skipped or fails. Then Line 296 continues send flow regardless. This can preserve corrupted trailing state and retry on top of it.💡 Proposed fix
- const handleDeleteTrailingMessages = async () => { + const handleDeleteTrailingMessages = async (): Promise<boolean> => { const earliestFailedUserMessageId = getEarliestFailedUserMessageId(messages); - if (earliestFailedUserMessageId && accessToken) { - const successfulDeletion = await deleteTrailingMessages({ - chatId: id, - fromMessageId: earliestFailedUserMessageId, - accessToken, - }); - if (successfulDeletion) { - setMessages((messages) => { - const index = messages.findIndex( - (m) => m.id === earliestFailedUserMessageId, - ); - if (index !== -1) { - return [...messages.slice(0, index)]; - } - - return messages; - }); - } - } - - setHasChatApiError(false); + if (!earliestFailedUserMessageId || !accessToken) return false; + const successfulDeletion = await deleteTrailingMessages({ + chatId: id, + fromMessageId: earliestFailedUserMessageId, + accessToken, + }); + if (!successfulDeletion) return false; + setMessages((messages) => { + const index = messages.findIndex( + (m) => m.id === earliestFailedUserMessageId, + ); + if (index !== -1) { + return [...messages.slice(0, index)]; + } + return messages; + }); + setHasChatApiError(false); + return true; }; @@ if (hasChatApiError) { - await handleDeleteTrailingMessages(); + const cleaned = await handleDeleteTrailingMessages(); + if (!cleaned) return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 262 - 297, The send flow currently clears hasChatApiError unconditionally and proceeds even if trailing-message deletion was skipped or failed; modify handleSendMessage to call handleDeleteTrailingMessages and only clear setHasChatApiError(false) and continue when deletion actually succeeded (i.e., when deleteTrailingMessages returned success via handleDeleteTrailingMessages). If deletion was skipped or unsuccessful, abort the send (return early) so you don't retry on top of corrupted trailing state; use the existing helpers getEarliestFailedUserMessageId, deleteTrailingMessages, setMessages and check the successfulDeletion result before calling setHasChatApiError(false) or proceeding with the rest of handleSendMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/api/getClientApiBaseUrl.ts`:
- Around line 13-21: The current logic in getClientApiBaseUrl (reading
API_OVERRIDE_STORAGE_KEY from window.sessionStorage and accepting any valid URL)
allows untrusted overrides; change it to parse the override with new
URL(override), then validate that the parsed URL.origin is in a trusted
allowlist (e.g., compare against a constant array of allowed origins or require
same-origin/secure protocol) and only return the override if it matches the
allowlist and uses https; otherwise fall back to NEW_API_BASE_URL. Ensure you
reference API_OVERRIDE_STORAGE_KEY, NEW_API_BASE_URL and the override
parsing/try-catch block when implementing the check.
In `@lib/messages/deleteTrailingMessages.ts`:
- Around line 15-29: The code assumes fetch always succeeds and response.json()
always returns JSON; wrap the network call and response parsing in a try/catch
so network errors and non-JSON responses don't throw but instead log and return
false. Specifically, in deleteTrailingMessages (the block using
getClientApiBaseUrl(), accessToken, chatId, fromMessageId and checking
response.ok) surround the fetch and subsequent response handling with try/catch,
and when !response.ok attempt to parse JSON but fall back to response.text() if
JSON parsing fails, log the parsed error/text with context, and return false;
also catch any fetch/parsing exception, log it, and return false to preserve the
Promise<boolean> contract.
---
Outside diff comments:
In `@components/VercelChat/message-editor.tsx`:
- Around line 40-73: The submit flow should not proceed when delete
preconditions are missing and must handle errors from deleteTrailingMessages;
update the function so that before mutating local state (setMessages) or
switching mode (setMode) you verify id and accessToken and if either is missing,
abort by setting setIsSubmitting(false) and returning early, and wrap the await
deleteTrailingMessages(...) call in a try/catch to catch network/parsing
errors—on catch, call setIsSubmitting(false), surface/log the error, and return
without updating messages or calling setMode("view"); keep reload() behavior
only after successful delete and state updates. Ensure you reference the
existing symbols: deleteTrailingMessages, setIsSubmitting, setMessages, setMode,
reload, id, accessToken, and message.id.
In `@hooks/useVercelChat.ts`:
- Around line 262-297: The send flow currently clears hasChatApiError
unconditionally and proceeds even if trailing-message deletion was skipped or
failed; modify handleSendMessage to call handleDeleteTrailingMessages and only
clear setHasChatApiError(false) and continue when deletion actually succeeded
(i.e., when deleteTrailingMessages returned success via
handleDeleteTrailingMessages). If deletion was skipped or unsuccessful, abort
the send (return early) so you don't retry on top of corrupted trailing state;
use the existing helpers getEarliestFailedUserMessageId, deleteTrailingMessages,
setMessages and check the successfulDeletion result before calling
setHasChatApiError(false) or proceeding with the rest of handleSendMessage.
🪄 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: 3719dc8d-44e3-44a4-8a20-be06dd4ab401
📒 Files selected for processing (8)
app/api/memories/delete-trailing/route.tscomponents/VercelChat/message-editor.tsxhooks/useVercelChat.tslib/api/getClientApiBaseUrl.tslib/messages/clientDeleteTrailingMessages.tslib/messages/deleteTrailingMessages.tslib/supabase/deleteMemoriesByChatIdAfterTimestamp.tslib/supabase/getMemoryById.ts
💤 Files with no reviewable changes (4)
- lib/messages/clientDeleteTrailingMessages.ts
- app/api/memories/delete-trailing/route.ts
- lib/supabase/deleteMemoriesByChatIdAfterTimestamp.ts
- lib/supabase/getMemoryById.ts
| const override = window.sessionStorage.getItem(API_OVERRIDE_STORAGE_KEY); | ||
| if (!override) { | ||
| return NEW_API_BASE_URL; | ||
| } | ||
|
|
||
| try { | ||
| new URL(override); | ||
| return override; | ||
| } catch { |
There was a problem hiding this comment.
Restrict API override to trusted origins to prevent bearer-token exfiltration.
Line 13 accepts any valid URL from sessionStorage. Since authenticated delete calls use this base URL, a malicious override can redirect Authorization headers off-platform.
🔐 Proposed fix
const API_OVERRIDE_STORAGE_KEY = "recoup_api_override";
+const ALLOWED_OVERRIDE_HOSTS = new Set([
+ "recoup-api.vercel.app",
+ "test-recoup-api.vercel.app",
+]);
export function getClientApiBaseUrl(): string {
if (typeof window === "undefined") {
return NEW_API_BASE_URL;
}
@@
try {
- new URL(override);
- return override;
+ const parsed = new URL(override);
+ if (
+ parsed.protocol === "https:" &&
+ ALLOWED_OVERRIDE_HOSTS.has(parsed.hostname)
+ ) {
+ return parsed.origin;
+ }
+ return NEW_API_BASE_URL;
} catch {
return NEW_API_BASE_URL;
}
}As per coding guidelines, "Implement built-in security practices for authentication and data handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/api/getClientApiBaseUrl.ts` around lines 13 - 21, The current logic in
getClientApiBaseUrl (reading API_OVERRIDE_STORAGE_KEY from window.sessionStorage
and accepting any valid URL) allows untrusted overrides; change it to parse the
override with new URL(override), then validate that the parsed URL.origin is in
a trusted allowlist (e.g., compare against a constant array of allowed origins
or require same-origin/secure protocol) and only return the override if it
matches the allowlist and uses https; otherwise fall back to NEW_API_BASE_URL.
Ensure you reference API_OVERRIDE_STORAGE_KEY, NEW_API_BASE_URL and the override
parsing/try-catch block when implementing the check.
| const url = getClientApiBaseUrl(); | ||
| const response = await fetch( | ||
| `${url}/api/chats/${encodeURIComponent(chatId)}/messages/trailing?from_message_id=${encodeURIComponent(fromMessageId)}`, | ||
| { | ||
| method: "DELETE", | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (!memory) { | ||
| throw new Error("Memory not found"); | ||
| if (!response.ok) { | ||
| const errorData = await response.json(); | ||
| console.error(errorData.error || "Failed to delete trailing messages"); | ||
| return false; |
There was a problem hiding this comment.
Preserve the Promise<boolean> contract on network/non-JSON failures.
Line 27 assumes error responses are JSON, and fetch failures are uncaught. Both cases can throw instead of returning false.
🛠️ Proposed fix
export async function deleteTrailingMessages({
@@
}): Promise<boolean> {
- const url = getClientApiBaseUrl();
- const response = await fetch(
- `${url}/api/chats/${encodeURIComponent(chatId)}/messages/trailing?from_message_id=${encodeURIComponent(fromMessageId)}`,
- {
- method: "DELETE",
- headers: {
- Authorization: `Bearer ${accessToken}`,
- },
- },
- );
-
- if (!response.ok) {
- const errorData = await response.json();
- console.error(errorData.error || "Failed to delete trailing messages");
- return false;
- }
-
- return true;
+ try {
+ const url = getClientApiBaseUrl();
+ const response = await fetch(
+ `${url}/api/chats/${encodeURIComponent(chatId)}/messages/trailing?from_message_id=${encodeURIComponent(fromMessageId)}`,
+ {
+ method: "DELETE",
+ headers: {
+ Authorization: `Bearer ${accessToken}`,
+ },
+ },
+ );
+
+ if (!response.ok) {
+ let message = "Failed to delete trailing messages";
+ try {
+ const errorData = (await response.json()) as { error?: string };
+ if (errorData.error) message = errorData.error;
+ } catch {
+ // ignore non-JSON error body
+ }
+ console.error(message);
+ return false;
+ }
+
+ return true;
+ } catch (error) {
+ console.error("Failed to delete trailing messages", error);
+ return false;
+ }
}📝 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.
| const url = getClientApiBaseUrl(); | |
| const response = await fetch( | |
| `${url}/api/chats/${encodeURIComponent(chatId)}/messages/trailing?from_message_id=${encodeURIComponent(fromMessageId)}`, | |
| { | |
| method: "DELETE", | |
| headers: { | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| }, | |
| ); | |
| if (!memory) { | |
| throw new Error("Memory not found"); | |
| if (!response.ok) { | |
| const errorData = await response.json(); | |
| console.error(errorData.error || "Failed to delete trailing messages"); | |
| return false; | |
| export async function deleteTrailingMessages({ | |
| chatId, | |
| fromMessageId, | |
| accessToken, | |
| }: DeleteTrailingMessagesParams): Promise<boolean> { | |
| try { | |
| const url = getClientApiBaseUrl(); | |
| const response = await fetch( | |
| `${url}/api/chats/${encodeURIComponent(chatId)}/messages/trailing?from_message_id=${encodeURIComponent(fromMessageId)}`, | |
| { | |
| method: "DELETE", | |
| headers: { | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| }, | |
| ); | |
| if (!response.ok) { | |
| let message = "Failed to delete trailing messages"; | |
| try { | |
| const errorData = (await response.json()) as { error?: string }; | |
| if (errorData.error) message = errorData.error; | |
| } catch { | |
| // ignore non-JSON error body | |
| } | |
| console.error(message); | |
| return false; | |
| } | |
| return true; | |
| } catch (error) { | |
| console.error("Failed to delete trailing messages", error); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/messages/deleteTrailingMessages.ts` around lines 15 - 29, The code
assumes fetch always succeeds and response.json() always returns JSON; wrap the
network call and response parsing in a try/catch so network errors and non-JSON
responses don't throw but instead log and return false. Specifically, in
deleteTrailingMessages (the block using getClientApiBaseUrl(), accessToken,
chatId, fromMessageId and checking response.ok) surround the fetch and
subsequent response handling with try/catch, and when !response.ok attempt to
parse JSON but fall back to response.text() if JSON parsing fails, log the
parsed error/text with context, and return false; also catch any fetch/parsing
exception, log it, and return false to preserve the Promise<boolean> contract.
| if (id && accessToken) { | ||
| await deleteTrailingMessages({ | ||
| chatId: id, | ||
| fromMessageId: message.id, | ||
| accessToken, | ||
| }); | ||
| } |
There was a problem hiding this comment.
nit: SRP / OCP - move handleSubmit + access token usage to a new hook for useDeleteTrailingMessages
| export function MessageEditor({ message, setMode }: EditingMessageProps) { | ||
| const { setMessages, reload } = useVercelChatContext(); | ||
| const { id, setMessages, reload } = useVercelChatContext(); | ||
| const accessToken = useAccessToken(); |
There was a problem hiding this comment.
This hook is poorly written. If a session exceeds the accessToken timeout, the customer receives an error.
I suggest replacing this with a direct call to privy SDK getAccessToken so the access token is fresh.
There was a problem hiding this comment.
I wrote the poorly written hook btw. I take accountability for the bad code and will remove this hook today.
There was a problem hiding this comment.
For now, can you remove it from your code so it's one less place I need to update?
There was a problem hiding this comment.
DRY - How could this function also be used in the existing override cases as well to reduce maintenance of this api override over time?
There was a problem hiding this comment.
instead of passing the baseUrl to lib functions, each lib can use this function directly to simplify getting the override + api server URL
There was a problem hiding this comment.
I agree. Did you already make that update so existing api overrides use this lib?
There was a problem hiding this comment.
I haven't made that update yet. Adding it to backlog for next PR
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hooks/useDeleteTrailingMessages.ts (1)
39-40: Remove@ts-expect-errorfrom the mutation state-update path.Please replace the suppression with explicit typing (e.g., typed updater + typed
updatedMessage) so future SDK/type changes fail loudly at compile time.As per coding guidelines,
**/*.{ts,tsx}requires “Use strict TypeScript types with zero 'any' types”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useDeleteTrailingMessages.ts` around lines 39 - 40, Remove the `@ts-expect-error` and give the mutation updater explicit types: type the updater passed to setMessages as (prev: YourMessageType[]) => YourMessageType[] (replace YourMessageType with the concrete message type used across the hook, e.g., Message | UIMessage union) and explicitly type the local updatedMessage variable to that same type before using it in the updater; update any imports or type aliases (e.g., UIMessage) so the compiler validates the state update path instead of suppressing errors. Ensure setMessages' callback signature and updatedMessage share the identical concrete type so future SDK/type changes fail at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/VercelChat/message-editor.tsx`:
- Around line 72-74: The current onClick uses a fire-and-forget call to
deleteTrailingMessages(), which swallows rejections; update the caller to handle
errors (e.g., await deleteTrailingMessages() and .catch to surface/log failures)
or add an onError handler to the mutation configuration for
deleteTrailingMessages to handle authentication/network errors; reference the
deleteTrailingMessages mutation and the onClick handler in message-editor.tsx
and ensure any error is logged/shown to the user or triggers appropriate
fallback behavior.
In `@hooks/useDeleteTrailingMessages.ts`:
- Around line 38-55: The current onSettled handler mutates local state via
setMessages (using message and draftContent) regardless of whether the delete
succeeded, causing messages to appear truncated even on failed deletions; move
the truncation logic into the successful path (e.g., the onSuccess callback) and
remove it from onSettled so setMessages only updates when the backend delete
confirms success; update both occurrences referencing setMessages, onSettled,
message, and draftContent accordingly and ensure onError leaves state unchanged
or restores it if needed.
---
Nitpick comments:
In `@hooks/useDeleteTrailingMessages.ts`:
- Around line 39-40: Remove the `@ts-expect-error` and give the mutation updater
explicit types: type the updater passed to setMessages as (prev:
YourMessageType[]) => YourMessageType[] (replace YourMessageType with the
concrete message type used across the hook, e.g., Message | UIMessage union) and
explicitly type the local updatedMessage variable to that same type before using
it in the updater; update any imports or type aliases (e.g., UIMessage) so the
compiler validates the state update path instead of suppressing errors. Ensure
setMessages' callback signature and updatedMessage share the identical concrete
type so future SDK/type changes fail at compile time.
🪄 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: 1b8817da-cae3-4893-b617-d159e46e84ba
📒 Files selected for processing (2)
components/VercelChat/message-editor.tsxhooks/useDeleteTrailingMessages.ts
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)
261-285:⚠️ Potential issue | 🟡 MinorConsider logging or surfacing skipped deletion when
accessTokenis unavailable.The guard on line 264 skips the deletion if
accessTokenis falsy, butsetHasChatApiError(false)executes unconditionally on line 284. This means an API error could be cleared without actually performing cleanup, potentially leaving orphaned messages in the chat state.If the user later retries, they may encounter the same failure since the problematic messages weren't pruned. Consider either:
- Only clearing the error flag when deletion actually succeeds or when no deletion is needed (
!earliestFailedUserMessageId).- Adding a debug log when deletion is skipped due to missing token.
🛡️ Proposed fix to conditionally clear error flag
const handleDeleteTrailingMessages = async () => { const earliestFailedUserMessageId = getEarliestFailedUserMessageId(messages); + + // No failed messages to clean up — clear error and exit + if (!earliestFailedUserMessageId) { + setHasChatApiError(false); + return; + } + + // Cannot delete without authentication — retain error state + if (!accessToken) { + console.warn("Skipping trailing message deletion: no access token"); + return; + } + - if (earliestFailedUserMessageId && accessToken) { - const successfulDeletion = await deleteTrailingMessages({ - chatId: id, - fromMessageId: earliestFailedUserMessageId, - accessToken, - }); - if (successfulDeletion) { - setMessages((messages) => { - const index = messages.findIndex( - (m) => m.id === earliestFailedUserMessageId, - ); - if (index !== -1) { - return [...messages.slice(0, index)]; - } - - return messages; - }); - } + const successfulDeletion = await deleteTrailingMessages({ + chatId: id, + fromMessageId: earliestFailedUserMessageId, + accessToken, + }); + + if (successfulDeletion) { + setMessages((messages) => { + const index = messages.findIndex( + (m) => m.id === earliestFailedUserMessageId, + ); + if (index !== -1) { + return [...messages.slice(0, index)]; + } + return messages; + }); + setHasChatApiError(false); } - - setHasChatApiError(false); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 261 - 285, The handleDeleteTrailingMessages function clears the chat error unconditionally even when deletion is skipped due to a missing accessToken; change the flow so setHasChatApiError(false) is only called when no deletion is needed (earliestFailedUserMessageId is falsy) or when deleteTrailingMessages returned true, and otherwise either log/debug that deletion was skipped because accessToken is missing or leave the error flag set; update the guard around getEarliestFailedUserMessageId / deleteTrailingMessages and adjust the setMessages branch accordingly to ensure orphaned messages aren’t left while clearing the error only on actual success.
🧹 Nitpick comments (1)
hooks/useChatTransport.ts (1)
7-29: Clean approach to dynamic token injection — the per-request token fetch ensures freshness and handles refresh scenarios gracefully.Two observations:
The
.catch(() => null)silently permits unauthenticated requests if token fetching fails. This is intentional for optional auth but differs from error-throwing patterns elsewhere in the codebase (e.g.,useScheduledActions,useTaskRuns). Ensure this behavior aligns with your API's expectations.Including
getAccessTokenin theuseMemodependency array could trigger transport recreation if Privy doesn't guarantee a stable function reference. The pattern appears elsewhere (e.g.,useAccessToken.ts), but consider whether caching the function reference would prevent unnecessary recreations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useChatTransport.ts` around lines 7 - 29, The current useChatTransport hook silently swallows errors from getAccessToken (via .catch(() => null)) and also lists getAccessToken in the useMemo deps which may recreate DefaultChatTransport if the function reference is unstable; update useChatTransport so prepareSendMessagesRequest either surface token-fetch failures as explicit null/controlled behavior (or log the error) instead of silently ignoring them to match the app's auth semantics, and stabilize the dependency by memoizing or capturing a stable getter reference before useMemo (e.g., derive a stable getAccessTokenRef or useCallback wrapper) so only baseUrl triggers transport recreation; adjust prepareSendMessagesRequest, getAccessToken usage, and the useMemo dependency list accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@hooks/useVercelChat.ts`:
- Around line 261-285: The handleDeleteTrailingMessages function clears the chat
error unconditionally even when deletion is skipped due to a missing
accessToken; change the flow so setHasChatApiError(false) is only called when no
deletion is needed (earliestFailedUserMessageId is falsy) or when
deleteTrailingMessages returned true, and otherwise either log/debug that
deletion was skipped because accessToken is missing or leave the error flag set;
update the guard around getEarliestFailedUserMessageId / deleteTrailingMessages
and adjust the setMessages branch accordingly to ensure orphaned messages aren’t
left while clearing the error only on actual success.
---
Nitpick comments:
In `@hooks/useChatTransport.ts`:
- Around line 7-29: The current useChatTransport hook silently swallows errors
from getAccessToken (via .catch(() => null)) and also lists getAccessToken in
the useMemo deps which may recreate DefaultChatTransport if the function
reference is unstable; update useChatTransport so prepareSendMessagesRequest
either surface token-fetch failures as explicit null/controlled behavior (or log
the error) instead of silently ignoring them to match the app's auth semantics,
and stabilize the dependency by memoizing or capturing a stable getter reference
before useMemo (e.g., derive a stable getAccessTokenRef or useCallback wrapper)
so only baseUrl triggers transport recreation; adjust
prepareSendMessagesRequest, getAccessToken usage, and the useMemo dependency
list accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e05e7d50-fb00-4d8c-b20e-a3b4408b564f
📒 Files selected for processing (2)
hooks/useChatTransport.tshooks/useVercelChat.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hooks/useVercelChat.ts (2)
177-191: Consider addinginitialMessagestouseChatconfiguration.The
useChathook is initialized without passinginitialMessages, yet the component receivesinitialMessagesas a prop (line 40). The initial messages are handled via a separateuseEffect(lines 286-309) that callshandleSendQueryMessages. This works, but passinginitialMessagesdirectly touseChatcould simplify the logic and eliminate the need for the effect that monitorsinitialMessages,status,userId, etc.However, I notice the effect also gates on
hasAccessTokento prevent 401 errors, which is a valid reason for the current approach. The current implementation is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 177 - 191, useChat is not being given the initialMessages prop so the hook relies on a separate effect (handleSendQueryMessages) to seed messages; pass the incoming initialMessages into useChat's configuration (initialMessages: initialMessages) and only supply it when hasAccessToken is true (or pass undefined until hasAccessToken is true) to preserve the 401 gating; update/remove the effect that watches initialMessages/status/userId if it only exists to inject those initial messages so you avoid duplicate sends; key symbols: useChat, initialMessages, hasAccessToken, handleSendQueryMessages, useEffect.
286-309: Multiple conditions in effect could benefit from early returns.The effect has a complex condition checking six different states. While functionally correct, extracting this into a guard pattern or documenting the intent would improve readability:
♻️ Optional: Extract conditions for clarity
useEffect(() => { - const isFullyLoggedIn = userId; - const isReady = status === "ready"; - const hasMessages = messages.length > 1; - const hasInitialMessages = initialMessages && initialMessages.length > 0; - const hasAccessToken = !!accessToken; - // Wait for access token before sending initial message to avoid 401 errors - if ( - !hasInitialMessages || - !isReady || - hasMessages || - !isFullyLoggedIn || - !hasAccessToken - ) - return; - handleSendQueryMessages(initialMessages[0]); + // Guard: require initial messages to send + if (!initialMessages || initialMessages.length === 0) return; + + // Guard: wait for chat to be ready + if (status !== "ready") return; + + // Guard: don't re-send if messages already exist + if (messages.length > 1) return; + + // Guard: wait for user authentication + if (!userId || !accessToken) return; + + handleSendQueryMessages(initialMessages[0]); }, [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 286 - 309, The effect block in useVercelChat.ts has a long compound condition; refactor it to improve readability by extracting the checks into named booleans or a guard function (e.g., keep isFullyLoggedIn, isReady, hasMessages, hasInitialMessages, hasAccessToken as top-level computed constants or create a helper like shouldSendInitialMessage()) and then use an early return guard (if (!shouldSendInitialMessage()) return;) before calling handleSendQueryMessages(initialMessages[0]); this preserves behavior but clarifies intent and reduces the complex inline condition in the useEffect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hooks/useVercelChat.ts`:
- Around line 177-191: useChat is not being given the initialMessages prop so
the hook relies on a separate effect (handleSendQueryMessages) to seed messages;
pass the incoming initialMessages into useChat's configuration (initialMessages:
initialMessages) and only supply it when hasAccessToken is true (or pass
undefined until hasAccessToken is true) to preserve the 401 gating;
update/remove the effect that watches initialMessages/status/userId if it only
exists to inject those initial messages so you avoid duplicate sends; key
symbols: useChat, initialMessages, hasAccessToken, handleSendQueryMessages,
useEffect.
- Around line 286-309: The effect block in useVercelChat.ts has a long compound
condition; refactor it to improve readability by extracting the checks into
named booleans or a guard function (e.g., keep isFullyLoggedIn, isReady,
hasMessages, hasInitialMessages, hasAccessToken as top-level computed constants
or create a helper like shouldSendInitialMessage()) and then use an early return
guard (if (!shouldSendInitialMessage()) return;) before calling
handleSendQueryMessages(initialMessages[0]); this preserves behavior but
clarifies intent and reduces the complex inline condition in the useEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c0b0e07-4709-4632-9c1f-56f50fd141af
📒 Files selected for processing (2)
hooks/useChatTransport.tshooks/useVercelChat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/useChatTransport.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hooks/useVercelChat.ts (1)
200-213: Consider:onSuccesscallback capturesearliestFailedUserMessageIdfrom closure.The callback closes over
earliestFailedUserMessageId, which is derived frommessagesviauseMemo. If messages were to change between invokingdeleteTrailingMessagesand theonSuccesscallback firing, the captured ID could reference a stale value.The current implementation is resilient because:
- The
setMessagesupdater receives the current state- The
findIndexcheck gracefully handles missing IDs by returning unchanged messagesThis is acceptable, but for extra safety, you could use a ref to track the current value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 200 - 213, The onSuccess callback in useVercelChat captures earliestFailedUserMessageId from a memoized value which can become stale; fix by creating a ref (e.g., earliestFailedUserMessageIdRef), update that ref whenever earliestFailedUserMessageId changes (inside the same effect/memo where earliestFailedUserMessageId is computed), and then read earliestFailedUserMessageIdRef.current inside the deleteTrailingMessages onSuccess handler before calling setMessages (keep using the functional updater to operate on the latest state). Ensure references: useVercelChat, deleteTrailingMessages, onSuccess, earliestFailedUserMessageId, setMessages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/VercelChat/message-editor.tsx`:
- Around line 18-37: The onSuccess callback passed into
useDeleteTrailingMessages closes over stale draftContent and text; update it to
read the latest values at invocation time by introducing refs for draftContent
and text (e.g., draftContentRef/current and textRef/current) and use those refs
inside the onSuccess body (while keeping the hook initialization unchanged), or
re-create the hook callback when draftContent/text change by memoizing onSuccess
with those dependencies; ensure you update the setMessages logic to use
draftContentRef.current and compare textRef.current !== draftContentRef.current
(or the memoized values) so the message update and reload check use current
values.
In `@hooks/useVercelChat.ts`:
- Around line 290-295: In handleSendMessage, the awaited call to
deleteTrailingMessages can throw and currently has no error handling; wrap the
deleteTrailingMessages({ chatId: id, fromMessageId: earliestFailedUserMessageId
}) call in a try/catch inside handleSendMessage and handle the error
appropriately—either abort and surface a user-facing error (e.g., trigger the
existing toast/error flow and return without calling handleSubmit) or log the
error and continue to call handleSubmit if that UX is acceptable; update error
paths to reference deleteTrailingMessages, handleSendMessage, and handleSubmit
so the flow is clear.
---
Nitpick comments:
In `@hooks/useVercelChat.ts`:
- Around line 200-213: The onSuccess callback in useVercelChat captures
earliestFailedUserMessageId from a memoized value which can become stale; fix by
creating a ref (e.g., earliestFailedUserMessageIdRef), update that ref whenever
earliestFailedUserMessageId changes (inside the same effect/memo where
earliestFailedUserMessageId is computed), and then read
earliestFailedUserMessageIdRef.current inside the deleteTrailingMessages
onSuccess handler before calling setMessages (keep using the functional updater
to operate on the latest state). Ensure references: useVercelChat,
deleteTrailingMessages, onSuccess, earliestFailedUserMessageId, setMessages.
🪄 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: c9990a5c-b77e-4ec8-9d6d-867ce29277f6
📒 Files selected for processing (4)
components/VercelChat/message-editor.tsxhooks/useDeleteTrailingMessages.tshooks/useVercelChat.tslib/messages/deleteTrailingMessages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/useDeleteTrailingMessages.ts
| const { deleteTrailingMessages, isDeletingTrailingMessages } = | ||
| useDeleteTrailingMessages({ | ||
| onSuccess: () => { | ||
| setMessages((messages) => { | ||
| const index = messages.findIndex((m) => m.id === message.id); | ||
| if (index === -1) return messages; | ||
|
|
||
| const updatedMessage: UIMessage = { | ||
| ...message, | ||
| parts: [{ type: "text", text: draftContent } as TextUIPart], | ||
| }; | ||
|
|
||
| return [...messages.slice(0, index), updatedMessage]; | ||
| }); | ||
| setMode("view"); | ||
| if (text !== draftContent) { | ||
| reload(); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Stale closure bug: onSuccess captures stale draftContent and text values.
The useDeleteTrailingMessages hook is initialized once, and its onSuccess callback closes over the initial values of draftContent and text. When the user edits the textarea and clicks "Send", the callback still references the stale values from when the hook was first created—not the current draft.
This will cause:
- The message to be updated with incorrect content
- The
text !== draftContentcomparison to behave incorrectly
🐛 Suggested fix using a ref to track current draft
+ const draftContentRef = useRef(draftContent);
+ useEffect(() => {
+ draftContentRef.current = draftContent;
+ }, [draftContent]);
+
const { deleteTrailingMessages, isDeletingTrailingMessages } =
useDeleteTrailingMessages({
onSuccess: () => {
+ const currentDraft = draftContentRef.current;
setMessages((messages) => {
const index = messages.findIndex((m) => m.id === message.id);
if (index === -1) return messages;
const updatedMessage: UIMessage = {
...message,
- parts: [{ type: "text", text: draftContent } as TextUIPart],
+ parts: [{ type: "text", text: currentDraft } as TextUIPart],
};
return [...messages.slice(0, index), updatedMessage];
});
setMode("view");
- if (text !== draftContent) {
+ if (text !== currentDraft) {
reload();
}
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/VercelChat/message-editor.tsx` around lines 18 - 37, The onSuccess
callback passed into useDeleteTrailingMessages closes over stale draftContent
and text; update it to read the latest values at invocation time by introducing
refs for draftContent and text (e.g., draftContentRef/current and
textRef/current) and use those refs inside the onSuccess body (while keeping the
hook initialization unchanged), or re-create the hook callback when
draftContent/text change by memoizing onSuccess with those dependencies; ensure
you update the setMessages logic to use draftContentRef.current and compare
textRef.current !== draftContentRef.current (or the memoized values) so the
message update and reload check use current values.
| if (earliestFailedUserMessageId) { | ||
| await deleteTrailingMessages({ | ||
| chatId: id, | ||
| fromMessageId: earliestFailedUserMessageId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing error handling for deleteTrailingMessages in handleSendMessage.
If the deletion fails (network error, auth failure, API error), the await will throw, but the error is unhandled. This will prevent handleSubmit from being called, and the user receives no feedback about what went wrong.
Consider wrapping in try/catch to either:
- Show an error toast and abort the send
- Log the error and proceed with sending anyway (if that's acceptable UX)
🛠️ Suggested fix
const handleSendMessage = async (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
if (earliestFailedUserMessageId) {
- await deleteTrailingMessages({
- chatId: id,
- fromMessageId: earliestFailedUserMessageId,
- });
+ try {
+ await deleteTrailingMessages({
+ chatId: id,
+ fromMessageId: earliestFailedUserMessageId,
+ });
+ } catch (error) {
+ console.error("Failed to delete trailing messages:", error);
+ toast.error("Failed to retry message. Please try again.");
+ return;
+ }
}
// Capture the input value before it's cleared by handleSubmit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useVercelChat.ts` around lines 290 - 295, In handleSendMessage, the
awaited call to deleteTrailingMessages can throw and currently has no error
handling; wrap the deleteTrailingMessages({ chatId: id, fromMessageId:
earliestFailedUserMessageId }) call in a try/catch inside handleSendMessage and
handle the error appropriately—either abort and surface a user-facing error
(e.g., trigger the existing toast/error flow and return without calling
handleSubmit) or log the error and continue to call handleSubmit if that UX is
acceptable; update error paths to reference deleteTrailingMessages,
handleSendMessage, and handleSubmit so the flow is clear.
Merge pull request #1620 from recoupable/codex/arpit-migrate-memories…
2 participants
Summary
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor