feature(visualizeNetworksWithHTML): Add INDRA link into javascript visualization#57
feature(visualizeNetworksWithHTML): Add INDRA link into javascript visualization#57tonywu1999 merged 3 commits intodevelfrom
Conversation
WalkthroughAdds sanitization and embedding of an optional evidenceLink into edge data emitted by createEdgeElements in Changes
Sequence Diagram(s)sequenceDiagram
participant R as R createEdgeElements
participant Helper as escape_js_string
participant JS as Output HTML/JS
R->>R: read row$evidenceLink (or NA)
R->>R: normalize NA/"NA" -> ""
R->>Helper: escape_js_string(evidence_link)
Helper-->>R: escaped string
R->>JS: build edge_data including evidenceLink (after category, before color)
JS-->>R: serialized edge objects embedded in HTML
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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 (
|
tonywu1999
changed the title
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: 0
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)
261-271: Escape and sanitize evidenceLink to avoid broken JS and potential injection
row$evidenceLinkis concatenated into JavaScript without escaping; quotes or newlines will break the script, and NA becomes the literal string "NA". Precompute a sanitized value and use it.Apply this diff within the loop to sanitize and use
evidence_link:for (i in 1:nrow(consolidated_edges)) { row <- consolidated_edges[i,] edge_key <- paste(row$source, row$target, row$interaction, sep = "-") # Get styling for this edge style <- getEdgeStyle(row$interaction, row$category, row$edge_type) + # Sanitize optional evidenceLink + evidence_link <- if ("evidenceLink" %in% names(row)) row$evidenceLink else NA_character_ + evidence_link <- ifelse(is.na(evidence_link) | evidence_link == "NA", "", evidence_link) + evidence_link <- escape_js_string(evidence_link) + # Create edge data with styling information edge_data <- paste0("{ data: { source: '", row$source, "', target: '", row$target, "', id: '", edge_key, "', interaction: '", row$interaction, "', edge_type: '", row$edge_type, "', category: '", row$category, - "', evidenceLink: '", row$evidenceLink, + "', evidenceLink: '", evidence_link, "', color: '", style$color, "', line_style: '", style$style, "', arrow_shape: '", style$arrow, "', width: ", style$width, " } }")Add this helper (outside the loop, e.g., near other helpers):
# Helper to safely embed strings into JS single-quoted literals escape_js_string <- function(x) { if (is.null(x)) return("") x <- as.character(x) x <- gsub("\\\\", "\\\\\\\\", x) # backslashes x <- gsub("'", "\\\\'", x) # single quotes x <- gsub("\r", "\\\\r", x) x <- gsub("\n", "\\\\n", x) x }I can wire this up across node labels too to harden the whole serializer.
🧹 Nitpick comments (1)
R/visualizeNetworksWithHTML.R (1)
261-271: Optionally omit evidenceLink when empty to reduce noiseIf
evidence_linkis empty, consider omitting the key entirely instead of emittingevidenceLink: ''.Example tweak inside the loop:
evidence_part <- if (nzchar(evidence_link)) paste0("', evidenceLink: '", evidence_link, "'") else "" edge_data <- paste0("{ data: { source: '", row$source, "', target: '", row$target, "', id: '", edge_key, "', interaction: '", row$interaction, "', edge_type: '", row$edge_type, "', category: '", row$category, evidence_part, "', color: '", style$color, # note leading quote stays balanced when evidence_part is empty "', line_style: '", style$style, "', arrow_shape: '", style$arrow, "', width: ", style$width, " } }")
📜 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(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: build
- GitHub Check: test-coverage
🔇 Additional comments (1)
R/visualizeNetworksWithHTML.R (1)
261-271: Confirmed evidenceLink presence and type
Edges always include anevidenceLinkcharacter column (documented in R/visualizeNetworks.R and man pages, populated with a default""upstream in both utils_getSubnetworkFromIndra.R and getPathwaysFromIndra.R, and covered by tests), so no additional handling is required downstream.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #57 +/- ##
==========================================
+ Coverage 57.06% 57.36% +0.30%
==========================================
Files 8 8
Lines 1211 1222 +11
==========================================
+ Hits 691 701 +10
- Misses 520 521 +1 ☔ 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 (1)
R/visualizeNetworksWithHTML.R (1)
265-276: Escape all string fields in edge payload, not just evidenceLink.Only
evidenceLinkis escaped; other fields can contain quotes or backslashes (or user-provided text) and may break the JS literal or enable injection. Please escape all string-valued fields before concatenation.# Create edge data with styling information - edge_data <- paste0("{ data: { source: '", row$source, - "', target: '", row$target, - "', id: '", edge_key, - "', interaction: '", row$interaction, - "', edge_type: '", row$edge_type, - "', category: '", row$category, + # Sanitize string fields + src <- escape_js_string(row$source) + tgt <- escape_js_string(row$target) + itx <- escape_js_string(row$interaction) + etype <- escape_js_string(row$edge_type) + catg <- escape_js_string(row$category) + ekey <- escape_js_string(edge_key) + + edge_data <- paste0("{ data: { source: '", src, + "', target: '", tgt, + "', id: '", ekey, + "', interaction: '", itx, + "', edge_type: '", etype, + "', category: '", catg, "', evidenceLink: '", evidence_link, "', color: '", style$color, "', line_style: '", style$style, "', arrow_shape: '", style$arrow, "', width: ", style$width, " } }")Note: If any of these fields can be non-character types (e.g., factors),
escape_js_string()already coerces them safely.
🧹 Nitpick comments (3)
R/visualizeNetworksWithHTML.R (3)
260-264: Good addition; tighten scalar/NA handling and consider allowlisting schemes.The NA/"NA" normalization and escaping look right. Since
evidence_linkhere is scalar, prefer short-circuit evaluation to avoid any vectorization surprises, and optionally normalize whitespace. Also consider (optional) validating that links are http(s) before serializing.- evidence_link <- ifelse(is.na(evidence_link) | evidence_link == "NA", "", evidence_link) + evidence_link <- if (isTRUE(is.na(evidence_link) || identical(evidence_link, "NA"))) "" else evidence_link + evidence_link <- trimws(evidence_link)Would you like this PR to enforce http/https only (and optionally an INDRA domain allowlist), or should we defer that to click-time checks?
866-869: Wire up the evidence link on edge tap (opens in new tab).Optional, but completes the feature so users can click through directly.
cy.on("tap", "edge", function(evt) { var edge = evt.target; - console.log("Edge clicked:", edge.data()); + console.log("Edge clicked:", edge.data()); + var link = edge.data("evidenceLink"); + if (link) { + try { + // Optional: basic scheme check + var u = new URL(link); + if (u.protocol === "http:" || u.protocol === "https:") { + window.open(u.href, "_blank", "noopener,noreferrer"); + } + } catch (_) { /* ignore invalid URLs */ } + } });If you prefer not to change default behavior, we can expose this as a default
edge_clickhandler that users can override.
121-149: Potential loss of reverse-edge evidence when consolidating.When consolidating bidirectional/undirected edges, only the first direction’s extra columns are copied. If each direction carries distinct
evidenceLinks, one will be dropped.If needed, merge both directions’ links (e.g., deduped, semicolon-delimited, or as a JSON array) during consolidation.
Also applies to: 114-119
📜 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(2 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
| # Helper to safely embed strings into JS single-quoted literals | ||
| escape_js_string <- function(x) { | ||
| if (is.null(x)) return("") | ||
| x <- as.character(x) | ||
| x <- gsub("\\\\", "\\\\\\\\", x) # backslashes | ||
| x <- gsub("'", "\\\\'", x) # single quotes | ||
| x <- gsub("\r", "\\\\r", x) | ||
| x <- gsub("\n", "\\\\n", x) | ||
| x | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden escaping against </script> termination and JS line-separator chars.
Current escaping doesn’t neutralize </script> or U+2028/U+2029, which can prematurely terminate the script tag or break JS parsing. Add these transformations.
escape_js_string <- function(x) {
if (is.null(x)) return("")
x <- as.character(x)
x <- gsub("\\\\", "\\\\\\\\", x) # backslashes
x <- gsub("'", "\\\\'", x) # single quotes
x <- gsub("\r", "\\\\r", x)
x <- gsub("\n", "\\\\n", x)
+ # Prevent </script> tag breakouts and line-separator issues
+ x <- gsub("</", "<\\\\/", x, fixed = TRUE)
+ x <- gsub(intToUtf8(0x2028), "\\\\u2028", x, fixed = TRUE)
+ x <- gsub(intToUtf8(0x2029), "\\\\u2029", x, fixed = TRUE)
x
}📝 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.
| # Helper to safely embed strings into JS single-quoted literals | |
| escape_js_string <- function(x) { | |
| if (is.null(x)) return("") | |
| x <- as.character(x) | |
| x <- gsub("\\\\", "\\\\\\\\", x) # backslashes | |
| x <- gsub("'", "\\\\'", x) # single quotes | |
| x <- gsub("\r", "\\\\r", x) | |
| x <- gsub("\n", "\\\\n", x) | |
| x | |
| } | |
| # Helper to safely embed strings into JS single-quoted literals | |
| escape_js_string <- function(x) { | |
| if (is.null(x)) return("") | |
| x <- as.character(x) | |
| x <- gsub("\\\\", "\\\\\\\\", x) # backslashes | |
| x <- gsub("'", "\\\\'", x) # single quotes | |
| x <- gsub("\r", "\\\\r", x) | |
| x <- gsub("\n", "\\\\n", x) | |
| # Prevent </script> tag breakouts and line-separator issues | |
| x <- gsub("</", "<\\\\/", x, fixed = TRUE) | |
| x <- gsub(intToUtf8(0x2028), "\\\\u2028", x, fixed = TRUE) | |
| x <- gsub(intToUtf8(0x2029), "\\\\u2029", x, fixed = TRUE) | |
| x | |
| } |
🤖 Prompt for AI Agents
In R/visualizeNetworksWithHTML.R around lines 284-294, the escape_js_string
helper must also neutralize occurrences of "</script>" and JS line-separator
characters U+2028 and U+2029; update the function to (after current
backslash/single-quote and CR/LF escapes) replace any case of "</script>" with
"<\\/script>" (or otherwise break the closing tag) and replace U+2028 and U+2029
characters with their escaped Unicode forms "\\u2028" and "\\u2029" so the
returned string cannot prematurely terminate the script tag or break JS parsing.
2 participants
User description
Motivation and Context
Please include relevant motivation and context of the problem along with a short summary of the solution.
Changes
Please provide a detailed bullet point list of your changes.
Testing
Please describe any unit tests you added or modified to verify your changes.
Checklist Before Requesting a Review
PR Type
Enhancement
Description
Add
evidenceLinkto edge dataEnable opening INDRA evidence pages
Diagram Walkthrough
File Walkthrough
visualizeNetworksWithHTML.R
Add evidenceLink field to edge JSON payloadR/visualizeNetworksWithHTML.R
evidenceLink.row$evidenceLinkinto Cytoscape data.Summary by CodeRabbit