Enhancement: SimplifyTLG Order Details Table, Internalize PCSPC Filtering by h5hoang · Pull Request #1380 · pharmaverse/aNCA

h5hoang · 2026-07-08T21:29:30Z

Issue

Stacked on #1343 — set the PR base branch to 1343-enhancement-expand-tlg-catalog, not main. This branch depends on the urine TLG functions introduced in #1343 and cannot merge until #1356 is merged.

Description

Simplifies the TLG Order Details table and internalizes urine (PCSPEC) filtering:

Order Details table: the internal Condition column is hidden from the UI and removed from the editable set. It remains in tlg.yaml as metadata and still drives urine auto-selection under the hood. The table is trimmed to the recommended columns: Type, Dataset, Output, Footnote, Stratification, Comment (PKid / Label / Description dropped from display).
Internal PCSPEC filtering: the urine TLG functions (t_pkpt08_uri, l_pkcl02_uri, p_pkpg01_cum, p_pkpg01_per, from the Enhancement: Expand TLG catalog with new graph and table functions #1343 base) filter to urine specimens by themselves. When PCSPEC/PPSPEC is absent they emit a warning, which is now surfaced to the user as an in-app showNotification() rather than failing silently.
Add-TLGs picker redesign: the old grouped reactable (whose Type/Dataset columns were blank on every row) is replaced with a catalog checklist: dataset tabs (PK Concentrations / PK Parameters) over one column per output type, with search, CSV/XLSX export, per-column and tab-scoped select-all/clear, live counts, and a live selection count on the confirm button.

Definition of Done

Condition column removed from Order Details table
Urine TLG functions filter by PCSPEC internally
Warning/notification shown when PCSPEC is missing but a urine TLG is rendered
Redundant columns reviewed and reduced
No regression in TLG rendering

How to test

Launch the app, upload a dataset, and run NCA so ADNCA/ADPP are available.
Go to the TLG tab → Order details. Confirm the table shows only Type, Dataset, Output, Footnote, Stratification, Comment — no Condition column, and Condition is not editable.
Confirm urine outputs still auto-select when the data contains urine specimens (PCSPEC = "URINE").
Click Add TLG:
- Switch between the PK Concentrations / PK Parameters tabs (columns realign and tab counts update).
- Type in Search (matches appear across both datasets and tab badges show match counts).
- Select all (toolbar) selects only the current tab's items; each column's Select all selects that column within the tab; Clear all resets everything.
- CSV / XLSX download the available-TLG list.
- The confirm button shows a live count ("Add N to order").
Render a urine output (e.g. a urine listing/graph) on data without PCSPEC/PPSPEC and confirm a warning notification appears.

Contributor checklist

Notes to reviewer

happy to split it into its own PR if there is a preference to push the core changes first before the UI changes

…key columns

… search, downloads, and scoped select-all

Gero1999

I really really love this! Everything worked as expected within the App experience

I will just write some non-blocking things I would suggest to do:

Codewise: This PR injects a large block of hardcoded CSS, perhaps we can consider moving it to the scss documents. There's already a modules/_colors.scss; insteadd of hardcoding, the colors (#007bc2, #E7ECFA, etc...) could come from there.
Experience wise: If any ADPP-TLG based is requested by the user, but no ADDP is availabe (i.e, NCA was not run) push a notification for the user to know why some TLGs may be missing/not rendered well. Right now the error/problem is silent. But this can easily be a new issue/PR, I will leave it to your criteria 😉

Shaakon35 · 2026-07-16T12:05:35Z

bugtlgs.mp4

Found a little bug when you filter on "tt", there is 0 pk conc, and 1 pk param, but selecting the one with 0 does not change the display

…ent-simplify-tlg-order-details-table-and-internalize-pcspec-filtering

Shaakon35 · 2026-07-16T12:15:20Z

+      if (length(checked_ids) > 0) {
        tlg_order_data <- tlg_order()
-        tlg_order_data$Selection[!tlg_order_data$Selection][selected_rows] <- TRUE
+        tlg_order_data$Selection[tlg_order_data$id %in% as.integer(checked_ids)] <- TRUE


[Error] New selection logic is untested.

The reactable→checkbox rewrite changes how selections map to tlg_order() (now id %in% checked_ids instead of positional index). This is the highest-risk change in the PR and has no test — both "passes all unit tests" and "new logic covered by tests" are unchecked.

Proposed resolution — add a testServer test (e.g. tests/testthat/test-tab_tlg.R):

describe("tab_tlg_server: add-picker selection", { it("sets Selection = TRUE for the checked ids", { testServer(tab_tlg_server, args = list(data = reactive(test_data)), { # two non-default rows available in the modal ids <- tlg_order()$id[!tlg_order()$Selection][1:2] modal_group_ids("modal_check_1") session$setInputs(modal_check_1 = as.character(ids)) session$setInputs(confirm_add_tlg = 1) expect_true(all(tlg_order()$Selection[tlg_order()$id %in% ids])) }) }) it("removes the selected rows", { testServer(tab_tlg_server, args = list(data = reactive(test_data)), { # remove handler uses positional index into currently-selected rows before <- sum(tlg_order()$Selection) # stub selected_tlg_state()$selected = 1 via the reactable mock, then: session$setInputs(remove_tlg = 1) expect_lt(sum(tlg_order()$Selection), before) }) }) })

Also add a unit test for the warning path in tlg_module.R (see separate comment there).

Shaakon35 · 2026-07-16T12:15:21Z

+      # line up on the same left edge.
+      div(
+        class = "tlg-add-modal",
+        tags$style(HTML("


[Warning] Inline CSS bypasses the SCSS workflow and duplicates existing color tokens.

~100 lines of CSS are embedded via tags$style, with hardcoded hex values (#007bc2, #E7ECFA) that already exist as SCSS variables $anca-blue / $anca-blue-light in styles/modules/_colors.scss. This is the only module using inline tags$style, and per AGENTS.md styling must live in .scss and be compiled to main.css. The checklist item "run compile_css.R" is unchecked.

Proposed resolution — move the block into a new partial and reuse tokens:

// inst/shiny/www/styles/partials/_tlg_add_modal.scss @use "../modules/colors" as *; .tlg-add-modal { --tlg-inset: 0.55em; .tlg-tab.active { color: $anca-blue; border-bottom-color: $anca-blue; } .tlg-add-checklist .checkbox:has(input:checked) { background: $anca-blue-light; } .tlg-add-checklist .checkbox input[type=checkbox] { accent-color: $anca-blue; } // …migrate the remaining rules here… }

// styles/main.scss @use "partials/tlg_add_modal";

Then drop the tags$style(...) from .build_add_checklist(), run Rscript data-raw/compile_css.R, and commit both the partial and the regenerated main.css.

Shaakon35 · 2026-07-16T12:15:22Z

 }

-tab_tlg_server <- function(id, data, adpp = reactive(NULL)) {
+tab_tlg_server <- function(id, data, adpp = reactive(NULL)) { # nolint: cyclocomp_linter


[Warning] Complexity suppressed instead of reduced.

# nolint: cyclocomp_linter was added because tab_tlg_server now mixes UI construction, a ~100-line inline <style>, an inline window.tlgAdd JS object, download handlers, and confirm/remove logic in one function. Per AGENTS.md ("flag overly complex implementations that could be simplified"), this should be decomposed rather than silenced.

Proposed resolution — extract helpers into inst/shiny/functions/ (this also makes the logic testable for E1):

# inst/shiny/functions/tlg_add_picker.R #' Build the "Add TLGs" catalog checklist UI. build_add_checklist <- function(avail, ns) { # …move the current .build_add_checklist body here… } #' Assets (CSS + JS) for the add-picker modal. tlg_add_picker_assets <- function() { tagList( includeCSS(app_sys("shiny/www/main.css")), # once styles are moved to SCSS tags$script(src = "tlg_add_picker.js") # move window.tlgAdd here ) } #' Rows checked in the modal, mapped back to tlg_order ids. checked_tlg_ids <- function(input, group_ids) { as.integer(unlist(lapply(group_ids, function(gid) input[[gid]]))) }

Then tab_tlg_server calls these helpers, window.tlgAdd moves to www/tlg_add_picker.js, and the nolint can be removed.

Shaakon35 · 2026-07-16T12:15:23Z

+          warning = function(w) {
+            showNotification(conditionMessage(w), type = "warning", duration = 10)
+            invokeRestart("muffleWarning")
+          }


[Warning] Warning-surfacing path is untested; raw warning text shown to users.

The withCallingHandlers → showNotification → muffleWarning path (the user-facing half of the PCSPEC-missing behavior) has no test, and it forwards the raw conditionMessage(w) — which may read technically for end users.

Proposed resolution — add a unit test and a friendlier prefix:

# tests/testthat/test-tlg_module.R it("surfaces render warnings as notifications and continues (issue #1335)", { warn_fn <- function(data, ...) { warning("PCSPEC/PPSPEC not found; specimen filtering skipped") list("plot_a") } testServer( tlg_module_server, args = list(data = test_data, type = "graph", render_list = warn_fn, options = list()), { session$setInputs(entries_per_page = "All") session$elapse(800); session$flushReact() expect_equal(tlg_list(), list("plot_a")) # muffled: rendering continues # notification captured via mockery::stub on showNotification, asserting it fired } ) })

Optional message tweak:

warning = function(w) { showNotification( paste0("Notice: ", conditionMessage(w)), type = "warning", duration = 10 ) invokeRestart("muffleWarning") }

Shaakon35 · 2026-07-16T12:15:39Z

[Error] Edit write-back may target the wrong column after trimming the display table.

(Posting as a general comment — inst/shiny/modules/tab_tlg.R L150–155 is unchanged context in this PR so an inline comment can't attach, but the risk is introduced by the display-column change on L~131.)

The Order Details table was trimmed to Type, Dataset, Output, Footnote, Stratification, Comment, but the edit handler still writes into the full tlg_order() frame:

new_tlg_order[new_tlg_order$Selection, ][info$row, info$column] <- info$value

The full frame still contains id, PKid, Link, Label, Description, Condition. If reactable_server() reports info$column as a positional index into the displayed frame, the write lands in the wrong column of the full frame. It's only safe if info$column is a column name.

Proposed resolution — resolve by name and guard against non-editable columns:

observeEvent(selected_tlg_state()$edit(), {
  info <- selected_tlg_state()$edit()

  # info$column may be a display-frame index; map to the full-frame column name
  editable_cols <- c("Footnote", "Stratification", "Comment")
  col_name <- if (is.numeric(info$column)) {
    names(displayed_order())[info$column]
  } else {
    info$column
  }
  req(col_name %in% editable_cols)  # never write to a non-editable column

  new_tlg_order <- tlg_order()
  new_tlg_order[new_tlg_order$Selection, ][info$row, col_name] <- info$value
  tlg_order(new_tlg_order)
})

Please confirm what reactable_server() returns for info$column and add a test that editing Footnote/Stratification/Comment updates the intended field (ties in with the untested-selection-logic comment).

…log' into 1335-enhancement-simplify-tlg-order-details-table-and-internalize-pcspec-filtering

… notice, fix search tabs

Gero1999 · 2026-07-21T15:39:56Z

@Shaakon35 bug is fixed feel free to approve! We just added a WISH feat for select-all / delete-all

…s (respecting search)

…log' into 1335-enhancement-simplify-tlg-order-details-table-and-internalize-pcspec-filtering # Conflicts: # DESCRIPTION # inst/shiny/modules/tab_tlg/tlg_module.R # tests/testthat/test-tab_tlg.R

Shaakon35

perfect thanks

h5hoang added 4 commits July 8, 2026 14:12

feat: hide internal Condition column and trim Order Details table to …

e2ea25c

…key columns

feat: surface urine PCSPEC/PPSPEC warnings as in-app notifications

d844503

feat: redesign Add-TLG picker as catalog checklist with dataset tabs,…

3b2dc6d

… search, downloads, and scoped select-all

docs: bump version to 0.1.0.9180 and add NEWS entries for #1335

1775466

h5hoang linked an issue Jul 8, 2026 that may be closed by this pull request

Enhancement: Simplify TLG Order Details table and internalize PCSPEC filtering #1335

Open

5 tasks

h5hoang changed the base branch from main to 1343-enhancement-expand-tlg-catalog July 10, 2026 19:36

h5hoang marked this pull request as ready for review July 10, 2026 19:38

docs: add XLSX to WORDLIST to fix spellcheck

0b2ea8a

h5hoang requested review from Gero1999, KOBANAK and Shaakon35 July 10, 2026 20:40

Gero1999 approved these changes Jul 14, 2026

View reviewed changes

Merge branch '1343-enhancement-expand-tlg-catalog' into 1335-enhancem…

f96512b

…ent-simplify-tlg-order-details-table-and-internalize-pcspec-filtering

Shaakon35 reviewed Jul 16, 2026

View reviewed changes

h5hoang added 6 commits July 17, 2026 14:12

Merge remote-tracking branch 'origin/1343-enhancement-expand-tlg-cata…

586b557

…log' into 1335-enhancement-simplify-tlg-order-details-table-and-internalize-pcspec-filtering

style: move Add-TLG modal CSS to SCSS partial with color tokens

00d480b

refactor: extract Add-TLG picker to helpers/JS, add edit guard + ADPP…

d438bb8

… notice, fix search tabs

fix: prefix surfaced render warnings with 'Notice:'

8820492

test: cover Add-TLG selection, edit write-back, and warning surfacing

8408680

chore: bump version to 0.1.0.9186 and update NEWS

30093ed

h5hoang added 3 commits July 23, 2026 13:21

fix: standardize Add-TLG picker select-all/clear-all to span both tab…

bec6b11

…s (respecting search)

chore: bump version to 0.1.0.9187 and update NEWS

92eb9a0

Merge remote-tracking branch 'origin/1343-enhancement-expand-tlg-cata…

ebb4723

…log' into 1335-enhancement-simplify-tlg-order-details-table-and-internalize-pcspec-filtering # Conflicts: # DESCRIPTION # inst/shiny/modules/tab_tlg/tlg_module.R # tests/testthat/test-tab_tlg.R

Shaakon35 approved these changes Jul 29, 2026

View reviewed changes

Navigation Menu

Uh oh!

Enhancement: SimplifyTLG Order Details Table, Internalize PCSPC Filtering - #1380

Enhancement: SimplifyTLG Order Details Table, Internalize PCSPC Filtering#1380
h5hoang wants to merge 15 commits into
1343-enhancement-expand-tlg-catalogfrom
1335-enhancement-simplify-tlg-order-details-table-and-internalize-pcspec-filtering

h5hoang commented Jul 8, 2026 •

edited

Loading

Uh oh!

Gero1999 left a comment

Uh oh!

Shaakon35 commented Jul 16, 2026 •

edited

Loading

Uh oh!

Shaakon35 Jul 16, 2026

Uh oh!

Shaakon35 Jul 16, 2026

Uh oh!

Shaakon35 Jul 16, 2026

Uh oh!

Shaakon35 Jul 16, 2026

Uh oh!

Shaakon35 commented Jul 16, 2026

Uh oh!

Gero1999 commented Jul 21, 2026

Uh oh!

Shaakon35 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

h5hoang commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Definition of Done

How to test

Contributor checklist

Notes to reviewer

Uh oh!

Gero1999 left a comment

Choose a reason for hiding this comment

Uh oh!

Shaakon35 commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shaakon35 Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Shaakon35 Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Shaakon35 Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Shaakon35 Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Shaakon35 commented Jul 16, 2026

Uh oh!

Gero1999 commented Jul 21, 2026

Uh oh!

Shaakon35 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

h5hoang commented Jul 8, 2026 •

edited

Loading

Shaakon35 commented Jul 16, 2026 •

edited

Loading