Skip to content

Surface recorded failures in a notification bell (Review Flow PR 4) - #7478

Open
EthanHealy01 wants to merge 3 commits into
feature/failure-read-scopefrom
feature/failure-notifications
Open

Surface recorded failures in a notification bell (Review Flow PR 4)#7478
EthanHealy01 wants to merge 3 commits into
feature/failure-read-scopefrom
feature/failure-notifications

Conversation

@EthanHealy01

@EthanHealy01 EthanHealy01 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Flow PR 4 of 5. Stacked on #7477. Recorded failures appear in a notification bell, showing each reader the failures they are allowed to see and the actions they can actually take.

Scope is deliberately viewing and triage only. Resolving a failure — retry, decrypt-and-retry — is #7479.

What's added

A notification bell in the editor and the processor shell. Polls GET /api/v1/notifications every 30 seconds, shows an unread badge, and lists open failures newest first. Each row shows the failure's title, its message with Copy error and Show full message chips, an occurrence count, and its available actions.

A notification API (stirling.software.proprietary.notification), derived from failures on read rather than stored in its own table:

Route Purpose
GET /api/v1/notifications the caller's open failures, newest first
POST /api/v1/notifications/{id}/actions/{actionId} apply an offered action
POST /api/v1/notifications/{id}/resolved record that a client-side retry worked

Every id is prefixed (failure:<uuid>); none of these routes accepts a raw failure id.

Per-reader actions. A FailureKind declares each action with an audience (OWNER, TEAM_REVIEWER, ANYONE_WHO_SEES). The server resolves that against the reader and derives Ownership (MINE / THEIRS / UNOWNED) from the row's actor, so an admin reviewing someone else's failure is not offered a document their browser does not hold. Adding a failure kind requires no frontend change.

Server-run and client-run actions are distinguished. FailureActionId carries an Execution facet; the registry requires a bean only for server actions, and dispatching a client action returns 400.

Actions available on this PR: View file (opens the document in the editor), View in processor (dev builds only), Dismiss. The server also declares Retry and Decrypt and retry on the password-protected kind; this build wires no handler for them, so the bell skips them rather than rendering a dead button. #7479 adds the handlers.

Attended policy runs record their document. POST /api/v1/policies/{id}/run accepts an optional opaque fileId, recorded when the run carries exactly one primary document. This is what lets a repeat fold onto one incident instead of opening a new one per upload, lets deleting the file clear its failure, and lets the owner open the document from the row.

A RESOLVED status transition, system-set when a client-side retry succeeds.

Behaviour changes

  • The bell re-reads as soon as a failure you caused is recorded, rather than leaving you to wait out a poll interval for news of your own upload. Applies to a failed tool run and to a policy run reaching FAILED. Other people's failures still arrive on the poll, which is what it is for.
  • An action the reader cannot use is not rendered. Where the server gave a reason for withholding it, that reason appears as the row's one-line note. An action that was never offered to that reader produces no note.
  • The failures list in /processor/documents stays behind import.meta.env.DEV, and View in processor is gated to match so it cannot navigate to a section that is not mounted. Both lift when failures get their own review screen.
  • One poll for all bells. The bell is mounted in three places; the list, document lookups and read marker are shared, so mounting more than one does not multiply requests.
  • ACKNOWLEDGE is no longer offered by any kind. The id, bean and status remain so existing rows stay readable.

Known limits

  • The poll does not pause when the tab is hidden.
  • No retention or per-team cap on file_run_events.

How to test

Needs a proprietary or SaaS build with login enabled. task dev:all, then sign in.

  1. Create a failure. Add a password-protected PDF to the editor and choose Skip for now when it asks to unlock. The upload starts a policy run that fails on it.
  2. Watch the bell. The badge should appear within a second or two, not after 30 — this is the refresh-on-failure path. Open it: a row titled "Password-protected document" with the error message and the two chips.
  3. The buttons should be View file, View in processor and Dismiss. Decrypt and retry is absent by design: the server offers it, this build has no handler, so it is skipped rather than shown dead.
  4. View file closes the panel and selects that document in the editor.
  5. Dismiss removes the row. In DevTools the request is POST /api/v1/notifications/failure%3A<uuid>/actions/DISMISS — note the prefixed id.
  6. Confirm the local-document probe. Create a second failure, then delete that file from the editor and reload. That row now reads "This document is not on this device..." and loses View file, while a row whose document is still present keeps it.
  7. Confirm attribution end to end. Sign in as a plain member, run a shared policy on your own upload so it fails. The member sees their own row in the bell. Sign in as the team leader: they see it too, but without View file, because the document is not in their browser.
  8. Confirm folding. Add the same locked PDF again and skip again. The existing row's occurrence count increases rather than a second row appearing.
  9. Confirm one poll for many bells. Open the editor and the processor in two tabs. Each tab issues its own poll, but within a tab the several mounted bells share one — the Network tab should show one GET /api/v1/notifications per 30s per tab, not three.

