Skip to content

Conversation

@jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Feb 10, 2025

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/6648

Description (What does it do?)

Applies the most recent designs from Figma: https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=16842-209507&t=B7U7mb05q9pzbKWK-0

Updates to disable text input submission while the assistant is responding and shows a stop button to abort the incoming message stream.

Reluctantly - removes the build:static run script as the static folder is now empty.

Screenshots (if appropriate):

image

How can this be tested?

}
// append cannot be added to the dependency array - infinite loop
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [initialPrompt])
Copy link
Collaborator

@ChristopherChudzicki ChristopherChudzicki Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When experimenting with mitodl/mit-learn#2019. two downsides of this effect-based approach became apparent

  1. The request is made twice in Strict Mode, which makes the component behave weirdly. This won't happen in prod, but it's still not great. We could disable strict mode, but best not to.
  2. Vercel's useChat hook, which useAiChat wraps, has an id property that allows restoring chats. So if you have <AiChat chatId="recommender-bot" />, and the component unmounts and remounts, the chat history is restored. But this effect will run again when re-mounting which means we'll get an extra entry in the chat log.

I can think of a few alternatives:

Option 0: Move the splash page to AiChat component.

Option 1:

{/* context value would be the return of `useAiChat` */}
<AiChatProvider requestOpts={...} chatId={...} initialMessages={...}>
    {/* intermediary component & AiChatDispla can call `useChatContext */}
   <AiChatDisplay {...otherPropsProbably } />
  <AiChatDisplay />
</AiChatProvider>

We could export an AiChat component that is provider + display for simple usage.

Option 2: Remove initialPrompt prop, add a chatRef prop that allows parent to access the return of useAiChat via https://react.dev/reference/react/useImperativeHandle . Then the parent can just access append.


Option 0 is probably my preference since it seems like we will want the same splash page for the resource drawer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added option 2 for now. It could be good to replace with 0 if it's confirmed that the splash screen is universal, though I'm reluctant for it to live in one big component if the chat is likely to be used anywhere without it. Also open to 1, though a provider for this single purpose feels excessive. The usage for 2 is also not ideal and I'm not sure it escapes the issue - it needed a timer to wait for the next render cycle when the ref has current (without otherwise rendering the chat before the component wants to and hiding) and its own handling to prevent sending twice:

  useEffect(() => {
    if (!initialPrompt || showEntryScreen) return
    const timer = setTimeout(() => {
      aiChatRef.current?.append({
        content: initialPrompt,
        role: "user",
      })
      setInitialPrompt("")
    }, 0)

    return () => {
      clearTimeout(timer)
      setInitialPrompt("")
    }
  }, [initialPrompt, showEntryScreen])

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I was expecting to call append in the event handlers, but in https://github.com/mitodl/mit-learn/pull/2019/files#diff-1577cd98f6e37dd813f4c5126d2645ae506bdc626ae5c4ffbba4d84b2047f186R201 AiChat isn't mounted initially, you don't have access to it yet when the event handlers run. I guess you could hide it w/ css instead.

I do think (0) makes sense, though. Splash could be enabled conditionally (via splash={true}, or splashTitle, etc.

classes.messageRow,
classes.messageRowAssistant,
)}
key={"loading"}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to test right now in smoot-design as you noted, but if I press "stop" before the response has started streaming, then the ... continues to show.

Not sure what the best behavior is here. When you click stop (as opposed to submitting a new message) we could do

stop()
setMessages(current => [...current, { role: "assistant", content: "*Request canceled*" }])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now disabling the stop button if the last message is not the user's.

Copy link
Collaborator

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request: I think we should handle stop() a little differently, as well as initialPrompt

See comments and let me know if you agree.

@jonkafton jonkafton merged commit 589515e into main Feb 12, 2025
7 checks passed
@ChristopherChudzicki
Copy link
Collaborator

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants