Fix Chat Page Refreshing Before Saving Chat #4
Conversation
…tate` - prevent the UI from refreshing before the db is updated
…conds for performance
|
@Saran33 is attempting to deploy a commit to the Uncurated Tests Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks @Saran33 - I used the code from this PR and it looks like it does solve the issue! |
WalkthroughThis update introduces several changes to improve chat state management and ensure database consistency before UI refreshes. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatComponent
participant AIState
participant Database
User->>ChatComponent: Sends message
ChatComponent->>AIState: Updates AI state
AIState->>Database: Calls updateChat() to persist state
Database-->>AIState: Confirms save
AIState->>ChatComponent: Calls aiState.done()
ChatComponent->>ChatComponent: Triggers router.refresh() after save
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, 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!
Summary of Changes
This pull request addresses a race condition where the UI was refreshing before the database was updated, leading to potential data inconsistencies. To resolve this, the onSetAIState callback has been removed and replaced with an aiStateDone wrapper around aiState.done. This wrapper ensures that the chat is saved to the database before marking AI state updates as complete. Additionally, the Purchase component's status now defaults to requires_action and includes logic to check and update the purchase status based on tool call messages and system messages. A check was added to the chat.tsx to refresh the router if the chat contains 3 messages and the last message is a tool call.
Highlights
- Race Condition Fix: The pull request resolves a race condition by ensuring the database is updated before the UI refreshes, preventing data inconsistencies.
- Purchase Status Handling: The
Purchasecomponent now defaults torequires_actionand includes logic to update the purchase status based on tool call and system messages, including an expiration check. - Router Refresh Logic: The router refresh logic in
chat.tsxhas been updated to handle scenarios where the first user message results in a tool call response.
Changelog
Click here to see the changelog
- components/chat.tsx
- Added import for AI type.
- Updated the
useAIStatehook to specify the type astypeof AI. - Added a check to refresh the router if the chat contains 3 messages and the last message is a tool call.
- components/stocks/stock-purchase.tsx
- Imported
useEffecthook. - Added
toolCallIdprop to thePurchaseinterface. - The
statusprop now defaults torequires_actioninstead ofexpired. - Added a
useEffecthook to check and update the purchase status based on tool call messages and system messages. - Added logic to set purchase status to 'expired' if the tool call message was created more than 30 seconds ago, and an interval to check every 5 seconds whether the status should be
expired.
- Imported
- lib/chat/actions.tsx
- Imported
MutableAIStatetype. - Imported
unixTsNowfromlib/utils. - Replaced direct calls to
aiState.donewith calls toaiStateDoneto ensure the chat is saved before the state is updated. - Added
createdAtproperty to the tool call message. - Passed
toolCallIdas a prop to thePurchasecomponent. - Removed
onSetAIStateand replaced it withaiStateDoneto prevent race conditions. - Added logic to handle errors in
runAsyncFnWithoutBlocking.
- Imported
- lib/types.ts
- Added an optional
createdAtproperty to theMessageinterface. - Added
MutableAIStatetype definition.
- Added an optional
- lib/utils.ts
- Added error handling to
runAsyncFnWithoutBlocking. - Added
unixTsNowfunction to get the current Unix timestamp.
- Added error handling to
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A race, a chase,
UI's swift, DB's slow pace.
Wrapper waits, all's well.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses a race condition in the chat page refreshing logic and improves the purchase component's status updates. The changes seem well-structured and address the identified issues effectively.
Summary of Findings
- Potential infinite loop in
useEffect: TheuseEffectinPurchasecomponent depends onaiState.messages. IfsetAIStateis called within theuseEffectcallback, it could potentially lead to an infinite loop. While the current logic seems to prevent this, it's a pattern that warrants careful consideration. - Clarity of
checkPurchaseStatuslogic: ThecheckPurchaseStatusfunction inPurchasecomponent could benefit from additional comments to clarify the order of operations and the conditions under which the purchase status is updated.
Merge Readiness
The pull request appears to address the identified issues effectively. However, the potential for an infinite loop in the Purchase component's useEffect hook should be carefully considered. I am unable to approve this pull request, and recommend that others review and approve this code before merging. I recommend that the author address the high severity issue before merging.
| 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.
The useEffect hook depends on aiState.messages. If setPurchaseStatus triggers a re-render and subsequently modifies aiState.messages (directly or indirectly), it could lead to an infinite loop. While the current logic seems to prevent this, it's a pattern that warrants careful consideration. Consider using a more stable dependency or a ref to store the previous state.
| 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'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic within checkPurchaseStatus could benefit from additional comments to clarify the order of operations and the conditions under which the purchase status is updated. This would improve readability and maintainability.
| fn().catch(error => { | ||
| console.error('An error occurred in the async function:', error); | ||
| }); |
There was a problem hiding this comment.
This catch block logs errors that occur within the async function. This is good for debugging. Consider adding more context to the error message, such as the function name or arguments.
| fn().catch(error => { | |
| console.error('An error occurred in the async function:', error); | |
| }); | |
| fn().catch(error => { | |
| console.error(`An error occurred in the async function ${fn.name}:`, error); | |
| }); |
![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
🧹 Nitpick comments (3)
components/stocks/stock-purchase.tsx (1)
89-91: Add fallback for missing timestamps.When checking for expiration, there's a proper check for when
createdAtis undefined, but you might want to add logging to track when this happens as it shouldn't be undefined.// Check for expiration const requestedAt = toolMessage.createdAt; if (!requestedAt || unixTsNow() - requestedAt > 30) { + if (!requestedAt) { + console.warn('Missing createdAt timestamp for tool call', toolCallId); + } setPurchaseStatus('expired'); }lib/chat/actions.tsx (2)
87-100: Minor: avoid multipleaiState.get()calls in the same tickInside
confirmPurchaseyou callaiState.get()three times when constructingnewState. Capture it once to guarantee consistency and reduce overhead:- aiStateDone(aiState, { - ...aiState.get(), - messages: [ - ...aiState.get().messages, + const prev = aiState.get() + aiStateDone(aiState, { + ...prev, + messages: [ + ...prev.messages, { id: nanoid(),
584-590: Props spread can leak unexpected fields intoPurchase
tool.resultalready containssymbol,price,numberOfShares, and now possiblystatus. Spreading the result object means any new property returned by the backend will silently become a prop, bypassingPurchase's explicit interface. Prefer listing allowed props:- props={{ - ...(tool.result as object), - toolCallId: tool.toolCallId, - }} + props={{ + symbol: tool.result.symbol, + price: tool.result.price, + numberOfShares: tool.result.numberOfShares, + status: tool.result.status ?? 'requires_action', + toolCallId: tool.toolCallId, + }}
📜 Review details
Configuration used: CodeRabbit UI
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)
components/chat.tsx (1)
lib/chat/actions.tsx (1)
AI(500-523)
lib/types.ts (1)
lib/chat/actions.tsx (1)
AIState(490-493)
🪛 Biome (1.9.4)
lib/chat/actions.tsx
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (12)
lib/utils.ts (2)
54-57: Good addition of error handling.Adding error handling to
runAsyncFnWithoutBlockingis a good improvement to prevent unhandled promise rejections. This will help with debugging by ensuring errors are logged rather than silently failing.
93-93: Well-implemented utility function.The
unixTsNowfunction provides a clean way to get Unix timestamps in seconds, which is used for tracking message creation times and determining expiration in the Purchase component.components/chat.tsx (3)
3-3: Good type import for better type safety.Importing the
AItype from actions provides better type checking and IDE support.
29-29: Improved type annotation.Adding the explicit type annotation to
useAIStateensures proper type inference for the AI state.
45-46: Good enhancement to router refresh logic.This additional condition correctly handles the case where the first user message triggers a tool call response, which was part of the race condition issue being fixed in this PR.
lib/types.ts (2)
5-5: Good addition of timestamp property.Adding the optional
createdAttimestamp to theMessagetype supports the new expiration tracking logic in the Purchase component.
44-48: Well-defined type for AI state management.The
MutableAIStategeneric type provides a clear interface for interacting with AI state, supporting the improved state management described in the PR objectives.components/stocks/stock-purchase.tsx (4)
13-13: Good addition of toolCallId for purchase tracking.Adding the
toolCallIdproperty allows the component to identify the specific tool call message in the AI state, enabling proper status tracking.
18-18: Good state management approach.Setting a default status of
'requires_action'and tracking it with an internal state variable allows the component to manage its lifecycle independently while still being initializable from props.Also applies to: 23-23
64-107: Well-implemented purchase status monitoring.This effect properly tracks the purchase status by:
- Finding the tool message with the matching toolCallId
- Checking if a subsequent system message indicates purchase completion
- Checking for expiration based on the timestamp
- Setting up an interval to periodically check status
- Properly cleaning up the interval
This addresses the core issue by ensuring UI state correctly reflects the actual status of the purchase transaction.
118-118: Consistent UI rendering based on internal state.The conditional rendering now correctly uses the internal
purchaseStatusstate instead of the prop, which is consistent with the state management approach.Also applies to: 183-183, 188-188
lib/chat/actions.tsx (1)
383-398: Ensure the extracreatedAtfield complies withMessagetypingAdding
createdAtto atoolmessage is great for timeout logic, but double-check thatMessagein@/lib/typesmarks it as optional. If not, TypeScript will complain in other call sites.
Also consider addingcreatedAtto all messages when they are first inserted (e.g. insubmitUserMessage) to keep the data model uniform.
| 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) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
updateChat omits the chatId field and is fragile when the first message is non-text
-
The
Chatobject you persist containsidbut notchatId, whereas downstream code (seegetUIStateFromAIState) relies onchatId. When the chat is read back from the DB the field will be missing andaiState.chatIdwill beundefined, breaking UI-key generation and router paths. -
titleis produced viamessages[0].content.substring(...).
• Ifmessagesis empty this will throw.
• For tool calls or any structured message,contentis an array, not a string – callingsubstringwill crash.
A robust fix:
- const firstMessageContent = messages[0].content as string
- const title = firstMessageContent.substring(0, 100)
+ const firstContent =
+ typeof messages[0]?.content === 'string' ? messages[0].content : ''
+ const title = firstContent.slice(0, 100)
const chat: Chat = {
+ chatId, // keep in sync with UI layer
id: chatId,
@@
path
}🧰 Tools
🪛 Biome (1.9.4)
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| 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.
🛠️ Refactor suggestion
aiStateDone needs error handling & preserves race-condition fix
Good idea to gate aiState.done behind updateChat, but if updateChat throws, aiState.done is silently skipped and UI never updates. Wrap the await in try / catch / finally so the UI is never blocked by a persistence failure while still logging the error:
-runAsyncFnWithoutBlocking(async () => {
- await updateChat(newState);
- aiState.done(newState);
-});
+runAsyncFnWithoutBlocking(async () => {
+ try {
+ await updateChat(newState);
+ } catch (err) {
+ console.error('[aiStateDone] failed to persist chat', err);
+ // TODO: surface non-fatal toast to the user if desired
+ } finally {
+ aiState.done(newState);
+ }
+});📝 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 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 = (aiState: MutableAIState<AIState>, newState: AIState) => { | |
| runAsyncFnWithoutBlocking(async () => { | |
| // resolves race condition in aiState.done - the UI refreshed before db was updated | |
| try { | |
| await updateChat(newState); | |
| } catch (err) { | |
| console.error('[aiStateDone] failed to persist chat', err); | |
| // TODO: surface non-fatal toast to the user if desired | |
| } finally { | |
| aiState.done(newState); | |
| } | |
| }); | |
| }; |
aiState.doneis called beforeonSetAIStateonSetAIStatecallback is removed and instead anaiStateDonewrapper aroundaiState.doneis called in place ofaiState.done. This wrapper function waits until the chat is saved to the db before marking the aiState updates as done.chat.tsx, theuseEffecthook for refreshing the router contains an additional check to refresh the router if the chat contains 3 messages and the last message is a tool call. The existing check only checked for 2 messages, meaning the router was not refreshed if the first user message in a chat resulted in a tool call response.Purchasecomponentstatusnow defaults torequires_action, instead ofexpired, with auseEffecthook added to check and update the purchase status based on the relevant tool call message and any related system messages.toolCallIdis passed as a prop toPurchase. This is used to find the relevant tool call in theaiState.messages.purchased, then the purchase is marked as completed.createdAtproperty is added to theMessageinterface. If the tool call message was created more than 30 seconds ago, the checkout is marked as expired. While the status remains in arequires_actionstate, an interval is set to check every 5 seconds whether the status should beexpired.Summary by CodeRabbit