fix(plots): Fix plotting bugs in statistical analysis page#203
Conversation
- Auto detect the DIANN dataset file type and automatically toggle the checkbox for better user experience, still retaining the ability to let user manually toggle the checkbox if required - Show a warning if the user selects an invalid state of the checkbox based on the type of file uploaded - Handle application crashing in case if the user uses an invalid configuration while uplaoding the data - Write test cases for all edge cases of the DIANN file version detection
- Remove "display protein name" checkbox - Set a default comparison name - Pre-select single comparison instead of "all" - Disable Heatmap when fewer than 2 comparisons - Remove "all" option for all cases of volcano plot
|
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:
📝 WalkthroughWalkthroughComparison selector now sources choices from contrast row names (removing ChangesGroup-comparison & visualization
Sequence Diagram(s)sequenceDiagram
participant UI as "Shiny UI"
participant Server as "Visualization Server"
participant Contrast as "Contrast Matrix"
participant Plotter as "Plot Generation"
UI->>Server: open visualization / select dataset
Server->>Contrast: read rownames(contrast$matrix)
Contrast-->>Server: comparisons list
Server->>UI: populate visualization_which_comparison (selected = first)
UI->>Server: user selects plot type
Server->>Server: evaluate n_comparisons
alt heatmap and n_comparisons >= 2
Server->>Plotter: request heatmap (which.Comparison = "all", ProteinName = TRUE)
else volcano or comparison
Server->>Plotter: request volcano/comparison (which.Comparison = selected, ProteinName = TRUE)
end
Plotter-->>UI: render or provide downloadable plot
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/statmodel-server-visualization.R (1)
105-149:⚠️ Potential issue | 🟠 Major
ProteinNameis inconsistent between on-screen plots and download for PTM/TMT.On-screen plotting passes
ProteinName = TRUEfor PTM (line 113) and TMT (line 124), but the download handler passesProteinName = FALSEfor PTM (line 290) and for the non-PTM branch — which also covers TMT — at line 301. The PR description says "Remove the display protein name checkbox" and the volcano UI now hardcodes toFALSE, so the intent appears to be uniformlyFALSE. The asymmetry means a downloaded volcano plot will look different from the one shown in-browser for the same data (labels missing in the PDF).Please align the two flows. If the intent is uniformly
FALSE, set lines 113 and 124 toFALSE. If the intent is to keep PTM/TMT labeled, set lines 290 and 301 toTRUE(or branch onloadpage_input()$BIO/DDA_DIAmatching the on-screen branches).Also applies to: 283-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-visualization.R` around lines 105 - 149, The download vs on-screen plotting code is inconsistent for the ProteinName argument causing mismatched labels: update the calls so both UI rendering and the download handler use the same ProteinName value; specifically edit the groupComparisonPlotsPTM call (plot_result in the first tryCatch branch) and the groupComparisonPlots call for the TMT branch (the branches that set ProteinName = TRUE) to match the download-handler branches (or vice versa) so that ProteinName is uniformly FALSE or TRUE across both flows (refer to function calls groupComparisonPlotsPTM and groupComparisonPlots and the variables loadpage_input$BIO / loadpage_input$DDA_DIA to decide which value to apply).
🧹 Nitpick comments (7)
tests/testthat/test-statmodel-ui-options-visualization.R (1)
42-43: Constant referenced here is now dead code.
NAMESPACE_STATMODEL$visualization_volcano_display_protein_nameis still defined inR/constants.R(around line 39) but is no longer referenced anywhere in app code after this PR — only this negative-assertion test uses it. Consider removing the constant fromR/constants.Rand changing this assertion to a string literal (e.g.,expect_false(grepl("visualization_volcano_display_protein_name", ui_html))) to avoid leaving an orphan namespace key that could mislead future contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/testthat/test-statmodel-ui-options-visualization.R` around lines 42 - 43, Remove the dead constant NAMESPACE_STATMODEL$visualization_volcano_display_protein_name from R/constants.R and update the test in tests/testthat/test-statmodel-ui-options-visualization.R to assert against the literal string used in the UI (e.g., "visualization_volcano_display_protein_name") instead of referencing the removed constant; ensure no other code references NAMESPACE_STATMODEL$visualization_volcano_display_protein_name before deleting the constant and adjust any documentation or tests that referenced it.R/statmodel-server-visualization.R (1)
16-39:updateSelectInputwill silently reset the user's plot-type selection on every comparison-mode change.This
observe()fires every timeinput[[comparison_mode]]orrownames()changes. Whencomparison_modechanges but no comparisons have been submitted yet,rownames()returnsNULL, son_comparisons == 0and choices collapse to Volcano + Comparison.updateSelectInput()is called withchoicesonly (noselected), which causes Shiny to fall back to the first choice if the previous selection is no longer in the list — silently moving the user off Heatmap they previously selected.Two suggestions:
- Preserve selection when possible by passing
selected = isolate(input[[NAMESPACE_STATMODEL$visualization_plot_type]])if it remains valid.- De-duplicate the choices vector — only the Heatmap entry differs:
♻️ Proposed simplification
- choices <- c( - "Volcano Plot" = CONSTANTS_STATMODEL$plot_type_volcano_plot, - "Comparison Plot" = CONSTANTS_STATMODEL$plot_type_comparison_plot - ) - if (n_comparisons >= 2) { - choices <- c( - "Volcano Plot" = CONSTANTS_STATMODEL$plot_type_volcano_plot, - "Heatmap" = CONSTANTS_STATMODEL$plot_type_heatmap, - "Comparison Plot" = CONSTANTS_STATMODEL$plot_type_comparison_plot - ) - } - updateSelectInput(session, NAMESPACE_STATMODEL$visualization_plot_type, - choices = choices) + choices <- c( + "Volcano Plot" = CONSTANTS_STATMODEL$plot_type_volcano_plot, + if (n_comparisons >= 2) c("Heatmap" = CONSTANTS_STATMODEL$plot_type_heatmap), + "Comparison Plot" = CONSTANTS_STATMODEL$plot_type_comparison_plot + ) + current <- isolate(input[[NAMESPACE_STATMODEL$visualization_plot_type]]) + keep <- if (!is.null(current) && current %in% choices) current else choices[[1]] + updateSelectInput(session, NAMESPACE_STATMODEL$visualization_plot_type, + choices = choices, selected = keep)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-visualization.R` around lines 16 - 39, The observe block that builds choices and calls updateSelectInput resets the user's selection when choices change; modify it to build a base choices vector once (using "Volcano Plot" and "Comparison Plot" from CONSTANTS_STATMODEL) and only insert the "Heatmap" entry when length(rownames()) >= 2, then call updateSelectInput(session, NAMESPACE_STATMODEL$visualization_plot_type, choices = choices, selected = {sel <- isolate(input[[NAMESPACE_STATMODEL$visualization_plot_type]]); if (!is.null(sel) && sel %in% choices) sel else NULL}); this preserves the current selection when still valid and avoids duplicate entries.R/utils.R (2)
35-43: Detection logic looks solid; minor note on legacy precedence semantics.The logic correctly distinguishes DIANN 2.0+ (numbered
Fr.<n>.Quantitycolumns) from legacy format. The "legacy takes precedence" rule (returningFALSEwhen both signatures appear) is intentional and tested, but worth confirming against real-world data: if a future DIANN 2.x export ever retains aFragmentQuantCorrected-style column for backwards compatibility, the auto-detection would silently treat it as 1.x. Consider adding a brief comment on the rationale (e.g., "if both are present, legacy column is the authoritative one for older converters").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils.R` around lines 35 - 43, Add a short clarifying comment inside the .is_diann_2plus function explaining why legacy fragment columns take precedence when both signatures are present (e.g., some exporters keep a legacy FragmentQuantCorrected column for compatibility), so future readers understand the intentional precedence logic around has_numbered_fragments and has_legacy_fragments checks.
8-24: Consider usingarrow::ParquetFileReaderfor more idiomatic schema extraction.The current
arrow::open_dataset(filepath, format = "parquet")$schemaapproach works for single files, butarrow::ParquetFileReader$create(filepath)$GetSchema()is the more direct and idiomatic choice.open_dataset()is designed for multi-file datasets and loads metadata without data, making it functional but less explicit thanParquetFileReaderfor this use case.For reference:
ParquetFileReader$create(filepath)$GetSchema()reads only file metadata and returns the Schema directly—making the intent clearer and the call more efficient.No changes needed for the
data.table::freadbehavior; it returns adata.table(which inherits fromdata.frame), and the Roxygen docstring is accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/utils.R` around lines 8 - 24, Replace the parquet schema extraction in .read_preview with the ParquetFileReader API: instead of using arrow::open_dataset(filepath, format = "parquet")$schema, call arrow::ParquetFileReader$create(filepath)$GetSchema() to obtain the schema (assign to schema or schema_obj) and then derive col_names and the empty_df the same way; keep the rest of .read_preview (including the fread branch and the tryCatch) unchanged.R/module-loadpage-server.R (3)
85-108: Forward reference:last_detected_diann_formatis used at Line 95 but declared at Line 111.R's lazy evaluation of observer bodies makes this work in practice (the observe body is not executed until after moduleServer's full body runs and reactivity fires), but it's fragile and confusing for readers. Consider hoisting the
reactiveVal(NULL)declaration above thisobserve()block so the dependency order is clear.♻️ Suggested reordering
+ # Track last detected DIANN format to avoid redundant notifications + last_detected_diann_format <- reactiveVal(NULL) + # Read first 100 rows for preview-based UI features. # Supported: Metamorpheus PTM (modification ID dropdown), DIANN (version auto-detection). # TODO: Extend to other input formats (Spectronaut, MaxQuant) as needed. observe({ should_preview <- (isTRUE(input$filetype == "meta") && isTRUE(input$BIO == "PTM")) || (isTRUE(input$filetype == "diann") && isTRUE(input$BIO != "PTM")) ... }) - # Track last detected DIANN format to avoid redundant notifications - last_detected_diann_format <- reactiveVal(NULL) - # Auto-toggle DIANN 2.0+ checkbox based on detected file format observe({ ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-loadpage-server.R` around lines 85 - 108, The observer uses last_detected_diann_format but that reactiveVal is declared later, which relies on lazy evaluation and is fragile; move the reactiveVal declaration for last_detected_diann_format(NULL) so it appears before the observe({ ... }) block that references it (the observe that computes should_preview, reads main_data_file(), calls .read_preview(), and sets preview_data); this ensures the reactive dependency is defined before use and makes the module's initialization order explicit.
135-150: Mismatch observer also fires on programmatic updates; verify no duplicate notifications.
observeEvent(input$diann_2plus, ...)will fire both for user toggles AND for theupdateCheckboxInput()call in the auto-toggle observer (Line 123). In the programmatic case, the newinput$diann_2plusvalue should matchdetected_2plus(no warning), so this is correct by construction — but it's a subtle invariant. A short inline comment explaining why no warning is shown in the auto-toggle path would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-loadpage-server.R` around lines 135 - 150, The observer on input$diann_2plus (observeEvent(input$diann_2plus, ...)) can be triggered by both user toggles and programmatic updates from the auto-toggle logic (updateCheckboxInput), so add a short inline comment above or inside this observer explaining that the auto-toggle path will set input$diann_2plus to match the detected_2plus (via .is_diann_2plus(preview_data())), therefore no warning will be emitted for programmatic updates and duplicate notifications are not expected; reference the auto-toggle/updateCheckboxInput location and the .is_diann_2plus and preview_data() calls so future maintainers understand the invariant.
88-108: Cross-format reset can cause stale-preview re-detection.When the user is on
meta+PTM and a new meta file is previewed, this block callslast_detected_diann_format(NULL)(Line 95) regardless of file type. If the user then switches back todiann,preview_datamay carry over from the prior meta file (or beNULL), and the auto-toggle observer'sprevious == is_2pluschange-detection is reset, potentially re-firing the "Detected DIANN ..." notification on stale data.Consider scoping the reset specifically to the DIANN branch:
♻️ Suggested scoping
if (should_preview) { file_info <- main_data_file() if (!is.null(file_info)) { - # Reset DIANN detection tracker so a new file re-triggers the notification - last_detected_diann_format(NULL) + # Reset DIANN detection tracker so a new file re-triggers the notification + if (isTRUE(input$filetype == "diann")) { + last_detected_diann_format(NULL) + } preview <- .read_preview(file_info$datapath, file_info$name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-loadpage-server.R` around lines 88 - 108, The reset of last_detected_diann_format() is unconditional and should only run when we're previewing a DIANN file; move the call to last_detected_diann_format(NULL) into the DIANN branch (the branch where should_preview is true due to isTRUE(input$filetype == "diann") && isTRUE(input$BIO != "PTM")), and only execute it when file_info is non-NULL (i.e., alongside preview <- .read_preview(...)) so switching between meta and diann doesn't reset the DIANN-detection tracker on non-DIANN previews; keep preview_data(preview) / preview_data(NULL) behavior as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/statmodel-ui-comparisons.R`:
- Around line 76-77: The default comparison name "custom comparison" can
collide; update the textInput handling for
ns(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name) so names are unique
before adding to the contrast matrix built by matrix_build(): either generate a
unique default by auto-suffixing (e.g., "custom comparison 1", "custom
comparison 2") based on existing rownames(matrix_build(...)) or validate on
submit and call showNotification with an error if the chosen name already
exists; ensure this prevents duplicate rownames that would break the
selectInput(..., comparison_names, selected = comparison_names[1]) logic used
elsewhere.
---
Outside diff comments:
In `@R/statmodel-server-visualization.R`:
- Around line 105-149: The download vs on-screen plotting code is inconsistent
for the ProteinName argument causing mismatched labels: update the calls so both
UI rendering and the download handler use the same ProteinName value;
specifically edit the groupComparisonPlotsPTM call (plot_result in the first
tryCatch branch) and the groupComparisonPlots call for the TMT branch (the
branches that set ProteinName = TRUE) to match the download-handler branches (or
vice versa) so that ProteinName is uniformly FALSE or TRUE across both flows
(refer to function calls groupComparisonPlotsPTM and groupComparisonPlots and
the variables loadpage_input$BIO / loadpage_input$DDA_DIA to decide which value
to apply).
---
Nitpick comments:
In `@R/module-loadpage-server.R`:
- Around line 85-108: The observer uses last_detected_diann_format but that
reactiveVal is declared later, which relies on lazy evaluation and is fragile;
move the reactiveVal declaration for last_detected_diann_format(NULL) so it
appears before the observe({ ... }) block that references it (the observe that
computes should_preview, reads main_data_file(), calls .read_preview(), and sets
preview_data); this ensures the reactive dependency is defined before use and
makes the module's initialization order explicit.
- Around line 135-150: The observer on input$diann_2plus
(observeEvent(input$diann_2plus, ...)) can be triggered by both user toggles and
programmatic updates from the auto-toggle logic (updateCheckboxInput), so add a
short inline comment above or inside this observer explaining that the
auto-toggle path will set input$diann_2plus to match the detected_2plus (via
.is_diann_2plus(preview_data())), therefore no warning will be emitted for
programmatic updates and duplicate notifications are not expected; reference the
auto-toggle/updateCheckboxInput location and the .is_diann_2plus and
preview_data() calls so future maintainers understand the invariant.
- Around line 88-108: The reset of last_detected_diann_format() is unconditional
and should only run when we're previewing a DIANN file; move the call to
last_detected_diann_format(NULL) into the DIANN branch (the branch where
should_preview is true due to isTRUE(input$filetype == "diann") &&
isTRUE(input$BIO != "PTM")), and only execute it when file_info is non-NULL
(i.e., alongside preview <- .read_preview(...)) so switching between meta and
diann doesn't reset the DIANN-detection tracker on non-DIANN previews; keep
preview_data(preview) / preview_data(NULL) behavior as-is.
In `@R/statmodel-server-visualization.R`:
- Around line 16-39: The observe block that builds choices and calls
updateSelectInput resets the user's selection when choices change; modify it to
build a base choices vector once (using "Volcano Plot" and "Comparison Plot"
from CONSTANTS_STATMODEL) and only insert the "Heatmap" entry when
length(rownames()) >= 2, then call updateSelectInput(session,
NAMESPACE_STATMODEL$visualization_plot_type, choices = choices, selected = {sel
<- isolate(input[[NAMESPACE_STATMODEL$visualization_plot_type]]); if
(!is.null(sel) && sel %in% choices) sel else NULL}); this preserves the current
selection when still valid and avoids duplicate entries.
In `@R/utils.R`:
- Around line 35-43: Add a short clarifying comment inside the .is_diann_2plus
function explaining why legacy fragment columns take precedence when both
signatures are present (e.g., some exporters keep a legacy
FragmentQuantCorrected column for compatibility), so future readers understand
the intentional precedence logic around has_numbered_fragments and
has_legacy_fragments checks.
- Around line 8-24: Replace the parquet schema extraction in .read_preview with
the ParquetFileReader API: instead of using arrow::open_dataset(filepath, format
= "parquet")$schema, call arrow::ParquetFileReader$create(filepath)$GetSchema()
to obtain the schema (assign to schema or schema_obj) and then derive col_names
and the empty_df the same way; keep the rest of .read_preview (including the
fread branch and the tryCatch) unchanged.
In `@tests/testthat/test-statmodel-ui-options-visualization.R`:
- Around line 42-43: Remove the dead constant
NAMESPACE_STATMODEL$visualization_volcano_display_protein_name from
R/constants.R and update the test in
tests/testthat/test-statmodel-ui-options-visualization.R to assert against the
literal string used in the UI (e.g.,
"visualization_volcano_display_protein_name") instead of referencing the removed
constant; ensure no other code references
NAMESPACE_STATMODEL$visualization_volcano_display_protein_name before deleting
the constant and adjust any documentation or tests that referenced it.
🪄 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: 43b9cbd3-27c9-478a-8030-885d19184be0
📒 Files selected for processing (8)
R/module-loadpage-server.RR/module-loadpage-ui.RR/statmodel-server-visualization.RR/statmodel-ui-comparisons.RR/statmodel-ui-options-visualization.RR/utils.Rtests/testthat/test-statmodel-ui-options-visualization.Rtests/testthat/test-utils.R
| textInput(ns(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name), | ||
| label = "Comparison Name", value = ""), | ||
| label = "Comparison Name", value = "custom comparison"), |
There was a problem hiding this comment.
Default comparison name may collide on multiple submissions.
Setting a static default of "custom comparison" means a user who clicks "Add" twice in the non-pairwise panel without editing the name will submit two comparisons sharing the same row name. Depending on how matrix_build() aggregates rows, this may overwrite the previous entry or produce a contrast matrix with duplicated rownames (which can break the downstream selectInput(..., comparison_names, selected = comparison_names[1]) logic in R/statmodel-server-visualization.R, since rownames() is used as choices/values).
Consider either:
- Auto-suffixing the default (e.g.,
"custom comparison 1","custom comparison 2") based on existing rownames, or - Validating uniqueness on submit and surfacing a
showNotificationif the name already exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/statmodel-ui-comparisons.R` around lines 76 - 77, The default comparison
name "custom comparison" can collide; update the textInput handling for
ns(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name) so names are unique
before adding to the contrast matrix built by matrix_build(): either generate a
unique default by auto-suffixing (e.g., "custom comparison 1", "custom
comparison 2") based on existing rownames(matrix_build(...)) or validate on
submit and call showNotification with an error if the chosen name already
exists; ensure this prevents duplicate rownames that would break the
selectInput(..., comparison_names, selected = comparison_names[1]) logic used
elsewhere.
There was a problem hiding this comment.
I guess this is valid, but honestly not a big deal
There was a problem hiding this comment.
@tonywu1999 Understood — it's a low-risk edge case and the current UX already requires the user to intentionally click "Add" twice without changing the name. Happy to defer this to a future cleanup if it ever becomes a practical issue.
| ui_html <- htmltools::renderTags(result)$html | ||
| expect_true(grepl(NAMESPACE_STATMODEL$visualization_which_comparison, ui_html), | ||
| info = "Which comparison namespace should be present") | ||
| expect_true(grepl(NAMESPACE_STATMODEL$visualization_volcano_display_protein_name, ui_html), |
There was a problem hiding this comment.
I think you can just get rid of this unit test. Also, can the constant be deleted too?
| textInput(ns(NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name), | ||
| label = "Comparison Name", value = ""), | ||
| label = "Comparison Name", value = "custom comparison"), |
There was a problem hiding this comment.
I guess this is valid, but honestly not a big deal
| selectInput(ns(NAMESPACE_STATMODEL$visualization_which_comparison), | ||
| label = h5("Select comparison to plot"), | ||
| c("all", rownames()), selected = "all") | ||
| comparison_names, selected = comparison_names[1]) |
There was a problem hiding this comment.
What happens with Heatmap plots here? I think Heatmap plots benefit from being able to select "all" (does a single comparison selected for a Heatmap work?).
There was a problem hiding this comment.
@tonywu1999 You're right, it doesn't work. for a single comparison
Removed the "Select comparison to plot" dropdown entirely for Heatmap since heatmaps inherently visualize all comparisons together. The dropdown still appears for Volcano Plot and Comparison Plot where selecting a single comparison makes sense. Also hardcoded which.Comparison = "all" for Heatmap in create_group_comparison_plot as a safety net in case the dropdown's last input value persists when switching plot types.
| FCcutoff = fold_change_cutoff, | ||
| logBase.pvalue = as.numeric(input[[NAMESPACE_STATMODEL$visualization_logp_base]]), | ||
| ProteinName = input[[NAMESPACE_STATMODEL$visualization_volcano_display_protein_name]], | ||
| ProteinName = FALSE, |
There was a problem hiding this comment.
When you hover over the volcano plot, can you see the protein name when hovering over a dot? This is the behavior we'd want.
There was a problem hiding this comment.
@tonywu1999 Yes, hovering over a dot in the Volcano plot shows the protein name in the tooltip.
- Update the significant protein extraction test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
R/statmodel-server-visualization.R (2)
8-14: Comparison selector resets user selection wheneverrownames()changes.
renderUIrebuilds theselectInputfrom scratch every timerownames()updates, so the user's currently-selected comparison is silently reset tocomparison_names[1]whenever the contrast matrix is edited or a new comparison row is added. Consider rendering the input once and updating choices viaupdateSelectInput, preserving the existing selection when it is still present in the newcomparison_names.♻️ Sketch
# Render the input once, then keep choices in sync without losing selection. output[[NAMESPACE_STATMODEL$visualization_which_comparison]] = renderUI({ selectInput(ns(NAMESPACE_STATMODEL$visualization_which_comparison), label = h5("Select comparison to plot"), choices = character(0)) }) observe({ comparison_names <- rownames() req(length(comparison_names) > 0) current <- isolate(input[[NAMESPACE_STATMODEL$visualization_which_comparison]]) selected <- if (!is.null(current) && current %in% comparison_names) current else comparison_names[1] updateSelectInput(session, NAMESPACE_STATMODEL$visualization_which_comparison, choices = comparison_names, selected = selected) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-visualization.R` around lines 8 - 14, The current renderUI for output[[NAMESPACE_STATMODEL$visualization_which_comparison]] rebuilds the selectInput from scratch using rownames(), causing the user's selection to reset to comparison_names[1] whenever rownames() changes; change this to render the selectInput once (with empty choices) and add an observe that watches rownames(), computes comparison_names and the preserved selected value (use isolate(input[[NAMESPACE_STATMODEL$visualization_which_comparison]]) and keep it if it exists in comparison_names, otherwise default to comparison_names[1]), then call updateSelectInput(session, NAMESPACE_STATMODEL$visualization_which_comparison, choices = comparison_names, selected = selected) to update options without losing selection.
18-39: LGTM — Heatmap correctly gated on ≥2 comparisons.This addresses the prior reviewer concern about Heatmap with a single comparison. One small nit: the
choicesvector is built twice; you can avoid the duplication by inserting Heatmap conditionally. No behavior change needed.♻️ Optional dedup
- choices <- c( - "Volcano Plot" = CONSTANTS_STATMODEL$plot_type_volcano_plot, - "Comparison Plot" = CONSTANTS_STATMODEL$plot_type_comparison_plot - ) - if (n_comparisons >= 2) { - choices <- c( - "Volcano Plot" = CONSTANTS_STATMODEL$plot_type_volcano_plot, - "Heatmap" = CONSTANTS_STATMODEL$plot_type_heatmap, - "Comparison Plot" = CONSTANTS_STATMODEL$plot_type_comparison_plot - ) - } + choices <- c("Volcano Plot" = CONSTANTS_STATMODEL$plot_type_volcano_plot) + if (n_comparisons >= 2) { + choices <- c(choices, "Heatmap" = CONSTANTS_STATMODEL$plot_type_heatmap) + } + choices <- c(choices, "Comparison Plot" = CONSTANTS_STATMODEL$plot_type_comparison_plot)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/statmodel-server-visualization.R` around lines 18 - 39, The choices vector is duplicated; instead of redefining it for n_comparisons >= 2, initialize choices with Volcano and Comparison Plot and then conditionally insert the Heatmap when n_comparisons >= 2; update the observe block that sets choices (variables: comparison_names, n_comparisons, choices) and leave the updateSelectInput(session, NAMESPACE_STATMODEL$visualization_plot_type, choices = choices) call unchanged so behavior is identical but the Heatmap option is appended only when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/module-statmodel-server.R`:
- Around line 133-147: The observer that auto-generates the default name (the
block that currently calls updateTextInput for
NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name) runs whenever
contrast$matrix changes and thus can overwrite user edits; change it to only run
on mode entry or when a genuinely new autogenerated name is required: replace
observe({...}) with observeEvent(input[[NAMESPACE_STATMODEL$comparison_mode]], {
... }) and inside the handler read contrast$matrix with isolate(contrast$matrix)
(or check the current input value via
input[[NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name]] and only call
updateTextInput when that value is empty or exactly matches a previous
autogenerated "custom comparison N"); keep the same name-generation loop but
ensure updateTextInput is conditional so it won't clobber user-typed names.
---
Nitpick comments:
In `@R/statmodel-server-visualization.R`:
- Around line 8-14: The current renderUI for
output[[NAMESPACE_STATMODEL$visualization_which_comparison]] rebuilds the
selectInput from scratch using rownames(), causing the user's selection to reset
to comparison_names[1] whenever rownames() changes; change this to render the
selectInput once (with empty choices) and add an observe that watches
rownames(), computes comparison_names and the preserved selected value (use
isolate(input[[NAMESPACE_STATMODEL$visualization_which_comparison]]) and keep it
if it exists in comparison_names, otherwise default to comparison_names[1]),
then call updateSelectInput(session,
NAMESPACE_STATMODEL$visualization_which_comparison, choices = comparison_names,
selected = selected) to update options without losing selection.
- Around line 18-39: The choices vector is duplicated; instead of redefining it
for n_comparisons >= 2, initialize choices with Volcano and Comparison Plot and
then conditionally insert the Heatmap when n_comparisons >= 2; update the
observe block that sets choices (variables: comparison_names, n_comparisons,
choices) and leave the updateSelectInput(session,
NAMESPACE_STATMODEL$visualization_plot_type, choices = choices) call unchanged
so behavior is identical but the Heatmap option is appended only when needed.
🪄 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: dd4a993c-0680-4a34-be05-75b9b31318ab
📒 Files selected for processing (3)
R/module-statmodel-server.RR/statmodel-server-visualization.Rtests/testthat/test-utils-statmodel-server.R
|
|
||
| # Auto-generate unique default name for non-pairwise comparisons | ||
| observe({ | ||
| req(input[[NAMESPACE_STATMODEL$comparison_mode]] == | ||
| CONSTANTS_STATMODEL$comparison_mode_custom_nonpairwise) | ||
| existing_names <- if (!is.null(contrast$matrix)) rownames(contrast$matrix) else character(0) | ||
| # Find the next available "custom comparison N" | ||
| n <- 1 | ||
| while (paste0("custom comparison ", n) %in% existing_names) { | ||
| n <- n + 1 | ||
| } | ||
| updateTextInput(session, | ||
| NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name, | ||
| value = paste0("custom comparison ", n)) | ||
| }) |
There was a problem hiding this comment.
Auto-default observer can clobber user-typed comparison names.
This observe re-fires not only when input[[comparison_mode]] switches to custom_nonpairwise, but also any time contrast$matrix changes (e.g., the input$table_cell_edit observer at lines 396–400, or after matrix_build mutates the matrix). Each time it fires it unconditionally calls updateTextInput(...), which will overwrite any name the user has typed into the comparison name field (for example, while they are editing a cell in the existing matrix or right after submitting a row).
Consider gating the update so it only runs on mode entry or when a new row was actually appended (e.g., scope the dependency with observeEvent(input[[comparison_mode]], ...) and read contrast$matrix inside isolate(), or only update when the current text input value already matches a previously-generated "custom comparison N").
🛠️ Suggested fix sketch
- # Auto-generate unique default name for non-pairwise comparisons
- observe({
- req(input[[NAMESPACE_STATMODEL$comparison_mode]] ==
- CONSTANTS_STATMODEL$comparison_mode_custom_nonpairwise)
- existing_names <- if (!is.null(contrast$matrix)) rownames(contrast$matrix) else character(0)
- # Find the next available "custom comparison N"
- n <- 1
- while (paste0("custom comparison ", n) %in% existing_names) {
- n <- n + 1
- }
- updateTextInput(session,
- NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name,
- value = paste0("custom comparison ", n))
- })
+ # Auto-generate unique default name for non-pairwise comparisons.
+ # Re-evaluate on mode switch or when rows are added; do not overwrite a
+ # name the user has customized away from the auto-generated pattern.
+ observeEvent(
+ list(input[[NAMESPACE_STATMODEL$comparison_mode]],
+ nrow(contrast$matrix)),
+ {
+ req(input[[NAMESPACE_STATMODEL$comparison_mode]] ==
+ CONSTANTS_STATMODEL$comparison_mode_custom_nonpairwise)
+ existing_names <- if (!is.null(contrast$matrix)) rownames(contrast$matrix) else character(0)
+ current <- isolate(input[[NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name]])
+ is_auto <- is.null(current) || !nzchar(current) ||
+ grepl("^custom comparison \\d+$", current)
+ if (!is_auto) return()
+ n <- 1
+ while (paste0("custom comparison ", n) %in% existing_names) n <- n + 1
+ updateTextInput(session,
+ NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name,
+ value = paste0("custom comparison ", n))
+ },
+ ignoreInit = FALSE
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/module-statmodel-server.R` around lines 133 - 147, The observer that
auto-generates the default name (the block that currently calls updateTextInput
for NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name) runs whenever
contrast$matrix changes and thus can overwrite user edits; change it to only run
on mode entry or when a genuinely new autogenerated name is required: replace
observe({...}) with observeEvent(input[[NAMESPACE_STATMODEL$comparison_mode]], {
... }) and inside the handler read contrast$matrix with isolate(contrast$matrix)
(or check the current input value via
input[[NAMESPACE_STATMODEL$comparisons_custom_nonpairwise_name]] and only call
updateTextInput when that value is empty or exactly matches a previous
autogenerated "custom comparison N"); keep the same name-generation loop but
ensure updateTextInput is conditional so it won't clobber user-typed names.
… heatmap to all alogn with removing the single comparison options dropdown
| #' @noRd | ||
| create_heatmap_options <- function(ns) { | ||
| tagList( | ||
| uiOutput(ns(NAMESPACE_STATMODEL$visualization_which_comparison)), |
There was a problem hiding this comment.
I think this may be unnecessary
There was a problem hiding this comment.
@tonywu1999
Got it, removing the uiOutput from create_heatmap_options. The dropdown was already guarded out in the server-side renderUI for Heatmap, so this slot was unused. Commit
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/statmodel-server-visualization.R`:
- Line 303: The heatmap download code is directly reading
input[[NAMESPACE_STATMODEL$visualization_which_comparison]] (symbol:
which.Comparison) and thus bypasses the "all" override logic implemented in
create_group_comparison_plot; update the heatmap download parameter to use the
same resolved comparison value that create_group_comparison_plot uses (e.g.,
reuse the reactive/resolution function or variable that applies the "all"
override) so which.Comparison can be "all" when appropriate instead of always
the raw single-selection input.
🪄 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: d67466f2-61e9-4ff9-ae1d-283b97ca6e98
📒 Files selected for processing (2)
R/statmodel-server-visualization.RR/statmodel-ui-options-visualization.R
tonywu1999
changed the title
2 participants
Statmodel Inference:
Motivation and Context
DIANN dataset integration exposed UX and consistency issues in the Statmodel Inference visualization flow. The goal is to simplify the visualization UI, make sensible defaults, and remove a redundant per-plot protein-name toggle so behavior is consistent across interactive plots and downloads. The PR also ensures heatmap availability is conditioned on having multiple comparisons and auto-generates unique names for custom non-pairwise comparisons.
Short solution summary
"all"choice from comparison selection; populate comparison choices from contrast row names and default to the first available comparison.Detailed changes
R/statmodel-server-visualization.R
"all"option; default visualization_which_comparison to the first available choice."all"selection that previously prevented multiple-screen volcano plots."all"; non-heatmap modes use the selected visualization_which_comparison."all"for heatmap downloads and selected comparison otherwise.R/statmodel-ui-options-visualization.R
R/module-statmodel-server.R
R/constants.R
Plot generation functions (create_group_comparison_plot and callers)
Unit tests added or modified
tests/testthat/test-statmodel-ui-options-visualization.R
tests/testthat/test-utils-statmodel-server.R