fix(workflows): keep deletion on the grid with toast recovery - #328
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review
Published via Wes's account. Reviewed head 34aa9d3097868c159c0b9b45e972ed6b30b918c8 against base 85d6bf82c54d1c8d930d58444597a1fe31cc8975.
One low-severity UI contract finding, attached inline: a refresh can remove the only pending-deletion indicator before its receipt arrives. This does not falsely confirm deletion or permit a duplicate command.
I traced grid/detail confirmation, editor disposal, landing read ownership, toast recovery, durable dismissal, and session/outbox integration. Mantis independently reviewed receipt/readback ordering and clear/interruption fencing, then corroborated the UI finding. I found no additional actionable defects in those paths. The change reuses the existing session, reader, outbox and toast ownership rather than adding a backend or delivery mechanism.
Validation boundary: source only; 33 downloaded source/reference files were hash-checked against the pinned head's tree. No PR code, tests, app launches, or live workflow operations were executed in this review. One remote checks snapshot at this head showed 13 successful checks and Windows native validation skipped, including successful CI required and all six Chromium/WebKit journey shards. That snapshot is not proof of the uncovered ordering or live/native acceptance. The PR description lists human tryout and live/packaged-native acceptance as pending; this review does not establish them. This is a COMMENT review, not approval or merge authorization.
| !snapshot || | ||
| snapshot.status === "idle" || | ||
| snapshot.status === "unavailable" || | ||
| operation.outcome === "pending" || |
There was a problem hiding this comment.
[P3] Preserve pending-deletion feedback when refresh removes its row
This unconditional pending early return assumes the card still exists. However, the enabled Refresh workflows action can complete a read after the relay applies the deletion but before its receipt reaches the client. WorkflowLanding.tsx:157–174 replaces the copied items on every ready result and :648 renders cards only from those items. An absent row therefore removes the card and its Deleting… status, while this branch suppresses the only other deletion presentation. For that window the grid gives no indication that the command is still pending, contrary to docs/workflows.md:78.
Keep the pending card's presentation when a read omits its coordinate, without treating that retained presentation as readback evidence; an equivalent non-dismissable pending notice would need an explicit adjustment to the card contract. Add a regression that holds the receipt, returns a ready empty definition read, and still observes pending feedback with no repeat/dismiss actions before releasing the receipt. Preserve the existing receipt-plus-fresh-read confirmation fence. This is a temporary UI gap, not a false-success or duplicate-publication defect.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One detail-deletion recovery finding; see inline comment.
| definition.owner !== viewer || | ||
| !capability.availability.delete | ||
| ) | ||
| return; |
There was a problem hiding this comment.
🤖 [P3] Recover when detail deletion is refused by the operation lock
After a detail save receives its receipt and exact readback, return to the grid and reopen the old card before the landing reread finishes. Open/Edit is not disabled by the save-revision lock, so this editor can retain the old draft.original and offer Delete. Confirming calls WorkflowChannel.remove(), which clears the draft before this callback runs; workflowOperationLocked then rejects the stale revision and returns silently. No deletion is submitted, no error notice appears, and the now-empty editor stays open instead of returning to the grid as promised in docs/workflows.md. This is a source-traced ordering case, not a runtime reproduction; server data remains intact.
Use the same lock in the editor before discarding its draft, or make this refusal follow the existing error recovery path (deleteError plus openChannel("")). Add a regression holding the landing refresh after a successful save, reopening the old card, and confirming deletion.
704239b to
53112d6
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
No new actionable findings in this bounded follow-up. Reviewed head 53112d64166b55fe49842f1c572e4bf996f0636a against base 1d6aee0896228f18a3f427ca65beab63906b65e7, comparing the fixes and affected integration with the previous review at 34aa9d3097868c159c0b9b45e972ed6b30b918c8.
- The prior pending-feedback gap is addressed in source: when a read removes the workflow row before the receipt arrives,
WorkflowDeletionNotices.tsx:79–96retains a non-dismissable “Deleting workflow…” notice.Toast.tsx:85–108gives it no expiry or dismiss action, and the revised component regression holds the pending operation while returning empty definitions before finishing it. The existing browser entry-point journeys retain the disabled-card/delete assertions removed from that component scenario (workflows.journey.mjs:803–831). WorkflowsPage.tsx:120–148now returns to the grid with an explanation when an unresolved change prevents deletion, without publishing another command. Traced this through the detail editor’s draft disposal and the new zero-delete regression.- Checked the deletion integration with inherited paginated/session-cached reads: accepted-receipt evidence is captured before the fresh read; complete coordinate absence, receipt ordering, cache invalidation, queued readback, and interruption/disposal fences remain in place. The four-file fix commit is distinguished from inherited base changes; unchanged unrelated areas were not reopened. All five PR commits contain DCO sign-off trailers.
Validation limits: 66 immutable source extracts verified against Git blob hashes; no dirty working-tree inputs. Tests and callers were read, not executed. No builds, installs, app/native launches, live-relay writes, or deletion workflow exercise. One hosted CI snapshot showed JavaScript, Rust/tool integration, browser measurements, Chromium shards 2/3 and 3/3, Semgrep, zizmor and DCO successful; Chromium 1/3 and all three WebKit shards were still running, and Windows native validation was skipped. No CI polling. Browser/native behavior and human acceptance remain unverified; this is not an all-green CI claim.
This non-blocking COMMENT is not an approval or merge authorization.
53112d6 to
710b7c5
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
One actionable P2 integration finding, inline: both deletion entry-point journeys still wait for the landing’s pre-rebase status copy, so they cannot reach the final recovery/no-repeat assertions with the current renderer.
Reviewed head 710b7c5765d38a1f689a9175c165909b9fc56ce7 against base 258c6d6b0b591e7ad478abea6c3cb255e9452ae6, as a bounded follow-up to 53112d64166b55fe49842f1c572e4bf996f0636a. The prior pending-notice and locked-deletion recovery fixes remain intact. I traced the rebased card/editor-menu composition, deletion submission ownership, retained landing/readback, and changed component/browser coverage. No additional production defect found in that integration scope; unrelated incoming main features were not reopened.
Validation limits: source-only; 1,617 extracted regular-file Git blobs and SHA-256 hashes verified, with no dirty worktree inputs. No PR code, tests, builds, installs, app launches, or live deletion operations executed. The inline failure is established from the source mismatch, not a locally reproduced browser run.
One hosted CI snapshot showed JavaScript failed, security/DCO and browser measurements passed, all six functional browser shards and Rust/tool integration still running, and Windows native validation skipped. JavaScript’s available annotations only report exit code 1; the job-log read was unavailable, so I am not attributing that failure to this finding. No CI polling.
The measurement artifact runs synthetic merge ec2a3da64d88c13d714d5973fd801c500e6b2737, not raw head: 7/7 passed, ~183.1 s wrapper wall / ~170.2 s summed execution; slowest was Chromium cursor paging (~81.8 s), with scroll.spec.mjs ~123.3 s and channel-opening.spec.mjs ~46.9 s. Those unrelated measurements do not validate workflow deletion, and no before/after performance conclusion is established. Native/live-relay behavior and current-head human acceptance remain unverified. This is a non-blocking COMMENT, not approval or merge authorization.
| await expect( | ||
| page.getByText( | ||
| "Workflow scan finished. Lists may be limited by the relay.", | ||
| { exact: true }, | ||
| ), | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
[P2] Update the deletion journeys to the rebased landing status
The rebase retains this exact-text lookup inside the for (const from of ["grid", "detail"]) deletion tests, but WorkflowLanding.tsx:778 now renders “Workflows loaded.” after the scan; the other completion assertions in this same journey file were updated to that text. After successful deletion removes the card, neither entry-point case can find the old string, so both time out before checking the absence of the recovery notice, reopening Create workflow, and the final single-publication assertion. tests/browser/workflows.spec.mjs imports this file into the ordinary Chromium/WebKit lanes.
Change this assertion to the current completed-scan status (preferably scoped to the landing status), preserving the lifecycle barrier and all subsequent checks. Validate the complete workflow journey file in both engines. This is a source-demonstrated rebase mismatch; I did not execute the tests.
Reconcile empty successful deletion receipts with complete fresh configuration reads, clear confirmed editor state, and refresh the landing. Preserve uncertain and legacy outcomes, fence queued reads across clear/interruption, and cover deletion ordering and recovery with focused regressions. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Use direct destructive confirmation and deletion-specific pending, checking, retained, and recovery copy. Share the existing complete-read success predicate with activity messages, disclose delivery details, and show submission errors in the confirmation. Preserve receipt reconciliation and editor cleanup; cover changed UI behavior and update gallery and browser assertions. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Keep grid confirmation on the grid and return there immediately after confirming in detail. Let the session and mounted landing reconcile deletion independently, retain pending cards and action locks, and surface submission, rejection, and uncertain outcomes through actionable toasts. Preserve complete fresh readback and durable dismissal safeguards, remove editor deletion progress, and cover both entry points, failure recovery, and duplicate prevention with component and browser regressions. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Replace a rejected deletion's toast when another attempt for the same workflow is active or confirmed, without assuming chronological outbox ordering. Preserve uncertain recovery and cover rejection, retry, verified echo, and successful readback through the real session. Signed-off-by: Matt Toohey <contact@matttoohey.com>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
710b7c5 to
4626a0e
Compare
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
No new actionable findings in this bounded follow-up. Reviewed head 7862abc43a3cf5de8aa7db0fb7dbf9bc60bff553 against base 3a19fa43075283423c88a68d4a1362fade28ad3e, following the previous review.
- The prior stale completion-text finding is fixed:
workflows.journey.mjs:1051now expects the landing’s current “Workflows loaded.” status. - The detail journey waits for completed landing discovery before arming its held editor read (lines 963–974). The fixture gate remains explicit and the existing pending-lock, immediate-return, confirmed-removal, and no-repeat assertions are retained. No browser cases were added or removed in this follow-up.
- Direct tree comparison with the previously reviewed head found only the two workflow test/fixture files plus the inherited page-error collector migration; the latter matches the current base. Production deletion, receipt/readback, and recovery code is unchanged. I traced the affected integration and success/cancel/error/retry ownership; native keyboard focus behavior remains unexercised.
Validation limits: source-only review; no local tests, builds, app launches, or live workflow operations. One hosted CI snapshot showed JavaScript, measurements, security, and DCO success; six browser shards and Rust/tool integration were still running, and Windows validation was skipped. The measurement log reports 7 passing cases in 2.5 minutes, on synthetic merge 0a7d611 with newer base ea7ddb81, not the pinned base above. This is not an all-green or runtime-acceptance claim.
Public-material check: reviewed the public PR description, changed source/docs/fixtures, and six commit messages without identifying an internal coordination-link or secret leak in that text. The description’s validation head/base and draft wording refer to an older revision (optional documentation cleanup). Its four attached MP4 recordings downloaded, but I could not visually inspect their frames with the available media tooling; attachment privacy and visual acceptance therefore remain unverified.
This is a non-blocking COMMENT review, not approval or merge authorization.
Bug and Fix
Deleting from a workflow card opened the detail editor, and users had to remain there while deletion ran. Successful removal could leave an unknown result and a stale "Leave this draft?" warning. Failures could then be stranded in the editor.
The combined branch now keeps the complete deletion interaction on the workflows grid:
Owner permissions, session isolation, complete-read/receipt ordering, clear/interruption fencing, and the durable outbox remain in place. Work already running may continue. No backend or
block/buzzchanges.Related issue
Continuation of this draft PR and the workflow-deletion feedback; no separate issue linked.
Validation
Final HEAD:
34aa9d3097868c159c0b9b45e972ed6b30b918c8. Commits in this delivery:f9c7b72458858693d077350abfcc30157eb72405and34aa9d3097868c159c0b9b45e972ed6b30b918c8, combined withe5883fd5andb330c32f. Actual PR base:85d6bf82c54d1c8d930d58444597a1fe31cc8975.bin/pnpm exec playwright test --config src/bundled/workflows/workflows.playwright.config.mjs.85d6bf82mounts one detail editor behind grid confirmation; final code mounts none. The rejection → retry → verified echo → receipt/readback journey reproduced a stale failure toast in both engines before the final correction; both complete files pass afterward.Before
Actual PR base
85d6bf82, 40.36 seconds; same isolated synthetic relay. Card deletion opens detail and uses the old "Request deletion" confirmation. Progress says "Saving…". After an empty successful receipt removes the definition, the editor stays locked with an unknown outcome. Closing prompts "Leave this draft?"; leaving and manually refreshing finally removes the card.before.mp4
After
Grid deletion — final HEAD
34aa9d30, 24.04 seconds. Confirmation stays on the grid; no detail editor is mounted. The pending card stays visible and locked. Complete readback removes it automatically with one deletion publication.grid.mp4
Detail deletion — final HEAD
34aa9d30, 30.00 seconds. Confirm from the open editor, return immediately to the grid while deletion is pending, then remove the card after confirmation. No leave-draft warning or editor progress.detail.mp4
Rejection and recovery — final HEAD
34aa9d30, 31.60 seconds. One deletion is rejected. A readable failure toast appears, the card remains, and its actions become usable. Explicit dismissal clears the notice without repeating deletion. Retry and echo ordering are covered by the automated browser journey above.failure.mp4
All four recordings use the production UI/session/outbox, in-memory synthetic relay data, and disposable keys. Baseline production sources were freshly extracted from the actual PR base; only the synthetic transport fixture is shared. Each clip publishes exactly one deletion, with zero page errors. H.264, 1280×960, deliberate reading pauses and a cursor/click indicator. Frames and local playback/seeking were verified before upload. All four GitHub inline players were then verified: playback advanced and seeking worked at 30%, 65%, and 90%, with no media errors. Helpers/media are outside committed code. No real workflow was deleted.
Refresh label finding
The running recording fixture at
http://127.0.0.1:1436/src/bundled/workflows/session-fixture.html?writeshas an icon witharia-label="Refresh workflows", empty visible text, and no title/tooltip. "Refresh discovered Buzz project channels…" is absent from the inspected DOM, current source/fixture, and prior local-history search, so its provenance is still unestablished. Old Buzz used the same compact "Refresh workflows" icon calling workflow refetch. This PR preserves that label/behavior and does not introduce channel-discovery copy or architecture changes.Remaining Acceptance
just scanwas not run; hosted CI supplies broad validation. Required human/code-owner review is still outstanding.