Simplify the logic in slope plots, add order option & allow download in ZIP

[Gero1999]

Description

This changes simplify the logic that makes the slope plots in order to make easier future implementations and also make able the download of all plots in a zip file easily. The advantages would be:

Features

• Slope order input. Users can group their plots in whatever way they want them to appear.

Enhancements

• All plots are generated, instead of only the rendered ones.
• The new function that generates plots is faster and more aligned with PKNCA (get_halflife_plot)
• Only modified plots are affected by changes, reducing unnecessary plot redraws
• Upstream, the PKNCA object is updated with the slope rules directly in setup.R, instead of in the post-processing of results. We no longer need to use filter_slopes
• We no longer use the custom is.included.hl or is.excluded.hl that then are used to build exclude_half.life. Instead we directly use the exclude_half.life & include_half.life columns
• Added more documentation and comments
• The new reactivity flow to create the plots that allows a simpler use of settings. We just simply need to udpate the dataset and/or manual slopes and everything will be auto-managed:

#' processed_pknca_data (input, from parent)
#'   └─> slope_selector_server
#'        ├──> plot_outputs (reactive, updated by processed_pknca_data changes)
#'        └──> ├> handle_plotly_click (updates manual_slopes on plot click)
#'                   └> handle_table_edits (updates manual_slopes on user edits & setting overrides)
#'                          └─> manual_slopes (output, used by parent to update processed_pknca_data)

Bugs fixed

• Pagination controls are less likely to break (unless you are a clicking psycho 💀)
• Exclusion/Inclusion of points or table edits won't reset subject search
• Units appear for the Y axis

Tests

• Setup FIXTURE_PKNCA_DATA obj will now contain the columns include_half.life, exclude_half.life

Definition of Done

Closes or contributes to #333 (I think with this the client interest is already satisfied)

• Grouping option for slopes plots
• Faceting so groups are all on one page

Closes #553

• Logic was simplified and code refactored
• All Slope Selector features work well without issues
• Make a folder Slope selector including its plots

How to test

How to test features not covered by unit tests.

Contributor checklist

• Code passes lintr checks
• Code passes all unit tests
• New logic covered by unit tests
• New logic is documented
• Package version is incremented

Notes to reviewer

Compare thoughtfully the feats of the plots to make sure I did not miss anything relevant.
Don't hesitate also to leave suggestions for refactoring or code comments that makes the code easier to read.

[js3110]

Hi,

I've noticed another couple of bugs:

1. If I add an exclusion, run NCA, then go back to setup and remove the exclusion, I get this error:
   Warning: Error in if: missing value where TRUE/FALSE needed

This happened when I added exclusion with double click, but removed with selecting from the table. I haven't tried other combinations but might be also worth checking.

2. If I run through the workflow quite fast, and add an exclusion, I clikc the run NCA buton then nothing happens. I wait a few seconds and click again then nothing happens, repeat. Then eventually it runs and it Runs NCA like 3 times. Has happened to me twice now so I know its a real bug- but hard to recreate unorganically...

[Copilot]

Pull Request Overview

This PR simplifies the slope selector logic for NCA half-life plots with significant architectural improvements. The changes replace the old lambda_slope_plot function with a new get_halflife_plots approach, streamline reactivity flows, and add plot ordering/grouping capabilities.

Key Changes:

• Introduced get_halflife_plots() to replace lambda_slope_plot(), improving performance and alignment with PKNCA patterns
• Refactored slope rule application to occur directly in PKNCA_update_data_object() via update_pknca_with_rules()
• Added plot ordering UI via orderInput and arrange_plots_by_groups()
• Implemented change detection (detect_pknca_data_changes()) to minimize unnecessary plot regeneration
• Removed deprecated columns is.included.hl and is.excluded.hl in favor of include_half.life and exclude_half.life

Reviewed Changes

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

Show a summary per file

