Skip to content

fix(chat_restore): messages shouldn't be cleared before restoring #84

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

Merged
merged 1 commit into from
Aug 7, 2025

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Aug 1, 2025

Follow up to #28.

Currently, when a chat gets restored, the startup messages (i.e., messages in chat_ui(messages = list(....))) get dropped. To repro, submit messages in this app, refresh the page, and note that the 1st message gets dropped:

library(shiny)
library(bslib)
library(shinychat)
library(ellmer)

ui <- function(...) {
  page_fillable(
    chat_ui(
      "chat",
      messages = list("A starting message with important user context")
    )
  )
}

server <- function(input, output, session) {
  chat_client <- chat_openai(system_prompt = "Be brief")
  chat_client$set_turns(
    list(
      Turn(role = "user", content = "Foo"),
      Turn(role = "assistant", content = "You said: Foo"),
    )
  )
  chat_restore("chat", chat_client)
  observeEvent(input$chat_user_input, {
    stream <- chat_client$stream_async(input$chat_user_input)
    chat_append("chat", stream)
  })
}

shinyApp(ui, server, enableBookmarking = "url")

After this change, they no longer get dropped.

For context, I think I had overlooked in #28 that this chat_clear() call was present. I'm pretty sure it was there because, at one time, we were doing the same in Python (because ui.Chat(messages=["..."]) would append to UI and state). We have since deprecated that usage for chat.ui(messages=["..."]) where "undoing" of the duplicate messages is no longer needed -- here is actually a comment in the Python source where we callout that this isn't needed

@cpsievert cpsievert requested a review from schloerke August 1, 2025 16:36
@gadenbuie
Copy link
Collaborator

Have you tested this when you're restoring from a bookmark but the client chat object has attached message history?

I think we've accounted for this, but we would only want the initial messages from chat_ui() to appear in the chat interface after the restore happens.

I think we should try to leave the initial messages in place, for sure, but in the future I would really like us to just have better affordances for these kinds of messages so that they're shown separately from chat history.

@cpsievert
Copy link
Collaborator Author

cpsievert commented Aug 1, 2025

Yes, it does (just updated the example to reflect what I think you're alluding to).

would only want the initial messages from chat_ui() to appear in the chat interface after the restore happens.

I don't quite follow why it needs to be after (did you mean to say before? If not, care to elaborate?).

so that they're shown separately from chat history.

I would like for us to move away from thinking of UI/ellmer state being one in the same. That is, my hope is that R will gain the equivalent of chat.messages() in Python (a more faithful representation of the UI state) rather than relying on restoring the UI state directly from ellmer state.

@gadenbuie
Copy link
Collaborator

would only want the initial messages from chat_ui() to appear in the chat interface after the restore happens.

I don't quite follow why it needs to be after (did you mean to say before? If not, care to elaborate?).

Sorry, I meant, that post restore we'd want the initial message plus the messages restored from the bookmark, but not the messages in the client history.

To test that, you'd need to have a client with some attached messages and you'd open the app to a bookmarked state with a different set of messages.

so that they're shown separately from chat history.

I would like for us to move away from thinking of UI/ellmer state being one in the same. That is, my hope is that R will gain the equivalent of chat.messages() in Python (a more faithful representation of the UI state) rather than relying on restoring the UI state directly from ellmer state.

Yeah that sounds both reasonable and inevitable. But what I'm saying is that we're currently using "initial messages" as a stand-in for UI around initial state and suggestions that could be handled separately and directly with better affordances.

Copy link
Collaborator

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thanks!

@gadenbuie gadenbuie merged commit c2fa3a9 into main Aug 7, 2025
10 checks passed
@gadenbuie gadenbuie deleted the fix/dont-clear-chat-on-restore branch August 7, 2025 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants