Issue with dynamic UI in 1.7.5 (runs fine in 1.7.4)

[vnijs]

I started a discussion at the link below and @gshotwell suggested I post an issue:

https://discord.com/channels/1109483223987277844/1154545900211937340

The issue seems to be caused by a change in how dynamic UI elements are processed in 1.7.5.1 compared to 1.7.4. The issue occurs in every component where there are column names from a dataset are used to populate a dropdown after switching from one dataset to another (e.g., diamonds to titanic)

Steps to reproduce:

1. Install radiant.model or radiant install.packages("radiant")
2. Start radiant from the Addins dropdown in Studio or using radiant::radiant()
3. Use shiny 1.7.5
4. Follow the steps in the (unlisted) video: https://youtu.be/RFQeFDKtk3c

To see what happens in 1.7.4.1 and all prior versions of shiny that I have used see below.

1. Install shiny 1.7.4.1: remotes::install_version("shiny", "1.7.4.1")
2. Follow the steps in the video above or view the (unlisted) video here: https://youtu.be/_7fOJemmtsw

As you can see in the first video, an error comes up in the JS console. There is only one ui_reg_rvar, a dynamic input that depends on the available data set, but somehow when a dataset is changed the JS code sees both the old one and refuses to create the new/updated one. Since most other inputs in Model > Linear regression depend ui_reg_rvar being available, those don't render either.

This is not just an issue with one component in Radiat. In every component that depends on a selected data to provide values for a dropdown, the same duplicate binding for ID ... error occurs.

Is there way to turn off the new dynamic processing approach in shiny 1.7.5? That would be quickest way forward with students starting classes next week.

Thanks,

Vincent

[gadenbuie]

Thanks for the report, Vincent. To quickly answer your question up front, no there isn't a way to opt out of the asynchronous UI rendering, other than staying with shiny < 1.7.5.

I've done some investigation, but haven't been able to find the root cause. Radiant is quite a complicated app! The rest of my notes are for others on the Shiny team who might have more insight.

I haven't been able to create a reprex that isolates the behavior; within radiant, this example includes nested dynamic UI (renderUI() that includes uiOutput()), dynamic UI outputs that create a tabsetPanel(). There are conditionalPanel()s within that stack and what may possibly be related is that the conditional panels use the current tab value of the tabset created in the UI as the signaling condition.

From the websocket logs I can tell that the client receives a single message with the UI intended for ui_regress.

This message creates ui_reg_rvars via uiOutput() among other outputs (ui_reg_rvars is the first).

What's unusual is that, if I add a breakpoint in renderContentAsync() somewhere around here or in the OutputBindingAdapter, I can see that renderContentAsync() is called twice for this message. (Note: you have to step through several render calls, but if you watch the html variable you can see that the messages are processed twice.) It's the second rendering of ui_regress that throws the error Vincent saw above, causing the content render to fail.

There are only a few places where renderContentAsync() is called:

1. In the message handler for shiny-insert-ui
2. In the message handler for shiny-insert-tab, once here, again here, and also here
3. It's called in the html output binding
4. Finally, It's also called when creating modals or notifications.

What I can see happening is that the OutputBindingAdapter.onValueChange() method is called twice for the #ui_regress output. The first time it works without error, but I don't see evidence of the new UI being inserted into the DOM (i.e. document.getElementById('ui_reg_rvar') returns undefined). But the second time the ui_regress element contains the shiny-bound-output class and .onValueChange() throws an error that ui_reg_rvar is an already-bound output.

Conceptually, this feels similar to #1399, but I also haven't been able to use that pattern to reproduce this issue.

@wch @jcheng5 @cpsievert Do you have any ideas or advice?

[wch]

I've been poking at this, and I've found something odd:

• When you switch to the Model->Linear Regression (OLS) tab for the first time, the dynamic UI element ui_regress contains a dynamic UI element named ui_reg_rvar.
• When you switch to the Model->Linear Regression (OLS) tab for the second time, it calls shinyUnbindAll(el) here:

  shiny/srcts/src/shiny/render.ts

  Line 44 in a6fc6bf

  shinyUnbindAll(el);

  
  At that point, inspecting el reveals that it contains only <div id="ui_regress" class="shiny-html-output"></div>.
• When it gets to the shinyBindAll(el) at

  shiny/srcts/src/shiny/render.ts

  Line 65 in a6fc6bf

  shinyBindAll(el);

  , it throws the error in question, I think because ui_reg_rvar output from the first run was not unbound in the previous step.

I believe the problem is that the previous UI is being removed before shinyUnbindAll() is called on it. There are a combination of factors that might be in play:

• ui_reg_rvar is a UI output inside of another UI output, ui_regress.
• These elements are on a separate tab from the input which causes the output values to change.
• ui_reg_rvar is inside of a conditionalPanel in ui_regress.

I think that by experimenting with various combinations of these things, it should be possible to create a simple reproducible example, which will make it easier to get to the bottom of things.

[vnijs]

FYI Linear regression is probably the most complex ui. Single mean in radiant.basics has the same issue and is less complex.

