Refactor loadpage: move converter panels to server-side show/hide#217
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe loadpage module is split into helper registrations for preview, visibility, proceed gating, data loading, and summary handling. UI visibility moves to shared namespaced IDs and server-driven toggles, with new tests covering the gating predicates and rendered containers. ChangesLoadpage Phase 2 split
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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.
| 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.
|
| 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?
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
| conditionalPanel(condition = "input['loadpage-BIO'] !== 'PTM'", | ||
| h4("Top 6 rows of the dataset"), | ||
| div(style = "overflow-x: auto;", tableOutput(ns("summary"))) | ||
| ), | ||
| conditionalPanel(condition = "input['loadpage-BIO'] == 'PTM'", | ||
| h4("Top 6 rows of the PTM dataset"), | ||
| div(style = "overflow-x: auto;", tableOutput(ns("summary_ptm"))), | ||
| tags$br(), | ||
| h4("Top 6 rows of the unmodified protein dataset"), | ||
| div(style = "overflow-x: auto;", tableOutput(ns("summary_prot"))) |
There was a problem hiding this comment.
There's still conditional panels here. Could you adjust?
| # PTM MSstats format. (The original JS condition had a redundant TMT | ||
| # clause that collapsed to `BIO=='PTM'`; the server predicate folds it.) |
There was a problem hiding this comment.
You can remove this comment
There was a problem hiding this comment.
For both DIANN and Spectronaut, I'm pretty sure you can migrate away from the conditional panel logic here. I would do renderUI - it doesn't matter that an uploaded file needs to be reuploaded if a user switches from DIANN to Spectronaut or another converter. The run order file is really small and we can get away with reuploading it.
…d template handlers
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 (1)
R/loadpage-server-converter-options-panel.R (1)
53-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the LabelFreeType selector’s ancestor gate in sample descriptions.
The description panels are standalone hidden divs, so a stale
LabelFreeTypeselection can still show DDA/DIA/SRM text after switching away from the label-free sample workflow. Reuse the selector gate so descriptions hide whenever BIO / DDA_DIA hides the picker.Proposed fix
-loadpage_show_sample_dataset_description <- function(filetype, label_free_type, label_free_mode) { - isTRUE(filetype == "sample") && isTRUE(label_free_type == label_free_mode) +loadpage_show_sample_dataset_description <- function(bio, filetype, dda_dia, + label_free_type, + label_free_mode) { + loadpage_show_sample_dataset_label_free_type_selector(bio, filetype, dda_dia) && + isTRUE(label_free_type == label_free_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 update to the DIA and SRM/PRM observers.
Also applies to: 353-381
🤖 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 53 - 55, The sample description visibility logic in loadpage_show_sample_dataset_description only checks filetype and label_free_type, so it can keep showing stale DDA/DIA/SRM text after the LabelFreeType picker is hidden. Update loadpage_show_sample_dataset_description to also take the selector’s ancestor gate used for BIO/DDA_DIA visibility, and require that gate to be true before showing any description. Then apply the same argument update in the DIA and SRM/PRM observers so all description panels use the shared gate consistently.
🤖 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 53-55: The sample description visibility logic in
loadpage_show_sample_dataset_description only checks filetype and
label_free_type, so it can keep showing stale DDA/DIA/SRM text after the
LabelFreeType picker is hidden. Update loadpage_show_sample_dataset_description
to also take the selector’s ancestor gate used for BIO/DDA_DIA visibility, and
require that gate to be true before showing any description. Then apply the same
argument update in the DIA and SRM/PRM observers so all description panels use
the shared gate consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab223b5b-682b-4472-aaec-a277a7ff244b
📒 Files selected for processing (6)
R/constants.RR/loadpage-server-converter-options-panel.RR/loadpage-server-summary.RR/module-loadpage-ui.Rtests/testthat/test-loadpage-server-rendering.Rtests/testthat/test-module-loadpage-ui.R
💤 Files with no reviewable changes (3)
- R/constants.R
- tests/testthat/test-loadpage-server-rendering.R
- R/loadpage-server-summary.R
- Change loadpage_show_sample_dataset_description - Update the three call sites blocks - Update the truth-table tests in test-loadpage-server-rendering.R
2 participants
Motivation and context
The loadpage workflow was refactored as a behavior-preserving cleanup to improve maintainability. Client-side
conditionalPanellogic (and related inline conditions) was replaced with server-side, deterministic visibility predicates that drive show/hide viashinyjs::toggle(). The large loadpage server module was split into focused helper components, including migrating converter-related/anomaly-score panels to server-siderenderUIwhere applicable. During review, a helper-count documentation mismatch (5 → 6) was corrected.Changes
Centralized namespaced control identifiers
NAMESPACE_LOADPAGEinR/constants.Rto define the UI/server “loadpage” control identifiers used for namespacing and server-driven toggling.Loadpage server split into an orchestrator + helper registrations
loadpageServer()inR/module-loadpage-server.Rto:condition_metadata) andshinyFiles-based local big-file path wiring for non-web servers.register_loadpage_preview()register_loadpage_visibility_observers()register_loadpage_converter_ui()register_loadpage_proceed_validation()register_loadpage_data_loaders()register_loadpage_summary()list(input=..., getData=..., getConditionMetadata=...).renderPrints.Server-side preview, including DIANN/PTM detection and UI wiring
register_loadpage_preview()inR/loadpage-server-preview.Rto:diann_2plus, including mismatch warnings.Server-side proceed enablement/validation
register_loadpage_proceed_validation()inR/loadpage-server-proceed-validation.Rto cascadeproceed1enable/disable based on:input$BIO,input$DDA_DIA, andinput$filetypeEvent-driven data loading and download/export
register_loadpage_data_loaders()inR/loadpage-server-data-loaders.Rto:get_data()andget_data_codederived frominput$calculate.output$download_msstats_formatto export either:data.frame, orget_summary1/get_summary2from the returned loader object.Post-proceed summary UI + editable condition metadata
register_loadpage_summary()inR/loadpage-server-summary.Rto:get_summary1/get_summary2aseventReactivecomputations driven byproceed1.condition_metadataand write edits back into areactiveVal.data$Condition(includingprotein_turnoverandchemoproteomicsbehaviors, with warning handling on chemoproteomics initialization failure).proceed2.Converter/anomaly UI moved to server-controlled show/hide +
renderUIR/loadpage-server-converter-options-panel.Rproviding:loadpage_show_*predicate functions controlling visibility for DIANN/LabelFree/OpenSWATH/quantification/annotation/PTM/TMT sections.register_loadpage_visibility_observers()that drives panel show/hide viashinyjs::toggle()(including BIO-driven toggles for PTM vs non-PTM summary containers afterproceed1).register_loadpage_converter_ui()implementingrenderUI()outputs for converter fragments (including anomaly-score-related UI viarenderUIslots) and enabling/disabling/dimming incompatible converter radio choices based on(BIO, DDA_DIA).which.proteinidhandling unified under a singlerenderUIslottmt_options_ui, with preservation/seed logic for switching between PD ↔ MaxQuant (loadpage_seed_proteinid()), tracked withreactiveVal.UI refactor: from
conditionalPanelto server-driven hidden containersloadpageUI()/ UI builders inR/module-loadpage-ui.Rto:conditionalPanel-based logic with server-mounted hidden containers (shinyjs::hidden(...)) controlled by the server.divcontainers.uiOutput(ns(NAMESPACE_LOADPAGE$tmt_options_ui)).renderUIslots (e.g., Spectronaut anomaly UI), removing reliance on static conditional-panel/JS condition strings.Tests
Added
tests/testthat/test-loadpage-server-rendering.RNAMESPACE_LOADPAGEdriver/container id assertions.loadpage_default_proteinid_for_filetype()defaults.loadpage_seed_proteinid()“typed vs default” preservation across PD ↔ MaxQuant switching (including preserved/outgoing edge cases and session reset behavior).proceed1summary-table predicate behavior for PTM vs non-PTM.Updated
tests/testthat/test-module-loadpage-ui.RrenderUIarchitecture.tmt_options_uiis provided viauiOutput(ns("tmt_options_ui"))and the previous static input is absent.renderUIslots and that old staticconditionalPanel/JS condition strings are no longer present.fileInput.Coding guidelines violated