feat(advanced-options): Set up advanced options panel for subnetwork search#178
feat(advanced-options): Set up advanced options panel for subnetwork search#178tonywu1999 wants to merge 7 commits intodevelfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds two imports to NAMESPACE ( Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Advanced Options Panel
participant Server as visualizeNetworkServer
participant Input as getInputParameters
participant Extract as extractSubnetwork
participant INDRA as getSubnetworkFromIndra
User->>UI: Click actionLink (toggle)
UI->>UI: (shinyjs::toggle) show/hide advanced options
User->>UI: Set filter_by_ptm_site / include_infinite_fc / direction
Server->>Input: getInputParameters() reads inputs
Input-->>Server: return parameters (including new filters)
Server->>Extract: extractSubnetwork(..., filter_by_ptm_site, include_infinite_fc, direction)
Extract->>INDRA: getSubnetworkFromIndra(..., filter_by_ptm_site, include_infinite_fc, direction)
INDRA-->>Extract: filtered subnetwork
Extract-->>Server: network result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/module-visualize-network-server.R (1)
214-229:⚠️ Potential issue | 🔴 CriticalThe MSstatsBioNet API does not document support for the three new parameters.
According to the official MSstatsBioNet package documentation,
getSubnetworkFromIndra()does not includefilter_by_ptm_site,include_infinite_fc, ordirectionparameters. Passing these undocumented arguments will cause a runtime error ("unused argument") unless your project depends on an unreleased or modified version of MSstatsBioNet. Confirm that the MSstatsBioNet version constraint in your DESCRIPTION file or package dependencies explicitly requires a version that supports these parameters, or remove them from the function call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-visualize-network-server.R` around lines 214 - 229, The call to getSubnetworkFromIndra inside extractSubnetwork passes three parameters that MSstatsBioNet does not document (filter_by_ptm_site, include_infinite_fc, direction); remove these three arguments from the getSubnetworkFromIndra(...) invocation in extractSubnetwork or alternatively update the package dependency/version in DESCRIPTION to a specific MSstatsBioNet release that explicitly supports those parameters—search for the extractSubnetwork function and the getSubnetworkFromIndra call to apply the change and keep the remaining named arguments unchanged.
🧹 Nitpick comments (1)
R/module-visualize-network-ui.R (1)
260-260: Remove redundantuseShinyjs()call.
useShinyjs()is already called in the mainnetworkUIfunction at line 468. Calling it again insidecreateAdvancedOptionsCollapsibleis redundant and can cause initialization issues or warnings. It should only be included once in the UI hierarchy.♻️ Proposed fix
createAdvancedOptionsCollapsible <- function(ns) { div( style = "margin-bottom: 15px;", tagList( - useShinyjs(), actionLink( ns("toggle_adv"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-visualize-network-ui.R` at line 260, Remove the redundant useShinyjs() call inside createAdvancedOptionsCollapsible: locate the function createAdvancedOptionsCollapsible and delete the useShinyjs() invocation so that useShinyjs() is only called once in the top-level networkUI function; ensure no other duplicate initializations remain in helper UI functions.
🤖 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-visualize-network-server.R`:
- Around line 563-565: The generated code writes params$direction without quotes
causing R to treat it as a symbol; update the string concatenation that builds
codes (the line referencing params$direction) to wrap the value in quotes (e.g.,
emit "direction = \"<value>\"" or use shQuote(params$direction)) so the produced
R code contains a quoted string ("both"/"up"/"down"); keep the other
concatenations unchanged.
- Around line 395-397: The toggle call inside observeEvent(input$toggle_adv)
uses the un-namespaced id "adv_panel" and must use the module namespace: change
the toggle invocation in the observeEvent (where input$toggle_adv is handled) to
call toggle(id = session$ns("adv_panel"), anim = TRUE) so it targets the
ns("adv_panel") element created in the UI within this module.
- Around line 214-216: Update the test calls to match the new extractSubnetwork
signature: in the tests that call extractSubnetwork (from
tests/testthat/test_network_visualization.R around the earlier failing calls),
pass the three new arguments filter_by_ptm_site, include_infinite_fc, and
direction (e.g., FALSE, FALSE, "both") as the 9th–11th parameters so the call
becomes extractSubnetwork(annotated_df, pValue, evidence, statementTypes,
sources, absLogFC, selectedProteins, filterByCuration, FALSE, FALSE, "both");
ensure all test invocations of extractSubnetwork use these three additional
arguments.
---
Outside diff comments:
In `@R/module-visualize-network-server.R`:
- Around line 214-229: The call to getSubnetworkFromIndra inside
extractSubnetwork passes three parameters that MSstatsBioNet does not document
(filter_by_ptm_site, include_infinite_fc, direction); remove these three
arguments from the getSubnetworkFromIndra(...) invocation in extractSubnetwork
or alternatively update the package dependency/version in DESCRIPTION to a
specific MSstatsBioNet release that explicitly supports those parameters—search
for the extractSubnetwork function and the getSubnetworkFromIndra call to apply
the change and keep the remaining named arguments unchanged.
---
Nitpick comments:
In `@R/module-visualize-network-ui.R`:
- Line 260: Remove the redundant useShinyjs() call inside
createAdvancedOptionsCollapsible: locate the function
createAdvancedOptionsCollapsible and delete the useShinyjs() invocation so that
useShinyjs() is only called once in the top-level networkUI function; ensure no
other duplicate initializations remain in helper UI functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 813e9dc4-43e2-4e66-8f6f-bbd9d2b1157c
📒 Files selected for processing (4)
NAMESPACER/MSstatsShiny.RR/module-visualize-network-server.RR/module-visualize-network-ui.R
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
R/module-visualize-network-server.R (1)
184-187: Consider adding defensive NULL handling for new parameters.The existing
filterByCurationparameter has explicit NULL handling (Lines 169-173), but the new parameters are read directly from input without defaults. If the UI inputs haven't been initialized or are conditionally hidden, these could be NULL.♻️ Suggested defensive handling
filterByCuration = filterByCuration, - filter_by_ptm_site = input$filter_by_ptm_site, - include_infinite_fc = input$include_infinite_fc, - direction = input$direction + filter_by_ptm_site = if (is.null(input$filter_by_ptm_site)) FALSE else input$filter_by_ptm_site, + include_infinite_fc = if (is.null(input$include_infinite_fc)) FALSE else input$include_infinite_fc, + direction = if (is.null(input$direction)) "both" else input$direction🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-visualize-network-server.R` around lines 184 - 187, The new parameters (filter_by_ptm_site, include_infinite_fc, direction) should get the same defensive NULL handling as filterByCuration: when building the list passed into the module, replace direct input reads with guarded expressions (e.g., for filter_by_ptm_site and include_infinite_fc coerce NULL to FALSE, and for direction coerce NULL to a sensible default such as "both" or the module's expected default) so the code using filterByCuration, filter_by_ptm_site, include_infinite_fc, and direction never receives NULL; implement these guards using if (is.null(input$...)) <default> else input$... or a coalescing helper before passing them through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/testthat/test_network_visualization.R`:
- Around line 182-186: The error test still calls extractSubnetwork with the old
8-argument signature (see the failing call using create_mock_annotated_data()) —
update that test to call extractSubnetwork with the new 11 parameters (add the
three new params: filterByCuration, filter_by_ptm_site, include_infinite_fc) in
the same order used elsewhere (e.g., direction = "both"); ensure the error
handling invocation matches the updated signature of extractSubnetwork so the
test exercises the intended error path.
---
Nitpick comments:
In `@R/module-visualize-network-server.R`:
- Around line 184-187: The new parameters (filter_by_ptm_site,
include_infinite_fc, direction) should get the same defensive NULL handling as
filterByCuration: when building the list passed into the module, replace direct
input reads with guarded expressions (e.g., for filter_by_ptm_site and
include_infinite_fc coerce NULL to FALSE, and for direction coerce NULL to a
sensible default such as "both" or the module's expected default) so the code
using filterByCuration, filter_by_ptm_site, include_infinite_fc, and direction
never receives NULL; implement these guards using if (is.null(input$...))
<default> else input$... or a coalescing helper before passing them through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42d9ab12-1543-4dd5-b461-a430dee35889
📒 Files selected for processing (2)
R/module-visualize-network-server.Rtests/testthat/test_network_visualization.R
1 participant
Motivation and Context
This PR adds a collapsible "Advanced Options" panel to the network visualization module so users can refine subnetwork extraction. The UI exposes three new filters — PTM-site matching, inclusion of infinite fold-changes, and regulation direction — and these inputs are threaded through the server to MSstatsBioNet::getSubnetworkFromIndra and into the generated reproducible code. The change enables more precise, PTM-aware network extraction and ensures the advanced options are captured in exported code.
Detailed Changes
NAMESPACE / package imports
UI module (R/module-visualize-network-ui.R)
Server module and helpers (R/module-visualize-network-server.R, R/MSstatsShiny.R)
Tests (tests/testthat/test_network_visualization.R)
Unit tests added/modified
Coding guidelines / potential issues
Test coverage gaps:
Input default/null handling inconsistency:
Documentation missing:
Redundant useShinyjs():
Expanded import surface:
Downstream compatibility: