Skip to content

v0.2.2: add gantt chart and plot.SupervisedSession#8

Merged
egenn merged 13 commits into
mainfrom
develop
Jun 19, 2026
Merged

v0.2.2: add gantt chart and plot.SupervisedSession#8
egenn merged 13 commits into
mainfrom
develop

Conversation

@egenn

@egenn egenn commented Jun 18, 2026

Copy link
Copy Markdown
Member

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread r/R/methods.R Outdated
Comment on lines +226 to +227
S7::method(plot, utils::getFromNamespace("SupervisedSession", "rtemis")) <-
plot_SupervisedSession

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
    })
  }
}

Comment thread r/R/draw_gantt.R
Comment on lines +189 to +190
starts <- .gantt_time_values(tasks[["start"]])
ends <- .gantt_time_values(tasks[["end"]])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)."
    )
  }

Comment on lines +59 to +69
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
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)
  }

Comment thread r/R/methods.R Outdated
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)))]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)))

Comment on lines +117 to +126
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 })
)
});
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 })
            )
          });
        });
      }

@egenn egenn merged commit 2e4364e into main Jun 19, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant