insightsengineering / teal.slice

Reproducible slice module for teal applications
https://insightsengineering.github.io/teal.slice/
Other
11 stars 5 forks source link

Reduce unwanted reactivity on filter panel changes #593

Closed vedhav closed 2 months ago

vedhav commented 4 months ago

Closes #62

Pending things before it can be undrafted:

  1. Make the private$reactive_call for a given sid. Right now it will not work when sid is provided. A possible solution is to make the sid a private object and use it inside the server logic.
  2. Figure out the right place to observe self$get_call() to extract the call.
  3. Test for normal data.frame. CDISC data.frame and MAE object.

Example app to test:

library(shiny)
library(teal.data)
pkgload::load_all("teal.slice")

datasets <- init_filtered_data(
  list(
    iris = iris,
    ADSL = rADSL, ADAE = rADAE,
    MAE = hermes::multi_assay_experiment
  ),
  join_keys = default_cdisc_join_keys[c("ADSL", "ADAE")]
)

ui <- fluidPage(
  fluidRow(
    column(
      width = 9,
      DT::DTOutput("iris_table"),
      DT::DTOutput("adsl_table"),
      DT::DTOutput("adae_table"),
      shiny::verbatimTextOutput("mae_text")
    ),
    column(width = 3, datasets$ui_filter_panel("filter_panel"))
  )
)

server <- function(input, output, session) {
  datasets$srv_filter_panel("filter_panel")
  iris_filtered_data <- reactive(datasets$get_data(dataname = "iris", filtered = TRUE))
  adsl_filtered_data <- reactive(datasets$get_data(dataname = "ADSL", filtered = TRUE))
  adae_filtered_data <- reactive(datasets$get_data(dataname = "ADAE", filtered = TRUE))
  mae_filtered_data <- reactive(datasets$get_data(dataname = "MAE", filtered = TRUE))

  observeEvent(iris_filtered_data(), {
    print("IRIS changed!!!")
  })
  observeEvent(adsl_filtered_data(), {
    print("ADSL changed!!!")
  })
  observeEvent(adae_filtered_data(), {
    print("ADAE changed!!!")
  })
  observeEvent(mae_filtered_data(), {
    print("MAE changed!!!")
  })
  output$iris_table <- DT::renderDT(head(iris_filtered_data()))
  output$adsl_table <- DT::renderDT(head(adsl_filtered_data()))
  output$adae_table <- DT::renderDT(head(adae_filtered_data()))
  output$mae_text <- renderPrint({
    mae_filtered_data()
  })
}

shinyApp(ui, server)
gogonzo commented 4 months ago

Looks good already. sid problem is a minor thing - it is only for reactive-counts in FilterState

vedhav commented 4 months ago

@gogonzo I was trying to figure out what was broken when this sid is not passed properly and I can't seem to figure out a way to test this. The only thing I know is that it is used here

gogonzo commented 4 months ago

@gogonzo I was trying to figure out what was broken when this sid is not passed properly and I can't seem to figure out a way to test this. The only thing I know is that it is used here

Yu, I just noticed that when count_type = "all" it uses the same dataset for each FilterState.

What if you try to bind on a call here? https://github.com/insightsengineering/teal/blob/5c3e3fd4380ef0fc7efc53ec1cdf4f5a7c922dd3/R/module_nested_tabs.R#L278

github-actions[bot] commented 4 months ago

Unit Tests Summary

  1 files   29 suites   22s :stopwatch: 362 tests 353 :white_check_mark: 0 :zzz: 9 :x: 831 runs  822 :white_check_mark: 0 :zzz: 9 :x:

For more details on these failures, see this check.

Results for commit 3ab89dbf.

:recycle: This comment has been updated with latest results.

donyunardi commented 3 months ago

@vedhav @gogonzo Is this urgent? Otherwise, should we hold off on this movement until we finished other priorities?

gogonzo commented 3 months ago

@vedhav @gogonzo Is this urgent? Otherwise, should we hold off on this movement until we finished other priorities?

This is not urgent and working solution hasn't been provided. Let's deprioritize this