From fc967bb04bdc5025cc241903543ea4a4f58d3edd Mon Sep 17 00:00:00 2001 From: EthanHealy01 Date: Wed, 19 Aug 2026 15:30:18 +0100 Subject: [PATCH] docs: cut the comments back to what a reader will still need Review found the comment load indefensible, worst on backend code at 43% of added lines. Every comment this branch added or touched is now at most two lines, and the ones that only restated the code, narrated a decision already visible in the diff, or justified the PR to its reviewer are gone. What survives is the reasoning a reader cannot recover from the code: why markFilesRemoved keys on the absence of a source, why the divider boundary is frozen as an id, why the contexts are read raw, why "/" is not a destination. Two were not merely long but wrong, describing a retry this PR no longer has: the usePolicyAutoRun gate cited notificationActions.adopt, and its sibling cited "Decrypt and retry". 857 comment lines to 446 across the branch, backend code 273 to 121. --- .../failure/FailureActionException.java | 31 ++----- .../proprietary/failure/FailureActionId.java | 33 ++----- .../failure/FailureActionRegistry.java | 9 +- .../proprietary/failure/FailureAudience.java | 11 +-- .../proprietary/failure/FailureKind.java | 29 ++----- .../failure/FileRunEventRepository.java | 12 +-- .../failure/FileRunEventService.java | 46 +++------- .../proprietary/failure/FileRunEventView.java | 6 +- .../proprietary/failure/Ownership.java | 10 +-- .../notification/NotificationController.java | 14 +-- .../notification/NotificationService.java | 19 +---- .../notification/NotificationSource.java | 11 +-- .../notification/NotificationView.java | 31 +------ .../policy/controller/PolicyController.java | 10 +-- .../policy/controller/PolicyRunFiles.java | 10 +-- .../policy/engine/PolicyEngine.java | 8 +- .../policy/engine/PolicyRunner.java | 9 +- .../failure/CheckConstrainedEnumsTest.java | 13 +-- .../proprietary/failure/FailureKindTest.java | 32 +++---- .../failure/FileRunEventServiceTest.java | 51 ++++------- .../failure/FileRunEventStoreDbTest.java | 12 +-- .../failure/NotificationProjectionTest.java | 21 ++--- .../failure/PolicyFailureOwnershipTest.java | 37 +++----- .../failure/PolicyFailureRecorderTest.java | 11 +-- .../failure/RecordFailurePrivacyTest.java | 6 +- .../controller/PolicyControllerTest.java | 17 ++-- .../components/notifications/BellIcon.tsx | 5 +- .../notifications/NotificationBell.test.tsx | 33 +++---- .../notifications/NotificationBell.tsx | 69 +++++---------- .../notifications/notificationActions.ts | 27 ++---- .../editor/src/core/contexts/FileContext.tsx | 7 +- .../src/core/contexts/NavigationContext.tsx | 5 +- .../file/removeFiles.reporting.test.tsx | 16 ++-- .../hooks/tools/shared/useToolOperation.ts | 3 +- .../src/core/hooks/useNotifications.test.ts | 20 ++--- .../editor/src/core/hooks/useNotifications.ts | 85 +++++-------------- .../editor/src/core/routes/portalBasename.ts | 6 +- .../src/core/services/localFilePresence.ts | 5 +- .../editor/src/core/services/notifications.ts | 45 ++-------- .../src/core/tests/helpers/api-stubs.ts | 6 +- frontend/editor/src/core/types/fileContext.ts | 5 +- .../components/failures/FileRunEventList.tsx | 6 +- .../notificationActions.test.tsx | 33 +++---- .../notifications/notificationActions.ts | 63 +++----------- .../policies/usePolicyAutoRun.chain.test.tsx | 15 +--- .../components/policies/usePolicyAutoRun.ts | 8 +- .../proprietary/services/policyApi.test.ts | 5 +- .../src/proprietary/services/policyApi.ts | 7 +- wt-perms | 1 + 49 files changed, 274 insertions(+), 700 deletions(-) create mode 160000 wt-perms diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java index aff3b7c50f..bd0ed2d001 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java @@ -4,20 +4,14 @@ import org.springframework.http.HttpStatus; import lombok.Getter; -/** - * Why an action could not be dispatched. Thrown carrying a {@link Reason} rather than an HTTP - * status, so the service stays web-agnostic and only a controller ever reaches for {@link - * #statusOf}. - */ +/** Carries a {@link Reason} rather than an HTTP status, so the service stays web-agnostic. */ @Getter public class FailureActionException extends RuntimeException { public enum Reason { /** - * No such event, it belongs to another team, or the caller's team did not resolve. One - * reason for all three, so the response does not vary with which it was. Unrelated to - * {@link FailureKind#UNKNOWN}, which is an unclassified failure rather than a refused - * action. + * No such event, another team's, or an unresolved team: one reason, so the answer cannot + * vary. */ EVENT_NOT_FOUND, @@ -25,16 +19,11 @@ public class FailureActionException extends RuntimeException { ACTION_NOT_RECOGNISED, /** - * The action exists but this kind does not declare it, so an incoherent pairing (releasing - * a document whose destination is what failed) cannot be dispatched even by hand. Reachable - * as of the kinds that stopped offering {@link FailureActionId#ACKNOWLEDGE}. + * The action exists but this kind does not offer it, so it cannot be dispatched by hand. */ ACTION_NOT_DECLARED, - /** - * The kind offers it, but the client is what runs it (see {@link - * FailureActionId.Execution#CLIENT}), so it is refused rather than half-performed. - */ + /** Offered, but the client is what runs it, so refused rather than half-performed. */ ACTION_NOT_DISPATCHABLE, /** The event is already closed, so no further transition is possible. */ @@ -47,20 +36,14 @@ public class FailureActionException extends RuntimeException { this(reason, message, null); } - /** For a refusal that follows from a lower-level failure, so its stack is not dropped. */ public FailureActionException(Reason reason, String message, Throwable cause) { super(message, cause); this.reason = reason; } /** - * The HTTP status each refusal reason answers with. Lives with the reasons it maps, so the two - * surfaces that dispatch actions, the failure queue and the notification bell, cannot drift - * apart and a client's error handling does not depend on which one it called. - * - *

A closed row is a conflict rather than a bad request: the request was well-formed and - * would have been valid a moment earlier. A missing row is a 404 whether it never existed, - * belongs to another team or is a colleague's, so trying does not confirm which. + * Lives with the reasons it maps, so every surface that dispatches an action answers alike. A + * closed row is a conflict, not a bad request: it was well-formed and valid a moment earlier. */ public static HttpStatus statusOf(Reason reason) { return switch (reason) { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java index cedb4d03c9..ad97fed8b2 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java @@ -3,42 +3,27 @@ package stirling.software.proprietary.failure; import lombok.Getter; /** - * The actions a {@link FailureKind} may declare, and which side of the wire runs each one. - * - *

The dispositions are the server's: they change how the event is shown and touch nothing else, - * which is what makes them valid for every kind including {@link FailureKind#UNKNOWN}. Everything - * else is the client's, because the server holds an opaque id for the document and nothing more. - * - *

Client actions are still declared here rather than invented by each client, so the server - * keeps deciding what a kind offers, in what order and under what label. + * The actions a {@link FailureKind} may declare. Client actions are declared here rather than + * invented per client, so the server keeps deciding what a kind offers, in what order and labelled + * how. */ @Getter public enum FailureActionId { - /** - * "Seen, and I own it." No kind offers this any more, but rows shipped before that are already - * {@code ACKNOWLEDGED} and must stay readable and closable. - */ + /** No kind offers this any more, but rows already {@code ACKNOWLEDGED} must stay closable. */ ACKNOWLEDGE(Execution.SERVER, "Acknowledge"), - /** "No remediation will happen; close it." See {@link DismissAction}. */ DISMISS(Execution.SERVER, "Dismiss"), - /** Open the document this incident is about. Only its owner's client can resolve the id. */ + /** Only the owner's client can resolve the id. */ VIEW_FILE(Execution.CLIENT, "View file"), - /** Open the run behind it, for whoever reviews the team rather than owns the document. */ VIEW_IN_PROCESSOR(Execution.CLIENT, "View in processor"); - /** - * Which side of the wire runs the action. The dispatch endpoint refuses a {@code CLIENT} id, so - * "the client does this one" is enforced rather than merely documented. - */ + /** Dispatch refuses a {@code CLIENT} id, so this is enforced rather than merely documented. */ public enum Execution { - /** - * Dispatchable, and {@link FailureActionRegistry} requires a {@link FailureAction} bean. - */ + /** {@link FailureActionRegistry} requires a {@link FailureAction} bean for these. */ SERVER, /** @@ -49,7 +34,7 @@ public enum FailureActionId { private final Execution execution; - /** English fallback, used when the client has no translation for the action's label key. */ + /** English fallback, for a client with no translation for the label key. */ private final String defaultLabel; FailureActionId(Execution execution, String defaultLabel) { @@ -57,7 +42,7 @@ public enum FailureActionId { this.defaultLabel = defaultLabel; } - /** Whether the server runs it itself, which is also whether it can be dispatched. */ + /** Also whether it can be dispatched. */ public boolean runsOnServer() { return execution == Execution.SERVER; } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java index 0bbfd47ea0..dad1bd5eda 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java @@ -17,8 +17,8 @@ import lombok.extern.slf4j.Slf4j; * point: because kinds declare action ids as data, one could name an action nobody implements, * which would otherwise show up as a button that 400s rather than as a failed boot. * - *

Only {@link FailureActionId.Execution#SERVER} ids belong here. A client action has no bean by - * design, and a bean for one is refused outright because dispatch could never reach it. + *

Only {@link FailureActionId.Execution#SERVER} ids belong here: a bean for a client action is + * refused, because dispatch could never reach it. */ @Slf4j @Service @@ -49,10 +49,7 @@ public class FailureActionRegistry { } } - /** - * Fail fast if any kind declares a server action with no handler, naming every gap rather than - * the first, so one boot tells you everything that is missing. - */ + /** Names every gap rather than the first, so one boot tells you everything that is missing. */ @PostConstruct void verifyEveryDeclaredActionHasAHandler() { List gaps = diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java index 04a577d13f..217dfc7975 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java @@ -1,20 +1,15 @@ package stirling.software.proprietary.failure; /** - * Who an offered action is for. The read scope has already decided the caller may see the incident; - * this decides which of its actions are theirs to take. - * - *

The distinction is possession, not seniority: a reviewer clearing up after a colleague cannot - * supply that colleague's password or reach a document only their browser holds. + * Who an offered action is for, the read scope having already decided they may see the incident. + * The distinction is possession, not seniority: a reviewer cannot reach a document only its owner + * holds. */ public enum FailureAudience { - - /** The person whose work failed, whose own client still holds the document. */ OWNER, /** Anyone who triages the team's incidents, whoever hit them. */ TEAM_REVIEWER, - /** Everyone the incident is shown to at all. */ ANYONE_WHO_SEES } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java index 978fbfacf2..fa2fd93448 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java @@ -24,14 +24,8 @@ import lombok.Getter; * The registry of failure kinds, described as data: a stable id, i18n keys and an English fallback * like {@code ExceptionUtils.ErrorCode}, plus the facets a review surface needs. * - *

Actions are declared here but run elsewhere: a server action in a {@link FailureAction} bean - * resolved by id, a client action in the browser that holds the document. Either way a new kind - * ships as a registry entry plus copy. Two members today: {@link #UNKNOWN} gives every failed run a - * record, and kinds get promoted out of it as production shows what occurs. - * - *

Each offer also says who it is for, because the same incident is read by the person who hit it - * and by whoever reviews after them: only the owner holds the document, only a reviewer wants the - * run. + *

A new kind ships as a registry entry plus copy. Each offer also says who it is for, since one + * incident is read both by whoever hit it and by whoever reviews after them. */ @Getter public enum FailureKind { @@ -42,8 +36,6 @@ public enum FailureKind { FailureScope.FILE, errorCodes("E004"), fallback("This document is password-protected, so the pipeline could not read it."), - // Only the owner holds the document, so the file is theirs to open; a reviewer - // gets the run instead, and anyone who sees the row may close it. offer(VIEW_FILE, OWNER), offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), offer(DISMISS, ANYONE_WHO_SEES)), @@ -55,11 +47,8 @@ public enum FailureKind { FailureScope.RUN, noErrorCodes(), fallback("This run failed for a reason Stirling does not yet recognise."), - // Nothing here is known to be fixable, so the offers are the places to look: - // the owner their document, a reviewer the run, and anyone may close the row. - // Declared in the same order as every other kind, because declaration order is - // display order: the document leads wherever it is offered, so a reader is not - // asked to re-learn which button leads from one failure to the next. + // Same order as every other kind: declaration order is display order, so the document + // leads wherever it is offered rather than moving between failures. offer(VIEW_FILE, OWNER), offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), offer(DISMISS, ANYONE_WHO_SEES)); @@ -110,19 +99,15 @@ public enum FailureKind { } /** - * One action this kind offers: who it is for, and the key to label it by. One ordered list - * rather than ids plus parallel maps of audiences and label overrides, which could disagree - * with each other. + * One ordered list rather than ids plus parallel maps of audiences and labels, which could + * disagree with each other. * * @param labelKeySuffix key under {@code portal.failures.action.}, or null for the generic * label */ private record Offer(FailureActionId id, FailureAudience audience, String labelKeySuffix) {} - /** - * An action this kind offers, for whoever can actually take it, labelled by the shared wording. - * Declaration order is display order. - */ + /** Declaration order is display order. */ private static Offer offer(FailureActionId id, FailureAudience audience) { return new Offer(id, audience, null); } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java index e844e73a46..316dd34925 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java @@ -98,16 +98,8 @@ public interface FileRunEventRepository extends JpaRepositoryRestricted to that owner's own rows. File ids are minted by the client, so scoping on team - * alone would let one caller close a colleague's incidents by naming ids. - * - *

Scoped by the ABSENCE OF A SOURCE rather than by origin, which is what tells the two id - * spaces in {@code fileId} apart. An editor report and an attended policy run both carry the id - * the client minted for its own document, so both are closable by the client that holds it; a - * source-fed run carries a one-way hash of a path or key that was never on any device, so no - * client can legitimately name it. Keying on {@code origin = TOOL} instead left a user's own - * policy failures in the queue after they deleted the very document those failures were about, - * because the run had been recorded by the processor rather than reported by the editor. + *

Scoped by the absence of a source rather than by origin: a source-fed run's {@code fileId} + * is a hash no client can name. Narrowed to the owner's own rows, since clients mint the ids. */ @Modifying(clearAutomatically = true) @Transactional diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java index fcdaf1de47..10f45f46c0 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java @@ -22,8 +22,7 @@ import stirling.software.proprietary.policy.config.PolicyManagementAuthority; * enabled so single-user deployments keep working. When the team cannot be resolved the caller * reads nothing; see {@link #readScope()}. * - *

Seeing an incident and being able to act on it are separate questions: the read scope decides - * the first, {@link #availableActions} the second. + *

The read scope decides who sees an incident; {@link #availableActions} decides who may act. */ @Slf4j @Service @@ -135,8 +134,8 @@ public class FileRunEventService { // failures they caused. Someone who fixes their own problem should not have to ask a leader // to clear the row. // - // Audience decides what is offered, not what may be dispatched, so this read scope is the - // whole gate. A server action aimed at OWNER alone would need its own guard here. + // Audience decides what is offered, not what may be dispatched, so this scope is the whole + // gate. A server action aimed at OWNER alone would need its own guard here. FileRunEvent event = requireVisible(eventId); FailureActionId resolvedId = parseActionId(actionId); @@ -148,8 +147,7 @@ public class FileRunEventService { FailureActionException.Reason.ACTION_NOT_DECLARED, "Kind " + event.kind().getId() + " does not offer action " + resolvedId); } - // Declared, and still not the server's to run: without this a client could post VIEW_FILE - // and be answered as though something had happened, when nothing here has the file. + // Without this a client could post VIEW_FILE and be answered as though something happened. if (!resolvedId.runsOnServer()) { throw new FailureActionException( FailureActionException.Reason.ACTION_NOT_DISPATCHABLE, @@ -173,10 +171,7 @@ public class FileRunEventService { return action.execute(event, inputs == null ? Map.of() : inputs, currentActor()); } - /** - * The event, if this caller may act on it at all. Reported as "no such event" rather than a - * refusal, so a member cannot learn that a colleague's incident exists by trying to close it. - */ + /** "No such event" rather than a refusal, so trying does not confirm a colleague's exists. */ private FileRunEvent requireVisible(String eventId) { ReadScope scope = readScope(); if (!scope.permitted()) { @@ -192,7 +187,6 @@ public class FileRunEventService { FailureActionException.Reason.EVENT_NOT_FOUND, "No such event: " + eventId); } - /** Whose incident this is, as far as the caller is concerned. See {@link Ownership}. */ public Ownership ownershipOf(FileRunEvent event) { if (event.actor() == null) { return Ownership.UNOWNED; @@ -202,22 +196,18 @@ public class FileRunEventService { } /** - * Which of an event's declared actions this caller is offered, and which are usable right now, - * so the client never renders a button that would be refused. An action outside the caller's - * audience is dropped rather than disabled: a greyed-out "Decrypt and retry" would read as a - * permission problem when it is simply not their document. + * Offers resolved for one caller, so no client renders a button that would be refused. Outside + * their audience is dropped, not disabled: greyed out would read as a permission problem. */ public List availableActions(FileRunEvent event) { Ownership ownership = ownershipOf(event); boolean reviewsTeam = reviewsTeam(); boolean closed = event.status().terminal(); - // Nobody's client is holding the document, so an owner action has nothing to act on. A - // login-disabled deployment is excluded: its rows are unowned only because it has no users, - // and its one operator owns everything they can see. + // Login disabled is excluded: its rows are unowned only for want of users, and its one + // operator owns everything they can see. boolean unattended = enforced() && ownership == Ownership.UNOWNED; - // No document reference at all, so an owner action has nothing to name even when the owner - // is right here holding the file. Answered here rather than left to the client, which would - // otherwise report "not on this device" about a document the row never identified. + // Answered here, or the client reports "not on this device" about a document the row never + // identified in the first place. boolean documentless = event.fileId() == null || event.fileId().isBlank(); return event.kind().getOfferedActions().stream() .filter(offer -> offeredTo(offer.audience(), ownership, reviewsTeam)) @@ -235,10 +225,7 @@ public class FileRunEventService { return new AvailableAction(offer.id(), offer.labelKey(), reason == null, reason); } - /** - * Why an offer cannot be taken right now, or null when it can. Closed wins over everything, - * then the owner-only reasons, most specific first. - */ + /** Closed wins over everything, then the owner-only reasons, most specific first. */ private static String disabledReasonFor( FailureAudience audience, boolean closed, boolean unattended, boolean documentless) { if (closed) { @@ -253,11 +240,7 @@ public class FileRunEventService { return documentless ? DOCUMENTLESS_REASON_KEY : null; } - /** - * Whether an offer aimed at {@code audience} is this caller's to take. An unattended incident - * has no owner, so its reviewer inherits the owner's actions; otherwise nobody is offered them - * at all. - */ + /** An unattended incident has no owner, so its reviewer inherits the owner's actions. */ private static boolean offeredTo( FailureAudience audience, Ownership ownership, boolean reviewsTeam) { return switch (audience) { @@ -268,7 +251,7 @@ public class FileRunEventService { }; } - /** Whether the caller triages the whole team's incidents. Login disabled has no roles. */ + /** Login disabled has no roles, so its one operator triages everything. */ private boolean reviewsTeam() { return !enforced() || policyManagementAuthority.canEditPolicies(); } @@ -343,7 +326,6 @@ public class FileRunEventService { return applicationProperties.getSecurity().isEnableLogin(); } - /** One action as offered to one caller about one event, with its availability resolved. */ public record AvailableAction( FailureActionId id, String labelKey, boolean enabled, String disabledReasonKey) {} } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java index af68233e25..b88ba3d48d 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java @@ -61,9 +61,8 @@ public record FileRunEventView( } /** - * One button, as offered to this caller about this row. {@code defaultLabel} and {@code - * execution} are here for the reason {@code defaultTitle} is on the row: a client can then - * render, and route, an action it was never built with. Declaration order is display order. + * {@code defaultLabel} and {@code execution} let a client render and route an action it was + * never built with. Declaration order is display order. */ public record ActionView( String id, @@ -73,7 +72,6 @@ public record FileRunEventView( boolean enabled, String disabledReasonKey) { - /** Public because the notification bell projects the same resolved offers. */ public static ActionView of(FileRunEventService.AvailableAction action) { return new ActionView( action.id().name(), diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java index 635e357431..65745a61b1 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java @@ -1,21 +1,17 @@ package stirling.software.proprietary.failure; /** - * Whose incident this is, from the point of view of whoever is reading it. Derived on read and - * never persisted: the same row is {@code MINE} to the member who hit it and {@code THEIRS} to the - * leader reviewing after them, so a stored answer would be wrong for everyone but one person. + * Whose incident this is, from the reader's point of view. Derived on read, never persisted: one + * row is {@code MINE} to whoever hit it and {@code THEIRS} to the leader reviewing after them. */ public enum Ownership { - - /** The caller's own failure: their work, and their client holding the document. */ MINE, /** A colleague's, visible because the caller reviews the team. */ THEIRS, /** - * Nobody's. An unattended run failed, with a folder, bucket or webhook as its only attribution, - * so there is no owner to hand the resolution to. + * An unattended run: a folder, bucket or webhook is its only attribution, so there is no owner. */ UNOWNED } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java index 047376a655..de24a0842f 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java @@ -14,13 +14,8 @@ import io.swagger.v3.oas.annotations.tags.Tag; import lombok.RequiredArgsConstructor; /** - * The caller's notifications. Open to any authenticated user, unlike the failure endpoints it draws - * on: each source scopes its own rows and resolves its own actions, so a member is told about their - * own failures and a leader about their team's. - * - *

Read-only: every action a notification offers is one the client runs on its own device, so - * there is nothing to post back here yet. Ids are still prefixed with their source, so the bell is - * never given the producing row's id; see {@link NotificationSource}. + * Open to any authenticated user, unlike the failure endpoints it draws on: each source scopes its + * own rows. Read-only, because every action a notification offers runs on the client's own device. */ @RestController @RequestMapping("/api/v1/notifications") @@ -29,7 +24,6 @@ import lombok.RequiredArgsConstructor; @Tag(name = "Notifications", description = "Things worth telling the caller about") public class NotificationController { - /** One page of a bell. Enough to fill a panel; the badge counts what it is given. */ private static final int DEFAULT_LIMIT = 20; private static final int MAX_LIMIT = 100; @@ -47,8 +41,6 @@ public class NotificationController { return new NotificationsResponse(notifications.list(capped)); } - /** - * Wrapped rather than a bare array so paging or a total can be added without breaking clients. - */ + /** Wrapped so paging or a total can be added without breaking clients. */ public record NotificationsResponse(List notifications) {} } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java index 690d8abf8b..f7bf3b8530 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java @@ -11,13 +11,8 @@ import stirling.software.proprietary.failure.FileRunEventService; import stirling.software.proprietary.failure.FileRunEventView; /** - * Assembles the caller's notifications from whatever produces them. Derived on read rather than - * stored: there is one source today, and a table would need a write path, a retention story and a - * per-user read model before it earned itself. - * - *

Who sees what is decided by each source rather than here. {@link FileRunEventService} already - * scopes its reads and resolves each row's actions against that reader, so this cannot widen either - * by accident. + * Derived on read rather than stored: one source today, and a table would need a write path, + * retention and a per-user read model first. Each source scopes its own rows, so this cannot widen. */ @Service @RequiredArgsConstructor @@ -25,17 +20,12 @@ public class NotificationService { private final FileRunEventService fileRunEvents; - /** - * The caller's notifications, newest first. Only open failures: a dismissed or resolved one has - * been dealt with and is not news. - */ + /** Newest first, and only open failures: one already dealt with is not news. */ public List list(int limit) { return fileRunEvents.list(null, null, limit).stream().map(this::fromFailure).toList(); } - /** - * One failure as a notification, with its row id prefixed on the way out and never sent bare. - */ + /** Prefixes the row id on the way out, so it is never sent bare. */ private NotificationView fromFailure(FileRunEvent event) { return new NotificationView( NotificationSource.FAILURE.qualify(event.id()), @@ -54,7 +44,6 @@ public class NotificationService { event.occurrences(), event.createdAt(), event.lastSeenAt(), - // The failure surface's own offers, filtered to the ones the client itself runs. // A disposition such as Dismiss belongs to the review surface, not the bell. fileRunEvents.availableActions(event).stream() .filter(action -> !action.id().runsOnServer()) diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java index bdb8bac680..007e51616f 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java @@ -3,25 +3,18 @@ package stirling.software.proprietary.notification; import java.util.Locale; /** - * Which subsystem produced a notification. One member today; the point of the field is that a - * client already branches on it, so a second source needs no client change to be ignorable. - * - *

Every notification id is prefixed with its source, so a client never holds the producing row's - * own id and cannot hand it to that source's endpoints by accident. + * Which subsystem produced a notification. Every id is prefixed with it, so a client never holds + * the producing row's own id and cannot reach that source's endpoints by accident. */ public enum NotificationSource { - - /** A recorded run failure: see {@code stirling.software.proprietary.failure}. */ FAILURE; private static final char SEPARATOR = ':'; - /** The prefix an id from this source carries, including the separator. */ public String prefix() { return name().toLowerCase(Locale.ROOT) + SEPARATOR; } - /** One of this source's own row ids, as the client sees it. */ public String qualify(String sourceRowId) { return prefix() + sourceRowId; } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java index 4555749e47..be04153686 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java @@ -10,35 +10,8 @@ import stirling.software.proprietary.failure.FileRunEventView; import stirling.software.proprietary.failure.Ownership; /** - * One thing worth telling the caller about, flattened to what a bell needs to render it and act on - * it. Sources project onto this rather than exposing their own shape, so a client renders and - * triages a notification without knowing which subsystem produced it. - * - *

The facets read as failure vocabulary because failures are the only source so far. A later - * source projects onto the same fields or leaves them null; what a client must not do is infer the - * source from which fields are populated, which is what {@code source} is for. - * - * @param id unique across every source, so a client can key and de-duplicate on it alone. Prefixed - * with its source, and the only handle the notification endpoints accept - * @param source which subsystem produced it, for grouping and for the click-through - * @param kindId the source's own id for what happened, so a client can group or route on it - * @param origin what was running when it happened, e.g. one tool or a policy - * @param ownership whether this is the caller's own to act on, derived per read - * @param severity how loudly to show it - * @param status where the underlying row stands, so a bell can show what has already been dealt - * with - * @param titleKey i18n key for the headline, with {@code defaultTitle} as the English fallback - * @param detail the one-line body, as the source recorded it - * @param fileId opaque reference to the document, never a name. Two id spaces share it, and {@code - * sourceId} is what tells them apart; see {@code PolicyEngine#runPolicy} - * @param sourceId which folder, bucket or webhook fed the failing run, and null for an attended run - * the caller started themselves - * @param policyId which policy the failing run belonged to; null for work with no policy around it - * @param occurrences how many times this same thing has happened, at least 1 - * @param createdAt when it first happened; the client orders and marks unread on this - * @param lastSeenAt when it last happened, equal to {@code createdAt} for a one-off - * @param actions the same resolved offers the failure surface renders, already filtered to this - * caller, so the bell needs no rules of its own + * A source's row flattened to what a bell renders. {@code fileId} is an opaque reference, never a + * name, and two id spaces share it: {@code sourceId} tells them apart. */ public record NotificationView( String id, diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java index dff925a96a..f959aeef05 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java @@ -680,13 +680,9 @@ public class PolicyController { } /** - * The document reference to record against a failure of this run: the caller's own opaque id, - * but only when the run carries exactly one primary document. A recorded incident has a single - * file reference, so naming one document out of several would attribute the failure to - * whichever happened to be bound first, which is worse than naming none. - * - *

Counted off the resolved inputs rather than the raw multipart list, so an empty part - * cannot make a single-document run look like two. + * Only for a single-document run: an incident holds one file reference, so naming one of + * several would attribute the failure to whichever bound first. Counted off resolved inputs, + * not parts. */ private static String documentReferenceFor(PolicyRunFiles files, PolicyInputs inputs) { String fileId = files.getFileId(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java index dab1a8edb6..fd4a791c8b 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java @@ -31,14 +31,8 @@ public class PolicyRunFiles { private List assets = new ArrayList<>(); /** - * The caller's own opaque reference to the document it is running on, recorded against any - * failure of this run so the client that filed it can resolve the row back to that document. - * Without it an attended failure names no document, and every action that needs the bytes is - * unreachable for the one person holding them. - * - *

Opaque by contract and never a name: the server stores it, hands it back and reads nothing - * out of it. Only honoured for a single-document run, see {@code - * PolicyController#documentReferenceFor}. + * Recorded against any failure of this run, so the client can resolve the row back to its + * document. Opaque by contract, never a name, and only honoured for a single-document run. */ @Schema( description = diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java index 022435aea1..f9c0f719ef 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java @@ -151,12 +151,8 @@ public class PolicyEngine { * As {@link #runPolicy(Policy, PolicyInputs, PolicyProgressListener)}, recording which source * fed the run and its opaque reference to the document. The first says where an unattended * failure came from; the second says which document, and is what lets the same document failing - * again fold into one incident. - * - *

An attended run (a user's upload) has no source, and that absence is the discriminator for - * the two id spaces {@code fileIdentity} carries: with no source it is the reference the client - * minted for its own document, otherwise a source's one-way hash of a path or key. Either way - * this engine only carries it, and reads nothing out of it. + * again fold into one incident. With no source {@code fileIdentity} is the client's own + * reference, with one it is that source's hash; this engine only carries it either way. */ public PolicyRunHandle runPolicy( Policy policy, diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java index 716f783cdc..d159ebf012 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java @@ -121,13 +121,8 @@ public class PolicyRunner { * The supplied documents are still counted against the virtual {@link EditorSource}, scoped to * the policy's team, so the Sources overview reports the whole team's editor throughput. * - *

Attended: no source fed this run, which is what later tells its recorded failures apart - * from an unattended sweep's. - * - * @param documentReference the caller's own opaque reference to the single document it is - * running on, or null when it supplied none (or supplied several). Passed through - * untouched, and lands where an unattended run's {@link ResolvedInput#forFile} identity - * does; see {@code PolicyEngine#runPolicy} for how the two are told apart. + * @param documentReference the caller's own opaque reference to the single document it runs on, + * or null when it supplied none or several. Passed through untouched. */ public PolicyRunHandle runWith( Policy policy, diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java index 15fd66cb1e..7eaeb01eb4 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java @@ -9,11 +9,8 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; /** - * Pins the five enums that {@code file_run_events} stores as strings behind CHECK constraints. The - * shipped migration spells out the permitted values, so adding one to {@code status}, {@code - * origin}, {@code stage}, {@code severity} or {@code scope} is a schema change dressed up as a Java - * change: it compiles, then fails against a real database on the row that most needed recording. - * Reordering is free, since values are persisted by name. + * Pins the five enums {@code file_run_events} stores behind CHECK constraints: adding a value is a + * schema change dressed as a Java one, compiling here and failing against a real database. */ class CheckConstrainedEnumsTest { @@ -36,8 +33,7 @@ class CheckConstrainedEnumsTest { @Test @DisplayName("the facets added since are derived, not stored") void nothingAddedToTheModelReachedTheTable() throws Exception { - // Audience, slot, execution and ownership are resolved per reader, so a column for any of - // them would hold the wrong answer for everybody but one person. + // Resolved per reader, so a column would hold the wrong answer for all but one person. List> persisted = Arrays.stream(FileRunEventEntity.class.getDeclaredFields()) .filter(field -> !field.isSynthetic()) @@ -50,8 +46,7 @@ class CheckConstrainedEnumsTest { FailureActionId.class, FailureActionId.Execution.class, Ownership.class); - // kind_id stays a plain varchar with no CHECK, which is what lets a new kind ship without a - // migration while the five columns above cannot. + // A plain varchar with no CHECK, which is what lets a new kind ship without a migration. assertThat(FileRunEventEntity.class.getDeclaredField("kindId").getType()) .isEqualTo(String.class); } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java index 0f0f451d65..03aa5c9402 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java @@ -32,10 +32,7 @@ import stirling.software.common.util.ExceptionUtils; */ class FailureKindTest { - /** - * One expected offer in full, rather than four separate extracting() assertions, so a - * declaration that pairs the right action with the wrong audience cannot pass. - */ + /** In full, so a declaration pairing the right action with the wrong audience cannot pass. */ private static FailureKind.OfferedAction offered( FailureActionId id, FailureAudience audience, String labelKeySuffix) { return new FailureKind.OfferedAction( @@ -76,10 +73,9 @@ class FailureKindTest { @ParameterizedTest @EnumSource(FailureKind.class) void declaresItsActionsInTheSameOrderAsEveryOtherKind(FailureKind kind) { - // Declaration order is display order, and the first offer a reader can use is the one - // rendered as the row's primary. Two kinds listing the same actions in different orders - // therefore flip the solid button between rows, which reads as a bug rather than as - // emphasis. Asserted as a shared ranking so a kind added later cannot reintroduce it. + // Declaration order is display order and the first usable offer is the row's primary, + // so + // two kinds disagreeing would flip the solid button between rows. List ranking = List.of( FailureActionId.VIEW_FILE, @@ -126,8 +122,7 @@ class FailureKindTest { @ParameterizedTest @EnumSource(FailureKind.class) void everyOfferSaysWhoItIsFor(FailureKind kind) { - // Read per row to decide what a caller is shown, so a missing one would be a button - // offered to whoever the null case happened to let through. + // Read per row to decide what a caller is shown, so a null would leak a button. for (FailureKind.OfferedAction offer : kind.getOfferedActions()) { assertThat(offer.audience()) .as("%s offers %s", kind.getId(), offer.id()) @@ -138,8 +133,8 @@ class FailureKindTest { @ParameterizedTest @EnumSource(FailureKind.class) void offersEachActionAtMostOnce(FailureKind kind) { - // Declaration order is the client's tie-break, so the same action twice would be two - // buttons with one meaning, and labelKeyFor would silently answer for the first. + // The same action twice would be two buttons with one meaning, and labelKeyFor would + // answer for the first. assertThat(kind.getActions()).doesNotHaveDuplicates(); } @@ -238,8 +233,7 @@ class FailureKindTest { @Test void offersItsOwnerTheirDocumentAndTheRunToWhoeverReviews() { - // Nothing here is known to be fixable, so the offers are the places to look: the - // owner their document, a reviewer the run, and anyone may close the row. + // Nothing here is known to be fixable, so the offers are just the places to look. assertThat(FailureKind.UNKNOWN.getOfferedActions()) .containsExactly( offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), @@ -301,8 +295,7 @@ class FailureKindTest { @Test void offersTheDocumentToItsOwnerAndTheRunToItsReviewer() { - // The whole point of the audiences: only the owner holds the document, so a reviewer - // is offered the run and a way to close the row instead. + // The point of the audiences: only the owner holds the document. assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getOfferedActions()) .containsExactly( offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), @@ -315,8 +308,8 @@ class FailureKindTest { @Test void noKindOffersAcknowledgeAnyMore() { - // Kept in the vocabulary because rows are already ACKNOWLEDGED, and those must stay - // readable. Nothing offers it, so nothing can dispatch it either. + // Kept in the vocabulary for rows already ACKNOWLEDGED; offered by nothing, so + // dispatchable by nothing. for (FailureKind kind : FailureKind.values()) { assertThat(kind.declares(FailureActionId.ACKNOWLEDGE)) .as("%s offers ACKNOWLEDGE", kind.getId()) @@ -326,8 +319,7 @@ class FailureKindTest { @Test void everyKindLabelsItsActionsWithTheSharedWordingToday() { - // The per-kind override still exists for wording that reads badly in context; nothing - // needs it now that Dismiss sits in an overflow menu, where the shared word is right. + // The per-kind override still exists for wording that reads badly in context. for (FailureKind kind : FailureKind.values()) { for (FailureActionId action : kind.getActions()) { assertThat(kind.labelKeyFor(action)) diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java index cff75e2fff..83ad173637 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java @@ -85,9 +85,7 @@ class FileRunEventServiceTest { class Acknowledge { /** - * No kind offers ACKNOWLEDGE any more, so it cannot be dispatched; the bean stays for rows - * that are already {@code ACKNOWLEDGED}. Exercised directly so their transition stays - * covered. + * No kind offers it, so it cannot be dispatched; exercised directly for rows that have it. */ private FileRunEvent acknowledge(FileRunEvent event, String actor) { return new AcknowledgeAction(store).execute(event, Map.of(), actor); @@ -128,8 +126,6 @@ class FileRunEventServiceTest { @Test void anAlreadyAcknowledgedRowStaysReadableAndClosable() { - // The reason the id and its bean stay: a row in this state predates the retry actions - // and must not become a row nobody can act on. FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); acknowledge(event, ACTOR); @@ -232,8 +228,7 @@ class FileRunEventServiceTest { @Test void anActionTheClientRunsIsRefusedRatherThanPretendedTo() { - // UNKNOWN offers VIEW_FILE, and the server has neither the document nor the tool. - // Answering 200 here would tell the client something happened when nothing did. + // Answering 200 would tell the client something happened when nothing did. FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); assertThatThrownBy(() -> service.dispatch(event.id(), "VIEW_FILE", Map.of())) @@ -247,8 +242,7 @@ class FileRunEventServiceTest { @Test void everyClientActionIsRefusedWhicheverKindDeclaresIt() { - // Asserted over the vocabulary rather than one id, so an action added on the client - // side cannot arrive dispatchable because nobody thought to test it. + // Over the whole vocabulary, so a client action added later cannot arrive dispatchable. for (FailureKind kind : FailureKind.values()) { FileRunEvent event = given(kind, TEAM, "f-" + kind.getId()); for (FailureActionId action : kind.getActions()) { @@ -337,8 +331,7 @@ class FileRunEventServiceTest { @Test void theSameRowIsMineToOnePersonAndTheirsToAnother() { - // Why it is derived and not stored: one stored answer would be wrong for everyone but - // the person it was stored for. + // Why it is derived: a stored answer would be wrong for everyone but one person. FileRunEvent event = givenHitBy(ACTOR, FailureKind.UNKNOWN, TEAM, "f1"); assertThat(service.ownershipOf(event)).isEqualTo(Ownership.MINE); @@ -360,8 +353,7 @@ class FileRunEventServiceTest { @Test void theOwnerIsOfferedTheirDocumentAndNotTheReviewersView() { - // A member reading their own password failure: the document is theirs to open, and the - // processor view is for whoever reviews the team rather than owns the file. + // The document is theirs to open; the processor view is for whoever reviews the team. when(authority.canEditPolicies()).thenReturn(false); FileRunEvent mine = givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); @@ -373,8 +365,7 @@ class FileRunEventServiceTest { @Test void aReviewerReadingAColleaguesIsNotOfferedTheDocumentTheyDoNotHave() { - // Dropped rather than disabled: a greyed-out "View file" would read as the - // reviewer's permission problem, when it is simply not their document. + // Dropped, not disabled: greyed out would read as their permission problem. FileRunEvent theirs = givenHitBy( "colleague@example.com", @@ -401,8 +392,8 @@ class FileRunEventServiceTest { @Test void inheritedOwnerActionsComeBackDisabledWithTheReasonWhy() { - // The file was fed by a folder, bucket or webhook, and re-running it from there is work - // that has not landed. Said out loud rather than offered as a button that does nothing. + // No browser holds a source-fed file, so it is stated rather than offered as a dead + // button. FileRunEvent unattended = givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); @@ -434,9 +425,8 @@ class FileRunEventServiceTest { @Test void theOwnersActionsAreDisabledWhenTheRowNamesNoDocument() { - // A row with no document id has nothing to name even for the owner holding the file. - // Answered here rather than left to the client, which would otherwise report it "not on - // this device" while it sits in their own workbench. + // Answered here, or the client calls it "not on this device" while it sits in their + // own workbench. FileRunEvent documentless = givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, null); @@ -465,8 +455,7 @@ class FileRunEventServiceTest { @Test void aMemberIsNotOfferedTheOwnerActionsOnAnUnattendedRow() { - // The inheritance is the reviewer's, not everybody's. A member has no claim on a run - // nobody attended, and cannot see one in the first place. + // The inheritance is the reviewer's: a member has no claim on a run nobody attended. when(authority.canEditPolicies()).thenReturn(false); FileRunEvent unattended = givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); @@ -476,8 +465,8 @@ class FileRunEventServiceTest { @Test void aLoginDisabledOperatorKeepsTheirOwnActions() { - // Its rows are unowned because it has no users, not because nothing attended them, and - // the one operator is holding the document. Disabling their retry would be wrong. + // Unowned for want of users, not because nothing attended: the one operator holds the + // file. ApplicationProperties props = new ApplicationProperties(); props.getSecurity().setEnableLogin(false); FileRunEventService unsecured = @@ -679,16 +668,14 @@ class FileRunEventServiceTest { complete.verifyEveryDeclaredActionHasAHandler(); for (FailureActionId id : FailureActionId.values()) { - // Only the server's actions need a handler: the rest are run by the client, which - // is why the boot check no longer asks about them. + // Only server actions need a handler, which is why the boot check ignores the rest. assertThat(complete.find(id).isPresent()).isEqualTo(id.runsOnServer()); } } @Test void doesNotAskForAHandlerForAnActionTheClientRuns() { - // Every kind declares client actions, so a registry holding only the server's two must - // still boot; otherwise every client action would need an empty handler beside it. + // Otherwise every client action would need an empty handler beside it. FailureActionRegistry serverOnly = new FailureActionRegistry( List.of(new AcknowledgeAction(store), new DismissAction(store))); @@ -699,16 +686,14 @@ class FileRunEventServiceTest { @Test void refusesAHandlerForAnActionTheClientRuns() { - // It could never be reached: dispatch refuses the id before resolving a handler. A bean - // that is never called is worse than no bean, because it reads as though it were. + // Dispatch refuses the id before resolving a handler, so the bean reads as live and is + // not. assertThatThrownBy(() -> new FailureActionRegistry(List.of(new ClientSideAction()))) .isInstanceOf(IllegalStateException.class) .hasMessageContaining("VIEW_FILE"); } - /** - * A handler for an action the client runs, which is exactly what must not be registered. - */ + /** A handler for a client action, which is exactly what must not be registered. */ private static final class ClientSideAction implements FailureAction { @Override diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java index 56ad40979a..af1353477d 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java @@ -241,9 +241,8 @@ class FileRunEventStoreDbTest { store.record( RecordFailure.forEditor( FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1", "boom")); - // A policy the user's own upload triggered. Recorded by the processor, but about the very - // document they just deleted, and carrying the id their client minted for it. Keying on - // origin left these behind: the user deleted the file and the failure stayed in the queue. + // Recorded by the processor, about the document they just deleted. Keying on origin left + // these in the queue. FileRunEvent myPolicyRun = store.record(failure(FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1")); FileRunEvent theirs = @@ -278,11 +277,8 @@ class FileRunEventStoreDbTest { @Test @DisplayName("a source-fed incident survives a client naming its file id") void markFilesRemovedLeavesSourceFedRowsAlone() { - // The two id spaces meet here. A source-fed run's fileId is a one-way hash of a path or key - // that was never on any device, so no client can legitimately claim to have deleted it. - // With login disabled the actor is null on both sides, so the actor clause matches and the - // absence of a source is the only thing standing between a local delete and a sweep's - // incidents. + // With login disabled the actor is null on both sides, so the absence of a source is all + // that stands between a local delete and a sweep's incidents. FileRunEvent sweep = store.record( new RecordFailure( diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java index b0ddd09fcc..e76a8b96ee 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java @@ -22,9 +22,8 @@ import stirling.software.proprietary.notification.NotificationView; import stirling.software.proprietary.policy.config.PolicyManagementAuthority; /** - * What the bell is given to render. Lives beside the failure tests because the invariants are - * failure invariants: the bell is never handed a raw event id, and it is offered only the actions - * the client itself runs, resolved for this reader by the same service that scopes the queue. + * What the bell is given to render: never a raw event id, and only the actions the client itself + * runs, resolved for this reader by the same service that scopes the queue. */ @ExtendWith(MockitoExtension.class) class NotificationProjectionTest { @@ -95,12 +94,10 @@ class NotificationProjectionTest { assertThat(notification.status()).isEqualTo(FileRunEventStatus.NEW); assertThat(notification.fileId()).isEqualTo("f-1"); assertThat(notification.policyId()).isNull(); - // No source fed it, which is how the client knows the fileId above is one of its own - // references and worth looking up locally. + // How the client knows the fileId above is one of its own and worth looking up. assertThat(notification.sourceId()).isNull(); assertThat(notification.defaultTitle()).isNotBlank(); - // The failure queue's own resolved offers, minus the ones the server runs: a bell - // offering different client actions from the queue would be a bell that lies. + // The queue's own offers minus the server's: a bell offering different ones would lie. assertThat(notification.actions()) .containsExactlyElementsOf( FileRunEventView.of(mine, failures.availableActions(mine)) @@ -115,8 +112,7 @@ class NotificationProjectionTest { @Test void offersNoActionTheServerRunsBecauseDispositionsBelongToTheQueue() { - // The bell routes the user to the right surface; deciding a failure's fate (dismissing - // it) happens on the review surface, so no disposition button reaches the panel. + // Deciding a failure's fate belongs to the review surface, not the panel. given(FailureKind.INPUT_PASSWORD_PROTECTED, ACTOR, "f-1"); assertThat(controller.list(null).notifications().getFirst().actions()) @@ -126,8 +122,8 @@ class NotificationProjectionTest { @Test void namesTheSourceThatFedAnUnattendedRunSoItsFileIdIsNotMistakenForAClientsOwn() { - // Only a source-fed run's fileId is a hash, so the source is the discriminator: without - // it a client would look up a hash it can never resolve and call the document missing. + // Without the source a client looks up a hash it can never resolve and calls it + // missing. store.record( RecordFailure.forRun( FailureKind.INPUT_PASSWORD_PROTECTED, @@ -147,8 +143,7 @@ class NotificationProjectionTest { @Test void aColleaguesNotificationOffersTheReviewersActionsOnly() { - // The bell shows a leader their team's failures, so the audience filtering has to reach - // it: no offering someone a document they do not have. + // A leader sees the team's failures, so audience filtering has to reach the bell too. given(FailureKind.INPUT_PASSWORD_PROTECTED, "colleague@example.com", "f-1"); assertThat(controller.list(null).notifications().getFirst().actions()) diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java index bbbbc55c06..0ae6c3a945 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java @@ -55,15 +55,9 @@ import stirling.software.proprietary.policy.store.PolicyStore; import tools.jackson.databind.json.JsonMapper; /** - * What a reader is offered on a real recorded row, with every collaborator between the failing tool - * call and the offered actions being the real one. That {@code actor} names whoever triggered the - * run is settled upstream by {@code PolicyFailureAttributionTest}; what is pinned here is what the - * reader's relationship to that actor then entitles them to. - * - *

Ownership is derived per reader rather than stored, so the same row answers differently to the - * person holding the document and to whoever reviews after them. Both directions are asserted, - * because an action offered to the wrong one is either a button that cannot work or a document - * handed to someone who should not have it. + * What a reader is offered on a real recorded row, every collaborator being the real one. Both + * directions are asserted: offered to the wrong reader is either a dead button or a leaked + * document. */ @ExtendWith(MockitoExtension.class) class PolicyFailureOwnershipTest { @@ -150,9 +144,7 @@ class PolicyFailureOwnershipTest { TEAM); } - /** - * Run the shared policy so its single tool step fails, as {@code triggeredBy} (null = sweep). - */ + /** Fails the policy's single tool step as {@code triggeredBy} (null = sweep). */ private void runAndFail(String triggeredBy, String sourceId, String fileIdentity) throws Exception { when(internalApiClient.post(eq(ROTATE), any())).thenThrow(new RuntimeException("boom")); @@ -183,10 +175,8 @@ class PolicyFailureOwnershipTest { } /** - * Read the single recorded row as {@code reader}, who is a plain member of the team. Lenient - * because a leader's read scope and an UNOWNED ownership check both answer without asking who - * is reading, so whether the name is consulted is the behaviour under test rather than a - * mistake. + * Lenient because a leader's scope and an UNOWNED check both answer without asking who reads, + * so whether the name is consulted is the behaviour under test. */ private FileRunEvent asMember(String reader) { lenient().when(userService.getCurrentUsername()).thenReturn(reader); @@ -231,9 +221,7 @@ class PolicyFailureOwnershipTest { void thePolicyOwnerIsNotHandedADocumentSheNeverTouched() throws Exception { runAndFail("bob", null, "bob-doc-1"); - // Alice owns the policy and pays for the run, but her browser has no copy of Bob's - // file. Offering her the document produced a button that vanished client-side with no - // explanation. + // She owns the policy and pays for the run, and still has no copy of Bob's file. FileRunEvent theirs = asReviewer("alice"); assertThat(service.ownershipOf(theirs)).isEqualTo(Ownership.THEIRS); assertThat(offeredTo(theirs)).doesNotContain(FailureActionId.VIEW_FILE); @@ -243,8 +231,7 @@ class PolicyFailureOwnershipTest { void theReviewerIsStillOfferedWhatReviewingNeeds() throws Exception { runAndFail("bob", null, "bob-doc-1"); - // Not her document, but still her team's incident: she gets the run and a way to close - // the row. + // Not her document, still her team's incident. assertThat(offeredTo(asReviewer("alice"))) .contains(FailureActionId.VIEW_IN_PROCESSOR, FailureActionId.DISMISS); } @@ -260,9 +247,7 @@ class PolicyFailureOwnershipTest { FileRunEvent unattended = asReviewer("alice"); assertThat(service.ownershipOf(unattended)).isEqualTo(Ownership.UNOWNED); - // The inheritance and its copy are reachable for the exact scenario they exist for: no - // browser holds this document, so the offer is stated and disabled rather than silently - // dropped. + // No browser holds this document, so the offer is stated and disabled, not dropped. assertThat(offeredTo(unattended)).contains(FailureActionId.VIEW_FILE); assertThat(service.availableActions(unattended)) .filteredOn(action -> action.id() == FailureActionId.VIEW_FILE) @@ -277,8 +262,8 @@ class PolicyFailureOwnershipTest { @Test void thePolicyOwnerDoesNotInheritItAsHerOwn() throws Exception { - // Alice is billed for the sweep, and that must not become ownership: she is offered the - // owner actions here only as the team's reviewer, and disabled, not as MINE. + // Being billed for the sweep must not become ownership: she gets these as reviewer + // only. runAndFail(null, "src-watched-folder", "file-hash-1"); assertThat(service.ownershipOf(asReviewer("alice"))).isNotEqualTo(Ownership.MINE); diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java index 519d5296ae..7472379d36 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java @@ -383,12 +383,8 @@ class PolicyFailureRecorderTest { @Test void theSameDocumentFailingInTwoAttendedRunsIsOneIncident() { - // WHAT THE DOCUMENT REFERENCE BUYS. A password-protected input is FILE-scoped, and - // every - // upload is a new run, so with no reference the run id stands in for the document and - // the - // same broken file re-uploaded reads as a second incident instead of a second - // occurrence. + // Every upload is a new run, so with no reference the run id stands in for the document + // and the same broken file reads as a second incident rather than a second occurrence. when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( @@ -403,8 +399,7 @@ class PolicyFailureRecorderTest { @Test void twoDocumentsFailingTheSameWayStaySeparateIncidents() { - // The other half of the rule: folding is per document, so two broken uploads are still - // two rows and neither is credited with the other's occurrence. + // Folding is per document, so neither row is credited with the other's occurrence. when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java index c56a7796b1..c876644133 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java @@ -57,10 +57,8 @@ class RecordFailurePrivacyTest { @Test void theRunRequestThatSuppliesADocumentReferenceCarriesNoNameEither() { - // An attended run sends its own opaque reference to the document so a failure of it can be - // resolved back to that document. The same discipline applies at the door as in the row: an - // id and nothing else, or a document name reaches the wire on the way to a table that - // deliberately has nowhere to put it. + // The same discipline at the door as in the row: an id and nothing else, or a document name + // reaches a table that deliberately has nowhere to put it. assertThat(List.of(PolicyRunFiles.class.getDeclaredFields())) .extracting(Field::getName) .contains("fileId") diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java index 3da6295908..345810474f 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java @@ -86,8 +86,7 @@ class PolicyControllerTest { @Mock private ProcessedLedger processedLedger; - // Real, not mocked: the run endpoints spool their uploads through it, and a test that supplies - // files needs them to actually land somewhere. + // Real, not mocked: the run endpoints spool uploads through it. private final TempFileManager tempFileManager = new TempFileManager(new TempFileRegistry(), new ApplicationProperties()); @@ -669,9 +668,7 @@ class PolicyControllerTest { @DisplayName("runStoredPolicy") class RunStoredPolicy { - /** - * The run files an editor sends: the documents, plus its own id for a single one of them. - */ + /** What an editor sends: the documents, plus its own id for a single one of them. */ private PolicyRunFiles filesWith(String fileId, int documents) { PolicyRunFiles files = new PolicyRunFiles(); files.setFileId(fileId); @@ -689,7 +686,6 @@ class PolicyControllerTest { return files; } - /** The document reference the run was actually started with. */ private String documentReferenceOf(PolicyRunFiles files) throws Exception { Policy p = policy("a", 1L); when(policyStore.get("a")).thenReturn(Optional.of(p)); @@ -724,8 +720,8 @@ class PolicyControllerTest { @Test @DisplayName("records the caller's own id for a single-document run") void carriesTheCallersDocumentReference() throws Exception { - // The point of the whole field: a failure of this run names a document the client that - // started it can resolve, so the actions that need the bytes are reachable. + // The point of the field: a failure names a document the client that started it can + // resolve. assertThat(documentReferenceOf(filesWith("editor-file-1", 1))) .isEqualTo("editor-file-1"); } @@ -733,15 +729,14 @@ class PolicyControllerTest { @Test @DisplayName("records nothing when the run carries several documents") void refusesToGuessWhichOfSeveralDocumentsItIs() throws Exception { - // One incident, one file reference: naming one of several would attribute the - // failure to whichever document happened to be bound first. + // One incident, one reference: naming one of several would attribute it to whichever + // bound first. assertThat(documentReferenceOf(filesWith("editor-file-1", 3))).isNull(); } @Test @DisplayName("records nothing when the caller sent no id") void toleratesACallerThatSendsNoReference() throws Exception { - // Every other client (and every older one) keeps working exactly as before. assertThat(documentReferenceOf(filesWith(null, 1))).isNull(); } diff --git a/frontend/editor/src/core/components/notifications/BellIcon.tsx b/frontend/editor/src/core/components/notifications/BellIcon.tsx index 283606d258..3cbd55519c 100644 --- a/frontend/editor/src/core/components/notifications/BellIcon.tsx +++ b/frontend/editor/src/core/components/notifications/BellIcon.tsx @@ -1,7 +1,6 @@ /** - * Outlined bell. The bundled Material Symbols set only carries the filled variant, and a filled - * bell reads as a permanently-ringing one, so this mirrors the portal's own {@code BellIcon} - * rather than using it: core cannot import from portal. + * The bundled Material Symbols set only carries the filled variant, which reads as permanently + * ringing. Mirrors the portal's own icon rather than importing it: core cannot reach into portal. */ export function BellIcon({ size = 18 }: { size?: number }) { return ( diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx index 9237e9224c..94937a3b85 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx @@ -16,9 +16,8 @@ const render = (ui: Parameters[0]) => baseRender(ui, { wrapper: MantineProvider }); /** - * The bell renders whatever the server sends, and does with each row's actions only what the registry - * for this build says it can. Two things are its own and worth pinning: which notifications the user has - * already looked at, and how a row behaves around an action (message on failure, re-read on success). + * Two things are the bell's own and worth pinning: which notifications the user has already looked + * at, and how a row behaves around an action. */ const fetchNotifications = vi.fn(); @@ -27,8 +26,7 @@ vi.mock("@app/services/notifications", () => ({ fetchNotifications: (...args: unknown[]) => fetchNotifications(...args), })); -// The document lookups read IndexedDB, which jsdom has none of. Answered here so a row's -// availability is a fact of the test rather than of the environment. +// IndexedDB, which jsdom has none of. Answered here so availability is a fact of the test. const h = vi.hoisted(() => ({ hasLocalFile: true, specs: {} as Record< @@ -45,16 +43,14 @@ vi.mock("@app/services/localFilePresence", () => ({ hasLocalFile: () => Promise.resolve(h.hasLocalFile), })); -// Stands in for the layer that owns the destinations. Core's own registry is empty, so without -// this there are no client actions to test. +// Core's own registry is empty, so without this there are no client actions to test. vi.mock("@app/components/notifications/notificationActions", () => ({ useNotificationActions: () => h.specs, })); vi.mock("react-i18next", () => ({ useTranslation: () => ({ - // Mirrors i18next closely enough for this component: a string fallback, or an options object - // carrying defaultValue plus the values it interpolates. + // A string fallback, or an options object with defaultValue plus what it interpolates. t: (key: string, fallback?: unknown) => { if (typeof fallback === "string") return fallback; if (fallback && typeof fallback === "object") { @@ -152,8 +148,7 @@ describe("NotificationBell", () => { render(); await openPanel(); - // Opening is what marks them read: waiting for the close would leave the badge lit - // while the user is looking at the list. + // Opening marks them read: waiting for the close would leave the badge lit. await waitFor(() => expect(screen.queryByText("2")).toBeNull()); }); @@ -172,8 +167,7 @@ describe("NotificationBell", () => { }); it("keeps the division on screen after opening marks them read", async () => { - // The boundary is frozen on open. Read live it would collapse the moment the badge cleared, - // taking the divider with it while the user was still looking at the list. + // Frozen on open: read live it would collapse the moment the badge cleared. window.localStorage.setItem("stirling.notifications.lastSeenId", "b"); fetchNotifications.mockResolvedValue([ notification("a"), @@ -229,8 +223,7 @@ describe("NotificationBell", () => { }); it("treats everything as unread when the last seen one is gone", async () => { - // Dismissed or expired: we cannot tell how far the user got, so show them rather than - // silently marking the lot read. + // We cannot tell how far the user got, so show them rather than marking the lot read. window.localStorage.setItem( "stirling.notifications.lastSeenId", "vanished", @@ -273,7 +266,7 @@ describe("NotificationBell", () => { render(); await openPanel(); - // Named for the row they belong to: every button in the list says the same thing. + // Named for their row: every button in the list says the same thing. for (const id of ["VIEW_IN_PROCESSOR", "VIEW_FILE"]) expect( screen.getByRole("button", { name: `${id}: Unrecognised failure` }), @@ -382,8 +375,7 @@ describe("NotificationBell", () => { }); it("claims nothing about a device for a row it never looks up", async () => { - // A source-fed run's fileId is a server-side identity that was never on any device, so it is not - // probed. Absent lookups must not read as an absent document, and the server said nothing either. + // Never on any device, so never probed, and an absent lookup is not an absent document. h.hasLocalFile = false; fetchNotifications.mockResolvedValue([ notification("a", "Password-protected document", { @@ -403,8 +395,7 @@ describe("NotificationBell", () => { }); it("renders no button for an action the server would refuse, and says why in words", async () => { - // A greyed button on a failure it can never work for is false hope, so the next action takes the - // row and the reason becomes its note. + // A greyed button that can never work is false hope, so the reason becomes the row's note. h.specs = { VIEW_FILE: { available: () => true, run: vi.fn() }, VIEW_IN_PROCESSOR: { available: () => true, run: vi.fn() }, @@ -457,7 +448,7 @@ describe("NotificationBell", () => { render(); await openPanel(); - // The message and its reading aids remain, so the row still reads as a row. + // The message and its chips remain, so the row still reads as a row. expect(screen.getByText("Unrecognised failure")).toBeTruthy(); expect( screen.getByText("Not available for this notification."), diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.tsx index c9f7a8de6f..9cd4d1a2c9 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.tsx @@ -28,11 +28,8 @@ import type { NotificationDocumentState } from "@app/hooks/useNotifications"; import "@app/components/notifications/NotificationBell.css"; /** - * The bell and its panel. Lives in core because both shells mount it and the dependency only runs - * one way: the portal may import from core, never the reverse. - * - *

Renders whatever the server sends without knowing which subsystem produced it, or what any of its - * actions mean, so a new source or failure kind needs no change here. + * Renders whatever the server sends without knowing which subsystem produced it or what its actions + * mean, so a new source or failure kind needs no change here. In core because both shells mount it. */ export function NotificationBell() { const { t } = useTranslation(); @@ -43,16 +40,12 @@ export function NotificationBell() { const container = useRef(null); const headingId = useId(); /** - * The first notification the user had already seen when they opened the panel, which is where the - * new ones stop. Held as an id rather than a count because opening marks everything read, so a - * live count would collapse to zero and take the divider with it while they were reading. - * - * An id also survives the list changing underneath: one arriving on a poll lands above the - * divider, where it belongs, instead of shifting a frozen index onto the wrong row. + * Where the new ones stop, frozen on open. An id rather than a count because opening marks + * everything read, and because one arriving on a poll must land above the divider, not shift it. */ const [firstSeenId, setFirstSeenId] = useState(null); - // Fixed to the viewport, positioned from the trigger. The workbench bar clips its overflow, so - // an absolutely positioned panel is cut off by its own toolbar. + // Fixed to the viewport: the workbench bar clips its overflow, so an absolutely positioned panel + // would be cut off by its own toolbar. const [anchor, setAnchor] = useState<{ top: number; right: number } | null>( null, ); @@ -76,12 +69,11 @@ export function NotificationBell() { }; }, [open]); - // Opening is what marks them read, not closing: the user has seen them by then, and waiting - // until close would leave the badge lit while they are looking at the list. + // Opening marks them read, not closing: waiting would leave the badge lit while they read. const toggle = () => { setOpen((wasOpen) => { if (!wasOpen) { - // Read the boundary before marking, or there is nothing left to read. + // Before marking, or there is nothing left to read. setFirstSeenId(notifications[unreadCount]?.id ?? null); markAllSeen(); } @@ -90,12 +82,8 @@ export function NotificationBell() { }; /** - * How many of the listed notifications count as new, for this reading of the panel. Everything - * above it is new, everything from it down is earlier. - * - * No boundary id means everything was new when the panel opened, so the whole list is. A boundary - * that has since left the list (dismissed elsewhere, its document deleted) leaves nothing to - * divide on, and reads as "none new" rather than guessing at a row. + * How many count as new. No boundary id means all of them were; one that has since left the list + * leaves nothing to divide on, so it reads as none rather than guessing at a row. */ const boundaryIndex = firstSeenId ? notifications.findIndex((notification) => notification.id === firstSeenId) @@ -165,8 +153,8 @@ export function NotificationBell() { /> )} - {/* Only where there is something on both sides of it: a lone "Earlier" heading - over the whole list says nothing the empty badge has not already said. */} + {/* Only with something on both sides: a lone "Earlier" over everything says + nothing the empty badge has not. */} {index === dividedAt && dividedAt > 0 && (

  • void; } -/** - * One row. Its own component because the message its last attempt came back with and whether that - * message is expanded are per-row state the panel cannot hold. - */ +/** Its own component because the last attempt's message and its expanded state are per-row. */ function NotificationItem({ notification, unread, @@ -255,18 +239,16 @@ function NotificationItem({ hasLocalFile: documentState.hasLocalFile, }; - // What this device can actually do, in the order the kind declared. An id this build has never - // heard of is skipped rather than rendered unwired: the server ships new kinds, and new actions, - // ahead of the clients that understand them. + // An id this build has never heard of is skipped rather than rendered unwired: the server ships + // new kinds, and new actions, ahead of the clients that understand them. const usable = notification.actions.filter((offer) => { if (!offer.enabled) return false; const spec = registry[offer.id]; return spec ? spec.available(context) : false; }); - // Why the row is thin, when the server withheld something and said so. Only from an action this - // build would otherwise have rendered, so a reason about an action it cannot perform anyway is not - // presented as the row's explanation. + // Only from an action this build would otherwise have rendered: a reason about one it cannot + // perform anyway is not this row's explanation. const withheldReasonKey = notification.actions.find( (offer) => @@ -308,7 +290,7 @@ function NotificationItem({ await navigator.clipboard.writeText(notification.detail); setCopied(true); } catch { - // No clipboard permission. The message is on screen and selectable, so this needs no error. + // No clipboard permission, and the message is on screen and selectable anyway. } }; @@ -346,8 +328,6 @@ function NotificationItem({ > {notification.detail} - {/* Chrome for the message itself, never part of the action slots: reading the failure and - acting on it should not crowd each other out. */}