From fcee11e0b8e38b26a6f640e08a1802c16700f173 Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 1 Jul 2026 09:21:08 -0500 Subject: [PATCH 01/16] refactor: migrate chat UI/server to shinychat's high-level API Replaces the hand-rolled ExtendedTask/cancel/bookmark wiring in both packages with shinychat's high-level chat_ui()/chat_server() (R) and client= (Python) entry points, so querychat no longer duplicates logic that shinychat now owns directly. - R: mod_ui/mod_server now delegate to chat_mod_ui()/chat_mod_server() instead of manually wiring ExtendedTask, cancel, and restore; greeting moves to the server side. - Python: mod_ui/mod_server pass client= to shinychat.Chat and drop the manual submit/cancel/bookmark wiring; generate_stream is replaced by generate_async, and the greeter is wired into _make_greeting. - Adds a minimum version requirement for the migration and removes argument-default overrides that are no longer needed. - Incorporates code review fixes: restored moduleServer isolation and original formatting in querychat_module.R, corrected greeting logic, and removed over-explaining comments left over from the migration. --- pkg-py/src/querychat/_querychat_greeter.py | 20 +- pkg-py/src/querychat/_shiny.py | 14 +- pkg-py/src/querychat/_shiny_module.py | 88 ++--- pkg-py/src/querychat/_utils_shinychat.py | 19 -- pkg-py/tests/test_shiny_module.py | 67 ++++ pkg-r/DESCRIPTION | 2 +- pkg-r/R/QueryChat.R | 2 +- pkg-r/R/querychat_module.R | 156 ++------- pkg-r/tests/testthat/test-querychat_module.R | 338 +++++++++++++++---- 9 files changed, 397 insertions(+), 309 deletions(-) delete mode 100644 pkg-py/src/querychat/_utils_shinychat.py diff --git a/pkg-py/src/querychat/_querychat_greeter.py b/pkg-py/src/querychat/_querychat_greeter.py index 19fc3e75..ef95551e 100644 --- a/pkg-py/src/querychat/_querychat_greeter.py +++ b/pkg-py/src/querychat/_querychat_greeter.py @@ -12,8 +12,6 @@ import chatlas - from shiny import reactive - class QueryChatGreeter: """Controls greeting generation for a QueryChat instance. Access via ``qc.greeter``.""" @@ -66,19 +64,7 @@ def generate( """Generate a greeting using the greeting system prompt.""" return str(self.build_client(base).chat(GREETING_PROMPT, echo=echo)) - async def generate_stream( - self, - *, - chat_ui, - current_greeting: reactive.Value[str | None], - base: chatlas.Chat | None = None, - ) -> None: - """Stream a greeting into the Shiny chat UI and capture the result.""" + async def generate_async(self, *, base: chatlas.Chat | None = None): + """Stream a greeting response from the greeting client.""" client = self.build_client(base) - stream = await client.stream_async(GREETING_PROMPT, echo="none") - from ._utils_shinychat import chat_greeting_persistent - - await chat_ui.set_greeting(chat_greeting_persistent(stream)) # pyright: ignore[reportArgumentType] - last_turn = client.get_last_turn(role="assistant") - if last_turn is not None: - current_greeting.set(last_turn.text) + return await client.stream_async(GREETING_PROMPT, echo="none") diff --git a/pkg-py/src/querychat/_shiny.py b/pkg-py/src/querychat/_shiny.py index 7fae2205..a5974c5e 100644 --- a/pkg-py/src/querychat/_shiny.py +++ b/pkg-py/src/querychat/_shiny.py @@ -462,12 +462,7 @@ def ui(self, *, id: Optional[str] = None, **kwargs): A UI component. """ - return mod_ui( - id or self.id, - preload_viz=has_viz_tool(self.tools), - greeting=self.greeting, - **kwargs, - ) + return mod_ui(id or self.id, preload_viz=has_viz_tool(self.tools), **kwargs) def server( self, @@ -914,12 +909,7 @@ def ui(self, *, id: Optional[str] = None, **kwargs): A UI component. """ - result = mod_ui( - id or self.id, - preload_viz=has_viz_tool(self.tools), - greeting=self.greeting, - **kwargs, - ) + result = mod_ui(id or self.id, preload_viz=has_viz_tool(self.tools), **kwargs) self._ensure_server_started() return result diff --git a/pkg-py/src/querychat/_shiny_module.py b/pkg-py/src/querychat/_shiny_module.py index 6d5b00c7..e7285f31 100644 --- a/pkg-py/src/querychat/_shiny_module.py +++ b/pkg-py/src/querychat/_shiny_module.py @@ -8,13 +8,11 @@ import chatlas import shinychat from narwhals.stable.v1.typing import IntoFrameT -from shinychat import Attachment, attachment_to_content from shiny import module, reactive, ui from ._querychat_core import warn_multi_table_flat_accessor from ._table_accessor import TableAccessor -from ._utils_shinychat import chat_greeting_persistent from ._viz_altair_widget import AltairWidget from ._viz_ggsql import execute_ggsql from ._viz_utils import has_viz_tool, preload_viz_deps_server, preload_viz_deps_ui @@ -70,14 +68,12 @@ class TableState(Generic[IntoFrameT]): @module.ui -def mod_ui(*, preload_viz: bool = False, greeting: str | None = None, **kwargs): +def mod_ui(*, preload_viz: bool = False, **kwargs): css_path = Path(__file__).parent / "static" / "css" / "styles.css" js_path = Path(__file__).parent / "static" / "js" / "querychat.js" kwargs.setdefault("enable_cancel", True) kwargs.setdefault("allow_attachments", True) - if greeting: - kwargs.setdefault("greeting", chat_greeting_persistent(greeting)) tag = shinychat.chat_ui(CHAT_ID, **kwargs) tag.add_class("querychat") @@ -217,14 +213,6 @@ def mod_server( greeter: QueryChatGreeter, greeting_base: chatlas.Chat | None = None, ) -> ServerValues[IntoFrameT]: - # Holds a generated greeting so it can be saved and restored on bookmark. - # Static greetings live in the UI (chat_ui(greeting=)) and persist already. - # Workaround for posit-dev/shinychat#253: shinychat does not bookmark - # greetings or expose their state. If that issue is fixed, this value, the - # get_last_turn() capture below, and the greeting handling in - # on_bookmark/on_restore can be dropped (and the shinychat minimum bumped). - current_greeting = ReactiveStringOrNone(None) - if not callable(client): raise TypeError("mod_server() requires a callable client factory.") @@ -309,48 +297,33 @@ def _stub_df(): if has_viz_tool(tools): preload_viz_deps_server() - # Chat UI logic - chat_ui = shinychat.Chat(CHAT_ID) - ctrl = chatlas.StreamController() - - @chat_ui.on_user_submit - async def _(user_input: str, attachments: list[Attachment]): - contents = [attachment_to_content(a) for a in attachments] - stream = await chat.stream_async( - user_input, *contents, echo="none", content="all", controller=ctrl + async def _make_greeting(): + # shinychat invokes this asynchronously, so there's no useful caller frame + # to point to — stacklevel=1 just points at this line. + warnings.warn( + "No greeting provided to `QueryChat()`. Using the LLM `client` to generate one now. " + "For faster startup, lower cost, and determinism, consider providing a greeting " + "to `QueryChat()` and `.generate_greeting()` to generate one beforehand.", + GreetWarning, + stacklevel=1, ) - await chat_ui.append_message_stream(stream) + stream = await greeter.generate_async(base=greeting_base) + return shinychat.chat_greeting(stream, persistent=True) - @reactive.effect - @reactive.event(input[f"{CHAT_ID}_cancel"]) - def _handle_cancel(): - ctrl.cancel() - - if greeting is None: - - @reactive.effect - @reactive.event(input[f"{CHAT_ID}_greeting_requested"]) - async def _handle_greeting_requested(): - # Re-display a restored greeting rather than generating a new one. - # On empty-chat restore both this and on_restore set the greeting - # (harmless, identical content); on non-empty restore this never - # fires, so on_restore is the only path that re-displays. - existing = current_greeting.get() - if existing is not None: - await chat_ui.set_greeting(chat_greeting_persistent(existing)) - return - warnings.warn( - "No greeting provided to `QueryChat()`. Using the LLM `client` to generate one now. " - "For faster startup, lower cost, and determinism, consider providing a greeting " - "to `QueryChat()` and `.generate_greeting()` to generate one beforehand.", - GreetWarning, - stacklevel=2, - ) - await greeter.generate_stream( - chat_ui=chat_ui, - current_greeting=current_greeting, - base=greeting_base, - ) + greeting_arg = ( + _make_greeting + if greeting is None + else shinychat.chat_greeting(greeting, persistent=True) + ) + + shinychat_chat = shinychat.Chat( + CHAT_ID, + client=chat, + greeting=greeting_arg, + ) + + if enable_bookmarking: + shinychat_chat.enable_bookmarking(chat) # Handle update button clicks @reactive.effect @@ -368,7 +341,6 @@ def _(): _current_table.set(table_name) if enable_bookmarking: - chat_ui.enable_bookmarking(chat) session.bookmark.exclude.append("chat_update") @session.bookmark.on_bookmark @@ -377,9 +349,6 @@ def _on_bookmark(x: BookmarkState) -> None: for name, state in table_states.items(): vals[f"querychat_sql_{name}"] = state.sql.get() vals[f"querychat_title_{name}"] = state.title.get() - greeting_val = current_greeting.get() - if greeting_val is not None: - vals["querychat_greeting"] = greeting_val if viz_widgets: vals["querychat_viz_widgets"] = viz_widgets @@ -396,11 +365,6 @@ async def _on_restore(x: RestoreState) -> None: state.title.set(vals[f"querychat_title_{name}"]) if last_restored is not None: _current_table.set(last_restored) - if "querychat_greeting" in vals: - current_greeting.set(vals["querychat_greeting"]) - await chat_ui.set_greeting( - chat_greeting_persistent(vals["querychat_greeting"]) - ) if "querychat_viz_widgets" in vals: restored = restore_viz_widgets(executor, vals["querychat_viz_widgets"]) viz_widgets[:] = restored diff --git a/pkg-py/src/querychat/_utils_shinychat.py b/pkg-py/src/querychat/_utils_shinychat.py deleted file mode 100644 index 5ce68b6b..00000000 --- a/pkg-py/src/querychat/_utils_shinychat.py +++ /dev/null @@ -1,19 +0,0 @@ -from __future__ import annotations - -from typing import TYPE_CHECKING, Any - -if TYPE_CHECKING: - from shinychat._chat_types import ChatGreeting - - -# TODO @gadenbuie: remove once shinychat >= 0.5.0 is the minimum (persistent added in 0.4.0.9000) -def chat_greeting_persistent(content: Any) -> ChatGreeting: - from importlib.metadata import version - - import shinychat - from packaging.version import Version - - if Version(version("shinychat")) > Version("0.4.0"): - return shinychat.chat_greeting(content, persistent=True) - else: - return shinychat.chat_greeting(content, dismissible=False) # pyright: ignore[reportArgumentType] diff --git a/pkg-py/tests/test_shiny_module.py b/pkg-py/tests/test_shiny_module.py index 812bb75e..4bd0a886 100644 --- a/pkg-py/tests/test_shiny_module.py +++ b/pkg-py/tests/test_shiny_module.py @@ -46,3 +46,70 @@ def test_mod_ui_allow_attachments_can_be_overridden(): with patch("querychat._shiny_module.shinychat.chat_ui", side_effect=_fake_chat_ui): mod_ui("test", allow_attachments=False) assert _fake_chat_ui.last_kwargs.get("allow_attachments") is False + + + +def _unwrap_module_server(decorated): + """ + Recover the undecorated function wrapped by @module.server. + + shiny's module.server decorator closes over the original function under the + freevar name "fn"; look it up by name rather than assuming a cell index, since + the closure's cell order isn't part of shiny's public contract. + """ + code = decorated.__code__ + idx = code.co_freevars.index("fn") + return decorated.__closure__[idx].cell_contents + + +def test_mod_server_passes_client_to_chat(): + """After migration, Chat is constructed with client= so manual wiring is gone.""" + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + + captured = {} + + fake_chat_instance = MagicMock() + + def fake_chat_constructor(id, *, client=None, greeting=None, **kwargs): + captured["client"] = client + captured["greeting"] = greeting + return fake_chat_instance + + # Minimal stubs + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + + with ( + patch("querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + enable_bookmarking=False, + tools=None, + greeter=MagicMock(), + ) + + assert captured.get("client") is not None, "client= should be passed to Chat" + assert callable(captured.get("greeting")), "greeting= should be a callable" + diff --git a/pkg-r/DESCRIPTION b/pkg-r/DESCRIPTION index 6c942282..577e6a9c 100644 --- a/pkg-r/DESCRIPTION +++ b/pkg-r/DESCRIPTION @@ -35,7 +35,7 @@ Imports: rlang (>= 1.1.0), S7, shiny, - shinychat (>= 0.4.0), + shinychat (> 0.4.0), utils, whisker, yaml diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 9e0731bc..0950a161 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -909,7 +909,7 @@ QueryChat <- R6::R6Class( id <- id %||% namespaced_id(self$id) - mod_ui(id, ..., greeting = self$greeting) + mod_ui(id, ...) }, #' @description diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index 224b46fe..ffb3a908 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -1,18 +1,5 @@ # Main module UI function -mod_ui <- function( - id, - ..., - greeting = NULL, - enable_cancel = TRUE, - allow_attachments = TRUE -) { - ns <- shiny::NS(id) - - if (!is.null(greeting) && any(nzchar(greeting))) { - greeting <- chat_greeting_persistent(greeting) - } else { - greeting <- NULL - } +mod_ui <- function(id, ...) { htmltools::tagList( htmltools::htmlDependency( @@ -24,12 +11,9 @@ mod_ui <- function( stylesheet = "styles.css" ), shinychat::chat_ui( - ns("chat"), + shiny::NS(id, "chat"), height = "100%", class = "querychat", - enable_cancel = enable_cancel, - allow_attachments = allow_attachments, - greeting = greeting, ... ) ) @@ -49,13 +33,6 @@ mod_server <- function( ) { shiny::moduleServer(id, function(input, output, session) { current_table_val <- shiny::reactiveVal(NULL, label = "current_table") - # Holds a generated greeting so it can be saved and restored on bookmark. - # Static greetings live in the UI (chat_ui(greeting=)) and persist already. - # Workaround for posit-dev/shinychat#253: shinychat does not bookmark - # greetings or expose their state. If that issue is fixed, this reactiveVal, - # the last_turn() capture below, and the greeting handling in - # onBookmark/onRestore can be dropped (and the shinychat minimum bumped). - current_greeting <- shiny::reactiveVal(NULL, label = "current_greeting") # Per-table reactive state tables <- list() @@ -83,17 +60,6 @@ mod_server <- function( }) } - append_output <- function(...) { - txt <- paste0(...) - shinychat::chat_append_message( - "chat", - list(role = "assistant", content = txt), - chunk = TRUE, - operation = "append", - session = session - ) - } - update_dashboard <- function(query, title, table) { if (!is.null(query)) { tables[[table]]$sql(query) @@ -116,9 +82,7 @@ mod_server <- function( ) } - # Non-reactive bookkeeping for bookmark save/restore of viz widgets viz_widgets <- list() - on_visualize <- function(data) { viz_widgets[[length(viz_widgets) + 1L]] <<- list( widget_id = data$widget_id, @@ -126,9 +90,8 @@ mod_server <- function( ) } - # Set up the chat object for this session check_function(client) - chat <- client( + pre_built_client <- client( update_dashboard = update_dashboard, reset_dashboard = reset_query, visualize = on_visualize, @@ -136,84 +99,41 @@ mod_server <- function( session = session ) - if (is.null(greeting)) { - shiny::observeEvent( - input$chat_greeting_requested, - label = "on_greeting_requested", - { - # Re-display a restored greeting rather than generating a new one. - # On empty-chat restore both this and onRestore set the greeting - # (harmless, identical content); on non-empty restore this never fires, - # so onRestore is the only path that re-displays. - if (!is.null(current_greeting())) { - shinychat::chat_set_greeting( - "chat", - chat_greeting_persistent(current_greeting()) - ) - return() - } - cli::cli_warn(c( - "No {.arg greeting} provided to {.fn QueryChat}. Using the LLM {.arg client} to generate one now.", - "i" = "For faster startup, lower cost, and determinism, consider providing a {.arg greeting} to {.fn QueryChat}.", - "i" = "You can use your {.help querychat::QueryChat} object's {.fn $generate_greeting} method to generate a greeting." - )) - greeter$generate_stream( - greeting_reactive = current_greeting, - base = greeting_base - ) - } - ) + greeting_arg <- if (is.null(greeting)) { + function() { + cli::cli_warn(c( + "No {.arg greeting} provided to {.fn QueryChat}. Using the LLM {.arg client} to generate one now.", + "i" = "For faster startup, lower cost, and determinism, consider providing a {.arg greeting} to {.fn QueryChat}.", + "i" = "You can use your {.help querychat::QueryChat} object's {.fn $generate_greeting} method to generate a greeting." + )) + greeting_client <- greeter$build_client(greeting_base) + stream <- greeting_client$stream_async(GREETING_PROMPT) + shinychat::chat_greeting(stream, persistent = TRUE) + } + } else { + shinychat::chat_greeting(greeting, persistent = TRUE) } - ctrl <- ellmer::stream_controller() + chat_module <- shinychat::chat_server("chat", pre_built_client, greeting = greeting_arg) - append_stream_task <- shiny::ExtendedTask$new( - function(client, user_input, controller = NULL) { - user_input_parts <- if (is.list(user_input)) { - user_input - } else { - list(user_input) + shiny::observeEvent( + input$chat_update, + label = "on_chat_update", + { + upd <- input$chat_update + tbl <- upd$table + if (!is.null(tbl) && tbl %in% names(tables)) { + q <- upd$query + ttl <- upd$title + tables[[tbl]]$sql(if (nzchar(q %||% "")) q else NULL) + tables[[tbl]]$title(if (nzchar(ttl %||% "")) ttl else NULL) + current_table_val(tbl) } - stream <- client$stream_async( - !!!user_input_parts, - stream = "content", - controller = controller - ) - - p <- promises::promise_resolve(stream) - promises::then(p, function(stream) { - shinychat::chat_append("chat", stream) - }) } ) - shiny::observeEvent(input$chat_user_input, label = "on_chat_user_input", { - append_stream_task$invoke(chat, input$chat_user_input, controller = ctrl) - }) - - shiny::observeEvent(input$chat_cancel, label = "on_chat_cancel", { - ctrl$cancel() - }) - - shiny::observeEvent(input$chat_update, label = "on_chat_update", { - tbl <- input$chat_update$table - if (!is.null(tbl) && tbl %in% names(tables)) { - q <- input$chat_update$query - ttl <- input$chat_update$title - tables[[tbl]]$sql(if (nzchar(q %||% "")) q else NULL) - tables[[tbl]]$title(if (nzchar(ttl %||% "")) ttl else NULL) - current_table_val(tbl) - } - }) - if (enable_bookmarking) { - shinychat::chat_restore( - "chat", - chat, - restore_ui = FALSE, - session = session - ) - shiny::setBookmarkExclude("chat_update", session = session) + shiny::setBookmarkExclude("chat_update") shiny::onBookmark(function(state) { table_states <- list() @@ -224,9 +144,6 @@ mod_server <- function( ) } state$values$querychat_tables <- table_states - if (!is.null(current_greeting())) { - state$values$querychat_greeting <- current_greeting() - } if (length(viz_widgets) > 0) { state$values$querychat_viz_widgets <- viz_widgets } @@ -249,14 +166,6 @@ mod_server <- function( current_table_val(last_restored) } } - if (!is.null(state$values$querychat_greeting)) { - current_greeting(state$values$querychat_greeting) - shinychat::chat_set_greeting( - "chat", - chat_greeting_persistent(state$values$querychat_greeting), - session = session - ) - } if (!is.null(state$values$querychat_viz_widgets)) { restored <- restore_viz_widgets( executor, @@ -280,11 +189,10 @@ mod_server <- function( table_names_fn <- function() names(tables) - # Backward compat: for single-table, expose sql/title/df directly if (length(data_sources) == 1) { first <- tables[[1]] list( - client = chat, + client = chat_module$client, sql = first$sql, title = first$title, df = first$df, @@ -302,7 +210,7 @@ mod_server <- function( } } list( - client = chat, + client = chat_module$client, sql = single_table_error("sql"), title = single_table_error("title"), df = single_table_error("df"), diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index 040db90f..e1adb9f8 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -24,7 +24,13 @@ test_that("mod_server() return includes table() and table_names() for single-tab executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) structure(list(), class = "MockChat") + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) + + local_mocked_bindings( + chat_server = function(id, client, ...) list(client = client), + .package = "shinychat" + ) shiny::testServer( mod_server, @@ -38,12 +44,14 @@ test_that("mod_server() return includes table() and table_names() for single-tab enable_bookmarking = FALSE ), { - # table_names_fn() returns the table name vector - expect_equal(table_names_fn(), "test_table") + # Verify the returned list exposes table() and table_names() + expect_true(is.function(session$returned$table)) + expect_true(is.function(session$returned$table_names)) + expect_equal(session$returned$table_names(), "test_table") - # table_fn() returns a TableAccessor backed by reactive state - acc <- table_fn("test_table") - expect_true(inherits(acc, "TableAccessor")) + # table() returns a TableAccessor backed by reactive state + acc <- session$returned$table("test_table") + expect_s3_class(acc, "TableAccessor") expect_equal(acc$table_name, "test_table") # TableAccessor$df() works (returns the full data frame when no filter set) @@ -51,18 +59,11 @@ test_that("mod_server() return includes table() and table_names() for single-tab expect_equal(nrow(df_result), 5L) # Single-table backward compat: first$df/sql/title are still in the return - first_state <- tables[[1]] + first_state <- session$returned$.tables[[1]] expect_true(is.function(first_state$df)) expect_true(is.function(first_state$sql)) expect_true(is.function(first_state$title)) - # Verify the returned list exposes table() and table_names() - expect_true(is.function(session$returned$table)) - expect_true(is.function(session$returned$table_names)) - acc <- session$returned$table("test_table") - expect_s3_class(acc, "TableAccessor") - expect_equal(session$returned$table_names(), "test_table") - # Verify backward-compat reactive accessors on the returned list expect_true(is.function(session$returned$df)) expect_true(is.function(session$returned$sql)) @@ -83,9 +84,14 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl result <- NULL client_factory <- function(...) { result <<- "client_called" - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } + local_mocked_bindings( + chat_server = function(id, client, ...) list(client = client), + .package = "shinychat" + ) + shiny::testServer( mod_server, args = list( @@ -98,34 +104,25 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl enable_bookmarking = FALSE ), { - # table_names_fn() returns all registered table names - expect_equal(table_names_fn(), c("tbl_a", "tbl_b")) + # Verify the returned list exposes table() and table_names() + expect_true(is.function(session$returned$table)) + expect_true(is.function(session$returned$table_names)) + expect_equal(sort(session$returned$table_names()), c("tbl_a", "tbl_b")) - # table_fn() returns a TableAccessor for each table - acc_a <- table_fn("tbl_a") - expect_true(inherits(acc_a, "TableAccessor")) + # table() returns a TableAccessor for each table + acc_a <- session$returned$table("tbl_a") + expect_s3_class(acc_a, "TableAccessor") expect_equal(acc_a$table_name, "tbl_a") - acc_b <- table_fn("tbl_b") - expect_true(inherits(acc_b, "TableAccessor")) + acc_b <- session$returned$table("tbl_b") + expect_s3_class(acc_b, "TableAccessor") expect_equal(acc_b$table_name, "tbl_b") - # table_fn() errors for unknown names - expect_error(table_fn("nonexistent"), class = "rlang_error") + # table() errors for unknown names + expect_error(session$returned$table("nonexistent"), "not found") # Multi-table: single_table_error functions mention qc_vals$table() - single_err <- single_table_error("sql") - expect_error(single_err(), regexp = "qc_vals\\$table") - - # Verify the returned list exposes table() and table_names() - expect_true(is.function(session$returned$table)) - expect_true(is.function(session$returned$table_names)) - acc <- session$returned$table("tbl_a") - expect_s3_class(acc, "TableAccessor") - expect_equal(sort(session$returned$table_names()), c("tbl_a", "tbl_b")) - - # Verify error is surfaced through the public API - expect_error(session$returned$table("nonexistent"), "not found") + expect_error(session$returned$sql(), regexp = "qc_vals\\$table") } ) }) @@ -140,9 +137,14 @@ test_that("mod_server() passes visualize callback and tools to client factory", client_factory <- function(...) { captured <<- list(...) - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } + local_mocked_bindings( + chat_server = function(id, client, ...) list(client = client), + .package = "shinychat" + ) + shiny::testServer( mod_server, args = list( @@ -171,7 +173,13 @@ test_that("mod_server() exposes current_table() starting as NULL", { executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) structure(list(), class = "MockChat") + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) + + local_mocked_bindings( + chat_server = function(id, client, ...) list(client = client), + .package = "shinychat" + ) shiny::testServer( mod_server, @@ -203,9 +211,14 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu captured_callbacks <- NULL client_factory <- function(...) { captured_callbacks <<- list(...) - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } + local_mocked_bindings( + chat_server = function(id, client, ...) list(client = client), + .package = "shinychat" + ) + shiny::testServer( mod_server, args = list( @@ -238,23 +251,23 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu ) }) -test_that("mod_ui() passes allow_attachments = TRUE to shinychat by default", { +test_that("mod_ui() does not force allow_attachments, deferring to chat_ui default", { captured <- NULL local_mocked_bindings( - chat_ui = function(...) { + chat_ui = function(id, ...) { captured <<- list(...) htmltools::div() }, .package = "shinychat" ) mod_ui("test") - expect_true(isTRUE(captured$allow_attachments)) + expect_false("allow_attachments" %in% names(captured)) }) test_that("mod_ui() passes allow_attachments = FALSE when requested", { captured <- NULL local_mocked_bindings( - chat_ui = function(...) { + chat_ui = function(id, ...) { captured <<- list(...) htmltools::div() }, @@ -264,6 +277,32 @@ test_that("mod_ui() passes allow_attachments = FALSE when requested", { expect_false(isTRUE(captured$allow_attachments)) }) +test_that("mod_ui() calls chat_ui with NS(id, 'chat')", { + captured <- NULL + local_mocked_bindings( + chat_ui = function(id, ...) { + captured <<- list(id = id, ...) + htmltools::div() + }, + .package = "shinychat" + ) + mod_ui("mymod") + expect_equal(captured$id, shiny::NS("mymod", "chat")) +}) + +test_that("mod_ui() passes enable_cancel through to chat_ui without warning", { + captured <- NULL + local_mocked_bindings( + chat_ui = function(id, ...) { + captured <<- list(...) + htmltools::div() + }, + .package = "shinychat" + ) + expect_no_warning(mod_ui("test", enable_cancel = FALSE)) + expect_false(isTRUE(captured$enable_cancel)) +}) + test_that("restored viz widgets survive a second bookmark cycle", { skip_if_no_dataframe_engine() @@ -277,18 +316,18 @@ test_that("restored viz widgets survive a second bookmark cycle", { client_factory <- function(...) { callbacks <<- list(...) - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } local_mocked_bindings( - chat_restore = function(id, chat, ..., session) {}, + chat_server = function(id, client, ...) list(client = client), .package = "shinychat" ) local_mocked_bindings( - onBookmark = function(fun) { + onBookmark = function(fun, session = NULL) { bookmark_fn <<- fun }, - onRestore = function(fun) { + onRestore = function(fun, session = NULL) { restore_fn <<- fun }, .package = "shiny" @@ -305,6 +344,72 @@ test_that("restored viz widgets survive a second bookmark cycle", { .package = "querychat" ) + expect_no_warning( + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = c("query", "visualize"), + enable_bookmarking = TRUE + ), + { + expect_true(is.function(bookmark_fn)) + expect_true(is.function(restore_fn)) + expect_true(is.function(callbacks$visualize)) + + saved <- list( + list( + widget_id = "querychat_viz_1", + ggsql = "SELECT 1 VISUALISE 1 AS x DRAW point" + ) + ) + + shiny::isolate(callbacks$visualize(saved[[1]])) + + first_state <- new.env(parent = emptyenv()) + first_state$values <- list() + shiny::isolate(bookmark_fn(first_state)) + expect_equal(first_state$values$querychat_viz_widgets, saved) + + restore_state <- new.env(parent = emptyenv()) + restore_state$values <- first_state$values + shiny::isolate(restore_fn(restore_state)) + expect_true(inherits(restored_args$executor, "QueryExecutor")) + expect_equal(restored_args$saved_widgets, saved) + + second_state <- new.env(parent = emptyenv()) + second_state$values <- list() + shiny::isolate(bookmark_fn(second_state)) + expect_equal(second_state$values$querychat_viz_widgets, saved) + } + ), + class = "lifecycle_warning_deprecated" + ) +}) + +test_that("mod_server() calls chat_server('chat', ...) with the pre-built client", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + captured_chat_args <- NULL + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) + + local_mocked_bindings( + chat_server = function(id, client, ...) { + captured_chat_args <<- list(id = id, client = client) + list(client = client) + }, + .package = "shinychat" + ) + shiny::testServer( mod_server, args = list( @@ -313,38 +418,125 @@ test_that("restored viz widgets survive a second bookmark cycle", { executor = executor, greeting = "Hello", client = client_factory, - tools = c("query", "visualize"), - enable_bookmarking = TRUE + tools = "query", + enable_bookmarking = FALSE ), { - expect_true(is.function(bookmark_fn)) - expect_true(is.function(restore_fn)) - expect_true(is.function(callbacks$visualize)) - - saved <- list( - list( - widget_id = "querychat_viz_1", - ggsql = "SELECT 1 VISUALISE 1 AS x DRAW point" - ) - ) + expect_equal(captured_chat_args$id, "chat") + expect_true(inherits(captured_chat_args$client, "Chat")) + } + ) +}) + +test_that("mod_server() builds the auto-generated greeting from the greeter, not the main client", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + main_client_calls <- list() + client_factory <- function(...) { + main_client_calls[[length(main_client_calls) + 1L]] <<- list(...) + structure(list(), class = c("MockChat", "Chat")) + } + + greeting_stream_prompt <- NULL + fake_greeting_client <- list( + stream_async = function(prompt) { + greeting_stream_prompt <<- prompt + "fake-stream" + } + ) + + build_client_calls <- list() + fake_greeter <- list( + build_client = function(base = NULL) { + build_client_calls[[length(build_client_calls) + 1L]] <<- base + fake_greeting_client + } + ) + + captured_greeting_arg <- NULL + local_mocked_bindings( + chat_server = function(id, client, greeting = NULL, ...) { + captured_greeting_arg <<- greeting + list(client = client) + }, + chat_greeting = function(content, ...) content, + .package = "shinychat" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = NULL, + client = client_factory, + tools = "query", + greeter = fake_greeter, + greeting_base = "base-client", + enable_bookmarking = FALSE + ), + { + expect_true(is.function(captured_greeting_arg)) + suppressWarnings(captured_greeting_arg()) + + # The greeting client must come from the greeter (configured with the + # dedicated greeting prompt), not from a second call to the main client + # factory with tools = NULL (which would use the query system prompt). + expect_equal(length(build_client_calls), 1L) + expect_equal(build_client_calls[[1]], "base-client") + expect_equal(length(main_client_calls), 1L) + expect_false(is.null(greeting_stream_prompt)) + } + ) +}) - shiny::isolate(callbacks$visualize(saved[[1]])) +test_that("mod_server() chat_update input updates table state", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) - first_state <- new.env(parent = emptyenv()) - first_state$values <- list() - shiny::isolate(bookmark_fn(first_state)) - expect_equal(first_state$values$querychat_viz_widgets, saved) + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) - restore_state <- new.env(parent = emptyenv()) - restore_state$values <- first_state$values - shiny::isolate(restore_fn(restore_state)) - expect_true(inherits(restored_args$executor, "QueryExecutor")) - expect_equal(restored_args$saved_widgets, saved) + local_mocked_bindings( + chat_server = function(id, client, ...) list(client = client), + .package = "shinychat" + ) - second_state <- new.env(parent = emptyenv()) - second_state$values <- list() - shiny::isolate(bookmark_fn(second_state)) - expect_equal(second_state$values$querychat_viz_widgets, saved) + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + enable_bookmarking = FALSE + ), + { + session$setInputs( + chat_update = list( + table = "test_table", + query = "SELECT * FROM test_table WHERE cyl = 4", + title = "4-cylinder cars" + ) + ) + expect_equal( + shiny::isolate(session$returned$sql()), + "SELECT * FROM test_table WHERE cyl = 4" + ) + expect_equal( + shiny::isolate(session$returned$current_table()), + "test_table" + ) } ) }) From d25afb6925bc49a1ae46714934775abcceda27b9 Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 1 Jul 2026 09:21:08 -0500 Subject: [PATCH 02/16] refactor: unify enable_bookmarking/bookmark_store into a single history param Both packages previously configured chat history persistence through two separate, overlapping knobs (enable_bookmarking and bookmark_store). This collapses them into a single history parameter (bool or HistoryOptions) that mod_server resolves once and uses to register both table and viz state for bookmarking/restore. - Python: QueryChatBase stores history verbatim; mod_server resolves it and dual-registers table/viz state; enable_bookmarking/bookmark_store are deprecated in favor of history, with changelog entries and updated playwright fixtures/tests. - R: mod_server takes the same history param and dual-registers table/viz state; enable_bookmarking/bookmark_store are deprecated the same way, with NEWS entries and test coverage for history resolution, the deprecation path, and app() inference. Dead generate_stream/ utils-shinychat.R code is removed. - Always calls Chat.enable_bookmarking() / registers chat_restore() hooks rather than conditionally, so history restore no longer depends on a legacy shim. - Pins shinychat to the feat/chat-history branch, which provides the history-aware bookmarking hooks this work depends on. - Follow-up fix: use Optional[...] instead of `| None` for the history parameter type, matching this file's existing typing convention. --- pkg-py/CHANGELOG.md | 8 + pkg-py/src/querychat/_querychat_base.py | 3 + pkg-py/src/querychat/_shiny.py | 165 +++++----- pkg-py/src/querychat/_shiny_module.py | 90 ++++-- .../tests/playwright/apps/viz_bookmark_app.py | 3 +- pkg-py/tests/test_base.py | 17 ++ pkg-py/tests/test_shiny.py | 155 ++++++++++ pkg-py/tests/test_shiny_module.py | 131 +++++++- pkg-r/DESCRIPTION | 2 +- pkg-r/NEWS.md | 8 + pkg-r/R/QueryChat.R | 80 +++-- pkg-r/R/QueryChatGreeter.R | 22 -- pkg-r/R/querychat_module.R | 121 +++++--- pkg-r/R/utils-check.R | 26 ++ pkg-r/R/utils-shinychat.R | 10 - pkg-r/man/QueryChat.Rd | 33 +- pkg-r/man/querychat-convenience.Rd | 8 +- pkg-r/tests/testthat/_snaps/QueryChat.md | 13 + pkg-r/tests/testthat/helper-fixtures.R | 26 ++ pkg-r/tests/testthat/test-QueryChat.R | 109 ++++++- pkg-r/tests/testthat/test-querychat_module.R | 289 ++++++++++++++---- pyproject.toml | 2 +- 22 files changed, 1035 insertions(+), 286 deletions(-) create mode 100644 pkg-py/tests/test_shiny.py delete mode 100644 pkg-r/R/utils-shinychat.R diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index 03fdbc7c..5239e87a 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -41,6 +41,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The `data_source` property has been removed. Use `qc.table("name").data_source` to read a table's data source, and `qc.add_table(df, "name", replace=True)` to replace it. The `data_source` parameter to `server()` (Shiny) has also been removed; call `add_table()` before `server()` instead. (#195) +* `.app()`'s `bookmark_store` parameter has been removed. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `.app()` defaults to `restore_mode="bookmark"` when no `history` is set anywhere, so existing `.app()` callers keep working without changes. + +* `QueryChat`/`QueryChatExpress` now default to `history=True` (shinychat's own default, browser-localStorage-backed conversation history) instead of no persistence. Pass `history=False` to opt back out. + +### Deprecated + +* `.server()`'s and `QueryChatExpress`'s `enable_bookmarking` parameter is deprecated in favor of `history`. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` instead of `enable_bookmarking=True` for the equivalent behavior. + ### Improvements * Chat greetings now use shinychat's greeting API (requires shinychat >= 0.4.0). A provided `greeting` renders instantly when the app loads, and when no `greeting` is given one is generated on demand — now **schema-aware**, so it can describe the data it's about to help you explore — without being added to the conversation history. Generated greetings are preserved across bookmark/restore. Tables passed to `QueryChat()` are described in the greeting automatically; opt additional tables in with `include_in_greeting=True` on `add_table()`/`add_tables()`, or fine-tune which tables and which template the greeting uses via `qc.greeter`. (#249, #261) diff --git a/pkg-py/src/querychat/_querychat_base.py b/pkg-py/src/querychat/_querychat_base.py index d0fd3d4a..d98ad94f 100644 --- a/pkg-py/src/querychat/_querychat_base.py +++ b/pkg-py/src/querychat/_querychat_base.py @@ -57,6 +57,7 @@ from ibis.backends.sql import SQLBackend from narwhals.stable.v1.typing import IntoFrame from pins.boards import BaseBoard + from shinychat.types import HistoryOptions from ._data_dict import DataDict from ._viz_tools import VisualizeData @@ -91,6 +92,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ): self._data_dicts: list[DataDict] = _normalize_data_dicts(data_dict) @@ -103,6 +105,7 @@ def __init__( self.tools = normalize_tools(tools, default=DEFAULT_TOOLS) self.greeting = greeting.read_text() if isinstance(greeting, Path) else greeting + self.history = history # Store init parameters for deferred system prompt building self._prompt_template = prompt_template diff --git a/pkg-py/src/querychat/_shiny.py b/pkg-py/src/querychat/_shiny.py index a5974c5e..6926f2ed 100644 --- a/pkg-py/src/querychat/_shiny.py +++ b/pkg-py/src/querychat/_shiny.py @@ -1,10 +1,12 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any, ClassVar, Literal, Optional, overload +import warnings +from typing import TYPE_CHECKING, Any, Literal, Optional, overload from narwhals.stable.v1.typing import IntoDataFrameT, IntoFrameT, IntoLazyFrameT from shiny.express._stub_session import ExpressStubSession from shiny.session import get_current_session +from shinychat.types import HistoryOptions from shiny import App, Inputs, Outputs, Session, reactive, render, req, ui @@ -159,6 +161,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -176,6 +179,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -193,6 +197,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -210,6 +215,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -227,6 +233,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... def __init__( @@ -243,6 +250,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ): super().__init__( data_source, @@ -255,11 +263,14 @@ def __init__( categorical_threshold=categorical_threshold, extra_instructions=extra_instructions, prompt_template=prompt_template, + history=history, ) self.id = id or (f"querychat_{table_name}" if table_name else "querychat") def app( - self, *, bookmark_store: Literal["url", "server", "disable"] = "url" + self, + *, + history: Optional[bool | HistoryOptions] = None, ) -> App: """ Quickly chat with a dataset. @@ -269,11 +280,16 @@ def app( Parameters ---------- - bookmark_store - The bookmarking store to use for the Shiny app. Options are: - - `"url"`: Store bookmarks in the URL (default). - - `"server"`: Store bookmarks on the server. - - `"disable"`: Disable bookmarking. + history + Conversation history configuration, passed through to + `shinychat.Chat(history=)`. Defaults to + `shinychat.types.HistoryOptions(restore_mode="bookmark")` when neither + this nor the constructor's `history` was set, since `.app()`'s whole + purpose is a single, shareable demo (chat + SQL editor + table state + tied together). When the resolved value has `restore_mode="bookmark"`, + the generated app automatically enables Shiny's own server-side + bookmarking (`bookmark_store="server"`) -- shinychat's `history` + mechanism only wires the chat side, not the app-level Shiny setting. Returns ------- @@ -282,7 +298,19 @@ def app( """ self._require_initialized("app") - enable_bookmarking = bookmark_store != "disable" + resolved_history: bool | HistoryOptions = ( + history + if history is not None + else ( + self.history + if self.history is not None + else HistoryOptions(restore_mode="bookmark") + ) + ) + enable_bookmarking = ( + isinstance(resolved_history, HistoryOptions) + and resolved_history.restore_mode == "bookmark" + ) first_table_name = next(iter(self._data_sources)) def app_ui(request): @@ -330,7 +358,7 @@ def app_server(input: Inputs, output: Outputs, session: Session): executor=self._require_query_executor("server"), greeting=self.greeting, client=self._create_session_client, - enable_bookmarking=enable_bookmarking, + history=resolved_history, tools=self.tools, greeter=self.greeter, greeting_base=None, @@ -401,7 +429,11 @@ def _(): query if query and query.strip() != default_query else None ) - return App(app_ui, app_server, bookmark_store=bookmark_store) + return App( + app_ui, + app_server, + bookmark_store="server" if enable_bookmarking else "disable", + ) def sidebar( self, @@ -468,7 +500,8 @@ def server( self, *, client: str | chatlas.Chat | MISSING_TYPE = MISSING, - enable_bookmarking: bool = False, + history: Optional[bool | HistoryOptions] = None, + enable_bookmarking: bool | None = None, id: Optional[str] = None, ) -> ServerValues[IntoFrameT]: """ @@ -487,52 +520,19 @@ def server( for the deferred pattern where the client cannot be created at initialization time (e.g., when using Posit Connect managed OAuth credentials that require session access). + history + Conversation history configuration, passed through to + `shinychat.Chat(history=)`. Overrides the value set on the `QueryChat` + constructor for this call only. Defaults to `True` when neither this + nor the constructor's `history` was set. enable_bookmarking - Whether to enable bookmarking for the querychat module. + Deprecated. Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` + instead (set on the `QueryChat` constructor, or passed here). id Optional module ID for the QueryChat instance. If not provided, will use the ID provided at initialization. This must match the ID used in the `.ui()` or `.sidebar()` methods. - Examples - -------- - ```python - from shiny import App, render, ui - from seaborn import load_dataset - from querychat import QueryChat - - titanic = load_dataset("titanic") - - qc = QueryChat(titanic, "titanic") - - - def app_ui(request): - return ui.page_sidebar( - qc.sidebar(), - ui.card( - ui.card_header(ui.output_text("title")), - ui.output_data_frame("data_table"), - ), - title="Titanic QueryChat App", - fillable=True, - ) - - - def server(input, output, session): - qc_vals = qc.server(enable_bookmarking=True) - - @render.data_frame - def data_table(): - return qc_vals.df() - - @render.text - def title(): - return qc_vals.title() or "My Data" - - - app = App(app_ui, server, bookmark_store="url") - ``` - Returns ------- : @@ -555,6 +555,21 @@ def title(): def create_session_client(**kwargs) -> chatlas.Chat: return self._create_session_client(base=resolved_client, **kwargs) + resolved_history: bool | HistoryOptions = ( + history + if history is not None + else (self.history if self.history is not None else True) + ) + + if enable_bookmarking is not None: + warnings.warn( + "`enable_bookmarking` is deprecated and no longer has any effect. " + 'Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` ' + "instead (on the QueryChat constructor or passed to .server()).", + FutureWarning, + stacklevel=2, + ) + self._mark_server_initialized() return mod_server( id or self.id, @@ -562,7 +577,7 @@ def create_session_client(**kwargs) -> chatlas.Chat: executor=self._require_query_executor("server"), greeting=self.greeting, client=create_session_client, - enable_bookmarking=enable_bookmarking, + history=resolved_history, tools=self.tools, greeter=self.greeter, greeting_base=resolved_client, @@ -582,11 +597,12 @@ class QueryChatExpress(QueryChatBase[IntoFrameT]): ```python from querychat.express import QueryChat from seaborn import load_dataset - from shiny.express import app_opts, render, ui + from shiny.express import render, ui + from shinychat.types import HistoryOptions titanic = load_dataset("titanic") - qc = QueryChat(titanic, "titanic") + qc = QueryChat(titanic, "titanic", history=HistoryOptions(restore_mode="bookmark")) qc.sidebar() with ui.card(fill=True): @@ -605,8 +621,6 @@ def data_table(): title="Titanic QueryChat App", fillable=True, ) - - app_opts(bookmark_store="url") ``` Parameters @@ -670,9 +684,6 @@ def data_table(): """ - # Class-level cache for bookmarking settings detected during stub session - _bookmarking_settings: ClassVar[dict[str, bool]] = {} - @overload def __init__( self: QueryChatExpress[Any], @@ -688,6 +699,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -706,6 +718,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -724,6 +737,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -742,6 +756,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -760,6 +775,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -777,6 +793,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ): # Sanity check: Express should always have a (stub/real) session @@ -798,26 +815,19 @@ def __init__( categorical_threshold=categorical_threshold, extra_instructions=extra_instructions, prompt_template=prompt_template, + history=history, ) self.id = id or (f"querychat_{table_name}" if table_name else "querychat") - # Determine bookmarking setting - # During stub session: detect from app_opts and cache in class variable - # During real session: retrieve from class variable - enable: bool - if enable_bookmarking == "auto": - if isinstance(session, ExpressStubSession): - store = session.app_opts.get("bookmark_store", "disable") - enable = store != "disable" - # Cache for the real session - QueryChatExpress._bookmarking_settings[self.id] = enable - else: - # Retrieve and clean up (pop prevents memory accumulation) - enable = QueryChatExpress._bookmarking_settings.pop(self.id, False) - else: - enable = enable_bookmarking + if enable_bookmarking != "auto": + warnings.warn( + "`enable_bookmarking` is deprecated and no longer has any effect. " + 'Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` ' + "instead.", + FutureWarning, + stacklevel=2, + ) - self._enable_bookmarking = enable self._vals: ServerValues[IntoFrameT] | None = None def _ensure_server_started(self) -> None: @@ -838,13 +848,16 @@ def _ensure_server_started(self) -> None: return self._require_initialized("_ensure_server_started") self._mark_server_initialized() + resolved_history: bool | HistoryOptions = ( + self.history if self.history is not None else True + ) self._vals = mod_server( self.id, data_sources=dict(self._data_sources), executor=self._require_query_executor("_ensure_server_started"), greeting=self.greeting, client=self._create_session_client, - enable_bookmarking=self._enable_bookmarking, + history=resolved_history, tools=self.tools, greeter=self.greeter, greeting_base=None, diff --git a/pkg-py/src/querychat/_shiny_module.py b/pkg-py/src/querychat/_shiny_module.py index e7285f31..decf9193 100644 --- a/pkg-py/src/querychat/_shiny_module.py +++ b/pkg-py/src/querychat/_shiny_module.py @@ -3,7 +3,7 @@ import warnings from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Generic, TypedDict, Union +from typing import TYPE_CHECKING, Any, Generic, TypedDict, Union import chatlas import shinychat @@ -21,6 +21,7 @@ from collections.abc import Callable from shiny.bookmark import BookmarkState, RestoreState + from shinychat.types import HistoryOptions from shiny import Inputs, Outputs, Session @@ -208,7 +209,7 @@ def mod_server( executor: QueryExecutor | None, greeting: str | None, client: Callable[..., chatlas.Chat], - enable_bookmarking: bool, + history: bool | HistoryOptions, tools: set[str] | None = None, greeter: QueryChatGreeter, greeting_base: chatlas.Chat | None = None, @@ -320,10 +321,16 @@ async def _make_greeting(): CHAT_ID, client=chat, greeting=greeting_arg, + history=history, ) - if enable_bookmarking: - shinychat_chat.enable_bookmarking(chat) + # Registered unconditionally so the chat client's own state (and greeting) + # round-trip through Shiny bookmarks whenever the host app has bookmarking + # enabled -- independent of `history`. Only the save/restore hooks are + # wanted here; the automatic bookmark trigger is turned off (bookmark_on= + # None) so `history` (or the host app) remains the sole source of *when* + # to bookmark. + shinychat_chat.enable_bookmarking(chat, bookmark_on=None) # Handle update button clicks @reactive.effect @@ -340,34 +347,53 @@ def _(): table_states[table_name].title.set(new_title) _current_table.set(table_name) - if enable_bookmarking: - session.bookmark.exclude.append("chat_update") - - @session.bookmark.on_bookmark - def _on_bookmark(x: BookmarkState) -> None: - vals = x.values - for name, state in table_states.items(): - vals[f"querychat_sql_{name}"] = state.sql.get() - vals[f"querychat_title_{name}"] = state.title.get() - if viz_widgets: - vals["querychat_viz_widgets"] = viz_widgets - - @session.bookmark.on_restore - async def _on_restore(x: RestoreState) -> None: - vals = x.values - last_restored: str | None = None - for name, state in table_states.items(): - if f"querychat_sql_{name}" in vals: - state.sql.set(vals[f"querychat_sql_{name}"]) - if vals[f"querychat_sql_{name}"] is not None: - last_restored = name - if f"querychat_title_{name}" in vals: - state.title.set(vals[f"querychat_title_{name}"]) - if last_restored is not None: - _current_table.set(last_restored) - if "querychat_viz_widgets" in vals: - restored = restore_viz_widgets(executor, vals["querychat_viz_widgets"]) - viz_widgets[:] = restored + def build_state_snapshot() -> dict[str, Any]: + snapshot: dict[str, Any] = {} + for name, state in table_states.items(): + snapshot[f"querychat_sql_{name}"] = state.sql.get() + snapshot[f"querychat_title_{name}"] = state.title.get() + if viz_widgets: + snapshot["querychat_viz_widgets"] = list(viz_widgets) + return snapshot + + def apply_state_snapshot(vals: dict[str, Any]) -> None: + last_restored: str | None = None + for name, state in table_states.items(): + if f"querychat_sql_{name}" in vals: + state.sql.set(vals[f"querychat_sql_{name}"]) + if vals[f"querychat_sql_{name}"] is not None: + last_restored = name + if f"querychat_title_{name}" in vals: + state.title.set(vals[f"querychat_title_{name}"]) + if last_restored is not None: + _current_table.set(last_restored) + if "querychat_viz_widgets" in vals: + restored = restore_viz_widgets(executor, vals["querychat_viz_widgets"]) + viz_widgets[:] = restored + + # Registered unconditionally: both registrations are no-ops unless the + # corresponding persistence layer (Shiny's own bookmarking, or shinychat's + # history) is actually active in the host app. In restore_mode="bookmark", + # shinychat's history-save triggers a real Shiny bookmark internally, so both + # callbacks fire for the same event -- the snapshot is written twice, + # harmlessly, since it's idempotent. + session.bookmark.exclude.append("chat_update") + + @session.bookmark.on_bookmark + def _on_bookmark(x: BookmarkState) -> None: + x.values.update(build_state_snapshot()) + + @session.bookmark.on_restore + def _on_restore(x: RestoreState) -> None: + apply_state_snapshot(x.values) + + @shinychat_chat.history.on_save + def _on_history_save(values: dict[str, Any]) -> None: + values.update(build_state_snapshot()) + + @shinychat_chat.history.on_restore + def _on_history_restore(values: dict[str, Any]) -> None: + apply_state_snapshot(values) if len(table_states) == 1: only_state = next(iter(table_states.values())) diff --git a/pkg-py/tests/playwright/apps/viz_bookmark_app.py b/pkg-py/tests/playwright/apps/viz_bookmark_app.py index d5be5497..13014ddf 100644 --- a/pkg-py/tests/playwright/apps/viz_bookmark_app.py +++ b/pkg-py/tests/playwright/apps/viz_bookmark_app.py @@ -4,6 +4,7 @@ from querychat import QueryChat from querychat.data import titanic +from shinychat.types import HistoryOptions from shiny import App, ui @@ -24,7 +25,7 @@ def app_ui(request): def server(input, output, session): - qc.server(enable_bookmarking=True) + qc.server(history=HistoryOptions(restore_mode="bookmark")) app = App(app_ui, server, bookmark_store="server") diff --git a/pkg-py/tests/test_base.py b/pkg-py/tests/test_base.py index 5aceab0e..c9074e5e 100644 --- a/pkg-py/tests/test_base.py +++ b/pkg-py/tests/test_base.py @@ -438,3 +438,20 @@ def test_system_prompt_built_exactly_once(self, ibis_backend_with_tables): and "Multiple tables" in str(x.message) ] assert len(multi_table_warns) == 1 + + +def test_history_stored_verbatim_no_default_substitution(): + """ + QueryChatBase stores history exactly as given -- including None -- so callers + can later distinguish 'never set' from 'explicitly set to a falsy/true value'. + """ + df = pd.DataFrame({"a": [1, 2, 3]}) + + qc_default = QueryChatBase(df, "a_table") + assert qc_default.history is None + + qc_explicit_false = QueryChatBase(df, "a_table2", history=False) + assert qc_explicit_false.history is False + + qc_explicit_true = QueryChatBase(df, "a_table3", history=True) + assert qc_explicit_true.history is True diff --git a/pkg-py/tests/test_shiny.py b/pkg-py/tests/test_shiny.py new file mode 100644 index 00000000..be46410d --- /dev/null +++ b/pkg-py/tests/test_shiny.py @@ -0,0 +1,155 @@ +""" +Tests for QueryChat's Shiny-specific public API: history/enable_bookmarking +resolution and deprecation, and $app()'s bookmark inference. +""" + +from __future__ import annotations + +import os + +import pytest + + +@pytest.fixture(autouse=True) +def set_dummy_api_key(): + old = os.environ.get("OPENAI_API_KEY") + os.environ["OPENAI_API_KEY"] = "sk-dummy" + yield + if old is not None: + os.environ["OPENAI_API_KEY"] = old + else: + del os.environ["OPENAI_API_KEY"] + + +def test_server_history_stored_verbatim_before_resolution(): + """Constructor-level history isn't substituted until .server()/.app() resolve it.""" + import pandas as pd + from querychat._shiny import QueryChat + + qc_no_history = QueryChat(pd.DataFrame({"a": [1]}), "a_table") + assert qc_no_history.history is None + + qc_explicit = QueryChat(pd.DataFrame({"a": [1]}), "a_table2", history=False) + assert qc_explicit.history is False + + +def test_server_resolves_history_and_warns_on_explicit_enable_bookmarking(monkeypatch): + import warnings + from unittest.mock import MagicMock, patch + + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table") + + captured = {} + + def fake_mod_server(*args, **kwargs): + captured.update(kwargs) + return MagicMock() + + fake_session = MagicMock() + with ( + patch("querychat._shiny.get_current_session", return_value=fake_session), + patch("querychat._shiny.mod_server", side_effect=fake_mod_server), + ): + # Not passing enable_bookmarking or history: no warning, history resolves to True. + with warnings.catch_warnings(): + warnings.simplefilter("error") + qc.server() + assert captured["history"] is True + + # Explicit enable_bookmarking=True/False: warns, but no longer has any + # effect on its own -- mod_server() no longer takes it at all. + with pytest.warns(FutureWarning, match="history"): + qc.server(enable_bookmarking=True) + assert "legacy_enable_bookmarking" not in captured + + with pytest.warns(FutureWarning, match="history"): + qc.server(enable_bookmarking=False) + assert "legacy_enable_bookmarking" not in captured + + # Explicit history= takes precedence over self.history. + qc.history = False + with warnings.catch_warnings(): + warnings.simplefilter("error") + qc.server(history=True) + assert captured["history"] is True + + # self.history takes precedence over the True fallback when history= not passed. + with warnings.catch_warnings(): + warnings.simplefilter("error") + qc.server() + assert captured["history"] is False + + +def test_app_defaults_history_to_bookmark_restore_mode_and_enables_shiny_bookmarking(): + + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table") + app = qc.app() + + assert app.bookmark_store == "server" + + +def test_app_disables_shiny_bookmarking_when_history_is_not_bookmark_mode(): + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table", history=True) + app = qc.app() + + assert app.bookmark_store == "disable" + + +def test_app_respects_explicit_constructor_history_over_apps_own_default(): + """ + An explicit QueryChat(history=False) must not be silently overridden by + $app()'s own restore_mode='bookmark' default. + """ + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table", history=False) + app = qc.app() + + assert app.bookmark_store == "disable" + + +def test_express_enable_bookmarking_auto_emits_no_warning_and_uses_history(): + from unittest.mock import MagicMock, patch + + import pandas as pd + from querychat._shiny import QueryChatExpress + from shiny.express._stub_session import ExpressStubSession + + fake_stub_session = MagicMock(spec=ExpressStubSession) + fake_stub_session.app_opts = {} + + with patch("querychat._shiny.get_current_session", return_value=fake_stub_session): + import warnings + + with warnings.catch_warnings(): + warnings.simplefilter("error") + QueryChatExpress(pd.DataFrame({"a": [1, 2, 3]}), "a_table") + + +def test_express_explicit_enable_bookmarking_warns(): + from unittest.mock import MagicMock, patch + + import pandas as pd + from querychat._shiny import QueryChatExpress + from shiny.express._stub_session import ExpressStubSession + + fake_stub_session = MagicMock(spec=ExpressStubSession) + fake_stub_session.app_opts = {} + + with ( + patch("querychat._shiny.get_current_session", return_value=fake_stub_session), + pytest.warns(FutureWarning, match="history"), + ): + QueryChatExpress( + pd.DataFrame({"a": [1, 2, 3]}), "a_table", enable_bookmarking=True + ) diff --git a/pkg-py/tests/test_shiny_module.py b/pkg-py/tests/test_shiny_module.py index 4bd0a886..bd906db1 100644 --- a/pkg-py/tests/test_shiny_module.py +++ b/pkg-py/tests/test_shiny_module.py @@ -48,7 +48,6 @@ def test_mod_ui_allow_attachments_can_be_overridden(): assert _fake_chat_ui.last_kwargs.get("allow_attachments") is False - def _unwrap_module_server(decorated): """ Recover the undecorated function wrapped by @module.server. @@ -62,8 +61,8 @@ def _unwrap_module_server(decorated): return decorated.__closure__[idx].cell_contents -def test_mod_server_passes_client_to_chat(): - """After migration, Chat is constructed with client= so manual wiring is gone.""" +def test_mod_server_passes_client_and_history_to_chat(): + """After this change, Chat is constructed with client= and history=.""" from unittest.mock import MagicMock, patch from querychat._shiny_module import mod_server @@ -72,12 +71,14 @@ def test_mod_server_passes_client_to_chat(): fake_chat_instance = MagicMock() - def fake_chat_constructor(id, *, client=None, greeting=None, **kwargs): + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): captured["client"] = client captured["greeting"] = greeting + captured["history"] = history return fake_chat_instance - # Minimal stubs fake_source = MagicMock() fake_source.get_data.return_value = [] fake_executor = MagicMock() @@ -94,7 +95,9 @@ def client_factory(**kwargs): fake_session.is_stub_session.return_value = False with ( - patch("querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor), + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), patch("querychat._shiny_module.has_viz_tool", return_value=False), ): inner_fn( @@ -105,11 +108,125 @@ def client_factory(**kwargs): executor=fake_executor, greeting=None, client=client_factory, - enable_bookmarking=False, + history=True, tools=None, greeter=MagicMock(), ) assert captured.get("client") is not None, "client= should be passed to Chat" + assert captured.get("history") is True, "history= should be forwarded verbatim" assert callable(captured.get("greeting")), "greeting= should be a callable" + +def test_mod_server_always_registers_chat_bookmarking_with_no_auto_trigger(): + """ + Chat.enable_bookmarking() must always be called -- independent of `history` + -- so the chat client's own state round-trips through Shiny bookmarks + whenever the host app has bookmarking enabled. It's called with + bookmark_on=None so `history` (or the host app) remains the sole source of + *when* to bookmark, not shinychat's own auto-trigger. + """ + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=True, + tools=None, + greeter=MagicMock(), + ) + + fake_chat_instance.enable_bookmarking.assert_called_once() + _, kwargs = fake_chat_instance.enable_bookmarking.call_args + assert kwargs.get("bookmark_on") is None + + +def test_mod_server_registers_table_state_with_both_bookmark_and_history_hooks(): + """Table/viz state callbacks must always register with both APIs, unconditionally.""" + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + fake_session.bookmark = MagicMock() + fake_session.bookmark.exclude = [] + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=False, # even with history disabled, registration must still happen + tools=None, + greeter=MagicMock(), + ) + + assert "chat_update" in fake_session.bookmark.exclude + fake_session.bookmark.on_bookmark.assert_called_once() + fake_session.bookmark.on_restore.assert_called_once() + fake_chat_instance.history.on_save.assert_called_once() + fake_chat_instance.history.on_restore.assert_called_once() diff --git a/pkg-r/DESCRIPTION b/pkg-r/DESCRIPTION index 577e6a9c..be05c3d1 100644 --- a/pkg-r/DESCRIPTION +++ b/pkg-r/DESCRIPTION @@ -59,7 +59,7 @@ Suggests: VignetteBuilder: knitr Remotes: - posit-dev/shinychat/pkg-r + posit-dev/shinychat/pkg-r@feat/chat-history Config/roxygen2/version: 8.0.0 Config/testthat/edition: 3 Config/testthat/parallel: true diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 7f8ba58a..c6a8c04c 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -33,6 +33,14 @@ * The `$data_source` property has been removed. Use `qc$table("name")$data_source` to read a table's data source, and `qc$add_table(df, "name", replace = TRUE)` to replace it. The `data_source` parameter to `$server()` has also been removed; call `$add_table()` before `$server()` instead. (#195) +* `$app()`/`$app_obj()`'s `bookmark_store` parameter has been removed. Pass `history = shinychat::history_options(restore_mode = "bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `$app()` defaults to `restore_mode = "bookmark"` when no `history` is set anywhere, so existing `$app()` callers keep working without changes. + +* `QueryChat` now defaults to `history = TRUE` (shinychat's own default, browser-localStorage-backed conversation history) instead of no persistence. Pass `history = FALSE` to opt back out. + +## Deprecated + +* `$server()`'s `enable_bookmarking` parameter is deprecated in favor of `history`. Pass `history = shinychat::history_options(restore_mode = "bookmark")` instead of `enable_bookmarking = TRUE` for the equivalent behavior. + ## Improvements * Chat greetings now use shinychat's greeting API (requires shinychat >= 0.4.0). A provided `greeting` renders instantly when the app loads, and when no `greeting` is given one is generated on demand — now **schema-aware**, so it can describe the data it's about to help you explore — without being added to the conversation history. Generated greetings are preserved across bookmark/restore. Tables passed to `QueryChat$new()` are described in the greeting automatically; opt additional tables in with `include_in_greeting = TRUE` on `$add_table()`/`$add_tables()`, or fine-tune which tables and which template the greeting uses via `qc$greeter`. (#249, #261) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 0950a161..2f701a27 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -228,6 +228,8 @@ QueryChat <- R6::R6Class( public = list( #' @field greeting The greeting message displayed to users. greeting = NULL, + #' @field history Conversation history configuration. + history = NULL, #' @field id ID for the QueryChat instance. id = NULL, #' @field id_override Whether the ID was explicitly set by the user. @@ -256,6 +258,10 @@ QueryChat <- R6::R6Class( #' a greeting will be generated at the start of each conversation using #' the LLM, which adds latency and cost. Use `$generate_greeting()` to #' create a greeting to save and reuse. + #' @param history Conversation history configuration: `NULL` (default; + #' resolves to `TRUE` when `$server()`/`$app()` is called and nothing else + #' was set), `TRUE`/`FALSE`, or a [shinychat::history_options()] object. + #' Passed straight through to `shinychat::chat_server(history = )`. #' @param client Optional chat client. Can be: #' - An [ellmer::Chat] object #' - A string to pass to [ellmer::chat()] (e.g., `"openai/gpt-4o"`) @@ -294,6 +300,7 @@ QueryChat <- R6::R6Class( ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -308,6 +315,7 @@ QueryChat <- R6::R6Class( # Validate arguments check_string(id, allow_null = TRUE) check_string(greeting, allow_null = TRUE) + check_history(history) arg_match( tools, values = c("filter", "update", "query", "visualize"), @@ -341,6 +349,7 @@ QueryChat <- R6::R6Class( greeting <- read_utf8(greeting) } self$greeting <- greeting + self$history <- history # Track whether id was explicitly set self$id_override <- id @@ -688,13 +697,16 @@ QueryChat <- R6::R6Class( #' Create and run a Shiny gadget for chatting with data #' #' @param ... Arguments passed to `$app_obj()`. - #' @param bookmark_store The bookmarking storage method. Passed to - #' [shiny::enableBookmarking()]. If `"url"` or `"server"`, the chat state - #' (including current query) will be bookmarked. Default is `"url"`. + #' @param history Conversation history configuration for the generated app. + #' Defaults to `shinychat::history_options(restore_mode = "bookmark")` when + #' neither this nor `$new()`'s `history` was set, since `$app()`'s whole + #' purpose is a single, shareable demo. When the resolved value has + #' `restore_mode = "bookmark"`, the generated app automatically enables + #' Shiny's own server-side bookmarking. #' #' @return Invisibly returns a list of session-specific values. - app = function(..., bookmark_store = "url") { - app <- self$app_obj(..., bookmark_store = bookmark_store) + app = function(..., history = NULL) { + app <- self$app_obj(..., history = history) vals <- tryCatch(shiny::runGadget(app), interrupt = function(cnd) NULL) invisible(vals) }, @@ -703,14 +715,20 @@ QueryChat <- R6::R6Class( #' A streamlined Shiny app for chatting with data #' #' @param ... Additional arguments (currently unused). - #' @param bookmark_store The bookmarking storage method. Passed to - #' [shiny::enableBookmarking()]. Default is `"url"`. + #' @param history Conversation history configuration for the generated app. + #' See `$app()`. #' #' @return A Shiny app object that can be run with `shiny::runApp()`. - app_obj = function(..., bookmark_store = "url") { + app_obj = function(..., history = NULL) { private$require_initialized("$app_obj") check_installed("DT") check_dots_empty() + check_history(history) + resolved_history <- history %||% + self$history %||% + shinychat::history_options(restore_mode = "bookmark") + enable_shiny_bookmarking <- inherits(resolved_history, "chat_history_config") && + identical(resolved_history$restore_mode, "bookmark") first_table_name <- names(private$.data_sources)[[1]] @@ -769,8 +787,7 @@ QueryChat <- R6::R6Class( "reset_query", "sql_editor" )) - enable_bookmarking <- bookmark_store %in% c("url", "server") - qc_vals <- self$server(enable_bookmarking = enable_bookmarking) + qc_vals <- self$server(history = resolved_history) active_table_name <- shiny::reactive({ ct <- qc_vals$current_table() @@ -866,7 +883,11 @@ QueryChat <- R6::R6Class( } } - shiny::shinyApp(ui, server, enableBookmarking = bookmark_store) + shiny::shinyApp( + ui, + server, + enableBookmarking = if (enable_shiny_bookmarking) "server" else "disable" + ) }, #' @description @@ -918,7 +939,12 @@ QueryChat <- R6::R6Class( #' @param data_source Optional data source for backward compatibility. #' If provided, calls `$add_table()` before initializing server logic. #' @param client Optional chat client override for this session. - #' @param enable_bookmarking Whether to enable bookmarking. Default is `FALSE`. + #' @param history Conversation history configuration for this call. Overrides + #' the value set on `$new()`. Resolves to `TRUE` when neither this nor the + #' constructor's `history` was set. + #' @param enable_bookmarking `r lifecycle::badge("deprecated")` Use `history = + #' shinychat::history_options(restore_mode = "bookmark")` instead (set on + #' `$new()`, or passed here). #' @param ... Ignored. #' @param id Optional module ID override. #' @param session The Shiny session object. @@ -932,7 +958,8 @@ QueryChat <- R6::R6Class( server = function( data_source = NULL, client = NULL, - enable_bookmarking = FALSE, + history = NULL, + enable_bookmarking = NULL, ..., id = NULL, session = shiny::getDefaultReactiveDomain() @@ -975,6 +1002,18 @@ QueryChat <- R6::R6Class( ) } + check_history(history) + resolved_history <- history %||% self$history %||% TRUE + + if (!is.null(enable_bookmarking)) { + lifecycle::deprecate_warn( + when = "0.4.0", + what = "QueryChat$server(enable_bookmarking = )", + with = "QueryChat$server(history = )", + details = 'Use history = shinychat::history_options(restore_mode = "bookmark") for the equivalent behavior.' + ) + } + result <- mod_server( id %||% self$id, data_sources = private$.data_sources, @@ -982,9 +1021,9 @@ QueryChat <- R6::R6Class( greeting = self$greeting, client = create_session_client, tools = self$tools, + history = resolved_history, greeter = self$greeter, - greeting_base = base_client, - enable_bookmarking = enable_bookmarking + greeting_base = base_client ) result }, @@ -1093,6 +1132,8 @@ QueryChat <- R6::R6Class( #' @param ... Additional arguments (currently unused). #' @param id Optional module ID for the QueryChat instance. #' @param greeting Optional initial message to display to users. +#' @param history Conversation history configuration. See [QueryChat]'s +#' `$new()` method. #' @param client Optional chat client. #' @param tools Which querychat tools to include in the chat client. #' @param data_description Optional description of the data. @@ -1114,6 +1155,7 @@ querychat <- function( ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -1141,6 +1183,7 @@ querychat <- function( ..., id = id, greeting = greeting, + history = history, client = client, tools = tools, data_description = data_description, @@ -1153,7 +1196,8 @@ querychat <- function( } #' @rdname querychat-convenience -#' @param bookmark_store The bookmarking storage method. Default is `"url"`. +#' @param history Conversation history configuration for the generated app. See +#' [QueryChat]'s `$app()` method. #' @return Invisibly returns the chat object after the app stops. #' #' @export @@ -1171,7 +1215,7 @@ querychat_app <- function( prompt_template = NULL, data_dict = NULL, cleanup = NA, - bookmark_store = "url" + history = NULL ) { if (shiny::isRunning()) { cli::cli_abort( @@ -1214,7 +1258,7 @@ querychat_app <- function( cleanup = cleanup ) - qc$app(bookmark_store = bookmark_store) + qc$app(history = history) } normalize_tools <- function(tools) { diff --git a/pkg-r/R/QueryChatGreeter.R b/pkg-r/R/QueryChatGreeter.R index 33e4257a..1af88edf 100644 --- a/pkg-r/R/QueryChatGreeter.R +++ b/pkg-r/R/QueryChatGreeter.R @@ -37,28 +37,6 @@ QueryChatGreeter <- R6::R6Class( echo <- rlang::arg_match(echo) client <- self$build_client(base) as.character(client$chat(GREETING_PROMPT, echo = echo)) - }, - - #' @description Stream a greeting into the chat UI and capture the result - #' (Shiny path). - #' @param greeting_reactive Session reactiveVal to receive the generated greeting. - #' @param base Optional resolved client to clone. - #' @param chat_id Chat component id targeted by the Shiny stream (default "chat"). - generate_stream = function( - greeting_reactive, - base = NULL, - chat_id = "chat" - ) { - client <- self$build_client(base) - stream <- client$stream_async(GREETING_PROMPT) - p <- shinychat::chat_set_greeting( - chat_id, - chat_greeting_persistent(stream) - ) - promises::then(p, function(value) { - greeting_reactive(client$last_turn()@text) - }) - p } ), active = list( diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index ffb3a908..1914c5ac 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -27,9 +27,9 @@ mod_server <- function( greeting, client, tools, + history, greeter = NULL, - greeting_base = NULL, - enable_bookmarking = FALSE + greeting_base = NULL ) { shiny::moduleServer(id, function(input, output, session) { current_table_val <- shiny::reactiveVal(NULL, label = "current_table") @@ -114,7 +114,26 @@ mod_server <- function( shinychat::chat_greeting(greeting, persistent = TRUE) } - chat_module <- shinychat::chat_server("chat", pre_built_client, greeting = greeting_arg) + chat_module <- shinychat::chat_server( + "chat", + pre_built_client, + greeting = greeting_arg, + history = history + ) + + # Registered unconditionally so the chat client's own state (and greeting) + # round-trip through Shiny bookmarks whenever the host app has bookmarking + # enabled -- independent of `history`. Only the save/restore hooks are + # wanted here; the automatic bookmark triggers are turned off so `history` + # (or the host app) remains the sole source of *when* to bookmark. + shinychat::chat_restore( + "chat", + pre_built_client, + bookmark_on_input = FALSE, + bookmark_on_response = FALSE, + restore_ui = FALSE, + session = session + ) shiny::observeEvent( input$chat_update, @@ -132,51 +151,71 @@ mod_server <- function( } ) - if (enable_bookmarking) { - shiny::setBookmarkExclude("chat_update") - - shiny::onBookmark(function(state) { - table_states <- list() - for (name in names(tables)) { - table_states[[name]] <- list( - sql = tables[[name]]$sql(), - title = tables[[name]]$title() - ) - } - state$values$querychat_tables <- table_states - if (length(viz_widgets) > 0) { - state$values$querychat_viz_widgets <- viz_widgets - } - }) + build_state_snapshot <- function() { + table_states <- list() + for (name in names(tables)) { + table_states[[name]] <- list( + sql = tables[[name]]$sql(), + title = tables[[name]]$title() + ) + } + snapshot <- list(querychat_tables = table_states) + if (length(viz_widgets) > 0) { + snapshot$querychat_viz_widgets <- viz_widgets + } + snapshot + } - shiny::onRestore(function(state) { - if (!is.null(state$values$querychat_tables)) { - last_restored <- NULL - for (name in names(state$values$querychat_tables)) { - tbl_state <- state$values$querychat_tables[[name]] - if (!is.null(tbl_state$sql)) { - tables[[name]]$sql(tbl_state$sql) - last_restored <- name - } - if (!is.null(tbl_state$title)) { - tables[[name]]$title(tbl_state$title) - } + apply_state_snapshot <- function(values) { + if (!is.null(values$querychat_tables)) { + last_restored <- NULL + for (name in names(values$querychat_tables)) { + tbl_state <- values$querychat_tables[[name]] + if (!is.null(tbl_state$sql)) { + tables[[name]]$sql(tbl_state$sql) + last_restored <- name } - if (!is.null(last_restored)) { - current_table_val(last_restored) + if (!is.null(tbl_state$title)) { + tables[[name]]$title(tbl_state$title) } } - if (!is.null(state$values$querychat_viz_widgets)) { - restored <- restore_viz_widgets( - executor, - restore_record_list(state$values$querychat_viz_widgets), - session - ) - viz_widgets <<- restored + if (!is.null(last_restored)) { + current_table_val(last_restored) } - }) + } + if (!is.null(values$querychat_viz_widgets)) { + restored <- restore_viz_widgets( + executor, + restore_record_list(values$querychat_viz_widgets), + session + ) + viz_widgets <<- restored + } + invisible(NULL) } + # Registered unconditionally: both registrations are no-ops unless the + # corresponding persistence layer (Shiny's own bookmarking, or shinychat's + # history) is actually active in the host app. In restore_mode = "bookmark", + # shinychat's history-save triggers a real Shiny bookmark internally, so both + # callbacks fire for the same event -- the snapshot is written twice, + # harmlessly, since it's idempotent. + shiny::setBookmarkExclude("chat_update") + + shiny::onBookmark(function(state) { + state$values <- utils::modifyList(state$values, build_state_snapshot()) + }) + + shiny::onRestore(function(state) { + apply_state_snapshot(state$values) + }) + + chat_module$history$on_save(function(values) { + utils::modifyList(values, build_state_snapshot()) + }) + + chat_module$history$on_restore(apply_state_snapshot) + table_fn <- function(name) { if (!name %in% names(tables)) { available <- paste0("'", names(tables), "'", collapse = ", ") diff --git a/pkg-r/R/utils-check.R b/pkg-r/R/utils-check.R index 966df62d..0588a695 100644 --- a/pkg-r/R/utils-check.R +++ b/pkg-r/R/utils-check.R @@ -12,6 +12,32 @@ check_data_source <- function( } } +# History parameter validation ------------------------------------------- + +#' Check that a value is a valid `history` argument +#' +#' Accepts `NULL` (not specified), `TRUE`, `FALSE`, or a +#' `shinychat::history_options()` object. +#' +#' @noRd +check_history <- function( + x, + ..., + arg = caller_arg(x), + call = caller_env() +) { + check_dots_empty() + if ( + is.null(x) || isTRUE(x) || isFALSE(x) || inherits(x, "chat_history_config") + ) { + return(invisible(NULL)) + } + cli::cli_abort( + "{.arg {arg}} must be {.code NULL}, {.code TRUE}, {.code FALSE}, or a {.fun shinychat::history_options} object, not {.obj_type_friendly {x}}.", + call = call + ) +} + # SQL table name validation ---------------------------------------------- #' Check SQL table name validity diff --git a/pkg-r/R/utils-shinychat.R b/pkg-r/R/utils-shinychat.R deleted file mode 100644 index a99ffa0a..00000000 --- a/pkg-r/R/utils-shinychat.R +++ /dev/null @@ -1,10 +0,0 @@ -# shinychat compatibility helpers - -# TODO: remove once shinychat >= 0.5.0 is the minimum (persistent added in 0.4.0.9000) -chat_greeting_persistent <- function(content) { - if (utils::packageVersion("shinychat") > "0.4.0") { - shinychat::chat_greeting(content, persistent = TRUE) - } else { - shinychat::chat_greeting(content, dismissible = FALSE) - } -} diff --git a/pkg-r/man/QueryChat.Rd b/pkg-r/man/QueryChat.Rd index 915b4f51..548dcae2 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -98,6 +98,8 @@ qc <- QueryChat$new(con, "mtcars") \describe{ \item{\code{greeting}}{The greeting message displayed to users.} + \item{\code{history}}{Conversation history configuration.} + \item{\code{id}}{ID for the QueryChat instance.} \item{\code{id_override}}{Whether the ID was explicitly set by the user.} @@ -151,6 +153,7 @@ access its \verb{$tables} and \verb{$prompt}.} ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -183,6 +186,10 @@ character string (in Markdown format) or a file path. If not provided, a greeting will be generated at the start of each conversation using the LLM, which adds latency and cost. Use \verb{$generate_greeting()} to create a greeting to save and reuse.} + \item{\code{history}}{Conversation history configuration: \code{NULL} (default; +resolves to \code{TRUE} when \verb{$server()}/\verb{$app()} is called and nothing else +was set), \code{TRUE}/\code{FALSE}, or a \code{\link[shinychat:history_options]{shinychat::history_options()}} object. +Passed straight through to \code{shinychat::chat_server(history = )}.} \item{\code{client}}{Optional chat client. Can be: \itemize{ \item An \link[ellmer:Chat]{ellmer::Chat} object @@ -404,16 +411,19 @@ By default, only the \code{"query"} tool is included, regardless of the Create and run a Shiny gadget for chatting with data \subsection{Usage}{ \if{html}{\out{
}} - \preformatted{QueryChat$app(..., bookmark_store = "url")} + \preformatted{QueryChat$app(..., history = NULL)} \if{html}{\out{
}} } \subsection{Arguments}{ \if{html}{\out{
}} \describe{ \item{\code{...}}{Arguments passed to \verb{$app_obj()}.} - \item{\code{bookmark_store}}{The bookmarking storage method. Passed to -\code{\link[shiny:enableBookmarking]{shiny::enableBookmarking()}}. If \code{"url"} or \code{"server"}, the chat state -(including current query) will be bookmarked. Default is \code{"url"}.} + \item{\code{history}}{Conversation history configuration for the generated app. +Defaults to \code{shinychat::history_options(restore_mode = "bookmark")} when +neither this nor \verb{$new()}'s \code{history} was set, since \verb{$app()}'s whole +purpose is a single, shareable demo. When the resolved value has +\code{restore_mode = "bookmark"}, the generated app automatically enables +Shiny's own server-side bookmarking.} } \if{html}{\out{
}} } @@ -429,15 +439,15 @@ By default, only the \code{"query"} tool is included, regardless of the A streamlined Shiny app for chatting with data \subsection{Usage}{ \if{html}{\out{
}} - \preformatted{QueryChat$app_obj(..., bookmark_store = "url")} + \preformatted{QueryChat$app_obj(..., history = NULL)} \if{html}{\out{
}} } \subsection{Arguments}{ \if{html}{\out{
}} \describe{ \item{\code{...}}{Additional arguments (currently unused).} - \item{\code{bookmark_store}}{The bookmarking storage method. Passed to -\code{\link[shiny:enableBookmarking]{shiny::enableBookmarking()}}. Default is \code{"url"}.} + \item{\code{history}}{Conversation history configuration for the generated app. +See \verb{$app()}.} } \if{html}{\out{
}} } @@ -512,7 +522,8 @@ By default, only the \code{"query"} tool is included, regardless of the \preformatted{QueryChat$server( data_source = NULL, client = NULL, - enable_bookmarking = FALSE, + history = NULL, + enable_bookmarking = NULL, ..., id = NULL, session = shiny::getDefaultReactiveDomain() @@ -525,7 +536,11 @@ By default, only the \code{"query"} tool is included, regardless of the \item{\code{data_source}}{Optional data source for backward compatibility. If provided, calls \verb{$add_table()} before initializing server logic.} \item{\code{client}}{Optional chat client override for this session.} - \item{\code{enable_bookmarking}}{Whether to enable bookmarking. Default is \code{FALSE}.} + \item{\code{history}}{Conversation history configuration for this call. Overrides +the value set on \verb{$new()}. Resolves to \code{TRUE} when neither this nor the +constructor's \code{history} was set.} + \item{\code{enable_bookmarking}}{\ifelse{html}{\href{https://lifecycle.r-lib.org/articles/stages.html#deprecated}{\figure{lifecycle-deprecated.svg}{options: alt='[Deprecated]'}}}{\strong{[Deprecated]}} Use \code{history = shinychat::history_options(restore_mode = "bookmark")} instead (set on +\verb{$new()}, or passed here).} \item{\code{...}}{Ignored.} \item{\code{id}}{Optional module ID override.} \item{\code{session}}{The Shiny session object.} diff --git a/pkg-r/man/querychat-convenience.Rd b/pkg-r/man/querychat-convenience.Rd index 5be1320f..4f1e6ecd 100644 --- a/pkg-r/man/querychat-convenience.Rd +++ b/pkg-r/man/querychat-convenience.Rd @@ -11,6 +11,7 @@ querychat( ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -35,7 +36,7 @@ querychat_app( prompt_template = NULL, data_dict = NULL, cleanup = NA, - bookmark_store = "url" + history = NULL ) } \arguments{ @@ -50,6 +51,9 @@ connection).} \item{greeting}{Optional initial message to display to users.} +\item{history}{Conversation history configuration for the generated app. See +\link{QueryChat}'s \verb{$app()} method.} + \item{client}{Optional chat client.} \item{tools}{Which querychat tools to include in the chat client.} @@ -67,8 +71,6 @@ values to consider as a categorical variable. Default is 20.} \item{cleanup}{Whether or not to automatically run \verb{$cleanup()} when the Shiny session/app stops.} - -\item{bookmark_store}{The bookmarking storage method. Default is \code{"url"}.} } \value{ A \code{QueryChat} object. See \link{QueryChat} for available methods. diff --git a/pkg-r/tests/testthat/_snaps/QueryChat.md b/pkg-r/tests/testthat/_snaps/QueryChat.md index 191e0677..b4db88d9 100644 --- a/pkg-r/tests/testthat/_snaps/QueryChat.md +++ b/pkg-r/tests/testthat/_snaps/QueryChat.md @@ -38,6 +38,19 @@ Error in `qc$server()`: ! `$server()` must be called within a Shiny server function +# QueryChat$server() resolves history (explicit > constructor > TRUE) and warns on enable_bookmarking + + Code + shiny::testServer(function(input, output, session) qc_no_history$server( + enable_bookmarking = TRUE), { }) + Message + Using model = "gpt-4.1". + Condition + Warning: + The `enable_bookmarking` argument of `QueryChat$server()` is deprecated as of querychat 0.4.0. + i Please use the `history` argument instead. + i Use history = shinychat::history_options(restore_mode = "bookmark") for the equivalent behavior. + # normalize_data_source() / errors with invalid data source types Code diff --git a/pkg-r/tests/testthat/helper-fixtures.R b/pkg-r/tests/testthat/helper-fixtures.R index 3775b84e..d69732db 100644 --- a/pkg-r/tests/testthat/helper-fixtures.R +++ b/pkg-r/tests/testthat/helper-fixtures.R @@ -175,3 +175,29 @@ mock_ellmer_chat_client <- function( MockChat$new(ellmer::Provider("test", "test", "test")) } + +# shinychat::chat_restore() validates that `client` is an ellmer::Chat() R6 +# object; the lightweight `structure(list(), class = c("MockChat", "Chat"))` +# fakes used throughout this file don't satisfy that. mod_server() calls +# chat_restore() unconditionally, so tests that don't care about its behavior +# need a no-op stand-in. +local_mock_chat_restore <- function(env = parent.frame()) { + testthat::local_mocked_bindings( + chat_restore = function(...) invisible(NULL), + .package = "shinychat", + .env = env + ) +} + +# Minimal mock matching the shape shinychat::chat_server() always returns, +# including a $history interface that's present regardless of the `history` +# argument's value (registrations are just inert if history isn't active). +mock_chat_server_result <- function(client) { + list( + client = client, + history = list( + on_save = function(fn) invisible(fn), + on_restore = function(fn) invisible(fn) + ) + ) +} diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index cfa43fda..97bac6a8 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -688,6 +688,112 @@ test_that("QueryChat$server() errors when called outside Shiny context", { }) }) +test_that("QueryChat$new() validates history and stores it verbatim", { + test_df <- new_test_df() + + qc_default <- QueryChat$new(test_df, greeting = "Test") + withr::defer(qc_default$cleanup()) + expect_null(qc_default$history) + + qc_false <- QueryChat$new( + test_df, + table_name = "test_df2", + greeting = "Test", + history = FALSE + ) + withr::defer(qc_false$cleanup()) + expect_false(qc_false$history) + + expect_error( + QueryChat$new( + test_df, + table_name = "test_df3", + greeting = "Test", + history = "nope" + ), + class = "rlang_error" + ) +}) + +test_that("QueryChat$server() resolves history (explicit > constructor > TRUE) and warns on enable_bookmarking", { + skip_if_no_dataframe_engine() + withr::local_envvar(OPENAI_API_KEY = "boop") + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + captured <- NULL + local_mocked_bindings( + mod_server = function(id, ..., history) { + captured <<- history + list() + }, + .package = "querychat" + ) + + qc <- local_querychat(history = FALSE) + qc$.__enclos_env__$private$.query_executor <- executor + + expect_no_warning( + shiny::testServer( + function(input, output, session) qc$server(), + { + expect_false(captured) + } + ), + class = "lifecycle_warning_deprecated" + ) + + shiny::testServer( + function(input, output, session) qc$server(history = TRUE), + { + expect_true(captured) + } + ) + + qc_no_history <- local_querychat() + qc_no_history$.__enclos_env__$private$.query_executor <- executor + shiny::testServer( + function(input, output, session) qc_no_history$server(), + { + expect_true(captured) + } + ) + + expect_snapshot( + shiny::testServer( + function(input, output, session) + qc_no_history$server(enable_bookmarking = TRUE), + { + } + ) + ) +}) + +test_that("QueryChat$app_obj() infers Shiny bookmarking from history's restore_mode", { + skip_if_no_dataframe_engine() + withr::local_envvar(OPENAI_API_KEY = "boop") + + qc_default <- local_querychat() + app_default <- qc_default$app_obj() + expect_equal(app_default$appOptions$bookmarkStore, "server") + + qc_browser <- local_querychat(history = TRUE) + app_browser <- qc_browser$app_obj() + expect_equal(app_browser$appOptions$bookmarkStore, "disable") + + qc_explicit_off <- local_querychat(history = FALSE) + app_explicit_off <- qc_explicit_off$app_obj() + expect_equal(app_explicit_off$appOptions$bookmarkStore, "disable") + + qc_no_history <- local_querychat() + app_call_override <- qc_no_history$app_obj( + history = shinychat::history_options(restore_mode = "bookmark") + ) + expect_equal(app_call_override$appOptions$bookmarkStore, "server") +}) + describe("querychat()", { skip_if_no_dataframe_engine() withr::local_envvar(OPENAI_API_KEY = "boop") @@ -876,7 +982,8 @@ test_that("querychat_app() only cleans up data frame sources on exit", { # have to use an option because the code is evaluated in a far-away env options(.test_cleanup = cleanup) }, - app = function(...) {} + app = function(...) { + } ) ) withr::local_options(rlang_interactive = TRUE) diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index e1adb9f8..85be7c90 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -28,9 +28,10 @@ test_that("mod_server() return includes table() and table_names() for single-tab structure(list(), class = c("MockChat", "Chat")) local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -40,8 +41,7 @@ test_that("mod_server() return includes table() and table_names() for single-tab executor = executor, greeting = "Hello", client = client_factory, - tools = "query", - enable_bookmarking = FALSE + tools = "query" ), { # Verify the returned list exposes table() and table_names() @@ -88,9 +88,10 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl } local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -100,8 +101,7 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl executor = executor, greeting = "Hello", client = client_factory, - tools = "query", - enable_bookmarking = FALSE + tools = "query" ), { # Verify the returned list exposes table() and table_names() @@ -141,9 +141,10 @@ test_that("mod_server() passes visualize callback and tools to client factory", } local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -153,8 +154,7 @@ test_that("mod_server() passes visualize callback and tools to client factory", executor = executor, greeting = "Hello", client = client_factory, - tools = c("query", "visualize"), - enable_bookmarking = FALSE + tools = c("query", "visualize") ), { expect_type(captured, "list") @@ -177,9 +177,10 @@ test_that("mod_server() exposes current_table() starting as NULL", { structure(list(), class = c("MockChat", "Chat")) local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -189,8 +190,7 @@ test_that("mod_server() exposes current_table() starting as NULL", { executor = executor, greeting = "Hello", client = client_factory, - tools = "query", - enable_bookmarking = FALSE + tools = "query" ), { expect_true(is.function(session$returned$current_table)) @@ -215,9 +215,10 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu } local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -227,8 +228,7 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu executor = executor, greeting = "Hello", client = client_factory, - tools = "query", - enable_bookmarking = FALSE + tools = "query" ), { # Initially NULL @@ -320,9 +320,10 @@ test_that("restored viz widgets survive a second bookmark cycle", { } local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() local_mocked_bindings( onBookmark = function(fun, session = NULL) { bookmark_fn <<- fun @@ -344,50 +345,46 @@ test_that("restored viz widgets survive a second bookmark cycle", { .package = "querychat" ) - expect_no_warning( - shiny::testServer( - mod_server, - args = list( - id = "test", - data_sources = list(test_table = ds), - executor = executor, - greeting = "Hello", - client = client_factory, - tools = c("query", "visualize"), - enable_bookmarking = TRUE - ), - { - expect_true(is.function(bookmark_fn)) - expect_true(is.function(restore_fn)) - expect_true(is.function(callbacks$visualize)) - - saved <- list( - list( - widget_id = "querychat_viz_1", - ggsql = "SELECT 1 VISUALISE 1 AS x DRAW point" - ) + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = c("query", "visualize") + ), + { + expect_true(is.function(bookmark_fn)) + expect_true(is.function(restore_fn)) + expect_true(is.function(callbacks$visualize)) + + saved <- list( + list( + widget_id = "querychat_viz_1", + ggsql = "SELECT 1 VISUALISE 1 AS x DRAW point" ) + ) - shiny::isolate(callbacks$visualize(saved[[1]])) + shiny::isolate(callbacks$visualize(saved[[1]])) - first_state <- new.env(parent = emptyenv()) - first_state$values <- list() - shiny::isolate(bookmark_fn(first_state)) - expect_equal(first_state$values$querychat_viz_widgets, saved) + first_state <- new.env(parent = emptyenv()) + first_state$values <- list() + shiny::isolate(bookmark_fn(first_state)) + expect_equal(first_state$values$querychat_viz_widgets, saved) - restore_state <- new.env(parent = emptyenv()) - restore_state$values <- first_state$values - shiny::isolate(restore_fn(restore_state)) - expect_true(inherits(restored_args$executor, "QueryExecutor")) - expect_equal(restored_args$saved_widgets, saved) + restore_state <- new.env(parent = emptyenv()) + restore_state$values <- first_state$values + shiny::isolate(restore_fn(restore_state)) + expect_true(inherits(restored_args$executor, "QueryExecutor")) + expect_equal(restored_args$saved_widgets, saved) - second_state <- new.env(parent = emptyenv()) - second_state$values <- list() - shiny::isolate(bookmark_fn(second_state)) - expect_equal(second_state$values$querychat_viz_widgets, saved) - } - ), - class = "lifecycle_warning_deprecated" + second_state <- new.env(parent = emptyenv()) + second_state$values <- list() + shiny::isolate(bookmark_fn(second_state)) + expect_equal(second_state$values$querychat_viz_widgets, saved) + } ) }) @@ -405,10 +402,11 @@ test_that("mod_server() calls chat_server('chat', ...) with the pre-built client local_mocked_bindings( chat_server = function(id, client, ...) { captured_chat_args <<- list(id = id, client = client) - list(client = client) + mock_chat_server_result(client) }, .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -418,8 +416,7 @@ test_that("mod_server() calls chat_server('chat', ...) with the pre-built client executor = executor, greeting = "Hello", client = client_factory, - tools = "query", - enable_bookmarking = FALSE + tools = "query" ), { expect_equal(captured_chat_args$id, "chat") @@ -428,6 +425,49 @@ test_that("mod_server() calls chat_server('chat', ...) with the pre-built client ) }) +test_that("mod_server() always calls chat_restore() with the auto-bookmark trigger disabled", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) + + captured_restore_args <- NULL + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + chat_restore = function(id, client, ...) { + captured_restore_args <<- list(id = id, client = client, ...) + invisible(NULL) + }, + .package = "shinychat" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = TRUE + ), + { + expect_equal(captured_restore_args$id, "chat") + expect_true(inherits(captured_restore_args$client, "Chat")) + expect_false(captured_restore_args$restore_ui) + # The hooks are registered, but querychat never triggers a bookmark + # itself -- that's left to `history` or the host app. + expect_false(captured_restore_args$bookmark_on_input) + expect_false(captured_restore_args$bookmark_on_response) + } + ) +}) + test_that("mod_server() builds the auto-generated greeting from the greeter, not the main client", { skip_if_no_dataframe_engine() @@ -461,11 +501,12 @@ test_that("mod_server() builds the auto-generated greeting from the greeter, not local_mocked_bindings( chat_server = function(id, client, greeting = NULL, ...) { captured_greeting_arg <<- greeting - list(client = client) + mock_chat_server_result(client) }, chat_greeting = function(content, ...) content, .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -477,8 +518,7 @@ test_that("mod_server() builds the auto-generated greeting from the greeter, not client = client_factory, tools = "query", greeter = fake_greeter, - greeting_base = "base-client", - enable_bookmarking = FALSE + greeting_base = "base-client" ), { expect_true(is.function(captured_greeting_arg)) @@ -506,9 +546,10 @@ test_that("mod_server() chat_update input updates table state", { structure(list(), class = c("MockChat", "Chat")) local_mocked_bindings( - chat_server = function(id, client, ...) list(client = client), + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -518,8 +559,7 @@ test_that("mod_server() chat_update input updates table state", { executor = executor, greeting = "Hello", client = client_factory, - tools = "query", - enable_bookmarking = FALSE + tools = "query" ), { session$setInputs( @@ -540,3 +580,124 @@ test_that("mod_server() chat_update input updates table state", { } ) }) + +test_that("mod_server() registers table/viz state with both bookmark and history hooks, unconditionally", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) + + bookmark_save_count <- 0 + bookmark_restore_count <- 0 + history_save_fn <- NULL + history_restore_fn <- NULL + + local_mocked_bindings( + chat_server = function(id, client, ...) { + list( + client = client, + history = list( + on_save = function(fn) { + history_save_fn <<- fn + invisible(fn) + }, + on_restore = function(fn) { + history_restore_fn <<- fn + invisible(fn) + } + ) + ) + }, + .package = "shinychat" + ) + local_mock_chat_restore() + local_mocked_bindings( + onBookmark = function(fun, session = NULL) { + bookmark_save_count <<- bookmark_save_count + 1 + }, + onRestore = function(fun, session = NULL) { + bookmark_restore_count <<- bookmark_restore_count + 1 + }, + .package = "shiny" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = FALSE # even with history disabled, registration must still happen + ), + { + expect_equal(bookmark_save_count, 1L) + expect_equal(bookmark_restore_count, 1L) + expect_true(is.function(history_save_fn)) + expect_true(is.function(history_restore_fn)) + } + ) +}) + +test_that("history on_save callback returns merged values (R history contract)", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) + structure(list(), class = c("MockChat", "Chat")) + + history_save_fn <- NULL + local_mocked_bindings( + chat_server = function(id, client, ...) { + list( + client = client, + history = list( + on_save = function(fn) { + history_save_fn <<- fn + invisible(fn) + }, + on_restore = function(fn) invisible(fn) + ) + ) + }, + .package = "shinychat" + ) + local_mock_chat_restore() + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = TRUE + ), + { + session$setInputs( + chat_update = list( + table = "test_table", + query = "SELECT * FROM test_table WHERE id = 1", + title = "One row" + ) + ) + result <- shiny::isolate(history_save_fn(list(unrelated_key = "kept"))) + expect_equal(result$unrelated_key, "kept") + expect_equal( + result$querychat_tables$test_table$sql, + "SELECT * FROM test_table WHERE id = 1" + ) + } + ) +}) diff --git a/pyproject.toml b/pyproject.toml index bec6902f..67c3aa67 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ maintainers = [ dependencies = [ "duckdb", "shiny>=1.6.2", - "shinychat @ git+https://github.com/posit-dev/shinychat.git", + "shinychat @ git+https://github.com/posit-dev/shinychat.git@feat/chat-history", "htmltools", "chatlas>=0.18.0", "narwhals>=2.2.0", From f1bd5b51f04e190359ce1c6d3d7885fb0f39a5a4 Mon Sep 17 00:00:00 2001 From: cpsievert Date: Wed, 1 Jul 2026 14:32:42 +0000 Subject: [PATCH 03/16] `air format` (GitHub Actions) --- pkg-r/R/QueryChat.R | 11 ++++++++-- pkg-r/R/querychat_module.R | 1 - pkg-r/tests/testthat/test-QueryChat.R | 11 +++++----- pkg-r/tests/testthat/test-querychat_module.R | 21 +++++++++++++------- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 2f701a27..395520d1 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -727,7 +727,10 @@ QueryChat <- R6::R6Class( resolved_history <- history %||% self$history %||% shinychat::history_options(restore_mode = "bookmark") - enable_shiny_bookmarking <- inherits(resolved_history, "chat_history_config") && + enable_shiny_bookmarking <- inherits( + resolved_history, + "chat_history_config" + ) && identical(resolved_history$restore_mode, "bookmark") first_table_name <- names(private$.data_sources)[[1]] @@ -886,7 +889,11 @@ QueryChat <- R6::R6Class( shiny::shinyApp( ui, server, - enableBookmarking = if (enable_shiny_bookmarking) "server" else "disable" + enableBookmarking = if (enable_shiny_bookmarking) { + "server" + } else { + "disable" + } ) }, diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index 1914c5ac..f63fa8b1 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -1,6 +1,5 @@ # Main module UI function mod_ui <- function(id, ...) { - htmltools::tagList( htmltools::htmlDependency( "querychat", diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index 97bac6a8..de1312ee 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -763,10 +763,10 @@ test_that("QueryChat$server() resolves history (explicit > constructor > TRUE) a expect_snapshot( shiny::testServer( - function(input, output, session) - qc_no_history$server(enable_bookmarking = TRUE), - { - } + function(input, output, session) { + qc_no_history$server(enable_bookmarking = TRUE) + }, + {} ) ) }) @@ -982,8 +982,7 @@ test_that("querychat_app() only cleans up data frame sources on exit", { # have to use an option because the code is evaluated in a far-away env options(.test_cleanup = cleanup) }, - app = function(...) { - } + app = function(...) {} ) ) withr::local_options(rlang_interactive = TRUE) diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index 85be7c90..f4a6b910 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -24,8 +24,9 @@ test_that("mod_server() return includes table() and table_names() for single-tab executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } local_mocked_bindings( chat_server = function(id, client, ...) mock_chat_server_result(client), @@ -173,8 +174,9 @@ test_that("mod_server() exposes current_table() starting as NULL", { executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } local_mocked_bindings( chat_server = function(id, client, ...) mock_chat_server_result(client), @@ -396,8 +398,9 @@ test_that("mod_server() calls chat_server('chat', ...) with the pre-built client withr::defer(executor$cleanup()) captured_chat_args <- NULL - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } local_mocked_bindings( chat_server = function(id, client, ...) { @@ -432,8 +435,9 @@ test_that("mod_server() always calls chat_restore() with the auto-bookmark trigg executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } captured_restore_args <- NULL local_mocked_bindings( @@ -542,8 +546,9 @@ test_that("mod_server() chat_update input updates table state", { executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } local_mocked_bindings( chat_server = function(id, client, ...) mock_chat_server_result(client), @@ -588,8 +593,9 @@ test_that("mod_server() registers table/viz state with both bookmark and history executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } bookmark_save_count <- 0 bookmark_restore_count <- 0 @@ -652,8 +658,9 @@ test_that("history on_save callback returns merged values (R history contract)", executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) + client_factory <- function(...) { structure(list(), class = c("MockChat", "Chat")) + } history_save_fn <- NULL local_mocked_bindings( From 130d7fda32163492309c956e9c607e2bce33e647 Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Mon, 6 Jul 2026 10:32:46 -0500 Subject: [PATCH 04/16] Update remotes Co-authored-by: Carson Sievert --- pkg-r/DESCRIPTION | 2 +- pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg-r/DESCRIPTION b/pkg-r/DESCRIPTION index be05c3d1..577e6a9c 100644 --- a/pkg-r/DESCRIPTION +++ b/pkg-r/DESCRIPTION @@ -59,7 +59,7 @@ Suggests: VignetteBuilder: knitr Remotes: - posit-dev/shinychat/pkg-r@feat/chat-history + posit-dev/shinychat/pkg-r Config/roxygen2/version: 8.0.0 Config/testthat/edition: 3 Config/testthat/parallel: true diff --git a/pyproject.toml b/pyproject.toml index 67c3aa67..1b38fd78 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ maintainers = [ dependencies = [ "duckdb", "shiny>=1.6.2", - "shinychat @ git+https://github.com/posit-dev/shinychat.git@feat/chat-history", + "shinychat>=0.6.0", "htmltools", "chatlas>=0.18.0", "narwhals>=2.2.0", From 25b675e923c893285fb37ad804ef5c192f3c7354 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 6 Jul 2026 11:14:04 -0500 Subject: [PATCH 05/16] fix(pkg-r): guard bookmark/history restore against stale table names apply_state_snapshot() indexed tables[[name]] for every key in the restored payload, so a bookmark or history snapshot referencing a table no longer registered on the current server would error and abort restore entirely. Iterate over the currently registered tables instead, matching the Python implementation. --- pkg-r/R/querychat_module.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index f63fa8b1..c8d4e12e 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -168,7 +168,7 @@ mod_server <- function( apply_state_snapshot <- function(values) { if (!is.null(values$querychat_tables)) { last_restored <- NULL - for (name in names(values$querychat_tables)) { + for (name in names(tables)) { tbl_state <- values$querychat_tables[[name]] if (!is.null(tbl_state$sql)) { tables[[name]]$sql(tbl_state$sql) From abb93fb117dd69642899089d25383d9090ab1cea Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 6 Jul 2026 11:42:17 -0500 Subject: [PATCH 06/16] fix: stabilize flaky viz footer e2e test; update stale R snapshot - test_toggle_hides_section_again clicked twice back-to-back while the chart widget below the button was still resizing (its responsive fill layout takes a couple seconds, and settles again after the query section expands), so the second click could land on stale coordinates. Wait for the button's position to stabilize before interacting. - The QueryChat.R snapshot for `enable_bookmarking` deprecation still had the pre-migration deparse of the mocked testServer() call; R CMD check consistently renders it with explicit braces now. --- pkg-py/tests/playwright/test_11_viz_footer.py | 43 ++++++++++++++++++- pkg-r/tests/testthat/_snaps/QueryChat.md | 5 ++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/pkg-py/tests/playwright/test_11_viz_footer.py b/pkg-py/tests/playwright/test_11_viz_footer.py index 8d359107..d556beaf 100644 --- a/pkg-py/tests/playwright/test_11_viz_footer.py +++ b/pkg-py/tests/playwright/test_11_viz_footer.py @@ -9,6 +9,7 @@ from __future__ import annotations +import time from pathlib import Path from typing import TYPE_CHECKING @@ -16,7 +17,7 @@ from playwright.sync_api import expect if TYPE_CHECKING: - from playwright.sync_api import Download, Page + from playwright.sync_api import Download, Locator, Page from shinychat.playwright import ChatController @@ -24,6 +25,36 @@ TOOL_RESULT_TIMEOUT = 90_000 +def _wait_for_stable_position( + locator: Locator, + quiet_checks: int = 5, + interval_s: float = 0.1, + max_wait_s: float = 8.0, +) -> None: + """ + Wait until `locator`'s bounding box stops changing. + + The viz widget (chart) keeps resizing for a couple of seconds after the + tool result first appears — likely a client/server size-negotiation + round trip for its responsive fill layout. That drags the footer button + below it along for the ride, so a click issued mid-drift can land on + stale coordinates and silently miss the button. + """ + deadline = time.monotonic() + max_wait_s + last_box = None + stable_count = 0 + while time.monotonic() < deadline: + box = locator.bounding_box() + if box is not None and box == last_box: + stable_count += 1 + if stable_count >= quiet_checks: + return + else: + stable_count = 0 + last_box = box + time.sleep(interval_s) + + def _download_from_save_menu(page: Page, export_format: str) -> tuple[Download, str]: """Open the save menu, click the requested format, and capture the download.""" page.locator(".querychat-save-btn").click() @@ -58,6 +89,11 @@ def _send_viz_prompt(page: Page, app_10_viz: str, chat_10_viz: ChatController) - # complete and shinychat has finished its final re-render of the tool # result card. expect(chat_10_viz.loc_input).to_be_enabled(timeout=30_000) + # The chart widget below the footer keeps resizing for a couple of + # seconds after that (see _wait_for_stable_position), dragging the + # Show Query button's position along with it. Let it settle before + # tests start clicking, or a click can land on stale coordinates. + _wait_for_stable_position(page.locator(".querychat-show-query-btn")) class TestShowQueryToggle: @@ -105,6 +141,11 @@ def test_toggle_hides_section_again(self, page: Page) -> None: btn.click() # show expect(label).to_have_text("Hide Query") + # Revealing the query section changes the card's layout, which + # kicks off another round of the chart's resize settling (see + # _wait_for_stable_position) and can drag the button along with it. + _wait_for_stable_position(btn) + btn.click() # hide expect(label).to_have_text("Show Query") diff --git a/pkg-r/tests/testthat/_snaps/QueryChat.md b/pkg-r/tests/testthat/_snaps/QueryChat.md index b4db88d9..b6190483 100644 --- a/pkg-r/tests/testthat/_snaps/QueryChat.md +++ b/pkg-r/tests/testthat/_snaps/QueryChat.md @@ -41,8 +41,9 @@ # QueryChat$server() resolves history (explicit > constructor > TRUE) and warns on enable_bookmarking Code - shiny::testServer(function(input, output, session) qc_no_history$server( - enable_bookmarking = TRUE), { }) + shiny::testServer(function(input, output, session) { + qc_no_history$server(enable_bookmarking = TRUE) + }, { }) Message Using model = "gpt-4.1". Condition From 7688df617369bc62276bc7b54933162a161f8481 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:32:29 -0500 Subject: [PATCH 07/16] fix(pkg-r): map deprecated enable_bookmarking=TRUE to bookmark-mode history QueryChat$server(enable_bookmarking = TRUE) warned but the value was then discarded -- resolved_history was computed from history %||% self$history %||% TRUE, so it silently degraded to history = TRUE (localStorage) instead of shareable bookmark state. Map enable_bookmarking = TRUE to history_options(restore_mode = "bookmark") when neither this call's nor the constructor's history was set, preserving the old bookmark-based behavior through the deprecation window. --- pkg-r/R/QueryChat.R | 10 ++++++++- pkg-r/tests/testthat/_snaps/QueryChat.md | 5 ++++- pkg-r/tests/testthat/test-QueryChat.R | 27 +++++++++++++++++++++++- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 395520d1..3c6bda5b 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -1010,7 +1010,6 @@ QueryChat <- R6::R6Class( } check_history(history) - resolved_history <- history %||% self$history %||% TRUE if (!is.null(enable_bookmarking)) { lifecycle::deprecate_warn( @@ -1021,6 +1020,15 @@ QueryChat <- R6::R6Class( ) } + resolved_history <- history %||% + self$history %||% + ( + if (isTRUE(enable_bookmarking)) { + shinychat::history_options(restore_mode = "bookmark") + } + ) %||% + TRUE + result <- mod_server( id %||% self$id, data_sources = private$.data_sources, diff --git a/pkg-r/tests/testthat/_snaps/QueryChat.md b/pkg-r/tests/testthat/_snaps/QueryChat.md index b6190483..8aaf6529 100644 --- a/pkg-r/tests/testthat/_snaps/QueryChat.md +++ b/pkg-r/tests/testthat/_snaps/QueryChat.md @@ -43,7 +43,10 @@ Code shiny::testServer(function(input, output, session) { qc_no_history$server(enable_bookmarking = TRUE) - }, { }) + }, { + expect_s3_class(captured, "chat_history_config") + expect_equal(captured$restore_mode, "bookmark") + }) Message Using model = "gpt-4.1". Condition diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index de1312ee..fb266407 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -766,7 +766,32 @@ test_that("QueryChat$server() resolves history (explicit > constructor > TRUE) a function(input, output, session) { qc_no_history$server(enable_bookmarking = TRUE) }, - {} + { + expect_s3_class(captured, "chat_history_config") + expect_equal(captured$restore_mode, "bookmark") + } + ) + ) + + # `history` (explicit or from the constructor) always wins over + # `enable_bookmarking`, which only fills in when neither was set. + suppressWarnings( + shiny::testServer( + function(input, output, session) { + qc_no_history$server(history = FALSE, enable_bookmarking = TRUE) + }, + { + expect_false(captured) + } + ) + ) + + suppressWarnings( + shiny::testServer( + function(input, output, session) qc$server(enable_bookmarking = TRUE), + { + expect_false(captured) + } ) ) }) From 5a9460192007bb88f8f24b3616dc34ade40e3239 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:32:45 -0500 Subject: [PATCH 08/16] fix(pkg-py): map deprecated enable_bookmarking=True to bookmark-mode history Same issue as the R side: .server(enable_bookmarking=True) and QueryChatExpress(enable_bookmarking=True) warned but the value was then discarded, silently resolving to history=True instead of bookmark-based restore. Map enable_bookmarking=True to HistoryOptions(restore_mode="bookmark") when history wasn't otherwise set, on both QueryChat.server() and QueryChatExpress's lazily-started server. Also aligns the two deprecation warning messages, which previously said different things ("for the equivalent behavior" on the R side vs. "no longer has any effect" here). --- pkg-py/src/querychat/_shiny.py | 43 +++++++++++++++++-------- pkg-py/tests/test_shiny.py | 58 +++++++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 17 deletions(-) diff --git a/pkg-py/src/querychat/_shiny.py b/pkg-py/src/querychat/_shiny.py index 6926f2ed..ba1d69eb 100644 --- a/pkg-py/src/querychat/_shiny.py +++ b/pkg-py/src/querychat/_shiny.py @@ -526,8 +526,9 @@ def server( constructor for this call only. Defaults to `True` when neither this nor the constructor's `history` was set. enable_bookmarking - Deprecated. Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` - instead (set on the `QueryChat` constructor, or passed here). + Deprecated. `True` resolves to `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` + when neither this call's `history` nor the constructor's `history` was set. Use `history=` + directly instead (set on the `QueryChat` constructor, or passed here). id Optional module ID for the QueryChat instance. If not provided, will use the ID provided at initialization. This must match the ID @@ -555,21 +556,30 @@ def server( def create_session_client(**kwargs) -> chatlas.Chat: return self._create_session_client(base=resolved_client, **kwargs) - resolved_history: bool | HistoryOptions = ( - history - if history is not None - else (self.history if self.history is not None else True) - ) - if enable_bookmarking is not None: warnings.warn( - "`enable_bookmarking` is deprecated and no longer has any effect. " + "`enable_bookmarking` is deprecated. " 'Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` ' - "instead (on the QueryChat constructor or passed to .server()).", + "instead (on the QueryChat constructor or passed to .server()) " + "for the equivalent behavior.", FutureWarning, stacklevel=2, ) + resolved_history: bool | HistoryOptions = ( + history + if history is not None + else ( + self.history + if self.history is not None + else ( + HistoryOptions(restore_mode="bookmark") + if enable_bookmarking + else True + ) + ) + ) + self._mark_server_initialized() return mod_server( id or self.id, @@ -821,13 +831,14 @@ def __init__( if enable_bookmarking != "auto": warnings.warn( - "`enable_bookmarking` is deprecated and no longer has any effect. " + "`enable_bookmarking` is deprecated. " 'Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` ' - "instead.", + "instead, for the equivalent behavior.", FutureWarning, stacklevel=2, ) + self._enable_bookmarking = enable_bookmarking self._vals: ServerValues[IntoFrameT] | None = None def _ensure_server_started(self) -> None: @@ -849,7 +860,13 @@ def _ensure_server_started(self) -> None: self._require_initialized("_ensure_server_started") self._mark_server_initialized() resolved_history: bool | HistoryOptions = ( - self.history if self.history is not None else True + self.history + if self.history is not None + else ( + HistoryOptions(restore_mode="bookmark") + if self._enable_bookmarking is True + else True + ) ) self._vals = mod_server( self.id, diff --git a/pkg-py/tests/test_shiny.py b/pkg-py/tests/test_shiny.py index be46410d..4ecbc31e 100644 --- a/pkg-py/tests/test_shiny.py +++ b/pkg-py/tests/test_shiny.py @@ -59,15 +59,25 @@ def fake_mod_server(*args, **kwargs): qc.server() assert captured["history"] is True - # Explicit enable_bookmarking=True/False: warns, but no longer has any - # effect on its own -- mod_server() no longer takes it at all. + # Explicit enable_bookmarking=True warns, and -- since history wasn't + # otherwise set -- resolves to bookmark-mode history (the equivalent + # of the old bookmarking behavior). + from shinychat.types import HistoryOptions + with pytest.warns(FutureWarning, match="history"): qc.server(enable_bookmarking=True) - assert "legacy_enable_bookmarking" not in captured + assert isinstance(captured["history"], HistoryOptions) + assert captured["history"].restore_mode == "bookmark" + # enable_bookmarking=False warns but has no effect on its own. with pytest.warns(FutureWarning, match="history"): qc.server(enable_bookmarking=False) - assert "legacy_enable_bookmarking" not in captured + assert captured["history"] is True + + # Explicit history= still takes precedence over enable_bookmarking. + with pytest.warns(FutureWarning, match="history"): + qc.server(history=False, enable_bookmarking=True) + assert captured["history"] is False # Explicit history= takes precedence over self.history. qc.history = False @@ -82,6 +92,11 @@ def fake_mod_server(*args, **kwargs): qc.server() assert captured["history"] is False + # self.history also takes precedence over the enable_bookmarking mapping. + with pytest.warns(FutureWarning, match="history"): + qc.server(enable_bookmarking=True) + assert captured["history"] is False + def test_app_defaults_history_to_bookmark_restore_mode_and_enables_shiny_bookmarking(): @@ -136,6 +151,41 @@ def test_express_enable_bookmarking_auto_emits_no_warning_and_uses_history(): QueryChatExpress(pd.DataFrame({"a": [1, 2, 3]}), "a_table") +def test_express_enable_bookmarking_resolves_to_bookmark_mode_history(monkeypatch): + """ + enable_bookmarking=True at construction time must map to bookmark-mode + history once the real server starts, mirroring QueryChat.server()'s + resolution (see test_server_resolves_history_and_warns_on_explicit_enable_bookmarking). + """ + from unittest.mock import MagicMock + + import pandas as pd + from querychat._shiny import QueryChatExpress + from shiny._namespaces import Root + from shiny.session import session_context + from shinychat.types import HistoryOptions + + captured = {} + + def fake_mod_server(*args, **kwargs): + captured.update(kwargs) + return MagicMock() + + monkeypatch.setattr("querychat._shiny.mod_server", fake_mod_server) + + mock_session = MagicMock() + mock_session.ns = Root + with session_context(mock_session): + with pytest.warns(FutureWarning, match="history"): + qc = QueryChatExpress( + pd.DataFrame({"a": [1, 2, 3]}), "a_table", enable_bookmarking=True + ) + qc._ensure_server_started() + + assert isinstance(captured["history"], HistoryOptions) + assert captured["history"].restore_mode == "bookmark" + + def test_express_explicit_enable_bookmarking_warns(): from unittest.mock import MagicMock, patch From 5806125caf2d186a7676452357f1d0c36784af0e Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:33:03 -0500 Subject: [PATCH 09/16] fix(pkg-r): skip chat_restore() when history is already bookmark mode chat_server() registers chat_enable_history() internally whenever `history` isn't FALSE, and shinychat's own docs call chat_enable_history() and chat_restore() mutually exclusive -- both independently try to restore the client's turns from a Shiny bookmark. Registering chat_restore() too, as querychat did unconditionally, meant doubled per-turn storage on every save and a visible flicker on restore whenever history's restore_mode was "bookmark" (chat_restore's onRestore fires first from the raw bookmark payload, then chat_enable_history's own restore clears and re-renders from its own conversation-store copy). Skip chat_restore() in that case; it's still registered otherwise, so the chat client's own state continues to round-trip through Shiny bookmarks the host app might trigger for unrelated reasons. --- pkg-r/R/querychat_module.R | 29 +++++---- pkg-r/tests/testthat/helper-fixtures.R | 4 +- pkg-r/tests/testthat/test-querychat_module.R | 67 +++++++++++++++++--- 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index c8d4e12e..f050ddba 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -120,19 +120,26 @@ mod_server <- function( history = history ) - # Registered unconditionally so the chat client's own state (and greeting) - # round-trip through Shiny bookmarks whenever the host app has bookmarking - # enabled -- independent of `history`. Only the save/restore hooks are + # Skipped when `history` is already in bookmark mode: chat_server() has + # then already registered chat_enable_history() for this id/client, and + # shinychat docs call chat_enable_history() and chat_restore() mutually + # exclusive. Otherwise, register chat_restore() so the chat client's own + # state (and greeting) still round-trip through Shiny bookmarks the host + # app might trigger for unrelated reasons. Only the save/restore hooks are # wanted here; the automatic bookmark triggers are turned off so `history` # (or the host app) remains the sole source of *when* to bookmark. - shinychat::chat_restore( - "chat", - pre_built_client, - bookmark_on_input = FALSE, - bookmark_on_response = FALSE, - restore_ui = FALSE, - session = session - ) + history_is_bookmark_mode <- inherits(history, "chat_history_config") && + identical(history$restore_mode, "bookmark") + if (!history_is_bookmark_mode) { + shinychat::chat_restore( + "chat", + pre_built_client, + bookmark_on_input = FALSE, + bookmark_on_response = FALSE, + restore_ui = FALSE, + session = session + ) + } shiny::observeEvent( input$chat_update, diff --git a/pkg-r/tests/testthat/helper-fixtures.R b/pkg-r/tests/testthat/helper-fixtures.R index d69732db..5180cf59 100644 --- a/pkg-r/tests/testthat/helper-fixtures.R +++ b/pkg-r/tests/testthat/helper-fixtures.R @@ -179,8 +179,8 @@ mock_ellmer_chat_client <- function( # shinychat::chat_restore() validates that `client` is an ellmer::Chat() R6 # object; the lightweight `structure(list(), class = c("MockChat", "Chat"))` # fakes used throughout this file don't satisfy that. mod_server() calls -# chat_restore() unconditionally, so tests that don't care about its behavior -# need a no-op stand-in. +# chat_restore() whenever `history` isn't in bookmark mode, so tests that +# don't care about its behavior need a no-op stand-in. local_mock_chat_restore <- function(env = parent.frame()) { testthat::local_mocked_bindings( chat_restore = function(...) invisible(NULL), diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index f4a6b910..659feb07 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -42,7 +42,8 @@ test_that("mod_server() return includes table() and table_names() for single-tab executor = executor, greeting = "Hello", client = client_factory, - tools = "query" + tools = "query", + history = TRUE ), { # Verify the returned list exposes table() and table_names() @@ -102,7 +103,8 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl executor = executor, greeting = "Hello", client = client_factory, - tools = "query" + tools = "query", + history = TRUE ), { # Verify the returned list exposes table() and table_names() @@ -155,7 +157,8 @@ test_that("mod_server() passes visualize callback and tools to client factory", executor = executor, greeting = "Hello", client = client_factory, - tools = c("query", "visualize") + tools = c("query", "visualize"), + history = TRUE ), { expect_type(captured, "list") @@ -192,7 +195,8 @@ test_that("mod_server() exposes current_table() starting as NULL", { executor = executor, greeting = "Hello", client = client_factory, - tools = "query" + tools = "query", + history = TRUE ), { expect_true(is.function(session$returned$current_table)) @@ -230,7 +234,8 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu executor = executor, greeting = "Hello", client = client_factory, - tools = "query" + tools = "query", + history = TRUE ), { # Initially NULL @@ -355,7 +360,8 @@ test_that("restored viz widgets survive a second bookmark cycle", { executor = executor, greeting = "Hello", client = client_factory, - tools = c("query", "visualize") + tools = c("query", "visualize"), + history = TRUE ), { expect_true(is.function(bookmark_fn)) @@ -419,7 +425,8 @@ test_that("mod_server() calls chat_server('chat', ...) with the pre-built client executor = executor, greeting = "Hello", client = client_factory, - tools = "query" + tools = "query", + history = TRUE ), { expect_equal(captured_chat_args$id, "chat") @@ -428,7 +435,7 @@ test_that("mod_server() calls chat_server('chat', ...) with the pre-built client ) }) -test_that("mod_server() always calls chat_restore() with the auto-bookmark trigger disabled", { +test_that("mod_server() calls chat_restore() with the auto-bookmark trigger disabled when history isn't bookmark mode", { skip_if_no_dataframe_engine() ds <- local_data_frame_source(new_test_df()) @@ -472,6 +479,44 @@ test_that("mod_server() always calls chat_restore() with the auto-bookmark trigg ) }) +test_that("mod_server() skips chat_restore() when history is bookmark mode", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + chat_restore_called <- FALSE + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + chat_restore = function(id, client, ...) { + chat_restore_called <<- TRUE + invisible(NULL) + }, + .package = "shinychat" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = shinychat::history_options(restore_mode = "bookmark") + ), + { + expect_false(chat_restore_called) + } + ) +}) + test_that("mod_server() builds the auto-generated greeting from the greeter, not the main client", { skip_if_no_dataframe_engine() @@ -522,7 +567,8 @@ test_that("mod_server() builds the auto-generated greeting from the greeter, not client = client_factory, tools = "query", greeter = fake_greeter, - greeting_base = "base-client" + greeting_base = "base-client", + history = TRUE ), { expect_true(is.function(captured_greeting_arg)) @@ -564,7 +610,8 @@ test_that("mod_server() chat_update input updates table state", { executor = executor, greeting = "Hello", client = client_factory, - tools = "query" + tools = "query", + history = TRUE ), { session$setInputs( From 827e322a2f2fed3df86ba22b272179af785797f8 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:33:27 -0500 Subject: [PATCH 10/16] fix(pkg-py): skip enable_bookmarking() when history is already bookmark mode Mirrors the R fix: shinychat.Chat(history=) already enables self.history (the Python equivalent of chat_enable_history()) whenever history isn't False, and shinychat treats it and enable_bookmarking() as mutually exclusive -- both independently try to restore the client's turns from a Shiny bookmark. Registering enable_bookmarking() too, as querychat did unconditionally, meant doubled per-turn storage on every save and a visible flicker on restore whenever history's restore_mode was "bookmark". Skip enable_bookmarking() in that case; it's still registered otherwise, so the chat client's own state continues to round-trip through Shiny bookmarks the host app might trigger for unrelated reasons. --- pkg-py/src/querychat/_shiny_module.py | 22 +++++---- pkg-py/tests/test_shiny_module.py | 68 ++++++++++++++++++++++++--- 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/pkg-py/src/querychat/_shiny_module.py b/pkg-py/src/querychat/_shiny_module.py index decf9193..efc12a4f 100644 --- a/pkg-py/src/querychat/_shiny_module.py +++ b/pkg-py/src/querychat/_shiny_module.py @@ -8,6 +8,7 @@ import chatlas import shinychat from narwhals.stable.v1.typing import IntoFrameT +from shinychat.types import HistoryOptions from shiny import module, reactive, ui @@ -21,7 +22,6 @@ from collections.abc import Callable from shiny.bookmark import BookmarkState, RestoreState - from shinychat.types import HistoryOptions from shiny import Inputs, Outputs, Session @@ -324,13 +324,19 @@ async def _make_greeting(): history=history, ) - # Registered unconditionally so the chat client's own state (and greeting) - # round-trip through Shiny bookmarks whenever the host app has bookmarking - # enabled -- independent of `history`. Only the save/restore hooks are - # wanted here; the automatic bookmark trigger is turned off (bookmark_on= - # None) so `history` (or the host app) remains the sole source of *when* - # to bookmark. - shinychat_chat.enable_bookmarking(chat, bookmark_on=None) + # Skipped when `history` is already in bookmark mode: shinychat_chat.history + # is then already enabled for this chat/client, and shinychat treats it and + # enable_bookmarking() as mutually exclusive. Otherwise, register + # enable_bookmarking() so the chat client's own state (and greeting) still + # round-trip through Shiny bookmarks the host app might trigger for + # unrelated reasons. Only the save/restore hooks are wanted here; the + # automatic bookmark trigger is turned off (bookmark_on=None) so `history` + # (or the host app) remains the sole source of *when* to bookmark. + history_is_bookmark_mode = ( + isinstance(history, HistoryOptions) and history.restore_mode == "bookmark" + ) + if not history_is_bookmark_mode: + shinychat_chat.enable_bookmarking(chat, bookmark_on=None) # Handle update button clicks @reactive.effect diff --git a/pkg-py/tests/test_shiny_module.py b/pkg-py/tests/test_shiny_module.py index bd906db1..cbed7581 100644 --- a/pkg-py/tests/test_shiny_module.py +++ b/pkg-py/tests/test_shiny_module.py @@ -118,13 +118,13 @@ def client_factory(**kwargs): assert callable(captured.get("greeting")), "greeting= should be a callable" -def test_mod_server_always_registers_chat_bookmarking_with_no_auto_trigger(): +def test_mod_server_registers_chat_bookmarking_with_no_auto_trigger_when_history_not_bookmark_mode(): """ - Chat.enable_bookmarking() must always be called -- independent of `history` - -- so the chat client's own state round-trips through Shiny bookmarks - whenever the host app has bookmarking enabled. It's called with - bookmark_on=None so `history` (or the host app) remains the sole source of - *when* to bookmark, not shinychat's own auto-trigger. + Chat.enable_bookmarking() is called when `history` isn't bookmark mode, so + the chat client's own state round-trips through Shiny bookmarks whenever + the host app has bookmarking enabled. It's called with bookmark_on=None so + `history` (or the host app) remains the sole source of *when* to + bookmark, not shinychat's own auto-trigger. """ from unittest.mock import MagicMock, patch @@ -176,6 +176,62 @@ def client_factory(**kwargs): assert kwargs.get("bookmark_on") is None +def test_mod_server_skips_chat_bookmarking_when_history_is_bookmark_mode(): + """ + Chat.enable_bookmarking() is skipped when `history` is bookmark mode -- + shinychat_chat.history is already enabled for this chat/client in that + case, and shinychat treats it and enable_bookmarking() as mutually + exclusive. + """ + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + from shinychat.types import HistoryOptions + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=HistoryOptions(restore_mode="bookmark"), + tools=None, + greeter=MagicMock(), + ) + + fake_chat_instance.enable_bookmarking.assert_not_called() + + def test_mod_server_registers_table_state_with_both_bookmark_and_history_hooks(): """Table/viz state callbacks must always register with both APIs, unconditionally.""" from unittest.mock import MagicMock, patch From 449a15ebcf0d713f93a254f11d93888845c47e76 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:33:43 -0500 Subject: [PATCH 11/16] test(pkg-py): add a non-mocked contract test for mod_server's shinychat usage Every existing mod_server() test patches shinychat.Chat with a fake constructor, so an upstream rename/removal of Chat(client=, greeting=, history=), .history.on_save()/.history.on_restore(), or .enable_bookmarking(client, bookmark_on=) wouldn't turn any of them red -- only the playwright e2e tests and the manual bookmark/restore test-plan step would catch it. Add one thin test that constructs the real shinychat.Chat and calls the real methods with the same arguments mod_server() uses, so a signature drift in that surface fails loudly here too. --- pkg-py/tests/test_shiny_module.py | 40 +++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/pkg-py/tests/test_shiny_module.py b/pkg-py/tests/test_shiny_module.py index cbed7581..e92a0779 100644 --- a/pkg-py/tests/test_shiny_module.py +++ b/pkg-py/tests/test_shiny_module.py @@ -286,3 +286,43 @@ def client_factory(**kwargs): fake_session.bookmark.on_restore.assert_called_once() fake_chat_instance.history.on_save.assert_called_once() fake_chat_instance.history.on_restore.assert_called_once() + + +def test_shinychat_chat_contract_used_by_mod_server(): + """ + Thin, non-mocked check of the shinychat surface mod_server() depends on. + + Every other test above patches `shinychat.Chat` with a fake constructor, + so an upstream rename/removal of `Chat(client=, greeting=, history=)`, + `.history.on_save()`/`.history.on_restore()`, or + `.enable_bookmarking(client, bookmark_on=)` wouldn't turn any of them red. + This constructs the real shinychat.Chat and calls the real methods with + the same arguments mod_server() uses, so it does. + """ + from unittest.mock import MagicMock + + import shinychat + from shiny._namespaces import Root + from shiny.session import session_context + + mock_session = MagicMock() + mock_session.ns = Root + mock_session.app = None + + with session_context(mock_session): + chat = shinychat.Chat( + "chat", client=MagicMock(), greeting=None, history=True + ) + + @chat.history.on_save + def _on_save(values): + return values + + @chat.history.on_restore + def _on_restore(values): + return values + + chat.enable_bookmarking(MagicMock(), bookmark_on=None) + + assert _on_save in chat.history._save_callbacks + assert _on_restore in chat.history._restore_callbacks From cb237440368e7aa5ed3eb7b2e80eef781dab08ca Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:33:59 -0500 Subject: [PATCH 12/16] test(pkg-py): fail loudly on viz-footer stabilization timeout; tolerate jitter _wait_for_stable_position() silently returned once max_wait_s elapsed even if the locator's bounding box never settled, so a genuine layout regression could masquerade as "settled" and pass. It also compared bounding boxes for exact float equality, which is jitter-sensitive. Raise a clear pytest.fail() with the last observed bounding box on deadline exhaustion, and compare boxes within a small pixel tolerance instead of exact equality. (playwright-python 1.58 doesn't expose expect.poll() for a custom condition like this, so the hand-rolled polling loop stays, just made stricter.) --- pkg-py/tests/playwright/test_11_viz_footer.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg-py/tests/playwright/test_11_viz_footer.py b/pkg-py/tests/playwright/test_11_viz_footer.py index d556beaf..be80a283 100644 --- a/pkg-py/tests/playwright/test_11_viz_footer.py +++ b/pkg-py/tests/playwright/test_11_viz_footer.py @@ -25,11 +25,20 @@ TOOL_RESULT_TIMEOUT = 90_000 +def _boxes_match( + a: dict[str, float] | None, b: dict[str, float] | None, tolerance_px: float +) -> bool: + if a is None or b is None: + return a is b + return all(abs(a[key] - b[key]) <= tolerance_px for key in a) + + def _wait_for_stable_position( locator: Locator, quiet_checks: int = 5, interval_s: float = 0.1, max_wait_s: float = 8.0, + tolerance_px: float = 0.5, ) -> None: """ Wait until `locator`'s bounding box stops changing. @@ -39,13 +48,16 @@ def _wait_for_stable_position( round trip for its responsive fill layout. That drags the footer button below it along for the ride, so a click issued mid-drift can land on stale coordinates and silently miss the button. + + Comparing boxes within `tolerance_px` (rather than exact equality) avoids + false "still moving" reads from sub-pixel rendering jitter. """ deadline = time.monotonic() + max_wait_s last_box = None stable_count = 0 while time.monotonic() < deadline: box = locator.bounding_box() - if box is not None and box == last_box: + if box is not None and _boxes_match(box, last_box, tolerance_px): stable_count += 1 if stable_count >= quiet_checks: return @@ -54,6 +66,11 @@ def _wait_for_stable_position( last_box = box time.sleep(interval_s) + pytest.fail( + f"Locator position did not stabilize within {max_wait_s}s " + f"(last bounding box: {last_box})" + ) + def _download_from_save_menu(page: Page, export_format: str) -> tuple[Download, str]: """Open the save menu, click the requested format, and capture the download.""" From 570392bc1066e2a1a514b08dfb7281faeaaa2e20 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 10:34:18 -0500 Subject: [PATCH 13/16] docs: call out the bookmark storage-mechanism change in $app()/.app() The existing NEWS/CHANGELOG entry described the bookmark_store removal as a behavior-preserving rename ("existing callers keep working without changes"), which is true functionally but glosses over a real deployment difference: the old default (bookmark_store="url") encoded the whole bookmark state in the URL with no server storage, while the new default requires server-side bookmark storage. Spell that out so anyone relying on the old stateless behavior isn't surprised. --- pkg-py/CHANGELOG.md | 2 +- pkg-r/NEWS.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index 5239e87a..519917d4 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -41,7 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The `data_source` property has been removed. Use `qc.table("name").data_source` to read a table's data source, and `qc.add_table(df, "name", replace=True)` to replace it. The `data_source` parameter to `server()` (Shiny) has also been removed; call `add_table()` before `server()` instead. (#195) -* `.app()`'s `bookmark_store` parameter has been removed. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `.app()` defaults to `restore_mode="bookmark"` when no `history` is set anywhere, so existing `.app()` callers keep working without changes. +* `.app()`'s `bookmark_store` parameter has been removed. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `.app()` defaults to `restore_mode="bookmark"` when no `history` is set anywhere, so existing `.app()` callers keep working without changes. Note this default is a storage-mechanism change, not just a rename: the old default (`bookmark_store="url"`) encoded the entire bookmark state in the URL itself, requiring no server storage; the new default requires server-side bookmark storage (`bookmark_store="server"`), with just a short state ID in the URL. Deployments that relied on `.app()` being fully stateless should pass `history=False` or a non-bookmark `HistoryOptions()`. * `QueryChat`/`QueryChatExpress` now default to `history=True` (shinychat's own default, browser-localStorage-backed conversation history) instead of no persistence. Pass `history=False` to opt back out. diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index c6a8c04c..55528156 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -33,7 +33,7 @@ * The `$data_source` property has been removed. Use `qc$table("name")$data_source` to read a table's data source, and `qc$add_table(df, "name", replace = TRUE)` to replace it. The `data_source` parameter to `$server()` has also been removed; call `$add_table()` before `$server()` instead. (#195) -* `$app()`/`$app_obj()`'s `bookmark_store` parameter has been removed. Pass `history = shinychat::history_options(restore_mode = "bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `$app()` defaults to `restore_mode = "bookmark"` when no `history` is set anywhere, so existing `$app()` callers keep working without changes. +* `$app()`/`$app_obj()`'s `bookmark_store` parameter has been removed. Pass `history = shinychat::history_options(restore_mode = "bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `$app()` defaults to `restore_mode = "bookmark"` when no `history` is set anywhere, so existing `$app()` callers keep working without changes. Note this default is a storage-mechanism change, not just a rename: the old default (`bookmark_store = "url"`) encoded the entire bookmark state in the URL itself, requiring no server storage; the new default requires server-side bookmark storage (`bookmarkStore = "server"`), with just a short state ID in the URL. Deployments that relied on `$app()` being fully stateless should pass `history = FALSE` or a non-bookmark `history_options()`. * `QueryChat` now defaults to `history = TRUE` (shinychat's own default, browser-localStorage-backed conversation history) instead of no persistence. Pass `history = FALSE` to opt back out. From c003a877ef30517207e847cf4a3db6d05cb2eae8 Mon Sep 17 00:00:00 2001 From: cpsievert Date: Fri, 10 Jul 2026 15:59:18 +0000 Subject: [PATCH 14/16] `air format` (GitHub Actions) --- pkg-r/R/QueryChat.R | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 3c6bda5b..38a92d79 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -1022,11 +1022,9 @@ QueryChat <- R6::R6Class( resolved_history <- history %||% self$history %||% - ( - if (isTRUE(enable_bookmarking)) { - shinychat::history_options(restore_mode = "bookmark") - } - ) %||% + (if (isTRUE(enable_bookmarking)) { + shinychat::history_options(restore_mode = "bookmark") + }) %||% TRUE result <- mod_server( From 494d55f11b69791306965606858669e7c02996a7 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 11:01:00 -0500 Subject: [PATCH 15/16] chore(pkg-py): drop stale bookmark_store="url" from viz example QueryChat now defaults to history=True (shinychat's browser-localStorage history) instead of no persistence, so this example doesn't need to opt into Shiny-level URL bookmarking on top of that. Also drops the now-unused app_opts import. --- pkg-py/examples/10-viz-app.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg-py/examples/10-viz-app.py b/pkg-py/examples/10-viz-app.py index 0702d3c8..857e8421 100644 --- a/pkg-py/examples/10-viz-app.py +++ b/pkg-py/examples/10-viz-app.py @@ -1,9 +1,8 @@ from pathlib import Path -from querychat.express import QueryChat from querychat.data import titanic - -from shiny.express import ui, app_opts +from querychat.express import QueryChat +from shiny.express import ui greeting = Path(__file__).parent / "greeting-viz.md" @@ -18,5 +17,3 @@ qc.ui() ui.page_opts(fillable=True, title="QueryChat Visualization Demo") - -app_opts(bookmark_store="url") From 3b052341b00313cb906705e6d641e999ee02b77c Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 10 Jul 2026 14:44:25 -0500 Subject: [PATCH 16/16] cleanup changelog/news --- pkg-py/CHANGELOG.md | 4 ++-- pkg-r/NEWS.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index 519917d4..be52927c 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -37,14 +37,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * File attachments are now enabled by default in the Shiny chat UI. Users can attach images, PDFs, and text files to their messages and the LLM will receive them. Disable with `allow_attachments=False` in `mod_ui()` or `QueryChat.ui()`. (#253) +* Conversation history is now persisted by default. `QueryChat`/`QueryChatExpress` keep a user's chat around across page reloads and browser sessions, backed by shinychat's history support. The default `restore_mode="browser"` stores the active conversation in the browser's localStorage, but you can pass `history=shinychat.types.HistoryOptions(restore_mode="url")` to restore via a plain, shareable URL instead, or `restore_mode="bookmark"` to fold the conversation into a full Shiny bookmark. Disable with `history=False`. + ### Breaking Changes * The `data_source` property has been removed. Use `qc.table("name").data_source` to read a table's data source, and `qc.add_table(df, "name", replace=True)` to replace it. The `data_source` parameter to `server()` (Shiny) has also been removed; call `add_table()` before `server()` instead. (#195) * `.app()`'s `bookmark_store` parameter has been removed. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `.app()` defaults to `restore_mode="bookmark"` when no `history` is set anywhere, so existing `.app()` callers keep working without changes. Note this default is a storage-mechanism change, not just a rename: the old default (`bookmark_store="url"`) encoded the entire bookmark state in the URL itself, requiring no server storage; the new default requires server-side bookmark storage (`bookmark_store="server"`), with just a short state ID in the URL. Deployments that relied on `.app()` being fully stateless should pass `history=False` or a non-bookmark `HistoryOptions()`. -* `QueryChat`/`QueryChatExpress` now default to `history=True` (shinychat's own default, browser-localStorage-backed conversation history) instead of no persistence. Pass `history=False` to opt back out. - ### Deprecated * `.server()`'s and `QueryChatExpress`'s `enable_bookmarking` parameter is deprecated in favor of `history`. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` instead of `enable_bookmarking=True` for the equivalent behavior. diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 55528156..504a803a 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -29,14 +29,14 @@ * File attachments are now enabled by default in the Shiny chat UI. Users can attach images, PDFs, and text files to their messages and the LLM will receive them. Disable with `allow_attachments = FALSE` in `mod_ui()` or `QueryChat$ui()`. (#253) +* Conversation history is now persisted by default. `QueryChat` keeps a user's chat around across page reloads and browser sessions, backed by shinychat's history support. The default `restore_mode = "browser"` stores the active conversation in the browser's localStorage, but you can pass `history = shinychat::history_options(restore_mode = "url")` to restore via a plain, shareable URL instead, or `restore_mode = "bookmark"` to fold the conversation into a full Shiny bookmark. Disable with `history = FALSE`. + ## Breaking changes * The `$data_source` property has been removed. Use `qc$table("name")$data_source` to read a table's data source, and `qc$add_table(df, "name", replace = TRUE)` to replace it. The `data_source` parameter to `$server()` has also been removed; call `$add_table()` before `$server()` instead. (#195) * `$app()`/`$app_obj()`'s `bookmark_store` parameter has been removed. Pass `history = shinychat::history_options(restore_mode = "bookmark")` to get the same shareable-bookmark behavior; any other `history` value disables Shiny-level bookmarking for the generated app. `$app()` defaults to `restore_mode = "bookmark"` when no `history` is set anywhere, so existing `$app()` callers keep working without changes. Note this default is a storage-mechanism change, not just a rename: the old default (`bookmark_store = "url"`) encoded the entire bookmark state in the URL itself, requiring no server storage; the new default requires server-side bookmark storage (`bookmarkStore = "server"`), with just a short state ID in the URL. Deployments that relied on `$app()` being fully stateless should pass `history = FALSE` or a non-bookmark `history_options()`. -* `QueryChat` now defaults to `history = TRUE` (shinychat's own default, browser-localStorage-backed conversation history) instead of no persistence. Pass `history = FALSE` to opt back out. - ## Deprecated * `$server()`'s `enable_bookmarking` parameter is deprecated in favor of `history`. Pass `history = shinychat::history_options(restore_mode = "bookmark")` instead of `enable_bookmarking = TRUE` for the equivalent behavior.