From 0da87a7c56bbd541b7119afbd89a6b562d51161c Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 4 Sep 2026 22:15:25 -0500 Subject: [PATCH 1/3] fix: save history/bookmark state without a reactive context or list assumption Two bugs surfaced when shinychat saves history from a promises::then() callback after a response stream completes (no reactive context, especially after a tool round-trip): * build_state_snapshot() read the sql/title reactiveVals bare, erroring with "Operation not allowed without an active reactive context". The reads are now wrapped in shiny::isolate(). * The onBookmark callback replaced state$values via modifyList(), but ShinySaveState$values is a Shiny-owned environment that cannot be replaced (and is not a list). The callback now mutates items in place. Tests now exercise on_save outside any reactive context and use an environment-backed state$values, matching Shiny's real contract. --- pkg-r/R/querychat_module.R | 14 ++-- pkg-r/tests/testthat/test-querychat_module.R | 69 ++++++++++++++++++-- 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index 978f129d..7dc8d300 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -158,11 +158,12 @@ mod_server <- function( ) build_state_snapshot <- function() { + # on_save can run from a promise handler with no reactive context. table_states <- list() for (name in names(tables)) { table_states[[name]] <- list( - sql = tables[[name]]$sql(), - title = tables[[name]]$title() + sql = shiny::isolate(tables[[name]]$sql()), + title = shiny::isolate(tables[[name]]$title()) ) } snapshot <- list(querychat_tables = table_states) @@ -209,10 +210,11 @@ mod_server <- function( shiny::setBookmarkExclude("chat_update") shiny::onBookmark(function(state) { - state$values <- utils::modifyList( - state$values %||% list(), - build_state_snapshot() - ) + # state$values is a Shiny-owned environment: mutate, never replace. + snapshot <- build_state_snapshot() + for (nm in names(snapshot)) { + state$values[[nm]] <- snapshot[[nm]] + } }) shiny::onRestore(function(state) { diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index 10d5fe20..af30f301 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -377,8 +377,9 @@ test_that("restored viz widgets survive a second bookmark cycle", { shiny::isolate(callbacks$visualize(saved[[1]])) + # ShinySaveState$values is an environment, not a list. first_state <- new.env(parent = emptyenv()) - first_state$values <- list() + first_state$values <- new.env(parent = emptyenv()) shiny::isolate(bookmark_fn(first_state)) expect_equal(first_state$values$querychat_viz_widgets, saved) @@ -389,14 +390,14 @@ test_that("restored viz widgets survive a second bookmark cycle", { expect_equal(restored_args$saved_widgets, saved) second_state <- new.env(parent = emptyenv()) - second_state$values <- list() + second_state$values <- new.env(parent = emptyenv()) shiny::isolate(bookmark_fn(second_state)) expect_equal(second_state$values$querychat_viz_widgets, saved) } ) }) -test_that("onBookmark callback tolerates NULL state$values", { +test_that("onBookmark callback mutates environment-backed state$values", { skip_if_no_dataframe_engine() ds <- local_data_frame_source(new_test_df()) @@ -436,9 +437,8 @@ test_that("onBookmark callback tolerates NULL state$values", { expect_true(is.function(bookmark_fn)) state <- new.env(parent = emptyenv()) - state$values <- NULL + state$values <- new.env(parent = emptyenv()) expect_no_error(shiny::isolate(bookmark_fn(state))) - expect_true(is.list(state$values)) expect_true("querychat_tables" %in% names(state$values)) } ) @@ -803,3 +803,62 @@ test_that("history on_save callback returns merged values (R history contract)", } ) }) + +test_that("history on_save callback works without an active reactive context", { + 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" + ) + ) + } + ) + + # shinychat invokes on_save from a promise handler with no reactive context. + expect_no_error(result <- history_save_fn(list())) + expect_equal( + result$querychat_tables$test_table$sql, + "SELECT * FROM test_table WHERE id = 1" + ) +}) From 81c1de9dfff00f735b587fe3c9f31667fdb91ea8 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 4 Sep 2026 22:25:50 -0500 Subject: [PATCH 2/3] test: call on_save inside testServer so the module session is still alive --- pkg-r/tests/testthat/test-querychat_module.R | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index af30f301..c23390f4 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -852,13 +852,15 @@ test_that("history on_save callback works without an active reactive context", { title = "One row" ) ) + # shinychat invokes on_save from a promise handler with no reactive + # context. testServer's expression provides no reactive consumer context + # either, so calling the callback bare here reproduces that situation + # (while the module session is still alive). + expect_no_error(result <- history_save_fn(list())) + expect_equal( + result$querychat_tables$test_table$sql, + "SELECT * FROM test_table WHERE id = 1" + ) } ) - - # shinychat invokes on_save from a promise handler with no reactive context. - expect_no_error(result <- history_save_fn(list())) - expect_equal( - result$querychat_tables$test_table$sql, - "SELECT * FROM test_table WHERE id = 1" - ) }) From f66784f4e2a02736f85598bdce9f88bde986ddfe Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 4 Sep 2026 22:36:04 -0500 Subject: [PATCH 3/3] test: condense comments --- pkg-r/tests/testthat/test-querychat_module.R | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index c23390f4..af82a615 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -377,7 +377,6 @@ test_that("restored viz widgets survive a second bookmark cycle", { shiny::isolate(callbacks$visualize(saved[[1]])) - # ShinySaveState$values is an environment, not a list. first_state <- new.env(parent = emptyenv()) first_state$values <- new.env(parent = emptyenv()) shiny::isolate(bookmark_fn(first_state)) @@ -852,10 +851,7 @@ test_that("history on_save callback works without an active reactive context", { title = "One row" ) ) - # shinychat invokes on_save from a promise handler with no reactive - # context. testServer's expression provides no reactive consumer context - # either, so calling the callback bare here reproduces that situation - # (while the module session is still alive). + # Called bare, as shinychat's promise handler would (no reactive context). expect_no_error(result <- history_save_fn(list())) expect_equal( result$querychat_tables$test_table$sql,