fix: save history/bookmark state without a reactive context or list assumption

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) {

pkg-r/tests/testthat/test-querychat_module.R

@@ -378,7 +378,7 @@ test_that("restored viz widgets survive a second bookmark cycle", {
       shiny::isolate(callbacks$visualize(saved[[1]]))
 
       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 +389,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 +436,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 +802,61 @@ 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"
+        )
+      )
+      # 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,
+        "SELECT * FROM test_table WHERE id = 1"
+      )
+    }
+  )
+})