From 9da2841cba12a52d3744cbea1b79a842662c42d7 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Wed, 10 Dec 2025 16:41:53 -0500 Subject: [PATCH 01/10] fix(pkg-r): `querychat_app()` only closes connections if it creates the DB --- pkg-r/R/QueryChat.R | 13 +++++++++++-- pkg-r/R/utils-shiny.R | 3 +++ pkg-r/man/querychat-convenience.Rd | 2 +- pkg-r/man/querychat-package.Rd | 10 +++++++++- 4 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 pkg-r/R/utils-shiny.R diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 6ed12a8d..f79b2530 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -192,7 +192,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 <- in_shiny_session() } if (cleanup) { @@ -668,13 +668,22 @@ querychat_app <- function( categorical_threshold = 20, extra_instructions = NULL, prompt_template = NULL, - cleanup = TRUE, + cleanup = NA, bookmark_store = "url" ) { if (is_missing(table_name) && is.data.frame(data_source)) { table_name <- deparse1(substitute(data_source)) } + if (is.na(cleanup) && interactive() && !in_shiny_session()) { + if ( + is.data.frame(data_source) || + inherits(data_source, "DataFrameSource") + ) { + cleanup <- TRUE + } + } + qc <- QueryChat$new( data_source = data_source, table_name = table_name, diff --git a/pkg-r/R/utils-shiny.R b/pkg-r/R/utils-shiny.R new file mode 100644 index 00000000..893a57a1 --- /dev/null +++ b/pkg-r/R/utils-shiny.R @@ -0,0 +1,3 @@ +in_shiny_session <- function() { + !is.null(shiny::getDefaultReactiveDomain()) # nocov +} diff --git a/pkg-r/man/querychat-convenience.Rd b/pkg-r/man/querychat-convenience.Rd index f07c03e4..06bde2ad 100644 --- a/pkg-r/man/querychat-convenience.Rd +++ b/pkg-r/man/querychat-convenience.Rd @@ -30,7 +30,7 @@ querychat_app( categorical_threshold = 20, extra_instructions = NULL, prompt_template = NULL, - cleanup = TRUE, + cleanup = NA, bookmark_store = "url" ) } diff --git a/pkg-r/man/querychat-package.Rd b/pkg-r/man/querychat-package.Rd index 63598eda..7c10e48a 100644 --- a/pkg-r/man/querychat-package.Rd +++ b/pkg-r/man/querychat-package.Rd @@ -75,11 +75,19 @@ Useful links: \itemize{ \item \url{https://posit-dev.github.io/querychat/pkg-r} \item \url{https://posit-dev.github.io/querychat} + \item \url{https://github.com/posit-dev/querychat} + \item Report bugs at \url{https://github.com/posit-dev/querychat/issues} } } \author{ -\strong{Maintainer}: Joe Cheng \email{joe@posit.co} +\strong{Maintainer}: Garrick Aden-Buie \email{garrick@posit.co} (\href{https://orcid.org/0000-0002-7111-0077}{ORCID}) + +Authors: +\itemize{ + \item Joe Cheng \email{joe@posit.co} [conceptor] + \item Carson Sievert \email{carson@posit.co} (\href{https://orcid.org/0000-0002-4958-2844}{ORCID}) +} Other contributors: \itemize{ From ac01850d4b91266ddcceaf3dc0b3d265f37e6d77 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 12 Dec 2025 12:01:24 -0500 Subject: [PATCH 02/10] chore(pkg-r) Refactor for clarity --- pkg-r/R/QueryChat.R | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index f79b2530..d35786a6 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -675,13 +675,13 @@ querychat_app <- function( table_name <- deparse1(substitute(data_source)) } - if (is.na(cleanup) && interactive() && !in_shiny_session()) { - if ( + check_bool(cleanup, allow_na = TRUE) + if (is.na(cleanup)) { + is_df <- is.data.frame(data_source) || - inherits(data_source, "DataFrameSource") - ) { - cleanup <- TRUE - } + inherits(data_source, "DataFrameSource") + + cleanup <- !in_shiny_session() && is_df && interactive() } qc <- QueryChat$new( From b4e77e55d4203afe8e96141b982822063cc71390 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 12 Dec 2025 12:04:22 -0500 Subject: [PATCH 03/10] chore: Add NEWS --- pkg-r/NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index d157e533..c9b28036 100644 --- a/pkg-r/NEWS.md +++ b/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) From 246e409c402d0bb04b1874b2460b7edf13404826 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 12 Dec 2025 12:05:36 -0500 Subject: [PATCH 04/10] chore(pkg-r): Actually, only if a data frame --- pkg-r/R/QueryChat.R | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index d35786a6..bf4c84ef 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -677,11 +677,10 @@ querychat_app <- function( check_bool(cleanup, allow_na = TRUE) if (is.na(cleanup)) { - is_df <- - is.data.frame(data_source) || - inherits(data_source, "DataFrameSource") - - cleanup <- !in_shiny_session() && is_df && interactive() + cleanup <- + is.data.frame(data_source) && + !in_shiny_session() && + interactive() } qc <- QueryChat$new( From f100e7962955401e91ab7b8ae9772538e8ae3ff7 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 12 Dec 2025 12:19:40 -0500 Subject: [PATCH 05/10] tests(pkg-r): Test that cleanup is correctly set in querychat_app() --- pkg-r/R/QueryChat.R | 2 +- pkg-r/tests/testthat/test-QueryChat.R | 36 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index bf4c84ef..9f30bcf6 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -680,7 +680,7 @@ querychat_app <- function( cleanup <- is.data.frame(data_source) && !in_shiny_session() && - interactive() + is_interactive() } qc <- QueryChat$new( diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index 91eb3847..526a41b6 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -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) + }) +}) From 1cbae22c3b0dab24881d49982c236f7f112a4e40 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Tue, 16 Dec 2025 11:16:08 -0500 Subject: [PATCH 06/10] feat: automatic cleanup QC when created in running Shiny app --- pkg-r/R/QueryChat.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 9f30bcf6..02ceb8e3 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -192,7 +192,7 @@ QueryChat <- R6::R6Class( # By default, only close automatically if a Shiny session is active if (is.na(cleanup)) { - cleanup <- in_shiny_session() + cleanup <- shiny::isRunning() } if (cleanup) { From d4adb514e8d55e909acf476f69fdb3dacfe8a290 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Tue, 16 Dec 2025 11:17:19 -0500 Subject: [PATCH 07/10] feat(querychat_app): Throw if called inside a Shiny app --- pkg-r/R/QueryChat.R | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 02ceb8e3..9434a24c 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -671,16 +671,19 @@ querychat_app <- function( 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.na(cleanup)) { - cleanup <- - is.data.frame(data_source) && - !in_shiny_session() && - is_interactive() + cleanup <- is.data.frame(data_source) } qc <- QueryChat$new( From 23f4e9bd7074a7904576287fb4799fec2c37e048 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Tue, 16 Dec 2025 11:23:35 -0500 Subject: [PATCH 08/10] feat(querychat_app): Always clean up dataframe dbs --- pkg-r/R/QueryChat.R | 11 +++++++---- pkg-r/man/querychat-convenience.Rd | 7 +++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 9434a24c..435895c8 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -585,8 +585,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. #' @@ -682,8 +685,8 @@ querychat_app <- function( } check_bool(cleanup, allow_na = TRUE) - if (is.na(cleanup)) { - cleanup <- is.data.frame(data_source) + if (is.data.frame(data_source)) { + cleanup <- TRUE } qc <- QueryChat$new( diff --git a/pkg-r/man/querychat-convenience.Rd b/pkg-r/man/querychat-convenience.Rd index 06bde2ad..2e176f08 100644 --- a/pkg-r/man/querychat-convenience.Rd +++ b/pkg-r/man/querychat-convenience.Rd @@ -81,8 +81,11 @@ used. See the package prompts directory for the default template format.} \item{cleanup}{Whether or not to automatically run \verb{$cleanup()} when the Shiny session/app stops. By default, cleanup only occurs if \code{QueryChat} -gets created within a Shiny session. Set to \code{TRUE} to always clean up, -or \code{FALSE} to never clean up automatically.} +is created within a Shiny app. Set to \code{TRUE} to always clean up, or +\code{FALSE} to never clean up automatically. + +In \code{querychat_app()}, in-memory databases created for data frames are +always cleaned up.} \item{bookmark_store}{The bookmarking storage method. Passed to \code{\link[shiny:enableBookmarking]{shiny::enableBookmarking()}}. If \code{"url"} or \code{"server"}, the chat state From 5971ab43c1eba8b756fd5132f618b55d5c974e1e Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Tue, 16 Dec 2025 11:30:44 -0500 Subject: [PATCH 09/10] chore: move table validation into `normalize_data_source()` --- pkg-r/R/QueryChat.R | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 435895c8..601afbfc 100644 --- a/pkg-r/R/QueryChat.R +++ b/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)) { @@ -711,6 +708,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)) } From 8f92f3e2a93406dba03c6cdb6e52a2fbf8c760bc Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Tue, 16 Dec 2025 11:34:37 -0500 Subject: [PATCH 10/10] chore(querychat_app): Resolve `cleanup` early --- pkg-r/R/QueryChat.R | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 601afbfc..47e7338b 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -684,6 +684,8 @@ querychat_app <- function( check_bool(cleanup, allow_na = TRUE) if (is.data.frame(data_source)) { cleanup <- TRUE + } else if (is.na(cleanup)) { + cleanup <- FALSE } qc <- QueryChat$new(