Clone fix chat page refreshing before saving chat #22
Conversation
…tate` - prevent the UI from refreshing before the db is updated
…conds for performance
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes implement asynchronous persistence for chat state updates, introduce automatic purchase status tracking via polling, and add timestamps to messages. A new wrapper function ensures database persistence completes before UI refresh, while the Purchase component monitors tool results to update purchase status based on backend responses. Changes
Sequence DiagramsequenceDiagram
participant UI as Purchase Component
participant AIState as AI State
participant Actions as Chat Actions
participant DB as Database
participant Refresh as Router Refresh
UI->>AIState: Poll for toolCallId match<br/>(every 5s while requires_action)
AIState-->>UI: Return messages with timestamps
alt Tool Result Found
UI->>UI: Check for 'purchased' in<br/>following system message
alt Purchase Confirmed
UI->>UI: Set purchaseStatus = 'completed'
else Expired (>30s old)
UI->>UI: Set purchaseStatus = 'expired'
end
end
Note over Actions: When appending to AI state
Actions->>Actions: aiStateDone(aiState, newState)<br/>wraps state update
Actions->>DB: Async updateChat(newState)
DB-->>Actions: Persist complete
Actions->>AIState: aiState.done(newState)
AIState->>Refresh: Trigger router.refresh()
Refresh-->>UI: Update UI with persisted state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
@refacto-visz |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses a critical race condition that caused chat pages to refresh prematurely before the chat's state, particularly tool-related messages, was fully saved. It introduces a more robust mechanism for managing AI state persistence and enhances the tracking and lifecycle management of interactive components like stock purchases. The changes ensure a more consistent and reliable user experience by synchronizing UI updates with backend data persistence. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Refacto PR SummaryFixed race condition where chat UI refreshed before database persistence completed, causing unsaved messages to disappear. The implementation introduces proper async sequencing with database-first persistence and adds real-time purchase status tracking for stock transactions. Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant U as User
participant C as Chat Component
participant A as AI Actions
participant DB as Database
participant P as Purchase Component
U->>C: Submit purchase request
C->>A: submitUserMessage()
A->>A: Create tool message with toolCallId
A->>DB: saveChat() - persist first
DB-->>A: Save complete
A->>A: aiState.done() - trigger UI refresh
A-->>C: Purchase component with toolCallId
C->>P: Render purchase UI
P->>P: Poll for status updates (5s interval)
P->>P: Check for completion/expiry (30s timeout)
P-->>U: Update purchase status display
Testing GuideClick to expand
|
codeant-ai
bot
added
the
size:L
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a race condition where the chat page would refresh before the chat state was saved. This is cleverly solved by introducing an aiStateDone wrapper that ensures the database update completes before the UI state is finalized. The changes also add functionality for an expiring stock purchase UI and improve overall code quality by adding typing, better error handling for async functions, and passing down necessary identifiers like toolCallId. My review includes suggestions to improve the robustness of the new purchase status checking logic and to simplify some conditional rendering.
| const nextMessage = aiState.messages[toolMessageIndex + 1]; | ||
| if ( | ||
| nextMessage?.role === 'system' && | ||
| nextMessage.content.includes('purchased') | ||
| ) { | ||
| setPurchaseStatus('completed'); |
There was a problem hiding this comment.
The current logic to check for purchase completion is fragile. It assumes that the system message confirming the purchase is always immediately after the tool message (aiState.messages[toolMessageIndex + 1]). However, other messages (e.g., from slider changes) can be inserted in between, which would break this logic.
To make this more robust, you should search for the confirmation message in all messages that appear after the tool message using some(), instead of just checking the next one.
| const nextMessage = aiState.messages[toolMessageIndex + 1]; | |
| if ( | |
| nextMessage?.role === 'system' && | |
| nextMessage.content.includes('purchased') | |
| ) { | |
| setPurchaseStatus('completed'); | |
| const hasPurchased = aiState.messages | |
| .slice(toolMessageIndex + 1) | |
| .some( | |
| message => | |
| message.role === 'system' && message.content.includes('purchased') | |
| ); | |
| if (hasPurchased) { | |
| setPurchaseStatus('completed'); |
| if (messagesLength === 2) { | ||
| router.refresh() | ||
| } else if (messagesLength === 3 && aiState.messages[2].role === 'tool') { | ||
| router.refresh() | ||
| } |
There was a problem hiding this comment.
The logic to refresh the router for a new chat can be simplified. The if and else if statements both call router.refresh() and can be combined into a single if statement with an OR (||) condition. This would make the code more concise and easier to read. Using optional chaining (?.) for aiState.messages[2] would also make it safer against potential errors if the message doesn't exist.
if (messagesLength === 2 || (messagesLength === 3 && aiState.messages[2]?.role === 'tool')) {
router.refresh()
}
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | ||
| setPurchaseStatus('expired'); | ||
| } |
There was a problem hiding this comment.
The expiration check !requestedAt || unixTsNow() - requestedAt > 30 could cause the purchase UI to expire immediately if requestedAt (which comes from toolMessage.createdAt) is undefined for any reason. A safer approach would be to only check for expiration if requestedAt has a value.
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | |
| setPurchaseStatus('expired'); | |
| } | |
| if (requestedAt && unixTsNow() - requestedAt > 30) { | |
| setPurchaseStatus('expired'); | |
| } |
Code Review: Async State Management & Race Condition FixesPR Confidence Score: 🟥 0 / 5👍 Well Done
📁 Selected files for review (5)
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| const requestedAt = toolMessage.createdAt; | ||
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | ||
| setPurchaseStatus('expired'); | ||
| } |
There was a problem hiding this comment.
Undefined Tool Message
Code accesses toolMessage.createdAt without null checking toolMessage first. If aiState.messages.find() returns undefined, accessing .createdAt causes runtime TypeError. The subsequent requestedAt check doesn't prevent the initial property access failure.
const requestedAt = toolMessage?.createdAt;
if (!requestedAt || unixTsNow() - requestedAt > 30) {
setPurchaseStatus('expired');
}
Commitable Suggestion
| const requestedAt = toolMessage.createdAt; | |
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | |
| setPurchaseStatus('expired'); | |
| } | |
| const requestedAt = toolMessage?.createdAt; | |
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | |
| setPurchaseStatus('expired'); | |
| } |
Standards
- Type-Safety-Null-Checks
- API-Contract-Defensive-Programming
| if (toolMessage) { | ||
| const toolMessageIndex = aiState.messages.indexOf(toolMessage); | ||
| // Check if the next message is a system message containing "purchased" | ||
| const nextMessage = aiState.messages[toolMessageIndex + 1]; |
There was a problem hiding this comment.
Array Bounds Violation
Code accesses array index toolMessageIndex + 1 without bounds checking. If toolMessage is the last element in aiState.messages array, accessing [toolMessageIndex + 1] returns undefined, causing subsequent property access failures on nextMessage?.role.
const nextMessage = toolMessageIndex + 1 < aiState.messages.length ? aiState.messages[toolMessageIndex + 1] : null;
Commitable Suggestion
| const nextMessage = aiState.messages[toolMessageIndex + 1]; | |
| const nextMessage = toolMessageIndex + 1 < aiState.messages.length ? aiState.messages[toolMessageIndex + 1] : null; |
Standards
- Data-Validation-Bounds-Checking
- Array-Access-Safety
| const aiStateDone = (aiState: MutableAIState<AIState>, newState: AIState) => { | ||
| runAsyncFnWithoutBlocking(async () => { | ||
| // resolves race condition in aiState.done - the UI refreshed before db was updated | ||
| await updateChat(newState); | ||
| aiState.done(newState); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Race Condition Risk
Async database update followed by UI state change creates race condition window. If updateChat fails after aiState.done executes, UI shows success while database remains inconsistent. Service reliability degrades due to state synchronization failures between UI and persistence layer.
const aiStateDone = async (aiState: MutableAIState<AIState>, newState: AIState) => {
try {
await updateChat(newState);
aiState.done(newState);
} catch (error) {
console.error('Failed to save chat:', error);
// Keep UI state consistent with database failure
throw error;
}
};
Commitable Suggestion
| const aiStateDone = (aiState: MutableAIState<AIState>, newState: AIState) => { | |
| runAsyncFnWithoutBlocking(async () => { | |
| // resolves race condition in aiState.done - the UI refreshed before db was updated | |
| await updateChat(newState); | |
| aiState.done(newState); | |
| }); | |
| }; | |
| const aiStateDone = async (aiState: MutableAIState<AIState>, newState: AIState) => { | |
| try { | |
| await updateChat(newState); | |
| aiState.done(newState); | |
| } catch (error) { | |
| console.error('Failed to save chat:', error); | |
| // Keep UI state consistent with database failure | |
| throw error; | |
| } | |
| }; |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| const aiStateDone = (aiState: MutableAIState<AIState>, newState: AIState) => { | ||
| runAsyncFnWithoutBlocking(async () => { | ||
| // resolves race condition in aiState.done - the UI refreshed before db was updated | ||
| await updateChat(newState); | ||
| aiState.done(newState); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Race Condition Vulnerability
The aiStateDone function attempts to resolve race conditions but creates a new one by making database updates asynchronous without proper synchronization. If multiple rapid calls occur, database state could become inconsistent with UI state. This could lead to data corruption or unauthorized access to stale chat data.
Standards
- CWE-362
- OWASP-A04
- NIST-SSDF-PW.1
| const requestedAt = toolMessage.createdAt; | ||
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | ||
| setPurchaseStatus('expired'); | ||
| } |
There was a problem hiding this comment.
Timestamp Validation Missing
Purchase expiration logic relies on client-controlled timestamp without server-side validation. Attackers could manipulate createdAt values to prevent purchase expiration or force premature expiration. This could lead to unauthorized transactions or denial of service by preventing legitimate purchases.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| fn().catch(error => { | ||
| console.error('An error occurred in the async function:', error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Silent Failure Risk
Generic error logging without error propagation or recovery mechanisms creates silent failure scenarios. Critical async operations like database saves fail without system notification or retry logic. Service availability impacts occur when essential operations fail silently without alerting or recovery.
Standards
- ISO-IEC-25010-Reliability-Recoverability
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Budget
| } else { | ||
| // Check for expiration | ||
| const requestedAt = toolMessage.createdAt; | ||
| if (!requestedAt || unixTsNow() - requestedAt > 30) { |
There was a problem hiding this comment.
Magic Number Usage
Hardcoded timeout values (30 seconds, 5000ms) reduce configurability and make business rules unclear. Magic numbers scattered in logic make timeout policies difficult to maintain and test with different scenarios.
Standards
- Clean-Code-Meaningful-Names
- Maintainability-Quality-Configuration
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/chat.tsx(3 hunks)components/stocks/stock-purchase.tsx(4 hunks)lib/chat/actions.tsx(13 hunks)lib/types.ts(2 hunks)lib/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/chat/actions.tsx (4)
lib/utils.ts (1)
unixTsNow(93-93)lib/types.ts (2)
Chat(8-16)MutableAIState(44-48)app/actions.ts (1)
saveChat(147-161)components/stocks/stock-purchase.tsx (1)
Purchase(17-193)
components/chat.tsx (1)
lib/chat/actions.tsx (1)
AI(500-523)
🔇 Additional comments (11)
lib/utils.ts (2)
54-57: Improved error visibility for async functions.The addition of
.catch()with error logging ensures that unhandled promise rejections are visible in the console, improving debuggability.
93-93: LGTM! Unix timestamp utility is correctly implemented.The function correctly converts milliseconds to seconds for Unix timestamp representation.
lib/types.ts (2)
5-5: LGTM! Optional timestamp field added to Message type.The optional
createdAtfield enables timestamp tracking for messages, which is used in the purchase expiration logic.
44-48: LGTM! MutableAIState type correctly models state container API.The type accurately represents a mutable state container with getter, updater, and completion methods. The union type for
doneappropriately handles both parameterized and parameterless forms.lib/chat/actions.tsx (3)
385-385: LGTM! Timestamp added for purchase expiration tracking.The
createdAttimestamp enables the Purchase component to determine when the purchase request expires (after 30 seconds).
409-409: LGTM! toolCallId propagation enables purchase status polling.The
toolCallIdallows the Purchase component to locate the corresponding tool message in the AI state for status tracking.
584-590: LGTM! Consistent toolCallId propagation in UI state.The changes ensure that the Purchase component receives the
toolCallIdwhen rendering from persisted state, maintaining consistency with the initial render path.components/chat.tsx (1)
3-3: LGTM! Type-safe AI state usage.The type-only import and generic parameter ensure type safety for the AI state, enabling better IDE support and compile-time checking.
Also applies to: 29-29
components/stocks/stock-purchase.tsx (3)
13-13: LGTM! Purchase interface updated to support status polling.The addition of
toolCallIdenables the component to track purchase status, and the default status change to'requires_action'aligns with the new polling-based workflow.Also applies to: 18-18
97-106: LGTM! Interval cleanup properly implemented.The cleanup function correctly clears the interval when the component unmounts or when dependencies change, preventing memory leaks.
118-118: LGTM! Consistent use of purchaseStatus state.The component correctly uses the
purchaseStatusstate for all conditional rendering, ensuring the UI reflects the current polling status rather than the initial prop value.Also applies to: 183-183, 188-188
| } else if (messagesLength === 3 && aiState.messages[2].role === 'tool') { | ||
| router.refresh() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and read the chat.tsx file to see the full context
fd -n "chat.tsx" -t fRepository: coderabbit-test/ai-chatbot
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Find chat.tsx file
fd "chat.tsx" -t fRepository: coderabbit-test/ai-chatbot
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the chat.tsx file to see the full context
cat -n components/chat.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 3233
🏁 Script executed:
#!/bin/bash
# Search for router.refresh() patterns in the codebase
rg -n "router\.refresh\(\)" --type=tsx --type=tsRepository: coderabbit-test/ai-chatbot
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for router.refresh() patterns - search without type restriction
rg -n "router\.refresh\(\)" -C 3Repository: coderabbit-test/ai-chatbot
Length of output: 1808
🏁 Script executed:
#!/bin/bash
# Look at the AI actions and message types to understand the context
fd "actions" -t f -e ts -e tsx | head -20Repository: coderabbit-test/ai-chatbot
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Check the chat actions to understand the message flow and tool messages
cat -n lib/chat/actions.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 21137
🏁 Script executed:
#!/bin/bash
# Check the Message type definition
fd -e ts -e tsx | xargs rg -n "type Message|interface Message" | head -20Repository: coderabbit-test/ai-chatbot
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Check the Message type in lib/types.ts
cat -n lib/types.tsRepository: coderabbit-test/ai-chatbot
Length of output: 1241
🏁 Script executed:
#!/bin/bash
# Search for all places where aiState messages are modified
rg -n "aiState\." lib/chat/actions.tsx | grep -E "(update|done)"Repository: coderabbit-test/ai-chatbot
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Let me trace the exact message sequence by looking at confirmPurchase
sed -n '41,109p' lib/chat/actions.tsx | cat -nRepository: coderabbit-test/ai-chatbot
Length of output: 2173
🏁 Script executed:
#!/bin/bash
# Trace through submitUserMessage to understand exact message sequence
# Get lines 111-127 which show the initial update
sed -n '111,127p' lib/chat/actions.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 365
🏁 Script executed:
#!/bin/bash
# Look at the tool generate functions to trace exact message additions
# Check listStocks tool from lines 182-240
sed -n '182,240p' lib/chat/actions.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 1667
🏁 Script executed:
#!/bin/bash
# Search for all places where role: 'tool' messages are added
rg -n "role: 'tool'" lib/chat/actions.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Look for when role: 'text' or similar is added
rg -n "role: 'assistant'" lib/chat/actions.tsx | head -20Repository: coderabbit-test/ai-chatbot
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Let me create a test to trace the exact message sequence
# Looking at how the AI framework processes tools vs text
# Check if there's documentation about streamUI behavior
cat -n lib/chat/actions.tsx | sed -n '131,180p'Repository: coderabbit-test/ai-chatbot
Length of output: 2345
Add clarifying comments and fix edge case for multiple tool messages.
The router refresh logic at messagesLength === 2 (first assistant response) is sound, but the condition messagesLength === 3 && aiState.messages[2].role === 'tool' only handles the first tool result. When multiple tools are called sequentially (e.g., tool results at indices 4, 6, etc.), these refreshes won't trigger, causing potential stale state.
Update the logic to cover all tool results:
useEffect(() => {
const messagesLength = aiState.messages?.length
+ // Refresh after initial assistant response to persist the conversation
if (messagesLength === 2) {
router.refresh()
+ // Refresh after any tool result to reflect updated state
- } else if (messagesLength === 3 && aiState.messages[2].role === 'tool') {
+ } else if (aiState.messages?.[messagesLength - 1]?.role === 'tool') {
router.refresh()
}
}, [aiState.messages, router])📝 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.
| } else if (messagesLength === 3 && aiState.messages[2].role === 'tool') { | |
| router.refresh() | |
| useEffect(() => { | |
| const messagesLength = aiState.messages?.length | |
| // Refresh after initial assistant response to persist the conversation | |
| if (messagesLength === 2) { | |
| router.refresh() | |
| // Refresh after any tool result to reflect updated state | |
| } else if (aiState.messages?.[messagesLength - 1]?.role === 'tool') { | |
| router.refresh() | |
| } | |
| }, [aiState.messages, router]) |
🤖 Prompt for AI Agents
In components/chat.tsx around lines 45-46, the current condition only refreshes
the router for a single tool result at aiState.messages[2], which misses
subsequent tool messages; replace the hard-coded index check with a guard that
detects any tool result at the end of the message stream (e.g., check
messagesLength >= 3 and that the last message's role === 'tool') so every time a
tool result is appended the router.refresh() runs, and add a short clarifying
comment explaining that we refresh whenever the most recent message is a tool
result to cover sequential tool calls.
| useEffect(() => { | ||
| const checkPurchaseStatus = () => { | ||
| if (purchaseStatus !== 'requires_action') { | ||
| return; | ||
| } | ||
| // check for purchase completion | ||
| // Find the tool message with the matching toolCallId | ||
| const toolMessage = aiState.messages.find( | ||
| message => | ||
| message.role === 'tool' && | ||
| message.content.some(part => part.toolCallId === toolCallId) | ||
| ); | ||
|
|
||
| if (toolMessage) { | ||
| const toolMessageIndex = aiState.messages.indexOf(toolMessage); | ||
| // Check if the next message is a system message containing "purchased" | ||
| const nextMessage = aiState.messages[toolMessageIndex + 1]; | ||
| if ( | ||
| nextMessage?.role === 'system' && | ||
| nextMessage.content.includes('purchased') | ||
| ) { | ||
| setPurchaseStatus('completed'); | ||
| } else { | ||
| // Check for expiration | ||
| const requestedAt = toolMessage.createdAt; | ||
| if (!requestedAt || unixTsNow() - requestedAt > 30) { | ||
| setPurchaseStatus('expired'); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| checkPurchaseStatus(); | ||
|
|
||
| let intervalId: NodeJS.Timeout | null = null; | ||
| if (purchaseStatus === 'requires_action') { | ||
| intervalId = setInterval(checkPurchaseStatus, 5000); | ||
| } | ||
|
|
||
| return () => { | ||
| if (intervalId) { | ||
| clearInterval(intervalId); | ||
| } | ||
| }; | ||
| }, [purchaseStatus, toolCallId, aiState.messages]); |
There was a problem hiding this comment.
Potential issue: Polling logic may not detect completion reliably.
The polling implementation has several concerns:
-
String-based completion detection (Line 83): The code checks if
nextMessage.content.includes('purchased'), which is fragile. If the system message format changes or uses different wording, the check will fail. -
Message ordering assumption (Lines 78-80): The code assumes the system message immediately follows the tool message, but this may not always be guaranteed if messages are added asynchronously.
-
Missing null check (Line 71):
message.content.some()could fail ifmessage.contentis not an array. -
Expiration logic (Line 89): If
createdAtis missing, the condition!requestedAt || unixTsNow() - requestedAt > 30will immediately expire the purchase, which may not be the desired behavior.
Consider these improvements:
useEffect(() => {
const checkPurchaseStatus = () => {
if (purchaseStatus !== 'requires_action') {
return;
}
// check for purchase completion
// Find the tool message with the matching toolCallId
const toolMessage = aiState.messages.find(
message =>
message.role === 'tool' &&
+ Array.isArray(message.content) &&
message.content.some(part => part.toolCallId === toolCallId)
);
if (toolMessage) {
const toolMessageIndex = aiState.messages.indexOf(toolMessage);
// Check if the next message is a system message containing "purchased"
const nextMessage = aiState.messages[toolMessageIndex + 1];
if (
nextMessage?.role === 'system' &&
- nextMessage.content.includes('purchased')
+ typeof nextMessage.content === 'string' &&
+ nextMessage.content.toLowerCase().includes('purchased')
) {
setPurchaseStatus('completed');
} else {
// Check for expiration
const requestedAt = toolMessage.createdAt;
- if (!requestedAt || unixTsNow() - requestedAt > 30) {
+ if (requestedAt && unixTsNow() - requestedAt > 30) {
setPurchaseStatus('expired');
}
}
}
};Alternatively, consider using a more robust completion signal (e.g., checking for specific message structure or status field) rather than substring matching.
🤖 Prompt for AI Agents
In components/stocks/stock-purchase.tsx around lines 64 to 107, the polling
relies on fragile assumptions: it calls message.content.some() without ensuring
content is an array, looks only at the very next message and does a substring
match for "purchased", and immediately treats a missing createdAt as expired.
Fix by first null-checking that message.content is an array before calling
.some; instead of only checking aiState.messages[toolMessageIndex + 1], scan
forward from toolMessageIndex+1 for a system message that references the same
toolCallId or a dedicated status field; replace substring matching with a
structured check (e.g., a status property or specific content part rather than
includes('purchased')); and change the expiration logic so missing createdAt
does not auto-expire (use a conservative default like treating it as not expired
or set requestedAt = createdAt ?? unixTsNow() and adjust the timeout), keeping
the polling/interval setup and cleanup as-is.
| const updateChat = async (state: AIState) => { | ||
| 'use server' | ||
|
|
||
| if (session && session.user) { | ||
| const { chatId, messages } = state | ||
|
|
||
| const createdAt = new Date() | ||
| const userId = session.user.id as string | ||
| const path = `/chat/${chatId}` | ||
|
|
||
| const firstMessageContent = messages[0].content as string | ||
| const title = firstMessageContent.substring(0, 100) | ||
|
|
||
| const chat: Chat = { | ||
| id: chatId, | ||
| title, | ||
| userId, | ||
| createdAt, | ||
| messages, | ||
| path | ||
| } | ||
| const session = await auth() | ||
|
|
||
| await saveChat(chat) | ||
| } else { | ||
| return | ||
| if (session && session.user) { | ||
| const { chatId, messages } = state | ||
|
|
||
| const createdAt = new Date() | ||
| const userId = session.user.id as string | ||
| const path = `/chat/${chatId}` | ||
|
|
||
| const firstMessageContent = messages[0].content as string | ||
| const title = firstMessageContent.substring(0, 100) | ||
|
|
||
| const chat: Chat = { | ||
| id: chatId, | ||
| title, | ||
| userId, | ||
| createdAt, | ||
| messages, | ||
| path | ||
| } | ||
|
|
||
| await saveChat(chat) | ||
| } else { | ||
| return | ||
| } | ||
| }) | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for saveChat failures.
The updateChat function doesn't handle errors from saveChat. If the database operation fails, the error will be caught by runAsyncFnWithoutBlocking and logged, but the application won't have visibility into persistence failures.
Consider adding explicit error handling:
const updateChat = async (state: AIState) => {
'use server'
const session = await auth()
if (session && session.user) {
const { chatId, messages } = state
const createdAt = new Date()
const userId = session.user.id as string
const path = `/chat/${chatId}`
const firstMessageContent = messages[0].content as string
const title = firstMessageContent.substring(0, 100)
const chat: Chat = {
id: chatId,
title,
userId,
createdAt,
messages,
path
}
- await saveChat(chat)
+ try {
+ await saveChat(chat)
+ } catch (error) {
+ console.error('Failed to save chat:', error)
+ // Optionally: throw error to prevent aiState.done from being called
+ }
} else {
return
}
};📝 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 updateChat = async (state: AIState) => { | |
| 'use server' | |
| if (session && session.user) { | |
| const { chatId, messages } = state | |
| const createdAt = new Date() | |
| const userId = session.user.id as string | |
| const path = `/chat/${chatId}` | |
| const firstMessageContent = messages[0].content as string | |
| const title = firstMessageContent.substring(0, 100) | |
| const chat: Chat = { | |
| id: chatId, | |
| title, | |
| userId, | |
| createdAt, | |
| messages, | |
| path | |
| } | |
| const session = await auth() | |
| await saveChat(chat) | |
| } else { | |
| return | |
| if (session && session.user) { | |
| const { chatId, messages } = state | |
| const createdAt = new Date() | |
| const userId = session.user.id as string | |
| const path = `/chat/${chatId}` | |
| const firstMessageContent = messages[0].content as string | |
| const title = firstMessageContent.substring(0, 100) | |
| const chat: Chat = { | |
| id: chatId, | |
| title, | |
| userId, | |
| createdAt, | |
| messages, | |
| path | |
| } | |
| await saveChat(chat) | |
| } else { | |
| return | |
| } | |
| }) | |
| }; | |
| const updateChat = async (state: AIState) => { | |
| 'use server' | |
| const session = await auth() | |
| if (session && session.user) { | |
| const { chatId, messages } = state | |
| const createdAt = new Date() | |
| const userId = session.user.id as string | |
| const path = `/chat/${chatId}` | |
| const firstMessageContent = messages[0].content as string | |
| const title = firstMessageContent.substring(0, 100) | |
| const chat: Chat = { | |
| id: chatId, | |
| title, | |
| userId, | |
| createdAt, | |
| messages, | |
| path | |
| } | |
| try { | |
| await saveChat(chat) | |
| } catch (error) { | |
| console.error('Failed to save chat:', error) | |
| // Optionally: throw error to prevent aiState.done from being called | |
| } | |
| } else { | |
| return | |
| } | |
| }; |
🤖 Prompt for AI Agents
In lib/chat/actions.tsx around lines 525-553 the call to await saveChat(chat)
has no error handling; wrap the persistence call in a try/catch, log the caught
error with contextual details (chatId, userId) using the server/logger
available, and either rethrow or return a sensible failure result so callers can
detect the persistence failure; ensure the catch preserves the original error
message/stack when rethrowing or includes it in the returned error information.
| const aiStateDone = (aiState: MutableAIState<AIState>, newState: AIState) => { | ||
| runAsyncFnWithoutBlocking(async () => { | ||
| // resolves race condition in aiState.done - the UI refreshed before db was updated | ||
| await updateChat(newState); | ||
| aiState.done(newState); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'actions.tsx' --type f | grep -E 'lib/chat|chat/actions' | head -5Repository: coderabbit-test/ai-chatbot
Length of output: 90
🏁 Script executed:
find . -name 'actions.tsx' -path '*/chat/*' | head -5Repository: coderabbit-test/ai-chatbot
Length of output: 92
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '520,600p'Repository: coderabbit-test/ai-chatbot
Length of output: 2926
🏁 Script executed:
grep -n "runAsyncFnWithoutBlocking" lib/chat/actions.tsx | head -10Repository: coderabbit-test/ai-chatbot
Length of output: 192
🏁 Script executed:
grep -rn "runAsyncFnWithoutBlocking" lib/ | head -10Repository: coderabbit-test/ai-chatbot
Length of output: 314
🏁 Script executed:
grep -n "aiStateDone" lib/chat/actions.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 406
🏁 Script executed:
cat -n lib/utils.ts | sed -n '45,70p'Repository: coderabbit-test/ai-chatbot
Length of output: 994
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '1,40p'Repository: coderabbit-test/ai-chatbot
Length of output: 1333
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '80,100p'Repository: coderabbit-test/ai-chatbot
Length of output: 752
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '155,175p'Repository: coderabbit-test/ai-chatbot
Length of output: 720
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '195,215p'Repository: coderabbit-test/ai-chatbot
Length of output: 746
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '260,280p'Repository: coderabbit-test/ai-chatbot
Length of output: 781
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '318,340p'Repository: coderabbit-test/ai-chatbot
Length of output: 996
🏁 Script executed:
grep -n "onSetAIState\|unstable_onSetAIState" lib/chat/actions.tsxRepository: coderabbit-test/ai-chatbot
Length of output: 52
🏁 Script executed:
tail -n 100 lib/chat/actions.tsx | head -50Repository: coderabbit-test/ai-chatbot
Length of output: 1076
🏁 Script executed:
grep -n "createAI\|onSetAIState\|unstable_onSetAIState" lib/chat/actions.tsx | head -20Repository: coderabbit-test/ai-chatbot
Length of output: 134
🏁 Script executed:
cat -n lib/chat/actions.tsx | tail -50Repository: coderabbit-test/ai-chatbot
Length of output: 2155
🏁 Script executed:
cat -n lib/chat/actions.tsx | sed -n '495,515p'Repository: coderabbit-test/ai-chatbot
Length of output: 651
The race condition is NOT resolved by the current implementation.
runAsyncFnWithoutBlocking is fire-and-forget (it calls the function without awaiting), which means aiStateDone returns immediately without waiting for either updateChat to complete or aiState.done() to execute. Since all call sites don't await aiStateDone, the caller proceeds while the database update and state finalization happen asynchronously in the background. This means:
- The caller returns before
updateChatcompletes - The caller returns before
aiState.done(newState)is called - The UI state update via
done()happens asynchronously after the caller has already returned
The comment's claim that this "resolves race condition" is incorrect. To actually fix this, either make aiStateDone async and await it at all call sites, or implement a different synchronization mechanism before calling done(). The current approach leaves the race condition in place.
🤖 Prompt for AI Agents
In lib/chat/actions.tsx around lines 555-561, the current use of
runAsyncFnWithoutBlocking makes aiStateDone fire-and-forget so callers continue
before updateChat and aiState.done run; change aiStateDone to return a Promise
(make it async) and remove runAsyncFnWithoutBlocking so it awaits
updateChat(newState) and then calls/awaits aiState.done(newState) (or otherwise
ensures done completes), and update all call sites to await aiStateDone so the
DB update and state finalization complete before the caller proceeds;
alternatively, if you cannot change all callers, implement a synchronization
mechanism (e.g., return a Promise that callers can await or a shared latch) and
document that callers must await it.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // Check if the next message is a system message containing "purchased" | ||
| const nextMessage = aiState.messages[toolMessageIndex + 1]; | ||
| if ( | ||
| nextMessage?.role === 'system' && | ||
| nextMessage.content.includes('purchased') |
There was a problem hiding this comment.
Suggestion: The completion check assumes the "purchased" system message is immediately after the matching tool message, so if any other messages (like slider-change system messages) are inserted in between, the purchase will never be marked as completed and will incorrectly expire after 30 seconds. [logic error]
Severity Level: Minor
| // Check if the next message is a system message containing "purchased" | |
| const nextMessage = aiState.messages[toolMessageIndex + 1]; | |
| if ( | |
| nextMessage?.role === 'system' && | |
| nextMessage.content.includes('purchased') | |
| // Check if any subsequent system message contains "purchased" | |
| const hasPurchaseSystemMessage = aiState.messages | |
| .slice(toolMessageIndex + 1) | |
| .some( | |
| nextMessage => | |
| nextMessage.role === 'system' && | |
| typeof nextMessage.content === 'string' && | |
| nextMessage.content.includes('purchased') | |
| ); | |
| if (hasPurchaseSystemMessage) { |
Why it matters? ⭐
The PR's current logic only inspects the immediate next message after the tool message. The UI code explicitly inserts system-like messages for slider changes (see onSliderChange inserting a system role message with the component id), so it's plausible other messages will appear between the tool and the final "purchased" system message. Searching all subsequent messages for a system message containing "purchased" is a correct logical fix — it prevents false expirations while preserving the existing expiry check based on toolMessage.createdAt. The improved code's extra typeof guard is also sensible because tool messages use an array for content while system messages are strings.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** components/stocks/stock-purchase.tsx
**Line:** 79:83
**Comment:**
*Logic Error: The completion check assumes the "purchased" system message is immediately after the matching tool message, so if any other messages (like slider-change system messages) are inserted in between, the purchase will never be marked as completed and will incorrectly expire after 30 seconds.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| export type MutableAIState<AIState> = { | ||
| get: () => AIState; | ||
| update: (newState: AIState | ((current: AIState) => AIState)) => void; | ||
| done: ((newState: AIState) => void) | (() => void); |
There was a problem hiding this comment.
Suggestion: The done property in MutableAIState is typed as a union of two function signatures, which does not match how it is used (always called with a new state) and makes aiState.done(newState) in the codebase a type error; modeling this as a single function with an optional parameter correctly reflects the runtime API and avoids incorrect TypeScript errors. [type error]
Severity Level: Minor
Why it matters? ⭐
The current union type for done is problematic: a union of two function signatures is only callable in ways compatible with BOTH signatures.
Because one variant accepts an argument and the other doesn't, calling done(newState) is considered unsafe by TypeScript and can trigger a type error.
Modeling it as a single function with an optional parameter (done: (newState?: AIState) => void) accurately reflects the runtime API (callable with or without a state) and removes the false-positive type errors without changing behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** lib/types.ts
**Line:** 47:47
**Comment:**
*Type Error: The `done` property in `MutableAIState` is typed as a union of two function signatures, which does not match how it is used (always called with a new state) and makes `aiState.done(newState)` in the codebase a type error; modeling this as a single function with an optional parameter correctly reflects the runtime API and avoids incorrect TypeScript errors.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| }).format(value) | ||
|
|
||
| export const runAsyncFnWithoutBlocking = ( | ||
| fn: (...args: any) => Promise<any> |
There was a problem hiding this comment.
Suggestion: The parameter type for the non-blocking helper is misleading: it declares that the function can accept arbitrary arguments ((...args: any) => Promise<any>), but the implementation never forwards any arguments, so TypeScript will happily allow passing functions that expect parameters while at runtime they receive undefined, leading to subtle logic errors. Restricting the type to a zero-argument async function makes the contract accurate and prevents misuse. [type error]
Severity Level: Minor
| fn: (...args: any) => Promise<any> | |
| fn: () => Promise<any> |
Why it matters? ⭐
The implementation always invokes fn() with no arguments, so typing it as (...args: any) => Promise is misleading and allows callers to pass functions that expect parameters (they would receive undefined at runtime). Narrowing the type to () => Promise better matches the runtime behavior, prevents a class of subtle bugs, and is a safe, non-breaking tightening of the contract.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** lib/utils.ts
**Line:** 52:52
**Comment:**
*Type Error: The parameter type for the non-blocking helper is misleading: it declares that the function can accept arbitrary arguments (`(...args: any) => Promise<any>`), but the implementation never forwards any arguments, so TypeScript will happily allow passing functions that expect parameters while at runtime they receive `undefined`, leading to subtle logic errors. Restricting the type to a zero-argument async function makes the contract accurate and prevents misuse.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
CodeAnt-AI Description
Fix chat page refreshing before chat is saved and make stock purchases detect completion/expiration reliably
What Changed
Impact
✅ No more premature chat page refreshes after sending messages✅ Clearer purchase status (completed vs expired) shown to users✅ Fewer frequent status checks during checkout💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.