Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix decorator problems after a good first run #1515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1d41fcf
feat: initial support
averissimo Apr 9, 2025
10ac479
feat: accordion still works
averissimo Apr 14, 2025
32c1cd4
feat: improve on styling and simplify shinyjs
averissimo Apr 14, 2025
36be93d
Merge branch 'main' into 1511-fix_decorator@main
averissimo Apr 14, 2025
44c4143
fix: reactivity error
averissimo Apr 16, 2025
5f7d1b3
chore: use inherits instead of rlang::is_condition
averissimo Apr 16, 2025
a86ef14
Merge branch 'main' into 1511-fix_decorator@main
averissimo May 7, 2025
cc80dae
fix: revert unecessary change
averissimo May 7, 2025
c4c9fe3
fix: missing space
averissimo May 7, 2025
121c468
feat: removes 'is_transform_failed' and improves solution
averissimo May 9, 2025
bf65ecc
[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update
github-actions[bot] May 9, 2025
9225ed0
feat: safer rethrow
averissimo May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 10 additions & 17 deletions R/module_nested_tabs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,33 +336,26 @@ srv_teal_module.teal_module <- function(id,
data = data,
is_active = is_active
)
is_transform_failed <- reactiveValues()
transformed_teal_data <- srv_transform_teal_data(
"data_transform",
data = filtered_teal_data,
transformators = modules$transformators,
modules = modules,
is_transform_failed = is_transform_failed
modules = modules
)
any_transform_failed <- reactive({
any(unlist(reactiveValuesToList(is_transform_failed)))
!inherits(try(transformed_teal_data(), silent = TRUE), "teal_data") &&
inherits(filtered_teal_data(), "teal_data")
Comment on lines +346 to +347
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As transformed_teal_data depends on the output of filtered_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).

Suggested change
!inherits(try(transformed_teal_data(), silent = TRUE), "teal_data") &&
inherits(filtered_teal_data(), "teal_data")
!inherits(try(transformed_teal_data(), silent = TRUE), "teal_data")

})

observeEvent(any_transform_failed(), {
if (isTRUE(any_transform_failed())) {
shinyjs::hide("teal_module_ui")
shinyjs::show("transform_failure_info")
} else {
shinyjs::show("teal_module_ui")
shinyjs::hide("transform_failure_info")
}
shinyjs::toggleElement("transform_failure_info", condition = any_transform_failed())
})

