Add btw-discoverable shiny-for-r agent skill - #4427
Merged
Merged
Conversation
Fix four small inaccuracies found in final review of the shiny-for-r skill: trim the session-lifecycle router entry to what the reference actually covers and align it on onSessionEnded(); introduce bsicons::bs_icon() on first use in dashboard-components.md; clarify that showNotification()/showModal()/Progress$new() need an active session rather than an observer, in feedback.md; and fix a partial snippet marker in navigation.md that omitted the server code it also contains.
…b.com/rstudio/shiny into schloerke/port-py-shiny-skills-to-btw
observeEvent()'s handlerExpr already runs inside an isolate() scope, so isolate(input$seed) in that example did nothing. Show the pattern in observe() instead, and call out that observeEvent()/eventReactive()/ bindEvent() bodies are already isolated.
R/input-submit.R discourages submitButton() in favor of actionButton(): it stops every input in the app from auto-sending, misbehaves with two submit buttons, and does not work at all when created by renderUI() or insertUI(). Recommend debounce()/throttle()/eventReactive() instead.
reactlogShow(time = TRUE) defaults to the log shiny just recorded, while reactlog_show(log, ...) requires the caller to pass one.
Input bindings do not namespace ids: getId() reads data-input-id or the element's id as-is (srcts/src/bindings/input/inputBinding.ts). What find(scope) actually provides is re-binding of elements inserted after page load. Note that module callers must pass ns() themselves.
el.replaceWith(el.cloneNode(true)) swaps the DOM node out from under Shiny, which still holds a reference to the original element for unbind/rebind. Use a namespaced jQuery handler removed in unsubscribe().
The 'last element wins' behavior applies only when trigger renders as multiple top-level elements. A single span() is entirely hoverable -- bslib's ?tooltip recommends wrapping in div()/span() for exactly that reason -- so the previous example illustrated the opposite of the rule.
bs_theme(brand = TRUE) errors with 'The package "brand.yml" is required.' unless that Suggests-level package is installed, so the snippet was not runnable as written. Also document that discovery searches _brand/ and brand/ subdirectories and that brand accepts a list.
fluidPage(..., title, theme, lang) accepts a bs_theme() object, as do navbarPage() and fillPage(). The real bslib page-function advantages are sidebar support, fill behavior, and theme = bs_theme() by default.
R/react.R:212 raises 'Operation not allowed without an active reactive context.'; references/debugging.md already quoted it correctly, so the two files disagreed.
R/bootstrap.R:1140-1177 forwards renderDataTable()/dataTableOutput() to DT::renderDT()/DT::DTOutput() when DT >= 0.32.1 is installed; the bundled legacy DataTables path only runs without DT or under options(shiny.legacy.datatable = TRUE).
cache = "app" is a cachem::cache_mem() LRU with a default ~200 MB limit, so entries are evicted; 'remembers every value for a key' overstated it.
R/otel-label.R builds span names from the kind (reactive/observe/output, plus cache/event modifiers) and the object's label. debounce, throttle, reactivePoll, and reactiveFileReader contribute to the label, so those spans read 'reactive reactivePoll <label>' rather than 'reactivePoll'.
Several signatures implied positional arguments that do not exist in that position: nearPoints()/brushedPoints() omitted panelvar1/panelvar2 before threshold and allRows, page_sidebar()/input_dark_mode() start with ..., bs_theme()'s first argument is version, showNotification() takes action before duration, and renderCachedPlot() has sizePolicy and res before cache. The prose examples already named these correctly.
expect_no_error()'s `message` argument filters which conditions count as a failure rather than labeling one, so the sprintf() location string never matched: the parse error escaped the expectation, aborted the whole test_that() as an uncaught error, and discarded the file/line label. Catch the error and pass the location through `info` instead, which also lets the loop report every bad chunk instead of stopping at the first.
(s + 1):(e - 1) counts backwards when a chunk has no body, so the test would parse the fence lines themselves rather than the (empty) chunk.
expect_length(missing, 0) already recorded a failure, so the follow-up fail() reported the same problem twice. Keep only fail(), which names the missing exports, paired with succeed() so each reference still contributes one expectation.
A reference file whose name contained a digit or underscore matched list.files() but not the link pattern, so it would be reported as an orphaned file rather than as a linked one.
async.md, debugging.md, dashboard-components.md, and dashboard-design.md had no entry, so the exports they name were unguarded. Add entries for them and assert that names(apis) matches references/ exactly, so a new reference cannot pass this test by being absent from the list.
Since the button starts disabled and enables only once the matching downloadHandler registers, a mismatched output id shows up as a permanently greyed-out button rather than a dead click.
R/server-input-handlers.R:55-60 stops with 'There is already an input handler for type: ...' unless force = TRUE, which matters given the advice to register from .onLoad().
tabsetPanel(), navbarPage(), and navlistPanel() carry no deprecation badge in man/ and remain fully supported, so 'legacy' overstated their status. Say superseded and point at the bslib replacement for each.
A three-card grid is one fluidRow() containing three column()s, not 'three nested fluidRow(column(4, ...)) calls'.
The overview now says the bslib functions supersede the shiny ones; make the section headings and migration table agree.
'Do NOT ... do NOT' told the reader what to avoid without the cost that makes it worth avoiding; state what each habit actually does to the reactive graph.
Theming, Extending, and Ecosystem each held one row. Merge them into neighbouring sections, tell the reader to open only the references their task touches, and say plainly which areas (plain inputs, deployment, performance tuning) the skill does not cover.
The reference described mirai and future interchangeably but demonstrated neither, showing a later::later() promise instead. Use mirai in the runnable example, mirroring ?ExtendedTask, and cover the two things that actually bite: daemons() setup and passing values into the background process explicitly.
Both files listed mirai and future as interchangeable alternatives joined
by 'or'. Name mirai as the concrete way to move work off the main R
process, and split the ecosystem row so {promises} covers composition
while future appears as the promise-coercion route it provides.
schloerke
commented
Aug 26, 2026
{yaml} is a Suggests, so the check-depends-only job installs shiny
without it and test-skills.R errored with 'there is no package called
yaml' instead of skipping. This failure predates the current review
fixes; it arrived with the test file itself.
then() takes the promise first, so it pipes with base R and needs no special operator. Keep a one-line pointer that %...>% appears in older code so it stays recognizable when reading, without recommending it.
A bare future::future() blocks the main R process once every worker is busy; future_promise() returns a promise immediately and starts the work when a worker frees up. Recommend it wherever future is mentioned.
Name promises::future_promise() alongside a plain promise and a future::future() object, and describe the quick-reference entry as any promise-like object rather than any promise.
- feedback: state the showNotification/showModal no-session mistake directly - files: oversized uploads are rejected with an error, not silent - extended-tasks: drop the ambiguous 'read both naively' sentence - navigation: navset_underline is a style choice, not a default - layouts: fluidPage defaults to Bootstrap 3 styling, not 'unthemed' - session-lifecycle: distinguish fatal (shiny.error.fatal, closes the session) from render errors that only show as 'Error' output - dashboard-components: collapse stray double blank line
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ships an Agent Skill in the shiny package, discoverable by {btw}, porting the py-shiny agent skill to Shiny for R.
What's included
inst/skills/shiny-for-r/SKILL.md— router/index skill (frontmatter per btw'svalidate_skill()contract) with grouped topic tables routing to 21 reference files.inst/skills/shiny-for-r/references/*.md— 21 references (400–800 words each): reactivity, modules, session-lifecycle, async, extended-tasks, layouts, navigation, dashboard-components, dynamic-ui, theming-assets, plots, tables, files, feedback, bookmarking, custom-components, testing, debugging, opentelemetry, dashboard-design, ecosystem. bslib/thematic/brand.yml content is adopted in depth (folded in from the MIT-licensed posit-dev/skills shiny category); other companion packages ({DT}, {plotly}, {leaflet}, {shinychat}, {shinytest2}, ...) stay pointer-level viaecosystem.md— each should ship its own skill in lockstep with its features.tests/testthat/test-skills.R— guard tests: frontmatter validity, SKILL.md↔references link parity, every R chunk parses, and an API-sync list so renaming a documented shiny export fails loudly.Delivery mechanism (passive, mirai-style)
No R code ships for delivery. btw auto-discovers skills from attached packages via
system.file("skills", package = pkg); users can copy the skill into a project withbtw::btw_skill_install_package("shiny")or thebtw skillsCLI. Same convention {mirai} uses (inst/skills/mirai/).Verification
NAMESPACE/man pages; bslib functions against installed bslib 0.12.0 (version caveats included where features postdate shiny'sbslib >= 0.6.0floor).testServer()during authoring.library(shiny)+btw::btw_tool_skill("shiny-for-r")returns the skill and lists all 21 reference paths.Follow-ups (separate)
shiny-for-rin the posit-dev/skills marketplace (as mirai does, alongside in-package delivery).