File	Description
R/get_halflife_plots.R	New function replacing lambda_slope_plot with streamlined half-life plotting
R/utils-slope_selector.R	Refactored utilities including change detection, plot updates, and rule application
R/PKNCA.R	Added checks_before_running_nca() and hl_adj_rules parameter for rule application
R/handle_plotly_click.R	Simplified click handling logic using .extract_click_info() helper
inst/shiny/modules/tab_nca/setup/slope_selector.R	Major refactor with improved reactivity flow and pagination module
inst/shiny/modules/tab_nca/setup/page_and_searcher.R	New pagination module extracted for cleaner separation of concerns
inst/shiny/modules/tab_nca/setup/handle_table_edits.R	Renamed from manual_slopes_table.R with improved override logic
tests/testthat/test-get_halflife_plots.R	New comprehensive tests for plot generation and marker styling
tests/testthat/test-utils-slope_selector.R	Updated tests removing filter_slopes, adding change detection tests

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Duplicate 'size' key in marker list: size is set to both 15 (line 264) and 20 (line 266). The second value (20) will override the first. Remove one of these assignments.

Suggested change

size = 15,

[Copilot]

The slope_groups are now derived from the 'new' data frame instead of being passed as a parameter. Ensure that 'new' always contains all necessary group columns, as any missing columns in 'new' will not be included in slope_groups, potentially causing incorrect behavior when comparing with 'existing'.

Suggested change

	slope_groups <- setdiff(names(new), c("TYPE", "RANGE", "REASON"))
	slope_groups <- union(
	setdiff(names(existing), c("TYPE", "RANGE", "REASON")),
	setdiff(names(new), c("TYPE", "RANGE", "REASON"))
	)
	# Ensure all slope_groups columns are present in both data frames
	missing_in_existing <- setdiff(slope_groups, names(existing))
	missing_in_new <- setdiff(slope_groups, names(new))
	if (length(missing_in_existing) > 0 || length(missing_in_new) > 0) {
	stop(
	paste0(
	"Missing group columns in data frames. ",
	if (length(missing_in_existing) > 0) {
	paste0("In 'existing': ", paste(missing_in_existing, collapse = ", "), ". ")
	} else "",
	if (length(missing_in_new) > 0) {
	paste0("In 'new': ", paste(missing_in_new, collapse = ", "), ".")
	} else ""
	)
	)
	}

[Copilot]

Changed default REASON from NA to empty string. This is a behavior change that may affect downstream code expecting NA values. Verify that all code checking for missing reasons handles empty strings correctly (e.g., line 739 uses nchar check, which works correctly with empty strings).

Suggested change

	df_conc$REASON <- ""
	df_conc$REASON <- NA_character_

[Copilot]

Potential index out of bounds error. If the inner_join result has fewer than 2 rows, accessing index [2] will fail. Add bounds checking or handle the case where insufficient data points exist.

Suggested change

	RANGE = paste0(
	inner_join(
	slopes_pknca_groups()[1, ],
	mydata()$conc$data
	)[[mydata()$conc$columns$time]][2]
	),
	RANGE = {
	joined <- inner_join(
	slopes_pknca_groups()[1, ],
	mydata()$conc$data
	)[[mydata()$conc$columns$time]]
	if (length(joined) >= 2) {
	paste0(joined[2])
	} else {
	NA_character_
	}
	},

[Copilot]

suppressWarnings() hides all warnings from pk.nca(). Consider using withCallingHandlers() to selectively suppress only expected warnings (like in the tests) while allowing unexpected warnings to surface.

Suggested change

	o_nca <- suppressWarnings(PKNCA::pk.nca(pknca_data))
	o_nca <- withCallingHandlers(
	PKNCA::pk.nca(pknca_data),
	warning = function(w) {
	# Suppress only expected warnings (customize the pattern as needed)
	expected_patterns <- c(
	"No intervals found for half.life", # Example pattern, adjust as needed
	"Some concentrations are below the limit of quantification" # Example
	)
	if (any(sapply(expected_patterns, function(pat) grepl(pat, conditionMessage(w))))) {
	invokeRestart("muffleWarning")
	}
	}
	)

[Copilot]

[nitpick] For exclusion rules, RANGE is set to a single time point. However, update_pknca_with_rules() expects RANGE in 'start:end' format and splits on ':'. A single value will cause range() to return just that value twice, which works but is unclear. Consider explicitly using 'time:time' format for consistency.

Suggested change

	new_rule$RANGE <- paste0(pnt$time)
	new_rule$RANGE <- paste0(pnt$time, ":", pnt$time)

[Copilot]

No error handling for invalid RANGE formats. If RANGE contains non-numeric values after splitting (e.g., '1,3' instead of '1:3'), as.numeric() will produce NA values, and range() will return c(NA, NA), causing downstream errors. Add validation or error handling for invalid RANGE formats.

Suggested change

	range <- strsplit(as.character(slopes$RANGE[i]), ":")[[1]] %>%
	as.numeric() %>%
	range()
	range_vals <- strsplit(as.character(slopes$RANGE[i]), ":")[[1]] %>%
	as.numeric()
	if (any(is.na(range_vals)) || length(range_vals) != 2) {
	stop(
	sprintf(
	"Invalid RANGE format in slopes row %d: '%s'. Expected format 'start:end' with numeric values.",
	i, as.character(slopes$RANGE[i])
	)
	)
	}
	range <- range(range_vals)

[Copilot]

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[Copilot]

The marker list has size specified twice (lines 264 and 266). The first size = 15 will be overridden by the second size = 20. This should be cleaned up to only specify size once, likely keeping the value 20 based on context.

Suggested change

size = 15,

[Copilot]

The documentation states that names are "key-value pairs separated by commas" but the implementation (line 138) uses underscores ("_\\s*") as separators, and line 118 shows the format is "name=value_name=value". The documentation should be updated to accurately reflect that names are in the format 'col1=val1_col2=val2_...' with underscores as separators between pairs and equals signs between keys and values.

Suggested change

	#' Converts a named list of plots (with names in the format 'col1: val1, col2: val2, ...')
	#' into a data frame with one row per plot and columns for each key.
	#'
	#' @param named_list A named list or vector, where names are key-value pairs separated by commas.
	#' Converts a named list of plots (with names in the format 'col1=val1_col2=val2_...'),
	#' where underscores separate key-value pairs and equals signs separate keys and values,
	#' into a data frame with one row per plot and columns for each key.
	#' @param named_list A named list or vector, where names are key-value pairs separated by underscores,
	#' with each pair in the format 'key=value'.

[Copilot]

The documentation states names are in the format 'col1: val1, col2: val2, ...' (with colons and commas), but the actual format used throughout the code is 'col1=val1_col2=val2_...' (with equals signs and underscores). The documentation should be corrected to match the implementation.

[Copilot]

The toggleState logic appears to be inverted. When current_page() == 1, the condition is TRUE, which enables the "previous_page" button (but it should be disabled on the first page). Similarly, when on the last page, the "next_page" button should be disabled. The conditions should be negated: condition = current_page() != 1 for previous and condition = current_page() != num_pages() for next.

Suggested change

	shinyjs::toggleState(id = "previous_page", condition = current_page() == 1)
	shinyjs::toggleState(id = "next_page", condition = current_page() == num_pages())
	shinyjs::toggleState(id = "previous_page", condition = current_page() != 1)
	shinyjs::toggleState(id = "next_page", condition = current_page() != num_pages())

[m-kolomanski]

style: The documentation is double-commented.

[m-kolomanski]

issue: So setting one of the arguments of the function to FALSE means the function does nothing? If the module / logic / user calling the function knows beforehand that exclusions don't have reasons, then the function should not be called anyway.

exclusions_have_reasons <- # some logic to check
if (exclusions_have_reasons) {
  processed_pknca_data <- checks_before_running_nca(processed_pknca_data)
}

or if we want to make this a function restriction, then the function itself should check it, not rely on the calling logic to tell it the data meets some requirements or not.

