From 32bf402771fbf26c251c0a8a969d718973ab9efd Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 11 Sep 2026 20:41:37 -0500 Subject: [PATCH 1/7] fix(r): make table_name optional for deferred QueryChat$new(NULL) QueryChat$new(NULL) (the deferred, per-session data source pattern) required table_name up front purely to compute a Shiny module $id -- but that's only needed for uniqueness across multiple instances in one app, not a real dependency of the deferred pattern itself. Python's QueryChat() already allows omitting it in this case, falling back to a generic module id. table_name is now optional when data_source is NULL: $id falls back to "querychat" if omitted (matching Python), and an explicit table_name = NULL is treated the same as omitting it. $server() gains a table_name parameter so a name can still be supplied per-session rather than only at $new() time, with a clear error (instead of a cryptic "subscript out of bounds") if data_source is given and no table name can be inferred at all -- from $server(), $new(), or an already-registered table. --- pkg-r/R/QueryChat.R | 54 ++++++++++++++++++++------- pkg-r/man/QueryChat.Rd | 17 +++++++-- pkg-r/tests/testthat/test-QueryChat.R | 50 ++++++++++++++++++++++--- 3 files changed, 99 insertions(+), 22 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index d3ea99d4..4e00291d 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -250,8 +250,11 @@ 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 @@ -383,14 +386,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 } - private$.deferred_table_name <- table_name - self$id <- id %||% sprintf("querychat_%s", table_name) + default_id <- if (table_name_given) { + sprintf("querychat_%s", table_name) + } else { + "querychat" + } + self$id <- id %||% default_id } # By default, only close automatically if a Shiny session is active @@ -1044,8 +1051,14 @@ 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 table_name Table name to register `data_source` under. Only + #' used when `data_source` is provided. #' @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 @@ -1065,6 +1078,7 @@ QueryChat <- R6::R6Class( #' or `NULL` before any query. server = function( data_source = NULL, + table_name = NULL, client = NULL, history = NULL, enable_bookmarking = NULL, @@ -1072,6 +1086,7 @@ QueryChat <- R6::R6Class( 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 +1097,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..d7a62194 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -176,8 +176,10 @@ 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 @@ -555,6 +557,7 @@ and \code{window_title} is omitted, it is also used as the document title.} \if{html}{\out{
}} \preformatted{QueryChat$server( data_source = NULL, + table_name = NULL, client = NULL, history = NULL, enable_bookmarking = NULL, @@ -567,8 +570,14 @@ 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{table_name}}{Table name to register \code{data_source} under. Only +used when \code{data_source} is provided.} \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 diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index bee69a07..05f14b95 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,42 @@ 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") + }) }) describe("QueryChat$add_tables()", { From d55185f3d1b36875338a1bdf6b007d2469706bdc Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 11 Sep 2026 20:51:11 -0500 Subject: [PATCH 2/7] fix(r): address review feedback on deferred table_name changes Two real bugs in the previous commit, both caught by review: - add_table()'s existing single-table auto-rename (self$id <- sprintf("querychat_%s", table_name) when is.null(self$id_override)) fired again when $server(data_source=) registered the deferred table, changing self$id after $ui()/$sidebar() had already rendered with the old one -- desyncing the Shiny module namespace between UI and server. Pin id_override once the deferred branch resolves self$id, so add_table() never renames it afterward. - $server()'s new table_name parameter was inserted right after data_source, shifting the meaning of existing positional calls like $server(data_source, client). Moved it after `...`/session so it's named-only, like id already is. --- pkg-r/R/QueryChat.R | 14 +++++-- pkg-r/man/QueryChat.Rd | 9 +++-- pkg-r/tests/testthat/test-QueryChat.R | 54 +++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 8 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 4e00291d..dfb69720 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -398,6 +398,11 @@ QueryChat <- R6::R6Class( "querychat" } self$id <- id %||% default_id + # $ui()/$sidebar() may render with this id before any session's + # $server(data_source = ) call registers a table -- pin id_override + # so add_table()'s single-table auto-rename doesn't fire later and + # desync the module namespace from what's already been rendered. + self$id_override <- self$id } # By default, only close automatically if a Shiny session is active @@ -1057,8 +1062,6 @@ QueryChat <- R6::R6Class( #' per-user OAuth credentials). Registered under `table_name` if given, #' otherwise the `table_name` passed to `$new()`, or the first #' already-registered table. - #' @param table_name Table name to register `data_source` under. Only - #' used when `data_source` is provided. #' @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 @@ -1069,6 +1072,9 @@ QueryChat <- R6::R6Class( #' @param ... Ignored. #' @param id Optional module ID override. #' @param session The Shiny session object. + #' @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. #' #' @return A list containing session-specific reactive values and the chat #' client. For single-table usage, includes `df`, `sql`, `title` directly. @@ -1078,13 +1084,13 @@ QueryChat <- R6::R6Class( #' or `NULL` before any query. server = function( data_source = NULL, - table_name = NULL, client = NULL, history = NULL, enable_bookmarking = NULL, ..., id = NULL, - session = shiny::getDefaultReactiveDomain() + session = shiny::getDefaultReactiveDomain(), + table_name = NULL ) { check_string(table_name, allow_null = TRUE, allow_empty = FALSE) check_string(id, allow_null = TRUE, allow_empty = FALSE) diff --git a/pkg-r/man/QueryChat.Rd b/pkg-r/man/QueryChat.Rd index d7a62194..b4147361 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -557,13 +557,13 @@ and \code{window_title} is omitted, it is also used as the document title.} \if{html}{\out{
}} \preformatted{QueryChat$server( data_source = NULL, - table_name = NULL, client = NULL, history = NULL, enable_bookmarking = NULL, ..., id = NULL, - session = shiny::getDefaultReactiveDomain() + session = shiny::getDefaultReactiveDomain(), + table_name = NULL )} \if{html}{\out{
}} } @@ -576,8 +576,6 @@ 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{table_name}}{Table name to register \code{data_source} under. Only -used when \code{data_source} is provided.} \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 @@ -587,6 +585,9 @@ constructor's \code{history} was set.} \item{\code{...}}{Ignored.} \item{\code{id}}{Optional module ID override.} \item{\code{session}}{The Shiny session object.} + \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.} } \if{html}{\out{
}} } diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index 05f14b95..635aa120 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -1158,6 +1158,60 @@ describe("QueryChat deferred client with $server()", { ) 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 + ) + }) }) describe("QueryChat$add_tables()", { From eb7355f48eafb941cbf4c90224acf7928e5817b5 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 11 Sep 2026 21:02:08 -0500 Subject: [PATCH 3/7] fix(r): address second round of review feedback - $server(data_source=, table_name=): reject a mismatch between an already-built DataSource's own table_name and the resolved table_name, instead of silently registering it under the wrong key (normalize_data_source() returns DataSource objects unchanged, so a mismatch would otherwise desync the registered name from the name the source actually queries/describes). - Stop reusing the public id_override field (documented as "the user explicitly set id") to pin the deferred-fallback id -- that field's nullness is also add_table()'s auto-rename guard, so writing to it for an auto-generated id blurred its contract. Added a private .id_pinned flag instead; add_table()'s guard now checks both. - Updated the @param id roxygen text, which still described the old always-inferred-from-table_name behavior. --- pkg-r/R/QueryChat.R | 33 +++++++++++++++++++++------ pkg-r/man/QueryChat.Rd | 5 ++-- pkg-r/tests/testthat/test-QueryChat.R | 26 +++++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index dfb69720..e25c423e 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -91,6 +91,7 @@ QueryChat <- R6::R6Class( private = list( .data_sources = list(), .deferred_table_name = NULL, + .id_pinned = FALSE, .query_executor = NULL, .server_initialized = FALSE, .client_spec = NULL, @@ -257,8 +258,9 @@ QueryChat <- R6::R6Class( #' 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 @@ -399,10 +401,12 @@ QueryChat <- R6::R6Class( } self$id <- id %||% default_id # $ui()/$sidebar() may render with this id before any session's - # $server(data_source = ) call registers a table -- pin id_override - # so add_table()'s single-table auto-rename doesn't fire later and - # desync the module namespace from what's already been rendered. - self$id_override <- self$id + # $server(data_source = ) call registers a table -- pin it (via a + # private flag, not id_override, which specifically means "the user + # passed id =") so add_table()'s single-table auto-rename doesn't + # fire later and desync the module namespace from what's already + # been rendered. + private$.id_pinned <- TRUE } # By default, only close automatically if a Shiny session is active @@ -479,7 +483,11 @@ QueryChat <- R6::R6Class( private$.query_executor <- NULL } - if (length(private$.data_sources) == 1 && is.null(self$id_override)) { + if ( + length(private$.data_sources) == 1 && + is.null(self$id_override) && + !private$.id_pinned + ) { self$id <- sprintf("querychat_%s", table_name) } @@ -1118,6 +1126,17 @@ QueryChat <- R6::R6Class( ) ) } + if ( + is_data_source(data_source) && + !identical(data_source$table_name, tbl_name) + ) { + cli::cli_abort( + c( + "{.arg data_source}'s own table name ({.val {data_source$table_name}}) does not match the resolved {.arg table_name} ({.val {tbl_name}}).", + "i" = "Pass a matching {.arg table_name}, or omit it to use {.val {data_source$table_name}}." + ) + ) + } self$add_table( data_source, tbl_name, diff --git a/pkg-r/man/QueryChat.Rd b/pkg-r/man/QueryChat.Rd index b4147361..5cf33b66 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -182,8 +182,9 @@ 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 diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index 635aa120..fcf74cec 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -1212,6 +1212,32 @@ describe("QueryChat deferred client with $server()", { 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 (only $id is pinned)", { + qc <- QueryChat$new(NULL, greeting = "Test") + expect_null(qc$id_override) + expect_equal(qc$id, "querychat") + }) }) describe("QueryChat$add_tables()", { From 2542f0cf4e3ecba240a194995502aa5858b57a8c Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 11 Sep 2026 21:14:20 -0500 Subject: [PATCH 4/7] fix(r): move DataSource table_name mismatch check into add_table() The check added in 88293861 only guarded $server(data_source=, table_name=), but the identical mismatch (a DataSource's own table_name differing from the dict key it's registered under) was still reachable unguarded through $add_table() called directly. Move the check into add_table() itself so all callers (including $server(), which delegates to it) get the same protection. --- pkg-r/R/QueryChat.R | 22 +++++++++++----------- pkg-r/tests/testthat/test-QueryChat.R | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index e25c423e..48fc0bf8 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -449,6 +449,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[ @@ -1126,17 +1137,6 @@ QueryChat <- R6::R6Class( ) ) } - if ( - is_data_source(data_source) && - !identical(data_source$table_name, tbl_name) - ) { - cli::cli_abort( - c( - "{.arg data_source}'s own table name ({.val {data_source$table_name}}) does not match the resolved {.arg table_name} ({.val {tbl_name}}).", - "i" = "Pass a matching {.arg table_name}, or omit it to use {.val {data_source$table_name}}." - ) - ) - } self$add_table( data_source, tbl_name, diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index fcf74cec..ee73c997 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -1240,6 +1240,20 @@ describe("QueryChat deferred client with $server()", { }) }) +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()", { local_multi_table_conn <- function(env = parent.frame()) { skip_if_not_installed("RSQLite") From f0cee460e4c65428e53f7c8e7f53f7863b53ea8f Mon Sep 17 00:00:00 2001 From: Carson Date: Sat, 12 Sep 2026 10:14:40 -0500 Subject: [PATCH 5/7] docs(r): NEWS entry for deferred-construction ergonomics Also drop the now-stale breaking-change note claiming $server()'s data_source parameter was removed in #195 -- it is re-blessed here for the deferred per-session pattern, and will never have been removed in a released version. --- pkg-r/NEWS.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 09213f3c..75b178fc 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()`. From 38009e29d9cc9371ed52976fa4166a404c7288a2 Mon Sep 17 00:00:00 2001 From: Carson Date: Sat, 12 Sep 2026 10:29:28 -0500 Subject: [PATCH 6/7] fix(r): remove single-table id auto-rename instead of pinning Rather than pinning $id in the deferred case (.id_pinned), drop the rename altogether: $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's set-once semantics. The rename was nearly unreachable (remove_table() refuses to remove the last table, and the constructor registers inline), and in its one live path -- registering a table between $ui() and $server() -- it could itself cause the namespace desync the pin was guarding against. --- pkg-r/NEWS.md | 2 ++ pkg-r/R/QueryChat.R | 16 ---------------- pkg-r/tests/testthat/test-QueryChat.R | 12 +++++++++++- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 75b178fc..8f7f9f83 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -70,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 48fc0bf8..4a3f89cd 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -91,7 +91,6 @@ QueryChat <- R6::R6Class( private = list( .data_sources = list(), .deferred_table_name = NULL, - .id_pinned = FALSE, .query_executor = NULL, .server_initialized = FALSE, .client_spec = NULL, @@ -400,13 +399,6 @@ QueryChat <- R6::R6Class( "querychat" } self$id <- id %||% default_id - # $ui()/$sidebar() may render with this id before any session's - # $server(data_source = ) call registers a table -- pin it (via a - # private flag, not id_override, which specifically means "the user - # passed id =") so add_table()'s single-table auto-rename doesn't - # fire later and desync the module namespace from what's already - # been rendered. - private$.id_pinned <- TRUE } # By default, only close automatically if a Shiny session is active @@ -494,14 +486,6 @@ QueryChat <- R6::R6Class( private$.query_executor <- NULL } - if ( - length(private$.data_sources) == 1 && - is.null(self$id_override) && - !private$.id_pinned - ) { - self$id <- sprintf("querychat_%s", table_name) - } - if (isTRUE(include_in_greeting)) { self$greeter$tables <- c(self$greeter$tables, table_name) } diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index ee73c997..0b810131 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -1233,11 +1233,21 @@ describe("QueryChat deferred client with $server()", { ) }) - it("id_override stays NULL for the generic deferred fallback (only $id is pinned)", { + 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()", { From 866311f9a8d51d9afefa5d05424b4806d3fa714b Mon Sep 17 00:00:00 2001 From: Carson Date: Sat, 12 Sep 2026 10:56:14 -0500 Subject: [PATCH 7/7] refactor(r): move $server(table_name=) just before id= Still named-only (after ...), so existing positional calls are unaffected; this just groups it with the other registration-related args. --- pkg-r/R/QueryChat.R | 8 ++++---- pkg-r/man/QueryChat.Rd | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 4a3f89cd..8b3b560a 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -1073,11 +1073,11 @@ QueryChat <- R6::R6Class( #' shinychat::history_options(restore_mode = "bookmark")` instead (set on #' `$new()`, or passed here). #' @param ... Ignored. - #' @param id Optional module ID override. - #' @param session The Shiny session object. #' @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. #' #' @return A list containing session-specific reactive values and the chat #' client. For single-table usage, includes `df`, `sql`, `title` directly. @@ -1091,9 +1091,9 @@ QueryChat <- R6::R6Class( history = NULL, enable_bookmarking = NULL, ..., + table_name = NULL, id = NULL, - session = shiny::getDefaultReactiveDomain(), - table_name = NULL + session = shiny::getDefaultReactiveDomain() ) { check_string(table_name, allow_null = TRUE, allow_empty = FALSE) check_string(id, allow_null = TRUE, allow_empty = FALSE) diff --git a/pkg-r/man/QueryChat.Rd b/pkg-r/man/QueryChat.Rd index 5cf33b66..f2b2837a 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -562,9 +562,9 @@ 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(), - table_name = NULL + session = shiny::getDefaultReactiveDomain() )} \if{html}{\out{
}} } @@ -584,11 +584,11 @@ 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{id}}{Optional module ID override.} - \item{\code{session}}{The Shiny session object.} \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.} } \if{html}{\out{}} }