Conversation
There was a problem hiding this comment.
Code Review
This pull request expands the rtemis.draw package beyond ECharts by introducing two new rendering backends: Sigma.js for network/graph visualizations and MapLibre GL for high-performance choropleth maps. It refactors the draw() function into an S7 generic that dispatches on specific option objects (EChartsOption, SigmaOption, and MapLibreOption), unifying theme resolution and widget creation. The review feedback identifies critical issues in the MapLibre JavaScript binding where regex replacements on style.cssText fail due to browser style normalization, which can be resolved by allowing optional whitespace in the regexes. Additionally, the feedback highlights a validation failure in MapRow when instantiated without an explicit value, and points out several performance bottlenecks in both the map and network model builders due to row-wise data frame subsetting inside loops, offering concrete vectorization and extraction suggestions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| tooltipEl.style.cssText = | ||
| tooltipEl.style.cssText.replace(/top:[^;]*;|bottom:[^;]*;|left:[^;]*;|right:[^;]*;/g, "") + | ||
| cornerStyle(s.tooltipPosition || "top-right"); |
There was a problem hiding this comment.
Browsers normalize style.cssText to include spaces after colons (e.g., top: 12px; right: 12px;). The regex /top:[^;]*;/g expects no spaces and will fail to match the normalized string. This prevents old positioning properties from being removed, causing conflicting styles (like both top and bottom) to accumulate in cssText.
Adding \s* after the colons makes the regex robust to browser normalization.
| tooltipEl.style.cssText = | |
| tooltipEl.style.cssText.replace(/top:[^;]*;|bottom:[^;]*;|left:[^;]*;|right:[^;]*;/g, "") + | |
| cornerStyle(s.tooltipPosition || "top-right"); | |
| tooltipEl.style.cssText = | |
| tooltipEl.style.cssText.replace(/top:\s*[^;]*;|bottom:\s*[^;]*;|left:\s*[^;]*;|right:\s*[^;]*;/g, "") + | |
| cornerStyle(s.tooltipPosition || "top-right"); |
| legendEl.style.cssText = | ||
| legendEl.style.cssText.replace(/top:[^;]*;|bottom:[^;]*;|left:[^;]*;|right:[^;]*;/g, "") + | ||
| cornerStyle(s.legendPosition || "bottom-right"); |
There was a problem hiding this comment.
Browsers normalize style.cssText to include spaces after colons (e.g., top: 12px; right: 12px;). The regex /top:[^;]*;/g expects no spaces and will fail to match the normalized string. This prevents old positioning properties from being removed, causing conflicting styles to accumulate in cssText.
Adding \s* after the colons makes the regex robust to browser normalization.
| legendEl.style.cssText = | |
| legendEl.style.cssText.replace(/top:[^;]*;|bottom:[^;]*;|left:[^;]*;|right:[^;]*;/g, "") + | |
| cornerStyle(s.legendPosition || "bottom-right"); | |
| legendEl.style.cssText = | |
| legendEl.style.cssText.replace(/top:\s*[^;]*;|bottom:\s*[^;]*;|left:\s*[^;]*;|right:\s*[^;]*;/g, "") + | |
| cornerStyle(s.legendPosition || "bottom-right"); |
| reportEl.style.cssText = | ||
| reportEl.style.cssText.replace(/top:[^;]*;|bottom:[^;]*;|left:[^;]*;|right:[^;]*;/g, "") + | ||
| cornerStyle(s.reportPosition || "bottom-left"); |
There was a problem hiding this comment.
Browsers normalize style.cssText to include spaces after colons (e.g., top: 12px; right: 12px;). The regex /top:[^;]*;/g expects no spaces and will fail to match the normalized string. This prevents old positioning properties from being removed, causing conflicting styles to accumulate in cssText.
Adding \s* after the colons makes the regex robust to browser normalization.
| reportEl.style.cssText = | |
| reportEl.style.cssText.replace(/top:[^;]*;|bottom:[^;]*;|left:[^;]*;|right:[^;]*;/g, "") + | |
| cornerStyle(s.reportPosition || "bottom-left"); | |
| reportEl.style.cssText = | |
| reportEl.style.cssText.replace(/top:\s*[^;]*;|bottom:\s*[^;]*;|left:\s*[^;]*;|right:\s*[^;]*;/g, "") + | |
| cornerStyle(s.reportPosition || "bottom-left"); |
| value = S7::new_property( | ||
| S7::class_numeric, | ||
| validator = function(value) { | ||
| if (length(value) != 1L) { | ||
| return("must be a single number") | ||
| } | ||
| NULL | ||
| } | ||
| ), |
There was a problem hiding this comment.
In S7, if a property has a validator that restricts its length to 1, but no default value is specified, S7 falls back to the type's default (which is numeric(0) or integer(0) of length 0). This causes any default construction of MapRow (e.g., MapRow(location = "USA")) to fail validation with an error: must be a single number.
Adding a default value like NA_real_ ensures the class can be instantiated safely.
value = S7::new_property(
S7::class_numeric,
default = NA_real_,
validator = function(value) {
if (length(value) != 1L) {
return("must be a single number")
}
NULL
}
),| rows <- lapply(seq_len(nrow(data)), function(i) { | ||
| extras <- if (length(tooltip) > 0L) { | ||
| stats::setNames( | ||
| lapply(tooltip, function(f) { | ||
| v <- data[[f]][i] | ||
| if (is.factor(v)) as.character(v) else v | ||
| }), | ||
| tooltip | ||
| ) | ||
| } else { | ||
| NULL | ||
| } | ||
| MapRow(location = loc[i], value = val[i], extras = extras) | ||
| }) |
There was a problem hiding this comment.
Evaluating data[[f]][i] inside the row-wise loop seq_len(nrow(data)) introduces a significant performance bottleneck due to repeated data frame column subsetting.
Extracting the tooltip columns as a list of vectors outside the loop and indexing them directly is much more efficient and runs significantly faster on larger datasets.
tooltip_list <- lapply(data[tooltip], function(v) {
if (is.factor(v)) as.character(v) else v
})
rows <- lapply(seq_len(nrow(data)), function(i) {
extras <- if (length(tooltip) > 0L) {
lapply(tooltip_list, `[[`, i)
} else {
NULL
}
MapRow(location = loc[i], value = val[i], extras = extras)
})| node_ids <- as.character(nodes[[id_col]]) | ||
| node_objs <- lapply(seq_len(nrow(nodes)), function(k) { | ||
| id <- node_ids[k] | ||
| GraphNode( | ||
| id = id, | ||
| label = if ("label" %in% names(nodes)) { | ||
| as.character(nodes[["label"]][k]) | ||
| } else { | ||
| id | ||
| }, | ||
| value = if ("value" %in% names(nodes)) { | ||
| as.numeric(nodes[["value"]][k]) | ||
| } else { | ||
| unname(degree[id] %||% 0) | ||
| }, | ||
| group = if ("group" %in% names(nodes)) { | ||
| as.character(nodes[["group"]][k]) | ||
| } else { | ||
| NULL | ||
| } | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Subsetting data frame columns (nodes[["label"]][k], etc.) inside the row-wise loop seq_len(nrow(nodes)) is highly inefficient in R.
Extracting these columns as flat vectors outside the loop and indexing them directly avoids this overhead and significantly improves performance.
node_ids <- as.character(nodes[[id_col]])
labels <- if ("label" %in% names(nodes)) as.character(nodes[["label"]]) else NULL
values <- if ("value" %in% names(nodes)) as.numeric(nodes[["value"]]) else NULL
groups <- if ("group" %in% names(nodes)) as.character(nodes[["group"]]) else NULL
node_objs <- lapply(seq_len(nrow(nodes)), function(k) {
id <- node_ids[k]
GraphNode(
id = id,
label = if (!is.null(labels)) labels[k] else id,
value = if (!is.null(values)) values[k] else unname(degree[id] %||% 0),
group = if (!is.null(groups)) groups[k] else NULL
)
})| edge_ids <- unique(c(src, tgt)) | ||
| degree <- stats::setNames(numeric(length(edge_ids)), edge_ids) | ||
| for (k in seq_len(nrow(edges))) { | ||
| w <- if (is.null(wgt) || is.na(wgt[k])) 1 else abs(wgt[k]) | ||
| degree[src[k]] <- degree[src[k]] + w | ||
| degree[tgt[k]] <- degree[tgt[k]] + w | ||
| } |
There was a problem hiding this comment.
The loop over nrow(edges) to calculate the weighted degree of each node can be completely vectorized using split and vapply. This avoids the overhead of a for loop and vector subsetting/assignment in R, which is extremely slow for large graphs.
edge_ids <- unique(c(src, tgt))
weights <- if (is.null(wgt)) rep(1, length(src)) else abs(wgt)
weights[is.na(weights)] <- 1
all_nodes <- c(src, tgt)
all_weights <- rep(weights, 2L)
deg_sums <- vapply(split(all_weights, all_nodes), sum, numeric(1L))
degree <- stats::setNames(numeric(length(edge_ids)), edge_ids)
degree[names(deg_sums)] <- deg_sums|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds two new interactive visualization backends to the rtemis.draw R package: a Sigma.js-based network visualization backend and a MapLibre GL-based choropleth map backend, complete with new S7 classes, builders, and comprehensive tests. The review feedback highlights several important issues: a bug in blendHex when handling shorthand hex colors, incorrect NA handling for isolated node degrees, potential serialization issues with named color vectors, and an invalid default value for the MapRow S7 class. Additionally, the reviewer recommended using a more robust file-reading method in map.R and removing an unused constant in rtemis-map.js.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const blendHex = (a, b) => { | ||
| const pa = parseInt(a.slice(1), 16); | ||
| const pb = parseInt(b.slice(1), 16); | ||
| const r = ((pa >> 16) + (pb >> 16)) >> 1; | ||
| const g = (((pa >> 8) & 0xff) + ((pb >> 8) & 0xff)) >> 1; | ||
| const bl = ((pa & 0xff) + (pb & 0xff)) >> 1; | ||
| return `#${((1 << 24) | (r << 16) | (g << 8) | bl).toString(16).slice(1)}`; | ||
| }; |
There was a problem hiding this comment.
The blendHex function assumes that both input colors are 6-character hex strings (excluding the #). If a user passes a 3-character shorthand hex color (e.g., #fff), parseInt and the bitwise shifts will produce incorrect color channels (e.g., the red channel will evaluate to 0). Expanding 3-character hex strings to 6-character equivalents before parsing resolves this issue.
| const blendHex = (a, b) => { | |
| const pa = parseInt(a.slice(1), 16); | |
| const pb = parseInt(b.slice(1), 16); | |
| const r = ((pa >> 16) + (pb >> 16)) >> 1; | |
| const g = (((pa >> 8) & 0xff) + ((pb >> 8) & 0xff)) >> 1; | |
| const bl = ((pa & 0xff) + (pb & 0xff)) >> 1; | |
| return `#${((1 << 24) | (r << 16) | (g << 8) | bl).toString(16).slice(1)}`; | |
| }; | |
| const blendHex = (a, b) => { | |
| const expand = (hex) => { | |
| const h = hex.replace("#", ""); | |
| return h.length === 3 ? "#" + h.split("").map(c => c + c).join("") : hex; | |
| }; | |
| const ea = expand(a); | |
| const eb = expand(b); | |
| const pa = parseInt(ea.slice(1), 16); | |
| const pb = parseInt(eb.slice(1), 16); | |
| const r = ((pa >> 16) + (pb >> 16)) >> 1; | |
| const g = (((pa >> 8) & 0xff) + ((pb >> 8) & 0xff)) >> 1; | |
| const bl = ((pa & 0xff) + (pb & 0xff)) >> 1; | |
| return `#${((1 << 24) | (r << 16) | (g << 8) | bl).toString(16).slice(1)}`; | |
| }; |
| GraphNode( | ||
| id = id, | ||
| label = if (!is.null(labels)) labels[k] else id, | ||
| value = if (!is.null(values)) values[k] else unname(degree[id] %||% 0), |
There was a problem hiding this comment.
If a node id from the nodes data frame is not present in the edges data frame (an isolated node), degree[id] will return NA (not NULL). Since the %||% operator only coalesces NULL values, value will be assigned NA instead of 0. This can be resolved by checking if the id exists in degree or using is.na() handling.
value = if (!is.null(values)) values[k] else if (id %in% names(degree)) unname(degree[id]) else 0,| nodeColor = x@node_color, | ||
| positiveColor = x@positive_color, | ||
| negativeColor = x@negative_color |
There was a problem hiding this comment.
If node_color, positive_color, or negative_color are passed as named character vectors (which is common for R color palettes), they will be serialized as JSON objects (e.g., {"primary": "#6CA3A0"}) instead of plain strings. This will cause rendering failures in the JS binding. Coercing them with as.character() or unname() in to_list prevents this.
nodeColor = as.character(x@node_color),
positiveColor = as.character(x@positive_color),
negativeColor = as.character(x@negative_color)| positive_color = rtemis_colors[1L], | ||
| negative_color = "#ff9e1f", |
There was a problem hiding this comment.
Using rtemis_colors[1L] returns a named vector of length 1 if rtemis_colors is named, whereas rtemis_colors[[1L]] (as used in draw_graph on line 554) safely extracts the unnamed scalar string. It is better to be consistent and use [[1L]] to avoid potential serialization issues.
node_color = rtemis_colors[[1L]],
positive_color = rtemis_colors[[1L]],| } | ||
| # Read the whole file as one JSON string; file size in bytes is an upper bound | ||
| # on the character count, so this reads the entire file. | ||
| topojson <- readChar(path, file.info(path)[["size"]], useBytes = TRUE) |
There was a problem hiding this comment.
Using file.info(path)[["size"]] to determine the number of characters to read can be unreliable on certain filesystems or OS mounts (where it may return 0 or NA), causing readChar to fail. A more robust way to read the entire text file is using readLines() and pasting the results.
topojson <- paste(readLines(path, warn = FALSE), collapse = "\\n")| value = S7::new_property( | ||
| S7::class_numeric, | ||
| validator = function(value) { | ||
| if (length(value) != 1L) { | ||
| return("must be a single number") | ||
| } | ||
| NULL | ||
| } | ||
| ), |
There was a problem hiding this comment.
The value property has no default specified, so S7 falls back to the class prototype (numeric(0)). However, the validator requires length(value) == 1L, which means the default prototype is invalid and calling MapRow() without arguments will throw an error. This also causes roxygen to generate an invalid default usage (value = integer(0)). Specifying a valid default like NA_real_ or 0 resolves this.
value = S7::new_property(
S7::class_numeric,
default = NA_real_,
validator = function(value) {
if (length(value) != 1L) {
return("must be a single number")
}
NULL
}
),| UA: "UKR", UG: "UGA", US: "USA", UY: "URY", UZ: "UZB", VE: "VEN", | ||
| VN: "VNM", VU: "VUT", YE: "YEM", ZA: "ZAF", ZM: "ZMB", ZW: "ZWE", | ||
| }; | ||
| const ISO3_CODES = new Set(Object.values(ISO2_TO_ISO3)); |
No description provided.