diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 09213f3c..8f7f9f83 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -42,9 +42,11 @@ * 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`. +* Deferred construction is more flexible: `table_name` is now optional in `QueryChat$new(NULL)` (if omitted, `$id` falls back to a generic `"querychat"` default), and `$server()` gains a `table_name` parameter so the table can be named per session when registering a data source via `$server(data_source = )`. (#305) + ## 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) +* 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. (#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()`. @@ -68,6 +70,8 @@ * Query results were being shown expanded, and often repeated in the LLM's response, far more often than intended. The LLM is now guided to expand a result only when the user explicitly asks to see the raw table. (#295) +* `$add_table()` no longer rewrites the Shiny module `$id` when the registered table is the only one; the id is now fixed at construction time, matching Python. The rewrite could desync the module namespace from an already-rendered UI when a table was registered between `$ui()` and `$server()` (e.g. via `$server(data_source = )`). (#305) + # querychat 0.3.0 ## New features diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index d3ea99d4..8b3b560a 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -250,12 +250,16 @@ QueryChat <- R6::R6Class( #' @param table_name A string specifying the table name to use in SQL #' queries. If `data_source` is a data.frame, this is the name to refer to #' it by in queries (typically the variable name). If not provided, will - #' be inferred from the variable name for data.frame inputs. For database - #' connections or `NULL` data sources, this parameter is required. + #' be inferred from the variable name for data.frame inputs. Required for + #' database connections. Optional when `data_source` is `NULL`: if + #' omitted, `$id` falls back to a generic default, and a table name must + #' be supplied later via `$add_table()` or `$server(data_source =, + #' table_name = )`. #' @param ... Additional arguments (currently unused). #' @param id Optional module ID for the QueryChat instance. If not provided, - #' will be auto-generated from `table_name`. The ID is used to namespace - #' the Shiny module. + #' will be auto-generated from `table_name` (or a generic default when + #' `data_source` is `NULL` and `table_name` is also omitted). The ID is + #' used to namespace the Shiny module. #' @param greeting Optional initial message to display to users. Can be a #' character string (in Markdown format) or a file path. If not provided, #' a greeting will be generated at the start of each conversation using @@ -383,14 +387,18 @@ QueryChat <- R6::R6Class( self$greeter$tables <- c(self$greeter$tables, normalized$table_name) self$id <- id %||% sprintf("querychat_%s", normalized$table_name) } else { - # Deferred pattern: data_source is NULL - if (is_missing(table_name)) { - cli::cli_abort( - "{.arg table_name} is required when {.arg data_source} is {.val NULL}." - ) + # Deferred pattern: data_source is NULL. table_name is optional here; + # explicit NULL is treated the same as omitting it. + table_name_given <- !is_missing(table_name) && !is.null(table_name) + if (table_name_given) { + private$.deferred_table_name <- table_name + } + default_id <- if (table_name_given) { + sprintf("querychat_%s", table_name) + } else { + "querychat" } - private$.deferred_table_name <- table_name - self$id <- id %||% sprintf("querychat_%s", table_name) + self$id <- id %||% default_id } # By default, only close automatically if a Shiny session is active @@ -433,6 +441,17 @@ QueryChat <- R6::R6Class( "Table {.val {table_name}} already exists. Use {.code replace = TRUE} to replace." ) } + if ( + is_data_source(data_source) && + !identical(data_source$table_name, table_name) + ) { + cli::cli_abort( + c( + "{.arg data_source}'s own table name ({.val {data_source$table_name}}) does not match the given {.arg table_name} ({.val {table_name}}).", + "i" = "Pass a matching {.arg table_name}, or omit it to use {.val {data_source$table_name}}." + ) + ) + } normalized <- normalize_data_source(data_source, table_name) other_sources <- private$.data_sources[ @@ -467,10 +486,6 @@ QueryChat <- R6::R6Class( private$.query_executor <- NULL } - if (length(private$.data_sources) == 1 && is.null(self$id_override)) { - self$id <- sprintf("querychat_%s", table_name) - } - if (isTRUE(include_in_greeting)) { self$greeter$tables <- c(self$greeter$tables, table_name) } @@ -1044,8 +1059,12 @@ QueryChat <- R6::R6Class( #' @description #' Initialize the querychat server logic. #' - #' @param data_source Optional data source for backward compatibility. - #' If provided, calls `$add_table()` before initializing server logic. + #' @param data_source Optional data source to register for this session, + #' for the deferred pattern where the data source can't be created + #' until the server function runs (e.g. a connection scoped to + #' per-user OAuth credentials). Registered under `table_name` if given, + #' otherwise the `table_name` passed to `$new()`, or the first + #' already-registered table. #' @param client Optional chat client override for this session. #' @param history Conversation history configuration for this call. Overrides #' the value set on `$new()`. Resolves to `TRUE` when neither this nor the @@ -1054,6 +1073,9 @@ QueryChat <- R6::R6Class( #' shinychat::history_options(restore_mode = "bookmark")` instead (set on #' `$new()`, or passed here). #' @param ... Ignored. + #' @param table_name Table name to register `data_source` under. Only + #' used when `data_source` is provided. Named-only (placed after `...`) + #' so it can't shift the meaning of existing positional calls. #' @param id Optional module ID override. #' @param session The Shiny session object. #' @@ -1069,9 +1091,11 @@ QueryChat <- R6::R6Class( history = NULL, enable_bookmarking = NULL, ..., + table_name = NULL, id = NULL, session = shiny::getDefaultReactiveDomain() ) { + check_string(table_name, allow_null = TRUE, allow_empty = FALSE) check_string(id, allow_null = TRUE, allow_empty = FALSE) check_dots_empty() @@ -1082,8 +1106,21 @@ QueryChat <- R6::R6Class( } if (!is.null(data_source)) { - tbl_name <- private$.deferred_table_name %||% - names(private$.data_sources)[[1]] + tbl_name <- table_name %||% private$.deferred_table_name + if (is.null(tbl_name)) { + existing_tables <- names(private$.data_sources) + if (length(existing_tables) > 0) { + tbl_name <- existing_tables[[1]] + } + } + if (is.null(tbl_name)) { + cli::cli_abort( + c( + "{.arg table_name} is required when {.arg data_source} is provided and no table name can be inferred.", + "i" = "Pass {.arg table_name} to {.fn $server}, or {.arg table_name} to {.fn QueryChat$new}, or register a table first with {.fn $add_table}." + ) + ) + } self$add_table( data_source, tbl_name, diff --git a/pkg-r/man/QueryChat.Rd b/pkg-r/man/QueryChat.Rd index e0ffa978..f2b2837a 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -176,12 +176,15 @@ to \verb{$server()} before calling methods that require data access.} \item{\code{table_name}}{A string specifying the table name to use in SQL queries. If \code{data_source} is a data.frame, this is the name to refer to it by in queries (typically the variable name). If not provided, will -be inferred from the variable name for data.frame inputs. For database -connections or \code{NULL} data sources, this parameter is required.} +be inferred from the variable name for data.frame inputs. Required for +database connections. Optional when \code{data_source} is \code{NULL}: if +omitted, \verb{$id} falls back to a generic default, and a table name must +be supplied later via \verb{$add_table()} or \verb{$server(data_source =, table_name = )}.} \item{\code{...}}{Additional arguments (currently unused).} \item{\code{id}}{Optional module ID for the QueryChat instance. If not provided, -will be auto-generated from \code{table_name}. The ID is used to namespace -the Shiny module.} +will be auto-generated from \code{table_name} (or a generic default when +\code{data_source} is \code{NULL} and \code{table_name} is also omitted). The ID is +used to namespace the Shiny module.} \item{\code{greeting}}{Optional initial message to display to users. Can be a character string (in Markdown format) or a file path. If not provided, a greeting will be generated at the start of each conversation using @@ -559,6 +562,7 @@ and \code{window_title} is omitted, it is also used as the document title.} history = NULL, enable_bookmarking = NULL, ..., + table_name = NULL, id = NULL, session = shiny::getDefaultReactiveDomain() )} @@ -567,8 +571,12 @@ and \code{window_title} is omitted, it is also used as the document title.} \subsection{Arguments}{ \if{html}{\out{
}} \describe{ - \item{\code{data_source}}{Optional data source for backward compatibility. -If provided, calls \verb{$add_table()} before initializing server logic.} + \item{\code{data_source}}{Optional data source to register for this session, +for the deferred pattern where the data source can't be created +until the server function runs (e.g. a connection scoped to +per-user OAuth credentials). Registered under \code{table_name} if given, +otherwise the \code{table_name} passed to \verb{$new()}, or the first +already-registered table.} \item{\code{client}}{Optional chat client override for this session.} \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 @@ -576,6 +584,9 @@ 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{table_name}}{Table name to register \code{data_source} under. Only +used when \code{data_source} is provided. Named-only (placed after \code{...}) +so it can't shift the meaning of existing positional calls.} \item{\code{id}}{Optional module ID override.} \item{\code{session}}{The Shiny session object.} } diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index bee69a07..0b810131 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -108,11 +108,15 @@ describe("QueryChat deferred client", { expect_equal(qc$id, "querychat_users") }) - it("requires table_name when data_source is NULL", { - expect_error( - QueryChat$new(NULL), - "table_name.*required" - ) + it("does not require table_name when data_source is NULL", { + qc <- QueryChat$new(NULL, greeting = "Test") + expect_equal(qc$id, "querychat") + expect_equal(length(qc$table_names()), 0L) + }) + + it("explicit table_name = NULL is treated the same as omitting it", { + qc <- QueryChat$new(NULL, table_name = NULL, greeting = "Test") + expect_equal(qc$id, "querychat") }) it("stores client spec without resolving it", { @@ -1118,6 +1122,146 @@ describe("QueryChat deferred client with $server()", { "must be called within a Shiny server function" ) }) + + it("$server(data_source=...) gives a clear error when no table name can be inferred", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new( + NULL, + greeting = "Test", + client = mock_ellmer_chat_client() + ) + + expect_error( + shiny::testServer( + function(input, output, session) { + qc$server(data_source = new_users_df()) + }, + {} + ), + "table_name.*required" + ) + }) + + it("$server(data_source=, table_name=) registers under the given name", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new( + NULL, + greeting = "Test", + client = mock_ellmer_chat_client() + ) + + shiny::testServer( + function(input, output, session) { + qc$server(data_source = new_users_df(), table_name = "users") + }, + {} + ) + expect_equal(qc$table_names(), "users") + }) + + it("id stays fixed across deferred registration (no desync from an already-rendered UI)", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new( + NULL, + greeting = "Test", + client = mock_ellmer_chat_client() + ) + id_before_server <- qc$id # simulates $ui()/$sidebar() having already rendered + + shiny::testServer( + function(input, output, session) { + qc$server(data_source = new_users_df(), table_name = "users") + }, + {} + ) + + expect_equal(qc$id, id_before_server) + }) + + it("id stays fixed when a table_name was given at $new() too", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new( + NULL, + "orders", + greeting = "Test", + client = mock_ellmer_chat_client() + ) + id_before_server <- qc$id + + shiny::testServer( + function(input, output, session) { + qc$server(data_source = new_users_df(), table_name = "different_name") + }, + {} + ) + + expect_equal(qc$id, id_before_server) + }) + + it("$server() preserves positional data_source/client call compatibility", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new(NULL, "users", greeting = "Test") + + expect_error( + shiny::testServer( + function(input, output, session) { + qc$server(new_users_df(), mock_ellmer_chat_client()) + }, + {} + ), + NA + ) + }) + + it("$server(data_source=, table_name=) errors when a DataSource's own name conflicts", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new( + NULL, + greeting = "Test", + client = mock_ellmer_chat_client() + ) + mismatched_source <- local_data_frame_source(new_users_df(), "orders") + + expect_error( + shiny::testServer( + function(input, output, session) { + qc$server(data_source = mismatched_source, table_name = "users") + }, + {} + ), + "table name" + ) + }) + + it("id_override stays NULL for the generic deferred fallback", { + qc <- QueryChat$new(NULL, greeting = "Test") + expect_null(qc$id_override) + expect_equal(qc$id, "querychat") + }) + + it("$add_table() never rewrites $id (it is fixed at construction time)", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new(NULL, "placeholder", greeting = "Test") + id_before <- qc$id + + qc$add_table(new_users_df(), "users") + + expect_equal(qc$id, id_before) + }) +}) + +describe("QueryChat$add_table()", { + it("errors when a DataSource's own table_name conflicts with the registration name", { + skip_if_no_dataframe_engine() + qc <- QueryChat$new(NULL, greeting = "Test") + mismatched_source <- local_data_frame_source(new_users_df(), "orders") + + expect_error( + qc$add_table(mismatched_source, "users"), + "table name" + ) + expect_equal(length(qc$table_names()), 0L) + }) }) describe("QueryChat$add_tables()", {