From 07e7a885e6ec47932aa4c164e9f72f6a89ec0a44 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Fri, 10 Apr 2026 10:00:06 -0400 Subject: [PATCH 1/5] fix: Wait for download href before fetching (#445) app_download() now waits for the download element's href attribute to be populated before fetching. Previously, if the element wasn't in the DOM yet (e.g., inside an unopened modal) or the href wasn't set by Shiny's output binding, the function would silently download the app's HTML page instead of the intended file. Co-Authored-By: Claude Opus 4.6 --- R/app-driver-expect-download.R | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/R/app-driver-expect-download.R b/R/app-driver-expect-download.R index 1ae8709f..60f57473 100644 --- a/R/app-driver-expect-download.R +++ b/R/app-driver-expect-download.R @@ -9,14 +9,36 @@ app_download <- function( ckm8_assert_app_driver(self, private) self$log_message(paste0("Downloading file for output id: ", output)) - # Find the URL to download from (the href of the tag) + # Wait for the download element to exist and have a non-empty href. + # This handles cases where the element is dynamically rendered (e.g., in a + # modal) or Shiny's output binding hasn't set the href yet. (#445) + href_js <- paste0( + "var el = $('#", output, "'); ", + "el.length > 0 && el.attr('href') !== undefined && el.attr('href') !== ''" + ) + tryCatch( + chromote_wait_for_condition( + self$get_chromote_session(), + href_js, + timeout = private$timeout + ), + error = function(e) { + app_abort( + self, private, + c( + paste0("Download from '#", output, "' failed."), + "i" = "Element not found or its `href` attribute was not set within the timeout.", + "i" = "If the download button is rendered dynamically (e.g., in a modal), make sure the UI is visible before calling `$get_download()` or `$expect_download()`." + ) + ) + } + ) + + # Now fetch the href value sub_url <- chromote_eval( self$get_chromote_session(), paste0("$('#", output, "').attr('href')") )$result$value - if (identical(sub_url, "")) { - app_abort(self, private, paste0("Download from '#", output, "' failed")) - } # Add the base location to the URL full_url <- paste0(private$shiny_url$get(), sub_url) From cf059e0e6472aae7d068a06f5f46b28f2eaf1a47 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Fri, 10 Apr 2026 10:03:41 -0400 Subject: [PATCH 2/5] fix: Preserve original error in download timeout handler Co-Authored-By: Claude Opus 4.6 --- R/app-driver-expect-download.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/app-driver-expect-download.R b/R/app-driver-expect-download.R index 60f57473..4daef3b2 100644 --- a/R/app-driver-expect-download.R +++ b/R/app-driver-expect-download.R @@ -29,7 +29,8 @@ app_download <- function( paste0("Download from '#", output, "' failed."), "i" = "Element not found or its `href` attribute was not set within the timeout.", "i" = "If the download button is rendered dynamically (e.g., in a modal), make sure the UI is visible before calling `$get_download()` or `$expect_download()`." - ) + ), + parent = e ) } ) From b31600c976581cf93de1539f109e9d09e7b1da30 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Fri, 10 Apr 2026 10:04:46 -0400 Subject: [PATCH 3/5] test: Remove manual wait_for_js guards from download tests app_download() now waits internally for the href to be set, so the manual wait_for_js calls before each download are no longer needed. Co-Authored-By: Claude Opus 4.6 --- tests/testthat/test-app-download.R | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/testthat/test-app-download.R b/tests/testthat/test-app-download.R index 909d6e93..5cb1afb8 100644 --- a/tests/testthat/test-app-download.R +++ b/tests/testthat/test-app-download.R @@ -72,13 +72,6 @@ test_that("download files work from link and button", { app <- AppDriver$new(shiny_app, variant = NULL) - app$wait_for_js("$('#download_link_csv').attr('href') != ''") - app$wait_for_js("$('#download_button_csv').attr('href') != ''") - app$wait_for_js("$('#download_link_txt').attr('href') != ''") - app$wait_for_js("$('#download_button_txt').attr('href') != ''") - app$wait_for_js("$('#download_link_binary').attr('href') != ''") - app$wait_for_js("$('#download_button_binary').attr('href') != ''") - app$expect_download("download_link_txt") app$expect_download("download_button_txt") @@ -105,9 +98,6 @@ test_that("download files can be retrieved", { app <- AppDriver$new(shiny_app, variant = NULL) - app$wait_for_js("$('#download_link_csv').attr('href') != ''") - app$wait_for_js("$('#download_button_csv').attr('href') != ''") - link_file <- app$get_download("download_link_csv") button_file <- app$get_download("download_button_csv", "barret.test") From 09d049412259fd4e38e854ac6a79c8aa2010b95f Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Fri, 10 Apr 2026 10:06:23 -0400 Subject: [PATCH 4/5] test: Add modal download test for #445 Verifies that get_download() works correctly when the download button is inside a modal dialog, without needing manual wait_for_js calls. Co-Authored-By: Claude Opus 4.6 --- tests/testthat/test-app-download.R | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/testthat/test-app-download.R b/tests/testthat/test-app-download.R index 5cb1afb8..e8e07d91 100644 --- a/tests/testthat/test-app-download.R +++ b/tests/testthat/test-app-download.R @@ -107,3 +107,47 @@ test_that("download files can be retrieved", { expect_gt(file.info(link_file)$size, 0) expect_gt(file.info(button_file)$size, 0) }) + + +test_that("get_download works for download button inside a modal (#445)", { + modal_app <- shinyApp( + ui = basicPage( + actionButton("show_modal", "Show Modal") + ), + server = function(input, output, session) { + observeEvent(input$show_modal, { + showModal(modalDialog( + downloadButton("modal_download", "Download CSV"), + easyClose = TRUE, + title = "Download" + )) + }) + + output$modal_download <- downloadHandler( + filename = function() { + "modal-data.csv" + }, + content = function(file) { + write.csv(head(iris, 5), file, row.names = FALSE) + } + ) + } + ) + + app <- AppDriver$new(modal_app, variant = NULL) + + # Open the modal + app$click("show_modal") + + # get_download should wait for the href to be ready — no manual wait needed + file_path <- app$get_download("modal_download") + + # Verify we got actual CSV content, not HTML + content <- readLines(file_path, n = 1) + expect_false(grepl(" Date: Fri, 10 Apr 2026 10:10:44 -0400 Subject: [PATCH 5/5] docs: Add NEWS entry for download href wait fix (#445) Co-Authored-By: Claude Opus 4.6 --- NEWS.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/NEWS.md b/NEWS.md index 8c671569..cc16509f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,13 @@ +# shinytest2 (development version) + +## Bug fixes and minor improvements + +* `$get_download()` and `$expect_download()` now wait for the download + element's `href` attribute to be populated before fetching. Previously, + if the element wasn't in the DOM yet (e.g., inside a modal) or Shiny's + output binding hadn't set the `href`, these methods would silently + return the app's HTML page instead of the intended file (#445). + # shinytest2 0.5.1 ## New features