refactor(chemoproteomics): Create a template for chemoproteomics workflows#188
Merged
tonywu1999 merged 13 commits intodevelfrom Apr 22, 2026
Merged
refactor(chemoproteomics): Create a template for chemoproteomics workflows#188tonywu1999 merged 13 commits intodevelfrom
tonywu1999 merged 13 commits intodevelfrom
Conversation
…e workflow - Add TEMPLATES/TEMPLATE_LABELS constants for default + chemoproteomics - Add template selector on home page - Add condition metadata (DoseVal, DrugName, DoseUnit) editable DT table on loadpage for chemoproteomics - Parse drug name, dose value, and unit from condition names with user-editable fallbacks - Convert dose values from measured unit to molar (M) before analysis - Block analysis when any '?' placeholder values remain in metadata - Set transform_dose=TRUE for chemoproteomics dose-response fitting - Remove log-scale checkbox; set transform_dose via template instead - Add app_template threading through loadpage, qc, statmodel, expdes servers - Refactor expdesServer to use app_template instead of statmodel_input - Remove numbered prefixes from QC page section headers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR introduces a template-based system with constants Changes
Sequence DiagramsequenceDiagram
actor User
participant Home as Home UI
participant Server as Main Server
participant LoadPage as LoadPage Module
participant StatModel as StatModel Module
participant Helpers as Response Curve Helpers
User->>Home: Select template (chemoproteomics)
Home->>Server: input$app_template
Server->>Server: app_template reactive updated
Server->>LoadPage: pass app_template reactive
LoadPage->>LoadPage: initialize condition_metadata (if chemoproteomics)
LoadPage->>Helpers: parse conditions, units, drug names
Helpers->>LoadPage: return auto-filled metadata
LoadPage->>LoadPage: render editable condition_metadata table
User->>LoadPage: (optional) edit condition metadata
LoadPage->>LoadPage: update condition_metadata reactive
Server->>StatModel: pass app_template & condition_metadata
StatModel->>StatModel: force response-curve mode (chemoproteomics)
StatModel->>Helpers: convert_dose_to_molar, parse metadata
Helpers->>StatModel: return derived contrast matrix
StatModel->>StatModel: render response-curve plot & fitting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/statmodel-server-visualization.R (1)
201-210:⚠️ Potential issue | 🟠 MajorHonor the response-curve log-scale checkbox in downloaded plots.
transform_dose = TRUEignoresmodeling_response_curve_log_xaxis, so downloaded plots can disagree with a user’s linear-scale setting.Proposed fix
response_plot <- visualizeResponseProtein( data = dia_prepared, protein_name = input[[NAMESPACE_STATMODEL$visualization_which_protein]], drug_name = input[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]], ratio_response = isTRUE(input[[NAMESPACE_STATMODEL$visualization_response_curve_ratio_scale]]), show_ic50 = TRUE, add_ci = TRUE, - transform_dose = TRUE, + transform_dose = isTRUE(input[[NAMESPACE_STATMODEL$modeling_response_curve_log_xaxis]]), n_samples = 1000, increasing = input[[NAMESPACE_STATMODEL$modeling_response_curve_increasing_trend]] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-visualization.R` around lines 201 - 210, The downloaded response-curve plot is always using log-dose because transform_dose is hardcoded TRUE; change the call to visualizeResponseProtein so transform_dose is driven by the user's checkbox (use isTRUE(input[[NAMESPACE_STATMODEL$modeling_response_curve_log_xaxis]]) instead of TRUE). Update the visualizeResponseProtein invocation (the call building response_plot) to pass transform_dose based on that input so the downloaded plot honors the modeling_response_curve_log_xaxis setting.
🧹 Nitpick comments (1)
R/module-home-ui.R (1)
45-54: Non-namespacedinputIdbreaks module encapsulation.
inputId = "app_template"skipsns()so thatserver.Rcan read it asinput$app_templateat the app level. It works today but couples the module to a single mount point — mountinghomeUIwith a differentid, or mounting twice, would either miss this input or produce duplicate IDs. Consider namespacing it and surfacing the selected template via the module's returned reactive instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-home-ui.R` around lines 45 - 54, The selectInput in module-home-ui.R uses a non-namespaced inputId ("app_template") which breaks module encapsulation; change the inputId to use the module namespace (wrap with ns(), e.g. ns("app_template") inside the selectInput call in the homeUI function) and update the module server to expose the selected value via a reactive (e.g. return a reactive that reads input$app_template namespaced inside the corresponding homeServer). Ensure references to TEMPLATES / TEMPLATE_LABELS remain the same but consume the namespaced input via the module's returned reactive so callers can access the chosen template without relying on a global input id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/constants.R`:
- Around line 1-4: TEMPLATES is missing the protein_turnover key which causes
downstream checks against TEMPLATES$protein_turnover to be NULL; add an explicit
entry to the TEMPLATES list (e.g., protein_turnover = "protein_turnover") and
add the corresponding label in TEMPLATE_LABELS so references like in
statmodel-server-download-code.R, statmodel-server-options-modeling.R,
statmodel-server-visualization.R, module-expdes-server.R and
module-statmodel-server.R work as intended and no longer evaluate to NULL.
In `@R/module-expdes-server.R`:
- Around line 75-77: is_response_curve() currently returns TRUE for both
TEMPLATES$protein_turnover and TEMPLATES$chemoproteomics, but the nM conversion
(scaling by 1e9) in the "chemoproteomics assume nM" path should only apply to
chemoproteomics; change the logic so that either is_response_curve() only
includes TEMPLATES$chemoproteomics (remove TEMPLATES$protein_turnover) or, more
explicitly, guard the scaling with a check app_template() ==
TEMPLATES$chemoproteomics before multiplying by 1e9; update the condition used
in the scaling code (the block that mentions "chemoproteomics assume nM" and
does the 1e9 multiplication) to reference TEMPLATES$chemoproteomics directly to
avoid corrupting protein_turnover data.
In `@R/module-loadpage-server.R`:
- Around line 440-458: The code may leave stale condition metadata if parsing
fails or no Condition column exists; before attempting to rebuild set
condition_metadata(NULL) to clear previous state, and inside the tryCatch ensure
both the "no Condition" branch and the error handler call
condition_metadata(NULL); update the chemoproteomics init block around
get_data(), parse_drug_name_from_conditions(), autofill_condition_value(), and
parse_dose_unit_from_conditions() so any failure or missing Condition explicitly
resets condition_metadata to NULL.
In `@R/module-qc-server.R`:
- Line 19: The qcServer function signature currently declares unused parameters
app_template and get_condition_metadata; remove these two parameters from the
qcServer(...) declaration so the function only accepts (input, output, session,
parent_session, loadpage_input, get_data) and update any callers if present to
match the new signature; if you intended to keep them for future use instead,
add corresponding `@param` entries to the roxygen block and references in the body
(e.g., pass them through or use them) rather than leaving them unused.
In `@R/module-statmodel-server.R`:
- Around line 317-320: The calls to fitResponseCurves explicitly set
transform_dose = TRUE, forcing log-transform even when the UI checkbox is
unchecked; replace the hardcoded TRUE with the reactive checkbox value used by
the UI (the same logical reactive used for plotting) and pass that into
transform_dose (i.e., fitResponseCurves(input, matrix, preprocess_data(),
transform_dose = <ui_checkbox_reactive>)); apply the same change to the other
call site referenced (the block around the second occurrence, lines ~431-440) so
both fitting branches respect the user's checkbox.
- Around line 296-317: The code only checks for "?" but not for non-numeric
DoseVal or invalid DoseUnit, so validate DoseVal and DoseUnit before calling
convert_dose_to_molar: after obtaining meta from condition_metadata() (inside
the chemoproteomics branch) ensure DoseVal parses to a non-NA numeric (no
suppressWarnings(as.numeric(...)) — explicitly coerce and check !is.na) and that
DoseUnit exists and is one of the allowed unit strings (e.g.,
"pM","nM","uM","mM","M") or provide a clear error if missing/invalid; if
validation fails showNotification with a descriptive message and req(FALSE).
Update the block that builds matrix/GROUP/dose_value/drug (used by
convert_dose_to_molar and fitResponseCurves) to rely on the validated values so
convert_dose_to_molar receives guaranteed numeric dose and a known unit.
In `@R/server.R`:
- Line 35: The reactive app_template currently yields NULL before inputs
initialize, which overrides downstream fallbacks; change the app_template
reactive (symbol: app_template) to return a non-NULL default when
input$app_template is NULL (e.g., return TEMPLATES$default) so modules that call
app_template() (e.g., the check using app_template() %in%
c(TEMPLATES$protein_turnover, TEMPLATES$chemoproteomics)) always receive a valid
value and avoid logical(0) behavior during startup.
In `@R/statmodel-server-comparisons.R`:
- Around line 7-13: The function convert_dose_to_molar silently treats unknown
dose_unit lookups as 1 (molar); change it so unknown units are not coerced to 1:
detect NA entries in mult produced by multipliers[dose_unit], leave those mult
values as NA (so callers like build_response_curve_matrix can handle/propagate
them) and emit a warning listing the unique unknown units (use warning() with
context including the unknown unit strings and the function name
convert_dose_to_molar); do not replace NA with 1—only map known keys to their
multipliers and return dose_val * mult (with NA where conversion failed).
- Around line 28-38: The function parse_drug_name_from_conditions currently
computes a single most-frequent prefix and returns a scalar, which gets recycled
and collapses distinct drugs; change it to return a character vector aligned to
the input conditions: for each element of conditions compute the prefix (using
the existing regex/gsub and trimws logic), set "Treatment" for control rows
(is_ctrl) or when the per-element prefix is empty, and return that vector;
ensure the function signature parse_drug_name_from_conditions and its behavior
mirrors parse_dose_unit_from_conditions (vectorized, same length as input) so
callers using ifelse(is_ctrl, conditions, parsed_drug) get per-condition drug
names rather than a recycled scalar.
In `@R/statmodel-server-visualization.R`:
- Around line 49-70: The treatment dropdown still only removes "DMSO" even
though controls were detected earlier via grepl("^(dmso|control|vehicle)$", ...)
in the chemoproteomics branch; update the filtering logic after
prepare_dose_response_fit so that unique_drugs_without_control is derived by
removing any drug values matching that same control regex (case-insensitive)
rather than just the literal "DMSO" — locate where response_curve_setup_matrix
is used, and change the computation of unique_drugs_without_control (and the
value passed to selectInput) to exclude drugs matching the control pattern (use
the same regex or a helper that relies on is_ctrl logic).
---
Outside diff comments:
In `@R/statmodel-server-visualization.R`:
- Around line 201-210: The downloaded response-curve plot is always using
log-dose because transform_dose is hardcoded TRUE; change the call to
visualizeResponseProtein so transform_dose is driven by the user's checkbox (use
isTRUE(input[[NAMESPACE_STATMODEL$modeling_response_curve_log_xaxis]]) instead
of TRUE). Update the visualizeResponseProtein invocation (the call building
response_plot) to pass transform_dose based on that input so the downloaded plot
honors the modeling_response_curve_log_xaxis setting.
---
Nitpick comments:
In `@R/module-home-ui.R`:
- Around line 45-54: The selectInput in module-home-ui.R uses a non-namespaced
inputId ("app_template") which breaks module encapsulation; change the inputId
to use the module namespace (wrap with ns(), e.g. ns("app_template") inside the
selectInput call in the homeUI function) and update the module server to expose
the selected value via a reactive (e.g. return a reactive that reads
input$app_template namespaced inside the corresponding homeServer). Ensure
references to TEMPLATES / TEMPLATE_LABELS remain the same but consume the
namespaced input via the module's returned reactive so callers can access the
chosen template without relying on a global input id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d12f9ec-e75b-4ef6-933b-503c1f0bf363
📒 Files selected for processing (15)
R/constants.RR/module-expdes-server.RR/module-home-ui.RR/module-loadpage-server.RR/module-qc-server.RR/module-qc-ui.RR/module-statmodel-server.RR/server.RR/statmodel-server-comparisons.RR/statmodel-server-download-code.RR/statmodel-server-options-modeling.RR/statmodel-server-visualization.RR/statmodel-ui-options-modeling.RR/statmodel-ui-options-visualization.RR/utils.R
Comment on lines
+296
to
+317
| if (isTRUE(app_template() == TEMPLATES$chemoproteomics)) { | ||
| meta <- tryCatch(condition_metadata(), error = function(e) NULL) | ||
| if (!is.null(meta)) { | ||
| check_cols <- intersect(c("DoseVal", "DoseUnit"), colnames(meta)) | ||
| if (any(sapply(check_cols, function(col) "?" %in% meta[[col]]))) { | ||
| showNotification( | ||
| "Please fill in all '?' values in the condition metadata table on the data upload page before running the analysis.", | ||
| type = "error", duration = 8 | ||
| ) | ||
| req(FALSE) | ||
| } | ||
| } | ||
| meta <- condition_metadata() | ||
| req(!is.null(meta) && "DoseVal" %in% colnames(meta)) | ||
| is_ctrl <- grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)) | ||
| matrix <- data.frame( | ||
| GROUP = meta$Condition, | ||
| dose_value = convert_dose_to_molar(suppressWarnings(as.numeric(meta$DoseVal)), if ("DoseUnit" %in% colnames(meta)) meta$DoseUnit else "nM"), | ||
| drug = ifelse(is_ctrl, meta$Condition, if ("DrugName" %in% colnames(meta)) meta$DrugName else parse_drug_name_from_conditions(meta$Condition)), | ||
| stringsAsFactors = FALSE | ||
| ) | ||
| fitResponseCurves(input, matrix, preprocess_data(), transform_dose = TRUE) |
There was a problem hiding this comment.
Validate edited dose values and units, not just "?".
Users can edit the metadata table to non-numeric values or mistyped units. With suppressWarnings(as.numeric(...)) and unit fallback in conversion, those become NA or incorrect molar values instead of a clear validation error.
Proposed fix
if (isTRUE(app_template() == TEMPLATES$chemoproteomics)) {
meta <- tryCatch(condition_metadata(), error = function(e) NULL)
if (!is.null(meta)) {
- check_cols <- intersect(c("DoseVal", "DoseUnit"), colnames(meta))
- if (any(sapply(check_cols, function(col) "?" %in% meta[[col]]))) {
+ dose_values <- suppressWarnings(as.numeric(meta$DoseVal))
+ dose_units <- if ("DoseUnit" %in% colnames(meta)) trimws(meta$DoseUnit) else rep("nM", nrow(meta))
+ valid_units <- c("", "nM", "nm", "uM", "um", "mM", "mm", "M", "m")
+ invalid_metadata <- is.na(dose_values) | !(dose_units %in% valid_units)
+ if (any(invalid_metadata)) {
showNotification(
- "Please fill in all '?' values in the condition metadata table on the data upload page before running the analysis.",
+ "Please enter numeric dose values and supported dose units in the condition metadata table before running the analysis.",
type = "error", duration = 8
)
req(FALSE)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/module-statmodel-server.R` around lines 296 - 317, The code only checks for
"?" but not for non-numeric DoseVal or invalid DoseUnit, so validate DoseVal and
DoseUnit before calling convert_dose_to_molar: after obtaining meta from
condition_metadata() (inside the chemoproteomics branch) ensure DoseVal parses
to a non-NA numeric (no suppressWarnings(as.numeric(...)) — explicitly coerce
and check !is.na) and that DoseUnit exists and is one of the allowed unit
strings (e.g., "pM","nM","uM","mM","M") or provide a clear error if
missing/invalid; if validation fails showNotification with a descriptive message
and req(FALSE). Update the block that builds matrix/GROUP/dose_value/drug (used
by convert_dose_to_molar and fitResponseCurves) to rely on the validated values
so convert_dose_to_molar receives guaranteed numeric dose and a known unit.
Comment on lines
+317
to
+320
| fitResponseCurves(input, matrix, preprocess_data(), transform_dose = TRUE) | ||
| } else { | ||
| fitResponseCurves(input, matrix, preprocess_data(), transform_dose = TRUE) | ||
| } |
There was a problem hiding this comment.
Use the checkbox value for dose transformation.
The UI now lets templates set the log-axis default, but fitting and plotting still force transform_dose = TRUE. If the user unchecks the log-scale option for linear time/dose values, the analysis and plot still transform them.
Proposed fix
- fitResponseCurves(input, matrix, preprocess_data(), transform_dose = TRUE)
+ transform_dose <- isTRUE(input[[NAMESPACE_STATMODEL$modeling_response_curve_log_xaxis]])
+ fitResponseCurves(input, matrix, preprocess_data(), transform_dose = transform_dose)
} else {
- fitResponseCurves(input, matrix, preprocess_data(), transform_dose = TRUE)
+ transform_dose <- isTRUE(input[[NAMESPACE_STATMODEL$modeling_response_curve_log_xaxis]])
+ fitResponseCurves(input, matrix, preprocess_data(), transform_dose = transform_dose)
} visualizeResponseProtein(
data = dia_prepared,
protein_name = input[[NAMESPACE_STATMODEL$visualization_which_protein]],
drug_name = input[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]],
ratio_response = isTRUE(input[[NAMESPACE_STATMODEL$visualization_response_curve_ratio_scale]]),
show_ic50 = TRUE,
add_ci = TRUE,
- transform_dose = TRUE,
+ transform_dose = isTRUE(input[[NAMESPACE_STATMODEL$modeling_response_curve_log_xaxis]]),
n_samples = 1000,
increasing = input[[NAMESPACE_STATMODEL$modeling_response_curve_increasing_trend]]
)Also applies to: 431-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/module-statmodel-server.R` around lines 317 - 320, The calls to
fitResponseCurves explicitly set transform_dose = TRUE, forcing log-transform
even when the UI checkbox is unchecked; replace the hardcoded TRUE with the
reactive checkbox value used by the UI (the same logical reactive used for
plotting) and pass that into transform_dose (i.e., fitResponseCurves(input,
matrix, preprocess_data(), transform_dose = <ui_checkbox_reactive>)); apply the
same change to the other call site referenced (the block around the second
occurrence, lines ~431-440) so both fitting branches respect the user's
checkbox.
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
Comment on lines
+265
to
+267
| } else { | ||
| contrast$matrix = build_response_curve_matrix(condition_list()) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
this is unnecessary
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
tonywu1999
commented
Apr 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/module-statmodel-server.R (1)
146-178:⚠️ Potential issue | 🟠 Major
rc_matrixmay be undefined when this reactive fires outside chemoproteomics.On Line 168 the code does
if (is.null(rc_matrix) || nrow(rc_matrix) == 0), butrc_matrixis only assigned inside theif (app_template() == TEMPLATES$chemoproteomics)branch (Lines 149-158), and only when!is.null(meta) && nrow(meta) > 0 && "DoseVal" %in% colnames(meta)(the else returns early on Line 165). For any other template, this observer still fires when the user selects the response-curve comparison mode, and execution reaches Line 168 withrc_matrixunbound, throwingobject 'rc_matrix' not found— caught by the outertryCatchand shown as an error notification to the user.Even if the UI currently restricts response-curve mode to chemoproteomics, defensively guard with an early return or initialize
rc_matrix <- NULL:🔧 Proposed fix
if (isTRUE(input[[NAMESPACE_STATMODEL$comparison_mode]] == CONSTANTS_STATMODEL$comparison_mode_response_curve)) { tryCatch({ + rc_matrix <- NULL if (app_template() == TEMPLATES$chemoproteomics) {Related: Line 149 should also use
isTRUE(app_template() == TEMPLATES$chemoproteomics)for consistency with Lines 188, 304, and 422 — a NULL return fromapp_template()here currently yieldslogical(0)and errors in theif.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-statmodel-server.R` around lines 146 - 178, The observer can error because rc_matrix is only set inside the app_template() == TEMPLATES$chemoproteomics branch and may be unbound when the reactive runs; initialize rc_matrix <- NULL at the start of the tryCatch block (before the app_template() check) and change the template check to isTRUE(app_template() == TEMPLATES$chemoproteomics) so a NULL app_template() doesn't produce logical(0); then keep the existing guard if (is.null(rc_matrix) || nrow(rc_matrix) == 0) to return/stop and ensure contrast$matrix and enabling/disabling of NAMESPACE_STATMODEL$modeling_start are handled consistently.R/statmodel-server-download-code.R (1)
1-1:⚠️ Potential issue | 🔴 CriticalCritical: function signature is missing
app_templateargument — call site passes 5 positional args.
R/module-statmodel-server.Rline 337 calls:generate_analysis_code(qc_input(), loadpage_input(), comp_mat, input, app_template())but the function is defined with only 4 parameters in
R/statmodel-server-download-code.R. When this code path executes (e.g., Download analysis code), R will raiseunused argument (app_template())and the reactive will fail.Either add
app_templateas a parameter or remove it from the call site.🔧 Proposed fix
-generate_analysis_code = function(qc_input, loadpage_input, comp_mat, input) { +generate_analysis_code = function(qc_input, loadpage_input, comp_mat, input, app_template = NULL) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-download-code.R` at line 1, The function generate_analysis_code is missing the app_template parameter while its caller passes five positional arguments; update the function signature to include app_template (e.g., generate_analysis_code = function(qc_input, loadpage_input, comp_mat, input, app_template)) so the extra argument is accepted, and then use or forward app_template within the function body as appropriate (or explicitly ignore it) to prevent the "unused argument" error; reference generate_analysis_code and the caller in module-statmodel-server.R that invokes generate_analysis_code(..., app_template()).
♻️ Duplicate comments (2)
R/module-statmodel-server.R (1)
149-166: 🛠️ Refactor suggestion | 🟠 MajorConsolidate the six copies of the chemoproteomics matrix construction.
The
is_ctrl <- grepl(...); data.frame(GROUP=..., dose_value=convert_dose_to_molar(...), drug=ifelse(is_ctrl, ...))pattern now appears at Lines 149-166, 188-199, 212-226, 264-274, 316-324, and 422-431 in this file alone, plus twice instatmodel-server-visualization.R. A per-site bug fix (e.g., theparse_drug_name_from_conditionsscalar issue, or validating"?"inDoseUnit) has to be applied eight times.Extract a single
build_chemoproteomics_matrix(meta)helper (suggested in the cross-file comment instatmodel-server-visualization.R) and replace each block with one call. This would shrink this server file by ~50 lines and eliminate the observed drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-statmodel-server.R` around lines 149 - 166, The duplicated chemoproteomics matrix-construction logic across this file (and in statmodel-server-visualization.R) should be refactored into a single helper function build_chemoproteomics_matrix(meta); implement build_chemoproteomics_matrix(meta) to: validate meta (non-null, nrow>0, has DoseVal), compute is_ctrl with grepl on meta$Condition, compute dose_value using convert_dose_to_molar(as.numeric(meta$DoseVal), meta$DoseUnit or fallback "nM") including handling of "?" units, compute drug as ifelse(is_ctrl, meta$Condition, meta$DrugName or parse_drug_name_from_conditions(meta$Condition)), and return the data.frame with GROUP, dose_value, drug; then replace each repeated block (the calls around app_template()==TEMPLATES$chemoproteomics, where condition_metadata() is used and showNotification/disable(NAMESPACE_STATMODEL$modeling_start); return() is triggered) with a single call rc_matrix <- build_chemoproteomics_matrix(meta) and keep existing error/notification behavior unchanged.R/statmodel-server-visualization.R (1)
49-64: 🛠️ Refactor suggestion | 🟠 MajorExtract the chemoproteomics matrix construction into a single helper.
The same 8-line
data.frame(GROUP, dose_value, drug, ...)block is now repeated in six places across the codebase:
statmodel-server-visualization.RLines 49-64 and 186-197module-statmodel-server.RLines 149-166, 188-199, 212-226, 264-274, 316-324, and 422-431Every change (e.g., fixing the "?" unit handling, the scalar drug-name bug, adding a new metadata column) has to be made in six places and will drift. Extract a helper, e.g. in
statmodel-server-comparisons.R:build_chemoproteomics_matrix <- function(meta) { if (is.null(meta) || nrow(meta) == 0 || !("DoseVal" %in% colnames(meta))) return(NULL) is_ctrl <- grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)) dose_unit <- if ("DoseUnit" %in% colnames(meta)) meta$DoseUnit else "nM" drug_col <- if ("DrugName" %in% colnames(meta)) meta$DrugName else parse_drug_name_from_conditions(meta$Condition) data.frame( GROUP = meta$Condition, dose_value = convert_dose_to_molar(suppressWarnings(as.numeric(meta$DoseVal)), dose_unit), drug = ifelse(is_ctrl, meta$Condition, drug_col), stringsAsFactors = FALSE ) }and call it from every site. This also concentrates the scalar-drug and unknown-unit fixes in one place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-visualization.R` around lines 49 - 64, Extract the repeated chemoproteomics data.frame construction into a single helper (e.g. build_chemoproteomics_matrix) placed in statmodel-server-comparisons.R and have callers (places building rc_mat: the block guarded by app_template() == TEMPLATES$chemoproteomics and other locations in module-statmodel-server.R) call this helper instead of duplicating logic; the helper should accept meta (from condition_metadata()), return NULL if meta is NULL/empty or lacks "DoseVal", compute is_ctrl via grepl on meta$Condition, determine dose_unit (fallback "nM"), determine drug_col using "DrugName" if present or parse_drug_name_from_conditions(meta$Condition) otherwise, and return the data.frame with GROUP, dose_value (via convert_dose_to_molar with suppressWarnings(as.numeric(meta$DoseVal))), and drug using ifelse(is_ctrl, meta$Condition, drug_col), so all six duplicated blocks are replaced by rc_mat <- build_chemoproteomics_matrix(meta) with fallback to contrast$matrix when helper returns NULL.
🧹 Nitpick comments (3)
R/module-statmodel-server.R (2)
367-371: Null-check targets the reactive wrapper, not its value.
app_templateis the reactive function reference and is neverNULLin this scope — onlyapp_template()can returnNULL. Simplify to:🔧 Proposed fix
- if (!is.null(app_template) && !is.null(app_template()) && - app_template() %in% c(TEMPLATES$chemoproteomics)) { + if (isTRUE(app_template() == TEMPLATES$chemoproteomics)) { return(NULL) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-statmodel-server.R` around lines 367 - 371, The null-check is testing the reactive function reference app_template instead of its returned value; in the renderUI block for output$matrix you should remove the is.null(app_template) check and only test app_template() (e.g., if (!is.null(app_template()) && app_template() %in% c(TEMPLATES$chemoproteomics)) return(NULL)), ensuring you call the reactive to inspect its value; adjust any conditional logic in the output$matrix renderUI to use app_template() consistently.
43-71: DuplicatedupdateRadioButtons/updateSelectInputwiring.This
observeEvent(app_template(), ...)and theobserveEvent(input[[comparison_mode]], ...)at Lines 121-137 set overlappingchoicesfor the plot-typeselectInput. When the user selects the chemoproteomics template, both observers will fire (the template observer setscomparison_mode, which triggers the mode observer, which sets plot-type choices again). Functionally harmless, but redundant and easy to get out of sync.Consider extracting shared option lists to helpers (or
CONSTANTS_STATMODEL) so both observers reference the same source:comparison_mode_choices_default <- c( "All possible pairwise comparisons" = CONSTANTS_STATMODEL$comparison_mode_all_pairwise, "Compare all against one" = CONSTANTS_STATMODEL$comparison_mode_all_vs_one, "Create custom pairwise comparisons" = CONSTANTS_STATMODEL$comparison_mode_custom_pairwise, "Create custom non-pairwise comparisons" = CONSTANTS_STATMODEL$comparison_mode_custom_nonpairwise )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-statmodel-server.R` around lines 43 - 71, The two observers (observeEvent(app_template()) and the observer watching input[[comparison_mode]] which call updateRadioButtons and updateSelectInput) duplicate the same choice lists and can go out of sync; extract the shared choice vectors (e.g., comparison_mode_choices_default and visualization_plot_type_choices_default) into a single source (preferably CONSTANTS_STATMODEL or a small helper function) and replace the hard-coded choice literals inside updateRadioButtons and updateSelectInput in both observeEvent blocks to reference those shared symbols; ensure the chemoproteomics-specific choices (comparison_mode and visualization_plot_type for TEMPLATES$chemoproteomics) remain set only once (in observeEvent(app_template())) or by using the shared constants plus a conditional selection, so both observers use the same choice definitions and no longer duplicate wiring.R/module-expdes-server.R (1)
138-142: Dose scaling by1e9couples this module toconvert_dose_to_molaroutput.The
1e9multiplication assumes.get_concentrations_from_matrix()andprepared_response_data()$doseare always in molar. That invariant now holds because the upstream matrix is built viaconvert_dose_to_molar()inmodule-statmodel-server.R/statmodel-server-visualization.R, but it's an implicit cross-module contract. Consider either:
- Centralizing the molar↔nM conversion in a single helper (e.g.,
.to_nM(dose_val)), or- Adding a short comment explicitly stating that upstream
dose_valueis molar by convention (produced byconvert_dose_to_molar).Also note:
.check_replicates_per_dose(sim_data, selected_protein)is invoked after the*1e9scale; since it only groups by distinct values withtable(...), results are unchanged, but readers may find the ordering confusing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 138 - 142, The code multiplies doses by 1e9 (user_concs <- user_concs * 1e9 and sim_data$dose <- sim_data$dose * 1e9) which implicitly assumes upstream values are in molar from .get_concentrations_from_matrix()/convert_dose_to_molar(); replace these literal multiplications with a single helper .to_nM(dose_val) and call .to_nM(user_concs) and .to_nM(sim_data$dose) (implement .to_nM to multiply by 1e9 and document its contract), and either move the .check_replicates_per_dose(sim_data, selected_protein) call to before scaling or add a short comment explaining why the check runs after scaling so readers understand the ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/statmodel-server-comparisons.R`:
- Around line 17-24: The parsed-unit sentinel "?" from
parse_dose_unit_from_conditions is causing silent mis-conversion downstream in
convert_dose_to_molar; update parse_dose_unit_from_conditions to return
NA_character_ for non-control, non-matching units instead of "?" so unknown
units propagate as NA into convert_dose_to_molar (and fail loudly or be handled
consistently in render_group_comparison_plot_inputs and
create_download_plot_handler); ensure any callers expecting "?" are updated to
treat NA_character_ as the missing/unknown-unit case or add a small
normalization wrapper that converts "?" to NA if you prefer centralized
handling.
---
Outside diff comments:
In `@R/module-statmodel-server.R`:
- Around line 146-178: The observer can error because rc_matrix is only set
inside the app_template() == TEMPLATES$chemoproteomics branch and may be unbound
when the reactive runs; initialize rc_matrix <- NULL at the start of the
tryCatch block (before the app_template() check) and change the template check
to isTRUE(app_template() == TEMPLATES$chemoproteomics) so a NULL app_template()
doesn't produce logical(0); then keep the existing guard if (is.null(rc_matrix)
|| nrow(rc_matrix) == 0) to return/stop and ensure contrast$matrix and
enabling/disabling of NAMESPACE_STATMODEL$modeling_start are handled
consistently.
In `@R/statmodel-server-download-code.R`:
- Line 1: The function generate_analysis_code is missing the app_template
parameter while its caller passes five positional arguments; update the function
signature to include app_template (e.g., generate_analysis_code =
function(qc_input, loadpage_input, comp_mat, input, app_template)) so the extra
argument is accepted, and then use or forward app_template within the function
body as appropriate (or explicitly ignore it) to prevent the "unused argument"
error; reference generate_analysis_code and the caller in
module-statmodel-server.R that invokes generate_analysis_code(...,
app_template()).
---
Duplicate comments:
In `@R/module-statmodel-server.R`:
- Around line 149-166: The duplicated chemoproteomics matrix-construction logic
across this file (and in statmodel-server-visualization.R) should be refactored
into a single helper function build_chemoproteomics_matrix(meta); implement
build_chemoproteomics_matrix(meta) to: validate meta (non-null, nrow>0, has
DoseVal), compute is_ctrl with grepl on meta$Condition, compute dose_value using
convert_dose_to_molar(as.numeric(meta$DoseVal), meta$DoseUnit or fallback "nM")
including handling of "?" units, compute drug as ifelse(is_ctrl, meta$Condition,
meta$DrugName or parse_drug_name_from_conditions(meta$Condition)), and return
the data.frame with GROUP, dose_value, drug; then replace each repeated block
(the calls around app_template()==TEMPLATES$chemoproteomics, where
condition_metadata() is used and
showNotification/disable(NAMESPACE_STATMODEL$modeling_start); return() is
triggered) with a single call rc_matrix <- build_chemoproteomics_matrix(meta)
and keep existing error/notification behavior unchanged.
In `@R/statmodel-server-visualization.R`:
- Around line 49-64: Extract the repeated chemoproteomics data.frame
construction into a single helper (e.g. build_chemoproteomics_matrix) placed in
statmodel-server-comparisons.R and have callers (places building rc_mat: the
block guarded by app_template() == TEMPLATES$chemoproteomics and other locations
in module-statmodel-server.R) call this helper instead of duplicating logic; the
helper should accept meta (from condition_metadata()), return NULL if meta is
NULL/empty or lacks "DoseVal", compute is_ctrl via grepl on meta$Condition,
determine dose_unit (fallback "nM"), determine drug_col using "DrugName" if
present or parse_drug_name_from_conditions(meta$Condition) otherwise, and return
the data.frame with GROUP, dose_value (via convert_dose_to_molar with
suppressWarnings(as.numeric(meta$DoseVal))), and drug using ifelse(is_ctrl,
meta$Condition, drug_col), so all six duplicated blocks are replaced by rc_mat
<- build_chemoproteomics_matrix(meta) with fallback to contrast$matrix when
helper returns NULL.
---
Nitpick comments:
In `@R/module-expdes-server.R`:
- Around line 138-142: The code multiplies doses by 1e9 (user_concs <-
user_concs * 1e9 and sim_data$dose <- sim_data$dose * 1e9) which implicitly
assumes upstream values are in molar from
.get_concentrations_from_matrix()/convert_dose_to_molar(); replace these literal
multiplications with a single helper .to_nM(dose_val) and call
.to_nM(user_concs) and .to_nM(sim_data$dose) (implement .to_nM to multiply by
1e9 and document its contract), and either move the
.check_replicates_per_dose(sim_data, selected_protein) call to before scaling or
add a short comment explaining why the check runs after scaling so readers
understand the ordering.
In `@R/module-statmodel-server.R`:
- Around line 367-371: The null-check is testing the reactive function reference
app_template instead of its returned value; in the renderUI block for
output$matrix you should remove the is.null(app_template) check and only test
app_template() (e.g., if (!is.null(app_template()) && app_template() %in%
c(TEMPLATES$chemoproteomics)) return(NULL)), ensuring you call the reactive to
inspect its value; adjust any conditional logic in the output$matrix renderUI to
use app_template() consistently.
- Around line 43-71: The two observers (observeEvent(app_template()) and the
observer watching input[[comparison_mode]] which call updateRadioButtons and
updateSelectInput) duplicate the same choice lists and can go out of sync;
extract the shared choice vectors (e.g., comparison_mode_choices_default and
visualization_plot_type_choices_default) into a single source (preferably
CONSTANTS_STATMODEL or a small helper function) and replace the hard-coded
choice literals inside updateRadioButtons and updateSelectInput in both
observeEvent blocks to reference those shared symbols; ensure the
chemoproteomics-specific choices (comparison_mode and visualization_plot_type
for TEMPLATES$chemoproteomics) remain set only once (in
observeEvent(app_template())) or by using the shared constants plus a
conditional selection, so both observers use the same choice definitions and no
longer duplicate wiring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be88a8fc-4844-4800-9705-30af2f07788f
📒 Files selected for processing (11)
R/module-expdes-server.RR/module-loadpage-server.RR/module-statmodel-server.RR/server.RR/statmodel-server-comparisons.RR/statmodel-server-download-code.RR/statmodel-server-options-modeling.RR/statmodel-server-visualization.RR/statmodel-ui-options-modeling.RR/statmodel-ui-options-visualization.Rtests/testthat/test-utils-statmodel-server.R
💤 Files with no reviewable changes (1)
- tests/testthat/test-utils-statmodel-server.R
🚧 Files skipped from review as they are similar to previous changes (4)
- R/server.R
- R/statmodel-server-options-modeling.R
- R/statmodel-ui-options-visualization.R
- R/module-loadpage-server.R
tonywu1999
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
This PR introduces a new "Chemoproteomics" template to MSstatsShiny, extending the application beyond its original scope of protein differential abundance analysis. The changes implement a template system that allows users to select between two workflows: the existing "Protein Differential Abundance Analysis" (default) and a new "Chemoproteomics" workflow. The chemoproteomics workflow adds specialized support for dose-response analysis with condition metadata tracking, including dose values, units, and drug names. This involves refactoring how response curves are configured and fitted, replacing the previous
build_response_curve_matrixfunction with a more flexible metadata-driven approach.Detailed Changes
Core Infrastructure
R/constants.R): AddedTEMPLATESlist defining "default" and "chemoproteomics" template identifiers, andTEMPLATE_LABELSproviding user-facing labels for each template.User Interface
R/module-home-ui.R): Added a "Select a Template" section with a dropdown selector (selectInputwithinputId = "app_template").R/module-qc-ui.R): Removed numeric prefixes from section headings (e.g., "1. Peptide level normalization" → "Peptide level normalization").R/statmodel-ui-options-modeling.R): Removed the log-x-axis checkbox (create_response_curve_log_xaxis_checkbox); the increasing-trend checkbox now accepts a parameterizedvalueparameter.R/statmodel-ui-options-visualization.R): Updated plot type label from "Dose Response Curve" to "Response Curve".Server Logic - Data Loading
R/module-loadpage-server.R):app_templateparameter.condition_metadatareactive state to track per-condition information (dose value, drug name, dose unit).onclick("proceed1", ...)), when the chemoproteomics template is selected and aConditioncolumn exists, automatically derives and populates condition metadata with parsed dose values, drug names, and units; wraps initialization in error handling.getConditionMetadata.Server Logic - Experiment Design
R/module-expdes-server.R):statmodel_inputparameter withapp_template = reactive(TEMPLATES$default)..is_response_curve_mode()helper; response-curve mode is now determined byapp_template() == TEMPLATES$chemoproteomics.1e9and scale dose values accordingly.n_proteinsfrom 1000 to 300.Server Logic - Statistical Modeling
Statmodel Server (
R/module-statmodel-server.R):app_template,turnover_ratios, andcondition_metadatareactive parameters.app_template()changes, forces comparison mode and visualization settings specific to chemoproteomics (response-curve mode, response-curve plot type, disabled increasing-trend checkbox).condition_metadatawith automatic dose conversion and drug name derivation from control detection or metadata."?"before fitting.Statmodel Comparison Helpers (
R/statmodel-server-comparisons.R):build_response_curve_matrix(condition_list)function entirely.convert_dose_to_molar(),parse_dose_unit_from_conditions(),parse_drug_name_from_conditions(), andautofill_condition_value().prepare_dose_response_fitdocumentation to reference new metadata format (GROUP, dose_value, drug columns).Statmodel Visualization (
R/statmodel-server-visualization.R):render_group_comparison_plot_inputs()to acceptapp_templateandcondition_metadataparameters; for chemoproteomics, derives response-curve input matrix from condition metadata instead of contrast matrix.create_download_plot_handler()similarly to apply metadata-derived matrices for chemoproteomics response-curve plotting.Server Logic - Code Generation and Utilities
R/statmodel-server-download-code.R): Hard-codedtransform_dose = TRUEfor response-curve analysis instead of reading from user input.R/statmodel-server-options-modeling.R): Updatedget_response_curve_fitting_options()to accept optionaltemplateparameter; for response curves, now returns only the increasing-trend checkbox (no log-x-axis checkbox).R/utils.R): UpdatedfitResponseCurves()to accept optionaltransform_doseparameter (defaultTRUE).Main Server
R/server.R):app_templatereactive that readsinput$app_templateor defaults toTEMPLATES$default.app_templateintoloadpageServer()andstatmodelServer().condition_metadatafrom loadpage module outputs and passed tostatmodelServer().expdesServer()call signature to receiveapp_templateinstead ofreactive(statmodel_input).Testing
tests/testthat/test-utils-statmodel-server.R): Removed 42 lines of test cases for the deletedbuild_response_curve_matrix()function (3test_thatblocks covering generic, time-based, and temperature-based conditions).Coding Guidelines
reactive(TEMPLATES$default)), but introduces a pattern where some parameters are optional (condition_metadata = reactive(NULL)), which requires defensive checks throughout consuming code.statmodel-server-comparisons.Rlack roxygen documentation tags (e.g.,@param,@return,@noRd), which is inconsistent with the existing codebase documentation standard.tryCatchwith user-facing notifications, but some downstream validation (placeholder check in dose values) usesstopExecution(), mixing error handling patterns.