Feat turnover clean#189
Conversation
Adds protein_turnover to the template system alongside chemoproteomics, with Spectronaut/DIANN turnover input support, calculateTurnoverRatios integration, and turnover-specific response curve visualization paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant LoadPage
participant QC
participant StatModel
participant MSstatsResp
User->>LoadPage: Select `protein_turnover` template & upload data
LoadPage->>LoadPage: Autofill condition metadata → map Condition → TimeVal
LoadPage-->>User: Show Condition time points table
User->>QC: Enter tracer constants per condition
QC->>QC: Show Turnover tab & sidebar inputs
QC->>MSstatsResp: calculateTurnoverRatios(data, tracer_constants)
MSstatsResp-->>QC: Return turnover ratios
QC-->>User: Display ratios table + download
User->>StatModel: Start response-curve analysis
StatModel->>QC: Retrieve turnover_ratios() & condition_metadata()
StatModel->>StatModel: prepare_turnover_for_dose_response(ratios)
StatModel->>MSstatsResp: doseResponseFit(precalculated_ratios=TRUE, transform_dose=FALSE)
MSstatsResp-->>StatModel: Fitted model
StatModel->>MSstatsResp: visualizeResponseProtein(drug_name="time", ratio_response=FALSE, add_ci=FALSE, precalculated_ratios=TRUE)
MSstatsResp-->>StatModel: Render turnover curve
StatModel-->>User: Display turnover curve & results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
man/statmodelServer.Rd (1)
14-33:⚠️ Potential issue | 🟡 MinorMissing
@paramdocumentation forturnover_ratiosandcondition_metadata.The usage block declares
turnover_ratios = reactive(NULL)andcondition_metadata = reactive(NULL), but onlyapp_templateis documented in\arguments{}. R CMD check will warn about undocumented arguments. Add@paramentries forturnover_ratiosandcondition_metadatato the roxygen block inR/module-statmodel-server.Rand re-rundevtools::document().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@man/statmodelServer.Rd` around lines 14 - 33, Add missing roxygen `@param` tags for the reactive arguments turnover_ratios and condition_metadata in the roxygen block for the server module (module-statmodel-server.R) so they match the usage block that declares turnover_ratios = reactive(NULL) and condition_metadata = reactive(NULL); update the roxygen section to include brief descriptions for each (e.g., "turnover_ratios: reactive returning turnover ratio data" and "condition_metadata: reactive returning condition metadata"), then run devtools::document() to regenerate man/statmodelServer.Rd and verify the R CMD check warning is resolved.R/module-loadpage-server.R (1)
464-503:⚠️ Potential issue | 🟡 MinorSilent error swallowing in the protein-turnover metadata init path.
The chemoproteomics branch surfaces initialization failures via
showNotification(...)and resetscondition_metadata(NULL), but the new protein-turnover branch useserror = function(e) {}— completely silent. Ifget_data()returnsNULL, a list (PTM case), or a frame withoutCondition, the user sees neither the metadata table nor any warning, making the failure invisible. Please mirror the chemoproteomics error handler (notification + reset) for consistency.Additionally,
loadpageServer's roxygen block (lines 1–14) doesn't document the newapp_templateparameter; add an@param app_templateline soman/loadpageServer.Rdpicks it up.🛡️ Suggested fix
- if (!is.null(app_template) && app_template() == TEMPLATES$protein_turnover) { - tryCatch({ - data <- get_data() - if (!is.null(data) && "Condition" %in% colnames(data)) { - conditions <- unique(as.character(data$Condition)) - time_vals <- as.character(autofill_condition_value(conditions)) - time_vals[is.na(time_vals) | time_vals == "NA"] <- "?" - meta_df <- data.frame(Condition = conditions, - TimeVal = time_vals, - stringsAsFactors = FALSE) - condition_metadata(meta_df) - } - }, error = function(e) {}) + if (!is.null(app_template) && app_template() == TEMPLATES$protein_turnover) { + tryCatch({ + data <- get_data() + if (!is.null(data) && "Condition" %in% colnames(data)) { + conditions <- unique(as.character(data$Condition)) + time_vals <- as.character(autofill_condition_value(conditions)) + time_vals[is.na(time_vals) | time_vals == "NA"] <- "?" + meta_df <- data.frame(Condition = conditions, + TimeVal = time_vals, + stringsAsFactors = FALSE) + condition_metadata(meta_df) + } + }, error = function(e) { + condition_metadata(NULL) + showNotification( + paste("Could not initialize condition metadata:", conditionMessage(e)), + type = "warning", + duration = 6 + ) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-loadpage-server.R` around lines 464 - 503, The protein-turnover branch swallows errors silently; update the tryCatch error handler inside the app_template == TEMPLATES$protein_turnover block to mirror the chemoproteomics branch by calling condition_metadata(NULL) and showNotification(paste("Could not initialize condition metadata:", conditionMessage(e)), type = "warning", duration = 6) in error = function(e) { ... }, ensuring failures from get_data()/missing Condition are surfaced; also add an `@param` app_template line to the roxygen block for loadpageServer so the new parameter is documented.R/utils.R (1)
681-739:⚠️ Potential issue | 🟠 MajorTurnover-specific converter args are not mirrored in the download-code path.
getData()now forwardsintensity,peptideSequenceColumn,heavyLabels(Spectronaut) andlabeledAminoAcids(DIANN) to the converters, but the corresponding branches ingetDataCode()(same file, around lines 972–995) still emit the oldSpectronauttoMSstatsFormat(...)/DIANNtoMSstatsFormat(...)calls without these arguments. Running the downloaded script for a protein-turnover analysis will therefore silently reproduce a non-turnover conversion and diverge from the in-app results.Please thread the turnover inputs through
getDataCode()(gated the same way as ingetData) so the generated reproducible script matches the app behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils.R` around lines 681 - 739, getDataCode() currently emits SpectronauttoMSstatsFormat(...) and DIANNtoMSstatsFormat(...) calls that lack the turnover-related arguments you added in getData(), causing downloaded scripts to diverge; update getDataCode() to mirror the same gating and argument wiring used in getData(): when input$spec_intensity_col, input$spec_peptide_seq_col, input$spec_heavy_labels are non-empty add intensity, peptideSequenceColumn, heavyLabels to the Spectronaut call (and if calculate_anomaly_scores && run_order_file also emit calculateAnomalyScores, runOrder, anomalyModelFeatures, anomalyModelFeatureTemporal, n_trees, max_depth, numberOfCores), and when forming the DIANN call include quantificationColumn and labeledAminoAcids (using input$diann_labeled_aa logic); reference the existing converter_args usage in getData(), and ensure emitted code names match SpectronauttoMSstatsFormat and DIANNtoMSstatsFormat parameter names so the downloaded script reproduces in-app behavior.R/module-qc-ui.R (1)
34-138:⚠️ Potential issue | 🟠 MajorSection ids are namespaced in UI, but
shinyjs::hide/showcalls in the server use un-namespaced ids, preventing toggles from working.Lines 34, 76, 116 define div ids using
ns():div(id = ns("log_section")),div(id = ns("standards_type_section")),div(id = ns("censoring_section"))— which render asqc-log_section,qc-standards_type_section,qc-censoring_section. However, inR/module-qc-server.Rlines 48–50 and 59–61, the server-side hide/show calls use un-namespaced strings:shinyjs::hide("log_section") shinyjs::hide("censoring_section") shinyjs::hide("standards_type_section")The
shinyjsfunctions do not automatically apply module namespacing; they require explicit wrapping withsession$ns(). As written, these selectors will not match the actual DOM ids, causing the hide/show toggles to silently fail. Users see these sections even in the protein-turnover template, where they should be hidden.Fix: Wrap each id with
session$ns():shinyjs::hide(session$ns("log_section")) shinyjs::hide(session$ns("censoring_section")) shinyjs::hide(session$ns("standards_type_section"))Or alternatively, remove
ns()from the UI div ids and use plain strings (less recommended in modular code, but simpler if no other code references these ids).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-qc-ui.R` around lines 34 - 138, The UI wraps section ids with ns() (e.g. div(id = ns("log_section")), div(id = ns("standards_type_section")), div(id = ns("censoring_section"))) but the server uses shinyjs::hide/show with un-namespaced strings (shinyjs::hide("log_section") etc.), so the selectors don't match; update the server calls in module-qc-server.R to wrap the ids with session$ns() (e.g. shinyjs::hide(session$ns("log_section")), shinyjs::show(session$ns("censoring_section")), shinyjs::hide(session$ns("standards_type_section"))) or, alternatively, remove ns() from those UI div ids if you prefer non-modular ids.R/statmodel-server-download-code.R (1)
13-87:⚠️ Potential issue | 🔴 CriticalThe download code for the protein turnover template emits a data-preparation path that conflicts with the visualization parameters.
For
app_template == TEMPLATES$protein_turnover, the code generates:
- Data prep using
ProteinLevelDatamerged with metadata, mappingresponse = LogIntensities(lines 13–52)doseResponseFit(..., ratio_response = FALSE)(line 59)visualizeResponseProtein(..., precalculated_ratios = TRUE, drug_name = "time", ...)(lines 62–75)This is inconsistent with the in-app turnover flow. In-app, when
app_template == TEMPLATES$protein_turnover, the code callscalculateTurnoverRatios(...)to obtain H_frac ratios, thenprepare_turnover_for_dose_response()(mappingresponse = H_frac), and then callsvisualizeResponseProtein(..., precalculated_ratios = TRUE). Theprecalculated_ratios = TRUEparameter signals that the response column contains pre-calculated ratio values, not raw log intensities.The downloaded script will fail or produce incorrect results because it passes raw
LogIntensitiesto a function expecting normalized turnover ratios. Emit a turnover-specific data-prep block that mirrors the in-app pipeline whenapp_template == TEMPLATES$protein_turnover(callcalculateTurnoverRatios(...)and apply the transformations fromprepare_turnover_for_dose_response()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-download-code.R` around lines 13 - 87, The generated script uses raw LogIntensities for the protein-turnover template but then calls visualizeResponseProtein with precalculated_ratios=TRUE; to fix, replace the generic ProteinLevelData preparation when app_template == TEMPLATES$protein_turnover with the in-app turnover pipeline: call calculateTurnoverRatios(summarized$ProteinLevelData, ...) and then prepare_turnover_for_dose_response(...) to produce a prepared_data where response is H_frac (not LogIntensities), set doseResponseFit to use ratio_response = TRUE for turnover fits, and keep visualizeResponseProtein(..., precalculated_ratios = TRUE, drug_name = "time") so the download mirrors the in-app flow (refer to calculateTurnoverRatios, prepare_turnover_for_dose_response, ProteinLevelData, doseResponseFit, visualizeResponseProtein, and the response = H_frac mapping).R/module-statmodel-server.R (3)
370-394:⚠️ Potential issue | 🟡 MinorGuard
turnover_ratios()before use indata_comparison.When the protein_turnover branch runs,
turnover_ratios()is consumed directly (Line 371) and then passed intoprepare_turnover_for_dose_response()/doseResponseFit(). If the user has not yet triggered the turnover calculation in QC (theeventReactive(input$run, …)inmodule-qc-server.R), this reactive has no value and will throw an error in theeventReactiveinstead of surfacing a clear message. Add areq(ratios)/validate(need(...))guard before callingdoseResponseFit, mirroring the explicit notification you added in the plot-download path ofR/statmodel-server-visualization.R(Lines 188-192).🔧 Suggested guard
if (app_template() == TEMPLATES$protein_turnover) { ratios <- turnover_ratios() + validate(need(!is.null(ratios) && nrow(ratios) > 0, + "Turnover ratios not yet calculated. Please run data processing on the QC tab first.")) dia_prepared <- prepare_turnover_for_dose_response(ratios)🤖 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 370 - 394, The protein_turnover branch uses turnover_ratios() without guarding it, which can be NULL if the QC turnover calculation hasn't run; before calling prepare_turnover_for_dose_response() / doseResponseFit() in the app_template() == TEMPLATES$protein_turnover branch, add a guard such as req(ratios) or validate(need(!is.null(ratios), "Turnover ratios not available — run QC turnover first")) to fail fast with a clear message; update the block that declares ratios <- turnover_ratios() to perform this check immediately after obtaining ratios so subsequent calls to prepare_turnover_for_dose_response() and doseResponseFit() are safe.
248-282:⚠️ Potential issue | 🟠 MajorExclude-conditions observer still assumes chemoproteomics schema (
DoseVal).The comment on Line 246-247 explicitly notes "
protein_turnoverandchemoproteomicsmetadata comes from loadpage", but the body hard-codes the chemoproteomics schema — requiresDoseVal(Line 260) and builds adose_value/drugmatrix. Under theprotein_turnovertemplate a user excluding a time point will hitstop("Unable to build group metadata from the included conditions.")becauseDoseVal %in% colnames(meta)is false. Either early-return forprotein_turnover+ rebuild{GROUP, TimeVal}filtered byfiltered_conditions, or gate the observer onapp_template() == TEMPLATES$chemoproteomics.🤖 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 248 - 282, The observer for input[[NAMESPACE_STATMODEL$comparisons_exclude_conditions]] assumes chemoproteomics metadata (checks DoseVal and builds dose_value/drug) which breaks the protein_turnover template; modify the observer to first check app_template() and branch: if app_template() == TEMPLATES$chemoproteomics keep the existing logic using condition_metadata(), DoseVal, DoseUnit, DrugName and assign contrast$matrix and enable(NAMESPACE_STATMODEL$modeling_start); if app_template() == TEMPLATES$protein_turnover then build a minimal rc_matrix (GROUP and TimeVal or Time numeric converted from meta$TimeVal) filtered by filtered_conditions and set contrast$matrix and enable the start button; also ensure you return/stop early with a clear notification only when neither template produces a valid meta table (use condition_metadata() and check appropriate columns DoseVal or TimeVal instead of always requiring DoseVal).
311-328: 🛠️ Refactor suggestion | 🟠 MajorChemoproteomics metadata→matrix construction is duplicated in five places.
Essentially the same block —
is_ctrl <- grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)) data.frame(GROUP = meta$Condition, dose_value = convert_dose_to_molar(...), drug = ifelse(is_ctrl, meta$Condition, ...), stringsAsFactors = FALSE)— appears at Lines 192-198, 236-241, 267-273, 320-326 of this file plus twice in
R/statmodel-server-visualization.R(Lines 57-62, 199-204). Any future change to howDoseUnit/DrugName/control detection works has to be kept in sync across all six sites. Extracting a single helper (e.g.build_chemoproteomics_rc_matrix(meta)) would materially reduce maintenance risk here.🤖 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 311 - 328, Extract the repeated chemoproteomics matrix construction into a single helper (e.g. build_chemoproteomics_rc_matrix(meta)) and replace each duplicated block (currently inside branches checking app_template() == TEMPLATES$chemoproteomics and the two spots in R/statmodel-server-visualization.R) with a call to that helper; the helper should accept the metadata frame returned by condition_metadata(), compute is_ctrl with grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)), compute dose_value using convert_dose_to_molar(suppressWarnings(as.numeric(meta$DoseVal)), if ("DoseUnit" %in% colnames(meta)) meta$DoseUnit else "nM"), and compute drug using ifelse(is_ctrl, meta$Condition, if ("DrugName" %in% colnames(meta)) meta$DrugName else parse_drug_name_from_conditions(meta$Condition)), and return the data.frame(GROUP=..., dose_value=..., drug=..., stringsAsFactors=FALSE); update all call sites to perform the same meta null/empty checks (i.e. !is.null(meta) && nrow(meta) > 0 && "DoseVal" %in% colnames(meta)) before invoking the helper.R/statmodel-server-visualization.R (1)
45-75:⚠️ Potential issue | 🔴 CriticalUndefined
rc_matrisk when template is not chemoproteomics (or when metadata is missing).Line 65 unconditionally calls
prepare_dose_response_fit(rc_mat), butrc_matis only assigned inside theisTRUE(app_template() == TEMPLATES$chemoproteomics)branch — and even then only whenmetais non-null and containsDoseVal. In all other paths (default template response curve, or chemoproteomics with missing/invalid metadata), this will throwobject 'rc_mat' not foundinsiderenderUI, breaking the response-curve drug selector instead of surfacing a graceful message.🔧 Suggested guard / initialization
} else { + rc_mat <- NULL if (isTRUE(app_template() == TEMPLATES$chemoproteomics)) { meta <- tryCatch(condition_metadata(), error = function(e) NULL) if (!is.null(meta) && "DoseVal" %in% colnames(meta)) { is_ctrl <- grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)) rc_mat <- 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 ) } - } - response_curve_setup_matrix = prepare_dose_response_fit(rc_mat) + } + if (is.null(rc_mat)) { + return(NULL) + } + response_curve_setup_matrix = prepare_dose_response_fit(rc_mat)🤖 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 45 - 75, The renderUI calls prepare_dose_response_fit(rc_mat) even though rc_mat is only created inside the chemoproteomics branch, causing an error when rc_mat is missing; fix by guarding and early-returning a safe UI when rc_mat is undefined or metadata is invalid: inside the visualization_response_curve_which_drug renderUI (and before calling prepare_dose_response_fit) check app_template() and that condition_metadata() returned non-null with "DoseVal" (and DoseUnit/DrugName or parsed names); if metadata is missing/invalid, return NULL or a small info message UI and do not call prepare_dose_response_fit; when chemoproteomics path succeeds, continue to build rc_mat and then call prepare_dose_response_fit(response_curve_setup_matrix) and the selectInput as before (reference: app_template, condition_metadata, rc_mat, prepare_dose_response_fit, response_curve_setup_matrix, visualization_response_curve_which_drug).
🧹 Nitpick comments (3)
R/utils.R (1)
689-691: Minor: inconsistent split/trim pattern between Spectronaut and DIANN paths.Line 690 splits
spec_heavy_labelswith the regex",\\s*"while line 726 splitsdiann_labeled_aawith the literal","and thentrimws(). Both work, but pick one idiom for consistency (e.g.,trimws(strsplit(x, ",", fixed = TRUE)[[1]])). Also note that labels like"L[Leu6]"containing characters that happen to be regex metacharacters are fine as inputs tostrsplit(they're the input, not the pattern), but usingfixed = TRUEon the separator avoids future footguns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils.R` around lines 689 - 691, The split/trim pattern for spec_heavy_labels is inconsistent with the DIANN path; change the handling of input$spec_heavy_labels so it uses the same idiom as diann_labeled_aa (e.g., trimws(strsplit(..., ",", fixed = TRUE)[[1]])) and assign that to converter_args$heavyLabels—this ensures consistent trimming and uses fixed = TRUE to avoid regex pitfalls when splitting on a literal comma; update the code that references spec_heavy_labels/input$spec_heavy_labels and mirror the diann_labeled_aa approach.R/module-qc-server.R (1)
668-686: Call todisabled()/enable()/disable()assumeshinyjsis loaded in this module.
disabled(downloadButton(...))(Line 664) andenable("download_turnover_ratios")(Line 671) rely onshinyjs. Elsewhere in this fileshinyjs::is used explicitly (Lines 48-50, 91). For consistency and to avoid surprises ifshinyjsisn’t attached on the search path in all deployments, qualify these calls (shinyjs::disabled(...),shinyjs::enable(...),shinyjs::disable(...)) or ensure a single@import shinyjsis documented.Also note
enable("download_turnover_ratios")insiderenderUIwill run on every re-render even after the button is already enabled — harmless, but ashinyjs::toggleState("download_turnover_ratios", !is.null(turnover_ratios()))is cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-qc-server.R` around lines 668 - 686, The calls to disabled(downloadButton(...)) and enable("download_turnover_ratios") assume shinyjs is attached; qualify them as shinyjs::disabled(...) and shinyjs::enable(...) (and any other disable()/toggleState() uses) or add a package import (e.g., `@import` shinyjs) so the module consistently references shinyjs. In output$turnover_ratios_table_ui (and surrounding renderUI logic that references turnover_ratios()) replace the repeated shinyjs::enable("download_turnover_ratios") with a cleaner state toggle such as shinyjs::toggleState("download_turnover_ratios", !is.null(turnover_ratios())) to avoid re-enabling on every re-render. Ensure all occurrences of disabled/enable/disable/toggleState in this module are fully qualified as shinyjs::functionName.R/statmodel-server-visualization.R (1)
187-238: Duplicated chemoproteomics meta→matrix construction and turnover branching.The chemoproteomics
data.frame(GROUP=…, dose_value=…, drug=…)block (Lines 195-206) and theprotein_turnovervs other template branching (Lines 187-209 and 211-238) are near-verbatim copies of logic that also appears inR/module-statmodel-server.R(roughly L190-207, L235-243, L267-273, L317-328, L382-390, L491-500) and again earlier in this file (L53-64). Consider extracting two helpers — e.g.build_chemoproteomics_rc_matrix(meta)andbuild_response_curve_plot_data(template, …)— and a singlecall_visualize_response_protein(template, …)wrapper around the twovisualizeResponseProtein()invocations. This will prevent drift when one template’s parameters/columns change and not the others.🤖 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 187 - 238, This code duplicates chemoproteomics matrix construction and template branching across files; extract a helper build_chemoproteomics_rc_matrix(meta) that encapsulates the GROUP/dose_value/drug data.frame logic (use condition_metadata(), convert_dose_to_molar(), parse_drug_name_from_conditions() and respect DoseUnit/DoseVal), extract a helper build_response_curve_plot_data(template, ...) that returns dia_prepared by calling prepare_turnover_for_dose_response(turnover_ratios()) for TEMPLATES$protein_turnover and otherwise merging preprocess_data()$ProteinLevelData with the matrix and calling prepare_dose_response_fit(), and replace the two visualizeResponseProtein calls with a single wrapper call_visualize_response_protein(template, dia_prepared, input, ...) that sets the correct args (drug_name, ratio_response, show_ic50, transform_dose, precalculated_ratios, color_by, target_response) while preserving existing input keys and functions (visualizeResponseProtein, turnover_ratios, condition_metadata, preprocess_data).
🤖 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/module-qc-server.R`:
- Around line 577-611: The sidebar (output$turnover_ratios_sidebar) creates
tracer input IDs from get_data()$Condition while the turnover_ratios reactive
reads IDs from preprocess_data()$ProteinLevelData$GROUP, causing mismatches; fix
by centralizing the condition list into a single reactive (e.g.
tracer_conditions <- reactive({
as.character(unique(preprocess_data()$ProteinLevelData$GROUP)) })) and use
tracer_conditions() to build input IDs in output$turnover_ratios_sidebar and to
look up values in turnover_ratios, ensuring names and make.names(...) usage are
identical; alternatively (or additionally) add an explicit check in
turnover_ratios that when val is NULL you either validate()/stop with a clear
warning or log a warning via shiny::validate/req/processLogger so missing tracer
inputs are surfaced instead of silently defaulting to 1.0.
- Around line 196-212: In ordered_preprocess_data (the reactive that wraps
preprocess_data() and uses get_condition_metadata()), after computing
final_levels and re-leveling data$FeatureLevelData$GROUP, also apply the same
factor levels to data$ProteinLevelData$GROUP and any GROUP_ORIGINAL column used
later by statistics(); i.e., set data$ProteinLevelData$GROUP <- factor(...,
levels = final_levels) and update data$...$GROUP_ORIGINAL (or rename) to the
same factor levels so protein-level summaries and statistics() use the identical
ordering as FeatureLevelData.
In `@R/module-statmodel-server.R`:
- Around line 488-506: The plot rendering path calls
prepare_turnover_for_dose_response(turnover_ratios()) without guarding that
turnover_ratios() is present; add a req(turnover_ratios()) check immediately
before the prepare_turnover_for_dose_response(...) call when app_template() ==
TEMPLATES$protein_turnover so the code mirrors the download/data_comparison
behavior and fails with a user-friendly message instead of an error; change the
block around app_template() == TEMPLATES$protein_turnover to call
req(turnover_ratios()) then assign dia_prepared <-
prepare_turnover_for_dose_response(turnover_ratios()).
In `@R/statmodel-server-comparisons.R`:
- Around line 360-384: prepare_turnover_for_dose_response currently silently
replaces NA H_frac with 0 and coerces TimeVal without validation; change it to
(1) remove the automatic NA→0 imputation for H_frac and instead filter out rows
with NA H_frac (e.g., drop rows where result$H_frac is NA) so missing
measurements are not treated as zeros, (2) validate/parse result$TimeVal before
coercion (check all non-missing TimeVal are numeric or fail early with a clear
message/notification via validate()/showNotification) and only convert to
numeric after validation, and (3) update the roxygen block to remove the stale
`@param` metadata_matrix entry; use the function name
prepare_turnover_for_dose_response and the column names H_frac and TimeVal to
locate the changes.
---
Outside diff comments:
In `@man/statmodelServer.Rd`:
- Around line 14-33: Add missing roxygen `@param` tags for the reactive arguments
turnover_ratios and condition_metadata in the roxygen block for the server
module (module-statmodel-server.R) so they match the usage block that declares
turnover_ratios = reactive(NULL) and condition_metadata = reactive(NULL); update
the roxygen section to include brief descriptions for each (e.g.,
"turnover_ratios: reactive returning turnover ratio data" and
"condition_metadata: reactive returning condition metadata"), then run
devtools::document() to regenerate man/statmodelServer.Rd and verify the R CMD
check warning is resolved.
In `@R/module-loadpage-server.R`:
- Around line 464-503: The protein-turnover branch swallows errors silently;
update the tryCatch error handler inside the app_template ==
TEMPLATES$protein_turnover block to mirror the chemoproteomics branch by calling
condition_metadata(NULL) and showNotification(paste("Could not initialize
condition metadata:", conditionMessage(e)), type = "warning", duration = 6) in
error = function(e) { ... }, ensuring failures from get_data()/missing Condition
are surfaced; also add an `@param` app_template line to the roxygen block for
loadpageServer so the new parameter is documented.
In `@R/module-qc-ui.R`:
- Around line 34-138: The UI wraps section ids with ns() (e.g. div(id =
ns("log_section")), div(id = ns("standards_type_section")), div(id =
ns("censoring_section"))) but the server uses shinyjs::hide/show with
un-namespaced strings (shinyjs::hide("log_section") etc.), so the selectors
don't match; update the server calls in module-qc-server.R to wrap the ids with
session$ns() (e.g. shinyjs::hide(session$ns("log_section")),
shinyjs::show(session$ns("censoring_section")),
shinyjs::hide(session$ns("standards_type_section"))) or, alternatively, remove
ns() from those UI div ids if you prefer non-modular ids.
In `@R/module-statmodel-server.R`:
- Around line 370-394: The protein_turnover branch uses turnover_ratios()
without guarding it, which can be NULL if the QC turnover calculation hasn't
run; before calling prepare_turnover_for_dose_response() / doseResponseFit() in
the app_template() == TEMPLATES$protein_turnover branch, add a guard such as
req(ratios) or validate(need(!is.null(ratios), "Turnover ratios not available —
run QC turnover first")) to fail fast with a clear message; update the block
that declares ratios <- turnover_ratios() to perform this check immediately
after obtaining ratios so subsequent calls to
prepare_turnover_for_dose_response() and doseResponseFit() are safe.
- Around line 248-282: The observer for
input[[NAMESPACE_STATMODEL$comparisons_exclude_conditions]] assumes
chemoproteomics metadata (checks DoseVal and builds dose_value/drug) which
breaks the protein_turnover template; modify the observer to first check
app_template() and branch: if app_template() == TEMPLATES$chemoproteomics keep
the existing logic using condition_metadata(), DoseVal, DoseUnit, DrugName and
assign contrast$matrix and enable(NAMESPACE_STATMODEL$modeling_start); if
app_template() == TEMPLATES$protein_turnover then build a minimal rc_matrix
(GROUP and TimeVal or Time numeric converted from meta$TimeVal) filtered by
filtered_conditions and set contrast$matrix and enable the start button; also
ensure you return/stop early with a clear notification only when neither
template produces a valid meta table (use condition_metadata() and check
appropriate columns DoseVal or TimeVal instead of always requiring DoseVal).
- Around line 311-328: Extract the repeated chemoproteomics matrix construction
into a single helper (e.g. build_chemoproteomics_rc_matrix(meta)) and replace
each duplicated block (currently inside branches checking app_template() ==
TEMPLATES$chemoproteomics and the two spots in
R/statmodel-server-visualization.R) with a call to that helper; the helper
should accept the metadata frame returned by condition_metadata(), compute
is_ctrl with grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)), compute
dose_value using
convert_dose_to_molar(suppressWarnings(as.numeric(meta$DoseVal)), if ("DoseUnit"
%in% colnames(meta)) meta$DoseUnit else "nM"), and compute drug using
ifelse(is_ctrl, meta$Condition, if ("DrugName" %in% colnames(meta))
meta$DrugName else parse_drug_name_from_conditions(meta$Condition)), and return
the data.frame(GROUP=..., dose_value=..., drug=..., stringsAsFactors=FALSE);
update all call sites to perform the same meta null/empty checks (i.e.
!is.null(meta) && nrow(meta) > 0 && "DoseVal" %in% colnames(meta)) before
invoking the helper.
In `@R/statmodel-server-download-code.R`:
- Around line 13-87: The generated script uses raw LogIntensities for the
protein-turnover template but then calls visualizeResponseProtein with
precalculated_ratios=TRUE; to fix, replace the generic ProteinLevelData
preparation when app_template == TEMPLATES$protein_turnover with the in-app
turnover pipeline: call calculateTurnoverRatios(summarized$ProteinLevelData,
...) and then prepare_turnover_for_dose_response(...) to produce a prepared_data
where response is H_frac (not LogIntensities), set doseResponseFit to use
ratio_response = TRUE for turnover fits, and keep visualizeResponseProtein(...,
precalculated_ratios = TRUE, drug_name = "time") so the download mirrors the
in-app flow (refer to calculateTurnoverRatios,
prepare_turnover_for_dose_response, ProteinLevelData, doseResponseFit,
visualizeResponseProtein, and the response = H_frac mapping).
In `@R/statmodel-server-visualization.R`:
- Around line 45-75: The renderUI calls prepare_dose_response_fit(rc_mat) even
though rc_mat is only created inside the chemoproteomics branch, causing an
error when rc_mat is missing; fix by guarding and early-returning a safe UI when
rc_mat is undefined or metadata is invalid: inside the
visualization_response_curve_which_drug renderUI (and before calling
prepare_dose_response_fit) check app_template() and that condition_metadata()
returned non-null with "DoseVal" (and DoseUnit/DrugName or parsed names); if
metadata is missing/invalid, return NULL or a small info message UI and do not
call prepare_dose_response_fit; when chemoproteomics path succeeds, continue to
build rc_mat and then call
prepare_dose_response_fit(response_curve_setup_matrix) and the selectInput as
before (reference: app_template, condition_metadata, rc_mat,
prepare_dose_response_fit, response_curve_setup_matrix,
visualization_response_curve_which_drug).
In `@R/utils.R`:
- Around line 681-739: getDataCode() currently emits
SpectronauttoMSstatsFormat(...) and DIANNtoMSstatsFormat(...) calls that lack
the turnover-related arguments you added in getData(), causing downloaded
scripts to diverge; update getDataCode() to mirror the same gating and argument
wiring used in getData(): when input$spec_intensity_col,
input$spec_peptide_seq_col, input$spec_heavy_labels are non-empty add intensity,
peptideSequenceColumn, heavyLabels to the Spectronaut call (and if
calculate_anomaly_scores && run_order_file also emit calculateAnomalyScores,
runOrder, anomalyModelFeatures, anomalyModelFeatureTemporal, n_trees, max_depth,
numberOfCores), and when forming the DIANN call include quantificationColumn and
labeledAminoAcids (using input$diann_labeled_aa logic); reference the existing
converter_args usage in getData(), and ensure emitted code names match
SpectronauttoMSstatsFormat and DIANNtoMSstatsFormat parameter names so the
downloaded script reproduces in-app behavior.
---
Nitpick comments:
In `@R/module-qc-server.R`:
- Around line 668-686: The calls to disabled(downloadButton(...)) and
enable("download_turnover_ratios") assume shinyjs is attached; qualify them as
shinyjs::disabled(...) and shinyjs::enable(...) (and any other
disable()/toggleState() uses) or add a package import (e.g., `@import` shinyjs) so
the module consistently references shinyjs. In output$turnover_ratios_table_ui
(and surrounding renderUI logic that references turnover_ratios()) replace the
repeated shinyjs::enable("download_turnover_ratios") with a cleaner state toggle
such as shinyjs::toggleState("download_turnover_ratios",
!is.null(turnover_ratios())) to avoid re-enabling on every re-render. Ensure all
occurrences of disabled/enable/disable/toggleState in this module are fully
qualified as shinyjs::functionName.
In `@R/statmodel-server-visualization.R`:
- Around line 187-238: This code duplicates chemoproteomics matrix construction
and template branching across files; extract a helper
build_chemoproteomics_rc_matrix(meta) that encapsulates the
GROUP/dose_value/drug data.frame logic (use condition_metadata(),
convert_dose_to_molar(), parse_drug_name_from_conditions() and respect
DoseUnit/DoseVal), extract a helper build_response_curve_plot_data(template,
...) that returns dia_prepared by calling
prepare_turnover_for_dose_response(turnover_ratios()) for
TEMPLATES$protein_turnover and otherwise merging
preprocess_data()$ProteinLevelData with the matrix and calling
prepare_dose_response_fit(), and replace the two visualizeResponseProtein calls
with a single wrapper call_visualize_response_protein(template, dia_prepared,
input, ...) that sets the correct args (drug_name, ratio_response, show_ic50,
transform_dose, precalculated_ratios, color_by, target_response) while
preserving existing input keys and functions (visualizeResponseProtein,
turnover_ratios, condition_metadata, preprocess_data).
In `@R/utils.R`:
- Around line 689-691: The split/trim pattern for spec_heavy_labels is
inconsistent with the DIANN path; change the handling of input$spec_heavy_labels
so it uses the same idiom as diann_labeled_aa (e.g., trimws(strsplit(..., ",",
fixed = TRUE)[[1]])) and assign that to converter_args$heavyLabels—this ensures
consistent trimming and uses fixed = TRUE to avoid regex pitfalls when splitting
on a literal comma; update the code that references
spec_heavy_labels/input$spec_heavy_labels and mirror the diann_labeled_aa
approach.
🪄 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: 9d7465a1-eb54-4d6d-8331-6b792cb82dcb
📒 Files selected for processing (19)
NAMESPACER/MSstatsShiny.RR/constants.RR/module-loadpage-server.RR/module-loadpage-ui.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/ui.RR/utils.Rman/qcServer.Rdman/statmodelServer.Rd
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/statmodel-server-visualization.R (1)
171-193:⚠️ Potential issue | 🟠 MajorPass
turnover_ratiosinto the download handler.
turnover_ratios()is referenced insidecreate_download_plot_handler(), but it is not a parameter of this top-level function. Because R uses lexical scoping, this will not see theturnover_ratiosreactive fromstatmodelServer()unless it is passed in explicitly.Suggested fix
-create_download_plot_handler <- function(output, input, contrast, preprocess_data, data_comparison, loadpage_input, app_template = reactive(TEMPLATES$default), condition_metadata = reactive(NULL)) { +create_download_plot_handler <- function(output, input, contrast, preprocess_data, data_comparison, loadpage_input, app_template = reactive(TEMPLATES$default), condition_metadata = reactive(NULL), turnover_ratios = reactive(NULL)) {Also update the caller to pass the existing
turnover_ratiosreactive through to this helper.🤖 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 171 - 193, The download handler create_download_plot_handler references turnover_ratios() but that reactive is not passed in, so due to R's lexical scoping it may not find the statmodelServer reactive; add a new parameter turnover_ratios to create_download_plot_handler (e.g., turnover_ratios = reactive(NULL)) and use that reactive inside the content block as currently done (call turnover_ratios() where needed), and update the call sites (the caller that constructs create_download_plot_handler in statmodelServer) to pass the existing turnover_ratios reactive through; keep all existing checks and tryCatch logic intact.R/statmodel-server-download-code.R (1)
52-75:⚠️ Potential issue | 🟠 MajorGenerate ratio-shaped data before emitting turnover plotting options.
The turnover branch passes
precalculated_ratios = TRUEandcolor_by = "BaseSequence", butprepared_datais still onlyprotein/drug/dose/responsefromLogIntensities. The downloaded code will either fail on missingBaseSequenceor plot intensities as if they were turnover ratios. GeneratecalculateTurnoverRatios(...)/prepare_turnover_for_dose_response(...)code first, or omit the turnover-only visualization arguments untilprepared_datahas those columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-download-code.R` around lines 52 - 75, The generated download inserts visualizeResponseProtein with precalculated_ratios = TRUE and color_by = "BaseSequence" while prepared_data is still just protein/drug/dose/response; add a step before the visualizeResponseProtein call to transform prepared_data into turnover-shaped data (e.g., call calculateTurnoverRatios(...) or prepare_turnover_for_dose_response(...) to add BaseSequence and ratio columns) and only set precalculated_ratios = TRUE and color_by = "BaseSequence" when that transformation has been run; alternatively, guard or omit those turnover-only arguments unless prepared_data contains the required columns so visualizeResponseProtein won't fail.
♻️ Duplicate comments (2)
R/module-qc-server.R (2)
194-208:⚠️ Potential issue | 🟠 MajorOrder
TimeValnumerically and propagate levels to protein data.
TimeValis character metadata, soorder(meta_with_time$TimeVal)sorts lexicographically ("24"before"4"). Also, the same ordering still needs to be applied toProteinLevelData$GROUP/GROUP_ORIGINAL; otherwise condition plots and statistics can disagree with feature-level plot ordering.Suggested fix
meta <- get_condition_metadata() - meta_with_time <- meta[!is.na(meta$TimeVal), ] + time_vals <- suppressWarnings(as.numeric(meta$TimeVal)) + meta_with_time <- meta[!is.na(time_vals), , drop = FALSE] if (nrow(meta_with_time) > 0) { - ordered_conditions <- meta_with_time$Condition[order(meta_with_time$TimeVal)] + ordered_conditions <- meta_with_time$Condition[order(time_vals[!is.na(time_vals)])] all_groups <- unique(as.character(data$FeatureLevelData$GROUP)) remaining <- setdiff(all_groups, ordered_conditions) final_levels <- c(ordered_conditions, remaining) final_levels <- final_levels[final_levels %in% all_groups] data$FeatureLevelData$GROUP <- factor(data$FeatureLevelData$GROUP, levels = final_levels) + if (!is.null(data$ProteinLevelData) && "GROUP" %in% colnames(data$ProteinLevelData)) { + data$ProteinLevelData$GROUP <- factor(as.character(data$ProteinLevelData$GROUP), levels = final_levels) + } + if (!is.null(data$ProteinLevelData) && "GROUP_ORIGINAL" %in% colnames(data$ProteinLevelData)) { + data$ProteinLevelData$GROUP_ORIGINAL <- factor(as.character(data$ProteinLevelData$GROUP_ORIGINAL), levels = final_levels) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-qc-server.R` around lines 194 - 208, The TimeVal ordering currently uses order(meta_with_time$TimeVal) which sorts strings lexicographically; convert TimeVal to numeric (e.g., as.numeric(meta_with_time$TimeVal) or as.integer) when computing ordered_conditions inside the ordered_preprocess_data reactive so ordering is numeric, and after computing final_levels also set the same factor levels on data$ProteinLevelData$GROUP and data$ProteinLevelData$GROUP_ORIGINAL (mirroring the assignment to data$FeatureLevelData$GROUP) so protein-level plots/statistics use the same condition ordering.
576-610:⚠️ Potential issue | 🟠 MajorUse one condition source for tracer inputs and ratio calculation.
The sidebar creates inputs from
get_data()$Condition, butturnover_ratiosreads them usingpreprocess_data()$ProteinLevelData$GROUP. If preprocessing filters or normalizes a condition name,valbecomesNULLand silently defaults to1.0, producing wrong ratios. Useget_condition_metadata()$Conditionas the shared source and warn/error when an expected input is missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-qc-server.R` around lines 576 - 610, The UI and calculation use different condition sources causing missing inputs to default silently; change both the sidebar generator (output$turnover_ratios_sidebar) and the eventReactive (turnover_ratios) to derive the conditions from get_condition_metadata()$Condition (using the same make.names-based input IDs: paste0("tracer_", make.names(cond))) so the created inputs and the lookup use the identical canonical list, and replace the silent defaulting behavior in turnover_ratios by checking for missing inputs (input[[paste0("tracer_", make.names(cond))]]), and emitting a validation error or warning (stop or showNotification) when any expected tracer input is NULL instead of coercing to 1.0.
🤖 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/module-loadpage-server.R`:
- Around line 461-475: The protein_turnover init is seeding ordinal indices via
autofill_condition_value(conditions) and swallowing errors; update the block
that builds meta_df (inside the tryCatch for app_template() ==
TEMPLATES$protein_turnover) to extract real numeric time values from condition
strings (e.g., parse digits/decimal and common units like "h"/"hr" to a numeric
hour value), set TimeVal to "?" when parsing fails (instead of relying on
positional output), and then call condition_metadata(meta_df); also replace the
empty error handler with one that logs or notifies the error the same way the
chemoproteomics branch does so initialization failures are visible. Apply the
same parsing-and-error-handling fix to the other identical initialization block
referenced (lines ~576-580).
In `@R/statmodel-server-visualization.R`:
- Around line 45-70: The renderUI block calls prepare_dose_response_fit(rc_mat)
even when rc_mat is only set inside the chemoproteomics branch; initialize
rc_mat for the non-chemoproteomics path (or bail out) before calling
prepare_dose_response_fit. Specifically, inside
output[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]] (the
renderUI), ensure rc_mat is either constructed from condition_metadata() the
same way as in the chemoproteomics branch (using convert_dose_to_molar,
DoseVal/DoseUnit, DrugName or parse_drug_name_from_conditions) or set to NULL
and return early (NULL) so prepare_dose_response_fit is not called with an
undefined variable; keep the existing logic around is_ctrl, rc_mat structure
(GROUP, dose_value, drug) and the use of prepare_dose_response_fit,
unique(...$drug), and selectInput intact.
---
Outside diff comments:
In `@R/statmodel-server-download-code.R`:
- Around line 52-75: The generated download inserts visualizeResponseProtein
with precalculated_ratios = TRUE and color_by = "BaseSequence" while
prepared_data is still just protein/drug/dose/response; add a step before the
visualizeResponseProtein call to transform prepared_data into turnover-shaped
data (e.g., call calculateTurnoverRatios(...) or
prepare_turnover_for_dose_response(...) to add BaseSequence and ratio columns)
and only set precalculated_ratios = TRUE and color_by = "BaseSequence" when that
transformation has been run; alternatively, guard or omit those turnover-only
arguments unless prepared_data contains the required columns so
visualizeResponseProtein won't fail.
In `@R/statmodel-server-visualization.R`:
- Around line 171-193: The download handler create_download_plot_handler
references turnover_ratios() but that reactive is not passed in, so due to R's
lexical scoping it may not find the statmodelServer reactive; add a new
parameter turnover_ratios to create_download_plot_handler (e.g., turnover_ratios
= reactive(NULL)) and use that reactive inside the content block as currently
done (call turnover_ratios() where needed), and update the call sites (the
caller that constructs create_download_plot_handler in statmodelServer) to pass
the existing turnover_ratios reactive through; keep all existing checks and
tryCatch logic intact.
---
Duplicate comments:
In `@R/module-qc-server.R`:
- Around line 194-208: The TimeVal ordering currently uses
order(meta_with_time$TimeVal) which sorts strings lexicographically; convert
TimeVal to numeric (e.g., as.numeric(meta_with_time$TimeVal) or as.integer) when
computing ordered_conditions inside the ordered_preprocess_data reactive so
ordering is numeric, and after computing final_levels also set the same factor
levels on data$ProteinLevelData$GROUP and data$ProteinLevelData$GROUP_ORIGINAL
(mirroring the assignment to data$FeatureLevelData$GROUP) so protein-level
plots/statistics use the same condition ordering.
- Around line 576-610: The UI and calculation use different condition sources
causing missing inputs to default silently; change both the sidebar generator
(output$turnover_ratios_sidebar) and the eventReactive (turnover_ratios) to
derive the conditions from get_condition_metadata()$Condition (using the same
make.names-based input IDs: paste0("tracer_", make.names(cond))) so the created
inputs and the lookup use the identical canonical list, and replace the silent
defaulting behavior in turnover_ratios by checking for missing inputs
(input[[paste0("tracer_", make.names(cond))]]), and emitting a validation error
or warning (stop or showNotification) when any expected tracer input is NULL
instead of coercing to 1.0.
🪄 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: 6c0b9c14-6032-40dc-b0e8-810d831ff8a9
📒 Files selected for processing (7)
R/constants.RR/module-loadpage-server.RR/module-qc-server.RR/module-qc-ui.RR/module-statmodel-server.RR/statmodel-server-download-code.RR/statmodel-server-visualization.R
✅ Files skipped from review due to trivial changes (1)
- R/constants.R
🚧 Files skipped from review as they are similar to previous changes (2)
- R/module-qc-ui.R
- R/module-statmodel-server.R
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/module-statmodel-server.R (1)
214-250:⚠️ Potential issue | 🟠 MajorBranch the exclusion handler for protein turnover metadata.
The UI says users can exclude turnover time points, but this observer always requires
DoseValand rebuilds a chemoproteomics-style dose matrix. In protein turnover mode, changing the exclusion select will hit theDoseValguard and fail instead of rebuilding{GROUP, TimeVal}.🔧 Suggested fix
meta <- tryCatch(condition_metadata(), error = function(e) NULL) - if (is.null(meta) || nrow(meta) == 0 || !("DoseVal" %in% colnames(meta))) { + if (is.null(meta) || nrow(meta) == 0) { stop("Unable to build group metadata from the included conditions.") } meta <- meta[meta$Condition %in% filtered_conditions, , drop = FALSE] if (nrow(meta) == 0) { stop("Unable to build group metadata from the included conditions.") } - is_ctrl <- grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)) - rc_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 - ) + if (isTRUE(app_template() == TEMPLATES$protein_turnover)) { + if (!("TimeVal" %in% colnames(meta))) { + stop("Unable to build time metadata from the included conditions.") + } + rc_matrix <- data.frame( + GROUP = meta$Condition, + TimeVal = meta$TimeVal, + stringsAsFactors = FALSE + ) + } else { + if (!("DoseVal" %in% colnames(meta))) { + stop("Unable to build group metadata from the included conditions.") + } + is_ctrl <- grepl("^(dmso|control|vehicle)$", tolower(meta$Condition)) + rc_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 + ) + }🤖 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 214 - 250, The exclusion observer currently always expects dose metadata and rebuilds a dose RC matrix (uses DoseVal/DoseUnit/DrugName) which breaks protein turnover mode; update the observer that listens to input[[NAMESPACE_STATMODEL$comparisons_exclude_conditions]] to branch on input[[NAMESPACE_STATMODEL$comparison_mode]] (or CONSTANTS_STATMODEL$comparison_mode_protein_turnover if available) and, when in protein turnover mode, build a time-based contrast$matrix with GROUP = meta$Condition and TimeVal (and any needed time unit conversion) instead of dose_value/drug; keep the existing DoseVal path for response_curve mode, assign contrast$matrix and enable(NAMESPACE_STATMODEL$modeling_start) in both branches, and preserve the same error handling and checks for meta presence/rows.
🤖 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/module-qc-server.R`:
- Around line 41-68: The observer currently calls app_template() unconditionally
causing errors when qcServer is used with the default NULL; wrap the
observeEvent registration in a guard that checks app_template is provided (e.g.
if (!is.null(app_template)) { observeEvent(app_template(), { ... }, ignoreNULL =
TRUE) }) and remove or fix the internal req(!is.null(app_template)) check so you
never call app_template() when it is NULL; this uses the existing observeEvent
and app_template symbols to locate and fix the code.
- Around line 590-640: The turnover_ratios reactive should pass the metadata
TimeVal to calculateTurnoverRatios and validate tracer inputs: replace time_col
= "GROUP" with time_col = "TimeVal" (use the TimeVal column from
get_condition_metadata()/preprocess_data() metadata) so parse_timepoint sees
user-edited time values, and change the tracer_consts construction (the code
using input[[paste0("tracer_", make.names(cond))]]) to validate each value is
present and numeric (e.g., use req/validate or need to error when is.null or
!is.numeric or is.na, and reject non-positive values) before calling
calculateTurnoverRatios; reference turnover_ratios, calculateTurnoverRatios,
tracer_<make.names(cond)> and get_condition_metadata()/TimeVal to locate spots
to change.
In `@R/module-statmodel-server.R`:
- Around line 338-340: Before calling prepare_turnover_for_dose_response(),
check that turnover_ratios() returned valid data (not NULL and not empty) when
app_template() == TEMPLATES$protein_turnover; if the ratios are missing, abort
the modeling flow with a user-facing validation message (mirroring the plot path
behavior) instead of passing NULL into prepare_turnover_for_dose_response() so
the UI shows a clear instruction to run QC first.
---
Outside diff comments:
In `@R/module-statmodel-server.R`:
- Around line 214-250: The exclusion observer currently always expects dose
metadata and rebuilds a dose RC matrix (uses DoseVal/DoseUnit/DrugName) which
breaks protein turnover mode; update the observer that listens to
input[[NAMESPACE_STATMODEL$comparisons_exclude_conditions]] to branch on
input[[NAMESPACE_STATMODEL$comparison_mode]] (or
CONSTANTS_STATMODEL$comparison_mode_protein_turnover if available) and, when in
protein turnover mode, build a time-based contrast$matrix with GROUP =
meta$Condition and TimeVal (and any needed time unit conversion) instead of
dose_value/drug; keep the existing DoseVal path for response_curve mode, assign
contrast$matrix and enable(NAMESPACE_STATMODEL$modeling_start) in both branches,
and preserve the same error handling and checks for meta presence/rows.
🪄 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: 00efd048-32f9-40c1-9e17-ce5d79a67646
📒 Files selected for processing (7)
R/main_calculations.RR/module-qc-server.RR/module-statmodel-server.RR/statmodel-server-comparisons.RR/statmodel-server-options-modeling.RR/statmodel-server-visualization.RR/statmodel-ui-options-modeling.R
✅ Files skipped from review due to trivial changes (1)
- R/statmodel-server-visualization.R
🚧 Files skipped from review as they are similar to previous changes (3)
- R/statmodel-server-comparisons.R
- R/statmodel-ui-options-modeling.R
- R/statmodel-server-options-modeling.R
1 participant
Motivation and Context
This PR adds first-class protein turnover (time-course) analysis to MSstatsShiny by introducing a new "protein_turnover" application template and wiring a turnover analysis workflow that reuses MSstatsResponse functionality (notably calculateTurnoverRatios). Goal: enable isotopic-labeling/time-course workflows (compute turnover ratios, visualize turnover curves, and fit time-based response models) alongside existing chemoproteomics dose-response features, using template-driven UI, data conversion, QC, and modeling behavior.
Short solution summary: new template constants and UI controls for turnover data import and metadata (TimeVal); QC module computes and exposes turnover ratios (via MSstatsResponse::calculateTurnoverRatios) with sidebar tracer-constant inputs and CSV download; modeling modules accept precomputed turnover ratios (converted by a new helper) and run/visualize response-curve fits using time as the "dose"; many UI and option functions were made template-aware.
Detailed Changes
Constants & Labels
Namespace / Imports
Load page / Data import (R/module-loadpage-*.R, R/utils.R)
QC Module (R/module-qc-server.R, R/module-qc-ui.R)
Modeling & Response-Curve Workflow (multiple R/statmodel-* and module-statmodel-server.R)
Utilities & Misc
Unit Tests Added or Modified
Coding Guidelines / Policy Violations