-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix DB race condition. Refresh at then end of a completed stream. #404
base: main
Are you sure you want to change the base?
Conversation
@shenst1 is attempting to deploy a commit to the Uncurated Tests Team on Vercel. A member of the Team first needs to authorize it. |
|
||
useEffect(() => { | ||
if (session?.user) { | ||
if (!path.includes('chat') && messages.length === 1) { | ||
window.history.replaceState({}, '', `/chat/${id}`) | ||
setNewChatAnimationId(id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sidebar item should only animate the very first time it is created.
|
||
useEffect(() => { | ||
const messagesLength = aiState.messages?.length | ||
if (messagesLength === 2) { | ||
if (aiState.refreshKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will only be a refresh key at the end of a aiState.done(). No extra refreshes on page loads, but also will have refreshes at the end of each message, not just the first, fixing existing navigation issues like #364
Nice idea, I hadn't noticed the issue with navigating between chats. The only caveat I would have about refreshing the router when aiState is done would be that if there's a delay in the Looking at it again, I'm wondering if could be worth updating the |
Following on from the last comment, a quick workaround for #364 could be to do something like: // actions.ts
import { revalidatePath } from 'next/cache';
export async function revalidate(path: string) {
revalidatePath(path);
} // sidebar-item.tsx
// ...
const handleLinkClick = useCallback(async () => {
await revalidate(`/chat/${chat.id}`);
}, [chat.id]);
// ...
<Link
href={`/chat/${chat.id}`}
onClick={handleLinkClick}
prefetch={false}
)}
> This way, at least we would only be refreshing the router cache when a user clicks to change chat. That may be less frequent than doing so after each message within the same chat. However, const handleLinkClick = useCallback(
async (e: React.MouseEvent) => {
e.preventDefault();
router.push(`/chat/${chat.id}`);
router.refresh();
},
[chat.id]
); |
Regarding the fix using a In my experience, modifying |
@shenst1 I totally agree, it's not ideal. I have the same issue if navigating to other landing pages and then clicking Back in the browser. But if navigating via mouse clicks in the UI, the chat is always refreshed. For my use case, I think the edge case is worth the tradeoff for a smoother chat experience, but currently refactoring it significantly so gonna take another look at it at some stage for sure |
It looks like this repo has been refactored to use the UI library instead of ai/rsc. However, the refresh issue is still present. I was able to get the behavior i visually want by adding a client fetch in chat.tsx like this const { data: loadedConvClientSide } = useQuery({
queryKey: ['conversation', id],
queryFn: async () => {
return await getConversation(id);
}
}) then in useChat: initialMessages: loadedConvClientSide?.messages I've spent a ton of time trying to find a solution that works with revalidatePath, but I couldn't come up with anything that worked consistently. |
Refresh-fix.mov