Migrate to shinychat's high-level chat API#267
Conversation
Replaces the hand-rolled ExtendedTask/cancel/bookmark wiring in both packages with shinychat's high-level chat_ui()/chat_server() (R) and client= (Python) entry points, so querychat no longer duplicates logic that shinychat now owns directly. - R: mod_ui/mod_server now delegate to chat_mod_ui()/chat_mod_server() instead of manually wiring ExtendedTask, cancel, and restore; greeting moves to the server side. - Python: mod_ui/mod_server pass client= to shinychat.Chat and drop the manual submit/cancel/bookmark wiring; generate_stream is replaced by generate_async, and the greeter is wired into _make_greeting. - Adds a minimum version requirement for the migration and removes argument-default overrides that are no longer needed. - Incorporates code review fixes: restored moduleServer isolation and original formatting in querychat_module.R, corrected greeting logic, and removed over-explaining comments left over from the migration.
…ry param Both packages previously configured chat history persistence through two separate, overlapping knobs (enable_bookmarking and bookmark_store). This collapses them into a single history parameter (bool or HistoryOptions) that mod_server resolves once and uses to register both table and viz state for bookmarking/restore. - Python: QueryChatBase stores history verbatim; mod_server resolves it and dual-registers table/viz state; enable_bookmarking/bookmark_store are deprecated in favor of history, with changelog entries and updated playwright fixtures/tests. - R: mod_server takes the same history param and dual-registers table/viz state; enable_bookmarking/bookmark_store are deprecated the same way, with NEWS entries and test coverage for history resolution, the deprecation path, and app() inference. Dead generate_stream/ utils-shinychat.R code is removed. - Always calls Chat.enable_bookmarking() / registers chat_restore() hooks rather than conditionally, so history restore no longer depends on a legacy shim. - Pins shinychat to the feat/chat-history branch, which provides the history-aware bookmarking hooks this work depends on. - Follow-up fix: use Optional[...] instead of `| None` for the history parameter type, matching this file's existing typing convention.
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
apply_state_snapshot() indexed tables[[name]] for every key in the restored payload, so a bookmark or history snapshot referencing a table no longer registered on the current server would error and abort restore entirely. Iterate over the currently registered tables instead, matching the Python implementation.
- test_toggle_hides_section_again clicked twice back-to-back while the chart widget below the button was still resizing (its responsive fill layout takes a couple seconds, and settles again after the query section expands), so the second click could land on stale coordinates. Wait for the button's position to stabilize before interacting. - The QueryChat.R snapshot for `enable_bookmarking` deprecation still had the pre-migration deparse of the mocked testServer() call; R CMD check consistently renders it with explicit braces now.
gadenbuie
left a comment
gadenbuie
left a comment
There was a problem hiding this comment.
Two quick things jump out at me from the reading the PR description:
-
Chat greetings now use shinychat's greeting API — schema-aware, generated on demand, preserved across bookmark/restore.
I thought we had this already with #261, so I'm guessing this is a bit of Claude missing the nuance in what happened here in the PR. Would you mind clarifying what specific changes we made around greetings?
-
There's a bunch of bookmarking changes in #266 that also touch
enable_bookmarkingandbookmark_storeand are going to be equally complicated by the new history feature in shinychat. That region of the API is sufficiently complex that I think it's worth us talking through the API design in person to make sure we're coordinating between bookmarking and history in the best way possible.The core proposal in #266 was to deprecate
enable_bookmarkingin favor ofbookmark_enableandbookmark_store. That's both a prefix hygiene change and a functional change: with the additional cards,bookmark_enablewould accept bothTRUEor specific content types that should be included in the bookmark (e.g."cards"or"chat").I'm very open to other options for the API, but if there's a chance we'll take #266 (which I do think we should), I think it's worth our time to make sure we're including the cards feature in the bookmarking arguments. The core request there is for it to be possible to restore the cards independently of or together with the chat history.
These changes don't undo the work of #261 -- they just lean into the fact that shinychat can manage the event handling, restoration, etc.
I agree, but I'm hoping we can not let that block a Python release, which I would love to have for SciPy (this Monday). If it helps, this PR does the following:
I anticipate if we're going to disagree on anything in the public API surface, it will be in the realm of whether
|
gadenbuie
left a comment
gadenbuie
left a comment
There was a problem hiding this comment.
The migration looks great and I agree, we don't need to block a Python release on fully figuring out the bookmarking/history API issues I called out. In fact, I'm confident that we can ship this and then I can work out how to best roll the dynamic cards feature into either history or bookmarking.
I ran the PR by Claude for a review, which flagged a few minor things below. Feel free to resolve and merge!
| S7, | ||
| shiny, | ||
| shinychat (>= 0.4.0), | ||
| shinychat (> 0.4.0), |
There was a problem hiding this comment.
Minor housekeeping (not a blocker): > 0.4.0 does not guarantee the API this PR calls. A dev build (0.4.0.9000) satisfies > 0.4.0 yet lacks chat_server() / history_options(). Enough has moved upstream in dev shinychat that I think we should bump the dev-version pin here just to be explicit, and leave a NEWS/CHANGELOG note that this floor will need to move to the released shinychat version once that release lands.
There was a problem hiding this comment.
Looked into pinning Remotes: to a commit, but tested remotes's actual resolution logic first (remotes::dev_package_deps()): it diffs the installed package's SHA against the live SHA of whatever ref Remotes: points to, not the Version: string, and defaults to auto-upgrading on non-interactive installs (interactive sessions get prompted with that diff). So a stale local shinychat gets caught and updated on a normal install_deps()/pak run regardless of whether we pin — pinning would mostly add manual upkeep (remembering to bump the SHA) without closing a real gap. Left DESCRIPTION as it was.
There was a problem hiding this comment.
Oh the suggestion here wasn't about pinning to SHA but instead about bumping the dev version of shinychat on the shinychat side, e.g. 0.4.0.9001, as a signal that we've introduced a coordinated API change within the dev cycles of both packages.
…istory QueryChat$server(enable_bookmarking = TRUE) warned but the value was then discarded -- resolved_history was computed from history %||% self$history %||% TRUE, so it silently degraded to history = TRUE (localStorage) instead of shareable bookmark state. Map enable_bookmarking = TRUE to history_options(restore_mode = "bookmark") when neither this call's nor the constructor's history was set, preserving the old bookmark-based behavior through the deprecation window.
…history
Same issue as the R side: .server(enable_bookmarking=True) and
QueryChatExpress(enable_bookmarking=True) warned but the value was then
discarded, silently resolving to history=True instead of bookmark-based
restore.
Map enable_bookmarking=True to HistoryOptions(restore_mode="bookmark")
when history wasn't otherwise set, on both QueryChat.server() and
QueryChatExpress's lazily-started server. Also aligns the two deprecation
warning messages, which previously said different things ("for the
equivalent behavior" on the R side vs. "no longer has any effect" here).
chat_server() registers chat_enable_history() internally whenever `history` isn't FALSE, and shinychat's own docs call chat_enable_history() and chat_restore() mutually exclusive -- both independently try to restore the client's turns from a Shiny bookmark. Registering chat_restore() too, as querychat did unconditionally, meant doubled per-turn storage on every save and a visible flicker on restore whenever history's restore_mode was "bookmark" (chat_restore's onRestore fires first from the raw bookmark payload, then chat_enable_history's own restore clears and re-renders from its own conversation-store copy). Skip chat_restore() in that case; it's still registered otherwise, so the chat client's own state continues to round-trip through Shiny bookmarks the host app might trigger for unrelated reasons.
…rk mode Mirrors the R fix: shinychat.Chat(history=) already enables self.history (the Python equivalent of chat_enable_history()) whenever history isn't False, and shinychat treats it and enable_bookmarking() as mutually exclusive -- both independently try to restore the client's turns from a Shiny bookmark. Registering enable_bookmarking() too, as querychat did unconditionally, meant doubled per-turn storage on every save and a visible flicker on restore whenever history's restore_mode was "bookmark". Skip enable_bookmarking() in that case; it's still registered otherwise, so the chat client's own state continues to round-trip through Shiny bookmarks the host app might trigger for unrelated reasons.
…at usage Every existing mod_server() test 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 -- only the playwright e2e tests and the manual bookmark/restore test-plan step would catch it. Add one thin test that constructs the real shinychat.Chat and calls the real methods with the same arguments mod_server() uses, so a signature drift in that surface fails loudly here too.
…te jitter _wait_for_stable_position() silently returned once max_wait_s elapsed even if the locator's bounding box never settled, so a genuine layout regression could masquerade as "settled" and pass. It also compared bounding boxes for exact float equality, which is jitter-sensitive. Raise a clear pytest.fail() with the last observed bounding box on deadline exhaustion, and compare boxes within a small pixel tolerance instead of exact equality. (playwright-python 1.58 doesn't expose expect.poll() for a custom condition like this, so the hand-rolled polling loop stays, just made stricter.)
The existing NEWS/CHANGELOG entry described the bookmark_store removal as a
behavior-preserving rename ("existing callers keep working without
changes"), which is true functionally but glosses over a real deployment
difference: the old default (bookmark_store="url") encoded the whole
bookmark state in the URL with no server storage, while the new default
requires server-side bookmark storage. Spell that out so anyone relying on
the old stateless behavior isn't surprised.
QueryChat now defaults to history=True (shinychat's browser-localStorage history) instead of no persistence, so this example doesn't need to opt into Shiny-level URL bookmarking on top of that. Also drops the now-unused app_opts import.
|
E2E test failures were introduced by a new pyarrow 25.0.0 release. Will fix that separately |
shinychat's high-level chat API is now available in both R and Python, and shinychat is investing further in it — file attachments, cancellation, conversation history, and editable messages, etc. are all moving to live there rather than being something each consuming package reimplements. Migrating querychat onto that API now means we drop our own hand-rolled
ExtendedTask/cancel/restore/bookmark wiring and instead get these features (and future ones) for free as shinychat adds them, in both languages at once.The most visible outcome of this migration is history/bookmarking config: instead of two overlapping knobs (
enable_bookmarkingandbookmark_store), there's now a singlehistoryparameter that covers both in-memory chat history and Shiny bookmarking, mirroring shinychat's own API.What changes for users
enable_bookmarkingis deprecated in favor ofhistory(e.g.history = shinychat::history_options(restore_mode = "bookmark")/history=shinychat.types.HistoryOptions(restore_mode="bookmark")).bookmark_storeis removed from$app()/.app(); passhistoryinstead.QueryChatnow defaults tohistory = TRUE/history=True(shinychat's own default, browser-localStorage-backed history) instead of no persistence.See the updated
NEWS.md/CHANGELOG.mdentries for full detail.Test plan
make r-checkmake py-check