Add INDRA integration to MSstatsShiny#110
Conversation
* adding network visualization core logic * linking step 5 to the pipeline and adding nodes table to output in step 5 * highlighting the selected edge from the network in edges table * error handling for indra method calls * swapping the positions of edges and nodes table in ui * PR suggestions * PR suggestions
|
""" WalkthroughA new Shiny module for interactive protein network visualization was added, including both UI and server components. The DESCRIPTION and NAMESPACE files were updated to include new dependencies. The main Shiny app's UI and server were modified to integrate the new network visualization tab and module. Extensive unit tests and documentation for the new visualization functions were also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Shiny_UI
participant NetworkUI_Module
participant NetworkServer_Module
participant MSstatsBioNet
participant CytoscapeJS
User->>Shiny_UI: Navigates to "Network Interpretation" tab
Shiny_UI->>NetworkUI_Module: Renders networkUI
User->>NetworkUI_Module: Uploads CSV/selects parameters
NetworkUI_Module->>NetworkServer_Module: Triggers server logic (show network)
NetworkServer_Module->>MSstatsBioNet: Annotate proteins / extract subnetwork
NetworkServer_Module->>CytoscapeJS: Prepares and sends visualization data
CytoscapeJS-->>User: Displays interactive network
User->>CytoscapeJS: Clicks edge/node
CytoscapeJS->>NetworkServer_Module: Sends selection event
NetworkServer_Module->>NetworkUI_Module: Updates edge/node tables
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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). (1)
🔇 Additional comments (5)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 6
🧹 Nitpick comments (7)
R/module-visualize-network-server.R (7)
30-31: Simplify the setNames usage.The
setNamescall is redundant here since you're using the same values for both names and values.- updateSelectInput(session, "selectedLabel", - choices = c(setNames(unique_labels, unique_labels))) + updateSelectInput(session, "selectedLabel", + choices = unique_labels)
45-49: Remove redundant req() call.The
req(input$statementTypes)on line 45 is redundant since you already require it on line 42.- # Handle "all" selections for statement type - statementTypes <- if("all" %in% req(input$statementTypes)) { + # Handle "all" selections for statement type + statementTypes <- if("all" %in% input$statementTypes) { NULL } else { input$statementTypes }
100-101: Consider using proper logging instead of print().The
print()statement might not be appropriate for production use. Consider using Shiny's logging capabilities or remove it.showNotification(paste("Error in subnetwork extraction:", e$message), type = "error") - print(e$message) + # Log error for debugging if needed + # shiny::logError(e) return(NULL)
125-134: Consider using lapply for better performance and JSON safety.Using
apply()withcbind()can be inefficient, and manual string concatenation for JSON-like structures is error-prone.- 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'], "' } }") - }) + nodes$color <- node_colors + lapply(1:nrow(nodes), function(i) { + row <- nodes[i, ] + # Use the appropriate label, fallback to id if hgncName is missing/empty + display_label <- if(label_column == "hgncName" && !is.na(row[[label_column]]) && row[[label_column]] != "") { + row[[label_column]] + } else { + row[["id"]] + } + + # Consider using jsonlite::toJSON for proper escaping + paste0("{ data: { id: '", row[["id"]], "', label: '", display_label, "', color: '", row[["color"]], "' } }") + })Also consider using
jsonlite::toJSON()to properly escape special characters in labels that could break the JavaScript.
137-150: Use lapply for more idiomatic R code.Replace the for loop with
lapplyfor better R style and potential performance benefits.createEdgeElements <- function(edges) { - edge_elements <- list() - - for (i in 1:nrow(edges)) { - row <- edges[i,] - edge_key <- paste(row['source'], row['target'], row['interaction'], sep = "-") - edge_elements[[edge_key]] <- paste0("{ data: { source: '", row['source'], - "', target: '", row['target'], - "', id: '", edge_key, - "', interaction: '", row['interaction'], "' } }") - } - - return(edge_elements) + lapply(1:nrow(edges), function(i) { + row <- edges[i,] + edge_key <- paste(row[['source']], row[['target']], row[['interaction']], sep = "-") + paste0("{ data: { source: '", row[['source']], + "', target: '", row[['target']], + "', id: '", edge_key, + "', interaction: '", row[['interaction']], "' } }") + }) }
257-261: Consider parameterizing the Shiny namespace.The hardcoded 'network-' prefix in the Shiny input ID might cause issues if the module is used with a different namespace.
Consider passing the namespace as a parameter to make the function more reusable:
-generateCytoscapeJS <- function(node_elements, edge_elements) { +generateCytoscapeJS <- function(node_elements, edge_elements, ns = "network") { # ... existing code ... - Shiny.setInputValue('network-edgeClicked', { + Shiny.setInputValue('" + ns + "-edgeClicked', {
410-416: Consider caching network data to avoid recomputation.The
renderNetwork()reactive is called again in the edge click observer, which might trigger unnecessary recomputation.Consider storing the network data in a reactive value to avoid recomputation:
+ # Cache network data + networkData <- reactiveVal(NULL) + + observeEvent(renderNetwork(), { + networkData(renderNetwork()) + }) + # Observe edge click events observeEvent(input$edgeClicked, { edge_data <- input$edgeClicked - edges_table <- renderNetwork()$edges_table + edges_table <- networkData()$edges_table highlightEdgeInTable(output, edge_data, edges_table) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
DESCRIPTION(2 hunks)NAMESPACE(2 hunks)R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.R(1 hunks)R/server.R(1 hunks)R/ui.R(1 hunks)
🔇 Additional comments (16)
DESCRIPTION (1)
45-45: RoxygenNote version update looks good.The update from 7.3.1 to 7.3.2 is appropriate for the new code being added.
R/server.R (1)
53-53: Clean integration of the network visualization module.The module integration is implemented correctly, passing the appropriate session and data_comparison reactive objects to the network visualization server.
R/ui.R (1)
59-59: Well-structured UI integration for the network module.The new tab panel is properly integrated with consistent ID usage, appropriate icon selection, and clear labeling that follows the established pattern.
NAMESPACE (3)
33-36: DT imports properly configured.The additional DT imports (DTOutput, datatable, renderDT) complement the existing ones and support the interactive tables needed for the network visualization.
39-40: MSstatsBioNet imports enable INDRA functionality.The imported functions
annotateProteinInfoFromIndraandgetSubnetworkFromIndraprovide the core INDRA integration capabilities for protein network analysis.
156-164: Comprehensive shinydashboard imports for UI components.All necessary shinydashboard components are imported to support the dashboard-style layout used in the network visualization module.
R/module-visualize-network-ui.R (4)
57-71: Well-structured radio button configurations.The protein ID type and display label radio buttons are properly configured with clear options and sensible defaults.
84-155: Comprehensive filtering options provided.The statement types and sources dropdowns offer extensive filtering capabilities that align well with INDRA's data structure and functionality.
193-225: Well-designed color legend component.The color legend provides clear visual guidance for interpreting log fold change values in the network visualization with proper styling and layout.
312-324: Clean module structure with proper namespacing.The main UI function follows Shiny module best practices with proper namespacing and integration of all components.
R/module-visualize-network-server.R (6)
6-21: Well-implemented data loading function with proper error handling.The function correctly handles both data sources (upload and reactive) and includes appropriate error handling for file reading operations.
74-81: Clean implementation of label filtering.The function properly handles the case where the Label column might not exist and correctly filters out NA values.
83-90: Good error handling for external package call.The function properly wraps the external call with error handling and user notification.
153-180: Well-implemented color mapping function.Excellent handling of edge cases including NA values and single unique values. The normalization and color gradient logic is sound.
270-287: Clean implementation of data table rendering.Good use of consistent options across both tables with appropriate selection mode for the edges table.
1-422: Overall excellent implementation of the INDRA network visualization module.The code is well-structured, includes proper error handling, and successfully integrates INDRA functionality as intended. The suggested improvements are mostly for optimization and code style rather than critical issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
R/module-visualize-network-ui.R (2)
40-44: Consider extracting inline styles to external CSS for better maintainability.Multiple UI components use inline CSS styles. Consider moving these to an external stylesheet or using CSS classes for better maintainability and consistency across the application.
Create a
www/network-styles.cssfile and include it in the UI:/* www/network-styles.css */ .data-source-info { padding: 10px; background-color: #d9edf7; border: 1px solid #bce8f1; border-radius: 4px; margin-bottom: 15px; } .icon-wrapper { position: absolute; top: 8px; right: 10px; z-index: 1000; } .loading-indicator { display: none; text-align: center; } .color-legend { position: absolute; top: 10px; right: 10px; background-color: rgba(255, 255, 255, 0.9); border: 1px solid #ddd; border-radius: 5px; padding: 10px; box-shadow: 0 2px 4px rgba(0,0,0,0.1); z-index: 1000; min-width: 150px; }Then update the UI components to use classes instead of inline styles.
Also applies to: 158-173, 184-188, 215-244
255-255: Consider making the network visualization height responsive.The network container has a fixed height of 500px, which may not be optimal for different screen sizes and resolutions.
Consider using viewport-relative units or making the height configurable:
- style = "position: relative; width: 100%; height: 500px;", + style = "position: relative; width: 100%; height: 70vh; min-height: 400px; max-height: 800px;",R/module-visualize-network-server.R (1)
131-131: Remove or replace console print with proper logging.The error message is being printed to console which may not be appropriate in production.
Consider removing the print statement or using a proper logging mechanism:
- print(e$message) + # Error is already shown via notification
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.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). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
R/module-visualize-network-ui.R (2)
5-18: Address security and version issues identified in past reviews.The past review comments have already identified critical issues in this code segment:
- Cytoscape.js version 3.19.1 needs to be updated to 3.32.0
- The
eval(message)call on line 14 poses a security risk and should be replaced with a safer pattern usingnew Function(code)()wrapped in try-catch
334-348: Address missing NAMESPACE export identified in past reviews.The past review comments have already identified that the
networkUIfunction needs to be exported in the NAMESPACE file since it's called fromR/ui.R.
🧹 Nitpick comments (2)
R/module-visualize-network-ui.R (2)
87-156: Consider extracting the long choice lists to constants or configuration.The statement types and sources choice lists are quite extensive and hardcoded within the function. For better maintainability, consider extracting these to named constants at the module level or in a configuration file.
Example refactor:
+# Define constants at module level +STATEMENT_TYPES <- list( + "All Types" = "all", + "Complex" = "Complex", + "Inhibition" = "Inhibition", + # ... rest of choices +) + +DATA_SOURCES <- list( + "All Sources" = "all", + "Reach" = "reach", + # ... rest of choices +) createFilterDropdowns <- function(ns) { tagList( selectInput(ns("statementTypes"), "Statement Types", - choices = list("All Types" = "all", ...), + choices = STATEMENT_TYPES, selected = "all", multiple = TRUE), selectInput(ns("sources"), "Sources", - choices = list("All Sources" = "all", ...), + choices = DATA_SOURCES, selected = "all", multiple = TRUE) ) }
217-249: Consider extracting inline CSS to improve maintainability.The
createColorLegend()function contains substantial inline CSS styling that could be better organized for maintainability and reusability.Consider moving the styles to a separate CSS file or at least organizing them as named style constants:
+# Define CSS styles as constants +LEGEND_CONTAINER_STYLE <- paste( + "position: absolute; top: 10px; right: 10px;", + "background-color: rgba(255, 255, 255, 0.9);", + "border: 1px solid #ddd; border-radius: 5px;", + "padding: 10px; box-shadow: 0 2px 4px rgba(0,0,0,0.1);", + "z-index: 1000; min-width: 150px;" +) +GRADIENT_BAR_STYLE <- paste( + "height: 20px; width: 100%;", + "background: linear-gradient(to right, #ADD8E6, #D3D3D3, #FFA590);", + "border: 1px solid #ccc; border-radius: 3px; margin-bottom: 5px;" +) createColorLegend <- function() { div( - style = "position: absolute; top: 10px; right: 10px; ...", + style = LEGEND_CONTAINER_STYLE, # ... rest of implementation ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.R(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- R/module-visualize-network-server.R
⏰ 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). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
R/module-visualize-network-server.R (4)
220-220: Fix container ID mismatch between UI and JavaScript code.The JavaScript code references
'network-cy'but the UI module creates a div with idns("cy")which will be namespaced.The container ID needs to be dynamically generated to match the namespaced ID:
- container: document.getElementById('network-cy'), + container: document.getElementById('", session$ns("cy"), "'),
322-325: Add validation for edge_data fields.The function assumes edge_data contains source, target, and interaction fields without validation.
highlightEdgeInTable <- function(output, edge_data, edges_table) { + # Validate edge_data has required fields + req(edge_data$source, edge_data$target, edge_data$interaction) + source <- edge_data$source target <- edge_data$target interaction <- edge_data$interaction
350-352: Add roxygen documentation for the main server function.The main server function lacks documentation. Consider adding roxygen comments to describe parameters and functionality.
+#' Server logic for network visualization module +#' +#' @param input Shiny input object +#' @param output Shiny output object +#' @param session Shiny session object +#' @param parent_session Parent Shiny session +#' @param dataComparison Reactive expression containing comparison data +#' +#' @return None (side effects only) +#' #' @importFrom MSstatsBioNet annotateProteinInfoFromIndra getSubnetworkFromIndra #' @importFrom DT renderDT datatable visualizeNetworkServer <- function(input, output, session, parent_session, dataComparison) {
446-451: Add null check for network data in edge click observer.The edge click observer could fail if
renderNetwork()returns NULL.Add validation to ensure network data exists:
observeEvent(input$edgeClicked, { edge_data <- input$edgeClicked - edges_table <- renderNetwork()$edges_table + network_data <- renderNetwork() + req(network_data) + edges_table <- network_data$edges_table highlightEdgeInTable(output, edge_data, edges_table) })
🧹 Nitpick comments (1)
R/module-visualize-network-server.R (1)
102-136: Good error handling in data processing functions.The data processing functions properly handle errors and provide user feedback. However, consider removing the debug print statement.
Consider removing the debug print statement:
}, error = function(e) { showNotification(paste("Error in subnetwork extraction:", e$message), type = "error") - print(e$message) return(NULL) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
R/module-visualize-network-server.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). (1)
- GitHub Check: build
🔇 Additional comments (1)
R/module-visualize-network-server.R (1)
6-95: Well-structured data management functions with proper error handling.The data management helper functions demonstrate good practices:
- Proper use of
req()for input validation- Graceful handling of missing columns in dropdown updates
- Comprehensive error handling with user notifications
- Clear separation of concerns between different data operations
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
R/module-visualize-network-ui.R (1)
5-18: Previous security and version concerns still apply.The existing review comments about updating Cytoscape.js to version 3.32.0 and replacing the unsafe
eval()usage with a more secure message handling pattern remain valid and should be addressed.
🧹 Nitpick comments (2)
R/module-visualize-network-ui.R (2)
87-179: Consider extracting hardcoded choices to constants or configuration.The
createFilterDropdownsfunction contains extensive hardcoded lists of statement types and sources. This makes the code harder to maintain and test.Consider extracting these to named constants at the top of the file:
+# Statement type choices +STATEMENT_TYPE_CHOICES <- list( + "All Types" = "all", + "Complex" = "Complex", + "Inhibition" = "Inhibition", + # ... rest of choices +) + +# Source choices +SOURCE_CHOICES <- list( + "All Sources" = "all", + "Reach" = "reach", + "Trips" = "trips", + # ... rest of choices +) createFilterDropdowns <- function(ns) { tagList( selectInput(ns("statementTypes"), "Statement Types", - choices = list("All Types" = "all", ...), + choices = STATEMENT_TYPE_CHOICES, selected = "all", multiple = TRUE), selectInput(ns("sources"), "Sources", - choices = list("All Sources" = "all", ...), + choices = SOURCE_CHOICES, selected = "all", multiple = TRUE),
217-249: Consider extracting inline CSS to external stylesheet or CSS classes.The
createColorLegendfunction contains extensive inline CSS styling which makes the code harder to read and maintain.Consider moving the CSS to an external stylesheet or defining CSS classes:
+# Add to a CSS file or at the top of the UI +tags$style(" +.color-legend { + position: absolute; top: 10px; right: 10px; + background-color: rgba(255, 255, 255, 0.9); + border: 1px solid #ddd; border-radius: 5px; + padding: 10px; box-shadow: 0 2px 4px rgba(0,0,0,0.1); + z-index: 1000; min-width: 150px; +} +.legend-title { font-weight: bold; font-size: 12px; margin-bottom: 8px; text-align: center; color: #333; } +.gradient-bar { height: 20px; width: 100%; background: linear-gradient(to right, #ADD8E6, #D3D3D3, #FFA590); border: 1px solid #ccc; border-radius: 3px; margin-bottom: 5px; } +.legend-labels { display: flex; justify-content: space-between; font-size: 10px; color: #666; } +.legend-info { margin-top: 8px; font-size: 9px; color: #888; text-align: center; border-top: 1px solid #eee; padding-top: 5px; } +") createColorLegend <- function() { div( - style = "position: absolute; top: 10px; right: 10px; ...", + class = "color-legend", div(class = "legend-title", "LogFC Legend"), - div(style = "height: 20px; width: 100%; ...", + div(class = "gradient-bar"), # ... rest with classes instead of inline styles ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.R(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- R/module-visualize-network-server.R
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (3)
R/module-visualize-network-ui.R (3)
301-314: Clean and well-structured layout function.The layout design with settings on the left (4 columns) and visualization on the right (8 columns) provides a good user experience balance.
320-328: Good abstraction of dashboard components.The helper functions provide clean separation of concerns and make the main UI function more readable.
334-348: Main UI function is well-structured with proper namespacing.The function correctly uses
NS()for module namespacing and organizes components logically. The@importFromdocumentation is comprehensive.Note: Ensure the
networkUIfunction is properly exported in the NAMESPACE file as mentioned in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
R/module-visualize-network-server.R (4)
406-412: Fix container ID mismatch between UI and JavaScript code.The container ID is hardcoded as 'network-cy' but should use the namespaced ID from the UI module.
Apply this diff to fix the container ID:
- container: document.getElementById('network-cy'), + container: document.getElementById('", session$ns("cy"), "'),
550-583: Add validation for edge_data fields.The function assumes edge_data contains required fields without validation.
Apply this diff to add validation:
highlightEdgeInTable <- function(output, edge_data, edges_table) { + # Validate edge_data has required fields + req(edge_data$source, edge_data$target, edge_data$interaction) + source <- edge_data$source target <- edge_data$target interaction <- gsub(" \\(bidirectional\\)", "", edge_data$interaction)
589-591: Add roxygen documentation for the main server function.The main server function lacks proper documentation.
Add roxygen documentation:
+#' Server logic for network visualization module +#' +#' @param input Shiny input object +#' @param output Shiny output object +#' @param session Shiny session object +#' @param parent_session Parent Shiny session +#' @param dataComparison Reactive expression containing comparison data +#' +#' @return None (side effects only) +#' #' @importFrom MSstatsBioNet annotateProteinInfoFromIndra getSubnetworkFromIndra #' @importFrom DT renderDT datatable visualizeNetworkServer <- function(input, output, session, parent_session, dataComparison) {
685-690: Add null check for network data in edge click observer.The observer could fail if renderNetwork() returns NULL.
Apply this diff to add validation:
observeEvent(input$edgeClicked, { edge_data <- input$edgeClicked - edges_table <- renderNetwork()$edges_table + network_data <- renderNetwork() + req(network_data) + edges_table <- network_data$edges_table highlightEdgeInTable(output, edge_data, edges_table) })
🧹 Nitpick comments (2)
R/module-visualize-network-server.R (2)
133-133: Consider using proper logging instead of print statement.Using print() for error messages is not ideal in production code.
Consider using Shiny's logging capabilities or the logger package:
- print(e$message) + # Use Shiny's built-in logging or a logging package + message("Subnetwork extraction error: ", e$message)
591-591: Remove unused parent_session parameter.The
parent_sessionparameter is not used anywhere in the function.If this parameter is not needed, consider removing it:
-visualizeNetworkServer <- function(input, output, session, parent_session, dataComparison) { +visualizeNetworkServer <- function(input, output, session, dataComparison) {Note: Only remove if you're certain it's not needed by the calling code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.R(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- R/module-visualize-network-ui.R
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (2)
R/module-visualize-network-server.R (2)
6-96: Well-structured data management functions with proper validation.The helper functions for data loading, choice updates, and parameter extraction are well-implemented with appropriate error handling, null checks, and input validation.
102-137: Data processing functions properly handle edge cases.The filtering, annotation, and subnetwork extraction functions have appropriate error handling and gracefully handle missing columns or processing failures.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
R/module-visualize-network-server.R (4)
413-457: Container ID mismatch needs fixingThe container ID in the JavaScript code doesn't account for Shiny's namespacing in modules.
The JavaScript code references the container by a direct ID, but in a Shiny module, IDs are namespaced. This will cause the Cytoscape initialization to fail.
566-599: Add validation for edge_data fieldsThe function assumes edge_data contains required fields without validation.
The function accesses edge_data properties without checking if they exist, which could cause runtime errors if the data structure is incomplete.
750-752: Add roxygen documentation for the main server functionThe main server function lacks documentation.
The function needs proper roxygen documentation to describe its purpose and parameters.
845-850: Add null check for network data in edge click observerThe edge click observer could fail if
renderNetwork()returns NULL.The observer accesses the edges_table without verifying that renderNetwork() returned valid data.
🧹 Nitpick comments (1)
R/module-visualize-network-server.R (1)
209-234: Optimize node element creation for better performanceUsing
apply()withcbind()converts all columns to character type, which is inefficient for large networks.Consider using vectorized operations:
- 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'], "' } }") - }) + # Vectorized label selection + display_labels <- if(label_column == "hgncName" && label_column %in% names(nodes)) { + ifelse(!is.na(nodes[[label_column]]) & nodes[[label_column]] != "", + nodes[[label_column]], nodes$id) + } else { + nodes$id + } + + # Vectorized element creation + paste0("{ data: { id: '", nodes$id, "', label: '", display_labels, + "', color: '", node_colors, "' } }")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.R(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- R/module-visualize-network-ui.R
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (13)
R/module-visualize-network-server.R (13)
7-35: Well-implemented color mapping functionThe function correctly handles edge cases (NA values, uniform data) and uses appropriate normalization. The default max value of 2 provides good color distribution for typical logFC ranges.
38-77: Clean configuration structure for relationship propertiesGood separation of concerns by centralizing relationship styling configuration. The consolidation rules will help manage bidirectional edges effectively.
170-207: Robust edge styling logicGood handling of different relationship categories and edge types. The special handling for regulatory colors and bidirectional arrows is well-implemented.
236-267: Good edge element creation with proper consolidationThe function correctly consolidates edges before creating elements and includes all necessary styling attributes in the edge data.
269-411: Excellent documentation and comprehensive configurationWell-documented function with clear parameter descriptions and examples. The dynamic node sizing based on label length and comprehensive style configuration are particularly well-implemented.
459-503: Well-implemented R to JavaScript convertersBoth helper functions properly handle type conversion and quote escaping. The distinction between different value types (strings, functions, booleans, numbers) is handled correctly.
547-564: Clean data table renderingGood use of consistent options across both tables. The single selection mode for edges table aligns well with the edge highlighting feature.
615-630: Robust data loading with proper error handlingGood implementation that handles both data sources (comparison results and file uploads) with appropriate error handling and user notifications.
633-647: Well-implemented dropdown update logicGood handling of edge cases including NA values and missing Label column. The fallback behavior provides clear user feedback.
650-667: Efficient protein dropdown with server-side processingGood use of server-side selectize input for handling potentially large protein lists. This will perform well even with thousands of proteins.
669-704: Clean parameter extraction with proper validationGood use of
req()for validation and clean handling of "all" selections. The NULL conversion for "all" values simplifies downstream filtering logic.
706-744: Well-structured helper functions with consistent error handlingGood abstraction of data processing steps with consistent error handling and user notifications across all functions.
80-167: Fix bidirectional edge processing bugThe function creates
reverse_keyon line 90 but never uses it. More critically, when consolidating bidirectional edges, onlypair_keyis added toprocessed_pairs(line 146), which means the reverse edge could still be processed separately, leading to duplicate edges.Apply this fix to properly track both directions:
- # Mark both directions as processed - processed_pairs <- c(processed_pairs, pair_key) + # Mark both directions as processed + reverse_pair_key <- paste(c(edge$target, edge$source, edge$interaction), collapse = "-") + processed_pairs <- c(processed_pairs, pair_key, reverse_pair_key)Also remove the unused
reverse_keyvariable on line 90.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
R/module-visualize-network-server.R (6)
509-554: Fix Shiny input namespacing for module reusabilityThe function hardcodes input names which breaks module namespacing conventions. This prevents module reusability.
The input names 'network-edgeClicked' and 'network-nodeClicked' need to be properly namespaced using the session's namespace function.
596-599: Add validation for edge_data fieldsThe function assumes edge_data contains required fields without validation.
Add validation using
req()to ensure edge_data has the required fields before accessing them.
780-782: Add roxygen documentation for the main server functionThe main server function lacks documentation.
Add roxygen comments to describe the function's purpose and parameters.
836-836: Pass namespace to Cytoscape generation functionThe function calls generateCytoscapeJSForShiny without passing the namespace parameter.
The namespace needs to be passed to ensure proper input handling in the module context.
875-880: Add null check for network data in edge click observerThe edge click observer could fail if renderNetwork() returns NULL.
Add validation using
req()to ensure network data exists before accessing edges_table.
448-448: Fix container ID to use proper namespacingThe container ID needs to be dynamically generated using the session namespace instead of hardcoded value.
The container ID should use
session$ns("cy")to match the namespaced ID in the UI module.
🧹 Nitpick comments (3)
R/module-visualize-network-server.R (3)
90-90: Remove unused variablereverse_keyThe variable
reverse_keyis created but never used in the function.pair_key <- paste(sort(c(edge$source, edge$target)), edge$interaction, collapse = "-") - reverse_key <- paste(sort(c(edge$source, edge$target), decreasing = TRUE), edge$interaction, sep = "-")
224-233: Consider vectorized approach for better performanceUsing
apply()with row-wise operations can be slow for large datasets. Consider a vectorized approach for better performance.- 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'], "' } }") - }) + # Vectorized label selection + display_labels <- if(label_column == "hgncName" && "hgncName" %in% names(nodes)) { + ifelse(!is.na(nodes$hgncName) & nodes$hgncName != "", nodes$hgncName, nodes$id) + } else { + nodes$id + } + + # Vectorized element creation + paste0("{ data: { id: '", nodes$id, "', label: '", display_labels, "', color: '", node_colors, "' } }")
244-264: Optimize edge element creation with vectorizationThe for loop can be replaced with vectorized operations for better performance with large edge sets.
Consider using
lapply()or vectorized string operations instead of a for loop for creating edge elements, especially when dealing with large networks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
R/module-visualize-network-server.R(1 hunks)
🔇 Additional comments (8)
R/module-visualize-network-server.R (8)
38-77: Well-structured relationship properties definitionThe function provides a clean, maintainable way to define visual properties for different relationship types. Good separation of concerns.
170-207: Clean edge styling logicThe function effectively handles different edge types and categories with appropriate visual styling. Good fallback handling for unknown relationships.
269-411: Well-designed configuration generatorExcellent function design with clear documentation, sensible defaults, and flexibility through optional parameters. The style configuration is comprehensive and handles various edge cases.
413-503: Robust JavaScript code generationThe JavaScript generation functions properly handle type conversions and string escaping. Good separation of style and layout conversion logic.
556-573: Standard datatable rendering implementationGood implementation with appropriate options for searchability and scrolling.
645-697: Well-implemented data management functionsGood error handling, null checking, and use of server-side selectize for handling large protein lists efficiently.
699-734: Clean parameter extraction with proper validationGood use of
req()for validation and logical handling of "all" selections.
740-774: Consistent error handling in data processing functionsExcellent use of tryCatch with user notifications for all data processing steps.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
R/module-visualize-network-server.R (4)
587-588: LGTM - Input validation properly implementedThe
req()validation for edge_data fields has been properly added as suggested in previous reviews.
772-784: LGTM - Proper documentation addedThe roxygen documentation for the main server function has been properly added as requested in previous reviews.
839-844: LGTM - Namespace issues resolvedThe container_id and module_id are now properly passed to address the namespace concerns raised in previous reviews.
883-887: LGTM - Null check properly implementedThe null check for network data has been properly added as suggested in previous reviews.
🧹 Nitpick comments (4)
R/module-visualize-network-server.R (4)
93-93: Remove unused variableThe
reverse_keyvariable is defined but never used in the consolidation logic.- reverse_key <- paste(sort(c(edge$source, edge$target), decreasing = TRUE), edge$interaction, sep = "-")
227-236: Consider vectorized approach for better performanceUsing
apply()withcbind()can be inefficient for large datasets. Consider a more vectorized approach.- 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'], "' } }") - }) + # Vectorized approach + display_labels <- ifelse( + label_column == "hgncName" & !is.na(nodes[[label_column]]) & nodes[[label_column]] != "", + nodes[[label_column]], + nodes$id + ) + + paste0("{ data: { id: '", nodes$id, "', label: '", display_labels, "', color: '", node_colors, "' } }")
449-493: Consider input sanitization for JavaScript generationWhile the current usage is internal, the JavaScript generation functions could theoretically be vulnerable to code injection if user-controlled data reaches the style/layout configurations.
Consider adding input validation if these functions might ever process user-provided configuration data:
convertStyleToJS <- function(style_list) { + # Validate input is a list + if (!is.list(style_list)) { + stop("style_list must be a list") + } + style_items <- sapply(style_list, function(item) {
800-828: Consider caching network computationThe
renderNetwork()reactive is called multiple times but performs expensive operations. Consider optimizing the reactive chain.The current implementation may recompute the network multiple times. Consider restructuring to cache intermediate results:
# Create a reactive expression to generate the network data renderNetwork <- reactive({ + # Add caching key based on input parameters + cache_key <- paste(input$selectedLabel, input$pValue, input$evidence, + paste(input$statementTypes, collapse=","), + paste(input$sources, collapse=","), + input$absLogFC, + paste(input$selectedProteins, collapse=","), + sep="_") + params <- getInputParameters(input)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
DESCRIPTION(2 hunks)NAMESPACE(7 hunks)R/module-visualize-network-server.R(1 hunks)R/module-visualize-network-ui.R(1 hunks)man/generateCytoscapeConfig.Rd(1 hunks)man/generateCytoscapeJSForShiny.Rd(1 hunks)man/networkUI.Rd(1 hunks)man/visualizeNetworkServer.Rd(1 hunks)tests/testthat/test_network_visualization.R(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- man/networkUI.Rd
- man/visualizeNetworkServer.Rd
🚧 Files skipped from review as they are similar to previous changes (6)
- DESCRIPTION
- NAMESPACE
- man/generateCytoscapeConfig.Rd
- man/generateCytoscapeJSForShiny.Rd
- R/module-visualize-network-ui.R
- tests/testthat/test_network_visualization.R
🔇 Additional comments (4)
R/module-visualize-network-server.R (4)
10-38: LGTM - Robust color mapping implementationThe color mapping function handles edge cases well, including NA values and uniform data. The past review comment about NA handling has been properly addressed with
na.rm = TRUEin the max() function.
290-401: Excellent decoupled architectureThe
generateCytoscapeConfigfunction provides a clean separation between configuration logic and UI rendering. The comprehensive styling and layout options create a professional visualization.
637-726: Well-structured data management functionsThe helper functions properly handle edge cases, validate inputs, and provide appropriate fallbacks for missing data. The separation of concerns is excellent.
742-766: Excellent error handling for external dependenciesThe wrapper functions properly handle errors from external MSstatsBioNet functions with user-friendly notifications. This defensive programming approach will improve user experience.
2 participants
PR Type
Enhancement
Description
Integrate INDRA-based network visualization module
Add UI with Cytoscape and interactive filters
Implement server logic: load, annotate, extract subnetwork
Update DESCRIPTION and NAMESPACE dependencies
Diagram Walkthrough
File Walkthrough
DESCRIPTION
Add new package dependenciesDESCRIPTION
NAMESPACE
Extend imports for network visualizationNAMESPACE
module-visualize-network-server.R
Implement network server logicR/module-visualize-network-server.R
module-visualize-network-ui.R
Create network visualization UIR/module-visualize-network-ui.R
server.R
Call visualizeNetworkServer moduleR/server.R
ui.R
Insert network tab into main UIR/ui.R
Summary by CodeRabbit
New Features
Documentation
Tests