diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index 162bbc4328..0a12ebe125 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -5067,7 +5067,7 @@ label = "Free credits" [notifications] adoptFailed = "The document was unlocked but could not be opened here. Try the tool directly." -empty = "Nothing to report." +empty = "You're all caught up." handoffUnavailable = "This browser will not let the processor pass the document to the editor. Open it from the editor instead." noDocumentLinked = "This failure is not linked to a specific document, so it cannot be opened or retried here." notOnThisDevice = "This document is not on this device, so it cannot be opened or retried here." @@ -5082,16 +5082,12 @@ unlockedRerunUndelivered = "The document was unlocked and the policy re-run star unread = "Unread" [notifications.action] +copiedLog = "Copied" +copyLog = "Copy log" failed = "That did not work. Try again in a moment." more = "More options" unavailable = "Not available for this notification." -[notifications.detail] -copied = "Copied" -copy = "Copy error" -less = "Show less" -more = "Show full message" - [notifications.section] earlier = "Earlier" new = "New" @@ -7434,11 +7430,11 @@ description = "Policy runs that fail will appear here with the actions you can t title = "No failures recorded" [portal.failures.kind.inputPasswordProtected] -description = "The pipeline could not open the document because it is password-protected. Unlock it and run it again, or skip this file." +description = "Your file is password protected, so the run could not read it." title = "Password-protected document" [portal.failures.kind.unknown] -description = "This run failed for a reason Stirling does not yet recognise. The raw message is shown below." +description = "Something went wrong that Stirling does not recognise yet." title = "Unrecognised failure" [portal.failures.origin] diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.css b/frontend/editor/src/core/components/notifications/NotificationBell.css index 081e2b12cd..59af38091c 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.css +++ b/frontend/editor/src/core/components/notifications/NotificationBell.css @@ -142,60 +142,6 @@ overflow-wrap: anywhere; } -/* Expanded, the message is the point of the row, so let it run and scroll rather than clamp. */ -.notification-bell__detail--full { - display: block; - max-height: 10rem; - overflow-y: auto; - -webkit-line-clamp: none; -} - -/* The message in its own box; copy and expand are corner glyphs, not buttons rivalling the row's actions. */ -.notification-bell__detailbox { - grid-column: 1 / -1; - position: relative; - margin-top: var(--sp-1, 0.25rem); - padding: 0.375rem 0.5rem; - border: 1px solid var(--c-border-subtle); - border-radius: var(--radius-sm, 0.25rem); - background: var(--c-surface-sunken, var(--c-bg-raised)); -} - -/* Inside the box: drop the row grid placement, and keep the two-line clamp clear of the icons. */ -.notification-bell__detailbox .notification-bell__detail { - grid-column: auto; - margin: 0; - padding-right: 2.75rem; -} - -.notification-bell__detailbox-actions { - position: absolute; - top: 0.25rem; - right: 0.25rem; - display: flex; - gap: 0.125rem; -} - -.notification-bell__iconbtn { - display: inline-flex; - align-items: center; - justify-content: center; - width: 1.25rem; - height: 1.25rem; - padding: 0; - border: none; - border-radius: var(--radius-sm, 0.25rem); - background: transparent; - color: var(--c-text-subtle); - cursor: pointer; -} - -.notification-bell__iconbtn:hover, -.notification-bell__iconbtn:focus-visible { - background: var(--c-hover); - color: var(--c-text); -} - /* Why the actions this row could have had are absent. Muted: it explains, it does not warn. */ .notification-bell__note { grid-column: 2; diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx index 8d0bebb0cf..6ffdca9c20 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx @@ -65,6 +65,8 @@ vi.mock("react-i18next", () => ({ useTranslation: () => ({ // A string fallback, or an options object with defaultValue plus what it interpolates. t: (key: string, fallback?: unknown) => { + // The kinds' sentences live in the locale files, so one stands in here. + if (key.endsWith(".description")) return "Kind description"; if (typeof fallback === "string") return fallback; if (fallback && typeof fallback === "object") { const options = fallback as Record; @@ -603,25 +605,43 @@ describe("NotificationBell", () => { expect(screen.getByLabelText("PDF password")).toBeTruthy(); }); - it("expands the message without touching the row's actions", async () => { + it("reads the kind's own words rather than the raw failure", async () => { + // A bell is not a log: the row gets a sentence, the message goes in the menu. + const stack = "org.apache.pdfbox.InvalidPasswordException"; fetchNotifications.mockResolvedValue([ - notification("a", "Unrecognised failure", { - detail: "org.apache.pdfbox.InvalidPasswordException", + notification("a", "Password-protected document", { + titleKey: "portal.failures.kind.inputPasswordProtected.title", + detail: stack, }), ]); render(); await openPanel(); - const expand = screen.getByRole("button", { - name: "Show full message: Unrecognised failure", - }); - fireEvent.click(expand); + expect(await screen.findByText("Kind description")).toBeTruthy(); + expect(screen.queryByText(stack)).toBeNull(); + }); - expect( - screen.getByRole("button", { name: "Show less: Unrecognised failure" }), - ).toBeTruthy(); - expect( - screen.getByRole("button", { name: "Copy error: Unrecognised failure" }), - ).toBeTruthy(); + it("keeps the log one click away, for a row whose only extra is the log", async () => { + h.specs = { VIEW_FILE: { available: () => true, run: vi.fn() } }; + const stack = "org.apache.pdfbox.InvalidPasswordException"; + const clipboard = vi.fn().mockResolvedValue(undefined); + Object.assign(navigator, { clipboard: { writeText: clipboard } }); + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + detail: stack, + actions: [offer("VIEW_FILE", "SECONDARY")], + }), + ]); + render(); + await openPanel(); + + fireEvent.click( + await screen.findByRole("button", { + name: "More options: Unrecognised failure", + }), + ); + fireEvent.click(await screen.findByRole("menuitem", { name: "Copy log" })); + + await waitFor(() => expect(clipboard).toHaveBeenCalledWith(stack)); }); }); diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.tsx index ae4f52d111..6947b90d00 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.tsx @@ -189,7 +189,7 @@ export function NotificationBell() { {notifications.length === 0 ? (

- {t("notifications.empty", "Nothing to report.")} + {t("notifications.empty", "You're all caught up.")}

) : (
    @@ -262,6 +262,11 @@ interface PasswordPrompt { * The server's reason wins, being about the failure rather than this browser. Otherwise only what we * actually looked up, so a row we never probed is never called absent. */ +/** The kind's own sentence, sharing the portal's copy. Empty for a kind this build has none for. */ +function summaryKeyOf(titleKey: string): string { + return titleKey.replace(/\.title$/, ".description"); +} + function noteFor( notification: AppNotification, documentState: NotificationDocumentState, @@ -300,7 +305,7 @@ interface NotificationItemProps { onRequestPassword: (prompt: PasswordPrompt) => void; } -/** Its own component because the last attempt's message and its expanded state are per-row. */ +/** Its own component because the last attempt's message and the copy state are per-row. */ function NotificationItem({ notification, unread, @@ -312,7 +317,6 @@ function NotificationItem({ const { t } = useTranslation(); const [message, setMessage] = useState(null); const [busy, setBusy] = useState(null); - const [expanded, setExpanded] = useState(false); const [copied, setCopied] = useState(false); const title = t(notification.titleKey, notification.defaultTitle); @@ -376,6 +380,7 @@ function NotificationItem({ }; const note = noteFor(notification, documentState, withheldReasonKey, t); + const summary = t(summaryKeyOf(notification.titleKey), { defaultValue: "" }); return (
  • )} - {notification.detail && ( -
    - {/* Copy and expand sit in the corner of the message, not as buttons of their own, so reading - the failure and acting on it do not crowd each other out. */} - - - - - - {notification.detail} - -
    - )} + {summary && {summary}} {note && {note}} @@ -469,7 +427,7 @@ function NotificationItem({ onRun={() => void run(secondary)} /> )} - {overflow.length > 0 && ( + {(overflow.length > 0 || notification.detail) && ( ))} + {notification.detail && ( + void copyDetail()} + > + {copied + ? t("notifications.action.copiedLog", "Copied") + : t("notifications.action.copyLog", "Copy log")} + + )} )} @@ -543,7 +511,6 @@ function ActionButton({ ); } -/* Inline so the message box needs no icon dependency: two small glyphs for copy and expand. */ const ICON_PROPS = { width: 14, height: 14, @@ -556,41 +523,6 @@ const ICON_PROPS = { "aria-hidden": true, }; -function CopyIcon() { - return ( - - - - - ); -} - -function CheckIcon() { - return ( - - - - ); -} - -function ExpandIcon() { - return ( - - - - - ); -} - -function CollapseIcon() { - return ( - - - - - ); -} - function MoreIcon() { return ( diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx b/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx index 52c4a5b186..58a8d41871 100644 --- a/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx @@ -421,7 +421,9 @@ describe("useNotificationActions", () => { it("replaces the encrypted original in place when it is open in the workbench", async () => { // The failed document is on screen, so the unlock versions it rather than adding a second copy. openFileIds = ["f-1"]; - openFilesById = { "f-1": { id: "f-1", name: "invoice.pdf", versionNumber: 1 } }; + openFilesById = { + "f-1": { id: "f-1", name: "invoice.pdf", versionNumber: 1 }, + }; unlockLocalDocument.mockResolvedValue({ ok: true, files: [ @@ -448,6 +450,30 @@ describe("useNotificationActions", () => { ).toBe(true); }); + it("versions a document the workbench has closed, rather than adding a copy of it", async () => { + // Closed in the sidebar but still on the device: adding here left the user holding both. + openFileIds = []; + openFilesById = {}; + + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"); + + expect(addFiles).not.toHaveBeenCalled(); + expect(consumeFiles.mock.calls[0][0]).toEqual(["f-1"]); + }); + + it("adds the unlocked document when nothing on this device holds the original", async () => { + // No stub anywhere: there is no version chain to extend, so adding is all that is left. + h.getStirlingFileStub.mockResolvedValue(null); + + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"); + + expect(consumeFiles).not.toHaveBeenCalled(); + expect(addFiles.mock.calls[0][1]).toEqual({ + selectFiles: true, + derivedFromTool: true, + }); + }); + it("closes the incident only once the document is safely in", async () => { // Reported first, then a failed adoption, would leave the row closed with nothing to show. retryWithPassword.mockResolvedValue({ @@ -557,6 +583,52 @@ describe("useNotificationActions", () => { expect(navigate).toHaveBeenCalledWith("/processor/documents#failures"); }); + it("opens a new tab rather than costing the reader a loaded workbench", () => { + // Navigating away would unload their files, costing them every upload again. + openFileIds = ["f-1"]; + const openTab = vi.spyOn(window, "open").mockReturnValue({} as Window); + + registry().VIEW_IN_PROCESSOR?.run(context()); + + expect(openTab).toHaveBeenCalledWith( + "/processor/documents#failures", + "_blank", + "noopener", + ); + expect(navigate).not.toHaveBeenCalled(); + openTab.mockRestore(); + }); + + it("navigates in place when the tab would be refused", () => { + openFileIds = ["f-1"]; + const openTab = vi.spyOn(window, "open").mockReturnValue(null); + + registry().VIEW_IN_PROCESSOR?.run(context()); + + expect(navigate).toHaveBeenCalledWith("/processor/documents#failures"); + openTab.mockRestore(); + }); + + it("navigates in place from an empty workbench, which costs the reader nothing", () => { + const openTab = vi.spyOn(window, "open"); + + registry().VIEW_IN_PROCESSOR?.run(context()); + + expect(openTab).not.toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith("/processor/documents#failures"); + openTab.mockRestore(); + }); + + it("navigates in place from the processor, which has no workbench to lose", () => { + const openTab = vi.spyOn(window, "open"); + + registry(inProcessor).VIEW_IN_PROCESSOR?.run(context()); + + expect(openTab).not.toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith("/processor/documents#failures"); + openTab.mockRestore(); + }); + it("offers the processor link whenever the server did", () => { // The server only sends it to someone it will let read the queue. expect( @@ -692,9 +764,9 @@ describe("retrying an attended policy run", () => { it("unlocks, takes the document in, runs the policy again, then closes the incident", async () => { const order: string[] = []; - addFiles.mockImplementation(async () => { + consumeFiles.mockImplementation(async () => { order.push("adopt"); - return [{ fileId: "f-unlocked" }]; + return ["f-unlocked"]; }); rechainPolicyOnDocument.mockImplementation(async () => { order.push("rerun"); @@ -713,14 +785,17 @@ describe("retrying an attended policy run", () => { expect(outcome).toEqual({ ok: true }); // The unlock is the remove-password call on the document the row names, not a stashed endpoint. expect(unlockLocalDocument).toHaveBeenCalledWith("f-1", "hunter2"); - // Added and selected, so the unlocked document is what is on screen once the panel closes. The - // encrypted original is left alone: the user never asked to lose it. - const [files, options] = addFiles.mock.calls[0]; + // Versioned onto the encrypted original, so there is one document rather than two. + expect(addFiles).not.toHaveBeenCalled(); + const [inputIds, files, stubs] = consumeFiles.mock.calls[0]; + expect(inputIds).toEqual(["f-1"]); expect((files as File[]).map((file) => file.name)).toEqual(["invoice.pdf"]); // derivedFromTool is what stops the adoption starting a SECOND run of this same policy: the // dispatch effect in usePolicyAutoRun treats a plain upload as work to enforce. A policy run is // a billed automation run, so a double dispatch double-charges and can open a second incident. - expect(options).toEqual({ selectFiles: true, derivedFromTool: true }); + expect( + (stubs as Array<{ derivedFromTool?: boolean }>)[0].derivedFromTool, + ).toBe(true); // Re-submitted under the ORIGINAL reference, so a second failure folds onto this same incident // instead of opening a new one about the same document - while the run's output is attributed to // the ADOPTED document, which is the one now in front of the user. @@ -744,13 +819,15 @@ describe("retrying an attended policy run", () => { // derivedFromTool above; see the gate's own test in usePolicyAutoRun.chain.test.tsx. expect(rechainPolicyOnDocument).toHaveBeenCalledTimes(1); expect(rerunPolicy).not.toHaveBeenCalled(); - expect(addFiles.mock.calls[0][1]).toMatchObject({ derivedFromTool: true }); + expect(consumeFiles.mock.calls[0][2][0]).toMatchObject({ + derivedFromTool: true, + }); }); it("still runs when the adoption reports no workspace id, rather than guessing one", async () => { // Nothing to attribute the output to, so the run goes untracked rather than being filed against // the encrypted original, which would version the wrong document. - addFiles.mockResolvedValue([]); + consumeFiles.mockResolvedValue([]); rechainPolicyOnDocument.mockResolvedValue({ ok: true, tracked: false }); await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"); @@ -778,7 +855,7 @@ describe("retrying an attended policy run", () => { "The document was unlocked and the policy re-run started, but its result cannot be delivered here, so this failure stays open.", }); // Adopted regardless: the password bought them the unlocked document either way. - expect(addFiles).toHaveBeenCalled(); + expect(consumeFiles).toHaveBeenCalled(); expect(reportNotificationResolved).not.toHaveBeenCalled(); }); @@ -811,7 +888,7 @@ describe("retrying an attended policy run", () => { }); it("neither re-runs nor closes the incident when the document cannot be taken in", async () => { - addFiles.mockRejectedValue(new Error("quota")); + consumeFiles.mockRejectedValue(new Error("quota")); expect( await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"), @@ -839,7 +916,7 @@ describe("retrying an attended policy run", () => { "The document was unlocked and opened here, but the policy could not be run on it again.", }); // Adopted anyway: the password bought them the unlocked document, and that is theirs to keep. - expect(addFiles).toHaveBeenCalled(); + expect(consumeFiles).toHaveBeenCalled(); // But nothing is fixed server-side, so the incident stays open. expect(reportNotificationResolved).not.toHaveBeenCalled(); }); diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts index c6d2de66f8..5d3f695aba 100644 --- a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts @@ -86,6 +86,11 @@ function takeSelection(): string | null { } } +/** False when the browser refused it, so the caller can fall back to navigating in place. */ +function openInNewTab(path: string): boolean { + return window.open(withBasePath(path), "_blank", "noopener") !== null; +} + /** * Not the router's `navigate`: the editor reads its tool from the URL on mount and on a history pop, * and a router push is neither, so the address would change and the workbench would not. @@ -415,13 +420,15 @@ export function useNotificationActions(): ClientActionRegistry { // It unlocked, so the user must end up holding it. A failed adoption fails the whole action: // claiming success and dropping the result leaves them nothing for the password they typed. const unlocked = asFiles(outcome.files ?? []); - // The original to version in place, when it is open here. Only a policy names one: a tool - // retry has no single input to replace, so it keeps adding its output. + // Only a policy names an original to version; a tool retry keeps adding its output. const originalId = target.kind === "policy" ? (target.policy.fileId as FileId) : null; - const parentStub = - (originalId && fileStore?.getState().files.byId?.[originalId]) || - null; + // Storage too, or a file merely closed in the sidebar ends up decrypted twice over. + const parentStub = originalId + ? (fileStore?.getState().files.byId?.[originalId] ?? + (await fileStorage.getStirlingFileStub(originalId)) ?? + null) + : null; let adopted: FileId[] = []; try { adopted = await adopt(fileContext.actions, parentStub, unlocked); @@ -478,7 +485,14 @@ export function useNotificationActions(): ClientActionRegistry { // is in portal/views/Documents, and both lift together. available: () => import.meta.env.DEV, closesPanel: true, - run: () => navigate(FAILURES_DESTINATION), + run: () => { + // Leaving would cost them a loaded workbench, and every file in it a re-upload. + const holdsFiles = (fileStore?.getState().files.ids.length ?? 0) > 0; + if (canOpenHere && holdsFiles && openInNewTab(FAILURES_DESTINATION)) { + return; + } + navigate(FAILURES_DESTINATION); + }, }; return { @@ -487,5 +501,5 @@ export function useNotificationActions(): ClientActionRegistry { VIEW_FILE: viewFile, VIEW_IN_PROCESSOR: viewInProcessor, }; - }, [canOpenHere, openInWorkbench, fileContext, navigate, t]); + }, [canOpenHere, openInWorkbench, fileContext, fileStore, navigate, t]); }