refactor(statsmodel): Refactor statsmodel UI components to be more readable#117
refactor(statsmodel): Refactor statsmodel UI components to be more readable#117tonywu1999 merged 6 commits intodevelfrom
Conversation
WalkthroughRefactors a monolithic Shiny UI into modular, namespaced builder functions, adds UI helper modules for headers/styles, splits contrast/modeling/visualization/results into separate builders, adds comprehensive UI tests, and bumps Changes
Sequence Diagram(s)sequenceDiagram
participant App as statmodelUI
participant Headers as create_custom_styles / create_header_section
participant Contrasts as create_contrast_section
participant Modeling as create_modeling_section
participant Viz as create_visualization_section
participant Results as create_results_section
participant User as User (browser)
rect rgb(240,248,255)
App ->> Headers: call to assemble header/styles
App ->> Contrasts: call to build contrast UI (namespaced)
App ->> Modeling: call to build modeling UI (namespaced)
App ->> Viz: call to build visualization UI (namespaced)
App ->> Results: call to build results UI (namespaced)
end
Note right of App: UI composed of modular sections\nreturned as tagLists / UI elements
User ->> Contrasts: select contrast radio -> conditional panel toggles
User ->> Modeling: toggle moderation / adjust significance slider
User ->> Viz: choose plot type -> conditional plot options appear
User ->> Results: view results / tabs render based on PTM flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
force-pushed
the
feat-dose-response
branch
from
0003b48 to
6573877
Compare
|
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: 2
🧹 Nitpick comments (7)
R/statmodel-ui-headers.R (1)
7-22: Consider centralizing button IDs or using a data-driven approach.The hardcoded button IDs in the style tags create coupling between this function and button definitions elsewhere. If button IDs change in the future, these styles will silently fail to apply.
Consider one of these approaches:
- Define button IDs as constants in a shared location
- Use CSS classes instead of IDs for styling
- Generate styles dynamically based on a list of button IDs
Example refactor using a constant list:
+# Define button IDs as constants +ORANGE_BUTTON_IDS <- c("submit3", "clear3", "submit1", "clear1", + "submit2", "clear2", "calculate", "plotresults", + "viewresults", "submit", "clear") + create_custom_styles <- function() { + button_styles <- lapply(ORANGE_BUTTON_IDS, function(id) { + tags$style(HTML(sprintf('#statmodel-%s{background-color:orange}', id))) + }) tags$head( - tags$style(HTML('#statmodel-submit3{background-color:orange}')), - tags$style(HTML('#statmodel-clear3{background-color:orange}')), - ... + button_styles, tags$link(rel = "stylesheet", type = "text/css", href = "assets/style.css") ) }R/statmodel-ui-options-visualization.R (3)
31-31: Remove commented-out code or track as an issue.The commented-out "Response Curve" option suggests incomplete work. Either implement it now, remove the comment, or open an issue to track future work.
Do you want me to open a new issue to track the Response Curve feature implementation?
40-46: Hardcoded namespace breaks module isolation.The conditionalPanel on lines 40-41 and 44 hardcodes the
'statmodel'namespace string. This creates tight coupling and prevents the module from being instantiated with different IDs. While this matches the current usage, it violates Shiny module best practices.Shiny's conditionalPanel doesn't support dynamic namespacing easily, but you can document the limitation or consider using
renderUIwithconditionalPanelinside for better namespace handling. Alternatively, document that these functions are tightly coupled to the "statmodel" namespace.Add a comment at the top of the file:
# Note: conditionalPanel conditions use hardcoded 'statmodel' namespace. # These functions are tightly coupled to statmodelUI("statmodel") usage.
121-128: JavaScript boolean comparison uses string instead of boolean.Line 122 compares against the string
"true"instead of the booleantrue. While this may work due to JavaScript's type coercion, it's better to use the correct type for clarity and to avoid potential issues.Consider updating to:
conditionalPanel( - condition = "input['statmodel-FC1'] == true", + condition = "input['statmodel-FC1']", numericInput(Or if you need explicit comparison:
- condition = "input['statmodel-FC1'] == true", + condition = "input['statmodel-FC1'] === true",R/statmodel-ui-results.R (1)
18-21: Address or remove TODO comments.Lines 18, 19, and 21 contain comments suggesting renames and feature additions. These indicate incomplete refactoring work.
Please either:
- Complete the suggested refactoring now
- Remove the comments if the current naming is final
- Create issues to track these future improvements
Do you want me to help generate updated code for these suggestions or open issues to track them?
R/statmodel-ui-options-modeling.R (1)
16-16: Remove or implement TODO comment.The comment suggests adding dose response options but the feature isn't implemented. Either implement it now or track it separately.
Do you want me to open an issue to track the dose response curve feature?
R/statmodel-ui-options-contrasts.R (1)
16-16: Remove or implement TODO comment.The comment about dose response curve mapping is one of several similar comments across the UI files. Consider consolidating these feature requests into a single tracked issue.
There are multiple dose response comments across files (Lines 16 in this file, line 16 in statmodel-ui-options-modeling.R, line 31 in statmodel-ui-options-visualization.R). Do you want me to help create a consolidated issue to track this feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
DESCRIPTION(1 hunks)R/module-statmodel-ui.R(1 hunks)R/statmodel-ui-headers.R(1 hunks)R/statmodel-ui-options-contrasts.R(1 hunks)R/statmodel-ui-options-modeling.R(1 hunks)R/statmodel-ui-options-visualization.R(1 hunks)R/statmodel-ui-results.R(1 hunks)tests/testthat/test-module-statmodel-ui.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 (3)
R/module-statmodel-ui.R (1)
14-35: Excellent refactoring to modular UI builders!The refactored code is much more readable and maintainable. Breaking the monolithic UI into focused builder functions (
create_contrast_section,create_modeling_section,create_visualization_section,create_results_section) makes the code easier to understand and test.The modular approach provides clear separation of concerns and makes each UI section independently testable.
R/statmodel-ui-options-contrasts.R (1)
5-19: Well-structured contrast section builder.The modular breakdown into radio buttons and conditional panels for each comparison type is clean and maintainable.
tests/testthat/test-module-statmodel-ui.R (1)
1-349: Comprehensive test coverage with excellent organization!The test suite provides thorough coverage of the UI structure, including:
- Component ordering and presence
- Conditional panel logic
- Button states and IDs
- Input controls and options
- Namespace consistency
- Validation messages
- Tabset structure
The organization into logical sections (1-11) makes the tests easy to navigate and maintain.
Note on test brittleness: The tests rely on HTML structure and specific encoding (e.g.,
'for single quotes). While comprehensive, these tests may require updates if:
- The
htmltoolspackage changes its HTML rendering- Shiny changes its HTML structure
- HTML escaping behavior changes
Consider adding some higher-level functional tests using
shinytest2to complement these structural tests, which would be more robust to implementation changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
R/statmodel-ui-options-visualization.R (4)
70-76: Fix hardcoded namespace in conditionalPanel.Line 72 has the same hardcoded namespace issue as flagged in the volcano plot options.
80-111: Fix hardcoded namespace and spelling errors.Line 82 has the same hardcoded namespace issue flagged earlier.
Additionally, lines 105-107 contain spelling errors in user-facing text.
Apply this diff to fix the typo:
c( - "protein dendogram" = "protein", - "comparison dendogram" = "comparison", - "protein and comparison dendograms" = "both" + "protein dendrogram" = "protein", + "comparison dendrogram" = "comparison", + "protein and comparison dendrograms" = "both" )
115-130: Fix hardcoded namespace in conditionalPanel.Line 122 has the same hardcoded namespace issue flagged earlier.
134-146: Fix hardcoded namespace in conditionalPanel.Line 138 has the same hardcoded namespace issue (
"loadpage") flagged earlier. This cross-module dependency should be handled more robustly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
R/module-loadpage-ui.R(1 hunks)R/statmodel-ui-options-modeling.R(1 hunks)R/statmodel-ui-options-visualization.R(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- R/module-loadpage-ui.R
🚧 Files skipped from review as they are similar to previous changes (1)
- R/statmodel-ui-options-modeling.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 (2)
R/statmodel-ui-options-visualization.R (2)
8-18: LGTM!The composition is clean and follows a modular pattern for building the visualization section.
22-35: LGTM!The plot type selector is well-structured, and the comment on line 31 clearly documents the planned dose response feature.
| create_volcano_plot_options <- function(ns) { | ||
| conditionalPanel( | ||
| condition = "input['statmodel-typeplot'] == 'VolcanoPlot'", | ||
| uiOutput(ns("WhichComp")), | ||
| conditionalPanel( | ||
| condition = "input['loadpage-DDA_DIA']!=='TMT'", | ||
| checkboxInput(ns("pname"), label = p("display protein name")) | ||
| ), | ||
| selectInput( | ||
| ns("logp"), | ||
| label = h5("Log transformation of adjusted p-value"), | ||
| c("base 2" = "2", "base 10" = "10"), | ||
| selected = "10" | ||
| ), | ||
| sliderInput( | ||
| ns("sig"), | ||
| label = h5( | ||
| "Adjusted p-value cutoff", | ||
| class = "icon-wrapper", | ||
| icon("question-circle", lib = "font-awesome"), | ||
| div("The cutoff used to determine significant results.", class = "icon-tooltip") | ||
| ), | ||
| 0, 1, 0.05 | ||
| ), | ||
| create_fold_change_options(ns), | ||
| tags$br() | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix hardcoded namespace in conditionalPanel conditions.
Lines 41 and 44 use hardcoded namespace strings ("statmodel" and "loadpage") in conditionalPanel conditions, which breaks modularity and will cause runtime failures if this module is ever instantiated with a different namespace. This is inconsistent with the parameterized ns() approach used for input IDs.
The proper pattern for dynamic namespace evaluation in conditionalPanel requires either:
- Using JavaScript-based namespace construction
- Passing the namespace as a data attribute and referencing it in the condition
Since Shiny's conditionalPanel uses JavaScript for the condition, you'll need to ensure the namespace matches. Consider one of these approaches:
Option 1: Use ns() wrapper in the condition (requires NS to be available in client-side JavaScript):
conditionalPanel(
- condition = "input['statmodel-typeplot'] == 'VolcanoPlot'",
+ condition = sprintf("input['%s'] == 'VolcanoPlot'", ns("typeplot")),Option 2: Document the namespace dependency and verify it matches the module's actual namespace ("statmodel"). If this is always used within the same fixed module, add a comment explaining the constraint.
Note: Line 59's tooltip text appears correctly fixed from the previous review.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
R/statmodel-ui-options-visualization.R around lines 39 to 66: the
conditionalPanel conditions on lines 41 and 44 use hardcoded IDs
("statmodel-typeplot" and "loadpage-DDA_DIA") which breaks module namespacing;
replace those hardcoded strings by constructing the JS condition with the module
namespace at render time (e.g. build the condition string using ns(...) and
sprintf/paste0 so the resulting condition references input['<namespaced-id>']
rather than a fixed prefix), updating both conditionalPanel calls accordingly
(or, if the module truly requires a fixed parent namespace, add a clear comment
documenting that constraint).
1 participant
Why
Statmodule UI needs to be refactored so that new developers can easily add the new dose response curve feature.
Changes
Testing