Skip to content

refactor(statmodel): Put conditional logic in backend for comparison matrices#129

Merged
tonywu1999 merged 1 commit intodevelfrom
refactor-stat-model
Nov 20, 2025
Merged

refactor(statmodel): Put conditional logic in backend for comparison matrices#129
tonywu1999 merged 1 commit intodevelfrom
refactor-stat-model

Conversation

@tonywu1999
Copy link
Copy Markdown
Contributor

@tonywu1999 tonywu1999 commented Nov 20, 2025

Summary by CodeRabbit

  • New Features

    • Contrast panel UI now dynamically updates based on the selected contrast mode, improving interface responsiveness and reducing unnecessary UI elements.
  • Tests

    • Added comprehensive test coverage for new dynamic contrast panel functionality and contrast comparison builders.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 20, 2025

Walkthrough

Refactors contrast panel UI rendering from static conditional panels to dynamic rendering via a new get_contrast_panel_ui() dispatcher function that maps contrast modes to panel builders, with renamed internal panel functions and updated tests to reflect the new architecture.

Changes

Cohort / File(s) Summary
Server-side dynamic rendering
R/module-statmodel-server.R
Introduces get_contrast_panel_ui(mode, ns) function mapping contrast mode strings to panel builders; adds rendering of dynamic_contrast_panel UI output in server flow based on selected contrast mode.
UI panel builders refactoring
R/statmodel-ui-options-contrasts.R
Replaces static conditionalPanel wrappers with single uiOutput() placeholder; renames four panel builder functions (create_*build_*); restructures panels from conditionalPanel to tagList containers with explicit UI elements (headers, buttons, selectors).
Test additions
tests/testthat/test-statmodel-ui-options-contrasts.R, tests/testthat/test-utils-statmodel-server.R
Adds comprehensive tests for new build_*_panel() functions validating ID namespacing, HTML structure, and element presence; adds unit tests for get_contrast_panel_ui() mode mapping and null-handling.
Test removals
tests/testthat/test-module-statmodel-ui.R
Removes tests for conditional panels, per-type action buttons, and UIOutput placeholders; narrows namespace checks to exclude per-type submit/clear field variants.

Sequence Diagram

sequenceDiagram
    autonumber
    actor User
    participant Shiny as Shiny Server
    participant Handler as get_contrast_panel_ui()
    participant Builder as Panel Builder<br/>(build_*_panel)
    participant UI as Rendered Panel

    User->>Shiny: Select contrast_mode<br/>(custom/all_one/all_pair/custom_np)
    activate Shiny
    Shiny->>Handler: Call get_contrast_panel_ui(mode, ns)
    activate Handler
    Handler->>Handler: Map mode to builder function
    Handler->>Builder: Call matched builder(ns)
    activate Builder
    Builder->>Builder: Construct tagList with<br/>UI elements (headers,<br/>buttons, inputs)
    Builder-->>Handler: Return tagList UI
    deactivate Builder
    Handler-->>Shiny: Return panel UI
    deactivate Handler
    Shiny->>UI: Render to dynamic_contrast_panel<br/>output
    deactivate Shiny
    UI-->>User: Display appropriate<br/>contrast panel
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Function renaming across files: create_*_panelbuild_*_panel requires verification that all references updated consistently
  • UI restructuring logic: Conversion from conditionalPanel to dynamic uiOutput() with dispatcher function requires understanding control flow changes
  • Test coverage shifts: Removed tests for conditional panels and action buttons require justification; new tests validate different aspects of the same functionality
  • Namespace consistency: Multiple panel builders with consistent ID patterns (e.g., statmodel-submit, statmodel-submit1, statmodel-submit2) requires careful verification of ID correctness across builders

Possibly related PRs

Suggested labels

Review effort 3/5

Poem

🐰 Dynamic panels dance with glee,
No more static checks—just tagList spree!
Modes map to builders, clean and tight,
Contrast rendering done just right!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main refactoring goal: moving conditional comparison matrix logic from the frontend UI layer to the backend server logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor-stat-model

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9ac173 and adb2dc3.

📒 Files selected for processing (5)
  • R/module-statmodel-server.R (2 hunks)
  • R/statmodel-ui-options-contrasts.R (4 hunks)
  • tests/testthat/test-module-statmodel-ui.R (1 hunks)
  • tests/testthat/test-statmodel-ui-options-contrasts.R (1 hunks)
  • tests/testthat/test-utils-statmodel-server.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 (7)
tests/testthat/test-utils-statmodel-server.R (1)

332-359: LGTM! Comprehensive test coverage for the dispatcher function.

The new test validates get_contrast_panel_ui() across all supported modes and edge cases (NULL, empty, invalid). The HTML structure validation confirms correct panel rendering for each contrast mode.

R/module-statmodel-server.R (2)

20-33: LGTM! Clean dispatcher function with appropriate error handling.

The function correctly maps contrast modes to their respective panel builders and handles edge cases (NULL, empty, invalid) by returning NULL. The switch statement mapping aligns with the modes defined in the UI radio buttons.


459-461: LGTM! Dynamic rendering correctly wired.

The dynamic contrast panel is properly rendered using the dispatcher function, and input$contrast_mode will trigger reactive re-rendering when the user changes the selection.

tests/testthat/test-module-statmodel-ui.R (1)

268-277: LGTM! Appropriate test adjustment for dynamic rendering.

Removing per-type button IDs from the namespace test is correct since those elements are now generated dynamically within panel builders rather than as static module-level inputs. The test now focuses on module-level controls that are always present.

tests/testthat/test-statmodel-ui-options-contrasts.R (1)

1-55: LGTM! Excellent test coverage for panel builders.

This new test file comprehensively validates all four panel builder functions (build_custom_pairwise_panel, build_all_vs_one_panel, build_all_pairwise_panel, build_custom_nonpairwise_panel). Each test verifies:

  • Correct HTML structure and element IDs
  • Proper namespace application
  • Expected shiny.tag.list structure
  • Action button labels

The tests provide good regression protection for the UI refactoring.

R/statmodel-ui-options-contrasts.R (2)

12-12: LGTM! Core architectural improvement.

Replacing static conditional panels with a single dynamic UI output (dynamic_contrast_panel) enables server-side control of panel rendering, improving maintainability and allowing more flexible UI updates based on application state.


40-80: LGTM! Consistent function renaming and structure.

The rename from create_* to build_* is semantically appropriate and consistent across all four panel builders. The tagList-based structure in each builder correctly encapsulates the UI elements for dynamic rendering.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant