Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shenst1
Copy link

@shenst1 shenst1 commented Aug 23, 2024

Refresh-fix.mov

Copy link

vercel bot commented Aug 23, 2024

@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)
Copy link
Author

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) {
Copy link
Author

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

@Saran33
Copy link

Saran33 commented Aug 28, 2024

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 generate function, for instance if calling a third party API, then refreshing the router could cause an error because the UI stream won't have completed. I'm currently working on decoupling it so that aiState.done() or aiState.update is only called after the message streams are closed. It seems like when AI state is updated mid-stream it's causing re-renders and some jumping. Also, as for refreshing the router on every message, that too can sometimes add to the jarring UX.

Looking at it again, I'm wondering if could be worth updating the Link in SidebarItem to refresh the router that way to resolve #364
Discussion on it here vercel/next.js#65487 (comment)

@Saran33
Copy link

Saran33 commented Aug 30, 2024

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, revalidatePath currently invalidates all the routes in the client router cache, a well as purging the Data Cache and Full Route Cache, and requires a server action. In light of that, an [even more hackish] approach could be something like:

const handleLinkClick = useCallback(
  async (e: React.MouseEvent) => {
    e.preventDefault();
    router.push(`/chat/${chat.id}`);
    router.refresh();
  },
  [chat.id]
);

@shenst1
Copy link
Author

shenst1 commented Sep 3, 2024

Regarding the fix using a handleLinkClick function, I believe this approach could lead to potential issues. The problem would surface with any <Link> click, not just isolated instances like the one in the sidebar-item. For example, my application includes a main menu with several navigational links to different pages. This would require adding a click handler to every single link on the page (including those in an informational footer) to ensure proper functionality.

In my experience, modifying Link click handlers can introduce unintended consequences and is generally not advisable. Instead, using refresh aligns better with the application state and follows the patterns recommended in the documentation. While this might occasionally cause some animation jankiness, it's better to address the animation itself rather than altering the fundamental state management of the router.

@Saran33
Copy link

Saran33 commented Sep 3, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants