Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1a83019 to
110b6af
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…overage step Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d543bae to
7e74c52
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7e74c52 to
c9a45e1
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the overloaded { id, handler, obj } wire format with
{ id, action, html_deps? } where action is a discriminated ChatAction
union. Server and client now speak the same vocabulary.
JS: delete legacyToActions shim, LegacyEnvelope/LegacyMessageObj/
LegacyUpdateInputObj types, and the separate shiny-tool-request-hide
handler. Transport accepts ShinyChatEnvelope directly.
Python: replace _send_custom_message with _send_action, delete
ClientMessage TypedDict, fold tool-request-hide into main channel.
R: add send_chat_action helper, rewrite chat_append_message/chat_clear/
update_chat_user_input to construct action lists directly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass `disabled={disabled}` to the textarea element so the input is
actually disabled during response generation, and so the
`.shiny-busy:has(shiny-chat-input textarea:disabled)` CSS rule works.
Also fix misleading comment in setInputValue's submit path — the
disabled guard is intentionally kept, we only skip sendInput() to
avoid its focus/clear side-effects.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix prettier formatting in serverPayloads.test.tsx and rebuild JS dist to include the ChatInput disabled prop change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… cleanup - Python: move `from itertools import groupby` to module level in _html_islands.py - Python: add strongly typed ChatAction/MessagePayload/ShinyChatEnvelope TypedDicts mirroring js/src/transport/types.ts, and use them in _chat.py - R: fix bug where initial messages in chat_ui() skipped split_html_islands() - R: add pre_process_ui() helper to DRY the split-then-process pattern - R: add snapshot test for initial messages with react elements Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ToolResultBridge was missing the `icon` attribute from its props interface, causing user-specified tool icons (e.g., folder icon from bsicons) to be silently dropped. The ToolCard then fell back to the default wrench icon. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ehavior remark (CommonMark) treats non-indented text after a list item without a blank line as a "lazy continuation line" inside the last <li>. The old marked parser treated it as a new paragraph. This caused suggestion lists followed by text like "Let me know which option!" to render inline with the last suggestion instead of on a new line. New rehype plugin rehypeLazyContinuation detects trailing text nodes in the last <li> and promotes them to a <p> sibling after the list. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f142c8c to
7ad93b0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
I think we should do this todo item, either in this PR or in a follow-up (in short, we should widen the |
|
I think we'll definitely end up wanting to revisit the design of server → client messages too, but that's certainly a follow-up PR. But I do want to call out that the design here — with types and |
…mmatic scroll guard Replace fragile direction-based scroll detection with position-only detection plus a programmatic scroll guard. The old approach misinterpreted browser scrollTop clamping (during markdown re-parsing height fluctuations) as user scroll-up, permanently disengaging auto-scroll during streaming. Key changes: - Remove prevScrollTopRef and direction-based logic from useAutoScroll - Add isProgrammaticScrollRef guard around scrollTo calls, cleared via rAF - Optimize findScrollableParent in MarkdownStream to skip repeated DOM walks - Remove conflicting smooth-scroll useEffect from MarkdownStream - Re-engage stickToBottom in ChatContainer when non-streaming messages arrive - Add CI path filter for js/** in py-check workflow - Update and expand test coverage (37 useAutoScroll tests, updated integration tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b3bcf74 to
90bb335
Compare
Nice catch, fixed in 389b65d. Interestingly, this was only broken for R, since R goes through the "streaming" message path when restoring.
When I wrote that comment a while back I was thinking that it would make the most sense to do this when we add image support. |
The chunk action handler in the JS chat reducer had an overly restrictive guard (`last.role !== "assistant"`) that silently dropped chunk content for non-assistant roles. This caused user messages sent via the streaming path (chunk_start/chunk/chunk_end) to appear with empty content. This broke bookmark restoration in R, where `client_set_ui()` replays all messages (including user messages) through `chat_append()`, which always uses the streaming path. The role guard was introduced during the React migration but did not exist in the original Lit implementation. The `!last.streaming` check is sufficient protection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
90bb335 to
389b65d
Compare
Yeah I know, I'm saying that now that we're rewriting this in React we should do this (as in, commit to doing it as a follow up in the very near future). |
…eaming (#184) * perf: separate streaming message from committed messages to prevent re-renders Pass `state.messages` and `state.streamingMessage` as separate props instead of combining them via `allMessages()`. This keeps the committed messages array reference stable during streaming, so `memo` on `ChatMessages` and individual `ChatMessage` components prevents unnecessary re-renders of old messages when streaming chunks arrive. Removes the now-unused `allMessages()` helper and its tests. Also adds a `js-build-dev` Makefile target and `--dev` flag in build.ts for React Profiler support during performance investigation. * chore: build and update assets
- Change displayContents default from false to true
- Run fill carrier check unconditionally instead of gating on displayContents
- Explicitly set displayContents={false} on ToolCard footer
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…h programmatic scroll guard" This reverts commit 74f7439.
…DataBot approach) Switch from useLayoutEffect with instant scrolling to useEffect with smooth scrolling, matching the DataBot auto-scroll implementation. This avoids scrollTop clamping issues during markdown re-parses without the fragility of the position-based programmatic-scroll guard approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…yContents default - useAutoScroll tests: expect "smooth" instead of "instant" during streaming - Integration test: non-streaming messages re-engage stickToBottom (by design) - RawHTML test: match new displayContents=true default - Playwright test: poll for scroll position to accommodate smooth scrolling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
The React migration (commit 2fd7134) changed the ToolCard title from dangerouslySetInnerHTML to React text interpolation, which escapes HTML. This broke titles like `HTML("Map of <i>Paris</i>")` from R. Restore the original Lit behavior of rendering the title as raw HTML, since titles are always developer-controlled (from display$title or annotations$title), not LLM-generated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
65a6b86 to
6fd1dd1
Compare
Incorporates 11 upstream commits including: - Migrate chat UI from Lit to React (posit-dev#181) - Tool result card improvements (fullscreen, footer, collapse/expand) - google-genai SDK migration for Python - Various fixes Preserves dowshinychat package naming throughout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Rewrites the front-end from Lit custom elements to React. User-facing behavior is unchanged — chat streaming, tool cards, fullscreen mode, external link dialogs all work the same. The R and Python packages require only minor changes (the two JS/CSS bundles consolidate into one).
Under the hood, this replaces ~2,100 lines of imperative Lit code with ~3,000 lines of React components plus ~4,900 lines of new JS tests (the old codebase had zero).
What's good about this migration
Centralized state. In the old code, "what is the current state of the chat?" required inspecting Lit
@property()decorators across multiple class instances, direct DOM reads, and a globalwindow.shinychat.hiddenToolRequestsSet. Now it's oneuseReducerwith a typedChatStateand discriminated union of actions. Every state transition is visible in one place and testable in isolation.No more DOMPurify gymnastics. The old
_utils.tshad a complex DOMPurify setup with lifecycle hooks, aWeakMapto preserve custom element attributes, and special allowlists for htmlwidget scripts. The new markdown pipeline produces React elements directly from a HAST (HTML Abstract Syntax Tree), so script tags and event handlers are inert by construction. Embedded Shiny UI (htmlwidgets, inputs) still usesinnerHTMLviaRawHTML, but the markdown rendering path itself never touchesinnerHTML.Incremental DOM updates during streaming. In the old code, every streaming chunk replaced the entire message DOM via
innerHTML— destroying and rebuilding all code blocks, syntax highlighting, and embedded widgets on every ~50ms chunk arrival. The new code re-parses the markdown into a HAST on each chunk, but React's reconciler diffs the resulting element tree against the previous one and only commits the changed DOM nodes. Existing content within the streaming message (highlighted code, widgets, etc.) is left in place rather than torn down and recreated.Testable. The transport abstraction, the pure reducer, and composable rehype plugins are all independently testable. 24 test files cover the state machine, all hooks, all plugins, bridge components, and integration flows. Regression tests are anchored to specific bugs.
Build simplification. Two separate entry points with separate CSS bundles consolidate into one
shinychat.js+shinychat.css.Before/After Screenshots
Captured from the
tool-basicR test app with Playwright.Initial state
Streaming with tool requests
Completed response
Mental model for the reviewer
The boundary: where Shiny ends and React begins
The Shiny server still renders
<shiny-chat-container>and<shiny-markdown-stream>custom elements in the initial HTML, just like before. What's different is that these custom elements are now thin shells.chat-entry.tsreads attributes from the server-rendered DOM, callscreateRoot(this), and hands off to<ChatApp>. From that point inward, everything is React. The same pattern applies tomarkdown-stream-entry.ts.A critical detail at this boundary:
chat-entry.tscallstransport.unbindAll(this)before mounting the React root. Without this, Shiny's internal binding registry retains stale references from the server-rendered HTML and refuses to re-bind the new React-rendered elements (Shiny thinks the inputs are already bound by ID). This is the kind of Shiny-specific integration concern that's now isolated in the entry files rather than scattered through the component tree.If you're reviewing R/Python package changes, you mostly only need to care about how attributes are set on the custom elements. If you're reviewing JS, you're in React-land.
State lives in the reducer, imperative stuff lives outside it
state.tsis the single source of truth for message data, input disabled state, and hidden tool requests. Every server action (message,chunk_start,chunk,chunk_end,clear,update_input,remove_loading,hide_tool_request) and one UI action (INPUT_SENT) flows throughchatReducer. The reducer is pure — given a state and an action, it returns the next state. No side effects.However, not everything goes through the reducer. The textarea is intentionally uncontrolled — its value and focus are managed imperatively via a
useImperativeHandleref onChatInput. This avoids the controlled-input cursor-jump problem and the cost of dispatching on every keystroke during streaming. When the server sends anupdate_inputaction withvalueorfocus,ChatAppforwards it to the imperative handle rather than the reducer. The reducer only tracksinputPlaceholderandinputDisabled.The transport layer bridges Shiny and React
transport/types.tsdefines two interfaces:ChatTransport— message passing:sendInput(id, value)andonMessage(id, callback). React components use this to send user input and subscribe to server actions.ShinyLifecycle— Shiny DOM plumbing:renderDependencies,bindAll,unbindAll,showClientMessage. These are the Shiny-specific operations that React components need but shouldn't call directly.ShinyTransportimplements both. It's a window-level singleton so that the chat entry and markdown-stream entry share one instance and one message handler registration.The Python/R backends send
{ id, action, html_deps? }envelopes whereactionis a discriminatedChatActionunion (e.g.,{ type: "chunk_start", message: { role: "assistant", content: "", content_type: "markdown" } }). The transport renders anyhtml_depsbefore dispatching, then passes theactiondirectly to the reducer — no translation layer needed.ShinyTransportalso has a pending-message queue: if ashinyChatMessagearrives before any listener has registered for that ID, the action is buffered and flushed on the firstonMessage()call. This covers race conditions during page load.The markdown pipeline: text -> AST -> React elements
The old pipeline was
marked-> DOMPurify ->innerHTML. The new pipeline is:Three frozen processors are defined in
processors.ts:markdownProcessor: full pipeline with rehype plugins for external link annotation, code highlighting, block-level CE unwrapping, uncontrolled inputs, and accessible suggestions. NorehypeSanitize— becausetoJsxRuntimeproduces React elements (not HTML strings), script tags and event handlers are inert by construction.htmlProcessor: for raw HTML content — preserves HTML fragment structure while normalizing uncontrolled form inputs, external link attributes, and accessible suggestions.semiMarkdownProcessor: includesremarkEscapeHtml(HTML tags become literal text) andrehypeSanitizefor defense-in-depth.Each rehype plugin is independent and tested in isolation. The key plugins to be aware of:
rehypeUnwrapBlockCEs— promotes block-level custom elements (like<shinychat-raw-html>) out of<p>tags where the markdown parser incorrectly nests them.rehypeUncontrolledInputs— rewritesvaluetodefaultValueon<input>/<textarea>elements so React doesn't treat them as controlled inputs (which would conflict with Shiny's input bindings).rehypeAccessibleSuggestions— wraps suggestion elements with appropriate ARIA attributes for accessibility.rehypeExternalLinks— annotates external links for the external link confirmation dialog.Two-stage rendering in
markdownToReact.ts:parseMarkdown()runs the full unified pipeline to produce a HAST, orparseHtml()parses a raw HTML fragment. URL sanitization is applied in both paths. Cached viauseMemokeyed on content + processor.hastToReact()callstoJsxRuntimeon the HAST to produce React elements. The streaming dot is injected here via immutable path-copy (O(depth), not O(tree size)) — the cached HAST is never mutated. This stage only re-runs whenstreamingtoggles.Custom elements with dashes in their tag names get special treatment:
fixCustomElementPropsinmarkdownToReact.tsconverts React-ified property names (e.g.,className,htmlFor) back to their HTML attribute equivalents, because React 19 sets properties (not attributes) on custom elements and properties likehtmlFordon't map to anything on a generic HTMLElement.RawHTML: server-side splitting for Shiny UI content
LLM responses can mix markdown text with embedded Shiny UI (htmlwidgets, inputs, tool cards). React's DOM model is incompatible with Shiny UI that relies on inline scripts, jQuery initialization, and
Shiny.bindAll(). The solution is server-side splitting: the server separates React-managed elements from traditional Shiny HTML before sending content to the client.How it works:
split_html_islands(). This function walks top-level tag children and checks for thedata-shinychat-reactattribute.tagToComponentMap.<shinychat-raw-html>wrapper tags — these become innerHTML islands on the client.The client-side
RawHTMLcomponent setsinnerHTMLon a wrapper div and manages Shiny binding lifecycle. WhenShinyLifecycleContextis available, it automatically callsbindAllafter setting innerHTML andunbindAllon cleanup. Thedisplay: contentsCSS property is used to avoid layout-breaking wrapper divs.In the HAST → React pipeline,
<shinychat-raw-html>elements are handled byMarkdownContent.tsx, which serializes HAST children back to an HTML string viatoHtml()and passes the result to<RawHTML html={...} displayContents />.Adding a new React custom element only requires the server to add
data-shinychat-reactto the tag. The server-side splitting ensures it bypasses the innerHTML path and flows through React's normal rendering. Internally, shinychat maps tool tags to specific React components viachatTagToComponentMap(e.g.,shiny-tool-request→ToolRequestBridge), but this is package-internal — there is not yet a public API for end users to register custom component mappings.Bridge components: HTML attributes -> typed React props
When
toJsxRuntimeencounters a<shiny-tool-request request-id="req-1" tool-name="search">in the HAST, it maps it to theToolRequestBridgeReact component (via thecomponentsoption). But it passes the attributes as raw strings with their original kebab-case names.Bridge components (
ToolRequestBridge,ToolResultBridge) translate these stringly-typed HTML attributes into properly-typed React props:This keeps the real components (
ToolRequest,ToolResult) clean and testable with typed props. The bridge layer is the only place that knows about the HTML-to-React attribute translation.What changed in R/Python
shinychat.js+shinychat.css)split_html_islands()before serialization:data-shinychat-react(tool cards) are emitted bare for React rendering<shinychat-raw-html>tags for innerHTML rendering{=html}fence convention for raw HTML blocks is replaced by<shinychat-raw-html>tags<shiny-tool-request>) gets adata-shinychat-reactattribute so the server knows to emit it outside of<shinychat-raw-html>wrappersTest plan
cd js && npm run lintcd js && npx vitest run(24 test files, ~208 tests)uv run python -m pytest pkg-py/tests/(~56 tests)cd pkg-r && Rscript -e "devtools::check(document = FALSE)"🤖 Generated with Claude Code