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 6 commits intodevelfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds two imports ( 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)
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 implements an "Advanced Options" panel for the network visualization module to let users fine-tune subnetwork extraction. The UI adds a collapsible advanced panel (toggleable via an action link) exposing three new filters — PTM-site matching, inclusion of infinite fold-changes, and regulation direction — and threads these inputs through server logic to the subnetwork extraction and generated reproducible code. The goal is to enable more precise, PTM-aware network extraction and reproducible code emission for downstream analyses.
Detailed Changes
NAMESPACE / package imports
UI module (R/module-visualize-network-ui.R)
Server module (R/module-visualize-network-server.R and helpers in R/MSstatsShiny.R)
Tests (tests/testthat/test_network_visualization.R)
Unit tests added/modified
Coding guidelines / potential issues
Test coverage gap:
Input default/null handling consistency:
Documentation missing for new parameters:
Duplicate useShinyjs() calls:
Imports surface expansion:
Test mock signatures: