refactor exportNetworktoHTML and previewNetwork functions#73
refactor exportNetworktoHTML and previewNetwork functions#73tonywu1999 wants to merge 1 commit intodevelfrom
Conversation
📝 WalkthroughWalkthroughThis pull request removes the public export of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/visualizeNetworksWithHTML.R (2)
39-39:⚠️ Potential issue | 🟡 MinorSame stale documentation and unused
...parameter.Consistent with
exportNetworkToHTML, the...parameter is documented as being passed toexportCytoscapeToHTML()(which was removed), and is not passed to theexportNetworkToHTML()call on lines 49-52.🔧 Proposed fix to remove unused parameter
-#' `@param` ... Additional arguments passed to exportCytoscapeToHTML() previewNetworkInBrowser <- function(nodes, edges, displayLabelType = "id", - nodeFontSize = 12, - ...) { + nodeFontSize = 12) {Also applies to: 43-43, 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` at line 39, Remove the stale ... parameter and its roxygen `@param` documentation from the visualizeNetworksWithHTML.R function (the documented reference to exportCytoscapeToHTML which no longer exists), and ensure calls to exportNetworkToHTML(...) do not expect or forward ...; update the function signature to drop ..., remove the corresponding `@param` ... doc entry, and clean any internal references that attempted to pass ... to exportNetworkToHTML so the function and its docs match the actual call pattern.
11-11:⚠️ Potential issue | 🟡 MinorStale documentation and unused
...parameter.The
@param ...documentation referencesexportCytoscapeToHTML()which was removed in this refactor. The...parameter is declared but never passed to any function call (cytoscapeNetworkorhtmlwidgets::saveWidget).Either remove the
...parameter entirely or pass it to the underlying functions if additional arguments should be supported.🔧 Proposed fix to remove unused parameter
-#' `@param` ... Additional arguments passed to exportCytoscapeToHTML() #' `@export` #' `@return` Invisibly returns the file path of the created HTML file exportNetworkToHTML <- function(nodes, edges, filename = "network_visualization.html", displayLabelType = "id", - nodeFontSize = 12, - ...) { + nodeFontSize = 12) {Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` at line 11, The roxygen `@param` ... and the unused dots parameter in visualizeNetworksWithHTML are stale (exportCytoscapeToHTML was removed) and not forwarded to cytoscapeNetwork or htmlwidgets::saveWidget; remove the ... parameter from the visualizeNetworksWithHTML function signature and delete its `@param` ... doc entry (and any other references to it) so the docs and signature match actual usage, or alternatively explicitly forward ... to cytoscapeNetwork and/or htmlwidgets::saveWidget if you want to support extra args (update the roxygen accordingly); locate the function visualizeNetworksWithHTML and the related calls to cytoscapeNetwork and htmlwidgets::saveWidget to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/visualizeNetworksWithHTML.R`:
- Around line 20-28: The function visualizeNetworksWithHTML is missing the
promised invisible return of the created file path and still declares an unused
... parameter; update the function to return(invisible(filename)) after calling
htmlwidgets::saveWidget (use the existing widget and filename symbols) so
callers receive the file path, and remove or repurpose the ... parameter (and
update previewNetworkInBrowser similarly) so that ... is either deleted from the
function signature and docs or wired to actual downstream calls if additional
args are needed; adjust the roxygen `@return` and `@param` ... docs to match the
final behavior.
---
Outside diff comments:
In `@R/visualizeNetworksWithHTML.R`:
- Line 39: Remove the stale ... parameter and its roxygen `@param` documentation
from the visualizeNetworksWithHTML.R function (the documented reference to
exportCytoscapeToHTML which no longer exists), and ensure calls to
exportNetworkToHTML(...) do not expect or forward ...; update the function
signature to drop ..., remove the corresponding `@param` ... doc entry, and clean
any internal references that attempted to pass ... to exportNetworkToHTML so the
function and its docs match the actual call pattern.
- Line 11: The roxygen `@param` ... and the unused dots parameter in
visualizeNetworksWithHTML are stale (exportCytoscapeToHTML was removed) and not
forwarded to cytoscapeNetwork or htmlwidgets::saveWidget; remove the ...
parameter from the visualizeNetworksWithHTML function signature and delete its
`@param` ... doc entry (and any other references to it) so the docs and signature
match actual usage, or alternatively explicitly forward ... to cytoscapeNetwork
and/or htmlwidgets::saveWidget if you want to support extra args (update the
roxygen accordingly); locate the function visualizeNetworksWithHTML and the
related calls to cytoscapeNetwork and htmlwidgets::saveWidget to apply the
change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
NAMESPACER/visualizeNetworksWithHTML.Rman/generateCytoscapeConfig.Rdman/generateJavaScriptCode.Rd
💤 Files with no reviewable changes (3)
- NAMESPACE
- man/generateJavaScriptCode.Rd
- man/generateCytoscapeConfig.Rd
| widget <- cytoscapeNetwork(nodes, edges, | ||
| displayLabelType = displayLabelType, | ||
| nodeFontSize = nodeFontSize) | ||
|
|
||
| # Generate configuration | ||
| config <- generateCytoscapeConfig( | ||
| nodes, | ||
| edges, | ||
| display_label_type = displayLabelType, | ||
| node_font_size = nodeFontSize, | ||
| event_handlers = event_handlers | ||
| htmlwidgets::saveWidget( | ||
| widget, | ||
| file = filename, | ||
| selfcontained = TRUE | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n R/visualizeNetworksWithHTML.RRepository: Vitek-Lab/MSstatsBioNet
Length of output: 2773
🏁 Script executed:
rg -n "cytoscapeNetwork\s*<-\s*function" --type rRepository: Vitek-Lab/MSstatsBioNet
Length of output: 127
🏁 Script executed:
rg -n "cytoscapeNetwork" --type r -A 3 -B 1Repository: Vitek-Lab/MSstatsBioNet
Length of output: 6004
Missing return statement breaks documented behavior.
The @return documentation (line 13) states the function "Invisibly returns the file path of the created HTML file", but the function body has no return statement. Any caller relying on this return value will receive NULL.
Additionally, the ... parameter (line 18) is documented as "Additional arguments passed to exportCytoscapeToHTML()", but exportCytoscapeToHTML() was removed in this refactor, and the ... is never used in the function body. This parameter should either be removed or documented with its actual purpose.
Note: The same ... documentation issue exists in previewNetworkInBrowser (line 39).
🐛 Proposed fix to add return statement
htmlwidgets::saveWidget(
widget,
file = filename,
selfcontained = TRUE
)
+
+ invisible(filename)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/visualizeNetworksWithHTML.R` around lines 20 - 28, The function
visualizeNetworksWithHTML is missing the promised invisible return of the
created file path and still declares an unused ... parameter; update the
function to return(invisible(filename)) after calling htmlwidgets::saveWidget
(use the existing widget and filename symbols) so callers receive the file path,
and remove or repurpose the ... parameter (and update previewNetworkInBrowser
similarly) so that ... is either deleted from the function signature and docs or
wired to actual downstream calls if additional args are needed; adjust the
roxygen `@return` and `@param` ... docs to match the final behavior.
1 participant
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
Motivation and Context
This PR refactors the visualization export pipeline for network diagrams in the MSstatsBioNet package. Previously, the code maintained a complex, custom HTML generation workflow that built Cytoscape configurations, generated JavaScript code, and assembled standalone HTML files through multiple intermediate steps (
generateCytoscapeConfig,createNodeElements,createEdgeElements,generateJavaScriptCode, andexportCytoscapeToHTML). This refactor simplifies the visualization pipeline by migrating to anhtmlwidgets-based approach, which provides a more maintainable and standardized mechanism for saving interactive web-based visualizations. The new implementation leverages thecytoscapeNetworkwidget andhtmlwidgets::saveWidgetfor direct HTML export, significantly reducing code complexity (1,330 lines removed) and eliminating bespoke HTML/JavaScript generation.Changes
Removed Public Functions and Exports:
generateCytoscapeConfigfromNAMESPACEexportCytoscapeToHTMLfromR/visualizeNetworksWithHTML.R(previously provided standalone HTML export via custom pipeline)generateCytoscapeConfig,createNodeElements,createEdgeElements,generateJavaScriptCode, and associated utilitiesman/generateCytoscapeConfig.Rd(42 lines)man/generateJavaScriptCode.Rd(17 lines)Modified Functions (Signatures Maintained, Implementation Changed):
exportNetworkToHTML(nodes, edges, filename = "network_visualization.html", displayLabelType = "id", nodeFontSize = 12, ...): Now usescytoscapeNetwork()widget creation followed byhtmlwidgets::saveWidget()instead of the previous custom HTML assembly pipelinepreviewNetworkInBrowser(nodes, edges, displayLabelType = "id", nodeFontSize = 12, ...): Now delegates to the widget-based export mechanism instead of custom HTML generationImplementation Details:
htmlwidgetsframeworkhtmlwidgets::saveWidgetmechanism for both file export and browser previewFile Changes:
NAMESPACE: 1 line removedR/visualizeNetworksWithHTML.R: Net change of -1,319 lines (1,330 removed, 11 added)Unit Tests
The test directory structure exists (
tests/testthat), but no information is provided regarding unit tests added or modified in this PR based on the available commit data.Coding Guidelines
No violations of coding guidelines are evident. The refactor represents best practice improvements:
generateCytoscapeConfigandexportCytoscapeToHTMLaligns with better encapsulation principleshtmlwidgetsframework over custom HTML generation improves alignment with R package best practices and maintainability standards