Skip to content

fix: R script and settings export missing volume unit simplification (#1190) #1228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Gero1999 wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

fix: R script and settings export missing volume unit simplification (#1190) #1228

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6a3272d
refactor: detect unit changes by comparing PPSTRESU vs PPORRESU
Gero1999 Apr 15, 2026
f750289
refactor: remove default flag from units table modal open logic
Gero1999 Apr 15, 2026
dc64958
refactor: remove default column from reactable and edit handler
Gero1999 Apr 15, 2026
be27b41
refactor: remove default flag filtering from settings export
Gero1999 Apr 15, 2026
339b559
refactor: remove default flag from global units_table observer
Gero1999 Apr 15, 2026
aa406cd
cleanup: remove residual default column guards
Gero1999 Apr 15, 2026
b66eec9
chore: bump version to 0.1.0.9150 and update NEWS.md
Gero1999 Apr 15, 2026
4b4de94
fix: auto-populate units_table from simplified PKNCAdata units
Gero1999 Apr 15, 2026
5b9b913
fix: exclude NA rows from units change detection
Gero1999 Apr 15, 2026
e372eb1
address if the user loads new data with no unit diffs after previous…
Gero1999 Apr 15, 2026
f4f71d2
fix: store full units table internally, filter to changed rows on export
Gero1999 Apr 20, 2026
d8b6875
fix: merge imported units into full data-derived table on settings up…
Gero1999 Apr 20, 2026
a6de2cf
Merge branch 'main' into 1190-fix/units-table-detect-changes-by-value
Gero1999 Apr 22, 2026
9d9f9c4
Bump version to 0.1.0.9153
Gero1999 Apr 22, 2026
8b53792
refactor: reduce cyclomatic complexity of tab_nca_server
Gero1999 Apr 27, 2026
1f311a3
Merge branch 'main' into 1190-fix/units-table-detect-changes-by-value
Gero1999 Apr 28, 2026
f84ee0f
Merge remote-tracking branch 'origin/main' into 1190-fix/units-table-…
Gero1999 Apr 29, 2026
840102e
revert: remove auto-replay system, keep only units-table changes
Gero1999 Apr 29, 2026
c511bd1
Merge main into 1190-fix/units-table-detect-changes-by-value
Gero1999 Apr 30, 2026
5b501d3
fix: isolate settings_override read in units auto-populate observer
Gero1999 Apr 30, 2026
6b6a464
Merge main into branch
Gero1999 May 4, 2026
6b0dcfd
refactor: extract .sync_units_table to reduce tab_nca_server complexity
Gero1999 May 8, 2026
e6ebd6d
fix: simplify .sync_units_table to avoid auto-replay interference
Gero1999 May 8, 2026
a57dcc4
fix: populate units_table lazily inside res_nca instead of observer
Gero1999 May 8, 2026
e94ce34
fix: decouple units_table from settings() reactive to prevent debounc…
May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: aNCA
Title: (Pre-)Clinical NCA in a Dynamic Shiny App
Version: 0.1.0.9149
Version: 0.1.0.9150
Authors@R: c(
person("Ercan", "Suekuer", email = "ercan.suekuer@roche.com", role = "aut",
comment = c(ORCID = "0009-0001-1626-1526")),
Expand Down
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
* Settings file now outputs and time duplicate exclusions and processes them automatically upon settings upload (#1195)

## Bugs fixed
* R script and settings export now include volume unit simplifications. Unit change detection uses value comparison (`PPSTRESU != PPORRESU`) instead of an edit-tracking flag, so automatic simplifications (e.g. `mg*L/mL` → `mg`) are captured alongside user edits (#1190)
* SASS compilation moved from runtime (`app.R`) to a `data-raw/compile_css.R` script, fixing startup crashes on read-only deployments (#1107)
* ZIP folder with results will now include the exploration tab outputs: individual plots, mean plots (#794)
* Updated TMAX label from Time of CMAX to Time of CMAX Observation (#787)
Expand Down
7 changes: 2 additions & 5 deletions inst/shiny/functions/zip-utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,11 +523,8 @@ prepare_export_files <- function(target_dir,
.export_settings <- function(target_dir, session) {
settings_list <- session$userData$settings()

if (!is.null(settings_list$units)) {
settings_list$units <- settings_list$units %>%
dplyr::filter(!default) %>%
dplyr::select(-default)
}
# units table from session$userData$units_table() already contains
# only changed rows (PPSTRESU != PPORRESU), no filtering needed.

settings_list$ratio_table <- session$userData$ratio_table()

Expand Down
18 changes: 15 additions & 3 deletions inst/shiny/modules/tab_nca.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,20 @@ tab_nca_server <- function(id, pknca_data, extra_group_vars, settings_override)
#' should respect the units, regardless of location.
session$userData$units_table <- reactiveVal(NULL)

# Keep units_table synchronized with the current dataset. Store the full
# units table so downstream code never receives a partial units table, and
# clear it when no units are available to avoid stale values persisting
# across uploads.
observeEvent(pknca_data(), {

@Shaakon35 Shaakon35 Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This observer fires on every pknca_data() invalidation and unconditionally overwrites units_table with the data-derived defaults. Meanwhile, nca_setup.R:175 has a bindEvent(processed_pknca_data(), once = TRUE) observer that merges imported settings into units_table.

If both observers flush in the same cycle (which happens when pknca_data() and processed_pknca_data() invalidate together during settings import), the execution order is undefined. If this observer runs after the settings import observer, it overwrites the merged units with plain defaults — silently discarding the imported unit overrides.

Two possible fixes:

Option A — priority ordering:

# tab_nca.R: run first (higher priority number = runs first)
observeEvent(pknca_data(), {
  ...
  session$userData$units_table(current_pknca_data$units)
}, ignoreNULL = FALSE, priority = 10)

# nca_setup.R: run second, overwrites with merged result
# (default priority = 0, so it runs after priority = 10... but actually
#  higher priority runs first in Shiny, so this would be wrong)

Actually, Shiny runs higher-priority observers first. So set this observer to a lower priority so it runs first, then the settings import observer runs after and overwrites with the merged result:

# This observer: set low priority so it runs early
observeEvent(pknca_data(), {
  ...
}, ignoreNULL = FALSE, priority = -10)

Option B (preferred) — skip when settings import is pending:

observeEvent(pknca_data(), {
  current_pknca_data <- pknca_data()
  if (is.null(current_pknca_data) || is.null(current_pknca_data$units)) {
    session$userData$units_table(NULL)
    return()
  }
  # Don't overwrite if a settings import already populated the table
  # with user-customized units for this dataset
  if (is.null(session$userData$units_table())) {
    session$userData$units_table(current_pknca_data$units)
  }
}, ignoreNULL = FALSE)

Option B is simpler and avoids the priority dance. The settings import observer will still merge on top when processed_pknca_data() fires, and on re-upload units_table is cleared to NULL first (line 98-99), so the guard passes correctly.

current_pknca_data <- pknca_data()

if (is.null(current_pknca_data) || is.null(current_pknca_data$units)) {
session$userData$units_table(NULL)
return()
}

session$userData$units_table(current_pknca_data$units)
}, ignoreNULL = FALSE)

@Shaakon35 Shaakon35 Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add a blank line between the observeEvent block and the adnca_data reactive definition for readability — matches the spacing pattern used elsewhere in this file.

adnca_data <- reactive(pknca_data()$conc$data)

# #' NCA Setup module
Expand Down Expand Up @@ -136,9 +150,7 @@ tab_nca_server <- function(id, pknca_data, extra_group_vars, settings_override)
# Update units table
processed_pknca_data <- processed_pknca_data()
if (!is.null(session$userData$units_table())) {
custom_units <- select(
session$userData$units_table(), -any_of("default")
)
custom_units <- session$userData$units_table()
by_cols <- intersect(names(processed_pknca_data$units), names(custom_units))
by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor"))
processed_pknca_data$units <- rows_update(
Expand Down
5 changes: 0 additions & 5 deletions inst/shiny/modules/tab_nca/nca_setup.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,11 +228,6 @@ nca_setup_server <- function(id, data, adnca_data, extra_group_vars, settings_ov
},
content = function(con) {
export_settings <- final_settings()
if (!is.null(export_settings$units)) {
export_settings$units <- export_settings$units %>%
filter(!default) %>%
select(-default)
}
export_settings$ratio_table <- ratio_table()
payload <- list(
settings = export_settings,
Expand Down
20 changes: 8 additions & 12 deletions inst/shiny/modules/tab_nca/units_table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,12 @@ units_table_server <- function(id, mydata) {

modal_units_table <- reactiveVal(NULL)
observeEvent(input$open_units_table, {
default_units <- mydata()$units %>%
dplyr::mutate(default = TRUE)
default_units <- mydata()$units

if (!is.null(session$userData$units_table())) {
custom_units <- dplyr::mutate(session$userData$units_table(), default = FALSE)
custom_units <- session$userData$units_table()
by_cols <- intersect(names(default_units), names(custom_units))
by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor", "default"))
by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor"))
dplyr::rows_update(
default_units,
custom_units,
Expand Down Expand Up @@ -102,8 +101,7 @@ units_table_server <- function(id, mydata) {
PPORRESU = colDef(name = "Default Unit"),
PPSTRESU = colDef(name = "Custom Unit"),
conversion_factor = colDef(name = "Conversion Factor"),
is_hidden = colDef(show = FALSE),
default = colDef(show = FALSE)
is_hidden = colDef(show = FALSE)
),
pagination = FALSE,
filterable = TRUE,
Expand Down Expand Up @@ -156,7 +154,6 @@ units_table_server <- function(id, mydata) {
)
}

modal_units_table[info$row, "default"] <- FALSE
modal_units_table[info$row, "conversion_factor"] <- conversion_factor_value
}

Expand Down Expand Up @@ -193,7 +190,7 @@ units_table_server <- function(id, mydata) {

log_trace("Applying custom units specification.")
modal_units_table() %>%
dplyr::filter(!default) %>%
dplyr::filter(!is.na(PPSTRESU), !is.na(PPORRESU), PPSTRESU != PPORRESU) %>%

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The save handler now persists only rows where PPSTRESU != PPORRESU. This drops user changes where only conversion_factor was edited (allowed via editable = c("PPSTRESU", "conversion_factor")) while keeping the unit label unchanged, so those changes won’t be exported/applied. Consider treating a row as changed when either PPSTRESU != PPORRESU OR conversion_factor != 1 (with appropriate NA handling).

Suggested change
dplyr::filter(!is.na(PPSTRESU), !is.na(PPORRESU), PPSTRESU != PPORRESU) %>%
dplyr::filter(
(!is.na(PPSTRESU) & !is.na(PPORRESU) & PPSTRESU != PPORRESU) |
dplyr::coalesce(conversion_factor, 1) != 1
) %>%

Copilot uses AI. Check for mistakes.
session$userData$units_table()

# Close the modal message window for the user
Expand All @@ -202,12 +199,11 @@ units_table_server <- function(id, mydata) {

#' Update local `modal_units_table()` if the global value changes.
observeEvent(session$userData$units_table(), {
default_units <- mydata()$units %>%
dplyr::mutate(default = TRUE)
default_units <- mydata()$units

custom_units <- dplyr::mutate(session$userData$units_table(), default = FALSE)
custom_units <- session$userData$units_table()
by_cols <- intersect(names(default_units), names(custom_units))
by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor", "default"))
by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor"))
dplyr::rows_update(
default_units,
custom_units,
Expand Down
Loading