Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new timeline/Gantt chart visualization (draw_gantt) using ECharts custom series, along with a dedicated S3 plot method for rtemis::SupervisedSession execution graphs. It also updates the ECharts dependency to version 6.1.0, adds support for toolbox and axis pointer options, and implements a double-click gesture to reset zoom states. The review feedback focuses on enhancing robustness across several areas: dynamically registering the S7 plot method in .onLoad to prevent installation failures when rtemis is missing, validating input data for missing start or end values, handling all-NA start times to avoid -Inf warnings, safely recycling palette colors, and ensuring the JavaScript zoom reset handler can handle single dataZoom objects.
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.
| S7::method(plot, utils::getFromNamespace("SupervisedSession", "rtemis")) <- | ||
| plot_SupervisedSession |
There was a problem hiding this comment.
This top-level S7 method registration runs during package build/installation. If the rtemis package is not installed on the system (which is common if it is only a suggested dependency or not yet installed), utils::getFromNamespace will throw an error and crash the package installation. This registration should be wrapped in a helper function and called dynamically inside .onLoad in zzz.R.
register_supervised_session_method <- function() {
if (requireNamespace("rtemis", quietly = TRUE)) {
tryCatch({
S7::method(plot, utils::getFromNamespace("SupervisedSession", "rtemis")) <-
plot_SupervisedSession
}, error = function(e) {
# Fail silently or warn if registration fails
})
}
}| starts <- .gantt_time_values(tasks[["start"]]) | ||
| ends <- .gantt_time_values(tasks[["end"]]) |
There was a problem hiding this comment.
If the input tasks contains missing values (NA) in the start or end columns, they will be converted to NA_real_ and serialized as null in JSON. This can cause ECharts custom series rendering to fail or throw JavaScript errors in the browser. Adding an early validation check ensures robustness.
starts <- .gantt_time_values(tasks[["start"]])
ends <- .gantt_time_values(tasks[["end"]])
if (any(is.na(starts)) || any(is.na(ends))) {
cli::cli_abort(
"Columns {.val start} and {.val end} in {.arg tasks} must not contain missing values (NA)."
)
}| finished <- x@finished | ||
| end_fallback <- if ( | ||
| !is.null(finished) && length(finished) > 0L && !is.na(finished) | ||
| ) { | ||
| to_ms(finished) | ||
| } else { | ||
| max( | ||
| vapply(events, function(e) to_ms(e[["t_start"]]), numeric(1L)), | ||
| na.rm = TRUE | ||
| ) | ||
| } |
There was a problem hiding this comment.
If all events have missing start times (t_start), vapply will return all NAs, and max(..., na.rm = TRUE) will return -Inf with a warning. Pre-calculating the start times and handling the all-NA case avoids this warning and potential downstream layout issues.
finished <- x@finished
starts_ms <- vapply(events, function(e) to_ms(e[["t_start"]]), numeric(1L))
end_fallback <- if (
!is.null(finished) && length(finished) > 0L && !is.na(finished)
) {
to_ms(finished)
} else {
if (all(is.na(starts_ms))) 0 else max(starts_ms, na.rm = TRUE)
}| present <- unique(tasks[["kind"]]) | ||
| # Fall back to the default palette for any unmapped kind. | ||
| cols <- kind_colors[present] | ||
| cols[is.na(cols)] <- rtemis_colors[seq_len(sum(is.na(cols)))] |
There was a problem hiding this comment.
If the number of unmapped event kinds (sum(is.na(cols))) exceeds the length of rtemis_colors, indexing with seq_len will introduce NA values into the color vector, which can cause rendering issues in ECharts. Using rep_len safely recycles the palette colors.
cols[is.na(cols)] <- rep_len(rtemis_colors, sum(is.na(cols)))| const zoomDefs = x.option?.dataZoom; | ||
| if (zoomDefs?.length) { | ||
| chart.getZr().on("dblclick", () => { | ||
| chart.setOption({ | ||
| dataZoom: zoomDefs.map((d) => | ||
| Object.assign({}, d, { start: 0, end: 100 }) | ||
| ) | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
If dataZoom is passed as a single object instead of an array (which can happen if a user bypasses the S7 class and passes a plain list to draw()), zoomDefs.map will throw a TypeError: zoomDefs.map is not a function. Checking if it is an array and wrapping it if necessary makes the double-click reset handler robust.
const zoomDefs = x.option?.dataZoom;
if (zoomDefs) {
const zoomArr = Array.isArray(zoomDefs) ? zoomDefs : [zoomDefs];
chart.getZr().on("dblclick", () => {
chart.setOption({
dataZoom: zoomArr.map((d) =>
Object.assign({}, d, { start: 0, end: 100 })
)
});
});
}
No description provided.