Refactor loadpage: move converter panels to server-side show/hide#217
Refactor loadpage: move converter panels to server-side show/hide#217swaraj-neu wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe ChangesLoadpage Phase 2 split
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/module-loadpage-ui.R (1)
858-878: 🎯 Functional Correctness | 🟠 MajorMake the
conditionalPanelconditions respect the module namespace.Lines 858 and 872 use hardcoded IDs (
loadpage-filetype) for theconditionattribute, while the inputs are namespaced withns(). This mismatch causes the conditional logic to fail whenever the module is instantiated with a prefix other than the literal string"loadpage".The logic must be updated to generate the ID string for the condition using
ns.Suggested Fix
Replace static ID strings with dynamically constructed ones using
sprintfandns.conditionalPanel( - condition = "input['loadpage-filetype'] == 'spec'", + condition = sprintf("input['%s'] == 'spec'", ns("filetype")), checkboxInput(ns("calculate_anomaly_scores"), @@ conditionalPanel( - condition = "input['loadpage-calculate_anomaly_scores']", + condition = sprintf("input['%s']", ns("calculate_anomaly_scores")), fileInput(ns("run_order_file"),Note: Ensure
"filetype"matches the actual ID passed tofileInputor the relevant global ID if one exists in this scope. If'loadpage-filetype'was a hard reference to a global or parent container ID that doesn't follow standard module namespacing, verify if a globalnsexists, but standard practice suggestsns("filetype")should map to the correct conditionalPanel target if that input is also namespaced or if a globalnsis available.If the inputs inside are namespaced, the condition checking them must also use that namespace to ensure they are visible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/module-loadpage-ui.R` around lines 858 - 878, The conditionalPanel checks are using hardcoded input IDs instead of the module namespace, so the visibility logic breaks for non-default module prefixes. Update the condition strings in the loadpage UI block to build the namespaced ID from ns for the filetype and calculate_anomaly_scores inputs, and verify the nested run_order_file panel still targets the correct namespaced control within the same module. Use the existing ns helper in the module UI code so the condition matches the actual input IDs created by checkboxInput and fileInput.
🧹 Nitpick comments (2)
R/loadpage-server-summary.R (2)
186-189: 🩺 Stability & Availability | 🔵 TrivialRegister
proceed2only once.The
onclick("proceed2", ...)call is nested inside theproceed1handler, causing theproceed2event listener to be re-registered every timeproceed1is clicked. Move theproceed2block to the same scope level as theproceed1block so it initializes only once.onclick("proceed2", { updateTabsetPanel(session = parent_session, inputId = "tablist", selected = "DataProcessing") }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/loadpage-server-summary.R` around lines 186 - 189, The proceed2 click handler is being registered inside the proceed1 callback, so it gets added again on every proceed1 click; move the onclick("proceed2", ...) registration out to the same scope as onclick("proceed1", ...) in loadpage-server-summary.R so it initializes only once. Use the existing proceed1/proceed2 handler blocks and keep the updateTabsetPanel(session = parent_session, inputId = "tablist", selected = "DataProcessing") behavior unchanged.
190-228: 🎯 Functional Correctness | 🔵 TrivialInconsistent ID referencing breaks module reusability
Although
loadpageServeris currently called with the specific ID"loadpage", the code usesnsfor dynamic ID generation in other places but hard-codes"loadpage-"" in CSS selectors andconditionalPanel` conditions. This renders the module brittle and unusable if renamed or wrapped in another namespace later.Update the references to use the namespace function consistently:
- CSS Selector: Generate the ID dynamically instead of hard-coding the prefix:
tags$style(HTML(sprintf('#%s{background-color:orange}', ns("proceed2"))))- conditionalPanel: Pass
ns = nsto handle the input ID translation automatically:conditionalPanel(condition = "input.BIO !== 'PTM'", ns = ns, ...)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/loadpage-server-summary.R` around lines 190 - 228, The UI in renderUI for summary_tables hard-codes the module prefix in the CSS selector and conditionalPanel logic, which breaks reuse if loadpageServer is renamed or nested. Update the proceed2 style in tags$head to use ns("proceed2") instead of a fixed selector, and pass ns = ns into each conditionalPanel so input IDs are translated automatically. Keep the existing renderUI structure and identifiers like summary_tables, proceed2, and conditionalPanel, but make all ID references namespace-aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@R/loadpage-server-preview.R`:
- Around line 35-59: The DIANN preview resolution in main_data_file() only
handles input$dianndata, so local large-file uploads never reach preview_data()
and skip the diann_2plus auto-detect/mismatch-warning flow. Update
main_data_file() to also consider the local_big_diann_path state used by
register_loadpage_proceed_validation(), and for that branch return a preview
source shaped like {datapath, name} so preview_data() can load it the same way
as other inputs.
- Around line 93-105: The preview handling in loadpage-server-preview.R treats
every non-2.0+ result from .is_diann_2plus() as DIANN 1.x, which is incorrect
for unknown formats. Update the logic around last_detected_diann_format(),
updateCheckboxInput(), and the showNotification calls so that only a confirmed
positive detection maps to “DIANN 2.0+”, a confirmed legacy fragment column maps
to “DIANN 1.x”, and the no-match case leaves the state unset/unchanged and shows
an unknown-format message instead. Apply the same distinction in the mismatch
warning path referenced by the same preview update flow.
In `@R/loadpage-server-rendering.R`:
- Around line 855-901: The radio-button selectors in the observe block are
hardcoded to the global loadpage-filetype name, so they won’t work correctly
when the module ID changes or multiple instances exist. Update the selector
logic in this observe() path to use session$ns with NAMESPACE_LOADPAGE$filetype
so the [type=radio][name=...] lookups are module-scoped, and keep the
value-based disable calls tied to that namespaced radio group.
In `@R/loadpage-server-summary.R`:
- Around line 78-81: The proceed1 click handler currently shows summary_tables
even when get_data() fails and returns NULL. Update the proceed1 callback to
capture the result of get_data() and only continue to get_annot() and
shinyjs::show("summary_tables") when a valid dataset is loaded; otherwise stop
early and keep the Next step hidden. Use the proceed1 onclick handler and
get_data() as the key points to locate the fix.
- Around line 85-97: Surface failures in the protein_turnover metadata setup
instead of swallowing them in the tryCatch around get_data and
condition_metadata. In the app_template() == TEMPLATES$protein_turnover branch,
update the error handler to mirror the chemoproteomics path by logging a warning
or message with the caught error e, so loadpage-server-summary.R makes
initialization issues visible while still allowing the UI to render.
In `@R/module-loadpage-server.R`:
- Around line 13-15: The helper-count documentation is outdated in the
module-loadpage server orchestrator. Update the roxygen text and the related
section header in the code that documents the helper registrations so they say
six helpers instead of five, keeping the wording in sync with the actual
registration order in the orchestrator and the final return list.
---
Outside diff comments:
In `@R/module-loadpage-ui.R`:
- Around line 858-878: The conditionalPanel checks are using hardcoded input IDs
instead of the module namespace, so the visibility logic breaks for non-default
module prefixes. Update the condition strings in the loadpage UI block to build
the namespaced ID from ns for the filetype and calculate_anomaly_scores inputs,
and verify the nested run_order_file panel still targets the correct namespaced
control within the same module. Use the existing ns helper in the module UI code
so the condition matches the actual input IDs created by checkboxInput and
fileInput.
---
Nitpick comments:
In `@R/loadpage-server-summary.R`:
- Around line 186-189: The proceed2 click handler is being registered inside the
proceed1 callback, so it gets added again on every proceed1 click; move the
onclick("proceed2", ...) registration out to the same scope as
onclick("proceed1", ...) in loadpage-server-summary.R so it initializes only
once. Use the existing proceed1/proceed2 handler blocks and keep the
updateTabsetPanel(session = parent_session, inputId = "tablist", selected =
"DataProcessing") behavior unchanged.
- Around line 190-228: The UI in renderUI for summary_tables hard-codes the
module prefix in the CSS selector and conditionalPanel logic, which breaks reuse
if loadpageServer is renamed or nested. Update the proceed2 style in tags$head
to use ns("proceed2") instead of a fixed selector, and pass ns = ns into each
conditionalPanel so input IDs are translated automatically. Keep the existing
renderUI structure and identifiers like summary_tables, proceed2, and
conditionalPanel, but make all ID references namespace-aware.
🪄 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: 958ed1a8-ecd3-49e6-8a81-78ef434b9bb1
📒 Files selected for processing (10)
R/constants.RR/loadpage-server-data-loaders.RR/loadpage-server-preview.RR/loadpage-server-proceed-validation.RR/loadpage-server-rendering.RR/loadpage-server-summary.RR/module-loadpage-server.RR/module-loadpage-ui.Rtests/testthat/test-loadpage-server-rendering.Rtests/testthat/test-module-loadpage-ui.R
| main_data_file <- reactive({ | ||
| req(input$filetype) | ||
| if (input$BIO == "PTM") { | ||
| switch(input$filetype, | ||
| "meta" = input$ptm_input, | ||
| # TODO: "maxq" = input$ptm_input, | ||
| # TODO: "PD" = input$ptm_input, | ||
| # TODO: "spec" = input$ptm_input, | ||
| # TODO: "sky" = input$ptm_input, | ||
| # TODO: "phil" = input$ptmdata, | ||
| # TODO: "msstats" = input$msstatsptmdata, | ||
| NULL | ||
| ) | ||
| } else { | ||
| switch(input$filetype, | ||
| # TODO: Map remaining non-PTM file types when preview features are needed | ||
| "prog" =, "PD" =, "open" =, "openms" =, "spmin" =, "phil" =, "meta" = input$data, | ||
| "msstats" = input$msstatsdata, | ||
| "sky" = input$skylinedata, | ||
| "spec" = input$specdata, | ||
| "diann" = input$dianndata, | ||
| "maxq" = input$evidence, | ||
| NULL | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wire DIANN large-file mode into preview resolution.
register_loadpage_proceed_validation() enables the DIANN large-file path flow in R/loadpage-server-proceed-validation.R Lines 68-73, but main_data_file() here only resolves input$dianndata. In local large-file mode that input stays NULL, so preview_data() never loads and the diann_2plus auto-detect / mismatch-warning path is skipped for a supported upload flow. Please thread local_big_diann_path into this helper and synthesize a {datapath, name}-shaped preview source for that branch too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/loadpage-server-preview.R` around lines 35 - 59, The DIANN preview
resolution in main_data_file() only handles input$dianndata, so local large-file
uploads never reach preview_data() and skip the diann_2plus
auto-detect/mismatch-warning flow. Update main_data_file() to also consider the
local_big_diann_path state used by register_loadpage_proceed_validation(), and
for that branch return a preview source shaped like {datapath, name} so
preview_data() can load it the same way as other inputs.
| is_2plus <- .is_diann_2plus(preview) | ||
| previous <- last_detected_diann_format() | ||
| # Only update and notify when the detected state actually changes | ||
| if (is.null(previous) || previous != is_2plus) { | ||
| updateCheckboxInput(session, "diann_2plus", value = is_2plus) | ||
| if (is_2plus) { | ||
| showNotification("Detected DIANN 2.0+ format (per-fragment columns).", | ||
| type = "message", duration = 5) | ||
| } else { | ||
| showNotification("Detected DIANN 1.x format (legacy fragment column).", | ||
| type = "message", duration = 5) | ||
| } | ||
| last_detected_diann_format(is_2plus) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t equate “not 2.0+” with “definitely DIANN 1.x”.
.is_diann_2plus() only answers the positive case. A preview with neither numbered fragment columns nor the legacy fragment column also returns FALSE, so this code auto-unchecks the box and tells the user “DIANN 1.x” even when the format is actually unknown; the mismatch warning below repeats the same assumption.
Suggested direction
- is_2plus <- .is_diann_2plus(preview)
+ cols <- names(preview)
+ is_2plus <- .is_diann_2plus(preview)
+ is_legacy <- any(cols %in% c("Fragment.Quant.Corrected", "FragmentQuantCorrected"))
previous <- last_detected_diann_format()
# Only update and notify when the detected state actually changes
- if (is.null(previous) || previous != is_2plus) {
- updateCheckboxInput(session, "diann_2plus", value = is_2plus)
- if (is_2plus) {
+ if (is_2plus && (is.null(previous) || !isTRUE(previous))) {
+ updateCheckboxInput(session, "diann_2plus", value = TRUE)
showNotification("Detected DIANN 2.0+ format (per-fragment columns).",
type = "message", duration = 5)
- } else {
+ last_detected_diann_format(TRUE)
+ } else if (is_legacy && (is.null(previous) || !identical(previous, FALSE))) {
+ updateCheckboxInput(session, "diann_2plus", value = FALSE)
showNotification("Detected DIANN 1.x format (legacy fragment column).",
type = "message", duration = 5)
- }
- last_detected_diann_format(is_2plus)
+ last_detected_diann_format(FALSE)
+ } else if (!is_2plus && !is_legacy) {
+ showNotification("Could not determine whether the preview is DIANN 1.x or 2.0+.",
+ type = "warning", duration = 5)
}Also applies to: 114-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/loadpage-server-preview.R` around lines 93 - 105, The preview handling in
loadpage-server-preview.R treats every non-2.0+ result from .is_diann_2plus() as
DIANN 1.x, which is incorrect for unknown formats. Update the logic around
last_detected_diann_format(), updateCheckboxInput(), and the showNotification
calls so that only a confirmed positive detection maps to “DIANN 2.0+”, a
confirmed legacy fragment column maps to “DIANN 1.x”, and the no-match case
leaves the state unset/unchanged and shows an unknown-format message instead.
Apply the same distinction in the mismatch warning path referenced by the same
preview update flow.
| observe({ | ||
| if ((input$BIO == "Protein" || input$BIO == "Peptide") && input$DDA_DIA == "LType") { | ||
| runjs("$('[type=radio][name=loadpage-filetype]:disabled').parent().parent().parent().find('div.radio').css('opacity', 1)") | ||
| enable("filetype") | ||
| disable(selector = "[type=radio][value=spmin]") | ||
| runjs("$.each($('[type=radio][name=loadpage-filetype]:disabled'), function(_, e){ $(e).parent().parent().css('opacity', 0.4) })") | ||
|
|
||
| } else if ((input$BIO == "Protein" || input$BIO == "Peptide") && input$DDA_DIA == "TMT") { | ||
| runjs("$('[type=radio][name=loadpage-filetype]:disabled').parent().parent().parent().find('div.radio').css('opacity', 1)") | ||
| enable("filetype") | ||
| disable(selector = "[type=radio][value=sky]") | ||
| disable(selector = "[type=radio][value=prog]") | ||
| disable(selector = "[type=radio][value=spec]") | ||
| disable(selector = "[type=radio][value=open]") | ||
| disable(selector = "[type=radio][value=ump]") | ||
| disable(selector = "[type=radio][value=diann]") | ||
| disable(selector = "[type=radio][value=meta]") | ||
| runjs("$.each($('[type=radio][name=loadpage-filetype]:disabled'), function(_, e){ $(e).parent().parent().css('opacity', 0.4) })") | ||
|
|
||
| } else if (input$BIO == "PTM" && input$DDA_DIA == "LType") { | ||
| runjs("$('[type=radio][name=loadpage-filetype]:disabled').parent().parent().parent().find('div.radio').css('opacity', 1)") | ||
| enable("filetype") | ||
| # disable(selector = "[type=radio][value=sky]") | ||
| disable(selector = "[type=radio][value=prog]") | ||
| disable(selector = "[type=radio][value=PD]") | ||
| disable(selector = "[type=radio][value=openms]") | ||
| disable(selector = "[type=radio][value=spmin]") | ||
| disable(selector = "[type=radio][value=open]") | ||
| disable(selector = "[type=radio][value=ump]") | ||
| disable(selector = "[type=radio][value=phil]") | ||
| disable(selector = "[type=radio][value=diann]") | ||
|
|
||
| runjs("$.each($('[type=radio][name=loadpage-filetype]:disabled'), function(_, e){ $(e).parent().parent().css('opacity', 0.4) })") | ||
| } else if (input$BIO == "PTM" && input$DDA_DIA == "TMT") { | ||
| runjs("$('[type=radio][name=loadpage-filetype]:disabled').parent().parent().parent().find('div.radio').css('opacity', 1)") | ||
| enable("filetype") | ||
| disable(selector = "[type=radio][value=prog]") | ||
| disable(selector = "[type=radio][value=openms]") | ||
| disable(selector = "[type=radio][value=spec]") | ||
| disable(selector = "[type=radio][value=open]") | ||
| disable(selector = "[type=radio][value=ump]") | ||
| disable(selector = "[type=radio][value=spmin]") | ||
| disable(selector = "[type=radio][value=diann]") | ||
| disable(selector = "[type=radio][value=sky]") | ||
| disable(selector = "[type=radio][value=meta]") | ||
|
|
||
| runjs("$.each($('[type=radio][name=loadpage-filetype]:disabled'), function(_, e){ $(e).parent().parent().css('opacity', 0.4) })") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
cat -n R/loadpage-server-rendering.R | head -n 920Repository: Vitek-Lab/MSstatsShiny
Length of output: 41220
🏁 Script executed:
grep -n "radio.*filetype\|actionButton.*filetype" R/module-loadpage-ui.R | head -20Repository: Vitek-Lab/MSstatsShiny
Length of output: 197
Scope the [type=radio][name=...] selectors to the module namespace.
The selectors at lines 858–901 hardcode loadpage-filetype and global [value=...] attributes. If this module is mounted with a different ID or multiple instances are added, these selectors will target the wrong elements or fail entirely.
Use session$ns(NAMESPACE_LOADPAGE$filetype) to construct the correct name attribute for the radio group.
Example fix
+ filetype_name <- session$ns(NAMESPACE_LOADPAGE$filetype)
+ filetype_name_attr <- sprintf("[name='%s']", filetype_name)
+ filetype_selector <- sprintf("[type=radio]%s", filetype_name_attr)
+
observe({
if ((input$BIO == "Protein" || input$BIO == "Peptide") && input$DDA_DIA == "LType") {
- runjs("$('[type=radio][name=loadpage-filetype]:disabled')...")
+ runjs(sprintf("$('%s:disabled').parent().parent().parent().find('div.radio').css('opacity', 1)", filetype_selector))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/loadpage-server-rendering.R` around lines 855 - 901, The radio-button
selectors in the observe block are hardcoded to the global loadpage-filetype
name, so they won’t work correctly when the module ID changes or multiple
instances exist. Update the selector logic in this observe() path to use
session$ns with NAMESPACE_LOADPAGE$filetype so the [type=radio][name=...]
lookups are module-scoped, and keep the value-based disable calls tied to that
namespaced radio group.
| onclick("proceed1", { | ||
| get_data() | ||
| get_annot() | ||
| shinyjs::show("summary_tables") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop before showing summaries when data loading fails.
get_data() can return NULL after a loader error, but this still shows summary_tables and exposes “Next step” with no valid dataset. Gate on the loaded value first.
Proposed fix
onclick("proceed1", {
- get_data()
+ loaded_data <- get_data()
+ req(!is.null(loaded_data))
get_annot()
shinyjs::show("summary_tables")📝 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.
| onclick("proceed1", { | |
| get_data() | |
| get_annot() | |
| shinyjs::show("summary_tables") | |
| onclick("proceed1", { | |
| loaded_data <- get_data() | |
| req(!is.null(loaded_data)) | |
| get_annot() | |
| shinyjs::show("summary_tables") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/loadpage-server-summary.R` around lines 78 - 81, The proceed1 click handler
currently shows summary_tables even when get_data() fails and returns NULL.
Update the proceed1 callback to capture the result of get_data() and only
continue to get_annot() and shinyjs::show("summary_tables") when a valid dataset
is loaded; otherwise stop early and keep the Next step hidden. Use the proceed1
onclick handler and get_data() as the key points to locate the 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) {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Surface protein-turnover metadata initialization failures.
The empty error handler silently leaves condition_metadata unset while rendering the turnover UI. Mirror the chemoproteomics warning path.
Proposed fix
- }, error = function(e) {})
+ }, error = function(e) {
+ condition_metadata(NULL)
+ showNotification(
+ paste("Could not initialize condition metadata:", conditionMessage(e)),
+ type = "warning",
+ duration = 6
+ )
+ })📝 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.
| 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/loadpage-server-summary.R` around lines 85 - 97, Surface failures in the
protein_turnover metadata setup instead of swallowing them in the tryCatch
around get_data and condition_metadata. In the app_template() ==
TEMPLATES$protein_turnover branch, update the error handler to mirror the
chemoproteomics path by logging a warning or message with the caught error e, so
loadpage-server-summary.R makes initialization issues visible while still
allowing the UI to render.
|
| ) | ||
|
|
||
| NAMESPACE_LOADPAGE = list( | ||
| # Cross-module public IDs (read from outside the loadpage module). |
There was a problem hiding this comment.
Could you remove the comments for the constants, they don't seem to add anything to understanding these constants.
| # Extracted from R/module-loadpage-server.R by the Phase 2 server split. | ||
| # Pure cut-and-paste: no behavior change, no reactivity timing change, no | ||
| # input-ID renames. Owns: | ||
| # - 11 single-file wrapper reactives (`get_annot`, `get_annot1/2/3`, | ||
| # `get_evidence`, `get_evidence2`, `get_global`, `get_proteinGroups`, | ||
| # `get_proteinGroups2`, `get_FragSummary`, `get_peptideSummary`, | ||
| # `get_protSummary`, `get_maxq_ptm_sites`) | ||
| # - the lynchpin `get_data` eventReactive (triggered on `proceed1`) | ||
| # - the download_msstats_format downloadHandler + its enable/disable | ||
| # observers | ||
| # - `get_data_code` (triggered on `calculate`) | ||
| # - `get_summary1`, `get_summary2` (triggered on `proceed1`) |
There was a problem hiding this comment.
Could you get rid of these comments and similar comments? Seems to be written in a way where you need context from the previous commit
| @@ -0,0 +1,177 @@ | |||
| # ============================================================================ | |||
| # Loadpage — data-loading reactives + download MSstats handler + summaries | |||
There was a problem hiding this comment.
Could you elaborate that the summaries are summary experimental statistics, e.g. number of conditions, number of replicates etc?
There was a problem hiding this comment.
@tonywu1999 Done. Clarified that these are the experimental-design summary statistics (number of conditions, number of replicates, etc.). Since they're moving to loadpage-server-summary.R per your other comment, the clarified description now lives there.
| get_summary1 <- eventReactive(input$proceed1, { | ||
| getSummary1(input, get_data(), get_annot()) | ||
| }) | ||
|
|
||
| get_summary2 <- eventReactive(input$proceed1, { | ||
| getSummary2(input, get_data()) | ||
| }) |
There was a problem hiding this comment.
Should this be in loadpage-server-summary.R?
| output$template <- downloadHandler( | ||
| filename = "extdata/templateannotation.csv", | ||
|
|
||
| content = function(file) { | ||
| file.copy("extdata/templateannotation.csv", file) | ||
| }, | ||
| contentType = "csv" | ||
| ) | ||
|
|
||
| output$template1 <- downloadHandler( | ||
| filename = function() { | ||
| paste("extdata/templateevidence", "txt", sep = ".") | ||
| }, | ||
|
|
||
| content = function(file) { | ||
| file.copy("extdata/templateevidence.txt", file) | ||
| }, | ||
| contentType = "txt" | ||
| ) |
There was a problem hiding this comment.
Could you remind me where we see the template downloads in the loadpage tab? I don't recall being able to download anything like this.
There was a problem hiding this comment.
@tonywu1999 you're right, there's no way to download these in the loadpage tab, they're dead handlers that predate this PR, want me to remove them?
| @@ -0,0 +1,907 @@ | |||
| # ============================================================================ | |||
There was a problem hiding this comment.
Can we rename this file to loadpage-server-converter-options-panel.R?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/loadpage-server-converter-options-panel.R (2)
60-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the LabelFreeType picker’s ancestor gates in sample descriptions.
sample_*_description_paneldivs are mounted separately fromlabel_free_type_selection_panel, so staleLabelFreeTypecan keep a sample description visible after switching to TMT or PTM. Mirrorloadpage_show_label_free_type_selection()by also checkingbioanddda_dia.Proposed fix
-loadpage_show_sample_dataset_description <- function(filetype, label_free_type, mode) { - isTRUE(filetype == "sample") && isTRUE(label_free_type == mode) +loadpage_show_sample_dataset_description <- function(bio, filetype, dda_dia, + label_free_type, mode) { + loadpage_show_label_free_type_selection(bio, filetype, dda_dia) && + isTRUE(label_free_type == mode) }condition = loadpage_show_sample_dataset_description( + input[[NAMESPACE_LOADPAGE$bio]], input[[NAMESPACE_LOADPAGE$filetype]], + input[[NAMESPACE_LOADPAGE$dda_dia]], input[[NAMESPACE_LOADPAGE$label_free_type]], "DDA" )Apply the same argument additions to the DIA and SRM/PRM calls.
Also applies to: 366-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/loadpage-server-converter-options-panel.R` around lines 60 - 62, The sample description visibility logic in loadpage_show_sample_dataset_description only checks filetype and label_free_type, so stale LabelFreeType state can leave sample panels visible after switching modes. Update this helper to mirror loadpage_show_label_free_type_selection by also requiring the bio and dda_dia ancestor gates, and then propagate the same extra arguments through the DIA and SRM/PRM call sites so sample_*_description_panel visibility stays in sync with label_free_type_selection_panel.
804-851: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
input$filetypewhen a selected converter becomes disabled. The observer only disables and dims radios; if the user had already picked one,input$filetypecan stay on that invalid value and keep driving theloadpage_show_*/getData()branches. Clear it or switch to a valid fallback when the active option is disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/loadpage-server-converter-options-panel.R` around lines 804 - 851, The observer in loadpage-server-converter-options-panel.R only disables and dims filetype radios, but it does not clear an already-selected now-invalid value, so input$filetype can still drive loadpage_show_* and getData() with a disabled converter. Update the observe block that handles BIO/DDA_DIA cases to reset input$filetype or assign a valid fallback whenever the chosen option is among the disabled values, using the existing filetype enable/disable logic and the loadpage_show_* branches as the reference points.
🧹 Nitpick comments (1)
R/loadpage-server-data-loaders.R (1)
89-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle
utils::zip()failures explicitly
utils::zip()returns an exit status rather than raising an R error, so a non-zero exit can leave an empty/invalid.zipwithout entering theerrorhandler.- The fallback
writeLines()path writes plain text into the download payload, which produces a broken.csv/.zipsilently. Surface the failure withshowNotification()and abort the download instead of writing an error message into the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/loadpage-server-data-loaders.R` around lines 89 - 125, The MSstats download handler in download_msstats_format currently relies on tryCatch, but utils::zip() can fail by returning a non-zero exit status without throwing, and the error fallback writes text into the download file. Update the content function to explicitly check the zip result after calling utils::zip() and, on failure, use showNotification() to report the problem and abort instead of writing any error payload into file; keep the data.frame path and the per-table export logic in download_msstats_format unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@R/loadpage-server-converter-options-panel.R`:
- Around line 60-62: The sample description visibility logic in
loadpage_show_sample_dataset_description only checks filetype and
label_free_type, so stale LabelFreeType state can leave sample panels visible
after switching modes. Update this helper to mirror
loadpage_show_label_free_type_selection by also requiring the bio and dda_dia
ancestor gates, and then propagate the same extra arguments through the DIA and
SRM/PRM call sites so sample_*_description_panel visibility stays in sync with
label_free_type_selection_panel.
- Around line 804-851: The observer in loadpage-server-converter-options-panel.R
only disables and dims filetype radios, but it does not clear an
already-selected now-invalid value, so input$filetype can still drive
loadpage_show_* and getData() with a disabled converter. Update the observe
block that handles BIO/DDA_DIA cases to reset input$filetype or assign a valid
fallback whenever the chosen option is among the disabled values, using the
existing filetype enable/disable logic and the loadpage_show_* branches as the
reference points.
---
Nitpick comments:
In `@R/loadpage-server-data-loaders.R`:
- Around line 89-125: The MSstats download handler in download_msstats_format
currently relies on tryCatch, but utils::zip() can fail by returning a non-zero
exit status without throwing, and the error fallback writes text into the
download file. Update the content function to explicitly check the zip result
after calling utils::zip() and, on failure, use showNotification() to report the
problem and abort instead of writing any error payload into file; keep the
data.frame path and the per-table export logic in download_msstats_format
unchanged otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2343e99c-9eda-42a0-92eb-345e68c34f05
📒 Files selected for processing (8)
R/constants.RR/loadpage-server-converter-options-panel.RR/loadpage-server-data-loaders.RR/loadpage-server-preview.RR/loadpage-server-proceed-validation.RR/loadpage-server-summary.RR/module-loadpage-ui.Rtests/testthat/test-module-loadpage-ui.R
💤 Files with no reviewable changes (1)
- R/constants.R
🚧 Files skipped from review as they are similar to previous changes (5)
- R/loadpage-server-proceed-validation.R
- R/loadpage-server-preview.R
- tests/testthat/test-module-loadpage-ui.R
- R/loadpage-server-summary.R
- R/module-loadpage-ui.R
2 participants
Motivation and context
Refactor the loadpage flow to move UI show/hide logic out of client-side
conditionalPanelhandling and into server-controlled visibility, while splitting the module server code into smaller helper files. The change is intended to preserve behavior while improving structure and maintainability.Changes
NAMESPACE_LOADPAGEto centralize loadpage-related container/input ids.conditionalPanellogic, including:loadpageServer()to act as an orchestrator that wires the new helpers together and returns a smaller public interface.Tests
NAMESPACE_LOADPAGEid literalsloadpage_seed_proteinid()state-preservation behaviorconditionalPanelcasesconditionalPanelexpectations where server-side visibility now appliesCoding guidelines violated