fix(r): make table_name optional for deferred QueryChat$new(NULL)

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

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,

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()", {