Add a workflow health page - #5108
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5108 +/- ##
=======================================
- Coverage 90.6% 90.6% -0.0%
=======================================
Files 422 425 +3
Lines 20043 20078 +35
=======================================
+ Hits 18160 18188 +28
- Misses 1883 1890 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
lmac-1
left a comment
lmac-1
left a comment
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.
| ) do | ||
| with %_{} = user <- socket.assigns[:current_user], | ||
| %_{} = project <- Projects.get_project(project_id), | ||
| %_{} <- Projects.get_project_user(project, user), |
There was a problem hiding this comment.
Support users have no project_users row, so this refuses them where other channels admit them. Worth going through Permissions.can(ProjectUsers, :access_project, ...) instead, it also covers projects with scheduled_deletion set.
| socket | ||
| ) do | ||
| with %_{} = user <- socket.assigns[:current_user], | ||
| %_{} = project <- Projects.get_project(project_id), |
There was a problem hiding this comment.
A malformed project_id crashes this instead of erroring cleanly. Repo.get raises Ecto.Query.CastError on a bad UUID, it only returns nil for a valid one that doesn't exist.
Other lookups guard with valid_uuid?/Ecto.UUID.cast first (job.ex, channel_proxy_plug.ex, workflow_channel.ex), should do the same here.
| with %_{} = user <- socket.assigns[:current_user], | ||
| %_{} = project <- Projects.get_project(project_id), | ||
| %_{} <- Projects.get_project_user(project, user), | ||
| true <- Workflows.workflow_exists_in_project?(project.id, workflow_id) do |
There was a problem hiding this comment.
I noticed that this join does quite a few separate DB lookups, including one to check if the workflow exists and then a separate one to fetch it. Is there a reason those aren't combined into a single query? Also the project fetch pulls in :parent, which doesn't look like it's used here.
| select: {wo.state, count(wo.id)} | ||
| ) | ||
| |> Repo.all() | ||
| |> Enum.reduce(%{success: 0, failed: 0, pending: 0}, fn |
There was a problem hiding this comment.
This is the same rule as DashboardStats.count_workorders/1 (dashboard_stats.ex:239-248). CON-112 is considering changing this - cancelled will not be counted as failed. If that happens, the rule would need updating in both places, and there is nothing here to tell you the other copy exists.
Would it be worth putting the rule in one place, in Lightning.WorkOrder next to active_states/0?
def outcome(:success), do: :success
def outcome(state) when state in @active_states, do: :pending
def outcome(_state), do: :failedThen both reducers would just be Map.update!(acc, WorkOrder.outcome(state), &(&1 + count)).
There was a problem hiding this comment.
I decided to completely isolate it. I din't want it sharing anything with the DashboardStats so that we can modify it as we like
|
Thanks @lmac-1 . Looks like you forgot to push your changes 👀 |
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.
|
Thanks @lmac-1 , I have implemented the changes. Regarding your two questions:
|
2 participants
Description
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.
Closes _
Validation steps
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)
(e.g.,
:owner,:admin,:editor,:viewer)