feat(sse): live workflow progress bars in Running section (bookshelf-t3z2w.2) #1227
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-t3z2w.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
workflow.progressSSE events from the lifecycle monitor goroutine (replay-safe — no workflow command sequence change)instance_id,done,totalfrom a batched SQL query againstinstances(ROW_NUMBER window groups by parent_instance_id)workflow-running-pushcontroller listens for the event and updates the new.wf-progress-celltd with a<meter class="wf-progress-meter">element (no inlinestyle=for CSP compliance)knownProgress map[string]ChildProgressin the lifecycle monitor — only emitted when done/total changesTest plan
engine_notifier_test.gocovering progress emission, child-only filter, and cleanup when instance disappearsworkflow_running_push_controller.test.jscovering progress event handlinginternal//* v8 ignore next */)style=attributes; uses<meter>element for CSP complianceScreenshot
See attached screenshot showing the Workflows Running section with the new Progress column and a
<meter>progress bar showing 7/10 children completed.Closes bead bookshelf-t3z2w.2 on merge.
Screenshot
Workflows Running section showing the new Progress column with a
<meter>progress bar (7/10 children completed):UI Review — PR #1227 (bookshelf-t3z2w.2)
Screenshot reviewed: attachment
e6757f72-eec9-46ef-bfe9-14c7fd3889da— confirmed PNG, 1400×900.What I see: Workflows > Settings page. Running section has a new PROGRESS column. The single running row (
scan-library-001-demo, state RUNNING) shows a filled<meter>bar taking roughly 70% fill width followed by the text "7/10". The bar is contained cleanly within the cell, vertically aligned with the other columns, and does not overflow or squash adjacent columns. History table has no Progress column (correct — historical workflows are complete). No duplicate headers or controls. Overall table rhythm is consistent with the .1 layout.[MINOR] static/css/main.css:4888 — meter fill colour uses browser-native green instead of
--accentThe
.wf-progress-meterrule sets width/height/vertical-align but does not setaccent-color. The rendered bar is OS/browser-default green (#4caf50 or similar), while the app accent token is--accent: #7c8cf8(indigo). The semantic green is not wrong —<meter>is designed to signal "good" with green — but it stands out against the dark-theme indigo palette. Addingaccent-color: var(--accent)to.wf-progress-meterwould align the fill with every other interactive control on the page and is a one-liner fix.Source cross-check:
style=attributes added (CSP-safe).remunits andvertical-align..wf-progress-meteris a scoped rule for a single native element; no bespoke parallel class system..wf-progress-cellis used in the template as a structural/target class only; no styling required and none missing.—in the{{range}}row is replaced dynamically by the Stimulus controller (confirmed by the rendered screenshot showing meter + fraction text).REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Review — PR #1227 (workflow.progress SSE +
<meter>bars)Scope: SSE isolation invariant from .1, progress payload sensitivity, SQL injection,
<meter>XSS.(1) Admin-only routing — PASS. The progress event is constructed in
internal/wfengine/engine_notifier.go(pollChildProgress) withAdminBroadcast: trueand noTargetUserID/Broadcast.internal/sse/hub.go:130 matches()is default-deny:AdminBroadcast→return sub.isAdmin; no other flag set → falls through toreturn false.sub.isAdminderives from session claims viaextractClaims(internal/sse/handler.go:32), not request-supplied. A non-admin authenticated/eventssubscriber therefore does NOT receiveworkflow.progressevents. No system-wide instance-ID leak to non-admins.(2) Payload sensitivity — PASS.
WorkflowProgressPayload(internal/sse/event.go) carries onlyinstance_id,done,total— no secrets/PII/user data.(3) SQL injection — PASS.
makeChildProgressQuery(internal/wfengine/diag_accessor.go) builds theIN (%s)clause from?placeholders (strings.Repeat("?,", n)); parent IDs pass as positionalargs. No value concatenated into SQL. Query is invoked only from the internal lifecycle monitor (pollChildProgress) — no new HTTP route/handler, no new unauth surface.(4)
<meter>render — see MINOR below.[MINOR] static/js/controllers/workflow_running_push_controller.js:78 — progress
<meter>built via innerHTML concat rather than DOM property_onProgressEventdoescell.innerHTML = '<meter ... value="' + done + '" max="' + total + '">...'withdone = payload.done || 0,total = payload.total || 0.instance_idis correctly_esc()-escaped for the selector, butdone/totalare concatenated into innerHTML unescaped. Not exploitable in practice: these values originate from server-side SQLCOUNT/SUM(...)scanned into Goints (scanChildProgressRows), marshalled as JSON numbers, and the channel is admin-only — so they are always numeric and never attacker-controlled. Still, the convention for this codebase (check #4, no-inline-style / defense-in-depth) prefers a DOM property:Number(done)coercion + setmeter.value/meter.maxas properties (or build viacreateElement) instead of an innerHTML string. Purely hardening; no correctness/security impact today.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Review — PR #1227 (workflow.progress SSE +
<meter>bars)Scope: SSE isolation invariant from .1, progress payload sensitivity, SQL injection,
<meter>XSS.(1) Admin-only routing — PASS. The progress event is constructed in
internal/wfengine/engine_notifier.go(pollChildProgress) withAdminBroadcast: trueand noTargetUserID/Broadcast.internal/sse/hub.go:130 matches()is default-deny:AdminBroadcast→return sub.isAdmin; no other flag set → falls through toreturn false.sub.isAdminderives from session claims viaextractClaims(internal/sse/handler.go:32), not request-supplied. A non-admin authenticated/eventssubscriber therefore does NOT receiveworkflow.progressevents. No system-wide instance-ID leak to non-admins.(2) Payload sensitivity — PASS.
WorkflowProgressPayload(internal/sse/event.go) carries onlyinstance_id,done,total— no secrets/PII/user data.(3) SQL injection — PASS.
makeChildProgressQuery(internal/wfengine/diag_accessor.go) builds theIN (%s)clause from?placeholders (strings.Repeat("?,", n)); parent IDs pass as positionalargs. No value concatenated into SQL. Query is invoked only from the internal lifecycle monitor (pollChildProgress) — no new HTTP route/handler, no new unauth surface.(4)
<meter>render — see MINOR below.[MINOR] static/js/controllers/workflow_running_push_controller.js:78 — progress
<meter>built via innerHTML concat rather than DOM property_onProgressEventdoescell.innerHTML = '<meter ... value="' + done + '" max="' + total + '">...'withdone = payload.done || 0,total = payload.total || 0.instance_idis correctly_esc()-escaped for the selector, butdone/totalare concatenated into innerHTML unescaped. Not exploitable in practice: these values originate from server-side SQLCOUNT/SUM(...)scanned into Goints (scanChildProgressRows), marshalled as JSON numbers, and the channel is admin-only — so they are always numeric and never attacker-controlled. Still, the convention for this codebase (check #4, no-inline-style / defense-in-depth) prefers a DOM property:Number(done)coercion + setmeter.value/meter.maxas properties (or build viacreateElement) instead of an innerHTML string. Purely hardening; no correctness/security impact today.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
[MINOR] internal/wfengine/engine_notifier_test.go:271–275 — "includes the instance ID in the progress payload" test violates one-Expect-per-It rule
The test executes two Expect calls (checking type assertion ok, then InstanceID value) but should have one assertion per It block. Remove the first
Expect(ok).To(BeTrue())call and let the type assertion panic if it fails (acceptable in tests), or split into two separate It blocks with shared setup. Fix: consolidate to a single Expect by dropping the explicit ok check.Verification Summary
✓ Replay safety: Progress events are emitted from the monitor goroutine (pollChildProgress), not from any workflow body. No ExecuteActivity or workflow commands added to the sequence.
✓ Query bounded & deterministic: childProgressSQL uses
WHERE parent_instance_id IN (...)with a bounded parent-ID list (filtered from currentIDs). Single query per 2s poll tick. ORDER BY includes tiebreaker (id DESC) making it deterministic.✓ AdminBroadcast routing: Progress events set
AdminBroadcast: truematching state events. Hub.Publish enforces default-deny via matches() — only admins receive these events.✓ JS handler idempotent + CSP-safe: _onProgressEvent() replaces cell content via
cell.innerHTML, handles zero/missing totals gracefully, uses<meter>class (no inlinestyle=), and escapes instance_id via existing _esc() function.✓ Cleanup: knownProgress map is properly deleted when instances terminate (pollLifecycle line 306).
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
- CSS: add accent-color: var(--accent) to .wf-progress-meter so the meter fill uses the app indigo instead of browser-default green - JS: refactor _onProgressEvent to create-or-reuse <meter> via DOM properties (meter.value / meter.max) instead of innerHTML string concat; fraction label set via sibling <span> textContent; idempotent on repeated events (no element recreation) - Test: update vitest assertions to read meter.value/meter.max DOM properties; add label-textContent and idempotency tests - Test (Go): consolidate two-Expect It("includes the instance ID...") to single Expect by using direct type assertion, per one-Expect-per-It Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Re-Review: Fix Delta (47241bb9..b15d2964)
Meter DOM-property refactor (static/js/controllers/workflow_running_push_controller.js:68–86): The
<meter>element is now created viadocument.createElement("meter")and updated via.value/.maxDOM properties (Number-coerced) with the label set viatextContenton a sibling span. NoinnerHTMLsink remains. The meter is idempotently reused on repeat progress events (query-find-or-create pattern). Zero/missing-total handled viacell.textContent = "—". ✓ Security minor resolved.CSS accent-color (static/css/main.css:4893): Added
accent-color: var(--accent);to.wf-progress-meter. Uses the CSS token defined at:root(line 9), no hardcoded color, no inline style attribute. ✓ UI minor resolved.Go test consolidation (internal/wfengine/engine_notifier_test.go:273): Type assertion now a guard; single
ExpectperItblock assertingp.InstanceID. Idiomatic Go—panic on failed assertion is a valid precondition guard, not an Expect. ✓ Test hygiene minor resolved.Vitest coverage: Assertions refactored from
getAttribute()to DOM properties (meter.value,meter.max). Added test for label text inspan.textContentand test for meter-element idempotency across repeated progress events. All coverage maintained; JS unit tests pass.CI status: All green (Test Race, Coverage, Lint, JS Unit, Integration, E2E API, E2E Browser).
REVIEW VERDICT: 0 blocker, 0 major, 0 minor