feature(visualizeNetworkWithHTML): add edge clicking to URL for regular HTML #63
Conversation
WalkthroughAdds a JS helper to safely open http(s) links and an internal R factory Changes
Sequence DiagramsequenceDiagram
participant User
participant HTML as "Exported HTML (Cytoscape)"
participant R as "R createEdgeClickHandler"
participant JS as "openLinkInNewTab()"
User->>HTML: Click edge
HTML->>R: emit edge_click with edge data (includes evidenceLink, stmt_hash)
R->>R: edge_click handler reads evidenceLink
R->>JS: call openLinkInNewTab(evidenceLink)
JS->>JS: validate URL (http/https) and try window.open
JS-->>User: new tab opened or error logged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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)
🔇 Additional comments (1)
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 |
|
Failed to generate code suggestions for PR |
![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
🧹 Nitpick comments (1)
R/visualizeNetworksWithHTML.R (1)
1087-1090: Optional: Consider whether debug handler is still needed.This generic edge click handler (logging to console) will fire alongside the new link-opening handler from the configuration. Multiple handlers on the same event is valid in Cytoscape.js, but if the console logging is only for debugging, you might consider removing or conditionalizing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 (4)
R/visualizeNetworksWithHTML.R (4)
832-848: Well-implemented security for opening external links.The
openLinkInNewTabfunction includes proper safeguards:
- Protocol restriction to HTTP(S) only prevents javascript:, data:, and file: URI attacks
noopener,noreferrerattributes and explicitwin.opener = nullprevent tab-nabbing- Input validation filters out empty, NA, and non-string values
- Error handling catches exceptions gracefully
1122-1133: LGTM - Clean event handler implementation.The edge click handler properly retrieves the
evidenceLinkfrom edge data and delegates to the security-validatedopenLinkInNewTabfunction. The validation checks here overlap slightly with those inopenLinkInNewTab, but this defensive approach is reasonable.
1153-1162: Consistent integration of edge click handlers.The modifications properly create and pass event handlers to the configuration pipeline. The approach is consistent with the pattern used in
previewNetworkInBrowser.
1182-1191: LGTM - Parallel implementation for preview function.The changes mirror the pattern in
exportNetworkToHTML, ensuring both export and preview paths support edge click functionality consistently.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #63 +/- ##
==========================================
- Coverage 59.69% 57.90% -1.80%
==========================================
Files 7 7
Lines 1315 1354 +39
==========================================
- Hits 785 784 -1
- Misses 530 570 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
R/utils_getSubnetworkFromIndra.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
🔇 Additional comments (1)
R/utils_getSubnetworkFromIndra.R (1)
303-305: The original review comment is based on incorrect assumptions about stmt_hash usage.The code adds
stmt_hashto the edges data.frame (lines 303-305), but this field is never actually used in the visualization layer. Edge clicking to URLs is already implemented using theevidenceLinkcolumn, notstmt_hash. Thestmt_hashcolumn is only used internally for INDRA curation API filtering (in.get_incorrect_curation_count()). It does not contribute to the "edge clicking" feature mentioned in the review.Likely an incorrect or invalid review comment.
R/utils_getSubnetworkFromIndra.R
Outdated
| edge <- .addAdditionalMetadataToIndraEdge(edge, input) | ||
| edge$data$paper_count <- 1 # TODO: fix paper count | ||
| if (!key %in% keys(edgeToMetadataMapping) || | ||
| edge$data$evidence_count > edgeToMetadataMapping[[key]]$data$evidence_count) { | ||
| edgeToMetadataMapping[[key]] <- edge | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the behavioral change from accumulating to replacing duplicate edges.
The logic has changed from accumulating evidence across duplicate edges to keeping only the edge with the highest evidence_count. This is a significant behavioral change that:
- Discards potentially valuable metadata from lower-evidence duplicates
- Calls
.addAdditionalMetadataToIndraEdge()for every edge, even those that will be discarded (performance concern) - Has no tie-breaking logic when
evidence_countvalues are equal
Please clarify:
- Is this behavioral change intentional for the URL-clicking feature?
- Should evidence be accumulated across duplicates instead of replaced?
- Can
.addAdditionalMetadataToIndraEdge()be moved after the condition to avoid enriching edges that will be discarded?
Run the following script to check if this function is tested and whether existing tests catch this behavioral change:
🏁 Script executed:
#!/bin/bash
# Description: Search for tests of .collapseDuplicateEdgesIntoEdgeToMetadataMapping and usage patterns
# Find test files
echo "=== Test files for this function ==="
rg -l "collapseDuplicateEdgesIntoEdgeToMetadataMapping" --type=r
# Check for test expectations around evidence_count accumulation
echo -e "\n=== Test expectations for evidence_count ==="
rg -A 5 -B 5 "evidence_count.*collapseDuplicate|collapseDuplicate.*evidence_count" --type=r
# Look for documentation about duplicate edge handling
echo -e "\n=== Documentation about duplicate edges ==="
rg -i "duplicate.*edge|collapse.*edge" --type=r -g "*.Rd" -g "*.md"Length of output: 160
I need to dig deeper into the codebase to understand the function implementation and its context.
Let me search directly in the codebase for the function implementation and context:
🏁 Script executed:
#!/bin/bash
# Description: Find function definition and surrounding context
echo "=== Finding collapseDuplicateEdgesIntoEdgeToMetadataMapping function definition ==="
rg -n "collapseDuplicateEdgesIntoEdgeToMetadataMapping" -A 30 -B 5 R/utils_getSubnetworkFromIndra.R --type=r
echo -e "\n=== Looking at the structure around line 256 ==="
sed -n '240,280p' R/utils_getSubnetworkFromIndra.RLength of output: 282
🏁 Script executed:
#!/bin/bash
# Description: Find function definition and surrounding context in cloned repository
echo "=== Finding file ==="
find . -name "utils_getSubnetworkFromIndra.R" -type f
echo -e "\n=== Finding collapseDuplicateEdgesIntoEdgeToMetadataMapping function definition ==="
rg -n "collapseDuplicateEdgesIntoEdgeToMetadataMapping" -A 30 -B 5 --type=r
echo -e "\n=== Looking at the structure around line 256 for better context ==="
rg -n "\.addAdditionalMetadataToIndraEdge" -A 10 -B 10 --type=r | head -100Length of output: 8449
🏁 Script executed:
#!/bin/bash
# Description: Check git history for this function and evidence handling
echo "=== Check git history for collapseDuplicateEdgesIntoEdgeToMetadataMapping ==="
git log -p --all -S "collapseDuplicateEdgesIntoEdgeToMetadataMapping" -- "R/utils_getSubnetworkFromIndra.R" | head -150
echo -e "\n=== Check git log to see recent changes ==="
git log --oneline -20 -- "R/utils_getSubnetworkFromIndra.R"
echo -e "\n=== Search for comments about evidence accumulation ==="
rg -i "accumulat|aggregate|combine.*evidence" -A 2 -B 2 --type=rLength of output: 2937
🏁 Script executed:
#!/bin/bash
# Description: Check the actual diff of the commit that modified evidence_count logic
echo "=== Show commit 14c4b19 diff ==="
git show 14c4b19 -- "R/utils_getSubnetworkFromIndra.R" | head -200
echo -e "\n=== Show the commit message and metadata ==="
git show --no-patch --format=fuller 14c4b19
echo -e "\n=== Check what changed in collapseDuplicateEdgesIntoEdgeToMetadataMapping specifically ==="
git show 14c4b19:R/utils_getSubnetworkFromIndra.R 2>/dev/null | sed -n '240,270p'Length of output: 2974
Behavioral change from evidence accumulation to replacement requires fixes and documentation.
The git history confirms this change was intentional (commit 14c4b19: "modify which statement is captured based on max evidence count"), but there are critical issues:
-
Evidence accumulation removed: Previously duplicates had
evidence_countandpaper_countaccumulated (+=); now only the edge with highestevidence_countis kept. This loses accumulated evidence semantics unless each INDRA edge already represents combined evidence. -
Metadata enrichment inefficiency:
.addAdditionalMetadataToIndraEdge()is now called unconditionally for all edges (line 256), but only edges passing theevidence_countcheck are kept (line 260). This performs unnecessary work on discarded edges. -
Broken paper_count: Previously accumulated actual paper count; now hardcoded to 1 (line 257) with a TODO comment, indicating incomplete implementation.
-
Missing tie-breaker: When
evidence_countvalues are equal, the first edge encountered wins by default. Consider using>=or adding a secondary sort criterion.
Recommended fixes:
- Move
.addAdditionalMetadataToIndraEdge()inside the conditional to avoid enriching discarded edges - Document why evidence accumulation was removed and confirm INDRA edges already represent combined evidence
- Fix
paper_countto properly track unique papers or clarify its intended meaning - Add tie-breaking logic for equal
evidence_countvalues
🤖 Prompt for AI Agents
In R/utils_getSubnetworkFromIndra.R around lines 256-261, the change replaced
evidence accumulation with a replace-on-max-evidence strategy and introduced
three regressions: unnecessary unconditional metadata enrichment, hardcoded
paper_count = 1, and no tie-breaker for equal evidence_count; to fix, move the
.addAdditionalMetadataToIndraEdge(edge, input) call inside the conditional so
enrichment only occurs for the kept/updated edge, restore accumulation semantics
for evidence_count and paper_count (e.g., add existing edge$data$evidence_count
and merge unique paper identifiers to compute paper_count) unless you explicitly
document that INDRA edges are already aggregated, and implement a deterministic
tie-breaker when evidence_count is equal (for example prefer larger paper_count
or use timestamp/source priority) and add a short comment documenting the
behavioral change and rationale.
R/utils_getSubnetworkFromIndra.R
Outdated
| edge <- .addAdditionalMetadataToIndraEdge(edge, input) | ||
| edge$data$paper_count <- 1 | ||
| edge <- .addAdditionalMetadataToIndraEdge(edge, input) | ||
| edge$data$paper_count <- 1 # TODO: fix paper count |
There was a problem hiding this comment.
TODO indicates incomplete paper_count implementation.
Setting paper_count to a hardcoded value of 1 with a TODO comment suggests this feature is incomplete. This could lead to incorrect paper count metrics in the network visualization and downstream analysis.
Verify whether paper_count should be:
- Extracted from
edge$datasimilar to evidence_count - Calculated from
source_countsJSON - Accumulated across duplicate edges
Additionally, consider whether this PR should be merged with an incomplete implementation, or if the paper_count feature should be properly implemented or removed before merge.
🤖 Prompt for AI Agents
In R/utils_getSubnetworkFromIndra.R around line 257, the code sets
edge$data$paper_count <- 1 which is a placeholder; replace this with a real
computation: if edge$data already contains a paper_count field use that,
otherwise parse edge$data$source_counts (JSON) to extract and count unique paper
identifiers (or sum counts per source if the JSON stores counts), and when
collapsing/merging duplicate edges accumulate paper_count across duplicates;
remove the TODO and ensure paper_count is an integer stored in edge$data.
Summary by CodeRabbit