From 133d6f7de2cf8a6779f7d11e8dfb45696c47e276 Mon Sep 17 00:00:00 2001 From: m7pr Date: Thu, 6 Nov 2025 13:09:30 +0100 Subject: [PATCH 01/13] module_bookmark_manager tests --- tests/testthat/test-module_bookmark_manager.R | 206 ++++++++++++++++++ 1 file changed, 206 insertions(+) create mode 100644 tests/testthat/test-module_bookmark_manager.R diff --git a/tests/testthat/test-module_bookmark_manager.R b/tests/testthat/test-module_bookmark_manager.R new file mode 100644 index 000000000..11db4c4ac --- /dev/null +++ b/tests/testthat/test-module_bookmark_manager.R @@ -0,0 +1,206 @@ +testthat::describe("get_bookmarking_option", { + testthat::it("returns NULL when no bookmarking option is set", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = NULL) + shinyOptions(bookmarkStore = NULL) + + testthat::expect_null(get_bookmarking_option()) + }) + + testthat::it("returns option value when shiny.bookmarkStore is set", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = "server") + shinyOptions(bookmarkStore = NULL) + + testthat::expect_identical(get_bookmarking_option(), "server") + }) + + testthat::it("returns shiny option value when set", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = NULL) + shinyOptions(bookmarkStore = "server") + + testthat::expect_identical(get_bookmarking_option(), "server") + }) + + testthat::it("prefers shiny option over R option", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = "url") + shinyOptions(bookmarkStore = "server") + + testthat::expect_identical(get_bookmarking_option(), "server") + }) +}) + +testthat::describe("need_bookmarking", { + testthat::it("returns all FALSE vector when all modules are bookmarkable", { + modules <- modules( + module("module_1", server = function(id, data) NULL), + module("module_2", server = function(id, data) NULL) + ) + attr(modules$children[[1]], "teal_bookmarkable") <- TRUE + attr(modules$children[[2]], "teal_bookmarkable") <- TRUE + + result <- need_bookmarking(modules) + testthat::expect_true(all(!result)) + testthat::expect_length(result, 2L) + }) + + testthat::it("returns vector indicating unbookmarkable modules", { + modules <- modules( + module("module_1", server = function(id, data) NULL), + module("module_2", server = function(id, data) NULL) + ) + attr(modules$children[[1]], "teal_bookmarkable") <- TRUE + attr(modules$children[[2]], "teal_bookmarkable") <- FALSE + + result <- need_bookmarking(modules) + testthat::expect_false(result[1]) + testthat::expect_true(result[2]) + testthat::expect_length(result, 2L) + }) + + testthat::it("returns all TRUE vector when none are bookmarkable", { + modules <- modules( + module("module_1", server = function(id, data) NULL), + module("module_2", server = function(id, data) NULL) + ) + attr(modules$children[[1]], "teal_bookmarkable") <- FALSE + attr(modules$children[[2]], "teal_bookmarkable") <- FALSE + + result <- need_bookmarking(modules) + testthat::expect_true(all(result)) + testthat::expect_length(result, 2L) + }) + + testthat::it("handles nested modules", { + modules_nested <- modules( + label = "parent", + modules( + label = "nested_group", + module("nested_1", server = function(id, data) NULL), + module("nested_2", server = function(id, data) NULL) + ) + ) + attr(modules_nested$children[[1]]$children[[1]], "teal_bookmarkable") <- TRUE + attr(modules_nested$children[[1]]$children[[2]], "teal_bookmarkable") <- FALSE + + result <- need_bookmarking(modules_nested) + testthat::expect_type(result, "logical") + testthat::expect_length(result, 2L) + testthat::expect_false(result[1]) + testthat::expect_true(result[2]) + }) +}) + +testthat::describe("srv_bookmark_panel", { + testthat::it("initializes without error when bookmarking is enabled", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = "server") + + modules <- modules( + module("module_1", server = function(id, data) NULL) + ) + attr(modules$children[[1]], "teal_bookmarkable") <- TRUE + + testthat::expect_no_error( + shiny::testServer( + app = srv_bookmark_panel, + args = list( + id = "test", + modules = modules + ), + expr = NULL + ) + ) + }) + + testthat::it("initializes without error when bookmarking is disabled", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = NULL) + + modules <- modules( + module("module_1", server = function(id, data) NULL) + ) + attr(modules$children[[1]], "teal_bookmarkable") <- TRUE + + testthat::expect_no_error( + shiny::testServer( + app = srv_bookmark_panel, + args = list( + id = "test", + modules = modules + ), + expr = NULL + ) + ) + }) + + testthat::it("does not fail when do_bookmark button is clicked", { + old_option <- getOption("shiny.bookmarkStore") + old_shiny_option <- getShinyOption("bookmarkStore") + on.exit({ + if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) + if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) + }) + + options(shiny.bookmarkStore = "server") + + modules <- modules( + module("module_1", server = function(id, data) NULL) + ) + attr(modules$children[[1]], "teal_bookmarkable") <- TRUE + + shiny::testServer( + app = srv_bookmark_panel, + args = list( + id = "test", + modules = modules + ), + expr = { + testthat::expect_no_error({ + session$setInputs(do_bookmark = 1) + }) + } + ) + }) +}) + + + From c7c960e0b7f9633e652ba1c07a6d3fffe92a071a Mon Sep 17 00:00:00 2001 From: m7pr Date: Mon, 24 Nov 2025 16:34:32 +0100 Subject: [PATCH 02/13] bring tests --- R/TealAppDriver.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index cac055f52..c72d39a77 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -189,7 +189,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. #' @return The value of the shiny input. get_active_module_input = function(input_id) { checkmate::check_string(input_id) - self$get_value(input = self$namespaces()$module(input_id)) + self$get_value(input = self$namespaces(TRUE)$module(input_id)) }, #' @description #' Get the output from the module in the `teal` app. @@ -200,7 +200,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. #' @return The value of the shiny output. get_active_module_output = function(output_id) { checkmate::check_string(output_id) - self$get_value(output = self$namespaces()$module(output_id)) + self$get_value(output = self$namespaces(TRUE)$module(output_id)) }, #' @description #' Get the output from the module's `teal.widgets::table_with_settings` or `DT::DTOutput` in the `teal` app. @@ -234,7 +234,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. get_active_module_plot_output = function(plot_id) { checkmate::check_string(plot_id) self$get_attr( - self$namespaces()$module(sprintf("%s-plot_main > img", plot_id)), + self$namespaces(TRUE)$module(sprintf("%s-plot_main > img", plot_id)), "src" ) }, From a32a9d9e17685282502f326faee818c0f9b40ce9 Mon Sep 17 00:00:00 2001 From: Marcin <133694481+m7pr@users.noreply.github.com> Date: Mon, 24 Nov 2025 16:36:39 +0100 Subject: [PATCH 03/13] Delete tests/testthat/test-module_bookmark_manager.R Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com> --- tests/testthat/test-module_bookmark_manager.R | 206 ------------------ 1 file changed, 206 deletions(-) delete mode 100644 tests/testthat/test-module_bookmark_manager.R diff --git a/tests/testthat/test-module_bookmark_manager.R b/tests/testthat/test-module_bookmark_manager.R deleted file mode 100644 index 11db4c4ac..000000000 --- a/tests/testthat/test-module_bookmark_manager.R +++ /dev/null @@ -1,206 +0,0 @@ -testthat::describe("get_bookmarking_option", { - testthat::it("returns NULL when no bookmarking option is set", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = NULL) - shinyOptions(bookmarkStore = NULL) - - testthat::expect_null(get_bookmarking_option()) - }) - - testthat::it("returns option value when shiny.bookmarkStore is set", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = "server") - shinyOptions(bookmarkStore = NULL) - - testthat::expect_identical(get_bookmarking_option(), "server") - }) - - testthat::it("returns shiny option value when set", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = NULL) - shinyOptions(bookmarkStore = "server") - - testthat::expect_identical(get_bookmarking_option(), "server") - }) - - testthat::it("prefers shiny option over R option", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = "url") - shinyOptions(bookmarkStore = "server") - - testthat::expect_identical(get_bookmarking_option(), "server") - }) -}) - -testthat::describe("need_bookmarking", { - testthat::it("returns all FALSE vector when all modules are bookmarkable", { - modules <- modules( - module("module_1", server = function(id, data) NULL), - module("module_2", server = function(id, data) NULL) - ) - attr(modules$children[[1]], "teal_bookmarkable") <- TRUE - attr(modules$children[[2]], "teal_bookmarkable") <- TRUE - - result <- need_bookmarking(modules) - testthat::expect_true(all(!result)) - testthat::expect_length(result, 2L) - }) - - testthat::it("returns vector indicating unbookmarkable modules", { - modules <- modules( - module("module_1", server = function(id, data) NULL), - module("module_2", server = function(id, data) NULL) - ) - attr(modules$children[[1]], "teal_bookmarkable") <- TRUE - attr(modules$children[[2]], "teal_bookmarkable") <- FALSE - - result <- need_bookmarking(modules) - testthat::expect_false(result[1]) - testthat::expect_true(result[2]) - testthat::expect_length(result, 2L) - }) - - testthat::it("returns all TRUE vector when none are bookmarkable", { - modules <- modules( - module("module_1", server = function(id, data) NULL), - module("module_2", server = function(id, data) NULL) - ) - attr(modules$children[[1]], "teal_bookmarkable") <- FALSE - attr(modules$children[[2]], "teal_bookmarkable") <- FALSE - - result <- need_bookmarking(modules) - testthat::expect_true(all(result)) - testthat::expect_length(result, 2L) - }) - - testthat::it("handles nested modules", { - modules_nested <- modules( - label = "parent", - modules( - label = "nested_group", - module("nested_1", server = function(id, data) NULL), - module("nested_2", server = function(id, data) NULL) - ) - ) - attr(modules_nested$children[[1]]$children[[1]], "teal_bookmarkable") <- TRUE - attr(modules_nested$children[[1]]$children[[2]], "teal_bookmarkable") <- FALSE - - result <- need_bookmarking(modules_nested) - testthat::expect_type(result, "logical") - testthat::expect_length(result, 2L) - testthat::expect_false(result[1]) - testthat::expect_true(result[2]) - }) -}) - -testthat::describe("srv_bookmark_panel", { - testthat::it("initializes without error when bookmarking is enabled", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = "server") - - modules <- modules( - module("module_1", server = function(id, data) NULL) - ) - attr(modules$children[[1]], "teal_bookmarkable") <- TRUE - - testthat::expect_no_error( - shiny::testServer( - app = srv_bookmark_panel, - args = list( - id = "test", - modules = modules - ), - expr = NULL - ) - ) - }) - - testthat::it("initializes without error when bookmarking is disabled", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = NULL) - - modules <- modules( - module("module_1", server = function(id, data) NULL) - ) - attr(modules$children[[1]], "teal_bookmarkable") <- TRUE - - testthat::expect_no_error( - shiny::testServer( - app = srv_bookmark_panel, - args = list( - id = "test", - modules = modules - ), - expr = NULL - ) - ) - }) - - testthat::it("does not fail when do_bookmark button is clicked", { - old_option <- getOption("shiny.bookmarkStore") - old_shiny_option <- getShinyOption("bookmarkStore") - on.exit({ - if (!is.null(old_option)) options(shiny.bookmarkStore = old_option) - if (!is.null(old_shiny_option)) shinyOptions(bookmarkStore = old_shiny_option) - }) - - options(shiny.bookmarkStore = "server") - - modules <- modules( - module("module_1", server = function(id, data) NULL) - ) - attr(modules$children[[1]], "teal_bookmarkable") <- TRUE - - shiny::testServer( - app = srv_bookmark_panel, - args = list( - id = "test", - modules = modules - ), - expr = { - testthat::expect_no_error({ - session$setInputs(do_bookmark = 1) - }) - } - ) - }) -}) - - - From 2f6ffdfd0136c80129bf322e670660c70665686a Mon Sep 17 00:00:00 2001 From: m7pr Date: Mon, 24 Nov 2025 16:56:09 +0100 Subject: [PATCH 04/13] revert 2 changes --- R/TealAppDriver.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index c72d39a77..73195f386 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -189,7 +189,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. #' @return The value of the shiny input. get_active_module_input = function(input_id) { checkmate::check_string(input_id) - self$get_value(input = self$namespaces(TRUE)$module(input_id)) + self$get_value(input = self$namespaces()$module(input_id)) }, #' @description #' Get the output from the module in the `teal` app. @@ -200,7 +200,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. #' @return The value of the shiny output. get_active_module_output = function(output_id) { checkmate::check_string(output_id) - self$get_value(output = self$namespaces(TRUE)$module(output_id)) + self$get_value(output = self$namespaces()$module(output_id)) }, #' @description #' Get the output from the module's `teal.widgets::table_with_settings` or `DT::DTOutput` in the `teal` app. From f8ea37e384081661d69f16fe69b8adbb02ba060c Mon Sep 17 00:00:00 2001 From: m7pr Date: Tue, 25 Nov 2025 15:37:09 +0100 Subject: [PATCH 05/13] unsure if all this is needed --- R/TealAppDriver.R | 82 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 74 insertions(+), 8 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index 73195f386..598367a64 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -87,8 +87,8 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. ) # end od check - private$set_active_ns() self$wait_for_idle() + private$set_active_ns() }, #' @description #' Append parent [`shinytest2::AppDriver`] `click` method with a call to `waif_for_idle()` method. @@ -555,17 +555,83 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. ), # private methods ---- set_active_ns = function() { + # Wait for page to be stable before trying to extract namespace + private$wait_for_page_stability() + all_inputs <- self$get_values()$input active_tab_inputs <- all_inputs[grepl("-active_module_id$", names(all_inputs))] - active_wrapper_id <- sub( - "^#", - "", - self$get_attr( - selector = sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs), - attribute = "href" + # If no active_module_id input found, find the selected/active tab button directly + if (length(active_tab_inputs) == 0 || is.null(active_tab_inputs) || active_tab_inputs == "") { + active_wrapper_id <- sub( + "^#", + "", + self$get_attr( + selector = ".teal-modules-tree li a.module-button.active, .teal-modules-tree li a.module-button[aria-selected='true']", + attribute = "href" + ) ) - ) + # Take first match if multiple found + if (length(active_wrapper_id) > 1) { + active_wrapper_id <- active_wrapper_id[1] + } + # If still not found, try any module button with a wrapper href + if (is.null(active_wrapper_id) || length(active_wrapper_id) == 0 || active_wrapper_id == "" || is.na(active_wrapper_id)) { + active_wrapper_id <- sub( + "^#", + "", + self$get_attr( + selector = ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])", + attribute = "href" + ) + ) + # Take first match if multiple found + if (length(active_wrapper_id) > 1) { + active_wrapper_id <- active_wrapper_id[1] + } + } + } else { + active_wrapper_id <- sub( + "^#", + "", + self$get_attr( + selector = sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs), + attribute = "href" + ) + ) + # Take first match if multiple found + if (length(active_wrapper_id) > 1) { + active_wrapper_id <- active_wrapper_id[1] + } + } + + # Ensure we have a valid wrapper ID + # get_attr returns character(0) when no elements found, or NA_character_ for missing attributes + if (is.null(active_wrapper_id) || + length(active_wrapper_id) == 0 || + (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { + # Try one more time after a short wait - the page might still be loading + Sys.sleep(0.5) + active_wrapper_id <- sub( + "^#", + "", + self$get_attr( + selector = ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])", + attribute = "href" + ) + ) + if (length(active_wrapper_id) > 1) { + active_wrapper_id <- active_wrapper_id[1] + } + } + + # Final check - if still not found, throw error + if (is.null(active_wrapper_id) || + length(active_wrapper_id) == 0 || + (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { + stop("Could not determine active module namespace. Make sure a module tab is selected and the page has finished loading.") + } + active_base_id <- sub("-wrapper$", "", active_wrapper_id) private$ns$base_id <- active_base_id From 001ae6fe9f84be66651a1982dfb39abef62cb1cc Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 25 Nov 2025 14:39:27 +0000 Subject: [PATCH 06/13] [skip style] [skip vbump] Restyle files --- R/TealAppDriver.R | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index 598367a64..496e78283 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -557,7 +557,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. set_active_ns = function() { # Wait for page to be stable before trying to extract namespace private$wait_for_page_stability() - + all_inputs <- self$get_values()$input active_tab_inputs <- all_inputs[grepl("-active_module_id$", names(all_inputs))] @@ -604,12 +604,12 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. active_wrapper_id <- active_wrapper_id[1] } } - + # Ensure we have a valid wrapper ID # get_attr returns character(0) when no elements found, or NA_character_ for missing attributes - if (is.null(active_wrapper_id) || - length(active_wrapper_id) == 0 || - (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { + if (is.null(active_wrapper_id) || + length(active_wrapper_id) == 0 || + (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { # Try one more time after a short wait - the page might still be loading Sys.sleep(0.5) active_wrapper_id <- sub( @@ -624,14 +624,14 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. active_wrapper_id <- active_wrapper_id[1] } } - + # Final check - if still not found, throw error - if (is.null(active_wrapper_id) || - length(active_wrapper_id) == 0 || - (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { + if (is.null(active_wrapper_id) || + length(active_wrapper_id) == 0 || + (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { stop("Could not determine active module namespace. Make sure a module tab is selected and the page has finished loading.") } - + active_base_id <- sub("-wrapper$", "", active_wrapper_id) private$ns$base_id <- active_base_id From 7a2a00447da92d8f9826dcb513d9f6cd22dec594 Mon Sep 17 00:00:00 2001 From: m7pr Date: Thu, 27 Nov 2025 16:56:38 +0100 Subject: [PATCH 07/13] simplify logic, based on feedback --- R/TealAppDriver.R | 99 +++++++++++++++++++++-------------------------- 1 file changed, 45 insertions(+), 54 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index 496e78283..96b1e7224 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -249,7 +249,6 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. #' @return The `TealAppDriver` object invisibly. set_active_module_input = function(input_id, value, ...) { checkmate::check_string(input_id) - checkmate::check_string(value) self$set_input( self$namespaces()$module(input_id), value, @@ -554,8 +553,29 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. filter_panel = character(0) ), # private methods ---- + # Helper function to extract wrapper ID from selector and take first match if multiple found + extract_wrapper_id = function(selector) { + wrapper_id <- sub( + "^#", + "", + self$get_attr(selector = selector, attribute = "href") + ) + # Take first match if multiple found + if (length(wrapper_id) > 1) { + wrapper_id <- wrapper_id[1] + } + wrapper_id + }, + # Helper function to check if wrapper ID is valid + is_valid_wrapper_id = function(wrapper_id) { + !is.null(wrapper_id) && + length(wrapper_id) > 0 && + !(length(wrapper_id) == 1 && (wrapper_id == "" || is.na(wrapper_id))) + }, set_active_ns = function() { - # Wait for page to be stable before trying to extract namespace + # Although wait_for_idle() is called before set_active_ns(), it only ensures Shiny is not processing. + # wait_for_page_stability() is needed here to ensure the DOM/UI is fully rendered and stable + # before trying to extract the namespace. private$wait_for_page_stability() all_inputs <- self$get_values()$input @@ -563,73 +583,44 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. # If no active_module_id input found, find the selected/active tab button directly if (length(active_tab_inputs) == 0 || is.null(active_tab_inputs) || active_tab_inputs == "") { - active_wrapper_id <- sub( - "^#", - "", - self$get_attr( - selector = ".teal-modules-tree li a.module-button.active, .teal-modules-tree li a.module-button[aria-selected='true']", - attribute = "href" - ) + active_wrapper_id <- private$extract_wrapper_id( + ".teal-modules-tree li a.module-button.active, .teal-modules-tree li a.module-button[aria-selected='true']" ) - # Take first match if multiple found - if (length(active_wrapper_id) > 1) { - active_wrapper_id <- active_wrapper_id[1] - } # If still not found, try any module button with a wrapper href - if (is.null(active_wrapper_id) || length(active_wrapper_id) == 0 || active_wrapper_id == "" || is.na(active_wrapper_id)) { - active_wrapper_id <- sub( - "^#", - "", - self$get_attr( - selector = ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])", - attribute = "href" - ) + if (!private$is_valid_wrapper_id(active_wrapper_id)) { + active_wrapper_id <- private$extract_wrapper_id( + ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])" ) - # Take first match if multiple found - if (length(active_wrapper_id) > 1) { - active_wrapper_id <- active_wrapper_id[1] - } } } else { - active_wrapper_id <- sub( - "^#", - "", - self$get_attr( - selector = sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs), - attribute = "href" - ) + active_wrapper_id <- private$extract_wrapper_id( + sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs) ) - # Take first match if multiple found - if (length(active_wrapper_id) > 1) { - active_wrapper_id <- active_wrapper_id[1] - } } # Ensure we have a valid wrapper ID # get_attr returns character(0) when no elements found, or NA_character_ for missing attributes - if (is.null(active_wrapper_id) || - length(active_wrapper_id) == 0 || - (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { + if (!private$is_valid_wrapper_id(active_wrapper_id)) { # Try one more time after a short wait - the page might still be loading Sys.sleep(0.5) - active_wrapper_id <- sub( - "^#", - "", - self$get_attr( - selector = ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])", - attribute = "href" - ) + active_wrapper_id <- private$extract_wrapper_id( + ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])" ) - if (length(active_wrapper_id) > 1) { - active_wrapper_id <- active_wrapper_id[1] - } } - # Final check - if still not found, throw error - if (is.null(active_wrapper_id) || - length(active_wrapper_id) == 0 || - (length(active_wrapper_id) == 1 && (active_wrapper_id == "" || is.na(active_wrapper_id)))) { - stop("Could not determine active module namespace. Make sure a module tab is selected and the page has finished loading.") + # Final check - if still not found, throw error with diagnostic information + if (!private$is_valid_wrapper_id(active_wrapper_id)) { + found_ids <- paste( + self$get_attr( + selector = ".teal-modules-tree li a.module-button[href*='-wrapper']", + attribute = "href" + ), + collapse = ", " + ) + stop(sprintf( + "Could not determine active module namespace. Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s", + if (length(found_ids) > 0) found_ids else "none" + )) } active_base_id <- sub("-wrapper$", "", active_wrapper_id) From 8a5d57b6f7077ccf6b8bc4c517a4a8f30f91dca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Llu=C3=ADs=20Revilla=20Sancho?= Date: Tue, 2 Dec 2025 09:30:46 +0100 Subject: [PATCH 08/13] Fix lintr issue --- R/TealAppDriver.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index 96b1e7224..4592e61b5 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -618,7 +618,8 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. collapse = ", " ) stop(sprintf( - "Could not determine active module namespace. Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s", + paste0("Could not determine active module namespace. ", + "Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s"), if (length(found_ids) > 0) found_ids else "none" )) } From 8aad92f6986d764c691c6aad4f5b88c937eb159d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Llu=C3=ADs=20Revilla=20Sancho?= Date: Tue, 2 Dec 2025 15:42:45 +0100 Subject: [PATCH 09/13] Logic simplification --- R/TealAppDriver.R | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index 4592e61b5..68930bec5 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -568,9 +568,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. }, # Helper function to check if wrapper ID is valid is_valid_wrapper_id = function(wrapper_id) { - !is.null(wrapper_id) && - length(wrapper_id) > 0 && - !(length(wrapper_id) == 1 && (wrapper_id == "" || is.na(wrapper_id))) + length(wrapper_id) == 1 && wrapper_id != "" && !is.na(wrapper_id) }, set_active_ns = function() { # Although wait_for_idle() is called before set_active_ns(), it only ensures Shiny is not processing. @@ -582,7 +580,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. active_tab_inputs <- all_inputs[grepl("-active_module_id$", names(all_inputs))] # If no active_module_id input found, find the selected/active tab button directly - if (length(active_tab_inputs) == 0 || is.null(active_tab_inputs) || active_tab_inputs == "") { + if (!length(active_tab_inputs) || active_tab_inputs == "") { active_wrapper_id <- private$extract_wrapper_id( ".teal-modules-tree li a.module-button.active, .teal-modules-tree li a.module-button[aria-selected='true']" ) From 7aaa547152b7ba386b14950eb756ddac5a338a00 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 2 Dec 2025 14:48:29 +0000 Subject: [PATCH 10/13] [skip style] [skip vbump] Restyle files --- R/TealAppDriver.R | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index 68930bec5..f5121ab8e 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -616,8 +616,10 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. collapse = ", " ) stop(sprintf( - paste0("Could not determine active module namespace. ", - "Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s"), + paste0( + "Could not determine active module namespace. ", + "Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s" + ), if (length(found_ids) > 0) found_ids else "none" )) } From a20a471ab8b703cb264a839657df98efa804a305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Llu=C3=ADs=20Revilla?= <185338939+llrs-roche@users.noreply.github.com> Date: Tue, 2 Dec 2025 15:58:13 +0100 Subject: [PATCH 11/13] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Oriol Senan <35930244+osenan@users.noreply.github.com> Signed-off-by: LluĂ­s Revilla <185338939+llrs-roche@users.noreply.github.com> --- R/TealAppDriver.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index f5121ab8e..d064cb5c9 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -570,7 +570,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. is_valid_wrapper_id = function(wrapper_id) { length(wrapper_id) == 1 && wrapper_id != "" && !is.na(wrapper_id) }, - set_active_ns = function() { + set_active_ns = function(sleep_time = 0.5) { # Although wait_for_idle() is called before set_active_ns(), it only ensures Shiny is not processing. # wait_for_page_stability() is needed here to ensure the DOM/UI is fully rendered and stable # before trying to extract the namespace. @@ -600,7 +600,7 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. # get_attr returns character(0) when no elements found, or NA_character_ for missing attributes if (!private$is_valid_wrapper_id(active_wrapper_id)) { # Try one more time after a short wait - the page might still be loading - Sys.sleep(0.5) + Sys.sleep(sleep_time) active_wrapper_id <- private$extract_wrapper_id( ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])" ) From 6e662e829f1a5cb2fd8f4637492687278552ce03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Llu=C3=ADs=20Revilla=20Sancho?= Date: Wed, 3 Dec 2025 14:09:57 +0100 Subject: [PATCH 12/13] Simplify the logic of the function --- R/TealAppDriver.R | 76 ++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 53 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index d064cb5c9..e325215b0 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -555,22 +555,14 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. # private methods ---- # Helper function to extract wrapper ID from selector and take first match if multiple found extract_wrapper_id = function(selector) { - wrapper_id <- sub( - "^#", - "", - self$get_attr(selector = selector, attribute = "href") - ) - # Take first match if multiple found - if (length(wrapper_id) > 1) { - wrapper_id <- wrapper_id[1] - } - wrapper_id + id <- self$get_attr(selector = selector, attribute = "href") + sub("^#", "", id[endsWith(id, "-wrapper")]) }, # Helper function to check if wrapper ID is valid is_valid_wrapper_id = function(wrapper_id) { - length(wrapper_id) == 1 && wrapper_id != "" && !is.na(wrapper_id) + length(wrapper_id) >= 1 & wrapper_id != "" & !is.na(wrapper_id) }, - set_active_ns = function(sleep_time = 0.5) { + set_active_ns = function() { # Although wait_for_idle() is called before set_active_ns(), it only ensures Shiny is not processing. # wait_for_page_stability() is needed here to ensure the DOM/UI is fully rendered and stable # before trying to extract the namespace. @@ -579,52 +571,30 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. all_inputs <- self$get_values()$input active_tab_inputs <- all_inputs[grepl("-active_module_id$", names(all_inputs))] - # If no active_module_id input found, find the selected/active tab button directly - if (!length(active_tab_inputs) || active_tab_inputs == "") { - active_wrapper_id <- private$extract_wrapper_id( - ".teal-modules-tree li a.module-button.active, .teal-modules-tree li a.module-button[aria-selected='true']" - ) - # If still not found, try any module button with a wrapper href - if (!private$is_valid_wrapper_id(active_wrapper_id)) { - active_wrapper_id <- private$extract_wrapper_id( - ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])" - ) - } - } else { - active_wrapper_id <- private$extract_wrapper_id( + ids <- c( + private$extract_wrapper_id( sprintf(".teal-modules-tree li a.module-button[data-value='%s']", active_tab_inputs) ) - } - - # Ensure we have a valid wrapper ID - # get_attr returns character(0) when no elements found, or NA_character_ for missing attributes - if (!private$is_valid_wrapper_id(active_wrapper_id)) { - # Try one more time after a short wait - the page might still be loading - Sys.sleep(sleep_time) - active_wrapper_id <- private$extract_wrapper_id( - ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])" - ) - } + # In principle once we get to this point we wouldn't need to search in other places + # FIXME: But it might be needed on the integration machine (somehow) + # private$extract_wrapper_id( + # ".teal-modules-tree li a.module-button.active, .teal-modules-tree li a.module-button[aria-selected='true']" + # ), + # private$extract_wrapper_id( + # ".teal-modules-tree li a.module-button[href*='-wrapper']:not([href='#'])" + # ) + ) + validity_ids <- private$is_valid_wrapper_id(unique(ids)) + valid_ids <- unique(ids[validity_ids]) - # Final check - if still not found, throw error with diagnostic information - if (!private$is_valid_wrapper_id(active_wrapper_id)) { - found_ids <- paste( - self$get_attr( - selector = ".teal-modules-tree li a.module-button[href*='-wrapper']", - attribute = "href" - ), - collapse = ", " - ) - stop(sprintf( - paste0( - "Could not determine active module namespace. ", - "Make sure a module tab is selected and the page has finished loading. Found wrapper IDs: %s" - ), - if (length(found_ids) > 0) found_ids else "none" - )) + if (length(valid_ids) > 1L) { + valid_ids <- valid_ids[1L] + } else if (length(valid_ids) < 1L) { + stop("Could not determine valid module namespace. ", + "Make sure a module tab is selected and the page has finished loading.") } - active_base_id <- sub("-wrapper$", "", active_wrapper_id) + active_base_id <- sub("-wrapper$", "", valid_ids) private$ns$base_id <- active_base_id private$ns$wrapper <- shiny::NS(active_base_id, "wrapper") From 9f231ca0d70fe24b54b39273f006a2ee6524abc9 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 3 Dec 2025 13:13:04 +0000 Subject: [PATCH 13/13] [skip style] [skip vbump] Restyle files --- R/TealAppDriver.R | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/R/TealAppDriver.R b/R/TealAppDriver.R index e325215b0..fd8cb196b 100644 --- a/R/TealAppDriver.R +++ b/R/TealAppDriver.R @@ -590,8 +590,10 @@ TealAppDriver <- R6::R6Class( # nolint: object_name. if (length(valid_ids) > 1L) { valid_ids <- valid_ids[1L] } else if (length(valid_ids) < 1L) { - stop("Could not determine valid module namespace. ", - "Make sure a module tab is selected and the page has finished loading.") + stop( + "Could not determine valid module namespace. ", + "Make sure a module tab is selected and the page has finished loading." + ) } active_base_id <- sub("-wrapper$", "", valid_ids)