checks_before_running_nca <- function(processed_pknca_data) {
  exclusions_have_reasons <- # ... some logic to check
  if (!exclusions_have_reasons) {
    warning("Exclusions do not have reasons, could not process the data")
    return(processed_pknca_data)
  }
  # ... rest of the function
}

[m-kolomanski]

issue: Name of the function is not precise enough, it should be something like check_valid_reasons or assert_valid_reasons (since we are using checkmate) or something.

If we want to build out this function with more checks in the future, then I still think we should split each check into its own function, and call this one something like check|assert_valid_pknca. Then this function could see which checks need to run and calls appropriate checking functions. Otherwise, if we just keep adding checks (which means more if statements), we will run out of cyclomatic complexity very quickly.

[m-kolomanski]

style: We don't need to assign these variables so early, as me might fail the first check anyway.

Suggested change

	data_conc <- processed_pknca_data$conc$data
	excl_hl_col <- processed_pknca_data$conc$columns$exclude_half.life
	conc_groups <- group_vars(processed_pknca_data$conc)
	time_col <- processed_pknca_data$conc$columns$time
	if (!is.null(excl_hl_col)) {
	missing_reasons <- data_conc[[excl_hl_col]] & nchar(data_conc[["REASON"]]) == 0
	missing_reasons_rows <- data_conc[missing_reasons, ] %>%
	select(any_of(c(conc_groups, time_col)))
	excl_hl_col <- processed_pknca_data$conc$columns$exclude_half.life
	if (!is.null(excl_hl_col)) {
	data_conc <- processed_pknca_data$conc$data
	conc_groups <- group_vars(processed_pknca_data$conc)
	time_col <- processed_pknca_data$conc$columns$time
	missing_reasons <- data_conc[[excl_hl_col]] & nchar(data_conc[["REASON"]]) == 0
	missing_reasons_rows <- data_conc[missing_reasons, ] %>%
	select(any_of(c(conc_groups, time_col)))

[m-kolomanski]

style: Maybe instead of spelling out all the sub-values for each of the variables, it would be neater to assign just

data_cols <- pknca_data$conc$columns

and then use it to call

if (is.null(data_cols$exclude_half.life)) {
 # ...
}

[m-kolomanski]

issue: Why not stick to dplyr functions here?

[Gero1999]

@m-kolomanski I am open to any suggestions, but here plot_outputs() is a list object. So I was not sure in other way that could be easier for human-reading

[m-kolomanski]

issue: It is page_and_searcher_page_ui, but page_and_searcher_server. Can we make the name more concise? Just like pager_ui and pager_server or pagination or something like that should be enough.

[m-kolomanski]

question: What is this file?

[m-kolomanski]

issue: Should be a separate it() call.

issue: Should be using its own copy of pknca_data, since we are modifying that for the future tests in the file.

[m-kolomanski]

issue: Apart for two first test, the test descriptions do not follow the structure. get_halflife_plot marker colors and shapes and shapes - no exclusion/inclusion is not a valid sentence. Should be something like it("renders markers, colors and shapes appropriately with no exclusion or exclusion").

[Copilot]

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The size parameter is specified twice in the marker list. The second definition (line 268) will override the first one (line 266). Remove the duplicate definition on line 266.

Suggested change

size = 15,

[Copilot]

There's an inconsistency in the plot naming format. The documentation for parse_plot_names_to_df (line 130) states the format is 'col1: val1, col2: val2, ...' with colons and commas, but the implementation on line 138 splits by underscore and equals sign ('col1=val1_col2=val2'). Similarly, the plot ID generation on line 118 uses the underscore-equals format. The documentation should be corrected to match the actual implementation: 'col1=val1_col2=val2_...'.

[Copilot]

The REASON concatenation on lines 242-245 may result in duplicate reasons accumulating over multiple rule applications. Each call to update_pknca_with_rules will append the reason to existing reasons without a separator or check for duplicates. Consider either: (1) clearing existing reasons before applying new rules, (2) using a separator between reasons (e.g., '; '), or (3) checking if the reason already exists before appending.

Suggested change

	data$conc$data$REASON[pnt_idx] <- paste0(
	data$conc$data$REASON[pnt_idx],
	rep(slopes$REASON[i], length(pnt_idx))
	)
	# Update REASON field with separator and avoid duplicate reasons
	if (length(pnt_idx) > 0 && !is.na(slopes$REASON[i]) && nzchar(slopes$REASON[i])) {
	existing_reasons <- data$conc$data$REASON[pnt_idx]
	# Treat NA as empty for concatenation
	existing_reasons[is.na(existing_reasons)] <- ""
	new_reason <- slopes$REASON[i]
	# Check where the new reason is already present (simple substring check)
	already_has_reason <- grepl(new_reason, existing_reasons, fixed = TRUE)
	# For entries with no existing reason, just set the new reason
	to_set <- existing_reasons == "" & !already_has_reason
	existing_reasons[to_set] <- new_reason
	# For entries with an existing reason that doesn't already contain the new one, append with "; "
	to_append <- existing_reasons != "" & !already_has_reason
	existing_reasons[to_append] <- paste(existing_reasons[to_append], new_reason, sep = "; ")
	data$conc$data$REASON[pnt_idx] <- existing_reasons
	}

[Copilot]

The validation on lines 188-199 hardcodes column names (PCSPEC, USUBJID, PARAM, ATPTREF, DOSNOA) which may not match the actual group columns in the PKNCA data. This will fail if the group structure differs. Use the actual group columns from slopes_pknca_groups() or group_vars(mydata()) instead of hardcoded column names for proper validation.

Suggested change

	override_valid <- apply(manual_slopes_override(), 1, function(r) {
	dplyr::filter(
	mydata()$conc$data,
	PCSPEC == r["PCSPEC"],
	USUBJID == r["USUBJID"],
	PARAM == r["PARAM"],
	ATPTREF == r["ATPTREF"],
	DOSNOA == r["DOSNOA"]
	) |>
	NROW() != 0
	}) |>
	all()
	# Determine grouping columns dynamically to validate overrides correctly
	group_cols <- slopes_pknca_groups(mydata())
	# All required grouping columns must be present in the override table
	if (!all(group_cols %in% colnames(manual_slopes_override()))) {
	override_valid <- FALSE
	} else {
	override_valid <- apply(manual_slopes_override(), 1, function(r) {
	# Build a one-row data.frame with just the grouping columns from this override row
	row_group_vals <- as.list(r[group_cols])
	row_df <- as.data.frame(row_group_vals, stringsAsFactors = FALSE)
	# Check that there is at least one matching row in the concentration data
	dplyr::semi_join(mydata()$conc$data, row_df, by = group_cols) |>
	NROW() != 0
	}) |>
	all()
	}

[Copilot]

The RANGE value for single-point exclusion (line 37) stores only a single time value instead of a range. However, update_pknca_with_rules expects ranges in the format 'start:end' and calls strsplit(..., ":") on line 225 of utils-slope_selector.R, which will fail for single values without a colon. This should use a format like 'time:time' or the logic in update_pknca_with_rules should handle single-value ranges.

Suggested change

	new_rule$RANGE <- paste0(pnt$time)
	new_rule$RANGE <- paste0(pnt$time, ":", pnt$time)

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The inner_join on line 86 may fail or produce unexpected results if first_group contains NA values or if there's no matching row in the concentration data. Consider adding error handling or validation to ensure the join succeeds and that at least 2 rows exist before accessing the second element with [2].

Suggested change

	new_row <- cbind(
	first_group,
	data.frame(
	TYPE = "Exclusion",
	RANGE = paste0(
	inner_join(first_group, pknca_data()$conc$data)[[time_col]][2]
	),
	# Safely join to concentration data and derive a time value for the RANGE column
	joined_conc <- inner_join(first_group, pknca_data()$conc$data)
	if (!is.null(time_col) && time_col %in% names(joined_conc) && nrow(joined_conc) > 0) {
	if (nrow(joined_conc) >= 2) {
	range_value <- joined_conc[[time_col]][2]
	} else {
	# Fallback to the first available time if only one row is present
	range_value <- joined_conc[[time_col]][1]
	}
	} else {
	# If join fails or time column is unavailable, use NA to avoid runtime errors
	range_value <- NA_character_
	}
	new_row <- cbind(
	first_group,
	data.frame(
	TYPE = "Exclusion",
	RANGE = paste0(range_value),

[Copilot]

The search pattern on line 54 hardcodes "USUBJID" as the grouping variable for filtering plots. If the data uses a different subject identifier column name, this search will not work correctly. Consider making the subject column name configurable or dynamically detecting it from the pknca_data object.

[Copilot]

The @returns tag in the documentation (line 200) says "A plotly object representing the data points (plot_data)" but this is incorrect. The function actually returns a complete plotly object with both the fit line and the scatter points, not just the data points. The description should be updated to accurately reflect what is returned.

Suggested change

	#' @returns A plotly object representing the data points (plot_data)
	#' @returns A complete plotly object including the fit line and scatter points

[Copilot]

The marker size is specified twice in the same list. Line 267 sets size to 15, but line 269 overwrites it with 20. Remove the duplicate on line 267.

Suggested change

size = 15,

[Copilot]

The detect_pknca_data_changes function will fail with an error if old is NULL (lines 31-38 attempt to access old$conc$data when old is NULL). The in_hl_adj check should be wrapped in a conditional similar to in_data to handle the case when old is NULL.

Suggested change

	in_hl_adj = !identical(
	old$conc$data[[excl_hl_col]],
	new$conc$data[[excl_hl_col]]
	) |
	!identical(
	old$conc$data[[incl_hl_col]],
	new$conc$data[[incl_hl_col]]
	),
	in_selected_intervals = !identical(new$intervals, old$intervals)
	in_hl_adj = if (is.null(old) & !is.null(new)) {
	TRUE
	} else {
	!identical(
	old$conc$data[[excl_hl_col]],
	new$conc$data[[excl_hl_col]]
	) |
	!identical(
	old$conc$data[[incl_hl_col]],
	new$conc$data[[incl_hl_col]]
	)
	},
	in_selected_intervals = if (is.null(old) & !is.null(new)) {
	TRUE
	} else {
	!identical(new$intervals, old$intervals)
	}

[Copilot]

The function uses all_of() (line 164) but this is not imported in the NAMESPACE. The NAMESPACE file shows that all_of was removed (line 74 in the diff shows it was previously imported). This will cause an error when the function is called. Add @importFrom dplyr all_of to the function documentation or re-add it to the NAMESPACE.

[Copilot]

The new call to checks_before_running_nca() can raise an error message that embeds values from the uploaded concentration data (e.g., grouping columns and time) directly into the error string. In this NCA pipeline, such errors are later passed into .parse_pknca_error() and then wrapped in HTML() for showNotification without escaping, so a crafted data value containing HTML/JavaScript can be reflected back into the browser and executed when this check fails. To mitigate this, ensure that any error messages derived from data (including those produced by checks_before_running_nca()) are HTML-escaped or otherwise sanitized before being inserted into HTML-based notifications, or restrict the error handler to display only static, non-data-dependent text.

Suggested change

	# Check if there are exclusions that contains a filled reason
	checks_before_running_nca() %>%
	# Check if there are exclusions that contains a filled reason.
	# Any error message from checks_before_running_nca() may include user-supplied
	# data values; wrap it to replace such messages with static text before the
	# global error handler renders them as HTML.
	{
	tryCatch(
	checks_before_running_nca(.),
	error = function(e) {
	stop(
	"NCA pre-checks failed. Please verify your input data and settings.",
	call. = FALSE
	)
	}
	)
	} %>%