Add a workflow health page - #5108
Open
midigofrank wants to merge 40 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5108 +/- ##
=======================================
+ Coverage 90.7% 90.8% +0.1%
=======================================
Files 419 422 +3
Lines 20755 20846 +91
=======================================
+ Hits 18831 18928 +97
+ Misses 1924 1918 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lmac-1
reviewed
Sep 1, 2026
lmac-1
left a comment
lmac-1
left a comment
Contributor
There was a problem hiding this comment.
Hey Frank, nice work! I have directly committed some updates to help improve the React side:
-
A socket that never connects now shows an error instead of "Loading…" forever. The hook only handled channel errors, so if the socket itself never came up the page just sat there. It only shows before the first load, so a brief drop doesn't blank a page that already has numbers on it.
-
Loading and error states moved into the card. Loading and error states were early returns in
HealthContent, so a failed request took the workflow name and card heading with it. It's also set things up for the next cards, since each one loads separately. -
A rejected join no longer prints "unauthorized" as the page body. That's the channel guard talking, and non-members get redirected before the page renders anyway. It goes to the console now. I left the
catchalone — those messages are already written for people.
I have left some inline comments on other areas that I found. I know you asked for a review focusing on the React architecture, but you got some Elixir bits for free 🤭
I also have some design questions:
-
Should stats update live, or only on page refresh? CON-107 covers refreshing when you switch range. I'm not sure whether they should also update while you're sat on the page. (Guessing no, but wanted to confirm.)
-
Are we counting work orders or runs? CON-106 says the subtitle should read "# runs" but this counts work orders, and CON-110 is a failed run breakdown. Different numbers on the same page will confuse people.
Contributor
Author
|
Thanks @lmac-1 . Looks like you forgot to push your changes 👀 |
Contributor
Author
|
Thanks @lmac-1 , I have implemented the changes. Regarding your two questions:
|
A per-workflow page at /w/:id/health showing a 30-day breakdown of its work orders. Stats come over a channel rather than the LiveView so each chart can be requested on its own — one slow query shouldn't hold up the rest of the page. Lightning.Workflows.Stats is kept separate from DashboardStats: the list view batches across many workflows to dodge an N+1, this queries one, and the windows diverge as soon as the page grows a range picker. The channel re-checks project membership on join. The LiveView's on_mount guards don't cover it, since a client can join any topic on the socket.
The health page only handled channel-level failures, so a socket that never connected left it sitting on "Loading…" indefinitely. Surface connectionError instead, but only before the first load — a dropped connection recovers on its own and shouldn't blank a page that already has numbers on it.
The whole page was replaced by a single line while a request was in flight or after it failed, so a broken chart took the workflow name and the card heading with it. Each card now handles its own loading and failure, which is the shape the remaining charts need anyway. A rejected join also printed the channel guard's own word — "unauthorized" — as the page body. Log that and show something the reader can act on instead.
The join checked for a project_users row, which support users don't have, so it refused them where the rest of the app admits them. Going through Permissions.can/4 also rejects projects with scheduled_deletion set. A malformed project_id crashed the join rather than erroring: Repo.get only returns nil for a well-formed id that doesn't exist, and raises Ecto.Query.CastError otherwise. Both ids now go through a cast first. Workflows.get_workflow_for_project/3 replaces the separate existence check and fetch, taking the join from four queries to two and dropping the unused :parent preload that Projects.get_project/1 carries.
The header summed all three buckets while the donut draws success and failed, so the two numbers on screen didn't add up.
The worker's vocabulary was written out twice: CompleteRun mapped reasons onto states on the way in, and the health page's error signatures mapped them back out. Move the table onto Run and derive both directions from it, so a state gaining a reason on one side can't quietly miss the other.
The page's unit becomes the run, not the work order. A retry is a second attempt with its own outcome, and a failure can only be attributed to a state that belongs to an attempt — counting work orders in one panel and runs in another would put two totals on the same screen. Adds get_failure_signatures, which attributes each failed run to its earliest failed step so the rows sum to the failure total the outcomes donut draws.
Both donuts read the same get_outcomes reply — one aggregate read two ways, so the two panels can't disagree about the failure total. The chart chrome and its legend live in a shared Donut, and each card's loading and error state goes through one Panel, so a slow query only holds up its own card.
Adds a cookie-authenticated JSON endpoint per chart, mirroring the two channel handlers. The authorisation guard is the channel's join/3 body moved into a controller plug — this is its own trust boundary for the same reason the channel was, since the health LiveView's on_mount hooks don't protect a path anyone can request. Every refusal answers 404 rather than 403, so a reply can't confirm a workflow exists to someone who can't see its project.
Two channel replies serialise behind each other — the channel is a GenServer — so the per-chart split never bought the parallelism it was built for. Two requests do. The hook keeps its exported types and its state shape, so the charts and the page compile untouched; only the transport changes. That drops the last reader of SocketProvider here, so the health page no longer opens a socket at all. One behaviour changes: an HTTP status carries no reason string, so every failed slice renders the same message instead of echoing the server's own words.
useHealthStats held a single state object with a field pair per chart, so its interface grew at the same rate as its implementation — two charts, four fields, and a copied fetch block inside one effect for each. useHealthQuery<T> takes a url and returns one result, called once per endpoint. Adding a chart is now a call site rather than an edit here: measured against a third chart, the fetch plumbing went from 18 lines to 2. The payload types move to types.ts, since the chart components were importing them from a hook module. Clearing the stored answer when the url changes is a fix, not a restructure. A panel shows "Loading…" only when it holds neither data nor an error, so a stale reply left in place kept the previous workflow's numbers on screen under the new heading. Nothing reaches that path today — the url is built from ids that don't change while the page is open — but the date range picker will put something in it that does.
An MFA-blocked member gets a 404 here rather than the /mfa_required redirect the LiveView gives them, because a JSON client has nothing to do with that redirect. Nothing asserted it, so the difference could have been removed without a test noticing. The blocked member is an owner: the MFA guard runs before any role is consulted, so the strongest member is where a reordering breaks first.
The error path checked whether the request had been aborted before writing state; the success path did not. A reply whose body finished parsing as the url changed could therefore write the previous request's numbers over the new ones — the same staleness the url reset closes from the other side. The ordering has not been reproduced in a browser: aborting usually makes `response.json()` reject, and the catch takes it from there. The guard is worth more than the certainty, since it makes the two paths agree on what an aborted request means. `return data` is what satisfies `promise/always-return`, rather than a filler value.
The Outcomes donut counted failures as everything that wasn't success, so it absorbed a new run state automatically. The failure breakdown iterated six states written out by hand. Adding a seventh to `Run.final_states/0` would have had one panel count it and the other silently drop it, with the two sitting side by side. `FAILURE_STATES` is now the one list. `RunStateCounts` derives from it rather than repeating it, Outcomes sums it, and the breakdown keys a `Record` by it — so a state added without a colour is a compile error rather than a missing slice. The colours themselves are unchanged.
`??` only catches null, so an `error_type` of `""` reached the table as itself: the signature rendered as `fail:` with nothing after the colon, and the tip as "Tip: " with no sentence. The column permits an empty string — nothing checks its length or shape on the way in from the worker. `||` catches both, and "the step didn't tell us" is what either one means.
`5d9bcc1e1` added `Channel.join`, `Channel.leave` and `Socket.channel` for the first version of the health page, which read its stats over a channel. `44df4722e` moved it onto fetch, and the declarations stopped being used. Nothing else picked them up in between: every other consumer of the phoenix types uses only `on`, `off` and `push`, and the two places that do call `.leave()` or `.channel()` either declare their own local interface or reach them through an `any` cast. The file is back to its state before this branch.
Build get_workflow_for_project/3 on Query.workflows_for/1 so every caller — the health API, the index's trigger toggle and its delete handler — refuses a workflow on its way out without checking for itself, and route workflow_exists_in_project?/2 through it rather than repeating the query.
The page exists to drive failures down, and only a work order's state can fall: a run's state is immutable, so a failure retried to success would sit here permanently. Counting work orders means the numbers move when someone fixes something. The window is anchored on last_activity rather than inserted_at, so an old work order retried today is counted as the work it currently is. Failures are attributed to the latest run — the one whose completion set the state — and its earliest failing step, so the triage rows still sum to the failure total the donuts draw. A rejected work order has no run to read a signature off, so it gets a fixed one. Cancelled moves out of the failure states: someone stopped that work order on purpose, so it is a finished outcome rather than something to triage. It gets its own Outcomes slice, drawn only when it happened, and rejected takes the palette slot it vacated in the failure breakdown.
Cover each work order state the page draws, with a spread of error signatures, so the same seed doubles as fixture data for it: rejected work orders that never got a run, retried-to-success histories where the failed attempt still has to not count, failing runs that stop at the step that broke, and last_activity drift so the window has something to exclude.
The signature joined the live jobs table, so renaming a job or bumping its adaptor relabelled history, and a job since deleted left the row unnamed. Read name and adaptor off the snapshot the step ran against instead, resolving after the group so the jsonb unnest only touches the snapshots that actually failed in the window.
…quest in RunLimitExceeded tip text
midigofrank
force-pushed
the
frank/con-106
branch
from
September 3, 2026 15:21
26bf574 to
8d4bd7e
Compare
Both health page endpoints filter on a workflow and a last_activity window. With only the single-column indexes the planner picks one and heap-filters the rest, so cost tracks total work order volume instead of the size of the window. Built concurrently, outside the migration lock.
A burst of viewers on the same workflow hits the same two aggregate queries. Cachex.fetch/4 collapses concurrent misses onto one, and caching the result whole -- window included -- stops the 30-day window rolling on every request, so a page's two panels agree with each other.
A project health page is coming, and it reuses this directory's donut, fetch hook and types unchanged. Renaming now means the second page is never written into a directory named for the first one, and the move is free while none of this has been merged. Pure move: no file's contents change beyond its import paths. The `workflow-health` DOM id stays as it is — it names that element, not the directory.
midigofrank
marked this pull request as ready for review
September 4, 2026 04:46
Security Review ✅
|
RuntimeCrash's tip described a ReferenceError, but a crashing step never reports that name: assertRuntimeCrash wraps exactly ReferenceError and SyntaxError, and the worker reports the wrapper's subtype over the error's name. So the two names arrive as themselves and fell through to the default tip, while RuntimeCrash's tip named a cause it can't have.
Work order events were only broadcast per project, so the health page had no way to hear about its own workflow without filtering everyone else's traffic. Adds a work_orders:workflow:<id> topic and subscribes the LiveView to it. A settled work order pushes health:changed, throttled to one push per 30 seconds with the first change in a quiet period going out immediately — during an incident the first failure should be on screen now. Each push drops the workflow's cached slices first, otherwise the refresh would be answered from the value computed before the change. The React side refetches on the push without clearing state, so panels hold their numbers instead of blanking to "Loading…", and a "Last Updated" clock makes it visible that pushes are still arriving.
The signature grammar and the error tips outlive the tickets that introduced them, and a reader who hits CON-31 in a comment has to go find a Linear board to learn nothing they can't read in the code beside it.
A workflow that fans out can break in two branches at once, and the second break is its own thing to fix — not fallout from the first. Attributing a failed work order to only its earliest failing step hid that: the triage table named one job and stayed silent about the other. Swap the single ordered pick for a lateral join over the latest run's failing steps, so each break gets its own row. left_lateral_join is what carries the work orders — lost, crashed, rejected — that never reached a step; a plain left join would emit an empty row per successful step instead. Rows now count distinct work orders per signature, so they can sum past the failure total the donuts draw. That's the trade, and it's the right way round: the donut answers how many broke, the table answers what to fix. Also drops the last Linear ref from the signature-grammar comment.
A work order that trips the run limit is inserted as :rejected and never gets a run, so nothing ever updates it — the only event it emits is the create. The health page subscribed to the workflow topic but only heard updates there, so those work orders sat off the page until a manual reload. Broadcast creates on the workflow topic too, and fold the final-state check into maybe_refresh/2 so both events share it.
* Add View Stats link to each workflow row Links to the workflow's health page. The row itself is clickable and navigates to the editor, so the link stops propagation to avoid being swallowed. * Rework the workflows list header and row actions The title, search and create button now sit in one row above the project metrics rather than being split between the metrics and the table heading. The trailing "Workflows" breadcrumb goes with them: the page title is on the page, so the crumb only repeated it. The project picker stays. The Monitoring column and the unlabelled column holding Delete collapse into a single "Actions" column, and the link there reads "Health" — it names the page it opens, where "View Stats" described something the page contains. The search input sat inside a flex container, so the input's own wrapper divs shrank to their content while the magnifier and clear icons stayed anchored to the full width of the box, drifting away from the field as it widened. Block flow puts them back. The create button is the flex item itself now; it reached for height through a wrapper div, and an explicit height opts a flex item out of align-items: stretch, so it rendered short of the search field instead of matching it. Metric cards stack the suffix under the value and sit on a ring rather than a drop shadow, with tighter gutters.
lmac-1
reviewed
Sep 4, 2026
lmac-1
left a comment
lmac-1
left a comment
Contributor
There was a problem hiding this comment.
This is getting quite hard to review. I'm worried we're going to miss things, and that the back and forth on these points is going to hold up work downstream. I think we need to regroup in sync on Monday about how to make this work better. The refresh changes are quite big.. I'm wondering if they should have been in their own branch while we finalise the details.
Can you confirm whether you've run /code-review on the most recent changes? It's hard to review at the moment and the PR description is now stale.
Here's what's come up so far.
Probably needs a conversation rather than a comment thread (raised on Slack to discuss but I am documenting here):
- The refresh works by the server signalling and the browser then asking for the numbers. Wondering whether the browser just polling would be simpler.
- We've added a second PubSub topic. WorkflowChannel has the same "just this one workflow" need and filters the project topic instead of adding one
- The stats cache lives in each server's own memory, so the invalidation only clears the server holding the websocket. The browser's next request could land elsewhere.
Smaller, probably just fixes (came back from reviewing with Claude)
- The triage counts can now total more than the failed number the donut shows, and nothing on the page says why.
- The 2022 index on
work_orders(workflow_id)looks redundant now the composite one exists - see inline comment WorkOrders.subscribeis inhandle_paramsrather thanmount, so it can re-subscribe on patch.- The
:workflow_statscache is shared across the async test suite with nothing resetting it between tests. - A few nits I'll leave inline.
| @@ -0,0 +1,14 @@ | |||
| defmodule Lightning.Repo.Migrations.AddWorkOrdersWorkflowActivityIndex do | |||
Contributor
There was a problem hiding this comment.
Nice! This is the right index for these queries.
One thing: we have had create index(:work_orders, [:workflow_id]) since the original work orders migration in 2022.
This new one starts with workflow_id as well, and Postgres will happily use a composite index for a query that only filters on its leading column. So the old single-column one can't answer anything this one can't.
Do you think we still need the original index? Postgres updates the index on every insert and update, so on a table as busy as work_orders we'd be paying for two where one does the job. What do you think?
Contributor
Author
There was a problem hiding this comment.
Good point, I would say we do this as a follow up once the new index has settled in
| @state_reasons Run.state_reasons() | ||
|
|
||
| @doc """ | ||
| Work order counts by final state over the last `days_back` days. |
Contributor
There was a problem hiding this comment.
we should try and get the date picker in soon cause i think we need it all working nicely together with caching etc. to regroup on monday to make a plan!
The stats cache is per node and lives 30 seconds, so a read that races the change behind a push can be answered from another node's cache, computed moments before it. Schedule one trailing re-read per burst, restarted on each push and jittered, so the requests still stop once the pushes do.
Renaming a job forks its history into a signature per name, so a long-lived workflow lists more rows than it has ways of breaking. Scroll past the fold rather than truncate — the tail is still worth reading. Counts are per failed step, so spell that out in the card meta: the rows can sum past the failure total the donuts draw.
The workflow name sat in the last crumb with nothing to click. Move it into a linked crumb next to Workflows and leave "Health" as the label. Loading the workflow in mount/3 drops the handle_params/3 that only existed to do it.
The health page only cares about one workflow, so it got its own work_orders:workflow:<id> topic — which meant broadcasting every work order create and update twice, for the benefit of a single page. Subscribing to the project topic and matching workflow_id in the handler does the same filtering for one pattern match.
Contributor
Author
|
Thanks @lmac-1 . I have resolved your change requests, really great feedback 👌 . By the way, I have removed the extra broadcast I introduced. It introduced some failing tests which I didn't want to spend more time reviewing — I'm now filtering the specific workflow events in the Liveview process |
* Accept a days window on the workflow health endpoints
The health page is about to offer three fixed ranges. Validate
`?days=` against exactly 1, 7, or 30 and reject anything else with
400 - a free integer would let someone table-scan a shared database.
Defaults to 30, so this changes nothing observable on its own.
* Add a range picker to the workflow health page
Lets you switch the health page between the last 24 hours, 7 days,
and 30 days. Both requests re-scope to the selected window, and the
subtitle and empty states describe it in the picker's own words
("24 hours") instead of a day count ("1 day").
* Hold the health page still while a range loads
Clicking a range collapsed the cards by roughly 250px and jumped the
page. The same jump was there on the empty state, which predated the
picker.
The donut now paints its 220px frame whether it holds a chart, an empty
message or the loading placeholder, so first load, empty and loaded are
all the same height. The legend below still follows the data; that is
content changing size, not a placeholder giving way. The height lives in
Donut.tsx and nowhere else: an earlier attempt reserved it on every card,
which went stale as soon as a legend had four rows and left the Triage
card mostly empty.
The panels still drop their numbers on a range switch, which is what
useHealthQuery already does for any new url. Keeping them would put the
new window in the subtitle while the Triage card below still named the
old one: outcomes is a group-by and failures a three-way join with a
distinct, so the cheap slice lands first and the gap between the two
replies is a gap the reader can see. A health:changed tick is the same
question asked again, and still keeps its answer.
The subtitle is the page's one status region. It says "Loading…" on
first load and on a range switch, and the window's summary once it
lands, so a screen reader hears each once rather than from every card in
turn.
Also:
- Key the page on the workflow id, so a patch to another workflow's
health page remounts rather than keeping the old one's state.
- "1 work order", not "1 work orders".
* Hide the donut chart from assistive tech
Recharts 3 turns its accessibility layer on by default, which rendered
each donut's svg as an application-role widget, and the pie's own root
group is a tab stop on top of that. So Tab from the range picker landed
on each chart in turn. The legend below already carries every label,
count and share, so the chart is a visual duplicate: the layer is off,
the frame is aria-hidden, and the pie's root is out of the tab order.
The legend is the chart's accessible representation.
A failed request now renders as an alert, so the failure is read out at
once where the polite subtitle would only fall silent.
* Say the failure total where a screen reader can reach it
The donut's centre total sits inside the aria-hidden frame, and the legend
below it lists slices but never their sum, so the number of failures was
announced nowhere. The Outcomes card got away with it because its total is
already in the card meta; this puts the failure total in the same place.
Summed from FAILURE_STATES rather than the drawn slices, which drop the
states that never happened.
* Drop the loading flag from the health query state
`Query<T>` only ever holds three states: nothing yet, data, or an error.
`loading` was true on exactly the state where both `data` and `error` are
null, so it carried nothing the other two fields didn't already say, and it
had to be set correctly in three separate setState calls to stay honest.
Subtitle was its only reader. It takes `error` now and shows "Loading…"
when there is neither data nor a failure to report.
Contributor
|
@midigofrank @stuartc I merged #5114 into this branch, so it now carries the range picker. I also edited 2 lines of the PR description to match this new behaviour. I did this directly in the PR body rather than a comment in case Stu picks up the review in the morning. Stu, apologies if you already started your review. I think whatever you had before this was in is still valid. Feel free to glance at the merge commit if you wish. |
7 tasks
Contributor
|
FYI Frank I have raised #5153 to unify the definition of "failed work order" across the site and also the dates we use to filter 30 days. No action needed in this PR but in case it comes up in any reviewing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A per-workflow page at
/projects/:project_id/w/:workflow_id/healthsummarising one workflow's work orders over a selectable window (last 24 hours, 7 days or 30 days, defaulting to 30): an outcomes donut, a breakdown of the failing ones by state, and a triage table grouping failures by error signature, heaviest first.Work orders rather than runs, deliberately — a run's state is immutable, so a retried failure would sit on this page forever. Retry the work order and the numbers actually move.
Data path. A LiveView (
WorkflowLive.Health) renders the shell and mounts a React component viaphx-hook="ReactComponent"; the charts fetch their own data from a cookie-authenticated JSON API (API.WorkflowHealthController), one action per chart, so a cheap chart isn't held up by an expensive one. The two donuts share the singleoutcomesreply, so their totals can't disagree.Refresh without polling. The LiveView subscribes to a new per-workflow work-order topic and pushes
health:changedwhen one settles — first change in a quiet period goes out immediately, then at most one per 30s window. The frontend refetches on that event and keeps the last numbers on screen while the new ones land. A workflow that nothing is running makes no requests at all.Query design (
Lightning.Workflows.Stats):last_activity, notinserted_at: an old work order retried today counts as the work it currently is.DashboardStats: that one batches across many workflows to dodge an N+1 and collapses every failure state into one:failedbucket — the granularity both donuts need apart.Supporting changes:
work_orders (workflow_id, last_activity), created concurrently. The existing single-column indexes made the planner pick one and heap-filter the rest.Run.state_reasons/0— one table mapping worker reasons to run states, read inbound byHandlers.CompleteRunand back out byStatsso the two directions can't drift. Replaces the inlinecasein the handler.WorkOrder.final_states/0andfailure_states/0.get_workflow_for_project/3now also excludes workflows marked for deletion (it routes throughQuery.workflows_for/1);workflow_exists_in_project?/2reuses it.export_load.exsseeds every state the page draws with a spread of real error signatures, so it doubles as fixture data.rechartsadded for the donuts.?days=validation came in via Workflow health: date range picker #5114, stacked on this branch.Authorization. The controller is its own trust boundary — the LiveView's
on_mountguards don't protect it, since a client can request any path. It re-checks:access_projectvia the same policy as the rest of the app, and returns 404 (not 403) on every failure so it can't confirm a workflow exists to someone who can't see the project. Covered by tests: anonymous, non-member, cross-project, support user, project scheduled for deletion, MFA not enrolled, workflow marked for deletion, malformed id — on both endpoints.Closes CON-106
Validation steps
Seed a project with health-page history (much smaller than the export test needs, since this page reads no logs or dataclips):
SPREAD_DAYS=45puts some history outside the 30-day window, so you can confirm the window actually excludes it.Visit
/projects/<project_id>/w/<workflow_id>/health. Check the two donut totals agree (Outcomes' red wedge = the breakdown's total), and that the triage table is sorted heaviest-first with a tip on every row.Run the workflow (or retry a failed work order) and watch "Last Updated" move within a second or two without a page reload. Retry several in a row: you should see one refresh immediately and one more when the 30s window closes, not one per work order.
Authorization: as a user who isn't a member of the project, request
/api/projects/<project_id>/workflows/<workflow_id>/health/outcomes— 404, not 403.Tests:
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code):owner,:admin,:editor,:viewer)