Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the workflow model (steps + inputs) to support editing workflow/inputs in the Shiny app, while refactoring step execution to use command + args (string) and a workflow-level input_list.
Changes:
- Refactor
workflowstepfromoperation/paramstocommand/argsand add required-field tracking for input/result tags. - Add Shiny table modules (workflow/inputs/results) including UI for editing step fields and input values.
- Update workflow file-loading helpers and associated tests/docs; adjust WebText fetching to a structural extraction approach.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-run-workflowstep.R | Updates step/run tests to new command/args model (some stale args remain). |
| tests/testthat/test-run-workflow.R | Updates workflow execution tests to use input_list. |
| tests/testthat/test-fetch_WebText.R | Removes selector-miss warning test; aligns with new fetch behavior. |
| tests/testthat/test-extract-workflow-from-files.R | Switches to workflow_steps_from_files() + explicit input loading. |
| man/workflow_table_ui.Rd | New docs for workflow table UI module. |
| man/workflow_table_server.Rd | New docs for workflow table server module. |
| man/workflow_steps_from_files.Rd | Renamed docs for extracting steps from files (missing input_list arg doc). |
| man/workflow_file_paths.Rd | Updates reference to new loader function. |
| man/update_input_list.workflow.Rd | New docs for updating workflow input list. |
| man/update_input_list.Rd | New generic docs for update_input_list(). |
| man/update.workflowstep.Rd | New docs for update.workflowstep() (field list is outdated). |
| man/update.workflow.Rd | New docs for update.workflow() (currently describes wrong types/return). |
| man/update.Rd | Updates generic update() documentation. |
| man/run.workflowstep.Rd | Updates run-step docs for step_i/input_list. |
| man/run.workflow.Rd | Updates run-workflow docs wording to “command”. |
| man/results_table_ui.Rd | New docs for results table UI module. |
| man/results_table_server.Rd | New docs for results table server module. |
| man/new_workflowsteprun.Rd | Updates wording from operation→command. |
| man/new_workflowstep.Rd | Updates constructor docs to entry/command/args. |
| man/new_workflow.Rd | Documents new input_list and argument order. |
| man/inputs_table_ui.Rd | New docs for inputs table UI module. |
| man/inputs_table_server.Rd | New docs for inputs table server module. |
| man/get_field.workflowstep.Rd | New docs for step field getter. |
| man/get_field.workflow.Rd | New docs for workflow field getter. |
| man/get_field.Rd | New generic docs for get_field(). |
| man/fetch_WebText.Rd | Removes css_selector param docs (description still mentions selector). |
| man/extract_inputs.workflowrun.Rd | Removes extract_inputs docs (feature removed). |
| man/extract_inputs.workflow.Rd | Removes extract_inputs docs (feature removed). |
| man/extract_inputs.Rd | Removes extract_inputs generic docs (generic removed). |
| man/as.data.frame.workflowstep.Rd | New docs for as.data.frame.workflowstep. |
| inst/scripts/example_wf.R | Updates example to use my_run_1$input_list. |
| inst/main.R | Switches to devtools::load_all() for launching app. |
| inst/app/ui.R | Switches from raw tables to module UIs; adds tab id for active-tab logic. |
| inst/app/server.R | Switches from renderTable to module servers; simplifies new_workflow() call. |
| R/02-workflow_table-module.R | Adds workflow table module + step editing UI/server. |
| R/02-results_table-module.R | Adds results table module. |
| R/02-inputs_table-module.R | Adds inputs table module + input editing UI/server. |
| R/02-WebText-factory.R | Removes css_selector; extracts text via main/article/body. |
| R/01-workflowsteprun-class.R | Updates printing/summary for entry/command. |
| R/01-workflowstep-class.R | Refactors step structure (entry/command/args/required_*), adds update/get/as.data.frame. |
| R/01-workflowstate-class.R | Updates state df + state update to use entry. |
| R/01-workflowrun-class.R | Removes extract_inputs.workflowrun(). |
| R/01-workflow-from-file.R | Adds input loading + required-field parsing; introduces workflow_steps_from_files(). |
| R/01-workflow-class.R | Adds workflow input_list, update helpers, and required input/step validation in run.workflow(). |
| R/01-operationparam-class.R | Minor message/comment tweak. |
| R/00-generics.R | Replaces extract_inputs with get_field + update_input_list generics. |
| R/00-Namespace.R | Adds capture.output import. |
| NEWS.md | Fixes headings to use ##. |
| NAMESPACE | Adds new exports/S3 methods, but still references removed extract_inputs. |
Comments suppressed due to low confidence (1)
man/workflow_steps_from_files.Rd:18
- The
workflow_steps_from_files()Rd listsinput_listin the usage, but it is missing from the\arguments{}section. Please add an argument description forinput_list(what it should contain and how it's used) so the public API docs are complete.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| results_table_server <- function(id, wf_run, is_active_tab) { | ||
| moduleServer(id, function(input, output, session) { | ||
| output$results_table <- renderTable({ | ||
| wfr <- wf_run() | ||
| if (is.null(wfr)) return(NULL) | ||
| as.data.frame(wfr) |
There was a problem hiding this comment.
is_active_tab is accepted by results_table_server() but never used. If you intend to avoid recomputation when the tab is inactive, add a req(isTRUE(is_active_tab())) (or remove the argument to keep the API minimal).
NAMESPACE
Outdated
| S3method(extract_inputs,workflow) | ||
| S3method(extract_inputs,workflowrun) |
There was a problem hiding this comment.
extract_inputs is still exported and registered as an S3 generic/method here, but the extract_inputs generic and its methods were removed from R/ in this PR. This will cause NAMESPACE/installation errors. Either reintroduce the generic + methods, or remove these S3method(extract_inputs, ...) entries and export(extract_inputs) from NAMESPACE.
| \arguments{ | ||
| \item{x}{A `workflowstep` object to update.} | ||
|
|
||
| \item{step}{The index of the step to update.} | ||
|
|
||
| \item{entry}{The name of the entry to update (one of "name", "label", "comments", "command", | ||
| "params", "loop")} | ||
|
|
||
| \item{value}{The new value to assign to the specified entry.} | ||
|
|
||
| \item{...}{Additional arguments (not used).} | ||
| } | ||
| \value{ | ||
| The updated `workflowstep` object. | ||
| } |
There was a problem hiding this comment.
The documentation describes x as a workflowstep and says the return value is an updated workflowstep, but update.workflow() is a method for workflow and returns the updated workflow object. Please fix the roxygen docs so generated Rd matches the actual S3 method signature and behavior.
| # return empty wf if no inputs | ||
| if (length(input_list) == 0) { | ||
| warn_msg <- sprintf("Empty input list. Returning empty workflow.") | ||
| PEITHO:::logWarn("%s", warn_msg) | ||
| warning(warn_msg, immediate. = TRUE, call. = FALSE) | ||
| return(list()) | ||
| } |
There was a problem hiding this comment.
workflow_steps_from_files() returns an empty step list whenever length(input_list) == 0. This prevents loading workflows that don't use any @#*I*#@...@#*I*#@ input tags (or have an intentionally empty inputs.json) even though steps with only literal args could still be valid. Consider only requiring input_list when steps actually reference inputs, or treating an empty input_list as valid and relying on the required_inputs check at run-time.
| #' Fetch and parse web text from a URL | ||
| #' | ||
| #' This function retrieves the HTML content from the specified URL, | ||
| #' extracts text based on the provided CSS selector, and returns a | ||
| #' `WebText` object containing the extracted text and metadata. |
There was a problem hiding this comment.
The roxygen description still mentions extracting text "based on the provided CSS selector", but css_selector is no longer a parameter and the implementation now uses
No description provided.