Merged
Conversation
📝 WalkthroughWalkthroughAdds a Cytoscape.js-based htmlwidget to MSstatsBioNet with R APIs and Shiny bindings, client-side rendering/interaction logic, dependency declarations (htmlwidgets, grDevices, shiny), documentation, and a vignette demonstrating usage and examples. Changes
Sequence Diagram(s)sequenceDiagram
participant User as R User
participant R as R (cytoscapeNetwork)
participant HW as htmlwidgets (createWidget)
participant JS as JS Binding (cytoscapeNetwork.js)
participant Cyt as Cytoscape.js
participant Browser as Browser/DOM
User->>R: Call cytoscapeNetwork(nodes, edges, ...)
R->>R: Validate inputs, build elements (nodes, PTMs, edges), compute colors/tooltips
R->>HW: createWidget(payload)
HW->>JS: Initialize widget with payload
JS->>Cyt: Initialize with elements & dagre layout
Cyt->>JS: Emit layoutstop
JS->>JS: Reposition PTMs, build legend
JS->>Browser: Render canvas and controls, register event handlers
User->>Browser: Interact (hover/click)
Browser->>JS: Event → show tooltip / open evidence link / report to Shiny
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
inst/htmlwidgets/cytoscapeNetwork.yaml (1)
13-47: Vendor widget dependencies locally for reproducible/offline execution.All five dependencies (graphlib, dagre, cytoscape, cytoscape-dagre, html2canvas) are loaded exclusively from CDNs. The htmlwidgets framework explicitly recommends bundling third-party JS/CSS under
inst/htmlwidgets/lib/to ensure the widget works offline and remains reproducible. CRAN policy also strongly prefers bundling external dependencies rather than requiring internet access at runtime.Consider vendoring these assets locally and updating the dependency manifest to reference the local copies (or providing both local and CDN paths with local as the primary source).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.yaml` around lines 13 - 47, The manifest lists external CDN-only dependencies (graphlib, dagre, cytoscape, cytoscape-dagre, html2canvas) which must be vendored for offline/reproducible use; download the corresponding min.js files into inst/htmlwidgets/lib/, update the dependency entries in cytoscapeNetwork.yaml to point src.href and the head script tags to the local lib/* filenames (or make local the primary href and keep the CDN as a fallback), and commit the vendored assets so the widget loads from inst/htmlwidgets/lib/ instead of only from the CDNs.inst/htmlwidgets/cytoscapeNetwork.js (3)
377-378: Register Cytoscape plugin once, not on every render.
cytoscape.use(cytoscapeDagre)is called insiderenderValue, meaning the plugin is re-registered on every render. While Cytoscape.js handles duplicate registrations gracefully, this is inefficient and may log warnings in the console. Consider registering once at module load or guarding with a flag.♻️ Proposed fix—register once at top of factory
factory: function (el, width, height) { // State kept between renderValue calls so we can destroy cleanly var cy = null; var tooltip = null; + var dagreRegistered = false;Then in renderValue:
/* Initialise Cytoscape */ - cytoscape.use(cytoscapeDagre); // register dagre layout + if (!dagreRegistered) { + cytoscape.use(cytoscapeDagre); // register dagre layout + dagreRegistered = true; + } cy = cytoscape({Alternatively, register at module scope before the widget definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 377 - 378, Move the cytoscape.use(cytoscapeDagre) call out of the renderValue function so the dagre plugin is registered only once; locate the renderValue implementation and remove the cytoscape.use(cytoscapeDagre) line, then add a single registration at module scope (top of the factory file) or guard it with a module-level flag (e.g., registeredDagre) so cytoscape.use(cytoscapeDagre) is invoked only once during module load/initialization rather than on every render.
468-472: Clarify or remove redundantbuildLegendcall.
buildLegendis invoked twice: once in thelayoutstophandler (line 390) withlegendPanel, and again here (line 472) searching for.cytoscape-network-legendin the parent. SincelegendPanelis already created and appended torightPanel, the second call at line 472 will find the same element (or a different external one if present).If this is intentional to support an external legend container, consider adding a comment explaining the dual-path behavior. Otherwise, this call may be redundant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 468 - 472, The second invocation of buildLegend (the call that queries el.parentNode.querySelector(".cytoscape-network-legend")) is redundant with the call made in the layoutstop handler that uses legendPanel appended to rightPanel; either remove this second call or make its intent explicit by guarding it and documenting it: if you want to support an external legend container, change the call to only run when an external legend element exists and the internal legendPanel is not already present (e.g., detect the presence of legendPanel/rightPanel vs an external legendEl), and add a short comment near the buildLegend invocation explaining the dual-path behavior (symbols to locate: buildLegend, legendPanel, rightPanel, and the layoutstop handler).
358-362: Consider adding error handling for JSON parsing.If malformed JSON is passed,
JSON.parsewill throw and prevent the widget from rendering. Wrapping in a try-catch would provide more graceful degradation.♻️ Proposed defensive parsing
/* Build combined elements array from pre-serialised strings. R passes them as an array of JSON-string fragments; we re-parse. */ - var elements = (x.elements || []).map(function (frag) { - return (typeof frag === "string") ? JSON.parse(frag) : frag; - }); + var elements = (x.elements || []).map(function (frag) { + if (typeof frag === "string") { + try { + return JSON.parse(frag); + } catch (e) { + console.warn("cytoscapeNetwork: failed to parse element", e); + return null; + } + } + return frag; + }).filter(Boolean);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 358 - 362, The current map that builds elements uses JSON.parse directly on fragments (variable elements in cytoscapeNetwork.js), which will throw on malformed JSON; update the map callback inside the elements = (x.elements || []).map(...) to wrap JSON.parse in a try-catch, log or console.warn/console.error the parse error including the offending frag and index, and return a safe fallback (e.g., the original frag or null/skip) so a single bad fragment does not break widget rendering; ensure downstream code that consumes elements can handle the fallback (filter out nulls if you choose to return null).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inst/htmlwidgets/cytoscapeNetwork.js`:
- Around line 282-290: The array used to build rightPanel.style.cssText contains
a stray empty element caused by a double comma after the "width" entry,
producing a double semicolon; locate the array assigned to
rightPanel.style.cssText (referencing rightPanel, PANEL_W and PANEL_BG) and
remove the extra comma or explicitly supply only the intended strings so the
width entry becomes "width:" + PANEL_W + "px" (single element) and the array
joins without producing an empty slot.
- Around line 64-66: The mapData() call is using the invalid expression
"label.length" which is treated as a literal property name; update the node data
to include a precomputed numeric field (e.g., label_length) in R and change the
mapping to use that field by replacing "mapData(label.length, 0, 20, 60, 150)"
with "mapData(label_length, 0, 20, 60, 150)" in
inst/htmlwidgets/cytoscapeNetwork.js so mapData receives a direct data property;
ensure the data property name matches the key you add to each node's data.
In `@R/cytoscapeNetwork.R`:
- Around line 420-425: The current input validation silently replaces invalid
edges with an empty data.frame; instead, update the validation so that if edges
is not a data.frame you stop with a clear error, and if it is a data.frame
validate that it contains the required columns "source", "target", and
"interaction" (and optionally "id" if used elsewhere) before proceeding; modify
the block that currently checks nodes and edges (the validation around variables
nodes and edges) to call stop() with a descriptive message when edges is invalid
or missing required columns rather than assigning edges <- data.frame().
In `@vignettes/Cytoscape-Visualization.Rmd`:
- Around line 141-145: Wrap the htmlwidgets::saveWidget call so it only runs
when pandoc is available: check pandoc_available() before calling
saveWidget(widget, ...); when running, write to a temporary file via tempfile()
instead of "network.html" and set selfcontained = FALSE to avoid bundling heavy
resources; keep the call using the same widget variable and ensure any
downstream references use the tempfile path if needed.
- Around line 107-135: The vignette loads shiny unconditionally causing failures
when shiny is only in Suggests; wrap the UI/server/reactive block in a check
using requireNamespace("shiny", quietly = TRUE) (or similar) so the code only
calls library(shiny), defines ui, server, and output$network
(renderCytoscapeNetwork/cytoscapeNetwork) when shiny is available, and otherwise
skip or emit a brief message so the vignette build won’t error; ensure the
shinyApp(...) launch line remains commented or is also guarded.
---
Nitpick comments:
In `@inst/htmlwidgets/cytoscapeNetwork.js`:
- Around line 377-378: Move the cytoscape.use(cytoscapeDagre) call out of the
renderValue function so the dagre plugin is registered only once; locate the
renderValue implementation and remove the cytoscape.use(cytoscapeDagre) line,
then add a single registration at module scope (top of the factory file) or
guard it with a module-level flag (e.g., registeredDagre) so
cytoscape.use(cytoscapeDagre) is invoked only once during module
load/initialization rather than on every render.
- Around line 468-472: The second invocation of buildLegend (the call that
queries el.parentNode.querySelector(".cytoscape-network-legend")) is redundant
with the call made in the layoutstop handler that uses legendPanel appended to
rightPanel; either remove this second call or make its intent explicit by
guarding it and documenting it: if you want to support an external legend
container, change the call to only run when an external legend element exists
and the internal legendPanel is not already present (e.g., detect the presence
of legendPanel/rightPanel vs an external legendEl), and add a short comment near
the buildLegend invocation explaining the dual-path behavior (symbols to locate:
buildLegend, legendPanel, rightPanel, and the layoutstop handler).
- Around line 358-362: The current map that builds elements uses JSON.parse
directly on fragments (variable elements in cytoscapeNetwork.js), which will
throw on malformed JSON; update the map callback inside the elements =
(x.elements || []).map(...) to wrap JSON.parse in a try-catch, log or
console.warn/console.error the parse error including the offending frag and
index, and return a safe fallback (e.g., the original frag or null/skip) so a
single bad fragment does not break widget rendering; ensure downstream code that
consumes elements can handle the fallback (filter out nulls if you choose to
return null).
In `@inst/htmlwidgets/cytoscapeNetwork.yaml`:
- Around line 13-47: The manifest lists external CDN-only dependencies
(graphlib, dagre, cytoscape, cytoscape-dagre, html2canvas) which must be
vendored for offline/reproducible use; download the corresponding min.js files
into inst/htmlwidgets/lib/, update the dependency entries in
cytoscapeNetwork.yaml to point src.href and the head script tags to the local
lib/* filenames (or make local the primary href and keep the CDN as a fallback),
and commit the vendored assets so the widget loads from inst/htmlwidgets/lib/
instead of only from the CDNs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
DESCRIPTIONNAMESPACER/cytoscapeNetwork.Rinst/htmlwidgets/cytoscapeNetwork.jsinst/htmlwidgets/cytoscapeNetwork.yamlman/cytoscapeNetwork.Rdman/cytoscapeNetworkOutput.Rdman/renderCytoscapeNetwork.Rdvignettes/Cytoscape-Visualization.Rmd
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #72 +/- ##
==========================================
- Coverage 59.91% 50.32% -9.59%
==========================================
Files 7 8 +1
Lines 1559 1856 +297
==========================================
Hits 934 934
- Misses 625 922 +297 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
R/cytoscapeNetwork.R (1)
250-357: Consider avoiding repeatedc(elements, list(...))in tight loops.This pattern reallocates repeatedly and can degrade performance on large networks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/cytoscapeNetwork.R` around lines 250 - 357, The code repeatedly grows the elements list inside the node and edge loops (variables elements, emitted_prots, emitted_cpds, emitted_ptm_n, emitted_ptm_e), causing repeated reallocations; change the accumulation to use indexed assignment (e.g., maintain a single integer counter like elem_idx and do elements[[elem_idx]] <- <item>; elem_idx <- elem_idx + 1) or collect node/ptm/edge items into separate preallocated lists and concatenate once after the loops (references: the for-loops that build elements in cytoscapeNetwork.R, the variables elements/emitted_* and the edge loop that uses .consolidateEdges and .edgeStyle), preserving the exact list structure and emitted_* checks but avoiding c(elements, list(...)) inside tight loops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inst/htmlwidgets/cytoscapeNetwork.js`:
- Around line 262-277: renderValue() currently hard-sets pixel widths/heights
(el.style.cssText and cyContainer.style.cssText) and resize() never reapplies
newWidth/newHeight, causing stale/truncated layouts on container resize; update
renderValue() to avoid locking to initial px (use flex + 100% height/width for
el and cyContainer or compute sizes dynamically) and modify
resize(newWidth,newHeight) to set el.style.cssText and cyContainer.style.cssText
using the incoming newWidth/newHeight (recompute width as (newWidth - PANEL_W)px
if PANEL_W applies), then call the Cytoscape instance's resize() and rerun
layout/fit (e.g., cy.resize(); cy.layout(...).run(); or cy.fit()) so the canvas
reflows correctly; refer to renderValue, resize, cyContainer, el, PANEL_W, and
the Cytoscape instance (cy) when making these changes.
- Around line 360-362: The JSON.parse call inside the elements mapping can throw
on malformed fragments and break rendering; update the map over (x.elements ||
[]) in cytoscapeNetwork.js to guard parsing by wrapping the parse in a try/catch
inside the map callback (the function that handles frag), returning the original
frag (or null/skip) when parse fails and optionally console.warn or
console.error the problematic frag and error; ensure downstream code handles any
nulls/invalid entries (filter them out) so a single bad fragment cannot crash
the widget.
- Around line 451-455: The node tap handler registered with cy.on("tap", "node",
function (evt) { ... }) only filters out compound nodes but the intent is to
also skip PTM nodes; update the handler to return early when
node.data("node_type") indicates a PTM (e.g. check for "ptm" or the project's
PTM node_type value) so taps on PTM nodes do not trigger the Shiny/_node_clicked
emission; modify the condition in the cy tap callback that currently checks
node.data("node_type") === "compound" to also include the PTM type before
emitting the _node_clicked event.
In `@R/cytoscapeNetwork.R`:
- Around line 423-425: The current validation only ensures nodes is a data.frame
with an id column but does not assert that nodes$id values are non-missing,
non-empty, and unique; add checks in the same validation block (the one that
currently tests is.data.frame(nodes) and "id" %in% names(nodes)) to (1) ensure
no NA or empty string values in nodes$id and (2) ensure all(nodes$id) are
unique, and if either check fails call stop() with a clear error message; this
prevents downstream silent drops in .buildElements that use emitted_prots to
deduplicate IDs.
- Around line 91-99: The .classify function can hit if(NA) when interaction is
missing; update the check inside the loop to guard against NA before using %in%
by testing interaction for missingness (e.g. with is.na(interaction)) and only
evaluating interaction %in% props[[cat_name]]$types when interaction is not NA
and props[[cat_name]]$types is present; keep the existing return(cat_name)
behavior and fallback "other" result. Use the .classify function name and the
interaction and props[[cat_name]]$types symbols to locate the change.
---
Nitpick comments:
In `@R/cytoscapeNetwork.R`:
- Around line 250-357: The code repeatedly grows the elements list inside the
node and edge loops (variables elements, emitted_prots, emitted_cpds,
emitted_ptm_n, emitted_ptm_e), causing repeated reallocations; change the
accumulation to use indexed assignment (e.g., maintain a single integer counter
like elem_idx and do elements[[elem_idx]] <- <item>; elem_idx <- elem_idx + 1)
or collect node/ptm/edge items into separate preallocated lists and concatenate
once after the loops (references: the for-loops that build elements in
cytoscapeNetwork.R, the variables elements/emitted_* and the edge loop that uses
.consolidateEdges and .edgeStyle), preserving the exact list structure and
emitted_* checks but avoiding c(elements, list(...)) inside tight loops.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
R/cytoscapeNetwork.Rinst/htmlwidgets/cytoscapeNetwork.jsvignettes/Cytoscape-Visualization.Rmd
🚧 Files skipped from review as they are similar to previous changes (1)
- vignettes/Cytoscape-Visualization.Rmd
There was a problem hiding this comment.
♻️ Duplicate comments (3)
inst/htmlwidgets/cytoscapeNetwork.js (3)
358-360:⚠️ Potential issue | 🟠 MajorGuard
JSON.parseto prevent render failures on malformed input.If
x.elementscontains a malformed JSON string, the unguardedJSON.parsewill throw and crash the entire widget render. This is still unaddressed from a previous review.🛡️ Proposed fix
- var elements = (x.elements || []).map(function (frag) { - return (typeof frag === "string") ? JSON.parse(frag) : frag; - }); + var elements = []; + (x.elements || []).forEach(function (frag) { + if (typeof frag === "string") { + try { + elements.push(JSON.parse(frag)); + } catch (err) { + console.warn("cytoscapeNetwork: skipping invalid JSON fragment", err); + } + } else if (frag && typeof frag === "object") { + elements.push(frag); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 358 - 360, The mapping over x.elements uses an unguarded JSON.parse which will throw on malformed strings and break render; update the map logic in the elements creation (the anonymous map callback that references x.elements and JSON.parse) to safely parse by wrapping JSON.parse in a try/catch or delegating to a small safeParse helper that returns the parsed object on success and falls back to the original frag (or null) on failure, then filter out null/invalid entries so malformed JSON does not crash the widget render.
473-479:⚠️ Potential issue | 🟠 MajorResize handler doesn't apply new dimensions to the container.
The
resizemethod receivesnewWidthandnewHeightbut never applies them toelorcyContainer. SincerenderValuesets fixed pixel heights (line 264), the widget won't properly reflow when its container resizes. This was noted in a previous review.💡 Proposed fix
resize: function (newWidth, newHeight) { if (cy) { + if (newHeight) { + el.style.height = newHeight + "px"; + var cyContainer = el.querySelector("div:first-child"); + if (cyContainer) cyContainer.style.height = newHeight + "px"; + } cy.resize(); cy.fit(); } }Alternatively, use percentage-based heights in
renderValueso the container naturally reflows:- "height:" + elH + "px", + "height:100%",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 473 - 479, The resize handler currently ignores its newWidth/newHeight params; update the resize function to set the container size (e.g., assign newWidth/newHeight as CSS width/height on el and/or cyContainer, using pixels or appropriate units) before calling cy.resize() and cy.fit(); also ensure this complements any sizing logic in renderValue (which currently sets fixed pixel heights) so the container actually reflows when resized.
449-461:⚠️ Potential issue | 🟡 MinorComment-code mismatch: PTM nodes are not actually skipped.
The comment at line 451 says "skip compound and ptm satellite nodes" but only
compoundis filtered at line 452. This was flagged in a previous review. Either add the PTM check to match the intent, or correct the comment if PTM clicks should be reported.🐛 Proposed fix (if PTM should be skipped)
cy.on("tap", "node", function (evt) { var node = evt.target; // skip compound and ptm satellite nodes - if (node.data("node_type") === "compound") return; + if (node.data("node_type") === "compound" || + node.data("node_type") === "ptm") return; if (window.Shiny) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 449 - 461, The code claims to skip "compound and ptm satellite nodes" but only checks for compound; update the cy.on("tap", "node", ...) handler to also return early for PTM satellite nodes by checking node.data("node_type") (e.g., add a condition like node.data("node_type") === "ptm" or === "ptm_satellite" alongside the existing compound check) before the Shiny.setInputValue call, or alternatively update the comment to remove the PTM reference if PTM clicks should be reported.
🧹 Nitpick comments (2)
inst/htmlwidgets/cytoscapeNetwork.js (2)
376-376: Consider registering the dagre extension once, outside the render loop.
cytoscape.use(cytoscapeDagre)is called on everyrenderValueinvocation. While Cytoscape.js handles duplicate registration gracefully, this is unnecessarily repeated work.♻️ Proposed fix
+ // Register dagre extension once at module load + var dagreRegistered = false; + /* ── factory ──────────────────────────────────────────────────────────── */ factory: function (el, width, height) { // ... return { renderValue: function (x) { // ... - cytoscape.use(cytoscapeDagre); // register dagre layout + if (!dagreRegistered) { + cytoscape.use(cytoscapeDagre); + dagreRegistered = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` at line 376, Move the cytoscapeDagre registration out of the per-render code: remove the call to cytoscape.use(cytoscapeDagre) from inside renderValue and instead register the extension once at module initialization (e.g., at top-level when cytoscape and cytoscapeDagre are first imported/required). Ensure renderValue still assumes dagre is registered but does not call cytoscape.use(cytoscapeDagre) itself, leaving function names cytoscape.use, cytoscapeDagre and renderValue as the reference points to change.
314-334: Consider adding user feedback during PNG export.The export button triggers two downloads with a 300ms delay between them, but provides no visual indication that the export is in progress. For large networks,
cy.png({ scale: 8 })can be slow and memory-intensive.💡 Suggested improvement
btn.addEventListener("click", function () { + btn.disabled = true; + btn.textContent = "Exporting..."; /* Network PNG via Cytoscape */ var networkPng = cy.png({ output: "base64uri", bg: "white", full: true, scale: 8 }); // ... download logic ... setTimeout(function () { if (typeof html2canvas === "function") { html2canvas(legendPanel, { backgroundColor: PANEL_BG, scale: 8 }) .then(function (canvas) { // ... download logic ... + }).finally(function () { + btn.disabled = false; + btn.textContent = "Export PNG"; }); + } else { + btn.disabled = false; + btn.textContent = "Export PNG"; } }, 300); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inst/htmlwidgets/cytoscapeNetwork.js` around lines 314 - 334, Wrap the export flow in a user-feedback sequence: when the export button (btn) click handler starts, disable btn and update its text (or add a spinner) so users see "Exporting…" before calling cy.png({ scale: 8, ... }) and starting the html2canvas flow for legendPanel; ensure the UI update takes effect by yielding to the event loop (e.g., schedule the heavy work after a short setTimeout or Promise.resolve) and then re-enable btn and restore its label after both the cy.png download and the html2canvas promise resolve or if either throws an error, handling errors by showing a brief status (or console.log) and re-enabling the button; keep the existing setTimeout(…,300) logic but integrate the disable/enable and status updates around cy.png, html2canvas, and the downloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@inst/htmlwidgets/cytoscapeNetwork.js`:
- Around line 358-360: The mapping over x.elements uses an unguarded JSON.parse
which will throw on malformed strings and break render; update the map logic in
the elements creation (the anonymous map callback that references x.elements and
JSON.parse) to safely parse by wrapping JSON.parse in a try/catch or delegating
to a small safeParse helper that returns the parsed object on success and falls
back to the original frag (or null) on failure, then filter out null/invalid
entries so malformed JSON does not crash the widget render.
- Around line 473-479: The resize handler currently ignores its
newWidth/newHeight params; update the resize function to set the container size
(e.g., assign newWidth/newHeight as CSS width/height on el and/or cyContainer,
using pixels or appropriate units) before calling cy.resize() and cy.fit(); also
ensure this complements any sizing logic in renderValue (which currently sets
fixed pixel heights) so the container actually reflows when resized.
- Around line 449-461: The code claims to skip "compound and ptm satellite
nodes" but only checks for compound; update the cy.on("tap", "node", ...)
handler to also return early for PTM satellite nodes by checking
node.data("node_type") (e.g., add a condition like node.data("node_type") ===
"ptm" or === "ptm_satellite" alongside the existing compound check) before the
Shiny.setInputValue call, or alternatively update the comment to remove the PTM
reference if PTM clicks should be reported.
---
Nitpick comments:
In `@inst/htmlwidgets/cytoscapeNetwork.js`:
- Line 376: Move the cytoscapeDagre registration out of the per-render code:
remove the call to cytoscape.use(cytoscapeDagre) from inside renderValue and
instead register the extension once at module initialization (e.g., at top-level
when cytoscape and cytoscapeDagre are first imported/required). Ensure
renderValue still assumes dagre is registered but does not call
cytoscape.use(cytoscapeDagre) itself, leaving function names cytoscape.use,
cytoscapeDagre and renderValue as the reference points to change.
- Around line 314-334: Wrap the export flow in a user-feedback sequence: when
the export button (btn) click handler starts, disable btn and update its text
(or add a spinner) so users see "Exporting…" before calling cy.png({ scale: 8,
... }) and starting the html2canvas flow for legendPanel; ensure the UI update
takes effect by yielding to the event loop (e.g., schedule the heavy work after
a short setTimeout or Promise.resolve) and then re-enable btn and restore its
label after both the cy.png download and the html2canvas promise resolve or if
either throws an error, handling errors by showing a brief status (or
console.log) and re-enabling the button; keep the existing setTimeout(…,300)
logic but integrate the disable/enable and status updates around cy.png,
html2canvas, and the downloads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
There's a lot of javascript embedded as strings in the visualization module, and I realized visualization can be upgraded by placing these JS embeddings into JS files, and then performing logic in R files. This makes extensibility way easier, and now, we can have a widget that allows users to download PNGs, display legends, all in one spot.
In a future PR, I will do some clean up, unit testing, and dependency handling
Motivation and Context
This PR adds a full Cytoscape.js-based htmlwidget to MSstatsBioNet to provide interactive network visualizations (proteins, interactions, PTMs, compounds). The widget supports log-fold-change coloring, PTM satellite nodes, dagre layout configuration, Shiny bindings, PNG export, and a client-side legend. The intent is to refactor/centralize visualization functionality into a reusable htmlwidget.
Short solution summary
Introduce a new htmlwidget ("cytoscapeNetwork") implemented in R and JavaScript, wire up Shiny output/render bindings, declare required package and JS dependencies, add documentation and a vignette with usage examples.
Detailed changes
DESCRIPTION
NAMESPACE
R implementation (R/cytoscapeNetwork.R)
JavaScript widget (inst/htmlwidgets/cytoscapeNetwork.js)
JS manifest (inst/htmlwidgets/cytoscapeNetwork.yaml)
Documentation
Size/lines
Unit tests
Coding guidelines / issues violated