Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork51
Fix decorator problems after a good first run#1515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
averissimo commentedMay 7, 2025
github-actionsbot commentedMay 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Unit Test Performance Difference
Additional test case details
Results for commit45147c8 ♻️ This comment has been updated with latest results. |
github-actionsbot commentedMay 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Code Coverage SummaryDiff against mainResults for commit:d7f553c Minimum allowed coverage is ♻️ This comment has been updated with latest results |
github-actionsbot commentedMay 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Unit Tests Summary 1 files 25 suites 2m 31s ⏱️ Results for commitd7f553c. ♻️ This comment has been updated with latest results. |
averissimo commentedMay 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
📑 Journal(rolling update):
Note: UI changes about placement and what should show where should be discussed#1509 Latest screenshot Older screenshots_(nothing to see here yet)_ |
| ) | ||
| summary_table<- srv_data_summary("data_summary",module_teal_data) | ||
| summary_table<- srv_data_summary("data_summary",summary_data)# Only updates on success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like that there is not data summary as it cannot be provided, but maybe it was hidden because this makes the different order pop up the error message more for the users. But I prefer to not have hidding elements messing with the layout of the app and leave the section even if empty.
llrs-roche left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I left a couple of comments as I couldn't run yet the branch. The most important is to make it easier to pull the branch. I had messed my git config but I fixed it.
It would also great if the example app at the beginning of the PR could be updated to really open an app (to avoid me messing it and test different things)
Uh oh!
There was an error while loading.Please reload this page.
| ) | ||
| summary_table<- srv_data_summary("data_summary",module_teal_data) | ||
| summary_table<- srv_data_summary("data_summary",summary_data)# Only updates on success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like that there is not data summary as it cannot be provided, but maybe it was hidden because this makes the different order pop up the error message more for the users. But I prefer to not have hidding elements messing with the layout of the app and leave the section even if empty.
llrs-roche left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Code looks good I left a couple of comments but I think this resolves the issue:
2025-05-23_15-00-44.mp4
I think the code on "example app" is not the one that generates the leading screenshots. But if there is further testing to be done let me know, the checks locally (andremotely) run well
| !inherits(try(transformed_teal_data(),silent=TRUE),"teal_data")&& | ||
| inherits(filtered_teal_data(),"teal_data") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Astransformed_teal_data depends on the output offiltered_teal_data it can be omitted and I think we would get the same result (at least on the app on the issue it works as before).
| !inherits(try(transformed_teal_data(),silent=TRUE),"teal_data")&& | |
| inherits(filtered_teal_data(),"teal_data") | |
| !inherits(try(transformed_teal_data(),silent=TRUE),"teal_data") |
| # Disable all elements if original data is not yet a teal_data | ||
| observeEvent(data_original_handled(), { | ||
| shinyjs::toggleState( | ||
| "wrapper_panel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think there is a div with id = "wrapper" for modules tabs, but where does this_panel come from? Would it work on teal modules used directly on shiny? Just thinking aloud after resolving an issue coming from an id not matching between different parts.
| data_handled() | ||
| data_original_handled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice trick, never thought about adding two expressions as a single event. Would be the same as to have onlydata_handled()? Or in reversed order?
Uh oh!
There was an error while loading.Please reload this page.
averissimo commentedMay 28, 2025
@llrs-roche this is not forgotten,@gogonzo wants to add some functionality on top after we get an understanding of |
llrs-roche commentedOct 14, 2025
I was checking the board and I don't know were we stay on this PR/feature. The coments I left on the review are minor and at the time I thought it could be merged. Do we merge this@gogonzo ? |
gogonzo commentedOct 14, 2025
PR was not enough as reason of failure lays deeper. I'd expect making a solution which can standardize error handling and would rather remove lines than add. There was one PR at that time I raised but I'm sceptical if anybody will review that anytime soon. I don't have capacity to handle this topic now :/ |
gogonzo commentedOct 14, 2025
Sorry@llrs-roche I confused this PR with#1509 teal-app-failuresoptions(teal.log_level="ERROR",teal.show_js_log=TRUE,# teal.bs_theme = bslib::bs_theme(version = 5),shiny.bookmarkStore="server")pkgload::load_all("teal")tm_decorated_plot<-function(label="module",transformators=list(),decorators=list(),datanames="all") {checkmate::assert_list(decorators,"teal_transform_module") module(label=label,ui=function(id,decorators) {ns<- NS(id) div( selectInput(ns("dataname"),label="select dataname",choices=NULL,multiple=TRUE), selectInput(ns("x"),label="select x",choices=NULL,multiple=TRUE), selectInput(ns("y"),label="select y",choices=NULL,multiple=TRUE), ui_transform_teal_data(ns("decorate"),transformators=decorators), plotOutput(ns("plot")), verbatimTextOutput(ns("text")) ) },server=function(id,data,decorators) { moduleServer(id,function(input,output,session) { observeEvent(data(), {dataname<-if (length(input$dataname))input$datanameelse names(data())[1] updateSelectInput(inputId="dataname",choices= names(data()),selected=dataname) }) observeEvent(input$dataname, { req(input$dataname) updateSelectInput(inputId="x",choices= colnames(data()[[input$dataname]])) updateSelectInput(inputId="y",choices= colnames(data()[[input$dataname]])) })dataname<- reactive(req(input$dataname))x<- reactive({ req(input$x,input$x%in% colnames(data()[[dataname()]]))input$x })y<- reactive({ req(input$y,input$y%in% colnames(data()[[dataname()]]))input$y })plot_data<- reactive({# todo: make sure it triggers once on init# and once on change of its input and once on change in previous stages req(dataname(), x(), y()) Sys.sleep(5)# to mimic relatively long computation within(data(), {plot<-ggplot2::ggplot(dataname,ggplot2::aes(x=x,y=y))+ggplot2::geom_point() },dataname= as.name(dataname()),x= as.name(x()),y= as.name(y()) ) })plot_data_decorated_no_print<- srv_transform_teal_data("decorate",data=plot_data,transformators=decorators )plot_data_decorated<- reactive( within(req(plot_data_decorated_no_print()),expr=plot) )plot_r<- reactive({ plot_data_decorated()[["plot"]] })output$plot<- renderPlot(plot_r())output$text<- renderText({teal.code::get_code(req(plot_data_decorated())) }) }) },ui_args=list(decorators=decorators),server_args=list(decorators=decorators),datanames=datanames,transformators=transformators )}make_data<-function(datanames= c("ADSL","ADTTE")) {data_obj<-teal.data::teal_data()if ("ADSL"%in%datanames) {data_obj<- within(data_obj,ADSL<-teal.data::rADSL) }if ("ADTTE"%in%datanames) {data_obj<- within(data_obj,ADTTE<-teal.data::rADTTE) } join_keys(data_obj)<-default_cdisc_join_keys[datanames]data_obj}data<- teal_data_module(once=FALSE,ui=function(id) {ns<- NS(id) tagList( selectizeInput( ns("errortype"),label="Error Type",choices= c("ok","insufficient datasets","no data","qenv.error","error in reactive","validate error","silent.shiny.error","not a reactive" ) ) ) },server=function(id,...) { moduleServer(id,function(input,output,session) {logger::log_trace("example_module_transform2 initializing.") reactive({switch(input$errortype,ok= make_data(),`insufficient datasets`= make_data(datanames="ADSL"),`no data`= teal_data(),qenv.error= within(data(), stop("\nthis is qenv.error in teal_data_module\n")),`error in reactive`= stop("\nerror in a reactive in teal_data_module\n"),`validate error`= validate(need(FALSE,"\nvalidate error in teal_data_module\n")),`silent.shiny.error`= req(FALSE) ) }) }) })trans<- teal_transform_module(ui=function(id) {ns<- NS(id) tagList( selectizeInput( ns("errortype"),label="Error Type",choices= c("ok","insufficient datasets","no data","qenv.error","error in reactive","validate error","silent.shiny.error","not a reactive" ) ) ) },server=function(id,data) { moduleServer(id,function(input,output,session) {logger::log_trace("example_module_transform2 initializing.")# to check if data with error causes problemsdata2<- reactive(data())data3<- eventReactive(data2(), data2()) observeEvent(data3(), {# do nothing }) reactive({# notes: make sure it:# - triggers once on init# - once on change of its input# - once on change in data inputnew_data<-switch(input$errortype,ok= data3(),`insufficient datasets`= data3()["ADSL"],`no data`= teal_data(),qenv.error= within(teal_data(), stop("\nthis is qenv.error in teal_transform_module\n")),`error in reactive`= stop("\nerror in a reactive in teal_transform_module\n"),`validate error`= validate(need(FALSE,"\nvalidate error in teal_transform_module\n")),`silent.shiny.error`= req(FALSE) )new_data }) }) })trans_empty<- teal_transform_module(server=function(id,data) { moduleServer(id,function(input,output,session) { reactive({ validate(need(nrow(data()$ADSL)>250,"ADSL needs 250 rows")) data() }) }) })decor<- teal_transform_module(label="X-axis decorator",ui=function(id) {ns<- NS(id) tagList( selectizeInput( ns("action"),label="Action type",choices= c("nothing","decorate","no data","qenv.error","error in reactive","validate error","silent.shiny.error","not a reactive" ) ) ) },server=function(id,data) { moduleServer(id,function(input,output,session) {logger::log_trace("example_module_transform2 initializing.") reactive({switch(input$action,nothing= data(),`decorate`= data()|> within(plot<-plot+ggplot2::ggtitle("Decorated Title")),`no data`= teal_data(),qenv.error= within(teal_data(), stop("\nthis is qenv.error in teal_transform_module\n")),`error in reactive`= stop("\nerror in a reactive in teal_transform_module\n"),`validate error`= validate(need(FALSE,"\nvalidate error in teal_transform_module\n")),`silent.shiny.error`= req(FALSE) ) }) }) })app<-teal::init(data=data,modules= modules( modules(label="first tab", tm_decorated_plot("mod-2",transformators=list(trans,trans,trans_empty),decorators=list(decor,decor),datanames= c("ADSL","ADTTE") ), example_module() ) ),filter= teal_slices( teal_slice("ADSL","SEX"), teal_slice("ADSL","AGE",selected= c(18L,65L)), teal_slice("ADTTE","PARAMCD",selected="CRSD"),include_varnames=list(ADSL= c("SEX","AGE") ) ))runApp(app) |
llrs-roche commentedOct 14, 2025
Thanks tothe app, I reviewed this again: Data summary is present even when there is an error on qenv: Errors are duplicated for each transform module: Error messages are different on the UI between the transform Data panel and the main panel: Initial error pops a modal and then the summary data is different from what the error says: In conclusion. it fixes the second issue (Last state is shown, be it an error or a plot) but fails to provide "consistent error messages in decorators". |
llrs-roche left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See comment above
averissimo commentedOct 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Summary of the meeting with@llrs-roche
It makes sense, the summary table should be reactive Action: grey out the "no datasets to show" in summary table
Plan: only show in transformators if the error occurred there
The error should also appear in the main module UI note from Lluis: see if namespace is wrong
Bug when there is no UI for the transfomators and it enters an error state Expected result is for the pseudo-UI to be visible with the error showing Extra mile would be to modify the notification when filters are removed due to incompatible datanames.
![]() |







Uh oh!
There was an error while loading.Please reload this page.
Pull Request
Fixes#1511
Changes description
teal_transform_dataon initializationdecorators🤖 Example app