Skip to content

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

Merged
Gero1999 merged 17 commits into
1210-enhancement/replace-logger-with-consolefrom
1219-enhancement/nca-input-log-points
Apr 29, 2026
Merged

feat: add log-points for NCA user input changes#1221
Gero1999 merged 17 commits into
1210-enhancement/replace-logger-with-consolefrom
1219-enhancement/nca-input-log-points

Conversation

@Gero1999

@Gero1999 Gero1999 commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Issue

Closes #1219

Description

Adds INFO-level logging for user input changes across the NCA setup modules so that exported session_log.txt files capture what configuration choices the user made — not just that a calculation happened.

New log-points added:

  • Settings — analyte, specimen, profile, extrapolation method, min half-life points, flag rule enable/disable and threshold changes
  • Data Imputation — BLQ strategy changes, C0 imputation toggle
  • Parameter Selection — full parameter list at DEBUG level (count was already logged at INFO)
  • Slope Selector — rule add/remove upgraded from TRACE→INFO, DEBUG summary of rule counts on table change
  • General Exclusions — exclusion add (with row count, type, reason), removal, settings restore

Definition of Done

  • Log messages appear in the R console when users change NCA settings, parameters, slope rules, or exclusions
  • Messages are included in the ZIP-exported session_log.txt
  • No logging on reactive invalidation noise — only meaningful user actions

How to test

  1. Run aNCA::run_app()
  2. Upload data and navigate to the NCA tab
  3. Change settings (method, analyte, specimen, flag rules, etc.)
  4. Add/remove slope rules and exclusions
  5. Verify log messages appear in the R console for each action
  6. Export ZIP and check session_log.txt contains the entries

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

This PR targets 1210-enhancement/replace-logger-with-console (#1212), not main. It will auto-retarget to main once #1212 is merged.

All log calls use the same log_info/log_debug functions introduced in #1212 — no new dependencies.

Version bump and NEWS entry still needed before merge.

Gero1999 and others added 5 commits April 14, 2026 10:03
Log analyte, specimen, profile, method, min half-life points,
and flag rule changes at INFO level.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Log the full list of selected parameters at DEBUG level
alongside the existing INFO count message.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Upgrade slope rule add/remove from TRACE to INFO level.
Add DEBUG summary of slope rules count on table changes.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Log exclusion add (with row count, type, reason), removal,
and settings restore at INFO level.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Log NCA profile changes, BLQ strategy changes, and C0
imputation toggle at INFO level.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
@Gero1999 Gero1999 marked this pull request as ready for review April 14, 2026 13:00
@Gero1999 Gero1999 marked this pull request as draft April 14, 2026 13:01
Gero1999 and others added 7 commits April 14, 2026 13:20
Replace all glue-style log calls ({var}) with paste-style
(multiple arguments) to avoid parent.frame(2) scoping issues
in closures and observeEvent callbacks. Guard profile log
against NA values.

Co-authored-by: Ona <no-reply@ona.com>
Log analyte, specimen, profile, method, min half-life points,
and flag rule changes at INFO level.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Log the full list of selected parameters at DEBUG level
alongside the existing INFO count message.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Upgrade slope rule add/remove from TRACE to INFO level.
Add DEBUG summary of slope rules count on table changes.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Log exclusion add (with row count, type, reason), removal,
and settings restore at INFO level.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Log NCA profile changes, BLQ strategy changes, and C0
imputation toggle at INFO level.

Closes #1219 (partial)

Co-authored-by: Ona <no-reply@ona.com>
Replace all glue-style log calls ({var}) with paste-style
(multiple arguments) to avoid parent.frame(2) scoping issues
in closures and observeEvent callbacks. Guard profile log
against NA values.

Co-authored-by: Ona <no-reply@ona.com>
@Gero1999 Gero1999 force-pushed the 1219-enhancement/nca-input-log-points branch from 91ca90b to 893084b Compare April 14, 2026 15:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds INFO/DEBUG logging across the Shiny NCA setup modules so exported session_log.txt captures user configuration changes (settings, imputation, parameter selection, slope rules, exclusions), improving post-hoc traceability of an NCA run.

Changes:

  • Added log points for selection changes (analyte/specimen/profile, extrapolation method, min half-life points, flag rules).
  • Added log points for BLQ and C0 imputation setting changes.
  • Added more detailed logging for slope rule and exclusion add/remove/restore actions, plus DEBUG summaries.

Reviewed changes

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

Show a summary per file
File Description
inst/shiny/modules/tab_nca/setup/slope_selector.R Adds DEBUG summary log when manual slope rules table updates.
inst/shiny/modules/tab_nca/setup/settings.R Adds INFO logs for key NCA settings and flag-rule changes.
inst/shiny/modules/tab_nca/setup/parameter_selection.R Adds DEBUG log listing selected parameters per study type.
inst/shiny/modules/tab_nca/setup/manual_slopes_table.R Upgrades add/remove slope rule logs from TRACE to INFO.
inst/shiny/modules/tab_nca/setup/general_exclusions.R Adds INFO logs for exclusions restored/added/removed.
inst/shiny/modules/tab_nca/setup/data_imputation.R Adds INFO logs for BLQ strategy and C0 imputation toggle changes.

Comment on lines +291 to +293
if (length(target_profile) > 0 && !anyNA(target_profile)) {
log_info("NCA profile selection changed: ", paste(target_profile, collapse = ", "))
}
Comment on lines +303 to +307
@@ -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 = ", "))
Comment on lines +339 to +343
@@ -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 = ", "))
Comment on lines 160 to 177
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
)
# 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")
# 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")
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)")
Replace separate add/remove INFO messages with a single INFO
summary that fires on any slope table change (button clicks,
plotly point selections, cell edits). Demote add/remove to TRACE.

