Skip to content

feat: add log-points for NCA user input changes #1221

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

Open
Gero1999 wants to merge 16 commits into
base: 1210-enhancement/replace-logger-with-console
Choose a base branch
from
Open

feat: add log-points for NCA user input changes #1221

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
41b9403
feat: add log-points for NCA settings changes
Gero1999 Apr 14, 2026
5922af8
feat: add debug log-point for selected parameter names
Gero1999 Apr 14, 2026
7ce0384
feat: add log-points for slope selector user actions
Gero1999 Apr 14, 2026
ef5b7de
feat: add log-points for general exclusion changes
Gero1999 Apr 14, 2026
4a4b92d
feat: add log-points for profile selection and data imputation
Gero1999 Apr 14, 2026
91ca90b
fix: use paste-style logging to avoid glue parent.frame issues
Gero1999 Apr 14, 2026
47f533b
feat: add log-points for NCA settings changes
Gero1999 Apr 14, 2026
550e0bb
feat: add debug log-point for selected parameter names
Gero1999 Apr 14, 2026
b378504
feat: add log-points for slope selector user actions
Gero1999 Apr 14, 2026
1227387
feat: add log-points for general exclusion changes
Gero1999 Apr 14, 2026
dacc0c2
feat: add log-points for profile selection and data imputation
Gero1999 Apr 14, 2026
893084b
fix: use paste-style logging to avoid glue parent.frame issues
Gero1999 Apr 14, 2026
2f0b9d8
Merge branch '1219-enhancement/nca-input-log-points' of https://githu…
Gero1999 Apr 14, 2026
2a347e1
refactor: unify slope selector logging into single table-change message
Gero1999 Apr 20, 2026
1cb6244
fix: address review feedback on log-points
Gero1999 Apr 23, 2026
07a2d6e
fix: consistent paste-style logging and init guards
Gero1999 Apr 24, 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
9 changes: 9 additions & 0 deletions inst/shiny/modules/tab_nca/setup/data_imputation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,15 @@ data_imputation_server <- function(id, settings_override) {
}
})

observeEvent(input$select_blq_strategy, {
log_info("BLQ imputation strategy changed: ", input$select_blq_strategy)
}, ignoreInit = TRUE)

observeEvent(input$should_impute_c0, {
state <- if (isTRUE(input$should_impute_c0)) "enabled" else "disabled"
log_info("Start concentration imputation ", state)
}, ignoreInit = TRUE)