Migration

None. No new column and no new value in any CHECK-constrained enum; CheckConstrainedEnumsTest fails if that changes.

@EthanHealy01
EthanHealy01 requested review from a team and Ludy87 as code owners August 12, 2026 23:24
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines ignoring generated files. enhancement New feature or request labels Aug 12, 2026
@stirlingbot stirlingbot Bot added the has conflicts Pull request has merge conflicts with the base branch label Aug 12, 2026
Comment thread frontend/editor/src/core/services/notificationRetry.ts Outdated
@EthanHealy01
EthanHealy01 force-pushed the feature/failure-notifications branch from 062a1b9 to dff3d86 Compare August 13, 2026 00:51
@stirlingbot stirlingbot Bot removed the has conflicts Pull request has merge conflicts with the base branch label Aug 13, 2026
Comment thread frontend/editor/src/proprietary/components/notifications/notificationActions.ts Outdated
"notifications.noDocumentLinked",
"This failure is not linked to a specific document, so it cannot be opened or retried here.",
);
return isResolvableHere(notification)

@aikido-pr-checks aikido-pr-checks Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noteFor returns "not on this device" when isResolvableHere(notification) is true, which inverts the condition and can show the opposite reason to users.

Suggested change
return isResolvableHere(notification)
return !isResolvableHere(notification)
Details

✨ AI Reasoning
​The function builds explanatory notes for why actions are unavailable. After ruling out non-owned rows, local-file-present rows, and missing file ids, it decides whether to show a "not on this device" message. That message is currently returned when the predicate indicates resolvability in the current environment, which contradicts the meaning of the text being shown. This creates a path where users receive the opposite explanation from the actual condition.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

@EthanHealy01
EthanHealy01 force-pushed the feature/failure-notifications branch 2 times, most recently from f495edc to 29d124e Compare August 13, 2026 21:27
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines ignoring generated files. and removed size:XXL This PR changes 1000+ lines ignoring generated files. labels Aug 13, 2026
A bell in the editor and the processor lists the failures the reader is
allowed to see, each with the actions they can actually take: open the
document, view the run in the processor, dismiss. Resolution (retry,
decrypt-and-retry) is a separate PR; the server declares those actions here and
a build with no client handler for them skips them.

Actions carry an audience (owner / team reviewer / anyone) and the server
derives the reader's ownership of each row, so an admin reviewing someone
else's failure is not offered a document their browser does not hold. Order is
the kind's declaration order, first leading.

An attended policy run now carries the client's own document reference, which
is what lets a repeat fold onto one incident, lets deleting the file clear its
failure, and lets the owner open the document from the row.

The bell also re-reads as soon as a failure this user caused is recorded,
rather than leaving them to wait out a poll interval for news of their own
upload. Other people's failures still arrive on the poll.
@EthanHealy01
EthanHealy01 force-pushed the feature/failure-notifications branch from 29d124e to ae906e7 Compare August 13, 2026 21:29
… feature/failure-notifications

# Conflicts:
#	frontend/editor/public/locales/en-US/translation.toml
#	frontend/editor/src/core/components/layout/Workbench.tsx
#	frontend/editor/src/portal/components/AppShell.tsx
… document

Deleting a document from the editor closed only the incidents the editor had
reported itself. A policy run the same upload triggered was recorded by the
processor, so its failures stayed in the queue about a document that no longer
exists anywhere, and the only way to clear them was to dismiss each one by hand.

The guard was keyed on origin, which is not the question. The question is which
of the two id spaces the row's fileId is in: an editor report and an attended
policy run both carry the id the client minted for its own document, so the
client that holds it can say it is gone; a source-fed run carries a one-way hash
of a path or key that was never on any device, so no client can name it. That is
the absence of a source, not the origin, and it is the same rule the frontend
already uses to decide whether a row is resolvable locally.

Losing the origin clause does not widen anything: the actor and team clauses
still hold. It matters most where the actor clause cannot help, which is a
login-disabled deployment where both sides are null and every unattended row
would otherwise match; that case is now pinned by its own test.

Both tests run against the real database, because the scoping lives entirely in
the JPQL and the in-memory fake implements the same rules by hand -- it would
agree with a wrong query.
@github-actions

Copy link
Copy Markdown
Contributor

🚀 V2 Auto-Deployment Complete!

🔗 Direct Test URL (non-SSL) http://54.175.155.236:7478

🧩 Admin portal included - try it at http://54.175.155.236:7478/portal.

This deployment will be automatically cleaned up when the PR is closed.

🔄 Auto-deployed for approved V2 contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XL This PR changes 500-999 lines ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant