Fixes reporter bug where buttons would appear in other tabs when reporter is NULL (alternative) - #1737
Fixes reporter bug where buttons would appear in other tabs when reporter is NULL (alternative)#1737averissimo wants to merge 4 commits into
NULL (alternative)#1737Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved reporter UI behavior and incomplete test coverage remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes reporter controls appearing in other tabs when reporter is NULL.
Changes:
- Propagates
reporterthrough UI modules. - Conditionally renders reporter menus and buttons.
- Updates tests, documentation, and NEWS.
File summaries
| File | Summary | Review notes |
|---|---|---|
tests/testthat/test-module_teal.R |
Adds reporter UI coverage. | Moderate issue (3 votes): test selector does not match the rendered element. |
R/module_teal.R |
Controls reporter menu rendering. | Moderate issue (1 vote): separate UI/server calls can leave controls visible without handlers. |
R/module_nested_tabs.R |
Propagates reporter state and conditionally renders buttons. | — |
R/init.R |
Passes reporter into the UI. | — |
NEWS.md |
Documents the bug fix. | — |
man/module_teal.Rd |
Documents the UI argument. | — |
man/module_teal_module.Rd |
Updates nested module documentation. | — |
Review details
Files not reviewed (2)
- man/module_teal.Rd: Generated file
- man/module_teal_module.Rd: Generated file
Suppressed comments (1)
R/module_teal.R:48
ui_teal()andsrv_teal()are separate module calls, so existing callers can use the default UI here while passingreporter = NULLonly tosrv_teal(). Before this change the server removed the already-rendered menu and wrappers for that supported combination; after removing that fallback, those controls remain visible but have no handlers. Preserve a fallback removal or make the UI/server reporter contract explicit and enforce it.
ui_teal <- function(id, modules, reporter = teal.reporter::Reporter$new()) {
- Files reviewed: 5/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
averissimo
changed the title
NULLNULL (alternative)
osenan
left a comment
osenan
left a comment
There was a problem hiding this comment.
This PR is also simple, not as minimal as its alternative #1738 but stays consistent. I think we need to update the status of withr in the DESCRIPTION and slightly modify the vignette of the teal as a shiny module.
Corresponds more to the product decision to see which solution is accepted. The complexity of this PR is small, IMO opinion the improvement is not very different, but we can accept both solutions.
| teal.reporter::reset_report_button_ui(ns("reset_reports"), label = "Reset Report") | ||
| } | ||
| if (!is.null(reporter)) { | ||
| withr::with_options(reporter_opts, { # for backwards compatibility of the report_previewer_module$server_args |
There was a problem hiding this comment.
I am surprised withr package is in suggest, it shuold be on imports as it is necessary for the reporter funcionality.
There was a problem hiding this comment.
Good point, we can use rlang::with_options which should result in the same outcome
There was a problem hiding this comment.
Maybe we can make small update in vignette teal-as-shiny-module to show the usage of reporter argument for function ui_teal
…rter is `NULL` (#1738) # Pull Request Fixes #1727 Alternative approach: #1737 ### Changes description - Remove all elements of "Add to report" in modules, instead of just one --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Pull Request
Fixes #1727
Alternative approach: #1737
Changes description
reporterargument toui_teal*functions and propagate it downAlternative
Part of the reason why we didn't do this before was to allow
srv_tealandui_tealto work with reporter by only supplying the server function and allow it to manage the UIThe drawback is that there is a visual glitch where the buttons are showed momentarily, before
removeUiis executed (however that was something already established)