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

[Gero1999]

Issue

Closes #1190

Description

The volume unit simplification applied in tab_data.R (e.g. mg*L/mL → mg) was not being captured in the exported settings or R script. This happened because session$userData$units_table() only stored rows that the user explicitly edited in the Units modal — it used a default flag to track which rows were touched, and the automatic simplification never set that flag.

Root cause: The default column (added in #900 / PR #946) tracked edits by flagging rows as default = FALSE when the user clicked on them in the modal. Automatic changes (like the volume simplification) mutated the PKNCAdata object directly without going through the modal, so they were never flagged.

Fix: Two-part approach:

1. Internally, session$userData$units_table() now stores the full units table (auto-populated from pknca_data()$units on first NCA run, updated with user edits from the modal). This is required because nca_results.R replaces res$data$units with it and joins it to results.

2. On export (settings YAML download and ZIP), the table is filtered to only rows where PPSTRESU != PPORRESU. This preserves the compact settings format from Enhancement: unit settings only manual changes and not entire units table #900 — only changed units appear in the YAML. On import, the partial rows are merged back into the full data-derived table via rows_update.

The default column is fully removed — change detection is now value-based.

Additionally, units_table was decoupled from the settings() reactive to prevent a debounce cascade that caused the "Restoring session..." modal to hang during auto-replay. The settings() reactive previously included units = session$userData$units_table(), which meant any units_table change invalidated settings → settings_debounced (2500ms) → processed_pknca_data → pknca_data_debounced (1000ms). Since the branch defers units_table population until after processed_pknca_data first evaluates, this created a second debounce cycle that pushed auto-replay beyond the 10s safety timeout. Export paths now read session$userData$units_table() directly.

Changes

1. Auto-populate (tab_nca.R): res_nca else branch stores full processed_pknca_data$units into session$userData$units_table() on first NCA run
2. Modal save (units_table.R): Stores full modal table (no filtering)
3. Modal open / observer (units_table.R): Removed default column from merge logic
4. Reactable / edit handler (units_table.R): Removed hidden default column and default <- FALSE on edit
5. Settings download (nca_setup.R): Reads units_table directly; filters to PPSTRESU != PPORRESU before export
6. ZIP export (zip-utils.R): Reads units_table directly; same filter before export
7. Settings import (nca_setup.R): Merges partial imported units into full data-derived table via rows_update
8. NCA run (tab_nca.R): Removed select(-any_of("default")) guard
9. Decouple from settings (settings.R): Removed units = session$userData$units_table() from settings() reactive to prevent debounce cascade during auto-replay

Definition of Done

• Volume unit simplification is included in session$userData$units_table()
• Settings YAML export contains only changed units (PPSTRESU != PPORRESU)
• Settings YAML import merges partial units into full data-derived table
• R script receives correct units via custom_units_table
• User edits in the Units modal still work correctly
• default column fully removed from the units table flow
• Auto-replay with settings YAML restores session without hanging
• Manual verification: R script produces same units as the Shiny app
• Manual verification: settings upload/download round-trip preserves units

How to test

1. Auto-replay / session restore (the main fix)

1. Upload a dataset and run NCA
2. Export the settings YAML
3. Reload the app, upload the same dataset + settings YAML simultaneously
4. Verify the "Restoring session..." modal dismisses and NCA results appear
5. Repeat with a YAML that has nca_ran: no — verify the modal dismisses after navigating to the saved tab

2. Units round-trip

1. Upload a dataset with volume-based parameters (e.g. Vd, Vss)
2. Run NCA — note the simplified units in the results (e.g. mg*L/mL → mg)
3. Export the settings YAML — verify it contains only changed units (PPSTRESU != PPORRESU), not the full table
4. Re-upload the settings YAML on the same or different dataset — verify the simplified units are applied correctly
5. Run NCA again — verify results match

3. Units modal

1. Open the Units modal, change a unit manually, save
2. Run NCA — verify the change is reflected in results
3. Export settings YAML — verify the manual change appears
4. Re-open the Units modal — verify the manual change persists
5. Change the unit back to the default — verify it disappears from the exported YAML

4. R script export

1. Run NCA in the Shiny app
2. Export the R script — verify custom_units_table contains the simplified units
3. Run the R script — verify the output units match the Shiny app

5. ZIP export

1. Run NCA, export the ZIP
2. Check the settings YAML inside the ZIP — verify only changed units are included

Contributor checklist

• Code passes lintr checks
• Code passes all unit tests
• New logic covered by unit tests
• New logic is documented
• App or package changes are reflected in NEWS
• Package version is incremented
• R script works with the new implementation (if applicable)
• Settings upload works with the new implementation (if applicable)
• If any .scss change was done, run data-raw/compile_css.R

Notes to reviewer

• The default column was introduced in PR Enhancement: settings file saves only edited units #946 (issue Enhancement: unit settings only manual changes and not entire units table #900). This PR removes it entirely in favor of value-based detection.
• session$userData$units_table() stores the full table internally (needed by nca_results.R), but exports are filtered to only changed rows (preserving Enhancement: unit settings only manual changes and not entire units table #900's compact format).
• units was removed from the settings() reactive because it only served export purposes and created a reactive dependency that caused auto-replay hangs. Export paths now read session$userData$units_table() directly.
• Edge case: if a user sets PPSTRESU back to equal PPORRESU, that row is correctly treated as unchanged and excluded from export.
• NA handling: rows where PPSTRESU or PPORRESU is NA are excluded from export filters to avoid .na.character entries in the YAML.

[Copilot]

Pull request overview

Fixes an export mismatch in the Shiny NCA workflow where automatic unit simplifications (not performed via the Units modal) were not reflected in exported settings or generated R scripts.

Changes:

• Replace the previous edit-tracking default flag with value-based detection of unit changes (PPSTRESU != PPORRESU).
• Remove default column handling across Units modal plumbing and export paths.
• Add an observer to auto-populate session$userData$units_table() from pknca_data() when units differ from defaults.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
inst/shiny/modules/tab_nca/units_table.R	Removes default flag usage; switches to value-based “changed unit” filtering.
inst/shiny/modules/tab_nca/nca_setup.R	Removes export-time filtering/stripping of default from units settings.
inst/shiny/modules/tab_nca.R	Auto-populates units_table based on PPSTRESU != PPORRESU; removes default drop guard.
inst/shiny/functions/zip-utils.R	Stops filtering units by default during ZIP settings export.
NEWS.md	Documents the bug fix and the new change-detection behavior.
DESCRIPTION	Bumps package version.

Comments suppressed due to low confidence (3)

inst/shiny/modules/tab_nca/units_table.R:39

• dplyr::rows_update() can error if custom_units contains columns not present in default_units (e.g., older settings uploads or other sources adding extra cols). Elsewhere the codebase defensively subsets the update table to shared columns before calling rows_update() (see R/PKNCA.R where custom_units_table[, common_cols, drop = FALSE] is used). Consider applying the same pattern here by restricting custom_units to intersect(names(default_units), names(custom_units)) before the update.

      if (!is.null(session$userData$units_table())) {
        custom_units <- session$userData$units_table()
        by_cols <- intersect(names(default_units), names(custom_units))
        by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor"))
        dplyr::rows_update(
          default_units,
          custom_units,
          by = by_cols,
          unmatched = "ignore"

inst/shiny/modules/tab_nca/units_table.R:212

• Same as above: rows_update() is passed custom_units without trimming to shared columns. To avoid runtime errors when session$userData$units_table() includes extra columns (e.g., from uploaded settings), subset custom_units to the shared column set before updating default_units (consistent with R/PKNCA.R).

      custom_units <- session$userData$units_table()
      by_cols <- intersect(names(default_units), names(custom_units))
      by_cols <- setdiff(by_cols, c("PPSTRESU", "conversion_factor"))
      dplyr::rows_update(
        default_units,
        custom_units,
        by = by_cols,
        unmatched = "ignore"
      ) %>%

inst/shiny/modules/tab_nca.R:160

• When applying custom_units via rows_update(), consider subsetting custom_units to the shared column set (intersect(names(processed_pknca_data$units), names(custom_units))) before updating. This matches the defensive pattern in R/PKNCA.R and avoids potential errors if session$userData$units_table() ever carries extra columns (e.g., from settings upload).

          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(
            processed_pknca_data$units,
            custom_units,
            by = by_cols,
            unmatched = "ignore"

[Copilot]

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
	) %>%

[js3110]

The settings yaml now contains the full units table instead of only the changed units, which reverts the feature introduced in #900 . While I agree that just checking for manual user changes is not the best approach, we should still only show the changed units (ie PPORRESU != PPSTRESU) in the settings file.

[js3110]

Its better than before, but now the settings show parameters that aren't currently selected or in the modal units table:

(I only had defaults selected)

While it doesn't disrupt any of the function of the app and settings upload, it would be preferable if only parameters actually selected are included in the units table...

[Gero1999]

hey @js3110 notice is showing all custom units (VSS parameters). This needs to be like that so in the case users decide to choose them later or for another study they use those preferred units and not the standard ones

Maybe would make sense that from now on, the open source stops doing the automatic -> mL transformation, and in the other version we just add these setting details

[js3110]

hey @js3110 notice is showing all custom units (VSS parameters). This needs to be like that so in the case users decide to choose them later or for another study they use those preferred units and not the standard ones

Maybe would make sense that from now on, the open source stops doing the automatic -> mL transformation, and in the other version we just add these setting details

Hmm so you're saying that if I uploaded settings without these additional units, then the other parameters would not be converted to mL? If they aren't selected in settings surely the unit will just be the defualt (which is with the mL conversion)

Is there any update on PKNCA progress to do this transformation automatically?

[js3110]

@Gero1999 I tried uploading the settings file but with the additional Vol params removed from the settings, and it converts them to mL anyway, so I don't think its an issue...

[Gero1999]

@js3110 It might work for the VSS parameters because we have hardcoded them, but this is not the point I am trying to make. The point is that the units table and the selected parameters are independent elements.

Imagine a team has their own standards, and they always do CMAX in g/L. Even if at the start of study1 they did not calculate CMAX they may want to do it later on, and keep the standards they always use. This is where the settings keeping track of the units preferences is preferred

[js3110]

@js3110 It might work for the VSS parameters because we have hardcoded them, but this is not the point I am trying to make. The point is that the units table and the selected parameters are independent elements.

Imagine a team has their own standards, and they always do CMAX in g/L. Even if at the start of study1 they did not calculate CMAX they may want to do it later on, and keep the standards they always use. This is where the settings keeping track of the units preferences is preferred

but if they didn't select CMAX then they wouldn't have been able to change the unit because the units table only shows selected parameters? I don't think I understand your point?
If they want to set CMAX units then they can add it themselves to the settings file manually, or they select CMAX as a parameter and then change the units, in which case it would be exported to the settings file

[Gero1999]

My point is that most of the times people will work with their own settings file as we do to include their specific HL-rules, units parameters... So they don't have to worry about manually interacting with the App for those little standardized things

[Shaakon35]

Looks good, did you fix only the urine units? because simflification could happen also here:

Does this issue also fix this?
#1145

[Shaakon35]

**How to test** Re-upload the settings YAML on the same or different dataset — verify the simplified units are applied correctly

-> Bug, here it loads forever
please try with:
settings.yaml
(It works on the main, but not on this branch)

[Shaakon35]

I guess we should fix this in another issue?
ATPTREF and ROUTE are not available via the cdisc_dataset$adpp script:

[Shaakon35]

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.

[Shaakon35]

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.

[Gero1999]

Looks good, did you fix only the urine units? because simflification could happen also here:

Does this issue also fix this? #1145

We can generalize #1145 to address this, but it isn't related with this PR per ce. Regarding if the current PR will adapt to that, yes it will work fine once solved

However, the normalized by Dose I would not change them personally

[Gero1999]

@Shaakon35 @js3110 we might need to solve this bug to prevent Ola from having the issue

[js3110]

LGTM

[Shaakon35]

#1228 (comment)
#1228 (comment) (I opened another issue: #1276)

My two comments were not addressed

[Shaakon35]

@KOBANAK In case you take over this, make sure this works: #1228 (comment)

[Gero1999]

Right now all is working, but in the settings file the units table is showing completely... which is against our interest