fix(pkg-r): querychat_app() should only close connections if it creates the DB (#164)

* fix(pkg-r): `querychat_app()` only closes connections if it creates the DB

* chore(pkg-r) Refactor for clarity

* chore: Add NEWS

* chore(pkg-r): Actually, only if a data frame

* tests(pkg-r): Test that cleanup is correctly set in querychat_app()

* feat: automatic cleanup QC when created in running Shiny app

* feat(querychat_app): Throw if called inside a Shiny app

* feat(querychat_app): Always clean up dataframe dbs

* chore: move table validation into `normalize_data_source()`

* chore(querychat_app): Resolve `cleanup` early

Author: gadenbuie

pkg-r/NEWS.md

@@ -1,5 +1,7 @@
 # querychat (development version)
 
+* `querychat_app()` will now only automatically clean up the data source if QueryChat creates the data source internally from a data frame. (#164)
+
 * **Breaking change:** The `$sql()` method now returns `NULL` instead of `""` (empty string) when no query has been set, aligning with the behavior of `$title()` for consistency. Most code using `isTruthy()` or similar falsy checks will continue working without changes. Code that explicitly checks `sql() == ""` should be updated to use falsy checks (e.g., `!isTruthy(sql())`) or explicit null checks (`is.null(sql())`). (#146)
 
 * Tool detail cards can now be expanded or collapsed by default when querychat runs a query or updates the dashboard via the `querychat.tool_details` R option or the `QUERYCHAT_TOOL_DETAILS` environment variable. Valid values are `"expanded"`, `"collapsed"`, or `"default"`. (#137)

pkg-r/R/QueryChat.R

@@ -166,9 +166,6 @@ QueryChat <- R6::R6Class(
 
       private$.data_source <- normalize_data_source(data_source, table_name)
 
-      # Validate table name
-      check_sql_table_name(table_name)
-
       self$id <- id %||% table_name
 
       if (!is.null(greeting) && file.exists(greeting)) {
@@ -192,7 +189,7 @@ QueryChat <- R6::R6Class(
 
       # By default, only close automatically if a Shiny session is active
       if (is.na(cleanup)) {
-        cleanup <- !is.null(shiny::getDefaultReactiveDomain())
+        cleanup <- shiny::isRunning()
       }
 
       if (cleanup) {
@@ -585,8 +582,11 @@ QueryChat <- R6::R6Class(
 #'   used. See the package prompts directory for the default template format.
 #' @param cleanup Whether or not to automatically run `$cleanup()` when the
 #'   Shiny session/app stops. By default, cleanup only occurs if `QueryChat`
-#'   gets created within a Shiny session. Set to `TRUE` to always clean up,
-#'   or `FALSE` to never clean up automatically.
+#'   is created within a Shiny app. Set to `TRUE` to always clean up, or
+#'   `FALSE` to never clean up automatically.
+#'
+#'   In `querychat_app()`, in-memory databases created for data frames are
+#'   always cleaned up.
 #'
 #' @return A `QueryChat` object. See [QueryChat] for available methods.
 #'
@@ -668,13 +668,26 @@ querychat_app <- function(
   categorical_threshold = 20,
   extra_instructions = NULL,
   prompt_template = NULL,
-  cleanup = TRUE,
+  cleanup = NA,
   bookmark_store = "url"
 ) {
+  if (shiny::isRunning()) {
+    cli::cli_abort(
+      "{.fn querychat_app} cannot be called from within a Shiny app. Use {.fn querychat} instead."
+    )
+  }
+
   if (is_missing(table_name) && is.data.frame(data_source)) {
     table_name <- deparse1(substitute(data_source))
   }
 
+  check_bool(cleanup, allow_na = TRUE)
+  if (is.data.frame(data_source)) {
+    cleanup <- TRUE
+  } else if (is.na(cleanup)) {
+    cleanup <- FALSE
+  }
+
   qc <- QueryChat$new(
     data_source = data_source,
     table_name = table_name,
@@ -697,6 +710,8 @@ normalize_data_source <- function(data_source, table_name) {
     return(data_source)
   }
 
+  check_sql_table_name(table_name, call = caller_env())
+
   if (is.data.frame(data_source)) {
     return(DataFrameSource$new(data_source, table_name))
   }

pkg-r/R/utils-shiny.R

@@ -0,0 +1,3 @@
+in_shiny_session <- function() {
+  !is.null(shiny::getDefaultReactiveDomain()) # nocov
+}

pkg-r/man/querychat-convenience.Rd

@@ -306,3 +306,39 @@ describe("normalize_data_source()", {
     })
   })
 })
+
+test_that("querychat_app() only cleans up data frame sources on exit", {
+  local_mocked_r6_class(
+    QueryChat,
+    public = list(
+      initialize = function(..., cleanup) {
+        # have to use an option because the code is evaluated in a far-away env
+        options(.test_cleanup = cleanup)
+      },
+      app = function(...) {}
+    )
+  )
+  withr::local_options(rlang_interactive = TRUE)
+
+  withr::with_options(list(.test_cleanup = NULL), {
+    test_df <- new_test_df()
+    querychat_app(test_df)
+    cleanup_result <- getOption(".test_cleanup")
+    expect_true(cleanup_result)
+  })
+
+  withr::with_options(list(.test_cleanup = NULL), {
+    test_ds <- local_data_frame_source(new_test_df())
+    querychat_app(test_ds)
+    cleanup_result <- getOption(".test_cleanup")
+    expect_false(cleanup_result)
+  })
+
+  withr::with_options(list(.test_cleanup = NULL), {
+    con <- local_sqlite_connection(new_test_df())
+    test_ds <- DBISource$new(con$conn, "test_table")
+    querychat_app(test_ds)
+    cleanup_result <- getOption(".test_cleanup")
+    expect_false(cleanup_result)
+  })
+})