refactor(statmodel): Refactor namespaces for modeling and visualization options#138
refactor(statmodel): Refactor namespaces for modeling and visualization options#138tonywu1999 merged 15 commits intodevelfrom
Conversation
WalkthroughReplaces hard-coded statmodel UI/input IDs with centralized namespaced constants across constants, server logic, UI builders, and tests; adds new modeling and visualization namespace keys and plot-type constants, and updates server/reactive wiring and UI option builders to use those constants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
tonywu1999
changed the title
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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-ui-options-visualization.R (1)
92-96: Typo: "dendogram" should be "dendrogram".The correct spelling is "dendrogram" (with an 'r'). This typo appears in three option labels for the cluster analysis select input.
Apply this diff:
c( - "protein dendogram" = "protein", - "comparison dendogram" = "comparison", - "protein and comparison dendograms" = "both" + "protein dendrogram" = "protein", + "comparison dendrogram" = "comparison", + "protein and comparison dendrograms" = "both" )
🧹 Nitpick comments (1)
R/module-statmodel-server.R (1)
512-516: Verify the tryCatch without error handling.The
Rownamesreactive usestryCatchbut only provides an empty error handler. This silently swallows errors, which could make debugging difficult.Consider logging the error or returning a more informative default:
Rownames = eventReactive(input[[NAMESPACE_STATMODEL$comparisons_submit]], { req(input[[NAMESPACE_STATMODEL$comparison_mode]]) req(loadpage_input()$DDA_DIA) - tryCatch({ rownames(matrix_build()) }, error = function(e) {}) + tryCatch({ + rownames(matrix_build()) + }, error = function(e) { + message("Error getting rownames: ", conditionMessage(e)) + character(0) + }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
R/constants.R(1 hunks)R/module-statmodel-server.R(9 hunks)R/statmodel-ui-headers.R(1 hunks)R/statmodel-ui-options-modeling.R(1 hunks)R/statmodel-ui-options-visualization.R(3 hunks)tests/testthat/test-module-statmodel-ui.R(4 hunks)tests/testthat/test-statmodel-ui-options-visualization.R(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
R/statmodel-ui-headers.R (1)
19-33: LGTM! Consistent namespace-based CSS selector construction.The new styling blocks follow the same pattern as the existing comparison button styles, ensuring uniform application of the orange background color across all action buttons.
tests/testthat/test-statmodel-ui-options-visualization.R (1)
1-62: Well-structured test coverage for namespace-based UI elements.The tests effectively validate the presence of namespaced identifiers in the generated UI HTML. Good use of descriptive test names and informative error messages.
R/statmodel-ui-options-modeling.R (2)
12-18: LGTM! Good architectural improvement.Moving the moderation UI to a conditional
uiOutputallows for dynamic rendering based on experiment type (TMT), which is more flexible than the previous inline approach.
21-32: The radio button value binding is correct. Thec(True = TRUE, False = FALSE)pattern is intentional and follows the same convention as other boolean radio buttons in the codebase (e.g.,c(Yes=TRUE, No=FALSE)inremove_unlocalized_peptides). The display labels ("True"/"False") and logical values (TRUE/FALSE) correctly bind to themoderatedparameter inMSstatsTMT::groupComparisonTMT()with no transformation needed.However, there is a minor UI inconsistency: the tooltip text uses all-caps "TRUE"/"FALSE" while the radio button labels use capitalized "True"/"False". Consider updating the tooltip to match the button labels for consistency: change "TRUE will moderate t statistic; FALSE (default)" to "True will moderate t statistic; False (default)".
tests/testthat/test-module-statmodel-ui.R (2)
68-89: LGTM! Thorough namespace migration in tests.The button presence and disabled state checks have been properly updated to use the new namespace constants, maintaining test coverage while aligning with the refactored codebase.
104-122: LGTM! Clean test simplification.The input controls test now focuses on the essential namespace-based checks, removing redundant assertions while maintaining coverage of key UI elements.
R/statmodel-ui-options-visualization.R (3)
5-16: LGTM! Improved separation of concerns.The move to a conditional
uiOutputfor plot options enables dynamic rendering based on plot type selection, which is cleaner than the previous approach.
19-31: LGTM! Consistent use of plot type constants.The plot type selector properly uses both namespace constants for the input ID and value constants for the plot type identifiers.
34-59: LGTM! Well-refactored volcano plot options.The function signature now properly accepts the
nsparameter and consistently uses namespace constants for all input IDs. The conditional protein name display based onshow_protein_nameparameter is a good design.R/constants.R (2)
2-28: LGTM! Well-organized namespace constants.The NAMESPACE_STATMODEL constants are comprehensive and well-documented with inline comments showing the old hard-coded IDs. The hierarchical naming convention (e.g.,
visualization_plot_type,visualization_logp_base) makes the purpose of each constant clear.
30-39: LGTM! Clean plot type constants.The CONSTANTS_STATMODEL entries properly define the plot type values used throughout the application, with helpful comments indicating their usage context.
R/module-statmodel-server.R (5)
214-221: LGTM! Clean conditional UI helper.The
get_tmt_moderation_radio_buttonfunction properly encapsulates the logic for conditionally rendering moderation options only for TMT experiments.
228-265: LGTM! Comprehensive namespace migration in plot inputs.The function has been successfully updated to:
- Accept additional parameters (
input,loadpage_input) for conditional rendering- Use namespace constants for all UI element IDs
- Implement dynamic plot options based on plot type selection
- Conditionally render fold change input
This is a well-structured refactoring that improves maintainability.
268-329: LGTM! Thorough namespace constant adoption in plot creation.All input references have been systematically migrated to use namespace constants, ensuring consistency with the centralized ID management approach. The fold change cutoff handling is also properly updated.
536-560: LGTM! Comprehensive namespace migration in matrix building.The matrix building logic has been thoroughly updated to use namespace constants for:
- Comparison mode checks
- Contrast building function calls
- UI element enabling
The refactoring maintains the original logic while improving maintainability through centralized constant management.
612-645: LGTM! Namespace constants properly used in dynamic UI.The conditional panel conditions correctly interpolate namespace constants into the JavaScript condition strings, ensuring the UI visibility logic aligns with the namespaced input IDs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
R/statmodel-ui-options-visualization.R (2)
34-59: Volcano options helper is consistent; confirm shared logp/fold-change behavior + consider tiny reuse refactorThe refactored
create_volcano_plot_options(ns, show_protein_name)cleanly:
- Namespaces all inputs/outputs via
NAMESPACE_STATMODEL.- Allows the protein-name checkbox to be toggled via
show_protein_name.- Reuses
visualization_logp_base,visualization_volcano_significance_cutoff, andcreate_fold_change_options(ns).Two small follow-ups:
- This now shares the
visualization_logp_baseand fold-change controls with other plot types (e.g., heatmap). That means the chosen base/cutoff can persist when switching plot types; please confirm this cross-plot state sharing is intentional from a UX perspective.- Since the logp-base selector is duplicated between volcano and heatmap, you could optionally extract a tiny helper (e.g.,
create_logp_base_input(ns)) to avoid drift if the label or choices ever change.
69-99: Heatmap options wiring looks correct; option values should stay in sync with serverHeatmap options are cleanly namespaced and reuse shared controls (logp base + fold-change) alongside a bounded
numericInputfor protein count and a cluster-modeselectInput. Implementation looks correct.If the server branches on
"protein","comparison", and"both"for clustering, ensure those string literals are used consistently there; if they get reused in multiple places, consider moving them into constants to reduce coupling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/statmodel-ui-options-visualization.R(2 hunks)tests/testthat/test-statmodel-ui-options-visualization.R(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/testthat/test-statmodel-ui-options-visualization.R
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
R/statmodel-ui-options-visualization.R (5)
8-14: Namespaced plot-options container wiring looks goodUsing
uiOutput(ns(NAMESPACE_STATMODEL$visualization_plot_options_conditional_panel))gives the server a single, namespaced anchor to swap volcano/heatmap/comparison option panels; layout and API usage look correct.
20-28: Selector correctly uses centralized plot-type constantsThe plot-type
selectInputnow uses a namespaced ID andCONSTANTS_STATMODEL$plot_type_*values, which should keep UI, server logic, and tests aligned around a single source of truth for plot types. No issues spotted with ordering or defaults.
62-65: Comparison plot options simplified to a single dynamic placeholderReturning only
uiOutput(ns(NAMESPACE_STATMODEL$visualization_comparison_plot_which_protein))keeps the comparison-plot UI flexible and server-driven. The namespacing is consistent with the rest of the refactor.
103-109: Centralized fold-change options helper is consistent
create_fold_change_options(ns)now provides a namespaced checkbox plus auiOutputplaceholder for the actual cutoff input, which matches the dynamic rendering approach described in the server changes. The shared helper should reduce duplication across plot types.
113-118: Action buttons correctly migrated to namespaced IDs
visualization_view_resultsandvisualization_download_plot_resultsare now passed throughns()and pulled fromNAMESPACE_STATMODEL, which should keep the header, server, and UI in sync. Button semantics remain unchanged.
1 participant
Summary by CodeRabbit
New Features
UI Improvements
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.