Skip to content

feat: Add chat_enable_bookmarking(id, client, ..., update_on_input = TRUE, update_on_response = TRUE) #28

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 39 commits into from
Jun 2, 2025

Conversation

schloerke
Copy link
Contributor

@schloerke schloerke commented Feb 20, 2025

Fixes #27
Fixes #55

Reprex app:

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

ui <- function(...) {
  page_fillable(
    chat_ui("chat", fill = TRUE)
  )
}

server <- function(input, output, session) {
  chat_client <- ellmer::chat_ollama(
    system_prompt = "Important: Always respond in a limerick",
    model = "qwen2.5-coder:1.5b",
    echo = TRUE
  )
  # Let the UI know about the client
  chat_enable_bookmarking("chat", chat_client)

  observeEvent(input$chat_user_input, {
    stream <- chat_client$stream_async(input$chat_user_input)
    chat_append("chat", stream)
  })
}

# Enable bookmarking!
shinyApp(ui, server, enableBookmarking = "url")

@schloerke schloerke marked this pull request as ready for review February 24, 2025 18:11
@schloerke schloerke requested a review from cpsievert February 24, 2025 18:11
@schloerke schloerke requested a review from cpsievert February 26, 2025 15:36
@schloerke schloerke marked this pull request as draft February 26, 2025 20:14
@schloerke
Copy link
Contributor Author

schloerke commented Feb 26, 2025

Talking with @icarusz and @wch , I am in agreement that we should change the API:

This change below (set_chat_client(id, client, ..., bookmark = "auto") -> chat_bookmark(id, client, ..., update_on_input = TRUE, update_on_response = TRUE) will only really change the interface. The main logic will remain.

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

ui <- function(...) {
  page_fillable(
    chat_ui("chat", fill = TRUE)
  )
}

server <- function(input, output, session) {
  chat_client <- ellmer::chat_ollama(
    system_prompt = "Important: Always respond in a limerick",
    model = "qwen2.5-coder:1.5b",
    echo = TRUE
  )

  # Let the UI know that shinychat should bookmark the chat_client
  shinychat::chat_bookmark("chat", chat_client) # update_on_input = TRUE, update_on_response = TRUE

  observeEvent(input$chat_user_input, {
    stream_promise <- chat_client$stream_async(input$chat_user_input)
    shinychat::chat_append("chat", stream_promise)
  })

}

# Enable bookmarking!
shinyApp(ui, server, enableBookmarking = "server")

If other features were to be made, they should have their own shinychat::chat_FEATURE() method.

@schloerke schloerke marked this pull request as ready for review February 26, 2025 21:20
@schloerke schloerke changed the title feat: Add set_chat_model(id, model, ..., bookmark = TRUE) feat: Add chat_bookmark(id, client, ..., update_on_input = TRUE, update_on_response = TRUE) Feb 26, 2025
@schloerke schloerke changed the title feat: Add chat_bookmark(id, client, ..., update_on_input = TRUE, update_on_response = TRUE) feat: Add chat_bookmark(id, client) Feb 26, 2025
@schloerke schloerke changed the title feat: Add chat_bookmark(id, client) feat: Add chat_bookmark(id, client, ..., update_on_input = TRUE, update_on_response = TRUE) Feb 26, 2025
Comment on lines 73 to 83
# TODO: Q - I feel this should be removed. Since we are only adding hooks, it doesn't matter if it's enabled or not. If the user diables chat, it would be very annoying to receive error messages for code they may not own.
if (bookmark_store == "disable") {
rlang::abort(
paste0(
"Error: Shiny bookmarking is not enabled. ",
"Please enable bookmarking in your Shiny app either by calling ",
"`shiny::enableBookmarking(\"server\")` or by setting the parameter in ",
"`shiny::shinyApp(enableBookmarking = \"server\")`"
)
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remaining question

@schloerke schloerke requested a review from wch February 26, 2025 21:27
@schloerke
Copy link
Contributor Author

@wch Looking for a review on the API: shinychat::chat_bookmark(ID, CLIENT) with default parameters of update_on_input = TRUE and update_on_response = TRUE.

These default parameters hide away the multiple calls to session$doBookmark().

Ex equivalent server code:

  shinychat::chat_bookmark("chat", chat_client, update_on_input = FALSE, update_on_response = FALSE)
  
  observeEvent(input$chat_user_input, {
    session$doBookmark()
    stream_promise <- chat_client$stream_async(input$chat_user_input)
	stream_promise$then(function(stream_value) { session$doBookmark() })
    shinychat::chat_append("chat", stream_promise)
  })

Final server code:

  shinychat::chat_bookmark("chat", chat_client)
  
  observeEvent(input$chat_user_input, {
    stream_promise <- chat_client$stream_async(input$chat_user_input)
    shinychat::chat_append("chat", stream_promise)
  })

@wch
Copy link

wch commented Feb 27, 2025

The new API does look better. A few questions:

  • The name chat_bookmark makes it sound like running that function will do the bookmarking. Maybe chat_enable_bookmarking?
  • Another thing we should consider: We want to use this mechanism to (A) do bookmarking in the usual way that Shiny does it (B) allow the user to save/restore multiple chats, similar to the ChatGPT web interface. Will this API make sense for the second case as well?
  • Suppose an app uses update_on_input=TRUE and there's a disconnection before the response is completed. When the chat is restored, is there be a way in the UI re-send the message to the server and get a response? If not, then restoring that conversation could leave the app in an unusable state, I think. (We should have the ability to re-send a message anyway, but that's outside the scope of this PR.)

@schloerke schloerke requested review from cpsievert and removed request for wch and cpsievert May 22, 2025 19:36
Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

LGTM once final comment is addressed, thanks!

schloerke and others added 4 commits May 29, 2025 11:35
Co-authored-by: Carson Sievert <[email protected]>
* main:
  chore(r): Fix readme logo
  chore: Update new R package link
  chore: add recommended extensions
  chore: fix logo
  chore: Add a README
  chore: remove unused scripts
  ci(quartodoc): Builds API docs in CI
  docs: metadata on index/404 pages
  docs: Add custom 404 page
  docs: Remove redirects
  docs: Fixup redirects
  ci(pkgdown): Run when `pkgdown.yaml` changes
  ci(docs): Need `contents: write` permissions
  ci(quartodoc): Needs read permissions
  ci(quartodoc): Use `quarto` binary to render quarto in CI
  ci(verify-js-built): Only check if JS files change
  ci(quartodoc): Just install all extras here too
  Reorganize `shinychat` into a monorepo (#56)
@schloerke schloerke merged commit 6a306e6 into main Jun 2, 2025
5 checks passed
@schloerke schloerke deleted the bookmark branch June 2, 2025 21:06
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.

Shinychat bookmarking serialized information hits url limit feat: Add set_chat_client()
3 participants