refactor: Move helper functions for statmodel server into separate files#155
refactor: Move helper functions for statmodel server into separate files#155tonywu1999 merged 7 commits intodevelfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds new server modules for contrast-building, plotting, results export, and analysis-code generation; the main module file received only syntactic assignment-style edits. New functions implement contrast UI/validation, matrix builders (including response-curve support), plotting and download handlers, and code assembly. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (Browser)"
participant UI as "Shiny UI"
participant Server as "Shiny Server Modules"
participant Model as "MSstats / PTM / TMT"
participant FS as "Filesystem"
User->>UI: select conditions, contrast mode, plot options
UI->>Server: namespaced inputs (contrast, visualization, downloads)
Server->>Server: resolve conditions, validate inputs
Server->>Server: build/modify contrast matrix (incl. response-curve)
Server->>Model: run groupComparison / groupComparisonPTM / groupComparisonTMT
Model-->>Server: return results and plot artifacts
Server->>FS: write generated analysis code and package plots
Server-->>UI: render results table, plots, download endpoints
UI-->>User: display outputs and provide downloads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@R/statmodel-server-comparisons.R`:
- Around line 62-72: The loop construction in render_custom_non_pairwise_inputs
uses 1:length(condition_list()) which fails when condition_list() is empty;
change the iteration to use seq_along(condition_list()) (or
seq_len(length(condition_list()))) so the lapply call becomes
lapply(seq_along(condition_list()), ...) and similarly replace all other
occurrences of 1:length(...) noted in this file (the other blocks around the
same UI render functions that build numericInput elements using
NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_weights) to avoid the 1:0
behavior and out-of-bounds indexing.
- Around line 90-116: The code currently deduplicates comp_list$dList (via
unique()) and contrast$matrix (via !duplicated on row values) independently,
which can cause a row-name length mismatch in build_custom_pairwise_contrast;
fix by deduplicating names and rows in tandem: after appending contrast$row and
the new name, compute a canonical key for each row (e.g., collapse numeric rows
to a string) and use duplicated() on those keys to derive a single set of unique
indices, then subset both contrast$matrix and comp_list$dList by the same unique
indices before assigning rownames; additionally add a guarded length check
before rownames(contrast$matrix) = comp_list$dList and apply the same
tandem-dedup pattern in the analogous block referenced (lines 118-146).
In `@R/statmodel-server-download-code.R`:
- Around line 7-11: The loop using for (i in 1:nrow(comp_mat)) is unsafe when
comp_mat has zero rows; replace that iteration with for (i in
seq_len(nrow(comp_mat))) (or guard with if (nrow(comp_mat) > 0) ...) so you
don't produce 1:0 and attempt to index non-existent rows—update the loop that
builds codes and appends to contrast.matrix (references: comp_mat, codes,
contrast.matrix) accordingly.
In `@R/statmodel-server-options-modeling.R`:
- Around line 7-11: The if in get_tmt_moderation_radio_button unguardedly
compares loadpage_input$DDA_DIA to "TMT" which can be NULL; update the condition
to first ensure DDA_DIA is present (e.g. !is.null(loadpage_input$DDA_DIA) &&
length(loadpage_input$DDA_DIA) > 0) before comparing to "TMT", and only call
create_moderation_radio_buttons(ns) when that guarded comparison is true.
In `@R/statmodel-server-results-table.R`:
- Around line 9-13: The req(SignificantProteins()) call makes the subsequent
is.null(SignificantProteins()) branch unreachable; either remove the req() guard
or replace the null check with a content-based emptiness check (e.g., test
nrow(SignificantProteins()) == 0 or length(SignificantProteins()) == 0) and keep
req(data_comparison()) so the empty-state branch (tagList(tags$br())) can run;
update the code around the SignificantProteins() usage accordingly, referencing
the req(SignificantProteins()), is.null(SignificantProteins()), and
tagList(tags$br()) locations.
- Around line 49-57: The function extract_significant_proteins has inconsistent
NA handling: the TMT branch uses a direct adj.pvalue < signif_threshold (which
preserves NA rows) while the default branch uses which(...) (dropping NAs);
update the TMT and PTM branches to match the default by using
which(data_comp$ComparisonResult$adj.pvalue < signif_threshold) and
which(data_comp$ADJUSTED.Model$adj.pvalue < signif_threshold) respectively, or
alternatively add an explicit !is.na(...) (e.g. adj.pvalue < signif_threshold &
!is.na(adj.pvalue)) to both branches so both paths consistently exclude NA
p-values when selecting rows from data_comp$ComparisonResult and
data_comp$ADJUSTED.Model in extract_significant_proteins.
In `@R/statmodel-server-visualization.R`:
- Around line 121-130: The download handler in create_download_plot_handler
registers a filename with a .zip extension but uses file.copy(latest_file, file)
which does not produce a ZIP archive; update the content function inside
downloadHandler (for NAMESPACE_STATMODEL$visualization_download_plot_results) to
create a real zip by calling zip(zipfile = file, files = latest_file) (or
alternatively change the filename() to match the true file extension), and
ensure you handle temporary paths and return/cleanup correctly so the served
file is a valid archive.
- Around line 64-118: The tryCatch around the plotting block currently swallows
errors by only calling message(), so user-facing errors (including the stop()
from the volcano-plot validation) never surface in the UI; update the error
handler (the error = function(e) block in the tryCatch that currently calls
remove_modal_spinner() and message()) to call a Shiny UI notification such as
shiny::showNotification(conditionMessage(e), type = "error", duration = NULL) or
shiny::showModal(shiny::modalDialog(title = "Error", conditionMessage(e))), keep
remove_modal_spinner() and also optionally log the error with message() or
warning() for console debugging, and ensure the validation stop() message passed
through conditionMessage(e) is what is displayed to the user.
🧹 Nitpick comments (4)
R/statmodel-server-download-code.R (2)
27-29: Redundantloadpage_input$BIO == "PTM"check inside the PTM branch.Line 28 re-checks
loadpage_input$BIO == "PTM"even though theelse ifon line 27 already guarantees it. This doesn't cause a bug but adds unnecessary noise.Simplified condition
- dt = if ((loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA == "TMT") | - loadpage_input$filetype == 'phil') "TMT" else "LabelFree" + dt = if (loadpage_input$DDA_DIA == "TMT" || + loadpage_input$filetype == "phil") "TMT" else "LabelFree"
46-52: Non-PTM generated code referencesgroupComparisonPlotswithout package qualifier.The TMT/default branch (lines 36–37) correctly qualifies
MSstatsTMT::groupComparisonTMTandMSstats::groupComparison, but the plotting call on line 47 uses the bare namegroupComparisonPlotswithout a namespace prefix. For a standalone reproducible script this may fail if the user hasn't loaded the package. The PTM branch (line 41) has the same issue withgroupComparisonPlotsPTM.R/statmodel-server-visualization.R (1)
80-111: TMT and default plot branches are nearly identical.Lines 80–95 and 96–111 share nearly all parameters. Consider extracting the common logic into a helper to reduce duplication and ease future maintenance. This is optional for this refactoring PR.
R/statmodel-server-comparisons.R (1)
5-18: RedundantBIO == "PTM"sub-conditions clutter the logic.The conditions on lines 6–7 and 10–11 each redundantly re-check
loadpage_input$BIO == "PTM"inside an expression that is already predicated on it. Also,&(vectorized AND) is used on scalar values where&&(short-circuit) is more idiomatic and safer.Simplified version
get_experimental_conditions = function(loadpage_input, preprocess_data) { - if (loadpage_input$BIO == "PTM" & - ((loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA == "TMT") | - loadpage_input$filetype == 'phil')) { + if (loadpage_input$BIO == "PTM" && + (loadpage_input$DDA_DIA == "TMT" || loadpage_input$filetype == "phil")) { levels(preprocess_data$PTM$ProteinLevelData$Condition) - } else if (loadpage_input$BIO == "PTM" & - (loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA != "TMT")) { + } else if (loadpage_input$BIO == "PTM") { levels(preprocess_data$PTM$ProteinLevelData$GROUP) } else if (loadpage_input$DDA_DIA == "TMT") { levels(preprocess_data$ProteinLevelData$Condition) } else { levels(preprocess_data$ProteinLevelData$GROUP) } }
| build_custom_pairwise_contrast = function(input, condition_list, contrast, comp_list, row) { | ||
| if (input[[NAMESPACE_STATMODEL$comparisons_custom_pairwise_choice1]] == input[[NAMESPACE_STATMODEL$comparisons_custom_pairwise_choice2]]) { | ||
| return(contrast$matrix) | ||
| } | ||
|
|
||
| index1 = which(condition_list == input[[NAMESPACE_STATMODEL$comparisons_custom_pairwise_choice1]]) | ||
| index2 = which(condition_list == input[[NAMESPACE_STATMODEL$comparisons_custom_pairwise_choice2]]) | ||
|
|
||
| comp_list$dList = unique(c(isolate(comp_list$dList), | ||
| paste(input[[NAMESPACE_STATMODEL$comparisons_custom_pairwise_choice1]], "vs", input[[NAMESPACE_STATMODEL$comparisons_custom_pairwise_choice2]], sep = " "))) | ||
|
|
||
| contrast$row = matrix(row, nrow = 1) | ||
| contrast$row[index1] = 1 | ||
| contrast$row[index2] = -1 | ||
|
|
||
| if (is.null(contrast$matrix)) { | ||
| contrast$matrix = contrast$row | ||
| } else { | ||
| contrast$matrix = rbind(contrast$matrix, contrast$row) | ||
| contrast$matrix = rbind(contrast$matrix[!duplicated(contrast$matrix),]) | ||
| } | ||
|
|
||
| rownames(contrast$matrix) = comp_list$dList | ||
| colnames(contrast$matrix) = condition_list | ||
|
|
||
| return(contrast$matrix) | ||
| } |
There was a problem hiding this comment.
comp_list$dList and contrast$matrix are deduped independently — potential row-name mismatch.
comp_list$dList is deduped by name string (unique()), while contrast$matrix is deduped by row values (!duplicated()). If two differently-named comparisons happen to produce the same numeric row vector, the matrix will have fewer rows than comp_list$dList, and rownames(contrast$matrix) = comp_list$dList will fail with a dimension error.
Consider deduplicating both structures in tandem, or adding a length check before assigning row names.
Also applies to: 118-146
🤖 Prompt for AI Agents
In `@R/statmodel-server-comparisons.R` around lines 90 - 116, The code currently
deduplicates comp_list$dList (via unique()) and contrast$matrix (via !duplicated
on row values) independently, which can cause a row-name length mismatch in
build_custom_pairwise_contrast; fix by deduplicating names and rows in tandem:
after appending contrast$row and the new name, compute a canonical key for each
row (e.g., collapse numeric rows to a string) and use duplicated() on those keys
to derive a single set of unique indices, then subset both contrast$matrix and
comp_list$dList by the same unique indices before assigning rownames;
additionally add a guarded length check before rownames(contrast$matrix) =
comp_list$dList and apply the same tandem-dedup pattern in the analogous block
referenced (lines 118-146).
| get_tmt_moderation_radio_button <- function(loadpage_input, ns) { | ||
| if (loadpage_input$DDA_DIA == "TMT") { | ||
| create_moderation_radio_buttons(ns) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing NULL guard on loadpage_input$DDA_DIA.
get_response_curve_fitting_options (line 16) correctly guards against a NULL mode before comparing, but get_tmt_moderation_radio_button does not. If loadpage_input$DDA_DIA is NULL, the == comparison will produce logical(0), causing the if to error.
Proposed fix
get_tmt_moderation_radio_button <- function(loadpage_input, ns) {
- if (loadpage_input$DDA_DIA == "TMT") {
+ if (!is.null(loadpage_input$DDA_DIA) && loadpage_input$DDA_DIA == "TMT") {
create_moderation_radio_buttons(ns)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get_tmt_moderation_radio_button <- function(loadpage_input, ns) { | |
| if (loadpage_input$DDA_DIA == "TMT") { | |
| create_moderation_radio_buttons(ns) | |
| } | |
| } | |
| get_tmt_moderation_radio_button <- function(loadpage_input, ns) { | |
| if (!is.null(loadpage_input$DDA_DIA) && loadpage_input$DDA_DIA == "TMT") { | |
| create_moderation_radio_buttons(ns) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@R/statmodel-server-options-modeling.R` around lines 7 - 11, The if in
get_tmt_moderation_radio_button unguardedly compares loadpage_input$DDA_DIA to
"TMT" which can be NULL; update the condition to first ensure DDA_DIA is present
(e.g. !is.null(loadpage_input$DDA_DIA) && length(loadpage_input$DDA_DIA) > 0)
before comparing to "TMT", and only call create_moderation_radio_buttons(ns)
when that guarded comparison is true.
| extract_significant_proteins = function(data_comp, loadpage_input, signif_threshold) { | ||
| if (loadpage_input$BIO == "PTM") { | ||
| data_comp$ADJUSTED.Model[data_comp$ADJUSTED.Model$adj.pvalue < signif_threshold,] | ||
| } else if (loadpage_input$DDA_DIA == "TMT") { | ||
| data_comp$ComparisonResult[data_comp$ComparisonResult$adj.pvalue < signif_threshold,] | ||
| } else { | ||
| data_comp$ComparisonResult[which(data_comp$ComparisonResult$adj.pvalue < signif_threshold),] | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent NA handling between TMT and default branches.
The TMT path (line 53) uses direct < comparison which will retain rows where adj.pvalue is NA (as NA < threshold is NA, producing NA rows). The default path (line 55) wraps the condition in which(), which drops NA rows. This inconsistency looks unintentional — consider using which() uniformly, or at minimum adding & !is.na(...) to the TMT/PTM branches.
Proposed fix — use which() consistently
extract_significant_proteins = function(data_comp, loadpage_input, signif_threshold) {
if (loadpage_input$BIO == "PTM") {
- data_comp$ADJUSTED.Model[data_comp$ADJUSTED.Model$adj.pvalue < signif_threshold,]
+ data_comp$ADJUSTED.Model[which(data_comp$ADJUSTED.Model$adj.pvalue < signif_threshold),]
} else if (loadpage_input$DDA_DIA == "TMT") {
- data_comp$ComparisonResult[data_comp$ComparisonResult$adj.pvalue < signif_threshold,]
+ data_comp$ComparisonResult[which(data_comp$ComparisonResult$adj.pvalue < signif_threshold),]
} else {
data_comp$ComparisonResult[which(data_comp$ComparisonResult$adj.pvalue < signif_threshold),]
}
}🤖 Prompt for AI Agents
In `@R/statmodel-server-results-table.R` around lines 49 - 57, The function
extract_significant_proteins has inconsistent NA handling: the TMT branch uses a
direct adj.pvalue < signif_threshold (which preserves NA rows) while the default
branch uses which(...) (dropping NAs); update the TMT and PTM branches to match
the default by using which(data_comp$ComparisonResult$adj.pvalue <
signif_threshold) and which(data_comp$ADJUSTED.Model$adj.pvalue <
signif_threshold) respectively, or alternatively add an explicit !is.na(...)
(e.g. adj.pvalue < signif_threshold & !is.na(adj.pvalue)) to both branches so
both paths consistently exclude NA p-values when selecting rows from
data_comp$ComparisonResult and data_comp$ADJUSTED.Model in
extract_significant_proteins.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/statmodel-server-visualization.R`:
- Around line 45-57: The code calls prepare_dose_response_fit(contrast$matrix)
without guarding that contrast$matrix exists, causing a crash when it's NULL;
add a req(contrast$matrix) (or req(!is.null(contrast$matrix))) at the top of the
renderUI block so the UI only proceeds when contrast$matrix is available;
specifically update the renderUI for
output[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]] to call
req(contrast$matrix) before invoking prepare_dose_response_fit and then continue
with unique(response_curve_setup_matrix$drug) as before.
🧹 Nitpick comments (2)
R/statmodel-server-comparisons.R (2)
12-25: RedundantBIO == "PTM"checks in nested conditions.The outer
ifalready assertsBIO == "PTM", making the repeated checks in the inner conjunction redundant. This hurts readability and makes future maintenance error-prone. Also, prefer&&over&for scalar short-circuit evaluation inifguards.Simplified logic
get_experimental_conditions = function(loadpage_input, preprocess_data) { - if (loadpage_input$BIO == "PTM" & - ((loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA == "TMT") | - loadpage_input$filetype == 'phil')) { + if (loadpage_input$BIO == "PTM" && + (loadpage_input$DDA_DIA == "TMT" || loadpage_input$filetype == "phil")) { levels(preprocess_data$PTM$ProteinLevelData$Condition) - } else if (loadpage_input$BIO == "PTM" & - (loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA != "TMT")) { + } else if (loadpage_input$BIO == "PTM") { + # At this point BIO == "PTM" && DDA_DIA != "TMT" && filetype != "phil" levels(preprocess_data$PTM$ProteinLevelData$GROUP) } else if (loadpage_input$DDA_DIA == "TMT") { levels(preprocess_data$ProteinLevelData$Condition)
223-247:loadpage_inputparameter is unused.The
@paramdoc on line 219 already notes it as "(unused)". Consider removing it from the signature to keep the API clean, or add a comment explaining why it's retained (e.g., for interface consistency with other builders).Same applies to
inputandloadpage_inputinbuild_all_pair_contrast(line 260).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@R/statmodel-server-comparisons.R`:
- Around line 372-374: Replace the plain stop(...) call in the else branch (the
line with stop("No intervention columns found (time, temperature, or
treatment)")) with a Shiny-friendly validation so the reactive can fail
gracefully; for example, call validate(need(...)) with the same user-facing
message (or showNotification(...) followed by req(FALSE)) so the UI displays a
friendly message instead of surfacing an unhandled error. Ensure you keep the
original message text and apply this change inside the same reactive context
where the intervention column check occurs.
- Around line 124-130: The current validation uses exact equality check wt_sum
== 0 inside the validate(need(...)) call which is fragile for floating-point
sums; change the condition to use a tolerance check (e.g., abs(wt_sum) < tol)
and pass that into need, defining a small tol (like 1e-10) nearby; apply the
same replacement for the identical check in build_custom_non_pairwise_contrast
so both validations use the tolerance-based abs(...) comparison instead of == 0.
In `@R/statmodel-server-results-table.R`:
- Around line 28-31: The download handler output$download_compar currently
always writes data_comparison()$ComparisonResult which is NULL for PTM datasets;
update output$download_compar to check loadpage_input$BIO == "PTM" and, when
true, export data_comparison()$ADJUSTED.Model (matching the logic used in
extract_significant_proteins) otherwise fall back to
data_comparison()$ComparisonResult; ensure the filename/content functions use
the chosen data object and handle NULL gracefully (e.g., skip/notify or write an
empty CSV) so PTM downloads export the ADJUSTED.Model results rather than an
empty ComparisonResult.
🧹 Nitpick comments (3)
R/statmodel-server-comparisons.R (3)
12-25: RedundantBIO == "PTM"sub-conditions clutter the logic.Lines 14 and 18 re-check
loadpage_input$BIO == "PTM"inside branches that are already guarded byloadpage_input$BIO == "PTM". This makes the intent harder to read.Simplified logic
get_experimental_conditions = function(loadpage_input, preprocess_data) { - if (loadpage_input$BIO == "PTM" & - ((loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA == "TMT") | - loadpage_input$filetype == 'phil')) { + if (loadpage_input$BIO == "PTM" & + (loadpage_input$DDA_DIA == "TMT" | loadpage_input$filetype == 'phil')) { levels(preprocess_data$PTM$ProteinLevelData$Condition) - } else if (loadpage_input$BIO == "PTM" & - (loadpage_input$BIO == "PTM" & loadpage_input$DDA_DIA != "TMT")) { + } else if (loadpage_input$BIO == "PTM") { levels(preprocess_data$PTM$ProteinLevelData$GROUP) } else if (loadpage_input$DDA_DIA == "TMT") { levels(preprocess_data$ProteinLevelData$Condition) } else { levels(preprocess_data$ProteinLevelData$GROUP) } }
260-289:rownames/colnamesassigned inside the inner loop unnecessarily.Lines 282-283 set
rownamesandcolnameson every iteration of the inner loop, but only the final assignment matters. Moving them after both loops (before thereturn) would be cleaner and avoid repeated work.Proposed fix
} else { contrast$matrix = rbind(contrast$matrix, contrast$row) contrast$matrix = rbind(contrast$matrix[!duplicated(contrast$matrix),]) } - - rownames(contrast$matrix) = comp_list$dList - colnames(contrast$matrix) = condition_list } } } + rownames(contrast$matrix) = comp_list$dList + colnames(contrast$matrix) = condition_list + return(contrast$matrix) }
301-301: Variable namematrixshadows the base Rmatrix()function.Consider renaming to something like
condition_dforresult_dfto avoid confusion ifmatrix()is called later in the scope.
| wt_sum = sum(sapply(seq_along(condition_list), function(i) { | ||
| input[[paste0(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_weights, i)]] | ||
| })) | ||
|
|
||
| validate( | ||
| need(wt_sum == 0, "The contrast weights should sum up to 0") | ||
| ) |
There was a problem hiding this comment.
Floating-point equality check on weight sum.
wt_sum == 0 (line 129) could fail for fractional weights due to floating-point arithmetic (e.g., 0.1 + 0.2 - 0.3 != 0 in R). Consider using a tolerance: abs(wt_sum) < 1e-10.
Proposed fix
- need(wt_sum == 0, "The contrast weights should sum up to 0")
+ need(abs(wt_sum) < 1e-10, "The contrast weights should sum up to 0")Note: Apply the same fix to the identical check on line 187 in build_custom_non_pairwise_contrast.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wt_sum = sum(sapply(seq_along(condition_list), function(i) { | |
| input[[paste0(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_weights, i)]] | |
| })) | |
| validate( | |
| need(wt_sum == 0, "The contrast weights should sum up to 0") | |
| ) | |
| wt_sum = sum(sapply(seq_along(condition_list), function(i) { | |
| input[[paste0(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_weights, i)]] | |
| })) | |
| validate( | |
| need(abs(wt_sum) < 1e-10, "The contrast weights should sum up to 0") | |
| ) |
🤖 Prompt for AI Agents
In `@R/statmodel-server-comparisons.R` around lines 124 - 130, The current
validation uses exact equality check wt_sum == 0 inside the validate(need(...))
call which is fragile for floating-point sums; change the condition to use a
tolerance check (e.g., abs(wt_sum) < tol) and pass that into need, defining a
small tol (like 1e-10) nearby; apply the same replacement for the identical
check in build_custom_non_pairwise_contrast so both validations use the
tolerance-based abs(...) comparison instead of == 0.
| output$download_compar = downloadHandler( | ||
| filename = function() paste("test_result-", Sys.Date(), ".csv", sep = ""), | ||
| content = function(file) write.csv(data_comparison()$ComparisonResult, file) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ComparisonResult and ADJUSTED.Model usage
rg -n "ComparisonResult|ADJUSTED\.Model" --type r -C 3Repository: Vitek-Lab/MSstatsShiny
Length of output: 14576
Fix download handler to support PTM data structure.
The download handler unconditionally accesses data_comparison()$ComparisonResult, but PTM models store results under different keys: PTM.Model, PROTEIN.Model, and ADJUSTED.Model. For PTM users, ComparisonResult will be NULL, causing the download to fail or export empty data. The handler should check loadpage_input$BIO == "PTM" and export ADJUSTED.Model for PTM data, matching the logic in extract_significant_proteins (lines 44-52).
🤖 Prompt for AI Agents
In `@R/statmodel-server-results-table.R` around lines 28 - 31, The download
handler output$download_compar currently always writes
data_comparison()$ComparisonResult which is NULL for PTM datasets; update
output$download_compar to check loadpage_input$BIO == "PTM" and, when true,
export data_comparison()$ADJUSTED.Model (matching the logic used in
extract_significant_proteins) otherwise fall back to
data_comparison()$ComparisonResult; ensure the filename/content functions use
the chosen data object and handle NULL gracefully (e.g., skip/notify or write an
empty CSV) so PTM downloads export the ADJUSTED.Model results rather than an
empty ComparisonResult.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@R/statmodel-server-visualization.R`:
- Around line 125-129: The download handler's content function (content =
function(file)) assumes files <- list.files(... pattern="^Ex_") is non-empty and
uses latest_file <- files[which.max(file_info$mtime)] which fails when files is
empty; add a guard after listing files to check if length(files) == 0 and handle
it gracefully (e.g., stop() with a clear message like "No plot files available
to download" or create a placeholder/empty file), and only proceed to compute
file_info, which.max, and call file.copy(latest_file, file) when files is
non-empty; reference the symbols files, file_info, latest_file, which.max, and
file.copy in your change.
🧹 Nitpick comments (1)
R/statmodel-server-visualization.R (1)
81-112: TMT and default branches are nearly identical — consider extracting a helper.The only difference between the TMT (lines 82–96) and default (lines 97–112) branches is the guard on
loadpage_input$DDA_DIA == "TMT". ThegroupComparisonPlots(...)call, its arguments, and the[[1]]extraction are completely duplicated. Extracting the shared call into a local helper or collapsing the two branches would reduce maintenance burden.♻️ Sketch
- } else if (loadpage_input$DDA_DIA == "TMT") { - plot_result = groupComparisonPlots( - data = data_comparison$ComparisonResult, - ... - )[[1]] } else { plot_result = groupComparisonPlots( data = data_comparison$ComparisonResult, - ... + type = input[[NAMESPACE_STATMODEL$visualization_plot_type]], + sig = input[[NAMESPACE_STATMODEL$visualization_volcano_significance_cutoff]], + FCcutoff = fold_change_cutoff, + logBase.pvalue = as.numeric(input[[NAMESPACE_STATMODEL$visualization_logp_base]]), + ProteinName = input[[NAMESPACE_STATMODEL$visualization_volcano_display_protein_name]], + numProtein = input[[NAMESPACE_STATMODEL$visualization_heatmap_number_proteins]], + clustering = input[[NAMESPACE_STATMODEL$visualization_heatmap_cluster_option]], + which.Comparison = input[[NAMESPACE_STATMODEL$visualization_which_comparison]], + which.Protein = input[[NAMESPACE_STATMODEL$visualization_which_protein]], + height = input[[NAMESPACE_STATMODEL$visualization_plot_height_slider]], + address = "Ex_", + isPlotly = TRUE )[[1]] }#!/bin/bash # Check if TMT and default branches diverge elsewhere in the codebase rg -n "groupComparisonPlots" --type=r -C3
| content = function(file) { | ||
| files = list.files(getwd(), pattern = "^Ex_", full.names = TRUE) | ||
| file_info = file.info(files) | ||
| latest_file = files[which.max(file_info$mtime)] | ||
| file.copy(latest_file, file) |
There was a problem hiding this comment.
No guard against missing plot files.
If no files matching "^Ex_" exist in getwd() (e.g., user clicks download before generating a plot), which.max on an empty vector returns integer(0), and the subsequent file.copy/zip call will fail with an obscure error.
Proposed fix
content = function(file) {
files = list.files(getwd(), pattern = "^Ex_", full.names = TRUE)
+ if (length(files) == 0) {
+ showNotification("No plot files found. Please generate a plot first.", type = "warning")
+ return(NULL)
+ }
file_info = file.info(files)
latest_file = files[which.max(file_info$mtime)]🤖 Prompt for AI Agents
In `@R/statmodel-server-visualization.R` around lines 125 - 129, The download
handler's content function (content = function(file)) assumes files <-
list.files(... pattern="^Ex_") is non-empty and uses latest_file <-
files[which.max(file_info$mtime)] which fails when files is empty; add a guard
after listing files to check if length(files) == 0 and handle it gracefully
(e.g., stop() with a clear message like "No plot files available to download" or
create a placeholder/empty file), and only proceed to compute file_info,
which.max, and call file.copy(latest_file, file) when files is non-empty;
reference the symbols files, file_info, latest_file, which.max, and file.copy in
your change.
1 participant
Motivation and Context
This PR refactors the statmodel server by extracting helper logic from the large R/module-statmodel-server.R into focused modules to improve organization, readability, and testability while preserving existing behavior and public API. The main module now orchestrates reactivity and delegates domain-specific tasks (contrast construction, visualization, results handling, modeling options, and code generation) to new helper files.
Summary of the solution
Detailed changes
R/module-statmodel-server.R
R/statmodel-server-comparisons.R (+417 lines)
R/statmodel-server-visualization.R (+132 lines)
R/statmodel-server-results-table.R (+52 lines)
R/statmodel-server-download-code.R (+55 lines)
R/statmodel-server-options-modeling.R (+24 lines)
Tests and project files
Unit tests added or modified