Skip to content

Commit 49c1e75

Browse files
authored
Surface recorded failures in a notification bell (Review Flow PR 4) (#7478)
Review Flow PR 4. 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 routing only. Resolving a failure — retry, decrypt-and-retry — is #7479, which also brings the write path for it; nothing resolution-shaped ships here, not even dark. ## 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 | Read-only by design: every action the bell offers is one the client runs on its own device, so there is nothing to post back. Every id is prefixed (`failure:<uuid>`), so the bell never holds a raw failure id it could hand to a failure endpoint. **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 on the failure surface returns 400. The notification projection goes further: it carries only client-run offers, so the bell cannot be sent a button it would refuse to draw. **Actions in the bell:** at most two. The owner of the document gets **View file** (opens it in the editor); a team reviewer gets **View in processor** (dev builds only). Dismiss stays on the failure queue in `/processor/documents` — deciding a failure's fate belongs to the review surface, not the panel that announces it. An action id the build has not wired is skipped rather than rendered dead, so the server can ship new kinds ahead of the clients that understand them. **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. ## 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. - **Deleting a document closes every incident about it that the deleter caused**, including a failed policy run on their own upload, so a user's own errors leave the bell with the file rather than lingering with a dead button. - **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 and View in processor, nothing else.** No Dismiss and no retries: dispositions live on the review surface, resolutions in #7479. 4. **View file** closes the panel and selects that document in the editor. 5. **Dismiss from the queue instead.** Open `/processor/documents` (dev build), find the row in the failures list and dismiss it there; the bell drops it on its next read. 6. **Confirm the local-document probe.** Create a second failure, then delete that file from the editor and reload. Its incident closes with it; a row whose document is still present keeps **View file**. 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 with **View in processor** instead of **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.
1 parent 826e487 commit 49c1e75

72 files changed

Lines changed: 4245 additions & 227 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,31 @@
11
package stirling.software.proprietary.failure;
22

3+
import org.springframework.http.HttpStatus;
4+
35
import lombok.Getter;
46

5-
/**
6-
* Why an action could not be dispatched. Carries a {@link Reason} rather than an HTTP status, so
7-
* the service stays web-agnostic and the controller owns the mapping.
8-
*/
7+
/** Carries a {@link Reason} rather than an HTTP status, so the service stays web-agnostic. */
98
@Getter
109
public class FailureActionException extends RuntimeException {
1110

1211
public enum Reason {
1312
/**
14-
* No such event, it belongs to another team, or the caller's team did not resolve. One
15-
* reason for all three, so the response does not vary with which it was. Unrelated to
16-
* {@link FailureKind#UNKNOWN}, which is an unclassified failure rather than a refused
17-
* action.
13+
* No such event, another team's, or an unresolved team: one reason, so the answer cannot
14+
* vary.
1815
*/
1916
EVENT_NOT_FOUND,
2017

2118
/** The action id is not in the vocabulary at all, or has no registered handler. */
2219
ACTION_NOT_RECOGNISED,
2320

2421
/**
25-
* The action exists but this kind does not declare it, so an incoherent pairing (releasing
26-
* a document whose destination is what failed) cannot be dispatched even by hand.
27-
*
28-
* <p>Unreachable today: both kinds declare both actions, so no request can trip this guard
29-
* until a kind ships with a restricted action set. Declared now because the guard must
30-
* exist before that kind does, not after.
22+
* The action exists but this kind does not offer it, so it cannot be dispatched by hand.
3123
*/
3224
ACTION_NOT_DECLARED,
3325

26+
/** Offered, but the client is what runs it, so refused rather than half-performed. */
27+
ACTION_NOT_DISPATCHABLE,
28+
3429
/** The event is already closed, so no further transition is possible. */
3530
ALREADY_CLOSED
3631
}
@@ -41,9 +36,21 @@ public FailureActionException(Reason reason, String message) {
4136
this(reason, message, null);
4237
}
4338

44-
/** For a refusal that follows from a lower-level failure, so its stack is not dropped. */
4539
public FailureActionException(Reason reason, String message, Throwable cause) {
4640
super(message, cause);
4741
this.reason = reason;
4842
}
43+
44+
/**
45+
* Lives with the reasons it maps, so every surface that dispatches an action answers alike. A
46+
* closed row is a conflict, not a bad request: it was well-formed and valid a moment earlier.
47+
*/
48+
public static HttpStatus statusOf(Reason reason) {
49+
return switch (reason) {
50+
case EVENT_NOT_FOUND -> HttpStatus.NOT_FOUND;
51+
case ACTION_NOT_RECOGNISED, ACTION_NOT_DECLARED, ACTION_NOT_DISPATCHABLE ->
52+
HttpStatus.BAD_REQUEST;
53+
case ALREADY_CLOSED -> HttpStatus.CONFLICT;
54+
};
55+
}
4956
}
Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,52 @@
11
package stirling.software.proprietary.failure;
22

3+
import lombok.Getter;
4+
35
/**
4-
* The actions a {@link FailureKind} may declare. Both are incident dispositions: they change how
5-
* the event is shown and touch nothing else, which is what makes them valid for every kind
6-
* including {@link FailureKind#UNKNOWN}, and why there is no {@code APPROVE} yet.
6+
* The actions a {@link FailureKind} may declare. Client actions are declared here rather than
7+
* invented per client, so the server keeps deciding what a kind offers, in what order and labelled
8+
* how.
79
*/
10+
@Getter
811
public enum FailureActionId {
9-
ACKNOWLEDGE,
10-
DISMISS
12+
13+
/**
14+
* Kept in the vocabulary for as long as any persisted row is {@code ACKNOWLEDGED}: such rows
15+
* must stay readable and closable whether or not any kind currently offers this.
16+
*/
17+
ACKNOWLEDGE(Execution.SERVER, "Acknowledge"),
18+
19+
DISMISS(Execution.SERVER, "Dismiss"),
20+
21+
/** Open the document behind the incident, in whichever client can resolve its id. */
22+
VIEW_FILE(Execution.CLIENT, "View file"),
23+
24+
VIEW_IN_PROCESSOR(Execution.CLIENT, "View in processor");
25+
26+
/** Dispatch refuses a {@code CLIENT} id, so this is enforced rather than merely documented. */
27+
public enum Execution {
28+
29+
/** {@link FailureActionRegistry} requires a {@link FailureAction} bean for these. */
30+
SERVER,
31+
32+
/**
33+
* Declared and rendered, never dispatched: the server has neither the file nor the tool.
34+
*/
35+
CLIENT
36+
}
37+
38+
private final Execution execution;
39+
40+
/** English fallback, for a client with no translation for the label key. */
41+
private final String defaultLabel;
42+
43+
FailureActionId(Execution execution, String defaultLabel) {
44+
this.execution = execution;
45+
this.defaultLabel = defaultLabel;
46+
}
47+
48+
/** Also whether it can be dispatched. */
49+
public boolean runsOnServer() {
50+
return execution == Execution.SERVER;
51+
}
1152
}

app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
* Resolves a {@link FailureActionId} to the bean that implements it. The startup check is the
1717
* point: because kinds declare action ids as data, one could name an action nobody implements,
1818
* which would otherwise show up as a button that 400s rather than as a failed boot.
19+
*
20+
* <p>Only {@link FailureActionId.Execution#SERVER} ids belong here: a bean for a client action is
21+
* refused, because dispatch could never reach it.
1922
*/
2023
@Slf4j
2124
@Service
@@ -25,6 +28,14 @@ public class FailureActionRegistry {
2528

2629
public FailureActionRegistry(List<FailureAction> actions) {
2730
for (FailureAction action : actions) {
31+
if (!action.id().runsOnServer()) {
32+
throw new IllegalStateException(
33+
"Action "
34+
+ action.id()
35+
+ " is run by the client, so "
36+
+ action.getClass().getName()
37+
+ " could never be dispatched");
38+
}
2839
FailureAction clash = byId.put(action.id(), action);
2940
if (clash != null) {
3041
throw new IllegalStateException(
@@ -38,17 +49,15 @@ public FailureActionRegistry(List<FailureAction> actions) {
3849
}
3950
}
4051

41-
/**
42-
* Fail fast if any kind declares an action with no handler, naming every gap rather than the
43-
* first, so one boot tells you everything that is missing.
44-
*/
52+
/** Names every gap rather than the first, so one boot tells you everything that is missing. */
4553
@PostConstruct
4654
void verifyEveryDeclaredActionHasAHandler() {
4755
List<String> gaps =
4856
Arrays.stream(FailureKind.values())
4957
.flatMap(
5058
kind ->
5159
kind.getActions().stream()
60+
.filter(FailureActionId::runsOnServer)
5261
.filter(action -> !byId.containsKey(action))
5362
.map(action -> kind.getId() + " -> " + action))
5463
.toList();
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package stirling.software.proprietary.failure;
2+
3+
/**
4+
* Who an offered action is for, the read scope having already decided they may see the incident.
5+
* The distinction is possession, not seniority: a reviewer cannot reach a document only its owner
6+
* holds.
7+
*/
8+
public enum FailureAudience {
9+
OWNER,
10+
11+
/** Anyone who triages the team's incidents, whoever hit them. */
12+
TEAM_REVIEWER,
13+
14+
ANYONE_WHO_SEES
15+
}

app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
package stirling.software.proprietary.failure;
22

3-
import static stirling.software.proprietary.failure.FailureActionId.ACKNOWLEDGE;
43
import static stirling.software.proprietary.failure.FailureActionId.DISMISS;
4+
import static stirling.software.proprietary.failure.FailureActionId.VIEW_FILE;
5+
import static stirling.software.proprietary.failure.FailureActionId.VIEW_IN_PROCESSOR;
6+
import static stirling.software.proprietary.failure.FailureAudience.ANYONE_WHO_SEES;
7+
import static stirling.software.proprietary.failure.FailureAudience.OWNER;
8+
import static stirling.software.proprietary.failure.FailureAudience.TEAM_REVIEWER;
59

610
import java.util.Arrays;
711
import java.util.HashMap;
@@ -20,13 +24,8 @@
2024
* The registry of failure kinds, described as data: a stable id, i18n keys and an English fallback
2125
* like {@code ExceptionUtils.ErrorCode}, plus the facets a review surface needs.
2226
*
23-
* <p>Actions are declared here but implemented in {@link FailureAction} beans resolved by id, so a
24-
* new kind ships as a registry entry plus copy. Two members today: {@link #UNKNOWN} gives every
25-
* failed run a record, and kinds get promoted out of it as production shows what occurs.
26-
*
27-
* <p>A kind offers an acknowledgement only where there is something to acknowledge <em>doing</em>.
28-
* With nothing to fix, "seen it" and "clear it" are the same decision, so the row offers only the
29-
* one that clears it.
27+
* <p>A new kind ships as a registry entry plus copy. Each offer also says who it is for, since one
28+
* incident is read both by whoever hit it and by whoever reviews after them.
3029
*/
3130
@Getter
3231
public enum FailureKind {
@@ -37,8 +36,9 @@ public enum FailureKind {
3736
FailureScope.FILE,
3837
errorCodes("E004"),
3938
fallback("This document is password-protected, so the pipeline could not read it."),
40-
offer(ACKNOWLEDGE),
41-
offer(DISMISS, "dismissSkipFile")),
39+
offer(VIEW_FILE, OWNER),
40+
offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER),
41+
offer(DISMISS, ANYONE_WHO_SEES)),
4242

4343
UNKNOWN(
4444
FailureStage.INTERNAL,
@@ -47,7 +47,11 @@ public enum FailureKind {
4747
FailureScope.RUN,
4848
noErrorCodes(),
4949
fallback("This run failed for a reason Stirling does not yet recognise."),
50-
offer(DISMISS));
50+
// Same order as every other kind: declaration order is display order, so the document
51+
// leads wherever it is offered rather than moving between failures.
52+
offer(VIEW_FILE, OWNER),
53+
offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER),
54+
offer(DISMISS, ANYONE_WHO_SEES));
5155

5256
private static final String KEY_PREFIX = "portal.failures.kind.";
5357
private static final String ACTION_KEY_PREFIX = "portal.failures.action.";
@@ -95,22 +99,26 @@ public enum FailureKind {
9599
}
96100

97101
/**
98-
* One action this kind offers, with the key to label it by. One ordered list rather than ids
99-
* plus a parallel map of overrides, which could disagree with each other.
102+
* One ordered list rather than ids plus parallel maps of audiences and labels, which could
103+
* disagree with each other.
100104
*
101105
* @param labelKeySuffix key under {@code portal.failures.action.}, or null for the generic
102106
* label
103107
*/
104-
private record Offer(FailureActionId id, String labelKeySuffix) {}
108+
private record Offer(FailureActionId id, FailureAudience audience, String labelKeySuffix) {}
105109

106-
/** An action labelled by this kind's own wording, where the generic label reads badly. */
107-
private static Offer offer(FailureActionId id, String labelKeySuffix) {
108-
return new Offer(id, labelKeySuffix);
110+
/** Declaration order is display order. */
111+
private static Offer offer(FailureActionId id, FailureAudience audience) {
112+
return new Offer(id, audience, null);
109113
}
110114

111-
/** An action labelled by the shared wording for that action. */
112-
private static Offer offer(FailureActionId id) {
113-
return new Offer(id, null);
115+
/**
116+
* As {@link #offer(FailureActionId, FailureAudience)}, but labelled by this kind's own wording
117+
* where the shared one reads badly.
118+
*/
119+
private static Offer offer(
120+
FailureActionId id, FailureAudience audience, String labelKeySuffix) {
121+
return new Offer(id, audience, labelKeySuffix);
114122
}
115123

116124
/**
@@ -149,6 +157,22 @@ public List<FailureActionId> getActions() {
149157
return offers.stream().map(Offer::id).toList();
150158
}
151159

160+
/**
161+
* What this kind offers, in declaration order, each with its label resolved. What a review
162+
* surface reads, so it never has to ask two separate questions about one offer.
163+
*/
164+
public List<OfferedAction> getOfferedActions() {
165+
return offers.stream()
166+
.map(
167+
offer ->
168+
new OfferedAction(
169+
offer.id(), labelKeyFor(offer.id()), offer.audience()))
170+
.toList();
171+
}
172+
173+
/** One action as a kind declares it: what to call it and who it is for. */
174+
public record OfferedAction(FailureActionId id, String labelKey, FailureAudience audience) {}
175+
152176
/** Whether this kind offers {@code action}. The dispatch guard: see {@code FailureActionId}. */
153177
public boolean declares(FailureActionId action) {
154178
return offers.stream().anyMatch(offer -> offer.id() == action);

app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,10 @@ public FileRunEventsResponse list(
7474
@Operation(
7575
summary = "Apply an action to a recorded failure",
7676
description =
77-
"Rejected with 400 if the failure's kind does not declare the action, so an"
78-
+ " action that makes no sense for a given failure cannot be applied.")
77+
"Rejected with 400 if the failure's kind does not declare the action, or if the"
78+
+ " action is one the client runs rather than the server, so neither an"
79+
+ " action that makes no sense for a given failure nor one the server"
80+
+ " cannot perform can be applied.")
7981
public FileRunEventView act(
8082
@PathVariable String eventId,
8183
@PathVariable String actionId,
@@ -87,7 +89,8 @@ public FileRunEventView act(
8789
FileRunEvent updated = service.dispatch(eventId, actionId, inputs);
8890
return FileRunEventView.of(updated, service.availableActions(updated));
8991
} catch (FailureActionException e) {
90-
throw new ResponseStatusException(statusFor(e.getReason()), e.getMessage(), e);
92+
throw new ResponseStatusException(
93+
FailureActionException.statusOf(e.getReason()), e.getMessage(), e);
9194
}
9295
}
9396

@@ -147,18 +150,6 @@ public List<FailureKindView> kinds() {
147150
return Arrays.stream(FailureKind.values()).map(FailureKindView::of).toList();
148151
}
149152

150-
/**
151-
* A closed row is a conflict rather than a bad request: the request was well-formed and would
152-
* have been valid a moment earlier.
153-
*/
154-
private static HttpStatus statusFor(FailureActionException.Reason reason) {
155-
return switch (reason) {
156-
case EVENT_NOT_FOUND -> HttpStatus.NOT_FOUND;
157-
case ACTION_NOT_RECOGNISED, ACTION_NOT_DECLARED -> HttpStatus.BAD_REQUEST;
158-
case ALREADY_CLOSED -> HttpStatus.CONFLICT;
159-
};
160-
}
161-
162153
/** Wrapped rather than a bare array so pagination can be added without breaking clients. */
163154
public record FileRunEventsResponse(List<FileRunEventView> events) {}
164155

@@ -178,10 +169,13 @@ List<String> safeFileIds() {
178169
}
179170
}
180171

181-
/** Inputs an action declared it needs. Empty for both actions that exist today. */
172+
/**
173+
* Inputs an action declared it needs. Empty for every action the server runs today: the one
174+
* that needs a password is run by the client, which never sends it here.
175+
*/
182176
public record ActionRequest(Map<String, String> inputs) {
183177

184-
Map<String, String> safeInputs() {
178+
public Map<String, String> safeInputs() {
185179
return inputs == null ? Map.of() : inputs;
186180
}
187181
}

app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,20 +98,18 @@ int applyStatusIf(
9898
* Close the incidents about documents their owner deleted from the editor: the queue is what
9999
* needs attention, and a document that no longer exists needs none.
100100
*
101-
* <p>Restricted to that owner's own editor rows. File ids are minted by the client, so scoping
102-
* on team alone would let one caller close a colleague's incidents by naming ids. Processor
103-
* rows are excluded outright: nothing was deleted from an editor there.
101+
* <p>Scoped by the absence of a source rather than by origin: a source-fed run's {@code fileId}
102+
* is a hash no client can name. Narrowed to the owner's own rows, since clients mint the ids.
104103
*/
105104
@Modifying(clearAutomatically = true)
106105
@Transactional
107106
@Query(
108107
"update FileRunEventEntity e set e.status ="
109108
+ " stirling.software.proprietary.failure.FileRunEventStatus.FILE_REMOVED,"
110-
+ " e.statusActor = :actor, e.statusAt = :now where e.origin ="
111-
+ " stirling.software.proprietary.failure.FailureOrigin.TOOL and ((:teamId is"
112-
+ " null and e.teamId is null) or e.teamId = :teamId) and ((:actor is null and"
113-
+ " e.actor is null) or e.actor = :actor) and e.fileId in :fileIds and e.status in"
114-
+ " :allowedFrom")
109+
+ " e.statusActor = :actor, e.statusAt = :now where e.sourceId is null and"
110+
+ " ((:teamId is null and e.teamId is null) or e.teamId = :teamId) and"
111+
+ " ((:actor is null and e.actor is null) or e.actor = :actor) and e.fileId in"
112+
+ " :fileIds and e.status in :allowedFrom")
115113
int markFilesRemoved(
116114
@Param("teamId") Long teamId,
117115
@Param("actor") String actor,

0 commit comments

Comments
 (0)