blq_imputation_rule <- reactive({
req(input$select_blq_strategy)
rule_list <- switch(input$select_blq_strategy,
Expand Down
17 changes: 17 additions & 0 deletions inst/shiny/modules/tab_nca/setup/general_exclusions.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ general_exclusions_server <- function(

xbtn_counter(max(new_ids))
exclusion_list(rehydrated_list)
log_info("Exclusions restored from settings: ", length(overrides), " rules loaded")
}
})

Expand All @@ -156,6 +157,21 @@ general_exclusions_server <- function(

# Add a new exclusion when the Add button is pressed
observeEvent(input$add_exclusion_reason, {
rows_sel <- getReactableState("conc_table-table", "selected")
reason <- input$exclusion_reason
nca_checked <- isTRUE(input$cb_manual_nca)
tlg_checked <- isTRUE(input$cb_tlg)

if (length(rows_sel) > 0 && nzchar(reason) && (nca_checked || tlg_checked)) {
type_label <- if (nca_checked && tlg_checked) "NCA + TLG"
else if (nca_checked) "NCA"
else "TLG"
log_info(
"Exclusion added: ", length(rows_sel), " rows, type=", type_label,
", reason='", reason, "'"
)
}

.handle_add_exclusion(
input, session, exclusion_list, xbtn_counter
)
Expand All @@ -167,6 +183,7 @@ general_exclusions_server <- function(
lapply(lst, function(item) {
xbtn_id <- item$xbtn_id
observeEvent(input[[xbtn_id]], {
log_info("Exclusion removed: reason='", item$reason, "'")
current <- exclusion_list()
exclusion_list(
Filter(function(x) x$xbtn_id != xbtn_id, current)
Expand Down
4 changes: 2 additions & 2 deletions inst/shiny/modules/tab_nca/setup/manual_slopes_table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ manual_slopes_table_server <- function(

# Add a new row to the table when the user clicks the add button
observeEvent(input$add_rule, {
log_trace("{id}: adding manual slopes row")
log_info("Slope selector: adding manual slope rule")
first_group <- slopes_pknca_groups()[1, ]
time_col <- pknca_data()$conc$columns$time
new_row <- cbind(
Expand Down Expand Up @@ -126,7 +126,7 @@ manual_slopes_table_server <- function(

# Remove selected rows from the table when the user clicks the remove button
observeEvent(input$remove_rule, {
log_trace("{id}: removing manual slopes row")
log_info("Slope selector: removing manual slope rule")
selected <- getReactableState("manual_slopes", "selected")
req(selected)
edited_slopes <- manual_slopes()[-selected, ]
Expand Down
3 changes: 3 additions & 0 deletions inst/shiny/modules/tab_nca/setup/parameter_selection.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,9 @@ parameter_selection_server <- function(id, processed_pknca_data, parameter_overr
n_params <- if (is.null(params)) 0 else length(params)

log_info("Parameter selection for '{study_type}': {n_params} parameters selected.")
if (n_params > 0) {
log_debug("Parameters for '", study_type, "': ", paste(params, collapse = ", "))
Copy link
Copy Markdown
Collaborator

@Shaakon35 Shaakon35 Apr 23, 2026

Choose a reason for hiding this comment

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

Inconsistent interpolation style. Line 289 (existing) uses glue-style:

log_info("Parameter selection for '{study_type}': {n_params} parameters selected.")

This new line uses paste-style:

log_debug("Parameters for '", study_type, "': ", paste(params, collapse = ", "))

If the base branch (#1212) defines log_info/log_debug as paste-based (no glue), then line 289 is the one that needs updating. Either way, pick one style and apply it consistently.

Suggested fix — assuming paste-style is the new convention:

log_info("Parameter selection for '", study_type, "': ", n_params, " parameters selected.")
if (n_params > 0) {
  log_debug("Parameters for '", study_type, "': ", paste(params, collapse = ", "))
}

}
}, ignoreNULL = FALSE, ignoreInit = TRUE)
})
})
Expand Down
29 changes: 29 additions & 0 deletions inst/shiny/modules/tab_nca/setup/settings.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,9 @@ settings_server <- function(id, data, adnca_data, settings_override) {
available_choices = profile_choices,
override_val = settings$profile
)
if (length(target_profile) > 0 && !anyNA(target_profile)) {
log_info("NCA profile selection changed: ", paste(target_profile, collapse = ", "))
}
updatePickerInput(
session, "select_profile",
choices = profile_choices, selected = target_profile
Expand All @@ -301,6 +304,8 @@ settings_server <- function(id, data, adnca_data, settings_override) {
updating_filters(TRUE)
on.exit(updating_filters(FALSE))

log_info("Analyte selection changed: ", paste(input$select_analyte, collapse = ", "))

settings <- .consume_settings()

all_pcspec <- unique(data()$PCSPEC) %>% na.omit()
Expand Down Expand Up @@ -335,6 +340,8 @@ settings_server <- function(id, data, adnca_data, settings_override) {
updating_filters(TRUE)
on.exit(updating_filters(FALSE))

log_info("Specimen selection changed: ", paste(input$select_pcspec, collapse = ", "))

settings <- .consume_settings()

all_analyte <- unique(data()$PARAM) %>% na.omit()
Expand All @@ -356,6 +363,15 @@ settings_server <- function(id, data, adnca_data, settings_override) {
.update_profile(settings)
})

# Log method and min half-life points changes
observeEvent(input$method, {
log_info("Extrapolation method changed: ", input$method)
}, ignoreInit = TRUE)
Copy link
Copy Markdown
Collaborator

@Shaakon35 Shaakon35 Apr 23, 2026

Choose a reason for hiding this comment

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

ignoreInit = TRUE prevents firing on first render, but input$method is still updated programmatically during settings restore via .restore_non_filter_settings → updateSelectInput(session, "method", ...). This will log "Extrapolation method changed" when the user uploads settings — which is a restore, not a user change.

The analyte, specimen, and profile observers already guard against this with filters_initialized(). Consider applying the same pattern here:

observeEvent(input$method, {
  if (filters_initialized()) {
    log_info("Extrapolation method changed: ", input$method)
  }
}, ignoreInit = TRUE)

observeEvent(input$min_hl_points, {
  if (filters_initialized()) {
    log_info("Min. half-life points changed: ", input$min_hl_points)
  }
}, ignoreInit = TRUE)

If logging during settings restore is intentional, add a comment noting that.


observeEvent(input$min_hl_points, {
log_info("Min. half-life points changed: ", input$min_hl_points)
}, ignoreInit = TRUE)

# Include keyboard limits for the settings GUI display

# Keyboard limits for the setting thresholds
Expand All @@ -365,6 +381,19 @@ settings_server <- function(id, data, adnca_data, settings_override) {
limit_input_value(input, session, "LAMZSPN_threshold", min = 0, lab = "LAMZSPN")
limit_input_value(input, session, "min_hl_points", max = 10, min = 2, lab = "Min. HL Points")

# Log flag rule changes
lapply(c("R2ADJ", "R2", "AUCPEO", "AUCPEP", "LAMZSPN"), function(flag) {
rule_id <- paste0(flag, "_rule")
threshold_id <- paste0(flag, "_threshold")
observeEvent(input[[rule_id]], {
state <- if (input[[rule_id]]) "enabled" else "disabled"
log_info("Flag rule ", flag, " ", state)
}, ignoreInit = TRUE)
observeEvent(input[[threshold_id]], {
log_info("Flag rule ", flag, " threshold changed: ", input[[threshold_id]])
}, ignoreInit = TRUE)
})

# Reactive value to store the partial intervals data table
# Define the parameters that can be used for partial area calculations
PARTIAL_INT_PARAMS <- metadata_nca_parameters %>%
Expand Down
5 changes: 5 additions & 0 deletions inst/shiny/modules/tab_nca/setup/slope_selector.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,11 @@ slope_selector_server <- function( # nolint
observeEvent(manual_slopes(), {
req(manual_slopes())

n_rules <- nrow(manual_slopes())
n_excl <- sum(manual_slopes()$TYPE == "Exclusion", na.rm = TRUE)
n_incl <- n_rules - n_excl
log_debug("Slope rules updated: ", n_rules, " total (", n_incl, " inclusions, ", n_excl, " exclusions)")

Copy link
Copy Markdown
Collaborator

@Shaakon35 Shaakon35 Apr 23, 2026

Choose a reason for hiding this comment

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

This log_info fires on every manual_slopes() invalidation — including programmatic updates (settings restore, initial render). The PR description states "No logging on reactive invalidation noise — only meaningful user actions," but this observer has no ignoreInit = TRUE and no guard against non-user-driven updates.

Suggested fix — track whether the user triggered the change via the add/remove buttons:

# Track user-initiated slope changes
user_changed_slopes <- reactiveVal(FALSE)

observeEvent(input$add_rule, {
  user_changed_slopes(TRUE)
  # ... existing add logic ...
})

observeEvent(input$remove_rule, {
  user_changed_slopes(TRUE)
  # ... existing remove logic ...
})

observeEvent(manual_slopes(), {
  req(manual_slopes())

  if (user_changed_slopes()) {
    n_rules <- nrow(manual_slopes())
    n_excl <- sum(manual_slopes()$TYPE == "Exclusion", na.rm = TRUE)
    n_incl <- n_rules - n_excl
    log_info("Slope rules updated: ", n_rules, " total (", n_incl, " selections, ", n_excl, " exclusions)")
    user_changed_slopes(FALSE)
  }

  # Update reactable with rules
  reactable::updateReactable(...)
})

Alternatively, move the log_info into the add_rule and remove_rule observers directly (after the table mutation), which avoids the guard entirely.

# Update reactable with rules
reactable::updateReactable(
outputId = "manual_slopes",
Expand Down