tests(visualizeNetworksWithHTML): Add unit tests for generateCytoscapeConfig#55
tests(visualizeNetworksWithHTML): Add unit tests for generateCytoscapeConfig#55tonywu1999 merged 4 commits intodevelfrom
Conversation
WalkthroughRefactors Cytoscape config generation to accept raw Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (export/preview)
participant GenCfgNew as generateCytoscapeConfig (new)
participant Builder as createNodeElements / createEdgeElements
participant Assembler as Config assembler (convertStyle/Layout, JS)
participant Client as Cytoscape consumer
Caller->>GenCfgNew: call(nodes, edges, display_label_type)
GenCfgNew->>Builder: createNodeElements(nodes, display_label_type)
GenCfgNew->>Builder: createEdgeElements(edges)
Builder-->>GenCfgNew: node_elements, edge_elements
GenCfgNew->>Assembler: convertStyleToJS / convertLayoutToJS + build js_code
Assembler-->>GenCfgNew: style, layout, js_code
GenCfgNew->>Client: return config (elements, style, layout, container_id, js_code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (3)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (8)
tests/testthat/test-visualizeNetworksWithHTML.R (8)
1-3: Scope and PR body are misaligned with title; please update PR description/checklistThe PR title focuses on generateCytoscapeConfig, but this file adds tests for multiple APIs. Either update the PR body/checklist to reflect this broader scope or split tests by feature for clarity.
11-32: Two mock creators are unused; trim or add tests that use themcreate_mock_input_data() and create_mock_annotated_data() aren’t referenced in any test. Keeping unused helpers inflates maintenance.
Apply this diff to remove the unused helpers:
-# Mock data for testing -create_mock_input_data <- function() { - data.frame( - Protein = c("P53_HUMAN", "MDM2_HUMAN", "ATM_HUMAN", "BRCA1_HUMAN"), - log2FC = c(2.5, -1.8, 1.2, -2.1), - adj.pvalue = c(0.001, 0.02, 0.03, 0.005), - Label = rep("Treatment_vs_Control", 4), - stringsAsFactors = FALSE - ) -} - -create_mock_annotated_data <- function() { - data.frame( - Protein = c("P53_HUMAN", "MDM2_HUMAN", "ATM_HUMAN", "BRCA1_HUMAN"), - log2FC = c(2.5, -1.8, 1.2, -2.1), - adj.pvalue = c(0.001, 0.02, 0.03, 0.005), - Label = rep("Treatment_vs_Control", 4), - HgncId = c("101", "102", "103", "104"), - HgncName = c("TP53", "MDM2", "ATM", "BRCA1"), - stringsAsFactors = FALSE - ) -}If you plan to test annotation-aware behavior later, keep them and add corresponding tests.
34-54: Edge endpoints don’t match node ids; generated config will reference non-existent nodesNode ids are UniProt-style (e.g., P53_HUMAN) while edges use HGNC symbols (e.g., TP53). Unless createEdgeElements remaps to the node id field, edges will point to nodes that don’t exist in the generated graph.
Apply this diff to make the mock edges consistent with the node ids:
create_mock_subnetwork_edges <- function() { data.frame( - source = c("TP53", "MDM2", "ATM", "TP53"), - target = c("MDM2", "TP53", "TP53", "BRCA1"), + source = c("P53_HUMAN", "MDM2_HUMAN", "ATM_HUMAN", "P53_HUMAN"), + target = c("MDM2_HUMAN", "P53_HUMAN", "P53_HUMAN", "BRCA1_HUMAN"), interaction = c("Inhibition", "Activation", "Phosphorylation", "Complex"), evidenceCount = c(15, 8, 12, 5), evidenceLink = c("link1", "link2", "link3", "link4"), source_counts = c("{reach:10, signor:5}", "{reach:5,biopax:3}", "{reach:8,phosphoelm:4}", "{biopax:5}"), stringsAsFactors = FALSE ) }If the implementation expects HGNC symbol endpoints, instead switch node id to HGNC and adapt createNodeElements accordingly.
96-116: Make the relationship names checks case-insensitive and future-proofHard-coding lowercase names makes tests brittle if implementation capitalizes keys or uses synonyms.
Apply this diff to make checks robust:
- expect_type(props, "list") - expect_true("complex" %in% names(props)) - expect_true("regulatory" %in% names(props)) - expect_true("phosphorylation" %in% names(props)) - expect_true("other" %in% names(props)) + expect_type(props, "list") + nms <- tolower(names(props)) + expect_true(all(c("complex", "regulatory", "phosphorylation", "other") %in% nms)) @@ - complex_props <- props$complex + complex_props <- props[[match("complex", nms)]] @@ - reg_props <- props$regulatory + reg_props <- props[[match("regulatory", nms)]]
122-145: Consolidation test is directionally correct; add a guard against duplicate reverse edgesYou assert one inhibition edge remains. Consider asserting that the two reverse-direction rows have been merged, not just reduced in count.
Apply this diff to strengthen the check:
@@ - expect_equal(nrow(inhibition_edges), 1) - expect_equal(inhibition_edges$edge_type, "bidirectional") + expect_equal(nrow(inhibition_edges), 1) + expect_equal(inhibition_edges$edge_type, "bidirectional") + # Ensure only one canonical direction remains after consolidation + expect_false(any( + (consolidated$source == "TP53" & consolidated$target == "MDM2" & consolidated$interaction == "Inhibition") & + (consolidated$source == "MDM2" & consolidated$target == "TP53" & consolidated$interaction == "Inhibition") + ))If your consolidation canonicalizes order (e.g., alphabetical), adapt the predicate accordingly.
163-179: Don’t lock tests to specific style property names; accept common Cytoscape keysImplementations often use "line-color" and "target-arrow-shape" rather than color/arrow. Make the test accept any of the canonical keys.
Apply this diff to generalize the assertions:
@@ - expect_type(style, "list") - expect_true("color" %in% names(style)) - expect_equal(style$color, "#FF4444") # Red for inhibition + expect_type(style, "list") + color_key <- intersect(names(style), c("color", "line-color", "lineColor")) + expect_true(length(color_key) >= 1) + expect_true(any(style[color_key] == "#FF4444")) # Red for inhibition @@ - expect_equal(complex_style$arrow, "none") - expect_equal(complex_style$color, "#8B4513") + arrow_key <- intersect(names(complex_style), c("arrow", "arrow-shape", "target-arrow-shape")) + expect_true(length(arrow_key) >= 1) + expect_true(any(complex_style[arrow_key] %in% c("none", "none-inwards"))) + color_key2 <- intersect(names(complex_style), c("color", "line-color", "lineColor")) + expect_true(any(complex_style[color_key2] == "#8B4513")) @@ - expect_equal(unknown_style$color, "#666666") + color_key3 <- intersect(names(unknown_style), c("color", "line-color", "lineColor")) + expect_true(any(unknown_style[color_key3] == "#666666"))
235-254: Config test checks presence but not element integrity; add minimal structure assertionsAdd guardrails that elements is a list and non-empty, and layout has a name.
Apply this diff:
@@ - expect_true("elements" %in% names(config)) + expect_true("elements" %in% names(config)) + expect_type(config$elements, "list") + expect_gt(length(config$elements), 0) @@ - expect_true("layout" %in% names(config)) + expect_true("layout" %in% names(config)) + expect_true("name" %in% names(config$layout)) @@ - expect_type(config$js_code, "character") + expect_type(config$js_code, "character")If your config splits elements into nodes/edges (e.g., config$elements$nodes), consider asserting those counts match the inputs.
256-278: Custom config test is solid; consider asserting handler integration more directlyYou already check console.log presence. If event handler keys are serialized, assert specific identifiers (e.g., edge_click) appear in js_code to catch mapping errors.
Apply this diff:
@@ - expect_true(grepl("console.log", config$js_code)) + expect_true(grepl("console.log", config$js_code)) + expect_true(grepl("edge_click", config$js_code))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/testthat/test-visualizeNetworksWithHTML.R(1 hunks)
🔇 Additional comments (2)
tests/testthat/test-visualizeNetworksWithHTML.R (2)
67-90: Good coverage for mapLogFCToColor; verify assumptions on NA/constant handlingThe test asserts NA and constant vectors map to #D3D3D3. That’s a policy choice; confirm the implementation’s contract matches this before locking it in.
147-157: Empty-input consolidation test looks goodGraceful handling of empty edge frames is validated correctly.
| test_that("mapLogFCToColor handles various input scenarios", { | ||
|
|
||
| # Test normal case with varied logFC values | ||
| logFC_values <- c(-2, -1, 0, 1, 2) | ||
| colors <- mapLogFCToColor(logFC_values) | ||
| expect_equal(length(colors), 5) | ||
| expect_true(all(grepl("^#[0-9A-Fa-f]{6}$", colors))) # Valid hex colors | ||
|
|
||
| # Test case with all NA values | ||
| na_values <- c(NA, NA, NA) | ||
| na_colors <- mapLogFCToColor(na_values) | ||
| expect_equal(length(na_colors), 3) | ||
| expect_true(all(na_colors == "#D3D3D3")) | ||
|
|
||
| # Test case with all same values | ||
| same_values <- c(1, 1, 1) | ||
| same_colors <- mapLogFCToColor(same_values) | ||
| expect_equal(length(same_colors), 3) | ||
| expect_true(all(same_colors == "#D3D3D3")) | ||
|
|
||
| # Test empty input | ||
| empty_colors <- mapLogFCToColor(numeric(0)) | ||
| expect_equal(length(empty_colors), 0) | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm these functions are exported; otherwise tests must call ::: or use helpers
The tests call mapLogFCToColor, getRelationshipProperties, consolidateEdges, getEdgeStyle, createNodeElements, createEdgeElements, and generateCytoscapeConfig directly. If any are internal, tests will fail under R CMD check.
Run this script to verify NAMESPACE exports and function definitions:
If some are internal, update tests to use MSstatsBioNet:::functionName or expose a test-only wrapper.
🏁 Script executed:
#!/bin/bash
# Verify exports exist in NAMESPACE
rg -nP '^(export\\((mapLogFCToColor|getRelationshipProperties|consolidateEdges|getEdgeStyle|createNodeElements|createEdgeElements|generateCytoscapeConfig)\\))' NAMESPACE || true
# Verify function definitions exist in R/ sources
rg -nP --type=r -C2 '^(mapLogFCToColor|getRelationshipProperties|consolidateEdges|getEdgeStyle|createNodeElements|createEdgeElements|generateCytoscapeConfig)\\s*<-\\s*function\\b'Length of output: 360
🏁 Script executed:
#!/bin/bash
# List top‐level R/ directory contents
echo "R/ directory files:"
find R -maxdepth 1 -type f -name "*.R" 2>/dev/null || echo "No R/ directory found"
echo
# List any .R files elsewhere
echo "Other .R files in repo:"
find . -type f -name "*.R" ! -path "./R/*" 2>/dev/null
echo
# Search for definitions and references of target functions
for fn in mapLogFCToColor getRelationshipProperties consolidateEdges getEdgeStyle createNodeElements createEdgeElements generateCytoscapeConfig; do
echo "---- Occurrences of ${fn} ----"
rg -n "${fn}" || echo "No occurrences of ${fn}"
echo
done
echo
# Inspect NAMESPACE exports
if [ -f NAMESPACE ]; then
echo "NAMESPACE exports:"
rg -n "export" NAMESPACE || echo "No export lines in NAMESPACE"
echo
echo "Export entries matching target functions:"
rg -nP "export\\((mapLogFCToColor|getRelationshipProperties|consolidateEdges|getEdgeStyle|createNodeElements|createEdgeElements|generateCytoscapeConfig)\\)" NAMESPACE || echo "No matching exports"
else
echo "NAMESPACE file not found"
fiLength of output: 9613
Internal functions need exporting or tests must call via :::
Only generateCytoscapeConfig is exported in NAMESPACE; the following functions are unexported yet directly invoked in your tests, which will fail under R CMD check:
- mapLogFCToColor
- getRelationshipProperties
- consolidateEdges
- getEdgeStyle
- createNodeElements
- createEdgeElements
Please choose one of the following to ensure tests pass:
• Export these functions in NAMESPACE (e.g. export(mapLogFCToColor), etc.)
• Update tests to call them via MSstatsBioNet:::mapLogFCToColor, etc.
🤖 Prompt for AI Agents
In tests/testthat/test-visualizeNetworksWithHTML.R around lines 67 to 90, the
unit tests call unexported internal functions (mapLogFCToColor,
getRelationshipProperties, consolidateEdges, getEdgeStyle, createNodeElements,
createEdgeElements) which will fail under R CMD check; fix by either exporting
each of these functions in the package NAMESPACE (add export(...) entries for
each function) or keep them internal and change the tests to reference them with
the triple-colon syntax (MSstatsBioNet:::mapLogFCToColor, etc.) so the tests use
the correct access method.
| test_that("createNodeElements creates proper node structures", { | ||
| nodes <- create_mock_subnetwork_nodes() | ||
|
|
||
| # Test with default label type (id) | ||
| node_elements <- createNodeElements(nodes, "id") | ||
| expect_equal(length(node_elements), nrow(nodes)) | ||
| expect_true(all(grepl("data:", node_elements))) | ||
| expect_true(all(grepl("id:", node_elements))) | ||
| expect_true(all(grepl("label:", node_elements))) | ||
|
|
||
| # Test with hgncName label type | ||
| node_elements_hgnc <- createNodeElements(nodes, "hgncName") | ||
| expect_equal(length(node_elements_hgnc), nrow(nodes)) | ||
|
|
||
| # Test nodes without logFC column | ||
| nodes_no_logfc <- nodes[, !names(nodes) %in% "logFC"] | ||
| node_elements_no_logfc <- createNodeElements(nodes_no_logfc, "id") | ||
| expect_equal(length(node_elements_no_logfc), nrow(nodes_no_logfc)) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Node element tests assume stringified elements; prefer structural assertions
If createNodeElements returns Cytoscape-compatible R lists (common), grepl("data:", node_elements) will fail. Assert list structure and required keys instead.
Apply this diff to assert structure rather than string content:
@@
- node_elements <- createNodeElements(nodes, "id")
- expect_equal(length(node_elements), nrow(nodes))
- expect_true(all(grepl("data:", node_elements)))
- expect_true(all(grepl("id:", node_elements)))
- expect_true(all(grepl("label:", node_elements)))
+ node_elements <- createNodeElements(nodes, "id")
+ expect_equal(length(node_elements), nrow(nodes))
+ expect_true(all(vapply(node_elements, function(e) is.list(e) && "data" %in% names(e), logical(1))))
+ expect_true(all(vapply(node_elements, function(e) all(c("id", "label") %in% names(e$data)), logical(1))))
@@
- node_elements_hgnc <- createNodeElements(nodes, "hgncName")
- expect_equal(length(node_elements_hgnc), nrow(nodes))
+ node_elements_hgnc <- createNodeElements(nodes, "hgncName")
+ expect_equal(length(node_elements_hgnc), nrow(nodes))
+ # spot-check that HGNC labels are used
+ labels <- vapply(node_elements_hgnc, function(e) e$data$label, character(1))
+ expect_true(any(labels %in% c("TP53", "MDM2", "ATM", "BRCA1")))
@@
- node_elements_no_logfc <- createNodeElements(nodes_no_logfc, "id")
- expect_equal(length(node_elements_no_logfc), nrow(nodes_no_logfc))
+ node_elements_no_logfc <- createNodeElements(nodes_no_logfc, "id")
+ expect_equal(length(node_elements_no_logfc), nrow(nodes_no_logfc))
+ # ensure absence of logFC is handled (no error and data key still present)
+ expect_true(all(vapply(node_elements_no_logfc, function(e) "data" %in% names(e), logical(1))))If your functions intentionally return stringified YAML/JSON, ignore this and keep the original assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_that("createNodeElements creates proper node structures", { | |
| nodes <- create_mock_subnetwork_nodes() | |
| # Test with default label type (id) | |
| node_elements <- createNodeElements(nodes, "id") | |
| expect_equal(length(node_elements), nrow(nodes)) | |
| expect_true(all(grepl("data:", node_elements))) | |
| expect_true(all(grepl("id:", node_elements))) | |
| expect_true(all(grepl("label:", node_elements))) | |
| # Test with hgncName label type | |
| node_elements_hgnc <- createNodeElements(nodes, "hgncName") | |
| expect_equal(length(node_elements_hgnc), nrow(nodes)) | |
| # Test nodes without logFC column | |
| nodes_no_logfc <- nodes[, !names(nodes) %in% "logFC"] | |
| node_elements_no_logfc <- createNodeElements(nodes_no_logfc, "id") | |
| expect_equal(length(node_elements_no_logfc), nrow(nodes_no_logfc)) | |
| }) | |
| test_that("createNodeElements creates proper node structures", { | |
| nodes <- create_mock_subnetwork_nodes() | |
| # Test with default label type (id) | |
| node_elements <- createNodeElements(nodes, "id") | |
| expect_equal(length(node_elements), nrow(nodes)) | |
| expect_true(all(vapply(node_elements, function(e) is.list(e) && "data" %in% names(e), logical(1)))) | |
| expect_true(all(vapply(node_elements, function(e) all(c("id", "label") %in% names(e$data)), logical(1)))) | |
| # Test with hgncName label type | |
| node_elements_hgnc <- createNodeElements(nodes, "hgncName") | |
| expect_equal(length(node_elements_hgnc), nrow(nodes)) | |
| # spot-check that HGNC labels are used | |
| labels <- vapply(node_elements_hgnc, function(e) e$data$label, character(1)) | |
| expect_true(any(labels %in% c("TP53", "MDM2", "ATM", "BRCA1"))) | |
| # Test nodes without logFC column | |
| nodes_no_logfc <- nodes[, !names(nodes) %in% "logFC"] | |
| node_elements_no_logfc <- createNodeElements(nodes_no_logfc, "id") | |
| expect_equal(length(node_elements_no_logfc), nrow(nodes_no_logfc)) | |
| # ensure absence of logFC is handled (no error and data key still present) | |
| expect_true(all(vapply(node_elements_no_logfc, function(e) "data" %in% names(e), logical(1)))) | |
| }) |
🤖 Prompt for AI Agents
In tests/testthat/test-visualizeNetworksWithHTML.R around lines 185 to 203, the
test uses grepl on returned node_elements which breaks if createNodeElements
returns structured R lists; change assertions to check structure instead:
confirm node_elements is a list of length nrow(nodes), iterate elements to
assert each is a list with a "data" entry, that data is a list containing "id"
and "label" (or that label equals the requested label type like "hgncName" in
the second case), and for the nodes_no_logfc case assert the same structural
requirements still hold; keep the length assertions and remove string-based
grepl checks.
| test_that("createEdgeElements creates proper edge structures", { | ||
| edges <- create_mock_subnetwork_edges() | ||
|
|
||
| edge_elements <- createEdgeElements(edges) | ||
| expect_type(edge_elements, "list") | ||
| expect_gt(length(edge_elements), 0) | ||
|
|
||
| # Check that all elements contain required fields | ||
| expect_true(all(sapply(edge_elements, function(x) grepl("source:", x)))) | ||
| expect_true(all(sapply(edge_elements, function(x) grepl("target:", x)))) | ||
|
|
||
| # Test empty edges | ||
| empty_edges <- data.frame( | ||
| source = character(0), | ||
| target = character(0), | ||
| interaction = character(0), | ||
| stringsAsFactors = FALSE | ||
| ) | ||
| empty_elements <- createEdgeElements(empty_edges) | ||
| expect_equal(length(empty_elements), 0) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Edge element tests also assume stringified elements; check data payload instead
Follow the node-element pattern and assert nested data keys exist for source/target.
Apply this diff:
@@
- edge_elements <- createEdgeElements(edges)
- expect_type(edge_elements, "list")
- expect_gt(length(edge_elements), 0)
+ edge_elements <- createEdgeElements(edges)
+ expect_type(edge_elements, "list")
+ expect_gt(length(edge_elements), 0)
@@
- expect_true(all(sapply(edge_elements, function(x) grepl("source:", x))))
- expect_true(all(sapply(edge_elements, function(x) grepl("target:", x))))
+ expect_true(all(vapply(edge_elements, function(e) is.list(e) && "data" %in% names(e), logical(1))))
+ expect_true(all(vapply(edge_elements, function(e) all(c("source", "target") %in% names(e$data)), logical(1))))
@@
- empty_elements <- createEdgeElements(empty_edges)
- expect_equal(length(empty_elements), 0)
+ empty_elements <- createEdgeElements(empty_edges)
+ expect_equal(length(empty_elements), 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_that("createEdgeElements creates proper edge structures", { | |
| edges <- create_mock_subnetwork_edges() | |
| edge_elements <- createEdgeElements(edges) | |
| expect_type(edge_elements, "list") | |
| expect_gt(length(edge_elements), 0) | |
| # Check that all elements contain required fields | |
| expect_true(all(sapply(edge_elements, function(x) grepl("source:", x)))) | |
| expect_true(all(sapply(edge_elements, function(x) grepl("target:", x)))) | |
| # Test empty edges | |
| empty_edges <- data.frame( | |
| source = character(0), | |
| target = character(0), | |
| interaction = character(0), | |
| stringsAsFactors = FALSE | |
| ) | |
| empty_elements <- createEdgeElements(empty_edges) | |
| expect_equal(length(empty_elements), 0) | |
| }) | |
| test_that("createEdgeElements creates proper edge structures", { | |
| edges <- create_mock_subnetwork_edges() | |
| edge_elements <- createEdgeElements(edges) | |
| expect_type(edge_elements, "list") | |
| expect_gt(length(edge_elements), 0) | |
| # Check that all elements contain required fields | |
| expect_true(all(vapply(edge_elements, function(e) is.list(e) && "data" %in% names(e), logical(1)))) | |
| expect_true(all(vapply(edge_elements, function(e) all(c("source", "target") %in% names(e$data)), logical(1)))) | |
| # Test empty edges | |
| empty_edges <- data.frame( | |
| source = character(0), | |
| target = character(0), | |
| interaction = character(0), | |
| stringsAsFactors = FALSE | |
| ) | |
| empty_elements <- createEdgeElements(empty_edges) | |
| expect_equal(length(empty_elements), 0) | |
| }) |
🤖 Prompt for AI Agents
In tests/testthat/test-visualizeNetworksWithHTML.R around lines 209 to 229, the
edge-element assertions currently look for "source:"/"target:" substrings in
stringified elements; instead follow the node-element pattern and assert the
actual nested data payload keys exist. Update the test to iterate over
edge_elements and for each element assert it is a list and contains a "data"
entry, then assert that element$data contains non-null "source" and "target"
keys (and optionally "interaction"), and keep the empty-edges case expecting
length 0; ensure use of is.list(), names(), and !is.null() (or identical) checks
rather than grepl on strings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #55 +/- ##
===========================================
+ Coverage 32.23% 57.06% +24.82%
===========================================
Files 8 8
Lines 1213 1211 -2
===========================================
+ Hits 391 691 +300
+ Misses 822 520 -302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 (4)
R/visualizeNetworksWithHTML.R (4)
526-530: Docs example uses old signature; update to nodes/edgesThe example still shows node_elements/edge_elements even though generateCytoscapeConfig now accepts nodes/edges. This will mislead users.
Apply this diff:
-#' config <- generateCytoscapeConfig(node_elements, edge_elements) +#' config <- generateCytoscapeConfig(nodes, edges, display_label_type = "id")
233-243: Escape node ids/labels for JS; current string concatenation risks broken JS/XSSNode ids/labels are inserted into JS without escaping. A single quote, backslash, or special char in id/label will break the generated JS (and is a potential injection vector if inputs are not fully controlled).
Apply this diff to escape the injected strings:
- apply(cbind(nodes, color = node_colors), 1, function(row) { + apply(cbind(nodes, color = node_colors), 1, function(row) { # Use the appropriate label, fallback to id if hgncName is missing/empty - display_label <- if(label_column == "hgncName" && !is.na(row['hgncName']) && row['hgncName'] != "") { - row['hgncName'] - } else { - row['id'] - } - - paste0("{ data: { id: '", row['id'], "', label: '", display_label, "', color: '", row['color'], "' } }") + display_label <- if (label_column == "hgncName" && !is.na(row['hgncName']) && row['hgncName'] != "") { + row['hgncName'] + } else { + row['id'] + } + + id_esc <- escapeForJS(row['id']) + label_esc <- escapeForJS(display_label) + color_esc <- escapeForJS(row['color']) + + paste0("{ data: { id: '", id_esc, "', label: '", label_esc, "', color: '", color_esc, "' } }") })Add this helper function once in the module (e.g., near other helpers):
# Escape strings for safe injection into single-quoted JS strings escapeForJS <- function(x) { x <- as.character(x) x <- gsub("\\\\", "\\\\\\\\", x) # backslash x <- gsub("'", "\\\\'", x) # single quote x <- gsub("\n", "\\\\n", x) # newline x <- gsub("\r", "\\\\r", x) # carriage return x <- gsub("\t", "\\\\t", x) # tab x }
260-271: Escape edge fields for JS; avoid unescaped injection into generated codeSame concern as nodes: edge source/target/interaction/category are concatenated into JS without escaping.
Apply this diff:
- edge_data <- paste0("{ data: { source: '", row$source, - "', target: '", row$target, - "', id: '", edge_key, - "', interaction: '", row$interaction, - "', edge_type: '", row$edge_type, - "', category: '", row$category, - "', color: '", style$color, - "', line_style: '", style$style, - "', arrow_shape: '", style$arrow, - "', width: ", style$width, " } }") + source_esc <- escapeForJS(row$source) + target_esc <- escapeForJS(row$target) + id_esc <- escapeForJS(edge_key) + interaction_esc <- escapeForJS(row$interaction) + edge_type_esc <- escapeForJS(row$edge_type) + category_esc <- escapeForJS(row$category) + color_esc <- escapeForJS(style$color) + line_style_esc <- escapeForJS(style$style) + arrow_shape_esc <- escapeForJS(style$arrow) + + edge_data <- paste0("{ data: { source: '", source_esc, + "', target: '", target_esc, + "', id: '", id_esc, + "', interaction: '", interaction_esc, + "', edge_type: '", edge_type_esc, + "', category: '", category_esc, + "', color: '", color_esc, + "', line_style: '", line_style_esc, + "', arrow_shape: '", arrow_shape_esc, + "', width: ", style$width, " } }")Note: This assumes you’ve added escapeForJS (see previous comment).
409-459: Update remaining call sites to use the newnodes, edgesparametersTwo locations still invoke the old signature with
node_elementsandedge_elementsand must be updated:
- R/visualizeNetworksWithHTML.R line 526 (roxygen example)
-#' config <- generateCytoscapeConfig(node_elements, edge_elements) +#' config <- generateCytoscapeConfig(nodes, edges)- R/visualizeNetworksWithHTML.R line 922 (inside
previewNetworkInBrowser)-config <- generateCytoscapeConfig(node_elements, edge_elements, display_label_type = displayLabelType) +config <- generateCytoscapeConfig(nodes, edges, display_label_type = displayLabelType)Once these are updated, all call sites will align with the revised function signature.
♻️ Duplicate comments (3)
tests/testthat/test-visualizeNetworksWithHTML.R (3)
185-203: Assertion style is string-centric; consider structural checks if elements become listsCurrent code returns stringified elements, so these assertions are fine. If you later switch to structured lists (recommended for safety/escaping), update tests to assert presence of data$id/label keys instead of grepping strings.
209-229: Edge element assertions also assume stringified outputSame note as nodes: if createEdgeElements returns structured lists later, assert nested data keys (source/target) instead of grepping.
67-90: Tests call internal helpers directly; use triple-colon or export themmapLogFCToColor, getRelationshipProperties, consolidateEdges, getEdgeStyle, createNodeElements, createEdgeElements, convertStyleToJS, and convertLayoutToJS are not exported. Direct calls will fail under R CMD check.
Option A (keep them internal): Bind local aliases at the top of the test file so calls remain concise.
@@ -# Load required libraries +# Load required libraries + +# Access internal functions for testing +mapLogFCToColor <- MSstatsBioNet:::mapLogFCToColor +getRelationshipProperties <- MSstatsBioNet:::getRelationshipProperties +consolidateEdges <- MSstatsBioNet:::consolidateEdges +getEdgeStyle <- MSstatsBioNet:::getEdgeStyle +createNodeElements <- MSstatsBioNet:::createNodeElements +createEdgeElements <- MSstatsBioNet:::createEdgeElements +convertStyleToJS <- MSstatsBioNet:::convertStyleToJS +convertLayoutToJS <- MSstatsBioNet:::convertLayoutToJSOption B (make them public): add @export tags in R/visualizeNetworksWithHTML.R and regenerate NAMESPACE.
Run this to see what’s exported today:
#!/bin/bash rg -nP '^(export\\()' NAMESPACE || true
🧹 Nitpick comments (3)
R/visualizeNetworksWithHTML.R (3)
284-287: Clarify and polish parameter docs for display_label_typeMinor clarity/grammar issue and missing allowed values. Suggest explicitly listing accepted values and the expected source column.
-#' @param display_label_type column of nodes table for displaying node names +#' @param display_label_type Which column to use for node labels ("id" or "hgncName"). +#' When "hgncName" is selected but the column is missing/NA, the function falls back to "id".
98-101: Remove unused variable reverse_keyreverse_key is computed but never used.
- reverse_key <- paste(sort(c(edge$source, edge$target), decreasing = TRUE), edge$interaction, sep = "-")
320-327: Optional: normalize parameter naming (snake_case vs camelCase)generateCytoscapeConfig uses display_label_type; exportNetworkToHTML/previewNetworkInBrowser use displayLabelType. Consider standardizing across the module to reduce cognitive load (pick one convention and stick to it).
If you choose to standardize on snake_case, I can generate a follow-up refactor and update call sites/tests accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
R/visualizeNetworksWithHTML.R(4 hunks)tests/testthat/test-visualizeNetworksWithHTML.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). (3)
- GitHub Check: pkgdown
- GitHub Check: test-coverage
- GitHub Check: build
🔇 Additional comments (4)
R/visualizeNetworksWithHTML.R (1)
303-306: LGTM: Centralizing element creation inside generateCytoscapeConfigCreating node/edge elements internally from raw nodes/edges simplifies the API and reduces duplication at call sites.
tests/testthat/test-visualizeNetworksWithHTML.R (3)
235-273: LGTM: Comprehensive coverage for config generation and custom parametersGood validation of defaults, js_code presence/length, layout overrides, container_id, and event handler inclusion.
279-309: LGTM: Style/layout conversion tests validate function/non-quoted handlingThese assertions will catch regressions in quoting and function string handling.
311-327: LGTM: Label-type variations and fallback behavior coveredGood checks for id vs hgncName labels and fallback when hgncName is missing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/visualizeNetworksWithHTML.R (1)
523-531: Outdated example still uses old signature (node_elements/edge_elements)This will mislead users post-API change. Update to pass raw nodes/edges and optionally show display_label_type usage.
-#' # Assuming you have nodes and edges data -#' config <- generateCytoscapeConfig(node_elements, edge_elements) +#' # Assuming you have data frames: nodes (id[, hgncName, logFC]) and edges (source, target, interaction) +#' config <- generateCytoscapeConfig(nodes, edges, display_label_type = "id")
🧹 Nitpick comments (1)
R/visualizeNetworksWithHTML.R (1)
284-286: Align parameter docs with actual inputs and enumerate accepted valuesDocs still say "List" but the implementation expects data frames; also clarify allowed values for display_label_type.
Apply this doc tweak:
-#' @param nodes List of nodes from getSubnetworkFromIndra -#' @param edges List of edges from getSubnetworkFromIndra -#' @param display_label_type column of nodes table for displaying node names +#' @param nodes Data frame of nodes (required: column `id`; optional: `hgncName`, `logFC`) +#' @param edges Data frame of edges (required: columns `source`, `target`, `interaction`; optional: extra columns preserved) +#' @param display_label_type Which node column to display as labels: "id" (default) or "hgncName"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
R/visualizeNetworksWithHTML.R(4 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). (3)
- GitHub Check: pkgdown
- GitHub Check: test-coverage
- GitHub Check: build
🔇 Additional comments (2)
R/visualizeNetworksWithHTML.R (2)
901-906: Call site updated to new signature — goodexportNetworkToHTML now forwards nodes/edges and displayLabelType correctly to generateCytoscapeConfig.
917-923: Fix confirmed: previewNetworkInBrowser now passes nodes/edges (resolves earlier breakage)This addresses the earlier report about undefined node_elements/edge_elements in this scope.
| generateCytoscapeConfig <- function(nodes, edges, | ||
| display_label_type = "id", | ||
| container_id = "network-cy", | ||
| event_handlers = NULL, | ||
| layout_options = NULL) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API change to accept raw nodes/edges looks good; verify all call sites are updated
Great simplification. Please ensure no callers still pass node_elements/edge_elements.
Run this to find any outdated usages:
🏁 Script executed:
#!/bin/bash
# Find any calls still passing node_elements/edge_elements
rg -nP -C2 --glob '!**/renv/**' "generateCytoscapeConfig\\s*\\(\\s*node_elements\\s*,\\s*edge_elements"
# List all call sites for manual inspection (arity, named args)
rg -nP -C2 --glob '!**/renv/**' "\\bgenerateCytoscapeConfig\\s*\\("Length of output: 4102
Update documentation examples to use nodes, edges instead of node_elements, edge_elements
The code itself no longer has any live call sites passing the old parameter names, but the examples and manual pages still reference them. Please update the following:
- In R/visualizeNetworksWithHTML.R (lines ~524–528), update the roxygen “dontrun” example:
#' \dontrun{ #' # Assuming you have nodes and edges data
-#' config <- generateCytoscapeConfig(node_elements, edge_elements)
+#' config <- generateCytoscapeConfig(nodes, edges)
#'
#' # Export to HTML
#' }
- In man/generateCytoscapeConfig.Rd (lines 7–9), update the `\usage{}` section:
```diff
\usage{
- generateCytoscapeConfig(
- node_elements,
- edge_elements,
+ generateCytoscapeConfig(
+ nodes,
+ edges,
display_label_type = "id",
container_id = "network-cy",
event_handlers = NULL,
layout_options = NULL
}
Once those are adjusted, all references to the old API will be removed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| generateCytoscapeConfig <- function(nodes, edges, | |
| display_label_type = "id", | |
| container_id = "network-cy", | |
| event_handlers = NULL, | |
| layout_options = NULL) { | |
| # In R/visualizeNetworksWithHTML.R (around lines 524–528) | |
| #' \dontrun{ | |
| #' # Assuming you have nodes and edges data | |
| #' config <- generateCytoscapeConfig(nodes, edges) | |
| #' | |
| #' # Export to HTML | |
| #' } |
| generateCytoscapeConfig <- function(nodes, edges, | |
| display_label_type = "id", | |
| container_id = "network-cy", | |
| event_handlers = NULL, | |
| layout_options = NULL) { | |
| # In man/generateCytoscapeConfig.Rd (around lines 7–9) | |
| \usage{ | |
| generateCytoscapeConfig( | |
| nodes, | |
| edges, | |
| display_label_type = "id", | |
| container_id = "network-cy", | |
| event_handlers = NULL, | |
| layout_options = NULL | |
| } |
🤖 Prompt for AI Agents
In R/visualizeNetworksWithHTML.R around lines 524–528 and
man/generateCytoscapeConfig.Rd around lines 7–9, update doc examples/usages that
still reference the old parameter names `node_elements` and `edge_elements` to
use the current API parameter names `nodes` and `edges` instead; specifically
modify the roxygen dontrun example in R/visualizeNetworksWithHTML.R to call
generateCytoscapeConfig(nodes, edges, ...) and edit the \usage{} section in
man/generateCytoscapeConfig.Rd to show generateCytoscapeConfig(nodes, edges,
display_label_type = "id", container_id = "network-cy", event_handlers = NULL,
layout_options = NULL) so the docs match the function signature.
| # Create elements | ||
| node_elements <- createNodeElements(nodes, display_label_type) | ||
| edge_elements <- createEdgeElements(edges) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate inputs before element creation + fix downstream NA label handling
Two actionable items:
- Add explicit input validation for required columns to fail fast with clear messages.
- createNodeElements uses apply over a data.frame, coercing NAs to the string "NA" when display_label_type = "hgncName" — labels can incorrectly render as "NA". Implement vectorised construction to preserve real NA semantics.
Minimal validation immediately before element creation:
- # Create elements
+ # Validate required columns before element creation
+ req_node_cols <- c("id")
+ miss_node <- setdiff(req_node_cols, names(nodes))
+ if (length(miss_node) > 0) {
+ stop("nodes is missing required column(s): ", paste(miss_node, collapse = ", "))
+ }
+ req_edge_cols <- c("source", "target", "interaction")
+ miss_edge <- setdiff(req_edge_cols, names(edges))
+ if (length(miss_edge) > 0) {
+ stop("edges is missing required column(s): ", paste(miss_edge, collapse = ", "))
+ }
+
+ # Create elements
node_elements <- createNodeElements(nodes, display_label_type)
edge_elements <- createEdgeElements(edges)Outside this hunk (in createNodeElements), replace the apply() block with a NA-safe, vectorised version like:
createNodeElements <- function(nodes, displayLabelType = "id") {
# colors as before ...
if ("logFC" %in% names(nodes)) node_colors <- mapLogFCToColor(nodes$logFC) else node_colors <- rep("#D3D3D3", nrow(nodes))
label_column <- if (displayLabelType == "hgncName" && "hgncName" %in% names(nodes)) "hgncName" else "id"
df <- nodes
df$color <- node_colors
labels <- if (label_column == "hgncName") ifelse(!is.na(df$hgncName) & nzchar(df$hgncName), df$hgncName, df$id) else df$id
# optionally escape single quotes in labels/ids to avoid breaking JS strings
esc <- function(x) gsub("'", "\\\\'", x, fixed = TRUE)
mapply(function(id, label, color) {
paste0("{ data: { id: '", esc(id), "', label: '", esc(label), "', color: '", color, "' } }")
}, df$id, labels, df$color, USE.NAMES = FALSE)
}🤖 Prompt for AI Agents
In R/visualizeNetworksWithHTML.R around lines 303 to 306, add explicit input
validation immediately before creating elements to assert required columns exist
(e.g., id, and optionally hgncName/logFC) and fail fast with clear messages;
then update createNodeElements to stop using apply over the data.frame and
instead use vectorised operations that preserve NA semantics: compute node
colors vectorised (use default color if logFC missing), select label_column
based on displayLabelType and availability, build labels with an NA-safe
fallback (if hgncName is NA or empty, use id), escape single quotes in
ids/labels to avoid breaking generated JS, and construct the element strings
with mapply or paste0 over the column vectors.
tonywu1999
changed the title
2 participants
PR Type
Tests
Description
Add comprehensive unit tests for visualization
Cover color mapping and edge consolidation
Validate edge and node element creation
Test Cytoscape config generation and options
Diagram Walkthrough
File Walkthrough
test-visualizeNetworksWithHTML.R
Add end-to-end tests for visualization helperstests/testthat/test-visualizeNetworksWithHTML.R
Summary by CodeRabbit
Tests
Refactor
Documentation