Fix module print method for nested decorator lists #1685
Merged
+504
−89
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Updated format.teal_module to recursively extract decorator labels from nested lists - Added tests for single, multiple, and nested decorator structures - Fixes issue where decorators in nested lists showed NULL instead of labels Co-authored-by: m7pr <133694481+m7pr@users.noreply.github.com>
…action - Added depth parameter with limit of 10 to prevent potential infinite recursion - Improved code safety based on code review feedback - All tests still pass Co-authored-by: m7pr <133694481+m7pr@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Improve module print method for multiple decorators
Fix module print method for nested decorator lists
Jan 27, 2026
Collaborator
devtools::load_all()
decorator1 <- teal_transform_module(
label = "Decorator One",
server = function(id, data) {
moduleServer(id, function(input, output, session) data)
}
)
decorator2 <- teal_transform_module(
label = "Decorator Two",
server = function(id, data) {
moduleServer(id, function(input, output, session) data)
}
)
mod <- module(
label = "test module",
server_args = list(decorators = list(decorator1, decorator2))
)
mod
|
m7pr
approved these changes
Jan 27, 2026
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Open
3 tasks
Contributor
Unit Tests Summary 1 files 29 suites 2m 20s ⏱️ Results for commit 7e94ee2. ♻️ This comment has been updated with latest results. |
Contributor
Unit Test Performance Difference
Additional test case details
Results for commit e9c5e64 ♻️ This comment has been updated with latest results. |
Contributor
|
Code Coverage SummaryDiff against mainResults for commit: 7e94ee2 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
m7pr
approved these changes
Jan 27, 2026
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
llrs-roche
reviewed
Jan 28, 2026
Contributor
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
Nicely tested with format()!
I miss some tests with a mix of decorators: one general and some others specific:
decorators = list(decorator0.
table = list(decorator1, decorator2),
plot = list(decorator3)
)
I see it works but see also my comment about having a tree structure to know which decorator is applied to each object.
Collaborator
|
Now decorator1 <- teal_transform_module(
label = "Dec1",
server = function(id, data) {
moduleServer(id, function(input, output, session) data)
}
)
decorator2 <- teal_transform_module(
label = "Dec2",
server = function(id, data) {
moduleServer(id, function(input, output, session) data)
}
)
decorator3 <- teal_transform_module(
label = "Dec3",
server = function(id, data) {
moduleServer(id, function(input, output, session) data)
}
)
mod <- module(
label = "test module",
server_args = list(
decorators = list(
table = list(decorator1, decorator2),
plot = list(decorator3)
)
)
)
mod
gives 
and this 
|
llrs-roche
approved these changes
Jan 28, 2026
Contributor
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
My local checks fail: [ FAIL 7 | WARN 0 | SKIP 0 | PASS 85 ] I'm not sure why, but it might be related to the [ opened that are not closed that are somehow misidentified.
One last comment, are the semicolons : not aligned on purpose? See the proposed changes
I think it is fine for the individual objects but wanted to check with you.
Possible alignment of semicolons:
| |- Server Arguments :
| |- Decorators :
| | |- Global Decorator
| | |- table: Decorator One, Decorator Two
| | L- plot : Decorator Three
| L- Transformators :
Now that I think of it a similar modification might be in order for Transformators.
Co-authored-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com> Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Collaborator
|
About the semicolons, this was a mistake. I think your proposition is good enough but I'm also thinking if we could add some more spaces before 
so it's like this 
|
Contributor
|
That would be perfect, but I wasn't sure if the decorators as being less important (?) were worth to indent as the other sections. Not sure if people use this feature much and we dedicated enough time already ... I'll leave it up to you |
Collaborator
|
Hey guys, with the newest approach (where arguments are grouped under one umbrella, and transformators and decorators are split into multiple lines) I decided that colon indentation is now obsolete and decided to show the print like that I think it's clean enough and serves the purpose of inspecting the module structure (currently nested for decorators and transformators) 
Hoping to get this closed, so we can focus on bigger priorities Codedevtools::load_all(".")
library(teal.data)
test_decorator <- teal::teal_transform_module(
label = "Test Decorator",
ui = function(id) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::textInput(ns("text"), "Text Input", value = "default")
)
},
server = function(id, data) {
shiny::moduleServer(id, function(input, output, session) {
shiny::reactive({
req(data())
within(data(), {
if (exists("test_var")) {
test_var <- paste(test_var, input$text)
}
})
})
})
}
)
table_decorator <- teal::teal_transform_module(
label = "Table Style Decorator",
ui = function(id) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::checkboxInput(ns("bold"), "Bold", value = FALSE)
)
},
server = function(id, data) {
shiny::moduleServer(id, function(input, output, session) {
shiny::reactive({
req(data())
data()
})
})
}
)
test_transformator <- teal::teal_transform_module(
label = "Data Transformator",
ui = function(id) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::numericInput(ns("multiplier"), "Multiplier", value = 1)
)
},
server = function(id, data) {
shiny::moduleServer(id, function(input, output, session) {
shiny::reactive({
req(data())
within(data(), {
if (exists("ADSL")) {
ADSL$new_var <- ADSL$AGE * input$multiplier
}
})
})
})
}
)
data <- teal_data(
ADSL = teal.data::rADSL
)
# Create a module with:
# - UI args (templates_ids)
# - Server args (templates, decorators)
# - Decorators (both global and object-specific)
# - Transformators
test_module <- teal::module(
label = "Test Module with All Features",
ui = function(id, templates_ids) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::h3("Test Module"),
shiny::p(paste("Template IDs:", paste(templates_ids, collapse = ", ")))
)
},
server = function(id, data, templates, reporter, ...) {
shiny::moduleServer(id, function(input, output, session) {
# Module server logic
output$plot <- shiny::renderPlot({
plot(1:10)
})
})
},
ui_args = list(
templates_ids = c("template1", "template2", "template3")
),
server_args = list(
templates = data.frame(
id = c("template1", "template2"),
name = c("Template 1", "Template 2")
),
decorators = list(
# Global decorator
test_decorator,
# Object-specific decorators
table = list(table_decorator)
)
),
transformators = list(test_transformator)
)
print(test_module)
# Also test with modules() to see nested structure
modules_obj <- teal::modules(
label = "Test Modules",
test_module
)
cat("\n=== Modules Structure ===\n")
print(modules_obj)
|
llrs-roche
reviewed
Feb 10, 2026
Contributor
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
I like the output when there are decorators and transformators.
I found format.teal_module quite complex (cyclocomp complexity of 46!).
Lines of codes to handle:
- Datasets: 430:436 -> 6
- properties: 438:447 ->9
- arguments: 449:560 -> 111
- decorators: 492:558 ->66
- transformators 562:588->26
Couldn't some code from transformators be reused on the arguments to handle decorators? Or extract some of these in a separate function? I think it would be great if we can make it simpler.
What should be the output when there are not transformators or decorators?
Testing without them they are printed but with no information: should the leaves not be printed, have NULL or something else? Maybe this was discussed and I missed it.
print(test_module)
|- Test Module with All Features
| |- Datasets: all
| |- Properties:
| | |- Bookmarkable: FALSE
| | L- Reportable: TRUE
| |- Arguments:
| | |- templates_ids (character)
| | L- templates (data.frame)
| | L- Decorators:
| |- Transformators: Test files check that something is present not how. I leave some minor comments about the code style and there is one on tests that I think applies to multiple tests.
Collaborator
Examples and results below Codedevtools::load_all(".")
library(teal.data)
test_decorator <- teal::teal_transform_module(
label = "Test Decorator",
ui = function(id) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::textInput(ns("text"), "Text Input", value = "default")
)
},
server = function(id, data) {
shiny::moduleServer(id, function(input, output, session) {
shiny::reactive({
req(data())
within(data(), {
if (exists("test_var")) {
test_var <- paste(test_var, input$text)
}
})
})
})
}
)
table_decorator <- teal::teal_transform_module(
label = "Table Style Decorator",
ui = function(id) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::checkboxInput(ns("bold"), "Bold", value = FALSE)
)
},
server = function(id, data) {
shiny::moduleServer(id, function(input, output, session) {
shiny::reactive({
req(data())
data()
})
})
}
)
test_transformator <- teal::teal_transform_module(
label = "Data Transformator",
ui = function(id) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::numericInput(ns("multiplier"), "Multiplier", value = 1)
)
},
server = function(id, data) {
shiny::moduleServer(id, function(input, output, session) {
shiny::reactive({
req(data())
within(data(), {
if (exists("ADSL")) {
ADSL$new_var <- ADSL$AGE * input$multiplier
}
})
})
})
}
)
data <- teal_data(
ADSL = teal.data::rADSL
)
# Create a module with:
# - UI args (templates_ids)
# - Server args (templates, decorators)
# - Decorators (both global and object-specific)
# - Transformators
test_module <- teal::module(
label = "Test Module with All Features",
ui = function(id, templates_ids) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::h3("Test Module"),
shiny::p(paste("Template IDs:", paste(templates_ids, collapse = ", ")))
)
},
server = function(id, data, templates, reporter, ...) {
shiny::moduleServer(id, function(input, output, session) {
# Module server logic
output$plot <- shiny::renderPlot({
plot(1:10)
})
})
},
ui_args = list(
templates_ids = c("template1", "template2", "template3")
),
server_args = list(
templates = data.frame(
id = c("template1", "template2"),
name = c("Template 1", "Template 2")
),
decorators = list(
# Global decorator
test_decorator,
# Object-specific decorators
table = list(table_decorator)
)
),
transformators = list(test_transformator)
)
print(test_module)
# Module without decorators or transformators
bare_module <- teal::module(
label = "Bare Module (no decorators, no transformators)",
ui = function(id, page_size) {
ns <- shiny::NS(id)
shiny::tagList(
shiny::h3("Bare Module"),
shiny::p(paste("Page size:", page_size))
)
},
server = function(id, data, cache, ...) {
shiny::moduleServer(id, function(input, output, session) NULL)
},
ui_args = list(page_size = 25),
server_args = list(cache = TRUE)
)
cat("\n=== Bare Module (no decorators, no transformators) ===\n")
print(bare_module)
# Minimal module — no args, no decorators, no transformators
minimal_module <- teal::module(
label = "Minimal Module"
)
cat("\n=== Minimal Module (nothing extra) ===\n")
print(minimal_module)
# Also test with modules() to see nested structure
modules_obj <- teal::modules(
label = "Test Modules",
test_module,
bare_module,
minimal_module
)
cat("\n=== Modules Structure ===\n")
print(modules_obj)
3 - module with everything |
Collaborator
|
llrs-roche
approved these changes
Feb 11, 2026
Contributor
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
Looks great! The code is easier to follow now.
One last nitpick, some leaves seem to carry on some | that does not continue below. If possible it would be great to fix, if not it good enough now in my opinion.
| |- Transformators:
- | | L- Data Transformator
+ | L- Data Transformator
|- Bare Module (no decorators, no transformators)
| |- Datasets: all
| |- Properties:
| | |- Bookmarkable: FALSE
| | L- Reportable: FALSE
| |- Arguments:
- | | |- page_size (numeric)
- | | L- cache (logical)
+ | |- page_size (numeric)
+ | L- cache (logical)…com/insightsengineering/teal into copilot/improve-module-print-method
Collaborator
|
@llrs-roche I added the requested change 
Lastly, I see 2 tests fail locally - one for teal.slice and one for teal.reporter hmm ── Failed tests ──────────────────────────────────────────────────────────────
Failure (test-teal_reporter.R:2:3): TealReportCard object can be initialized
Expected `TealReportCard$new()` to produce warnings.
Backtrace:
▆
1. └─lifecycle::expect_deprecated(TealReportCard$new()) at test-teal_reporter.R:2:3
2. └─testthat::expect_warning(...)
Error (test-teal_slices.R:192:3): c.teal_slices combines mapping of two equal slices objects but ignores adding duplicated one
Error in `teal.slice::teal_slices(..., exclude_varnames = exclude_varnames, include_varnames = include_varnames, count_type = count_type, allow_add = allow_add)`: Some teal_slice objects have the same id:
test1
Backtrace:
▆
1. ├─testthat::expect_identical(...) at test-teal_slices.R:192:3
2. │ └─testthat::quasi_label(enquo(object), label)
3. │ └─rlang::eval_bare(expr, quo_get_env(quo))
4. ├─base::c(tss1, tss2)
5. ├─teal:::c.teal_slices(tss1, tss2)
6. │ ├─base::do.call(...)
7. │ └─teal (local) `<fn>`(...)
8. │ ├─shiny::isolate(...) at teal/R/teal_slices.R:81:3
9. │ │ ├─shiny::..stacktraceoff..(...)
10. │ │ └─ctx$run(...)
11. │ │ ├─promises::with_promise_domain(...)
12. │ │ │ └─domain$wrapSync(expr)
13. │ │ ├─shiny::withReactiveDomain(...)
14. │ │ │ └─promises::with_promise_domain(...)
15. │ │ │ └─domain$wrapSync(expr)
16. │ │ │ └─base::force(expr)
17. │ │ ├─shiny::captureStackTraces(...)
18. │ │ │ └─promises::with_promise_domain(...)
19. │ │ │ └─domain$wrapSync(expr)
20. │ │ │ └─base::withCallingHandlers(expr, error = doCaptureStack)
21. │ │ └─env$runWith(self, func)
22. │ │ └─shiny (local) contextFunc()
23. │ │ └─shiny::..stacktraceon..(expr)
24. │ └─teal.slice::teal_slices(...) at teal/R/teal_slices.R:111:5
25. │ └─base::stop("Some teal_slice objects have the same id:\n", toString(unique(slices_id[duplicated(slices_id)])))
26. └─base::.handleSimpleError(...)
27. └─shiny (local) h(simpleError(msg, call))
[ FAIL 2 | WARN 0 | SKIP 51 | PASS 598 ] |
Contributor
|
Thanks @m7pr! Merge as soon as the checks are green |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
The module print method displayed
NULLfor decorators when structured as nested lists:Changes
R/modules.R: Added recursive helper informat.teal_module()to extract labels from nested decorator structures. Includes depth limit (10) to prevent infinite recursion.tests/testthat/test-modules.R: Added tests for single, flat list, nested list, and mixed decorator structures.Now correctly displays:
Decorators: decorator1, decorator2regardless of list nesting depth.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.