Co-authored-by: Ona <no-reply@ona.com>
@Gero1999 Gero1999 requested a review from Shaakon35 April 22, 2026 12:25
@Gero1999 Gero1999 assigned js3110 and unassigned js3110 Apr 22, 2026
@Gero1999 Gero1999 assigned js3110 and unassigned js3110 Apr 22, 2026
@Gero1999 Gero1999 requested a review from js3110 April 22, 2026 12:26
@Gero1999 Gero1999 marked this pull request as ready for review April 22, 2026 12:26

@js3110 js3110 left a comment

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.

Looks good, but there seems to be a lag on some of the widgets, especially the analyte pcspec and profile widgets- the log only appears after I change something else, not when I change the condition

- Suppress analyte/specimen/profile logs during initial data load
  using a filters_initialized guard
- Move profile log to dedicated observeEvent(input$select_profile)
  with ignoreInit = TRUE
- Move exclusion-add logging inside .handle_add_exclusion to avoid
  duplicating input reads
- Use 'selections' instead of 'inclusions' in slope log to match
  the UI terminology (TYPE == 'Selection')

Co-authored-by: Ona <no-reply@ona.com>
@Gero1999

Copy link
Copy Markdown
Collaborator Author

@js3110 reactive is now on time also for those, let me know if there are others

Comment thread inst/shiny/modules/tab_nca/setup/slope_selector.R
Comment thread inst/shiny/modules/tab_nca/setup/parameter_selection.R

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

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.

ignoreInit = TRUE prevents firing on first render, but input$method is still updated programmatically during settings restore via .restore_non_filter_settingsupdateSelectInput(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.

@Shaakon35

Copy link
Copy Markdown
Collaborator

Finding 6 — No unit tests for log output

The "New logic covered by unit tests" checklist item is unchecked. Unit testing Shiny observeEvent log calls is difficult without a test harness for the modules, so this isn't a blocker. However, if the logging functions (log_info, log_debug, log_trace) from #1212 support a capture/sink mechanism, a few smoke tests verifying that key log messages are emitted (e.g., via capture.output or a custom log collector) would add confidence against silent regressions.

Suggested approach if feasible:

it("logs analyte selection change", {
  # Use shiny::testServer or capture.output around the log function
  msgs <- capture.output(log_info("Analyte selection changed: AnalyteA"), type = "message")
  expect_true(any(grepl("Analyte selection changed", msgs)))
})

Not blocking — just noting for consideration.

Comment thread inst/shiny/modules/tab_nca/setup/manual_slopes_table.R
- Convert remaining glue-style log calls to paste-style
  (parameter_selection.R log_info, manual_slopes_table.R log_trace)
- Add filters_initialized guard to method and min_hl_points
  observers to suppress logs during settings restore

Co-authored-by: Ona <no-reply@ona.com>

@js3110 js3110 left a comment

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.

Slope rules updates too many times- gives the same message wehn I:

  • add exlcusion
  • add reason (to same exclusion) (where it gives message 3 times)
image

Add user_changed_slopes flag to only log slope rule changes
triggered by user actions (plotly clicks, add/remove buttons),
not by settings restore or initial render.

Co-authored-by: Ona <no-reply@ona.com>
@Gero1999

Copy link
Copy Markdown
Collaborator Author

@js3110 I think this one finally fixes it. It will show the message only whenever you make a change on the table (1 time if you just add a reason)

@js3110 js3110 left a comment

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.

much better, thank you!

@Gero1999 Gero1999 requested review from Shaakon35 and bachapman April 28, 2026 08:39
@Gero1999 Gero1999 merged commit bc1bc80 into 1210-enhancement/replace-logger-with-console Apr 29, 2026
1 check was pending
@Gero1999 Gero1999 deleted the 1219-enhancement/nca-input-log-points branch April 29, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants