Conversation
|
Bosun CI signal: Bosun-created PR currently has failing checks.
|
There was a problem hiding this comment.
Pull request overview
Adds a richer “Workflows” TUI experience intended to list installed workflows, show recent/live runs, and allow triggering/inspection/cancellation directly from the terminal UI.
Changes:
- Introduces a dedicated
WorkflowsScreenInk component plus helper utilities for rendering rows, run history, and live status flashes/spinners. - Extends
useWorkflowsto fetch workflow summaries, read run history/details, and expose trigger/cancel/toggle/uninstall actions. - Adds vitest coverage for the new helpers and a lightweight “architecture” test asserting App mounts the new screen.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/tui/workflows-screen-helpers.js | New helper module for formatting, row building, and reducing workflow status events. |
| ui/tui/useWorkflows.js | Expands workflows hook to include history/detail plus trigger/cancel/config editing actions. |
| ui/tui/WorkflowsScreen.js | New Ink screen implementing list/history panes, trigger form, and run detail tree view. |
| ui/tui/App.js | Switches workflows tab from a simple table to WorkflowsScreen (but currently breaks an import). |
| tests/workflows-screen-helpers.test.mjs | Unit tests for helpers (form, rows, reducer, spinner ticking). |
| tests/tui-workflows-screen.test.mjs | Additional helper-focused tests around limits/sorting and reducer behavior. |
| tests/tui-workflows-architecture.test.mjs | Asserts App mounts the new workflows screen (currently brittle/failing). |
Comments suppressed due to low confidence (1)
ui/tui/App.js:297
- There are multiple trailing blank lines at the end of the file. This is likely to fail formatting/lint rules and adds noise to diffs—please trim the extra empty lines so the file ends cleanly with a single newline.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const payload = typeof input === "string" ? parseWorkflowInput(input) : input; | ||
| return engine.execute(workflowId, payload, { force: true, triggerSource: "manual" }); |
There was a problem hiding this comment.
triggerWorkflow() passes { triggerSource: "manual" } in the options object, but WorkflowEngine records trigger source from inputData._triggerSource (and emits it in payload.meta.triggerSource). If you want runs started from the TUI to be attributed correctly, set _triggerSource on the payload (or use whatever engine option is actually supported).
| const payload = typeof input === "string" ? parseWorkflowInput(input) : input; | |
| return engine.execute(workflowId, payload, { force: true, triggerSource: "manual" }); | |
| const rawPayload = typeof input === "string" ? parseWorkflowInput(input) : input; | |
| const payload = { ...rawPayload, _triggerSource: "manual" }; | |
| return engine.execute(workflowId, payload, { force: true }); |
| function renderTreeLines(value, expandedPaths, path = "root", depth = 0, lines = []) { | ||
| const isObject = value && typeof value === "object"; | ||
| if (!isObject) { | ||
| lines.push({ key: path, path, depth, label: `${path.split(".").at(-1)}: ${String(value)}`, expandable: false }); | ||
| return lines; | ||
| } | ||
|
|
||
| const entries = Array.isArray(value) | ||
| ? value.map((entry, index) => [String(index), entry]) | ||
| : Object.entries(value); | ||
| const isExpanded = expandedPaths.has(path) || depth === 0; | ||
| lines.push({ | ||
| key: path, | ||
| path, | ||
| depth, | ||
| label: `${path.split(".").at(-1)} ${isExpanded ? "▾" : "▸"}`, | ||
| expandable: true, | ||
| }); | ||
| if (!isExpanded) return lines; | ||
| for (const [key, child] of entries) { | ||
| renderTreeLines(child, expandedPaths, `${path}.${key}`, depth + 1, lines); | ||
| } | ||
| return lines; |
There was a problem hiding this comment.
renderTreeLines() walks the entire run detail object recursively even though the UI only renders lines.slice(0, 24). Since getRunDetail() includes large arrays (e.g., logs, nodeStatusEvents), this can become expensive and make the TUI sluggish. Consider adding an early-exit line limit (and/or max depth) to stop recursing once enough lines are collected.
| const appSource = readFileSync(resolve(process.cwd(), "ui/tui/App.js"), "utf8"); | ||
|
|
||
| it("mounts a dedicated workflows screen instead of a simple table", () => { | ||
| expect(appSource).toContain('./WorkflowsScreen.js'); |
There was a problem hiding this comment.
This assertion is brittle and currently mismatches ui/tui/App.js: App imports WorkflowsScreen using double quotes ("./WorkflowsScreen.js"), but the test searches for the single-quoted substring ./WorkflowsScreen.js. Update the check to match the actual source (e.g., look for "./WorkflowsScreen.js" or use a regex that ignores quote style).
| expect(appSource).toContain('./WorkflowsScreen.js'); | |
| expect(appSource).toMatch(/['"]\.\/WorkflowsScreen\.js['"]/); |
| import { useTasks } from "./useTasks.js"; | ||
| import { useWorkflows } from "./useWorkflows.js"; | ||
| import WorkflowsScreen from "./WorkflowsScreen.js"; | ||
| import { useTasks } from "./useTasks.js"; |
There was a problem hiding this comment.
useWebSocket is referenced (wsState = useWebSocket(...)) but the import for it was removed, so this file won’t compile. Re-add the useWebSocket import from ./useWebSocket.js (or remove the usage if intentional).
| import { useTasks } from "./useTasks.js"; | |
| import { useTasks } from "./useTasks.js"; | |
| import { useWebSocket } from "./useWebSocket.js"; |
| } | ||
|
|
||
| export function useWorkflows(config) { | ||
| const resolvedConfig = config || loadConfig(process.argv); |
There was a problem hiding this comment.
resolvedConfig = config || loadConfig(process.argv) will call loadConfig on every render when config is undefined, producing a new object identity each time. Because refresh depends on resolvedConfig, that can cause repeated refreshes and render loops. Memoize the fallback config (e.g., useMemo(() => config ?? loadConfig(process.argv), [config])) or require config to be passed in.
| const resolvedConfig = config || loadConfig(process.argv); | |
| const resolvedConfig = useMemo(() => config ?? loadConfig(process.argv), [config]); |
| if ((input?.toLowerCase() === "i" || key.return) && (selectedHistory || selectedTemplate)) { | ||
| const runId = selectedHistory?.runId || null; | ||
| if (runId) { | ||
| setInspectingRunId(runId); | ||
| setSelectedTreePath("root"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The inspect hotkey handler checks (selectedHistory || selectedTemplate), but then only derives runId from selectedHistory. When focus is on templates, pressing Enter/I will be a no-op if there’s no selected history row, despite the help text advertising inspect. Consider gating inspect on focusArea === "history" (or implementing template inspection) so the keybinding behaves consistently.
| if ((input?.toLowerCase() === "i" || key.return) && (selectedHistory || selectedTemplate)) { | |
| const runId = selectedHistory?.runId || null; | |
| if (runId) { | |
| setInspectingRunId(runId); | |
| setSelectedTreePath("root"); | |
| return; | |
| } | |
| if ((input?.toLowerCase() === "i" || key.return) && focusArea === "history" && selectedHistory?.runId) { | |
| setInspectingRunId(selectedHistory.runId); | |
| setSelectedTreePath("root"); | |
| return; |
| if (normalized === "completed" || normalized === "success") return "green"; | ||
| if (normalized === "failed" || normalized === "error") return "red"; | ||
| if (normalized === "running") return "yellow"; | ||
| if (normalized === "cancelled") return figures.cross ? "red" : "red"; |
There was a problem hiding this comment.
This conditional is redundant: figures.cross ? "red" : "red" always returns the same value. Simplify it (and drop the unused figures.cross check) to keep the helper clearer.
| if (normalized === "cancelled") return figures.cross ? "red" : "red"; | |
| if (normalized === "cancelled") return "red"; |
| return new WorkflowEngine({ | ||
| workflowDir: resolve(configDir, "workflows"), | ||
| runsDir: resolve(configDir, "workflow-runs"), | ||
| services: {}, |
There was a problem hiding this comment.
createEngine() initializes WorkflowEngine with services: {}. The workflow engine enforces required capabilities per node type (e.g., agent nodes require agentPool, task nodes require kanban) and will throw at runtime when these services aren’t provided. Either wire the same service dependencies the server uses (agentPool/kanban/etc.), or route trigger/cancel actions to the running backend instead of executing locally.
| services: {}, |
| const engine = createEngine(resolvedConfig); | ||
| const summaries = await Promise.resolve(listWorkflowSummaries(resolvedConfig)); | ||
| const normalized = (Array.isArray(summaries) ? summaries : []).map(summarizeWorkflowDefinition); | ||
| setWorkflows(normalized); | ||
| setHistory(engine.getRunHistory(null, 50)); |
There was a problem hiding this comment.
The hook mixes two different workflow systems: listWorkflowSummaries() (from workflow-cli) lists configured declarative workflows, but triggerWorkflow() / history / detail use WorkflowEngine (disk-backed DAG workflows + TUI status events). This will likely cause triggers/cancels to target workflows that aren’t present in the engine. Consider listing templates via WorkflowEngine.list() / listWorkflows() (or, alternatively, triggering via the declarative workflow runner) so the IDs and run history come from the same source of truth.
dd1ec1a to
c8c62c6
Compare
|
Bosun PR classification: Bosun-created.
|
…st-trigger-inspect-a
…list-trigger-inspect-a
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Task-ID: 4567dec9-f7e3-41a3-8817-367856c2aca0\n\nAutomated PR for task 4567dec9-f7e3-41a3-8817-367856c2aca0