https://github.com/radiant-rstats/radiant.basics/blob/master/inst/app/tools/analysis/single_mean_ui.R

[cpsievert]

@wch I don't (yet) have a radiant-less reprex, but I was able to simplify radiant's UI logic quite a bit, and get a simple log of the problematic unbind/bind ordering (and, as @gadenbuie mentioned, #ui_regress is being rendered twice, for some reason I don't yet understand)

[wch]

I've been trying to make a minimal example but I haven't gotten very far. @vnijs if you are able to strip down Radiant to the bare minimum to reproduce this problem, that would be very helpful!

[vnijs]

@wch I may be on to something. Will post back asap when I have had a chance to do more testing. Thanks for looking into this!

[vnijs]

Hi All,

It seems that if I remove or comment out the req line shown below in the main ui for each component (e.g., ui_logistic), the problem goes away. This was required in with past versions of shiny to avoid error message if no dataset was yet available or had been selected from a dropdown. Unfortunately, I have no idea why this workaround should have the desired effect so I am not confident that this is a true fix to the problem. It seems like there is a very good chance the issue comes back to bite me somewhere, sometime. Any comments?

[wch]

@vnijs Thanks for that the information about req().

It's hard to say for sure if commenting it out will be a reliable fix until we really understand the issue. I think our next step is to try to create a truly minimal reproducible example, so that we can isolate the cause of the problem.

[wch]

I have a minimal reproducible example:

library(shiny)
library(bslib)

ui <- navbarPage(
  title = "Reprex",
  tabPanel(
    "Tab 1",
    selectInput("data", NULL, choices = c("a", "b")),
  ),
  tabPanel(
    "Tab 2",
    uiOutput("outer_ui")
  )
)

server <- function(input, output, session) {
  output$inner_inner_ui <- renderUI({
    "This is inner_inner_ui. YOU SHOULD BE ABLE TO SEE THIS!"
  })

  output$inner_ui <- renderUI({
    input$data
    div(
      uiOutput("inner_inner_ui"),
      icon("play"),
      sliderInput("s", "Slider:", min = 0, 10, 5)
    )
  })

  output$outer_ui <- renderUI({
    input$data
    uiOutput("inner_ui")
  })
}


shinyApp(ui, server)

To reproduce:

• Click on Tab 2. You will see the text, "This is inner_inner_ui. YOU SHOULD BE ABLE TO SEE THIS!"
• Click on Tab 1
• Change selection from "a" to "b"
• Click on Tab 2.
  • You will not see the text, "This is inner_inner_ui. YOU SHOULD BE ABLE TO SEE THIS!". The text should be there, but it will not be.
  • Also, there will be an error in the JS console: Duplicate binding for ID inner_inner_ui

Almost any significant change to the app will make the error not happen. For example, the following will cause the error to go away:

• Removing either of the references to input$data in the renderUI() calls.
• Removing either of the icon() or `sliderInput() calls.
• Moving output$inner_ui so that it comes after output$outer_ui.
• Putting the selectInput() and uiOutput on the same tab.
• Moving the icon() to the static UI.

[wch]

I modified renderContentAsync to print out messages as it goes. Just looking at the first eight messages, two things stand out to me:

• It goes from outer_ui: bind to inner_ui: unbind. I'm not 100% sure, but I think that it should complete the cycle for outer_ui (and print out a few more messages for outer_ui) before it goes to inner_ui.
• As it does inner_ui, it prints out inner_ui: renderHtmlAsync, and then goes to outer_ui: bind done. That is almost certainly wrong. Even if inner_ui can correctly be processed inside of the outer_ui, it should not interleave their steps like that.

[jcheng5]

Great insights, @wch. I followed the rabbit trail a little to see how the interleaving is happening, and got here:

shiny/srcts/src/shiny/shinyapp.ts

Line 515 in a6fc6bf

if (this.$values[id] !== undefined) binding.onValueChange(this.$values[id]);

Seems like that onValueChange() call is missing an await? (and of course we can't do that without following the call stack up)

[wch]

@jcheng5 Thanks, it looks like that line is the culprit. I tried adding an await to that function call, and adding async and await all the way down the stack, and it fixes the behavior.

The main entry points are (internally) shinyBindAll(), and (externally) Shiny.bindAll() (which is assigned as windowShiny.bindAll()).

There are some issues with making everything on the stack async:

• There are some synchronous functions which call shinyBindAll() or Shiny.bindAll(). We could probably change all the internal functions which call it. However any external code which calls Shiny.bindAll() would still have this problem, since they will expect the effect to be synchronous. (The change would make us no worse off than we are with Shiny 1.7.5, but compared to 1.7.4, there could be problems.)

[gadenbuie]

I was surprised that this isn't something that Typescript catches, but it turns out there are linting rules for this. Should we add no-floating-promises to our eslint config?

[wch]

Nice, I didn't know about the no-floating-promises rule. We definitely should turn that on. I just tried it locally and found that our .eslintrc.yml also needs the following for it to work:

parserOptions:
  project:
    - './tsconfig.json'

After enabling it, it flags a number of other places where we created promises without handling them.