module_teal_data <- reactive({
req(inherits(transformed_teal_data(), "teal_data"))
all_teal_data <- transformed_teal_data()
module_datanames <- .resolve_module_datanames(data = all_teal_data, modules = modules)
all_teal_data[c(module_datanames, ".raw_data")]
summary_data <- reactiveVal(NULL)
module_teal_data <- eventReactive(transformed_teal_data(), {
module_datanames <- .resolve_module_datanames(data = transformed_teal_data(), modules = modules)
summary_data(transformed_teal_data()[c(module_datanames, ".raw_data")])
summary_data()
})

srv_check_module_datanames(
Expand All @@ -371,7 +364,7 @@ srv_teal_module.teal_module <- function(id,
modules = modules
)

summary_table <- srv_data_summary("data_summary", module_teal_data)
summary_table <- srv_data_summary("data_summary", summary_data) # Only updates on success
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not be in error state or hidden when transformators introduce an error.

The alternative is to use module_teal_data and remove the (newly) introduced summary_data as below.

Q: WDYT?

image

Copy link
Contributor

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.


observeEvent(input$data_summary_toggle, {
bslib::toggle_sidebar(id = "teal_module_sidebar", open = TRUE)
Expand Down
62 changes: 41 additions & 21 deletions R/module_transform_data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,19 @@ ui_transform_teal_data <- function(id, transformators, class = "well") {

display_fun(
bslib::accordion(
id = ns("wrapper"),
class = "validation-wrapper",
bslib::accordion_panel(
id = ns("wrapper_panel"),
class = "validation-panel",
attr(data_mod, "label"),
icon = bsicons::bs_icon("palette-fill"),
tags$div(
class = "disabled-info",
title = "Disabled until data becomes valid",
bsicons::bs_icon("info-circle"),
"Disabled until data becomes valid. Check your inputs."
),
tags$div(
id = transform_wrapper_id,
if (is.null(data_mod$ui)) {
Expand All @@ -62,7 +72,7 @@ ui_transform_teal_data <- function(id, transformators, class = "well") {

#' @export
#' @rdname module_transform_data
srv_transform_teal_data <- function(id, data, transformators, modules = NULL, is_transform_failed = reactiveValues()) {
srv_transform_teal_data <- function(id, data, transformators, modules = NULL) {
checkmate::assert_string(id)
assert_reactive(data)
checkmate::assert_class(modules, "teal_module", null.ok = TRUE)
Expand All @@ -76,39 +86,47 @@ srv_transform_teal_data <- function(id, data, transformators, modules = NULL, is
names(transformators) <- sprintf("transform_%d", seq_len(length(transformators)))

moduleServer(id, function(input, output, session) {
data_original_handled <- reactive(tryCatch(data(), error = function(e) e))
module_output <- Reduce(
function(data_previous, name) {
moduleServer(name, function(input, output, session) {
logger::log_debug("srv_transform_teal_data@1 initializing module for { name }.")

data_out <- reactiveVal()

# Disable all elements if original data is not yet a teal_data
observeEvent(data_original_handled(), {
shinyjs::toggleState(
"wrapper_panel",
Copy link
Contributor

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.

condition = inherits(data_original_handled(), "teal_data")
)
})

.call_once_when(inherits(data_previous(), "teal_data"), {
logger::log_debug("srv_teal_transform_teal_data@2 triggering a transform module call for { name }.")
data_unhandled <- transformators[[name]]$server("transform", data = data_previous)
data_handled <- reactive(tryCatch(data_unhandled(), error = function(e) e))

observeEvent(data_handled(), {
if (inherits(data_handled(), "teal_data")) {
if (!identical(data_handled(), data_out())) {
observeEvent(
{
data_handled()
data_original_handled()
Comment on lines +111 to +112
Copy link
Contributor

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 only data_handled()? Or in reversed order?

},
{
if (inherits(data_original_handled(), "condition")) {
data_out(data_original_handled())
} else if (inherits(data_handled(), "teal_data")) {
if (!identical(data_handled(), data_out())) {
data_out(data_handled())
}
} else {
data_out(data_handled())
}
}
})

is_transform_failed[[name]] <- FALSE
observeEvent(data_handled(), {
if (inherits(data_handled(), "teal_data")) {
is_transform_failed[[name]] <- FALSE
} else {
is_transform_failed[[name]] <- TRUE
}
})
)

is_previous_failed <- reactive({
idx_this <- which(names(is_transform_failed) == name)
is_transform_failed_list <- reactiveValuesToList(is_transform_failed)
idx_failures <- which(unlist(is_transform_failed_list))
any(idx_failures < idx_this)
inherits(data_original_handled(), "teal_data") &&
!inherits(try(data_previous(), silent = TRUE), "teal_data")
})

srv_validate_error("silent_error", data_handled, validate_shiny_silent_error = FALSE)
Expand All @@ -118,6 +136,7 @@ srv_transform_teal_data <- function(id, data, transformators, modules = NULL, is
}

# When there is no UI (`ui = NULL`) it should still show the errors
# It is a 1-way operation as there is no UI to correct the state
observe({
if (!inherits(data_handled(), "teal_data") && !is_previous_failed()) {
shinyjs::show("wrapper")
Expand All @@ -132,7 +151,7 @@ srv_transform_teal_data <- function(id, data, transformators, modules = NULL, is
"One of previous transformators failed. Please check its inputs.",
class = "teal-output-warning"
)
} else {
} else if (inherits(data_original_handled(), "teal_data")) {
shinyjs::enable(transform_wrapper_id)
shiny::tagList(
ui_validate_error(session$ns("silent_error")),
Expand All @@ -145,7 +164,8 @@ srv_transform_teal_data <- function(id, data, transformators, modules = NULL, is

# Ignoring unwanted reactivity breaks during initialization
reactive({
req(data_out())
validate(need(!inherits(data_out(), "condition"), message = data_out()$message)) # rethrow message
data_out()
})
})
},
Expand Down
28 changes: 28 additions & 0 deletions inst/css/validation.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,31 @@
border: 1px solid red;
padding: 1em;
}

.validation-wrapper:has(.validation-panel[disabled="disabled"]) {
--bs-accordion-bg: var(--bs-gray-100);
background-color: var(--bs-gray-100);
}

.validation-wrapper:has(.validation-panel[disabled="disabled"]) .teal_validated,
.validation-wrapper:has(.validation-panel[disabled="disabled"]) .shiny-output-error,
.validation-wrapper:has(.validation-panel[disabled="disabled"]) .teal-output-warning {
display: none;
}

.validation-wrapper .disabled-info {
display: none;
}

.validation-wrapper .validation-panel[disabled="disabled"] {
padding-top: 0;
}

.validation-wrapper .validation-panel[disabled="disabled"] .disabled-info {
display: block;
border: 1px solid var(--bs-info);
border-radius: 4px;
padding: 1em;
color: color-mix(in srgb, var(--bs-info), black 20%);
background-color: color-mix(in srgb, var(--bs-info) 5%, transparent);
}
12 changes: 1 addition & 11 deletions man/module_transform_data.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions teal.Rproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
Version: 1.0
ProjectId: fa9b6a6c-a470-42a8-bc1a-165563e5ac01

RestoreWorkspace: Default
SaveWorkspace: Default
Expand Down