diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index 03fdbc7c6..be52927c7 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -37,10 +37,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * File attachments are now enabled by default in the Shiny chat UI. Users can attach images, PDFs, and text files to their messages and the LLM will receive them. Disable with `allow_attachments=False` in `mod_ui()` or `QueryChat.ui()`. (#253) +* Conversation history is now persisted by default. `QueryChat`/`QueryChatExpress` keep 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.types.HistoryOptions(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`. + ### 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()` (Shiny) has also been removed; call `add_table()` before `server()` instead. (#195) +* `.app()`'s `bookmark_store` parameter has been removed. Pass `history=shinychat.types.HistoryOptions(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 (`bookmark_store="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 `HistoryOptions()`. + +### Deprecated + +* `.server()`'s and `QueryChatExpress`'s `enable_bookmarking` parameter is deprecated in favor of `history`. Pass `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` instead of `enable_bookmarking=True` for the equivalent behavior. + ### Improvements * Chat greetings now use shinychat's greeting API (requires shinychat >= 0.4.0). A provided `greeting` renders instantly when the app loads, and when no `greeting` is given one is generated on demand — now **schema-aware**, so it can describe the data it's about to help you explore — without being added to the conversation history. Generated greetings are preserved across bookmark/restore. Tables passed to `QueryChat()` are described in the greeting automatically; opt additional tables in with `include_in_greeting=True` on `add_table()`/`add_tables()`, or fine-tune which tables and which template the greeting uses via `qc.greeter`. (#249, #261) diff --git a/pkg-py/examples/10-viz-app.py b/pkg-py/examples/10-viz-app.py index 0702d3c87..857e8421d 100644 --- a/pkg-py/examples/10-viz-app.py +++ b/pkg-py/examples/10-viz-app.py @@ -1,9 +1,8 @@ from pathlib import Path -from querychat.express import QueryChat from querychat.data import titanic - -from shiny.express import ui, app_opts +from querychat.express import QueryChat +from shiny.express import ui greeting = Path(__file__).parent / "greeting-viz.md" @@ -18,5 +17,3 @@ qc.ui() ui.page_opts(fillable=True, title="QueryChat Visualization Demo") - -app_opts(bookmark_store="url") diff --git a/pkg-py/src/querychat/_querychat_base.py b/pkg-py/src/querychat/_querychat_base.py index d0fd3d4aa..d98ad94fd 100644 --- a/pkg-py/src/querychat/_querychat_base.py +++ b/pkg-py/src/querychat/_querychat_base.py @@ -57,6 +57,7 @@ from ibis.backends.sql import SQLBackend from narwhals.stable.v1.typing import IntoFrame from pins.boards import BaseBoard + from shinychat.types import HistoryOptions from ._data_dict import DataDict from ._viz_tools import VisualizeData @@ -91,6 +92,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ): self._data_dicts: list[DataDict] = _normalize_data_dicts(data_dict) @@ -103,6 +105,7 @@ def __init__( self.tools = normalize_tools(tools, default=DEFAULT_TOOLS) self.greeting = greeting.read_text() if isinstance(greeting, Path) else greeting + self.history = history # Store init parameters for deferred system prompt building self._prompt_template = prompt_template diff --git a/pkg-py/src/querychat/_querychat_greeter.py b/pkg-py/src/querychat/_querychat_greeter.py index 19fc3e75e..ef95551ef 100644 --- a/pkg-py/src/querychat/_querychat_greeter.py +++ b/pkg-py/src/querychat/_querychat_greeter.py @@ -12,8 +12,6 @@ import chatlas - from shiny import reactive - class QueryChatGreeter: """Controls greeting generation for a QueryChat instance. Access via ``qc.greeter``.""" @@ -66,19 +64,7 @@ def generate( """Generate a greeting using the greeting system prompt.""" return str(self.build_client(base).chat(GREETING_PROMPT, echo=echo)) - async def generate_stream( - self, - *, - chat_ui, - current_greeting: reactive.Value[str | None], - base: chatlas.Chat | None = None, - ) -> None: - """Stream a greeting into the Shiny chat UI and capture the result.""" + async def generate_async(self, *, base: chatlas.Chat | None = None): + """Stream a greeting response from the greeting client.""" client = self.build_client(base) - stream = await client.stream_async(GREETING_PROMPT, echo="none") - from ._utils_shinychat import chat_greeting_persistent - - await chat_ui.set_greeting(chat_greeting_persistent(stream)) # pyright: ignore[reportArgumentType] - last_turn = client.get_last_turn(role="assistant") - if last_turn is not None: - current_greeting.set(last_turn.text) + return await client.stream_async(GREETING_PROMPT, echo="none") diff --git a/pkg-py/src/querychat/_shiny.py b/pkg-py/src/querychat/_shiny.py index 7fae22059..ba1d69eb4 100644 --- a/pkg-py/src/querychat/_shiny.py +++ b/pkg-py/src/querychat/_shiny.py @@ -1,10 +1,12 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any, ClassVar, Literal, Optional, overload +import warnings +from typing import TYPE_CHECKING, Any, Literal, Optional, overload from narwhals.stable.v1.typing import IntoDataFrameT, IntoFrameT, IntoLazyFrameT from shiny.express._stub_session import ExpressStubSession from shiny.session import get_current_session +from shinychat.types import HistoryOptions from shiny import App, Inputs, Outputs, Session, reactive, render, req, ui @@ -159,6 +161,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -176,6 +179,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -193,6 +197,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -210,6 +215,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... @overload @@ -227,6 +233,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ) -> None: ... def __init__( @@ -243,6 +250,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, ): super().__init__( data_source, @@ -255,11 +263,14 @@ def __init__( categorical_threshold=categorical_threshold, extra_instructions=extra_instructions, prompt_template=prompt_template, + history=history, ) self.id = id or (f"querychat_{table_name}" if table_name else "querychat") def app( - self, *, bookmark_store: Literal["url", "server", "disable"] = "url" + self, + *, + history: Optional[bool | HistoryOptions] = None, ) -> App: """ Quickly chat with a dataset. @@ -269,11 +280,16 @@ def app( Parameters ---------- - bookmark_store - The bookmarking store to use for the Shiny app. Options are: - - `"url"`: Store bookmarks in the URL (default). - - `"server"`: Store bookmarks on the server. - - `"disable"`: Disable bookmarking. + history + Conversation history configuration, passed through to + `shinychat.Chat(history=)`. Defaults to + `shinychat.types.HistoryOptions(restore_mode="bookmark")` when neither + this nor the constructor's `history` was set, since `.app()`'s whole + purpose is a single, shareable demo (chat + SQL editor + table state + tied together). When the resolved value has `restore_mode="bookmark"`, + the generated app automatically enables Shiny's own server-side + bookmarking (`bookmark_store="server"`) -- shinychat's `history` + mechanism only wires the chat side, not the app-level Shiny setting. Returns ------- @@ -282,7 +298,19 @@ def app( """ self._require_initialized("app") - enable_bookmarking = bookmark_store != "disable" + resolved_history: bool | HistoryOptions = ( + history + if history is not None + else ( + self.history + if self.history is not None + else HistoryOptions(restore_mode="bookmark") + ) + ) + enable_bookmarking = ( + isinstance(resolved_history, HistoryOptions) + and resolved_history.restore_mode == "bookmark" + ) first_table_name = next(iter(self._data_sources)) def app_ui(request): @@ -330,7 +358,7 @@ def app_server(input: Inputs, output: Outputs, session: Session): executor=self._require_query_executor("server"), greeting=self.greeting, client=self._create_session_client, - enable_bookmarking=enable_bookmarking, + history=resolved_history, tools=self.tools, greeter=self.greeter, greeting_base=None, @@ -401,7 +429,11 @@ def _(): query if query and query.strip() != default_query else None ) - return App(app_ui, app_server, bookmark_store=bookmark_store) + return App( + app_ui, + app_server, + bookmark_store="server" if enable_bookmarking else "disable", + ) def sidebar( self, @@ -462,18 +494,14 @@ def ui(self, *, id: Optional[str] = None, **kwargs): A UI component. """ - return mod_ui( - id or self.id, - preload_viz=has_viz_tool(self.tools), - greeting=self.greeting, - **kwargs, - ) + return mod_ui(id or self.id, preload_viz=has_viz_tool(self.tools), **kwargs) def server( self, *, client: str | chatlas.Chat | MISSING_TYPE = MISSING, - enable_bookmarking: bool = False, + history: Optional[bool | HistoryOptions] = None, + enable_bookmarking: bool | None = None, id: Optional[str] = None, ) -> ServerValues[IntoFrameT]: """ @@ -492,52 +520,20 @@ def server( for the deferred pattern where the client cannot be created at initialization time (e.g., when using Posit Connect managed OAuth credentials that require session access). + history + Conversation history configuration, passed through to + `shinychat.Chat(history=)`. Overrides the value set on the `QueryChat` + constructor for this call only. Defaults to `True` when neither this + nor the constructor's `history` was set. enable_bookmarking - Whether to enable bookmarking for the querychat module. + Deprecated. `True` resolves to `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` + when neither this call's `history` nor the constructor's `history` was set. Use `history=` + directly instead (set on the `QueryChat` constructor, or passed here). id Optional module ID for the QueryChat instance. If not provided, will use the ID provided at initialization. This must match the ID used in the `.ui()` or `.sidebar()` methods. - Examples - -------- - ```python - from shiny import App, render, ui - from seaborn import load_dataset - from querychat import QueryChat - - titanic = load_dataset("titanic") - - qc = QueryChat(titanic, "titanic") - - - def app_ui(request): - return ui.page_sidebar( - qc.sidebar(), - ui.card( - ui.card_header(ui.output_text("title")), - ui.output_data_frame("data_table"), - ), - title="Titanic QueryChat App", - fillable=True, - ) - - - def server(input, output, session): - qc_vals = qc.server(enable_bookmarking=True) - - @render.data_frame - def data_table(): - return qc_vals.df() - - @render.text - def title(): - return qc_vals.title() or "My Data" - - - app = App(app_ui, server, bookmark_store="url") - ``` - Returns ------- : @@ -560,6 +556,30 @@ def title(): def create_session_client(**kwargs) -> chatlas.Chat: return self._create_session_client(base=resolved_client, **kwargs) + if enable_bookmarking is not None: + warnings.warn( + "`enable_bookmarking` is deprecated. " + 'Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` ' + "instead (on the QueryChat constructor or passed to .server()) " + "for the equivalent behavior.", + FutureWarning, + stacklevel=2, + ) + + resolved_history: bool | HistoryOptions = ( + history + if history is not None + else ( + self.history + if self.history is not None + else ( + HistoryOptions(restore_mode="bookmark") + if enable_bookmarking + else True + ) + ) + ) + self._mark_server_initialized() return mod_server( id or self.id, @@ -567,7 +587,7 @@ def create_session_client(**kwargs) -> chatlas.Chat: executor=self._require_query_executor("server"), greeting=self.greeting, client=create_session_client, - enable_bookmarking=enable_bookmarking, + history=resolved_history, tools=self.tools, greeter=self.greeter, greeting_base=resolved_client, @@ -587,11 +607,12 @@ class QueryChatExpress(QueryChatBase[IntoFrameT]): ```python from querychat.express import QueryChat from seaborn import load_dataset - from shiny.express import app_opts, render, ui + from shiny.express import render, ui + from shinychat.types import HistoryOptions titanic = load_dataset("titanic") - qc = QueryChat(titanic, "titanic") + qc = QueryChat(titanic, "titanic", history=HistoryOptions(restore_mode="bookmark")) qc.sidebar() with ui.card(fill=True): @@ -610,8 +631,6 @@ def data_table(): title="Titanic QueryChat App", fillable=True, ) - - app_opts(bookmark_store="url") ``` Parameters @@ -675,9 +694,6 @@ def data_table(): """ - # Class-level cache for bookmarking settings detected during stub session - _bookmarking_settings: ClassVar[dict[str, bool]] = {} - @overload def __init__( self: QueryChatExpress[Any], @@ -693,6 +709,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -711,6 +728,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -729,6 +747,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -747,6 +766,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -765,6 +785,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ) -> None: ... @@ -782,6 +803,7 @@ def __init__( prompt_template: Optional[str | Path] = None, categorical_threshold: int = 20, data_description: Optional[str | Path] = None, + history: Optional[bool | HistoryOptions] = None, enable_bookmarking: Literal["auto", True, False] = "auto", ): # Sanity check: Express should always have a (stub/real) session @@ -803,26 +825,20 @@ def __init__( categorical_threshold=categorical_threshold, extra_instructions=extra_instructions, prompt_template=prompt_template, + history=history, ) self.id = id or (f"querychat_{table_name}" if table_name else "querychat") - # Determine bookmarking setting - # During stub session: detect from app_opts and cache in class variable - # During real session: retrieve from class variable - enable: bool - if enable_bookmarking == "auto": - if isinstance(session, ExpressStubSession): - store = session.app_opts.get("bookmark_store", "disable") - enable = store != "disable" - # Cache for the real session - QueryChatExpress._bookmarking_settings[self.id] = enable - else: - # Retrieve and clean up (pop prevents memory accumulation) - enable = QueryChatExpress._bookmarking_settings.pop(self.id, False) - else: - enable = enable_bookmarking + if enable_bookmarking != "auto": + warnings.warn( + "`enable_bookmarking` is deprecated. " + 'Use `history=shinychat.types.HistoryOptions(restore_mode="bookmark")` ' + "instead, for the equivalent behavior.", + FutureWarning, + stacklevel=2, + ) - self._enable_bookmarking = enable + self._enable_bookmarking = enable_bookmarking self._vals: ServerValues[IntoFrameT] | None = None def _ensure_server_started(self) -> None: @@ -843,13 +859,22 @@ def _ensure_server_started(self) -> None: return self._require_initialized("_ensure_server_started") self._mark_server_initialized() + resolved_history: bool | HistoryOptions = ( + self.history + if self.history is not None + else ( + HistoryOptions(restore_mode="bookmark") + if self._enable_bookmarking is True + else True + ) + ) self._vals = mod_server( self.id, data_sources=dict(self._data_sources), executor=self._require_query_executor("_ensure_server_started"), greeting=self.greeting, client=self._create_session_client, - enable_bookmarking=self._enable_bookmarking, + history=resolved_history, tools=self.tools, greeter=self.greeter, greeting_base=None, @@ -914,12 +939,7 @@ def ui(self, *, id: Optional[str] = None, **kwargs): A UI component. """ - result = mod_ui( - id or self.id, - preload_viz=has_viz_tool(self.tools), - greeting=self.greeting, - **kwargs, - ) + result = mod_ui(id or self.id, preload_viz=has_viz_tool(self.tools), **kwargs) self._ensure_server_started() return result diff --git a/pkg-py/src/querychat/_shiny_module.py b/pkg-py/src/querychat/_shiny_module.py index 6d5b00c7a..efc12a4fd 100644 --- a/pkg-py/src/querychat/_shiny_module.py +++ b/pkg-py/src/querychat/_shiny_module.py @@ -3,18 +3,17 @@ import warnings from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Generic, TypedDict, Union +from typing import TYPE_CHECKING, Any, Generic, TypedDict, Union import chatlas import shinychat from narwhals.stable.v1.typing import IntoFrameT -from shinychat import Attachment, attachment_to_content +from shinychat.types import HistoryOptions from shiny import module, reactive, ui from ._querychat_core import warn_multi_table_flat_accessor from ._table_accessor import TableAccessor -from ._utils_shinychat import chat_greeting_persistent from ._viz_altair_widget import AltairWidget from ._viz_ggsql import execute_ggsql from ._viz_utils import has_viz_tool, preload_viz_deps_server, preload_viz_deps_ui @@ -70,14 +69,12 @@ class TableState(Generic[IntoFrameT]): @module.ui -def mod_ui(*, preload_viz: bool = False, greeting: str | None = None, **kwargs): +def mod_ui(*, preload_viz: bool = False, **kwargs): css_path = Path(__file__).parent / "static" / "css" / "styles.css" js_path = Path(__file__).parent / "static" / "js" / "querychat.js" kwargs.setdefault("enable_cancel", True) kwargs.setdefault("allow_attachments", True) - if greeting: - kwargs.setdefault("greeting", chat_greeting_persistent(greeting)) tag = shinychat.chat_ui(CHAT_ID, **kwargs) tag.add_class("querychat") @@ -212,19 +209,11 @@ def mod_server( executor: QueryExecutor | None, greeting: str | None, client: Callable[..., chatlas.Chat], - enable_bookmarking: bool, + history: bool | HistoryOptions, tools: set[str] | None = None, greeter: QueryChatGreeter, greeting_base: chatlas.Chat | None = None, ) -> ServerValues[IntoFrameT]: - # Holds a generated greeting so it can be saved and restored on bookmark. - # Static greetings live in the UI (chat_ui(greeting=)) and persist already. - # Workaround for posit-dev/shinychat#253: shinychat does not bookmark - # greetings or expose their state. If that issue is fixed, this value, the - # get_last_turn() capture below, and the greeting handling in - # on_bookmark/on_restore can be dropped (and the shinychat minimum bumped). - current_greeting = ReactiveStringOrNone(None) - if not callable(client): raise TypeError("mod_server() requires a callable client factory.") @@ -309,48 +298,45 @@ def _stub_df(): if has_viz_tool(tools): preload_viz_deps_server() - # Chat UI logic - chat_ui = shinychat.Chat(CHAT_ID) - ctrl = chatlas.StreamController() - - @chat_ui.on_user_submit - async def _(user_input: str, attachments: list[Attachment]): - contents = [attachment_to_content(a) for a in attachments] - stream = await chat.stream_async( - user_input, *contents, echo="none", content="all", controller=ctrl + async def _make_greeting(): + # shinychat invokes this asynchronously, so there's no useful caller frame + # to point to — stacklevel=1 just points at this line. + warnings.warn( + "No greeting provided to `QueryChat()`. Using the LLM `client` to generate one now. " + "For faster startup, lower cost, and determinism, consider providing a greeting " + "to `QueryChat()` and `.generate_greeting()` to generate one beforehand.", + GreetWarning, + stacklevel=1, ) - await chat_ui.append_message_stream(stream) + stream = await greeter.generate_async(base=greeting_base) + return shinychat.chat_greeting(stream, persistent=True) - @reactive.effect - @reactive.event(input[f"{CHAT_ID}_cancel"]) - def _handle_cancel(): - ctrl.cancel() - - if greeting is None: - - @reactive.effect - @reactive.event(input[f"{CHAT_ID}_greeting_requested"]) - async def _handle_greeting_requested(): - # Re-display a restored greeting rather than generating a new one. - # On empty-chat restore both this and on_restore set the greeting - # (harmless, identical content); on non-empty restore this never - # fires, so on_restore is the only path that re-displays. - existing = current_greeting.get() - if existing is not None: - await chat_ui.set_greeting(chat_greeting_persistent(existing)) - return - warnings.warn( - "No greeting provided to `QueryChat()`. Using the LLM `client` to generate one now. " - "For faster startup, lower cost, and determinism, consider providing a greeting " - "to `QueryChat()` and `.generate_greeting()` to generate one beforehand.", - GreetWarning, - stacklevel=2, - ) - await greeter.generate_stream( - chat_ui=chat_ui, - current_greeting=current_greeting, - base=greeting_base, - ) + greeting_arg = ( + _make_greeting + if greeting is None + else shinychat.chat_greeting(greeting, persistent=True) + ) + + shinychat_chat = shinychat.Chat( + CHAT_ID, + client=chat, + greeting=greeting_arg, + history=history, + ) + + # Skipped when `history` is already in bookmark mode: shinychat_chat.history + # is then already enabled for this chat/client, and shinychat treats it and + # enable_bookmarking() as mutually exclusive. Otherwise, register + # enable_bookmarking() so the chat client's own state (and greeting) still + # round-trip through Shiny bookmarks the host app might trigger for + # unrelated reasons. Only the save/restore hooks are wanted here; the + # automatic bookmark trigger is turned off (bookmark_on=None) so `history` + # (or the host app) remains the sole source of *when* to bookmark. + history_is_bookmark_mode = ( + isinstance(history, HistoryOptions) and history.restore_mode == "bookmark" + ) + if not history_is_bookmark_mode: + shinychat_chat.enable_bookmarking(chat, bookmark_on=None) # Handle update button clicks @reactive.effect @@ -367,43 +353,53 @@ def _(): table_states[table_name].title.set(new_title) _current_table.set(table_name) - if enable_bookmarking: - chat_ui.enable_bookmarking(chat) - session.bookmark.exclude.append("chat_update") - - @session.bookmark.on_bookmark - def _on_bookmark(x: BookmarkState) -> None: - vals = x.values - for name, state in table_states.items(): - vals[f"querychat_sql_{name}"] = state.sql.get() - vals[f"querychat_title_{name}"] = state.title.get() - greeting_val = current_greeting.get() - if greeting_val is not None: - vals["querychat_greeting"] = greeting_val - if viz_widgets: - vals["querychat_viz_widgets"] = viz_widgets - - @session.bookmark.on_restore - async def _on_restore(x: RestoreState) -> None: - vals = x.values - last_restored: str | None = None - for name, state in table_states.items(): - if f"querychat_sql_{name}" in vals: - state.sql.set(vals[f"querychat_sql_{name}"]) - if vals[f"querychat_sql_{name}"] is not None: - last_restored = name - if f"querychat_title_{name}" in vals: - state.title.set(vals[f"querychat_title_{name}"]) - if last_restored is not None: - _current_table.set(last_restored) - if "querychat_greeting" in vals: - current_greeting.set(vals["querychat_greeting"]) - await chat_ui.set_greeting( - chat_greeting_persistent(vals["querychat_greeting"]) - ) - if "querychat_viz_widgets" in vals: - restored = restore_viz_widgets(executor, vals["querychat_viz_widgets"]) - viz_widgets[:] = restored + def build_state_snapshot() -> dict[str, Any]: + snapshot: dict[str, Any] = {} + for name, state in table_states.items(): + snapshot[f"querychat_sql_{name}"] = state.sql.get() + snapshot[f"querychat_title_{name}"] = state.title.get() + if viz_widgets: + snapshot["querychat_viz_widgets"] = list(viz_widgets) + return snapshot + + def apply_state_snapshot(vals: dict[str, Any]) -> None: + last_restored: str | None = None + for name, state in table_states.items(): + if f"querychat_sql_{name}" in vals: + state.sql.set(vals[f"querychat_sql_{name}"]) + if vals[f"querychat_sql_{name}"] is not None: + last_restored = name + if f"querychat_title_{name}" in vals: + state.title.set(vals[f"querychat_title_{name}"]) + if last_restored is not None: + _current_table.set(last_restored) + if "querychat_viz_widgets" in vals: + restored = restore_viz_widgets(executor, vals["querychat_viz_widgets"]) + viz_widgets[:] = restored + + # Registered unconditionally: both registrations are no-ops unless the + # corresponding persistence layer (Shiny's own bookmarking, or shinychat's + # history) is actually active in the host app. In restore_mode="bookmark", + # shinychat's history-save triggers a real Shiny bookmark internally, so both + # callbacks fire for the same event -- the snapshot is written twice, + # harmlessly, since it's idempotent. + session.bookmark.exclude.append("chat_update") + + @session.bookmark.on_bookmark + def _on_bookmark(x: BookmarkState) -> None: + x.values.update(build_state_snapshot()) + + @session.bookmark.on_restore + def _on_restore(x: RestoreState) -> None: + apply_state_snapshot(x.values) + + @shinychat_chat.history.on_save + def _on_history_save(values: dict[str, Any]) -> None: + values.update(build_state_snapshot()) + + @shinychat_chat.history.on_restore + def _on_history_restore(values: dict[str, Any]) -> None: + apply_state_snapshot(values) if len(table_states) == 1: only_state = next(iter(table_states.values())) diff --git a/pkg-py/src/querychat/_utils_shinychat.py b/pkg-py/src/querychat/_utils_shinychat.py deleted file mode 100644 index 5ce68b6b3..000000000 --- a/pkg-py/src/querychat/_utils_shinychat.py +++ /dev/null @@ -1,19 +0,0 @@ -from __future__ import annotations - -from typing import TYPE_CHECKING, Any - -if TYPE_CHECKING: - from shinychat._chat_types import ChatGreeting - - -# TODO @gadenbuie: remove once shinychat >= 0.5.0 is the minimum (persistent added in 0.4.0.9000) -def chat_greeting_persistent(content: Any) -> ChatGreeting: - from importlib.metadata import version - - import shinychat - from packaging.version import Version - - if Version(version("shinychat")) > Version("0.4.0"): - return shinychat.chat_greeting(content, persistent=True) - else: - return shinychat.chat_greeting(content, dismissible=False) # pyright: ignore[reportArgumentType] diff --git a/pkg-py/tests/playwright/apps/viz_bookmark_app.py b/pkg-py/tests/playwright/apps/viz_bookmark_app.py index d5be5497c..13014ddf2 100644 --- a/pkg-py/tests/playwright/apps/viz_bookmark_app.py +++ b/pkg-py/tests/playwright/apps/viz_bookmark_app.py @@ -4,6 +4,7 @@ from querychat import QueryChat from querychat.data import titanic +from shinychat.types import HistoryOptions from shiny import App, ui @@ -24,7 +25,7 @@ def app_ui(request): def server(input, output, session): - qc.server(enable_bookmarking=True) + qc.server(history=HistoryOptions(restore_mode="bookmark")) app = App(app_ui, server, bookmark_store="server") diff --git a/pkg-py/tests/playwright/test_11_viz_footer.py b/pkg-py/tests/playwright/test_11_viz_footer.py index 8d3591073..be80a2836 100644 --- a/pkg-py/tests/playwright/test_11_viz_footer.py +++ b/pkg-py/tests/playwright/test_11_viz_footer.py @@ -9,6 +9,7 @@ from __future__ import annotations +import time from pathlib import Path from typing import TYPE_CHECKING @@ -16,7 +17,7 @@ from playwright.sync_api import expect if TYPE_CHECKING: - from playwright.sync_api import Download, Page + from playwright.sync_api import Download, Locator, Page from shinychat.playwright import ChatController @@ -24,6 +25,53 @@ TOOL_RESULT_TIMEOUT = 90_000 +def _boxes_match( + a: dict[str, float] | None, b: dict[str, float] | None, tolerance_px: float +) -> bool: + if a is None or b is None: + return a is b + return all(abs(a[key] - b[key]) <= tolerance_px for key in a) + + +def _wait_for_stable_position( + locator: Locator, + quiet_checks: int = 5, + interval_s: float = 0.1, + max_wait_s: float = 8.0, + tolerance_px: float = 0.5, +) -> None: + """ + Wait until `locator`'s bounding box stops changing. + + The viz widget (chart) keeps resizing for a couple of seconds after the + tool result first appears — likely a client/server size-negotiation + round trip for its responsive fill layout. That drags the footer button + below it along for the ride, so a click issued mid-drift can land on + stale coordinates and silently miss the button. + + Comparing boxes within `tolerance_px` (rather than exact equality) avoids + false "still moving" reads from sub-pixel rendering jitter. + """ + deadline = time.monotonic() + max_wait_s + last_box = None + stable_count = 0 + while time.monotonic() < deadline: + box = locator.bounding_box() + if box is not None and _boxes_match(box, last_box, tolerance_px): + stable_count += 1 + if stable_count >= quiet_checks: + return + else: + stable_count = 0 + last_box = box + time.sleep(interval_s) + + pytest.fail( + f"Locator position did not stabilize within {max_wait_s}s " + f"(last bounding box: {last_box})" + ) + + def _download_from_save_menu(page: Page, export_format: str) -> tuple[Download, str]: """Open the save menu, click the requested format, and capture the download.""" page.locator(".querychat-save-btn").click() @@ -58,6 +106,11 @@ def _send_viz_prompt(page: Page, app_10_viz: str, chat_10_viz: ChatController) - # complete and shinychat has finished its final re-render of the tool # result card. expect(chat_10_viz.loc_input).to_be_enabled(timeout=30_000) + # The chart widget below the footer keeps resizing for a couple of + # seconds after that (see _wait_for_stable_position), dragging the + # Show Query button's position along with it. Let it settle before + # tests start clicking, or a click can land on stale coordinates. + _wait_for_stable_position(page.locator(".querychat-show-query-btn")) class TestShowQueryToggle: @@ -105,6 +158,11 @@ def test_toggle_hides_section_again(self, page: Page) -> None: btn.click() # show expect(label).to_have_text("Hide Query") + # Revealing the query section changes the card's layout, which + # kicks off another round of the chart's resize settling (see + # _wait_for_stable_position) and can drag the button along with it. + _wait_for_stable_position(btn) + btn.click() # hide expect(label).to_have_text("Show Query") diff --git a/pkg-py/tests/test_base.py b/pkg-py/tests/test_base.py index 5aceab0ea..c9074e5e2 100644 --- a/pkg-py/tests/test_base.py +++ b/pkg-py/tests/test_base.py @@ -438,3 +438,20 @@ def test_system_prompt_built_exactly_once(self, ibis_backend_with_tables): and "Multiple tables" in str(x.message) ] assert len(multi_table_warns) == 1 + + +def test_history_stored_verbatim_no_default_substitution(): + """ + QueryChatBase stores history exactly as given -- including None -- so callers + can later distinguish 'never set' from 'explicitly set to a falsy/true value'. + """ + df = pd.DataFrame({"a": [1, 2, 3]}) + + qc_default = QueryChatBase(df, "a_table") + assert qc_default.history is None + + qc_explicit_false = QueryChatBase(df, "a_table2", history=False) + assert qc_explicit_false.history is False + + qc_explicit_true = QueryChatBase(df, "a_table3", history=True) + assert qc_explicit_true.history is True diff --git a/pkg-py/tests/test_shiny.py b/pkg-py/tests/test_shiny.py new file mode 100644 index 000000000..4ecbc31e8 --- /dev/null +++ b/pkg-py/tests/test_shiny.py @@ -0,0 +1,205 @@ +""" +Tests for QueryChat's Shiny-specific public API: history/enable_bookmarking +resolution and deprecation, and $app()'s bookmark inference. +""" + +from __future__ import annotations + +import os + +import pytest + + +@pytest.fixture(autouse=True) +def set_dummy_api_key(): + old = os.environ.get("OPENAI_API_KEY") + os.environ["OPENAI_API_KEY"] = "sk-dummy" + yield + if old is not None: + os.environ["OPENAI_API_KEY"] = old + else: + del os.environ["OPENAI_API_KEY"] + + +def test_server_history_stored_verbatim_before_resolution(): + """Constructor-level history isn't substituted until .server()/.app() resolve it.""" + import pandas as pd + from querychat._shiny import QueryChat + + qc_no_history = QueryChat(pd.DataFrame({"a": [1]}), "a_table") + assert qc_no_history.history is None + + qc_explicit = QueryChat(pd.DataFrame({"a": [1]}), "a_table2", history=False) + assert qc_explicit.history is False + + +def test_server_resolves_history_and_warns_on_explicit_enable_bookmarking(monkeypatch): + import warnings + from unittest.mock import MagicMock, patch + + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table") + + captured = {} + + def fake_mod_server(*args, **kwargs): + captured.update(kwargs) + return MagicMock() + + fake_session = MagicMock() + with ( + patch("querychat._shiny.get_current_session", return_value=fake_session), + patch("querychat._shiny.mod_server", side_effect=fake_mod_server), + ): + # Not passing enable_bookmarking or history: no warning, history resolves to True. + with warnings.catch_warnings(): + warnings.simplefilter("error") + qc.server() + assert captured["history"] is True + + # Explicit enable_bookmarking=True warns, and -- since history wasn't + # otherwise set -- resolves to bookmark-mode history (the equivalent + # of the old bookmarking behavior). + from shinychat.types import HistoryOptions + + with pytest.warns(FutureWarning, match="history"): + qc.server(enable_bookmarking=True) + assert isinstance(captured["history"], HistoryOptions) + assert captured["history"].restore_mode == "bookmark" + + # enable_bookmarking=False warns but has no effect on its own. + with pytest.warns(FutureWarning, match="history"): + qc.server(enable_bookmarking=False) + assert captured["history"] is True + + # Explicit history= still takes precedence over enable_bookmarking. + with pytest.warns(FutureWarning, match="history"): + qc.server(history=False, enable_bookmarking=True) + assert captured["history"] is False + + # Explicit history= takes precedence over self.history. + qc.history = False + with warnings.catch_warnings(): + warnings.simplefilter("error") + qc.server(history=True) + assert captured["history"] is True + + # self.history takes precedence over the True fallback when history= not passed. + with warnings.catch_warnings(): + warnings.simplefilter("error") + qc.server() + assert captured["history"] is False + + # self.history also takes precedence over the enable_bookmarking mapping. + with pytest.warns(FutureWarning, match="history"): + qc.server(enable_bookmarking=True) + assert captured["history"] is False + + +def test_app_defaults_history_to_bookmark_restore_mode_and_enables_shiny_bookmarking(): + + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table") + app = qc.app() + + assert app.bookmark_store == "server" + + +def test_app_disables_shiny_bookmarking_when_history_is_not_bookmark_mode(): + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table", history=True) + app = qc.app() + + assert app.bookmark_store == "disable" + + +def test_app_respects_explicit_constructor_history_over_apps_own_default(): + """ + An explicit QueryChat(history=False) must not be silently overridden by + $app()'s own restore_mode='bookmark' default. + """ + import pandas as pd + from querychat._shiny import QueryChat + + qc = QueryChat(pd.DataFrame({"a": [1, 2, 3]}), "a_table", history=False) + app = qc.app() + + assert app.bookmark_store == "disable" + + +def test_express_enable_bookmarking_auto_emits_no_warning_and_uses_history(): + from unittest.mock import MagicMock, patch + + import pandas as pd + from querychat._shiny import QueryChatExpress + from shiny.express._stub_session import ExpressStubSession + + fake_stub_session = MagicMock(spec=ExpressStubSession) + fake_stub_session.app_opts = {} + + with patch("querychat._shiny.get_current_session", return_value=fake_stub_session): + import warnings + + with warnings.catch_warnings(): + warnings.simplefilter("error") + QueryChatExpress(pd.DataFrame({"a": [1, 2, 3]}), "a_table") + + +def test_express_enable_bookmarking_resolves_to_bookmark_mode_history(monkeypatch): + """ + enable_bookmarking=True at construction time must map to bookmark-mode + history once the real server starts, mirroring QueryChat.server()'s + resolution (see test_server_resolves_history_and_warns_on_explicit_enable_bookmarking). + """ + from unittest.mock import MagicMock + + import pandas as pd + from querychat._shiny import QueryChatExpress + from shiny._namespaces import Root + from shiny.session import session_context + from shinychat.types import HistoryOptions + + captured = {} + + def fake_mod_server(*args, **kwargs): + captured.update(kwargs) + return MagicMock() + + monkeypatch.setattr("querychat._shiny.mod_server", fake_mod_server) + + mock_session = MagicMock() + mock_session.ns = Root + with session_context(mock_session): + with pytest.warns(FutureWarning, match="history"): + qc = QueryChatExpress( + pd.DataFrame({"a": [1, 2, 3]}), "a_table", enable_bookmarking=True + ) + qc._ensure_server_started() + + assert isinstance(captured["history"], HistoryOptions) + assert captured["history"].restore_mode == "bookmark" + + +def test_express_explicit_enable_bookmarking_warns(): + from unittest.mock import MagicMock, patch + + import pandas as pd + from querychat._shiny import QueryChatExpress + from shiny.express._stub_session import ExpressStubSession + + fake_stub_session = MagicMock(spec=ExpressStubSession) + fake_stub_session.app_opts = {} + + with ( + patch("querychat._shiny.get_current_session", return_value=fake_stub_session), + pytest.warns(FutureWarning, match="history"), + ): + QueryChatExpress( + pd.DataFrame({"a": [1, 2, 3]}), "a_table", enable_bookmarking=True + ) diff --git a/pkg-py/tests/test_shiny_module.py b/pkg-py/tests/test_shiny_module.py index 812bb75e4..e92a0779a 100644 --- a/pkg-py/tests/test_shiny_module.py +++ b/pkg-py/tests/test_shiny_module.py @@ -46,3 +46,283 @@ def test_mod_ui_allow_attachments_can_be_overridden(): with patch("querychat._shiny_module.shinychat.chat_ui", side_effect=_fake_chat_ui): mod_ui("test", allow_attachments=False) assert _fake_chat_ui.last_kwargs.get("allow_attachments") is False + + +def _unwrap_module_server(decorated): + """ + Recover the undecorated function wrapped by @module.server. + + shiny's module.server decorator closes over the original function under the + freevar name "fn"; look it up by name rather than assuming a cell index, since + the closure's cell order isn't part of shiny's public contract. + """ + code = decorated.__code__ + idx = code.co_freevars.index("fn") + return decorated.__closure__[idx].cell_contents + + +def test_mod_server_passes_client_and_history_to_chat(): + """After this change, Chat is constructed with client= and history=.""" + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + + captured = {} + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + captured["client"] = client + captured["greeting"] = greeting + captured["history"] = history + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=True, + tools=None, + greeter=MagicMock(), + ) + + assert captured.get("client") is not None, "client= should be passed to Chat" + assert captured.get("history") is True, "history= should be forwarded verbatim" + assert callable(captured.get("greeting")), "greeting= should be a callable" + + +def test_mod_server_registers_chat_bookmarking_with_no_auto_trigger_when_history_not_bookmark_mode(): + """ + Chat.enable_bookmarking() is called when `history` isn't bookmark mode, so + the chat client's own state round-trips through Shiny bookmarks whenever + the host app has bookmarking enabled. It's called with bookmark_on=None so + `history` (or the host app) remains the sole source of *when* to + bookmark, not shinychat's own auto-trigger. + """ + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=True, + tools=None, + greeter=MagicMock(), + ) + + fake_chat_instance.enable_bookmarking.assert_called_once() + _, kwargs = fake_chat_instance.enable_bookmarking.call_args + assert kwargs.get("bookmark_on") is None + + +def test_mod_server_skips_chat_bookmarking_when_history_is_bookmark_mode(): + """ + Chat.enable_bookmarking() is skipped when `history` is bookmark mode -- + shinychat_chat.history is already enabled for this chat/client in that + case, and shinychat treats it and enable_bookmarking() as mutually + exclusive. + """ + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + from shinychat.types import HistoryOptions + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=HistoryOptions(restore_mode="bookmark"), + tools=None, + greeter=MagicMock(), + ) + + fake_chat_instance.enable_bookmarking.assert_not_called() + + +def test_mod_server_registers_table_state_with_both_bookmark_and_history_hooks(): + """Table/viz state callbacks must always register with both APIs, unconditionally.""" + from unittest.mock import MagicMock, patch + + from querychat._shiny_module import mod_server + + fake_chat_instance = MagicMock() + + def fake_chat_constructor( + id, *, client=None, greeting=None, history=None, **kwargs + ): + return fake_chat_instance + + fake_source = MagicMock() + fake_source.get_data.return_value = [] + fake_executor = MagicMock() + fake_executor.execute_query.return_value = [] + + def client_factory(**kwargs): + return MagicMock(spec=["stream_async"]) + + inner_fn = _unwrap_module_server(mod_server) + + fake_input = MagicMock() + fake_input.__getitem__ = MagicMock(return_value=MagicMock()) + fake_session = MagicMock() + fake_session.is_stub_session.return_value = False + fake_session.bookmark = MagicMock() + fake_session.bookmark.exclude = [] + + with ( + patch( + "querychat._shiny_module.shinychat.Chat", side_effect=fake_chat_constructor + ), + patch("querychat._shiny_module.has_viz_tool", return_value=False), + ): + inner_fn( + fake_input, + MagicMock(), + fake_session, + data_sources={"t": fake_source}, + executor=fake_executor, + greeting=None, + client=client_factory, + history=False, # even with history disabled, registration must still happen + tools=None, + greeter=MagicMock(), + ) + + assert "chat_update" in fake_session.bookmark.exclude + fake_session.bookmark.on_bookmark.assert_called_once() + fake_session.bookmark.on_restore.assert_called_once() + fake_chat_instance.history.on_save.assert_called_once() + fake_chat_instance.history.on_restore.assert_called_once() + + +def test_shinychat_chat_contract_used_by_mod_server(): + """ + Thin, non-mocked check of the shinychat surface mod_server() depends on. + + Every other test above patches `shinychat.Chat` with a fake constructor, + so an upstream rename/removal of `Chat(client=, greeting=, history=)`, + `.history.on_save()`/`.history.on_restore()`, or + `.enable_bookmarking(client, bookmark_on=)` wouldn't turn any of them red. + This constructs the real shinychat.Chat and calls the real methods with + the same arguments mod_server() uses, so it does. + """ + from unittest.mock import MagicMock + + import shinychat + from shiny._namespaces import Root + from shiny.session import session_context + + mock_session = MagicMock() + mock_session.ns = Root + mock_session.app = None + + with session_context(mock_session): + chat = shinychat.Chat( + "chat", client=MagicMock(), greeting=None, history=True + ) + + @chat.history.on_save + def _on_save(values): + return values + + @chat.history.on_restore + def _on_restore(values): + return values + + chat.enable_bookmarking(MagicMock(), bookmark_on=None) + + assert _on_save in chat.history._save_callbacks + assert _on_restore in chat.history._restore_callbacks diff --git a/pkg-r/DESCRIPTION b/pkg-r/DESCRIPTION index 6c9422824..577e6a9c1 100644 --- a/pkg-r/DESCRIPTION +++ b/pkg-r/DESCRIPTION @@ -35,7 +35,7 @@ Imports: rlang (>= 1.1.0), S7, shiny, - shinychat (>= 0.4.0), + shinychat (> 0.4.0), utils, whisker, yaml diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 7f8ba58a1..504a803af 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -29,10 +29,18 @@ * File attachments are now enabled by default in the Shiny chat UI. Users can attach images, PDFs, and text files to their messages and the LLM will receive them. Disable with `allow_attachments = FALSE` in `mod_ui()` or `QueryChat$ui()`. (#253) +* 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`. + ## 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) +* `$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()`. + +## Deprecated + +* `$server()`'s `enable_bookmarking` parameter is deprecated in favor of `history`. Pass `history = shinychat::history_options(restore_mode = "bookmark")` instead of `enable_bookmarking = TRUE` for the equivalent behavior. + ## Improvements * Chat greetings now use shinychat's greeting API (requires shinychat >= 0.4.0). A provided `greeting` renders instantly when the app loads, and when no `greeting` is given one is generated on demand — now **schema-aware**, so it can describe the data it's about to help you explore — without being added to the conversation history. Generated greetings are preserved across bookmark/restore. Tables passed to `QueryChat$new()` are described in the greeting automatically; opt additional tables in with `include_in_greeting = TRUE` on `$add_table()`/`$add_tables()`, or fine-tune which tables and which template the greeting uses via `qc$greeter`. (#249, #261) diff --git a/pkg-r/R/QueryChat.R b/pkg-r/R/QueryChat.R index 9e0731bc5..38a92d79c 100644 --- a/pkg-r/R/QueryChat.R +++ b/pkg-r/R/QueryChat.R @@ -228,6 +228,8 @@ QueryChat <- R6::R6Class( public = list( #' @field greeting The greeting message displayed to users. greeting = NULL, + #' @field history Conversation history configuration. + history = NULL, #' @field id ID for the QueryChat instance. id = NULL, #' @field id_override Whether the ID was explicitly set by the user. @@ -256,6 +258,10 @@ QueryChat <- R6::R6Class( #' a greeting will be generated at the start of each conversation using #' the LLM, which adds latency and cost. Use `$generate_greeting()` to #' create a greeting to save and reuse. + #' @param history Conversation history configuration: `NULL` (default; + #' resolves to `TRUE` when `$server()`/`$app()` is called and nothing else + #' was set), `TRUE`/`FALSE`, or a [shinychat::history_options()] object. + #' Passed straight through to `shinychat::chat_server(history = )`. #' @param client Optional chat client. Can be: #' - An [ellmer::Chat] object #' - A string to pass to [ellmer::chat()] (e.g., `"openai/gpt-4o"`) @@ -294,6 +300,7 @@ QueryChat <- R6::R6Class( ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -308,6 +315,7 @@ QueryChat <- R6::R6Class( # Validate arguments check_string(id, allow_null = TRUE) check_string(greeting, allow_null = TRUE) + check_history(history) arg_match( tools, values = c("filter", "update", "query", "visualize"), @@ -341,6 +349,7 @@ QueryChat <- R6::R6Class( greeting <- read_utf8(greeting) } self$greeting <- greeting + self$history <- history # Track whether id was explicitly set self$id_override <- id @@ -688,13 +697,16 @@ QueryChat <- R6::R6Class( #' Create and run a Shiny gadget for chatting with data #' #' @param ... Arguments passed to `$app_obj()`. - #' @param bookmark_store The bookmarking storage method. Passed to - #' [shiny::enableBookmarking()]. If `"url"` or `"server"`, the chat state - #' (including current query) will be bookmarked. Default is `"url"`. + #' @param history Conversation history configuration for the generated app. + #' Defaults to `shinychat::history_options(restore_mode = "bookmark")` when + #' neither this nor `$new()`'s `history` was set, since `$app()`'s whole + #' purpose is a single, shareable demo. When the resolved value has + #' `restore_mode = "bookmark"`, the generated app automatically enables + #' Shiny's own server-side bookmarking. #' #' @return Invisibly returns a list of session-specific values. - app = function(..., bookmark_store = "url") { - app <- self$app_obj(..., bookmark_store = bookmark_store) + app = function(..., history = NULL) { + app <- self$app_obj(..., history = history) vals <- tryCatch(shiny::runGadget(app), interrupt = function(cnd) NULL) invisible(vals) }, @@ -703,14 +715,23 @@ QueryChat <- R6::R6Class( #' A streamlined Shiny app for chatting with data #' #' @param ... Additional arguments (currently unused). - #' @param bookmark_store The bookmarking storage method. Passed to - #' [shiny::enableBookmarking()]. Default is `"url"`. + #' @param history Conversation history configuration for the generated app. + #' See `$app()`. #' #' @return A Shiny app object that can be run with `shiny::runApp()`. - app_obj = function(..., bookmark_store = "url") { + app_obj = function(..., history = NULL) { private$require_initialized("$app_obj") check_installed("DT") check_dots_empty() + check_history(history) + resolved_history <- history %||% + self$history %||% + shinychat::history_options(restore_mode = "bookmark") + enable_shiny_bookmarking <- inherits( + resolved_history, + "chat_history_config" + ) && + identical(resolved_history$restore_mode, "bookmark") first_table_name <- names(private$.data_sources)[[1]] @@ -769,8 +790,7 @@ QueryChat <- R6::R6Class( "reset_query", "sql_editor" )) - enable_bookmarking <- bookmark_store %in% c("url", "server") - qc_vals <- self$server(enable_bookmarking = enable_bookmarking) + qc_vals <- self$server(history = resolved_history) active_table_name <- shiny::reactive({ ct <- qc_vals$current_table() @@ -866,7 +886,15 @@ QueryChat <- R6::R6Class( } } - shiny::shinyApp(ui, server, enableBookmarking = bookmark_store) + shiny::shinyApp( + ui, + server, + enableBookmarking = if (enable_shiny_bookmarking) { + "server" + } else { + "disable" + } + ) }, #' @description @@ -909,7 +937,7 @@ QueryChat <- R6::R6Class( id <- id %||% namespaced_id(self$id) - mod_ui(id, ..., greeting = self$greeting) + mod_ui(id, ...) }, #' @description @@ -918,7 +946,12 @@ QueryChat <- R6::R6Class( #' @param data_source Optional data source for backward compatibility. #' If provided, calls `$add_table()` before initializing server logic. #' @param client Optional chat client override for this session. - #' @param enable_bookmarking Whether to enable bookmarking. Default is `FALSE`. + #' @param history Conversation history configuration for this call. Overrides + #' the value set on `$new()`. Resolves to `TRUE` when neither this nor the + #' constructor's `history` was set. + #' @param enable_bookmarking `r lifecycle::badge("deprecated")` Use `history = + #' 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. @@ -932,7 +965,8 @@ QueryChat <- R6::R6Class( server = function( data_source = NULL, client = NULL, - enable_bookmarking = FALSE, + history = NULL, + enable_bookmarking = NULL, ..., id = NULL, session = shiny::getDefaultReactiveDomain() @@ -975,6 +1009,24 @@ QueryChat <- R6::R6Class( ) } + check_history(history) + + if (!is.null(enable_bookmarking)) { + lifecycle::deprecate_warn( + when = "0.4.0", + what = "QueryChat$server(enable_bookmarking = )", + with = "QueryChat$server(history = )", + details = 'Use history = shinychat::history_options(restore_mode = "bookmark") for the equivalent behavior.' + ) + } + + resolved_history <- history %||% + self$history %||% + (if (isTRUE(enable_bookmarking)) { + shinychat::history_options(restore_mode = "bookmark") + }) %||% + TRUE + result <- mod_server( id %||% self$id, data_sources = private$.data_sources, @@ -982,9 +1034,9 @@ QueryChat <- R6::R6Class( greeting = self$greeting, client = create_session_client, tools = self$tools, + history = resolved_history, greeter = self$greeter, - greeting_base = base_client, - enable_bookmarking = enable_bookmarking + greeting_base = base_client ) result }, @@ -1093,6 +1145,8 @@ QueryChat <- R6::R6Class( #' @param ... Additional arguments (currently unused). #' @param id Optional module ID for the QueryChat instance. #' @param greeting Optional initial message to display to users. +#' @param history Conversation history configuration. See [QueryChat]'s +#' `$new()` method. #' @param client Optional chat client. #' @param tools Which querychat tools to include in the chat client. #' @param data_description Optional description of the data. @@ -1114,6 +1168,7 @@ querychat <- function( ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -1141,6 +1196,7 @@ querychat <- function( ..., id = id, greeting = greeting, + history = history, client = client, tools = tools, data_description = data_description, @@ -1153,7 +1209,8 @@ querychat <- function( } #' @rdname querychat-convenience -#' @param bookmark_store The bookmarking storage method. Default is `"url"`. +#' @param history Conversation history configuration for the generated app. See +#' [QueryChat]'s `$app()` method. #' @return Invisibly returns the chat object after the app stops. #' #' @export @@ -1171,7 +1228,7 @@ querychat_app <- function( prompt_template = NULL, data_dict = NULL, cleanup = NA, - bookmark_store = "url" + history = NULL ) { if (shiny::isRunning()) { cli::cli_abort( @@ -1214,7 +1271,7 @@ querychat_app <- function( cleanup = cleanup ) - qc$app(bookmark_store = bookmark_store) + qc$app(history = history) } normalize_tools <- function(tools) { diff --git a/pkg-r/R/QueryChatGreeter.R b/pkg-r/R/QueryChatGreeter.R index 33e4257a3..1af88edff 100644 --- a/pkg-r/R/QueryChatGreeter.R +++ b/pkg-r/R/QueryChatGreeter.R @@ -37,28 +37,6 @@ QueryChatGreeter <- R6::R6Class( echo <- rlang::arg_match(echo) client <- self$build_client(base) as.character(client$chat(GREETING_PROMPT, echo = echo)) - }, - - #' @description Stream a greeting into the chat UI and capture the result - #' (Shiny path). - #' @param greeting_reactive Session reactiveVal to receive the generated greeting. - #' @param base Optional resolved client to clone. - #' @param chat_id Chat component id targeted by the Shiny stream (default "chat"). - generate_stream = function( - greeting_reactive, - base = NULL, - chat_id = "chat" - ) { - client <- self$build_client(base) - stream <- client$stream_async(GREETING_PROMPT) - p <- shinychat::chat_set_greeting( - chat_id, - chat_greeting_persistent(stream) - ) - promises::then(p, function(value) { - greeting_reactive(client$last_turn()@text) - }) - p } ), active = list( diff --git a/pkg-r/R/querychat_module.R b/pkg-r/R/querychat_module.R index 224b46fea..f050ddba6 100644 --- a/pkg-r/R/querychat_module.R +++ b/pkg-r/R/querychat_module.R @@ -1,19 +1,5 @@ # Main module UI function -mod_ui <- function( - id, - ..., - greeting = NULL, - enable_cancel = TRUE, - allow_attachments = TRUE -) { - ns <- shiny::NS(id) - - if (!is.null(greeting) && any(nzchar(greeting))) { - greeting <- chat_greeting_persistent(greeting) - } else { - greeting <- NULL - } - +mod_ui <- function(id, ...) { htmltools::tagList( htmltools::htmlDependency( "querychat", @@ -24,12 +10,9 @@ mod_ui <- function( stylesheet = "styles.css" ), shinychat::chat_ui( - ns("chat"), + shiny::NS(id, "chat"), height = "100%", class = "querychat", - enable_cancel = enable_cancel, - allow_attachments = allow_attachments, - greeting = greeting, ... ) ) @@ -43,19 +26,12 @@ mod_server <- function( greeting, client, tools, + history, greeter = NULL, - greeting_base = NULL, - enable_bookmarking = FALSE + greeting_base = NULL ) { shiny::moduleServer(id, function(input, output, session) { current_table_val <- shiny::reactiveVal(NULL, label = "current_table") - # Holds a generated greeting so it can be saved and restored on bookmark. - # Static greetings live in the UI (chat_ui(greeting=)) and persist already. - # Workaround for posit-dev/shinychat#253: shinychat does not bookmark - # greetings or expose their state. If that issue is fixed, this reactiveVal, - # the last_turn() capture below, and the greeting handling in - # onBookmark/onRestore can be dropped (and the shinychat minimum bumped). - current_greeting <- shiny::reactiveVal(NULL, label = "current_greeting") # Per-table reactive state tables <- list() @@ -83,17 +59,6 @@ mod_server <- function( }) } - append_output <- function(...) { - txt <- paste0(...) - shinychat::chat_append_message( - "chat", - list(role = "assistant", content = txt), - chunk = TRUE, - operation = "append", - session = session - ) - } - update_dashboard <- function(query, title, table) { if (!is.null(query)) { tables[[table]]$sql(query) @@ -116,9 +81,7 @@ mod_server <- function( ) } - # Non-reactive bookkeeping for bookmark save/restore of viz widgets viz_widgets <- list() - on_visualize <- function(data) { viz_widgets[[length(viz_widgets) + 1L]] <<- list( widget_id = data$widget_id, @@ -126,9 +89,8 @@ mod_server <- function( ) } - # Set up the chat object for this session check_function(client) - chat <- client( + pre_built_client <- client( update_dashboard = update_dashboard, reset_dashboard = reset_query, visualize = on_visualize, @@ -136,138 +98,130 @@ mod_server <- function( session = session ) - if (is.null(greeting)) { - shiny::observeEvent( - input$chat_greeting_requested, - label = "on_greeting_requested", - { - # Re-display a restored greeting rather than generating a new one. - # On empty-chat restore both this and onRestore set the greeting - # (harmless, identical content); on non-empty restore this never fires, - # so onRestore is the only path that re-displays. - if (!is.null(current_greeting())) { - shinychat::chat_set_greeting( - "chat", - chat_greeting_persistent(current_greeting()) - ) - return() - } - cli::cli_warn(c( - "No {.arg greeting} provided to {.fn QueryChat}. Using the LLM {.arg client} to generate one now.", - "i" = "For faster startup, lower cost, and determinism, consider providing a {.arg greeting} to {.fn QueryChat}.", - "i" = "You can use your {.help querychat::QueryChat} object's {.fn $generate_greeting} method to generate a greeting." - )) - greeter$generate_stream( - greeting_reactive = current_greeting, - base = greeting_base - ) - } - ) + greeting_arg <- if (is.null(greeting)) { + function() { + cli::cli_warn(c( + "No {.arg greeting} provided to {.fn QueryChat}. Using the LLM {.arg client} to generate one now.", + "i" = "For faster startup, lower cost, and determinism, consider providing a {.arg greeting} to {.fn QueryChat}.", + "i" = "You can use your {.help querychat::QueryChat} object's {.fn $generate_greeting} method to generate a greeting." + )) + greeting_client <- greeter$build_client(greeting_base) + stream <- greeting_client$stream_async(GREETING_PROMPT) + shinychat::chat_greeting(stream, persistent = TRUE) + } + } else { + shinychat::chat_greeting(greeting, persistent = TRUE) } - ctrl <- ellmer::stream_controller() - - append_stream_task <- shiny::ExtendedTask$new( - function(client, user_input, controller = NULL) { - user_input_parts <- if (is.list(user_input)) { - user_input - } else { - list(user_input) - } - stream <- client$stream_async( - !!!user_input_parts, - stream = "content", - controller = controller - ) - - p <- promises::promise_resolve(stream) - promises::then(p, function(stream) { - shinychat::chat_append("chat", stream) - }) - } + chat_module <- shinychat::chat_server( + "chat", + pre_built_client, + greeting = greeting_arg, + history = history ) - shiny::observeEvent(input$chat_user_input, label = "on_chat_user_input", { - append_stream_task$invoke(chat, input$chat_user_input, controller = ctrl) - }) - - shiny::observeEvent(input$chat_cancel, label = "on_chat_cancel", { - ctrl$cancel() - }) - - shiny::observeEvent(input$chat_update, label = "on_chat_update", { - tbl <- input$chat_update$table - if (!is.null(tbl) && tbl %in% names(tables)) { - q <- input$chat_update$query - ttl <- input$chat_update$title - tables[[tbl]]$sql(if (nzchar(q %||% "")) q else NULL) - tables[[tbl]]$title(if (nzchar(ttl %||% "")) ttl else NULL) - current_table_val(tbl) - } - }) - - if (enable_bookmarking) { + # Skipped when `history` is already in bookmark mode: chat_server() has + # then already registered chat_enable_history() for this id/client, and + # shinychat docs call chat_enable_history() and chat_restore() mutually + # exclusive. Otherwise, register chat_restore() so the chat client's own + # state (and greeting) still round-trip through Shiny bookmarks the host + # app might trigger for unrelated reasons. Only the save/restore hooks are + # wanted here; the automatic bookmark triggers are turned off so `history` + # (or the host app) remains the sole source of *when* to bookmark. + history_is_bookmark_mode <- inherits(history, "chat_history_config") && + identical(history$restore_mode, "bookmark") + if (!history_is_bookmark_mode) { shinychat::chat_restore( "chat", - chat, + pre_built_client, + bookmark_on_input = FALSE, + bookmark_on_response = FALSE, restore_ui = FALSE, session = session ) - shiny::setBookmarkExclude("chat_update", session = session) + } - shiny::onBookmark(function(state) { - table_states <- list() - for (name in names(tables)) { - table_states[[name]] <- list( - sql = tables[[name]]$sql(), - title = tables[[name]]$title() - ) - } - state$values$querychat_tables <- table_states - if (!is.null(current_greeting())) { - state$values$querychat_greeting <- current_greeting() - } - if (length(viz_widgets) > 0) { - state$values$querychat_viz_widgets <- viz_widgets + shiny::observeEvent( + input$chat_update, + label = "on_chat_update", + { + upd <- input$chat_update + tbl <- upd$table + if (!is.null(tbl) && tbl %in% names(tables)) { + q <- upd$query + ttl <- upd$title + tables[[tbl]]$sql(if (nzchar(q %||% "")) q else NULL) + tables[[tbl]]$title(if (nzchar(ttl %||% "")) ttl else NULL) + current_table_val(tbl) } - }) + } + ) - shiny::onRestore(function(state) { - if (!is.null(state$values$querychat_tables)) { - last_restored <- NULL - for (name in names(state$values$querychat_tables)) { - tbl_state <- state$values$querychat_tables[[name]] - if (!is.null(tbl_state$sql)) { - tables[[name]]$sql(tbl_state$sql) - last_restored <- name - } - if (!is.null(tbl_state$title)) { - tables[[name]]$title(tbl_state$title) - } + build_state_snapshot <- function() { + table_states <- list() + for (name in names(tables)) { + table_states[[name]] <- list( + sql = tables[[name]]$sql(), + title = tables[[name]]$title() + ) + } + snapshot <- list(querychat_tables = table_states) + if (length(viz_widgets) > 0) { + snapshot$querychat_viz_widgets <- viz_widgets + } + snapshot + } + + apply_state_snapshot <- function(values) { + if (!is.null(values$querychat_tables)) { + last_restored <- NULL + for (name in names(tables)) { + tbl_state <- values$querychat_tables[[name]] + if (!is.null(tbl_state$sql)) { + tables[[name]]$sql(tbl_state$sql) + last_restored <- name } - if (!is.null(last_restored)) { - current_table_val(last_restored) + if (!is.null(tbl_state$title)) { + tables[[name]]$title(tbl_state$title) } } - if (!is.null(state$values$querychat_greeting)) { - current_greeting(state$values$querychat_greeting) - shinychat::chat_set_greeting( - "chat", - chat_greeting_persistent(state$values$querychat_greeting), - session = session - ) - } - if (!is.null(state$values$querychat_viz_widgets)) { - restored <- restore_viz_widgets( - executor, - restore_record_list(state$values$querychat_viz_widgets), - session - ) - viz_widgets <<- restored + if (!is.null(last_restored)) { + current_table_val(last_restored) } - }) + } + if (!is.null(values$querychat_viz_widgets)) { + restored <- restore_viz_widgets( + executor, + restore_record_list(values$querychat_viz_widgets), + session + ) + viz_widgets <<- restored + } + invisible(NULL) } + # Registered unconditionally: both registrations are no-ops unless the + # corresponding persistence layer (Shiny's own bookmarking, or shinychat's + # history) is actually active in the host app. In restore_mode = "bookmark", + # shinychat's history-save triggers a real Shiny bookmark internally, so both + # callbacks fire for the same event -- the snapshot is written twice, + # harmlessly, since it's idempotent. + shiny::setBookmarkExclude("chat_update") + + shiny::onBookmark(function(state) { + state$values <- utils::modifyList(state$values, build_state_snapshot()) + }) + + shiny::onRestore(function(state) { + apply_state_snapshot(state$values) + }) + + chat_module$history$on_save(function(values) { + utils::modifyList(values, build_state_snapshot()) + }) + + chat_module$history$on_restore(apply_state_snapshot) + table_fn <- function(name) { if (!name %in% names(tables)) { available <- paste0("'", names(tables), "'", collapse = ", ") @@ -280,11 +234,10 @@ mod_server <- function( table_names_fn <- function() names(tables) - # Backward compat: for single-table, expose sql/title/df directly if (length(data_sources) == 1) { first <- tables[[1]] list( - client = chat, + client = chat_module$client, sql = first$sql, title = first$title, df = first$df, @@ -302,7 +255,7 @@ mod_server <- function( } } list( - client = chat, + client = chat_module$client, sql = single_table_error("sql"), title = single_table_error("title"), df = single_table_error("df"), diff --git a/pkg-r/R/utils-check.R b/pkg-r/R/utils-check.R index 966df62d7..0588a6950 100644 --- a/pkg-r/R/utils-check.R +++ b/pkg-r/R/utils-check.R @@ -12,6 +12,32 @@ check_data_source <- function( } } +# History parameter validation ------------------------------------------- + +#' Check that a value is a valid `history` argument +#' +#' Accepts `NULL` (not specified), `TRUE`, `FALSE`, or a +#' `shinychat::history_options()` object. +#' +#' @noRd +check_history <- function( + x, + ..., + arg = caller_arg(x), + call = caller_env() +) { + check_dots_empty() + if ( + is.null(x) || isTRUE(x) || isFALSE(x) || inherits(x, "chat_history_config") + ) { + return(invisible(NULL)) + } + cli::cli_abort( + "{.arg {arg}} must be {.code NULL}, {.code TRUE}, {.code FALSE}, or a {.fun shinychat::history_options} object, not {.obj_type_friendly {x}}.", + call = call + ) +} + # SQL table name validation ---------------------------------------------- #' Check SQL table name validity diff --git a/pkg-r/R/utils-shinychat.R b/pkg-r/R/utils-shinychat.R deleted file mode 100644 index a99ffa0a1..000000000 --- a/pkg-r/R/utils-shinychat.R +++ /dev/null @@ -1,10 +0,0 @@ -# shinychat compatibility helpers - -# TODO: remove once shinychat >= 0.5.0 is the minimum (persistent added in 0.4.0.9000) -chat_greeting_persistent <- function(content) { - if (utils::packageVersion("shinychat") > "0.4.0") { - shinychat::chat_greeting(content, persistent = TRUE) - } else { - shinychat::chat_greeting(content, dismissible = FALSE) - } -} diff --git a/pkg-r/man/QueryChat.Rd b/pkg-r/man/QueryChat.Rd index 915b4f517..548dcae23 100644 --- a/pkg-r/man/QueryChat.Rd +++ b/pkg-r/man/QueryChat.Rd @@ -98,6 +98,8 @@ qc <- QueryChat$new(con, "mtcars") \describe{ \item{\code{greeting}}{The greeting message displayed to users.} + \item{\code{history}}{Conversation history configuration.} + \item{\code{id}}{ID for the QueryChat instance.} \item{\code{id_override}}{Whether the ID was explicitly set by the user.} @@ -151,6 +153,7 @@ access its \verb{$tables} and \verb{$prompt}.} ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -183,6 +186,10 @@ character string (in Markdown format) or a file path. If not provided, a greeting will be generated at the start of each conversation using the LLM, which adds latency and cost. Use \verb{$generate_greeting()} to create a greeting to save and reuse.} + \item{\code{history}}{Conversation history configuration: \code{NULL} (default; +resolves to \code{TRUE} when \verb{$server()}/\verb{$app()} is called and nothing else +was set), \code{TRUE}/\code{FALSE}, or a \code{\link[shinychat:history_options]{shinychat::history_options()}} object. +Passed straight through to \code{shinychat::chat_server(history = )}.} \item{\code{client}}{Optional chat client. Can be: \itemize{ \item An \link[ellmer:Chat]{ellmer::Chat} object @@ -404,16 +411,19 @@ By default, only the \code{"query"} tool is included, regardless of the Create and run a Shiny gadget for chatting with data \subsection{Usage}{ \if{html}{\out{
}} - \preformatted{QueryChat$app(..., bookmark_store = "url")} + \preformatted{QueryChat$app(..., history = NULL)} \if{html}{\out{
}} } \subsection{Arguments}{ \if{html}{\out{
}} \describe{ \item{\code{...}}{Arguments passed to \verb{$app_obj()}.} - \item{\code{bookmark_store}}{The bookmarking storage method. Passed to -\code{\link[shiny:enableBookmarking]{shiny::enableBookmarking()}}. If \code{"url"} or \code{"server"}, the chat state -(including current query) will be bookmarked. Default is \code{"url"}.} + \item{\code{history}}{Conversation history configuration for the generated app. +Defaults to \code{shinychat::history_options(restore_mode = "bookmark")} when +neither this nor \verb{$new()}'s \code{history} was set, since \verb{$app()}'s whole +purpose is a single, shareable demo. When the resolved value has +\code{restore_mode = "bookmark"}, the generated app automatically enables +Shiny's own server-side bookmarking.} } \if{html}{\out{
}} } @@ -429,15 +439,15 @@ By default, only the \code{"query"} tool is included, regardless of the A streamlined Shiny app for chatting with data \subsection{Usage}{ \if{html}{\out{
}} - \preformatted{QueryChat$app_obj(..., bookmark_store = "url")} + \preformatted{QueryChat$app_obj(..., history = NULL)} \if{html}{\out{
}} } \subsection{Arguments}{ \if{html}{\out{
}} \describe{ \item{\code{...}}{Additional arguments (currently unused).} - \item{\code{bookmark_store}}{The bookmarking storage method. Passed to -\code{\link[shiny:enableBookmarking]{shiny::enableBookmarking()}}. Default is \code{"url"}.} + \item{\code{history}}{Conversation history configuration for the generated app. +See \verb{$app()}.} } \if{html}{\out{
}} } @@ -512,7 +522,8 @@ By default, only the \code{"query"} tool is included, regardless of the \preformatted{QueryChat$server( data_source = NULL, client = NULL, - enable_bookmarking = FALSE, + history = NULL, + enable_bookmarking = NULL, ..., id = NULL, session = shiny::getDefaultReactiveDomain() @@ -525,7 +536,11 @@ By default, only the \code{"query"} tool is included, regardless of the \item{\code{data_source}}{Optional data source for backward compatibility. If provided, calls \verb{$add_table()} before initializing server logic.} \item{\code{client}}{Optional chat client override for this session.} - \item{\code{enable_bookmarking}}{Whether to enable bookmarking. Default is \code{FALSE}.} + \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 +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.} diff --git a/pkg-r/man/querychat-convenience.Rd b/pkg-r/man/querychat-convenience.Rd index 5be1320fb..4f1e6ecd6 100644 --- a/pkg-r/man/querychat-convenience.Rd +++ b/pkg-r/man/querychat-convenience.Rd @@ -11,6 +11,7 @@ querychat( ..., id = NULL, greeting = NULL, + history = NULL, client = NULL, tools = c("filter", "query"), data_description = NULL, @@ -35,7 +36,7 @@ querychat_app( prompt_template = NULL, data_dict = NULL, cleanup = NA, - bookmark_store = "url" + history = NULL ) } \arguments{ @@ -50,6 +51,9 @@ connection).} \item{greeting}{Optional initial message to display to users.} +\item{history}{Conversation history configuration for the generated app. See +\link{QueryChat}'s \verb{$app()} method.} + \item{client}{Optional chat client.} \item{tools}{Which querychat tools to include in the chat client.} @@ -67,8 +71,6 @@ values to consider as a categorical variable. Default is 20.} \item{cleanup}{Whether or not to automatically run \verb{$cleanup()} when the Shiny session/app stops.} - -\item{bookmark_store}{The bookmarking storage method. Default is \code{"url"}.} } \value{ A \code{QueryChat} object. See \link{QueryChat} for available methods. diff --git a/pkg-r/tests/testthat/_snaps/QueryChat.md b/pkg-r/tests/testthat/_snaps/QueryChat.md index 191e06779..8aaf65295 100644 --- a/pkg-r/tests/testthat/_snaps/QueryChat.md +++ b/pkg-r/tests/testthat/_snaps/QueryChat.md @@ -38,6 +38,23 @@ Error in `qc$server()`: ! `$server()` must be called within a Shiny server function +# QueryChat$server() resolves history (explicit > constructor > TRUE) and warns on enable_bookmarking + + Code + shiny::testServer(function(input, output, session) { + qc_no_history$server(enable_bookmarking = TRUE) + }, { + expect_s3_class(captured, "chat_history_config") + expect_equal(captured$restore_mode, "bookmark") + }) + Message + Using model = "gpt-4.1". + Condition + Warning: + The `enable_bookmarking` argument of `QueryChat$server()` is deprecated as of querychat 0.4.0. + i Please use the `history` argument instead. + i Use history = shinychat::history_options(restore_mode = "bookmark") for the equivalent behavior. + # normalize_data_source() / errors with invalid data source types Code diff --git a/pkg-r/tests/testthat/helper-fixtures.R b/pkg-r/tests/testthat/helper-fixtures.R index 3775b84ee..5180cf59d 100644 --- a/pkg-r/tests/testthat/helper-fixtures.R +++ b/pkg-r/tests/testthat/helper-fixtures.R @@ -175,3 +175,29 @@ mock_ellmer_chat_client <- function( MockChat$new(ellmer::Provider("test", "test", "test")) } + +# shinychat::chat_restore() validates that `client` is an ellmer::Chat() R6 +# object; the lightweight `structure(list(), class = c("MockChat", "Chat"))` +# fakes used throughout this file don't satisfy that. mod_server() calls +# chat_restore() whenever `history` isn't in bookmark mode, so tests that +# don't care about its behavior need a no-op stand-in. +local_mock_chat_restore <- function(env = parent.frame()) { + testthat::local_mocked_bindings( + chat_restore = function(...) invisible(NULL), + .package = "shinychat", + .env = env + ) +} + +# Minimal mock matching the shape shinychat::chat_server() always returns, +# including a $history interface that's present regardless of the `history` +# argument's value (registrations are just inert if history isn't active). +mock_chat_server_result <- function(client) { + list( + client = client, + history = list( + on_save = function(fn) invisible(fn), + on_restore = function(fn) invisible(fn) + ) + ) +} diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index cfa43fdad..fb266407a 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -688,6 +688,137 @@ test_that("QueryChat$server() errors when called outside Shiny context", { }) }) +test_that("QueryChat$new() validates history and stores it verbatim", { + test_df <- new_test_df() + + qc_default <- QueryChat$new(test_df, greeting = "Test") + withr::defer(qc_default$cleanup()) + expect_null(qc_default$history) + + qc_false <- QueryChat$new( + test_df, + table_name = "test_df2", + greeting = "Test", + history = FALSE + ) + withr::defer(qc_false$cleanup()) + expect_false(qc_false$history) + + expect_error( + QueryChat$new( + test_df, + table_name = "test_df3", + greeting = "Test", + history = "nope" + ), + class = "rlang_error" + ) +}) + +test_that("QueryChat$server() resolves history (explicit > constructor > TRUE) and warns on enable_bookmarking", { + skip_if_no_dataframe_engine() + withr::local_envvar(OPENAI_API_KEY = "boop") + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + captured <- NULL + local_mocked_bindings( + mod_server = function(id, ..., history) { + captured <<- history + list() + }, + .package = "querychat" + ) + + qc <- local_querychat(history = FALSE) + qc$.__enclos_env__$private$.query_executor <- executor + + expect_no_warning( + shiny::testServer( + function(input, output, session) qc$server(), + { + expect_false(captured) + } + ), + class = "lifecycle_warning_deprecated" + ) + + shiny::testServer( + function(input, output, session) qc$server(history = TRUE), + { + expect_true(captured) + } + ) + + qc_no_history <- local_querychat() + qc_no_history$.__enclos_env__$private$.query_executor <- executor + shiny::testServer( + function(input, output, session) qc_no_history$server(), + { + expect_true(captured) + } + ) + + expect_snapshot( + shiny::testServer( + function(input, output, session) { + qc_no_history$server(enable_bookmarking = TRUE) + }, + { + expect_s3_class(captured, "chat_history_config") + expect_equal(captured$restore_mode, "bookmark") + } + ) + ) + + # `history` (explicit or from the constructor) always wins over + # `enable_bookmarking`, which only fills in when neither was set. + suppressWarnings( + shiny::testServer( + function(input, output, session) { + qc_no_history$server(history = FALSE, enable_bookmarking = TRUE) + }, + { + expect_false(captured) + } + ) + ) + + suppressWarnings( + shiny::testServer( + function(input, output, session) qc$server(enable_bookmarking = TRUE), + { + expect_false(captured) + } + ) + ) +}) + +test_that("QueryChat$app_obj() infers Shiny bookmarking from history's restore_mode", { + skip_if_no_dataframe_engine() + withr::local_envvar(OPENAI_API_KEY = "boop") + + qc_default <- local_querychat() + app_default <- qc_default$app_obj() + expect_equal(app_default$appOptions$bookmarkStore, "server") + + qc_browser <- local_querychat(history = TRUE) + app_browser <- qc_browser$app_obj() + expect_equal(app_browser$appOptions$bookmarkStore, "disable") + + qc_explicit_off <- local_querychat(history = FALSE) + app_explicit_off <- qc_explicit_off$app_obj() + expect_equal(app_explicit_off$appOptions$bookmarkStore, "disable") + + qc_no_history <- local_querychat() + app_call_override <- qc_no_history$app_obj( + history = shinychat::history_options(restore_mode = "bookmark") + ) + expect_equal(app_call_override$appOptions$bookmarkStore, "server") +}) + describe("querychat()", { skip_if_no_dataframe_engine() withr::local_envvar(OPENAI_API_KEY = "boop") diff --git a/pkg-r/tests/testthat/test-querychat_module.R b/pkg-r/tests/testthat/test-querychat_module.R index 040db90f5..659feb07f 100644 --- a/pkg-r/tests/testthat/test-querychat_module.R +++ b/pkg-r/tests/testthat/test-querychat_module.R @@ -24,7 +24,15 @@ test_that("mod_server() return includes table() and table_names() for single-tab executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) structure(list(), class = "MockChat") + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + .package = "shinychat" + ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -35,15 +43,17 @@ test_that("mod_server() return includes table() and table_names() for single-tab greeting = "Hello", client = client_factory, tools = "query", - enable_bookmarking = FALSE + history = TRUE ), { - # table_names_fn() returns the table name vector - expect_equal(table_names_fn(), "test_table") + # Verify the returned list exposes table() and table_names() + expect_true(is.function(session$returned$table)) + expect_true(is.function(session$returned$table_names)) + expect_equal(session$returned$table_names(), "test_table") - # table_fn() returns a TableAccessor backed by reactive state - acc <- table_fn("test_table") - expect_true(inherits(acc, "TableAccessor")) + # table() returns a TableAccessor backed by reactive state + acc <- session$returned$table("test_table") + expect_s3_class(acc, "TableAccessor") expect_equal(acc$table_name, "test_table") # TableAccessor$df() works (returns the full data frame when no filter set) @@ -51,18 +61,11 @@ test_that("mod_server() return includes table() and table_names() for single-tab expect_equal(nrow(df_result), 5L) # Single-table backward compat: first$df/sql/title are still in the return - first_state <- tables[[1]] + first_state <- session$returned$.tables[[1]] expect_true(is.function(first_state$df)) expect_true(is.function(first_state$sql)) expect_true(is.function(first_state$title)) - # Verify the returned list exposes table() and table_names() - expect_true(is.function(session$returned$table)) - expect_true(is.function(session$returned$table_names)) - acc <- session$returned$table("test_table") - expect_s3_class(acc, "TableAccessor") - expect_equal(session$returned$table_names(), "test_table") - # Verify backward-compat reactive accessors on the returned list expect_true(is.function(session$returned$df)) expect_true(is.function(session$returned$sql)) @@ -83,9 +86,15 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl result <- NULL client_factory <- function(...) { result <<- "client_called" - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + .package = "shinychat" + ) + local_mock_chat_restore() + shiny::testServer( mod_server, args = list( @@ -95,37 +104,28 @@ test_that("mod_server() return includes table() and table_names() for multi-tabl greeting = "Hello", client = client_factory, tools = "query", - enable_bookmarking = FALSE + history = TRUE ), { - # table_names_fn() returns all registered table names - expect_equal(table_names_fn(), c("tbl_a", "tbl_b")) + # Verify the returned list exposes table() and table_names() + expect_true(is.function(session$returned$table)) + expect_true(is.function(session$returned$table_names)) + expect_equal(sort(session$returned$table_names()), c("tbl_a", "tbl_b")) - # table_fn() returns a TableAccessor for each table - acc_a <- table_fn("tbl_a") - expect_true(inherits(acc_a, "TableAccessor")) + # table() returns a TableAccessor for each table + acc_a <- session$returned$table("tbl_a") + expect_s3_class(acc_a, "TableAccessor") expect_equal(acc_a$table_name, "tbl_a") - acc_b <- table_fn("tbl_b") - expect_true(inherits(acc_b, "TableAccessor")) + acc_b <- session$returned$table("tbl_b") + expect_s3_class(acc_b, "TableAccessor") expect_equal(acc_b$table_name, "tbl_b") - # table_fn() errors for unknown names - expect_error(table_fn("nonexistent"), class = "rlang_error") + # table() errors for unknown names + expect_error(session$returned$table("nonexistent"), "not found") # Multi-table: single_table_error functions mention qc_vals$table() - single_err <- single_table_error("sql") - expect_error(single_err(), regexp = "qc_vals\\$table") - - # Verify the returned list exposes table() and table_names() - expect_true(is.function(session$returned$table)) - expect_true(is.function(session$returned$table_names)) - acc <- session$returned$table("tbl_a") - expect_s3_class(acc, "TableAccessor") - expect_equal(sort(session$returned$table_names()), c("tbl_a", "tbl_b")) - - # Verify error is surfaced through the public API - expect_error(session$returned$table("nonexistent"), "not found") + expect_error(session$returned$sql(), regexp = "qc_vals\\$table") } ) }) @@ -140,9 +140,15 @@ test_that("mod_server() passes visualize callback and tools to client factory", client_factory <- function(...) { captured <<- list(...) - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + .package = "shinychat" + ) + local_mock_chat_restore() + shiny::testServer( mod_server, args = list( @@ -152,7 +158,7 @@ test_that("mod_server() passes visualize callback and tools to client factory", greeting = "Hello", client = client_factory, tools = c("query", "visualize"), - enable_bookmarking = FALSE + history = TRUE ), { expect_type(captured, "list") @@ -171,7 +177,15 @@ test_that("mod_server() exposes current_table() starting as NULL", { executor <- build_query_executor(list(test_table = ds)) withr::defer(executor$cleanup()) - client_factory <- function(...) structure(list(), class = "MockChat") + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + .package = "shinychat" + ) + local_mock_chat_restore() shiny::testServer( mod_server, @@ -182,7 +196,7 @@ test_that("mod_server() exposes current_table() starting as NULL", { greeting = "Hello", client = client_factory, tools = "query", - enable_bookmarking = FALSE + history = TRUE ), { expect_true(is.function(session$returned$current_table)) @@ -203,9 +217,15 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu captured_callbacks <- NULL client_factory <- function(...) { captured_callbacks <<- list(...) - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + .package = "shinychat" + ) + local_mock_chat_restore() + shiny::testServer( mod_server, args = list( @@ -215,7 +235,7 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu greeting = "Hello", client = client_factory, tools = "query", - enable_bookmarking = FALSE + history = TRUE ), { # Initially NULL @@ -238,23 +258,23 @@ test_that("mod_server() current_table() updates on update_dashboard and reset_qu ) }) -test_that("mod_ui() passes allow_attachments = TRUE to shinychat by default", { +test_that("mod_ui() does not force allow_attachments, deferring to chat_ui default", { captured <- NULL local_mocked_bindings( - chat_ui = function(...) { + chat_ui = function(id, ...) { captured <<- list(...) htmltools::div() }, .package = "shinychat" ) mod_ui("test") - expect_true(isTRUE(captured$allow_attachments)) + expect_false("allow_attachments" %in% names(captured)) }) test_that("mod_ui() passes allow_attachments = FALSE when requested", { captured <- NULL local_mocked_bindings( - chat_ui = function(...) { + chat_ui = function(id, ...) { captured <<- list(...) htmltools::div() }, @@ -264,6 +284,32 @@ test_that("mod_ui() passes allow_attachments = FALSE when requested", { expect_false(isTRUE(captured$allow_attachments)) }) +test_that("mod_ui() calls chat_ui with NS(id, 'chat')", { + captured <- NULL + local_mocked_bindings( + chat_ui = function(id, ...) { + captured <<- list(id = id, ...) + htmltools::div() + }, + .package = "shinychat" + ) + mod_ui("mymod") + expect_equal(captured$id, shiny::NS("mymod", "chat")) +}) + +test_that("mod_ui() passes enable_cancel through to chat_ui without warning", { + captured <- NULL + local_mocked_bindings( + chat_ui = function(id, ...) { + captured <<- list(...) + htmltools::div() + }, + .package = "shinychat" + ) + expect_no_warning(mod_ui("test", enable_cancel = FALSE)) + expect_false(isTRUE(captured$enable_cancel)) +}) + test_that("restored viz widgets survive a second bookmark cycle", { skip_if_no_dataframe_engine() @@ -277,18 +323,19 @@ test_that("restored viz widgets survive a second bookmark cycle", { client_factory <- function(...) { callbacks <<- list(...) - structure(list(), class = "MockChat") + structure(list(), class = c("MockChat", "Chat")) } local_mocked_bindings( - chat_restore = function(id, chat, ..., session) {}, + chat_server = function(id, client, ...) mock_chat_server_result(client), .package = "shinychat" ) + local_mock_chat_restore() local_mocked_bindings( - onBookmark = function(fun) { + onBookmark = function(fun, session = NULL) { bookmark_fn <<- fun }, - onRestore = function(fun) { + onRestore = function(fun, session = NULL) { restore_fn <<- fun }, .package = "shiny" @@ -314,7 +361,7 @@ test_that("restored viz widgets survive a second bookmark cycle", { greeting = "Hello", client = client_factory, tools = c("query", "visualize"), - enable_bookmarking = TRUE + history = TRUE ), { expect_true(is.function(bookmark_fn)) @@ -348,3 +395,363 @@ test_that("restored viz widgets survive a second bookmark cycle", { } ) }) + +test_that("mod_server() calls chat_server('chat', ...) with the pre-built client", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + captured_chat_args <- NULL + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + local_mocked_bindings( + chat_server = function(id, client, ...) { + captured_chat_args <<- list(id = id, client = client) + mock_chat_server_result(client) + }, + .package = "shinychat" + ) + local_mock_chat_restore() + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = TRUE + ), + { + expect_equal(captured_chat_args$id, "chat") + expect_true(inherits(captured_chat_args$client, "Chat")) + } + ) +}) + +test_that("mod_server() calls chat_restore() with the auto-bookmark trigger disabled when history isn't bookmark mode", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + captured_restore_args <- NULL + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + chat_restore = function(id, client, ...) { + captured_restore_args <<- list(id = id, client = client, ...) + invisible(NULL) + }, + .package = "shinychat" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = TRUE + ), + { + expect_equal(captured_restore_args$id, "chat") + expect_true(inherits(captured_restore_args$client, "Chat")) + expect_false(captured_restore_args$restore_ui) + # The hooks are registered, but querychat never triggers a bookmark + # itself -- that's left to `history` or the host app. + expect_false(captured_restore_args$bookmark_on_input) + expect_false(captured_restore_args$bookmark_on_response) + } + ) +}) + +test_that("mod_server() skips chat_restore() when history is bookmark mode", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + chat_restore_called <- FALSE + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + chat_restore = function(id, client, ...) { + chat_restore_called <<- TRUE + invisible(NULL) + }, + .package = "shinychat" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = shinychat::history_options(restore_mode = "bookmark") + ), + { + expect_false(chat_restore_called) + } + ) +}) + +test_that("mod_server() builds the auto-generated greeting from the greeter, not the main client", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + main_client_calls <- list() + client_factory <- function(...) { + main_client_calls[[length(main_client_calls) + 1L]] <<- list(...) + structure(list(), class = c("MockChat", "Chat")) + } + + greeting_stream_prompt <- NULL + fake_greeting_client <- list( + stream_async = function(prompt) { + greeting_stream_prompt <<- prompt + "fake-stream" + } + ) + + build_client_calls <- list() + fake_greeter <- list( + build_client = function(base = NULL) { + build_client_calls[[length(build_client_calls) + 1L]] <<- base + fake_greeting_client + } + ) + + captured_greeting_arg <- NULL + local_mocked_bindings( + chat_server = function(id, client, greeting = NULL, ...) { + captured_greeting_arg <<- greeting + mock_chat_server_result(client) + }, + chat_greeting = function(content, ...) content, + .package = "shinychat" + ) + local_mock_chat_restore() + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = NULL, + client = client_factory, + tools = "query", + greeter = fake_greeter, + greeting_base = "base-client", + history = TRUE + ), + { + expect_true(is.function(captured_greeting_arg)) + suppressWarnings(captured_greeting_arg()) + + # The greeting client must come from the greeter (configured with the + # dedicated greeting prompt), not from a second call to the main client + # factory with tools = NULL (which would use the query system prompt). + expect_equal(length(build_client_calls), 1L) + expect_equal(build_client_calls[[1]], "base-client") + expect_equal(length(main_client_calls), 1L) + expect_false(is.null(greeting_stream_prompt)) + } + ) +}) + +test_that("mod_server() chat_update input updates table state", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + local_mocked_bindings( + chat_server = function(id, client, ...) mock_chat_server_result(client), + .package = "shinychat" + ) + local_mock_chat_restore() + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = TRUE + ), + { + session$setInputs( + chat_update = list( + table = "test_table", + query = "SELECT * FROM test_table WHERE cyl = 4", + title = "4-cylinder cars" + ) + ) + expect_equal( + shiny::isolate(session$returned$sql()), + "SELECT * FROM test_table WHERE cyl = 4" + ) + expect_equal( + shiny::isolate(session$returned$current_table()), + "test_table" + ) + } + ) +}) + +test_that("mod_server() registers table/viz state with both bookmark and history hooks, unconditionally", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + bookmark_save_count <- 0 + bookmark_restore_count <- 0 + history_save_fn <- NULL + history_restore_fn <- NULL + + local_mocked_bindings( + chat_server = function(id, client, ...) { + list( + client = client, + history = list( + on_save = function(fn) { + history_save_fn <<- fn + invisible(fn) + }, + on_restore = function(fn) { + history_restore_fn <<- fn + invisible(fn) + } + ) + ) + }, + .package = "shinychat" + ) + local_mock_chat_restore() + local_mocked_bindings( + onBookmark = function(fun, session = NULL) { + bookmark_save_count <<- bookmark_save_count + 1 + }, + onRestore = function(fun, session = NULL) { + bookmark_restore_count <<- bookmark_restore_count + 1 + }, + .package = "shiny" + ) + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = FALSE # even with history disabled, registration must still happen + ), + { + expect_equal(bookmark_save_count, 1L) + expect_equal(bookmark_restore_count, 1L) + expect_true(is.function(history_save_fn)) + expect_true(is.function(history_restore_fn)) + } + ) +}) + +test_that("history on_save callback returns merged values (R history contract)", { + skip_if_no_dataframe_engine() + + ds <- local_data_frame_source(new_test_df()) + executor <- build_query_executor(list(test_table = ds)) + withr::defer(executor$cleanup()) + + client_factory <- function(...) { + structure(list(), class = c("MockChat", "Chat")) + } + + history_save_fn <- NULL + local_mocked_bindings( + chat_server = function(id, client, ...) { + list( + client = client, + history = list( + on_save = function(fn) { + history_save_fn <<- fn + invisible(fn) + }, + on_restore = function(fn) invisible(fn) + ) + ) + }, + .package = "shinychat" + ) + local_mock_chat_restore() + + shiny::testServer( + mod_server, + args = list( + id = "test", + data_sources = list(test_table = ds), + executor = executor, + greeting = "Hello", + client = client_factory, + tools = "query", + history = TRUE + ), + { + session$setInputs( + chat_update = list( + table = "test_table", + query = "SELECT * FROM test_table WHERE id = 1", + title = "One row" + ) + ) + result <- shiny::isolate(history_save_fn(list(unrelated_key = "kept"))) + expect_equal(result$unrelated_key, "kept") + expect_equal( + result$querychat_tables$test_table$sql, + "SELECT * FROM test_table WHERE id = 1" + ) + } + ) +}) diff --git a/pyproject.toml b/pyproject.toml index bec6902f5..1b38fd785 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ maintainers = [ dependencies = [ "duckdb", "shiny>=1.6.2", - "shinychat @ git+https://github.com/posit-dev/shinychat.git", + "shinychat>=0.6.0", "htmltools", "chatlas>=0.18.0", "narwhals>=2.2.0",