From de461c2cf8dd83ce6733fe826167eca3a231c072 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:30:34 +0200 Subject: [PATCH] feat(support): report both channel outcomes on the feedback breadcrumb (#1252) * feat(support): report both channel outcomes on the feedback breadcrumb support_feedback_submitted recorded only whether the email delivered, so "did the PostHog ticket actually open?" was unanswerable from PostHog. The first time Support shipped, the only way to check was to reproduce the submission with devtools open. Both channels fail silently from the user's side, which is why this is worth instrumenting: email is the delivery guarantee, so the UI shows success even when the ticket failed, and a ticket that never opened leaves nothing in PostHog Support to look at either. Adds `email`, `ticket` and a derived `lost` to the event. `delivered` is kept as-is so any existing insight filtering on it keeps working. ticket: 'unavailable' is deliberately distinct from 'failed'. Unavailable is the expected steady state (Support disabled, analytics disabled, self-hosted); failed means conversations were live and the call still did not land. Collapsing them would make the useful signal unalertable. `lost` is true only when the message reached neither channel, which is the one property worth an alert. Still carries no message body: a test pins that free text never appears in event properties. Co-Authored-By: Claude Opus 5 (1M context) * fix(support): run the ticket call concurrently and cap it Addresses CodeRabbit's three findings on #1252. The real one: submitFeedback awaited submitViaTicket AFTER the email, so a hung sendMessage would hold the confirmation dialog open for as long as it hung. The code comment claimed a slow ticket "must never delay" the user while the code did exactly that. Both channels now start together, so the user waits max(email, ticket) rather than the sum, and the ticket is additionally capped at 4s. On expiry it resolves to a new 'timeout' outcome rather than being rounded to 'failed', keeping "conversations were live but slow" distinguishable from "conversations errored". Email still decides ok either way. Also: renamed the lost-state test, which claimed both channels failed while configuring ticket: 'unavailable', and added the genuinely-failed case alongside it plus coverage for the hanging-call path. Reformatted the decision-log entry to the required [date] : shape. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- DECISIONS.md | 2 + lib/support/__tests__/submit-feedback.test.ts | 92 +++++++++++++++++++ lib/support/submit-feedback.ts | 78 +++++++++++++--- 3 files changed, 157 insertions(+), 15 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index 89d358c5..4605015a 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -623,3 +623,5 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-27] Fixed the "Underlag saknas on a verifikat that shows the invoice" contradiction by ANCHORING the floating document, not by loosening the missing-underlag predicate. The predicate's anchoring requirement (migration 20260724090000) is legally right: block_document_deletion keys on journal_entry_id, so an unanchored doc is deletable and must not silence the warning. The bug was that nothing ever anchored it, and delete_last_voucher actively un-anchors (it must: the FK is ON DELETE RESTRICT). So the fix re-anchors after a voucher delete, at every supplier-invoice payment path (dashboard + v1 mark-paid, dashboard + v1 match-supplier-invoice, which between them had zero, partial and cash-only coverage), and backfills the 5 prod rows. getJournalEntryUnderlagReferences now also withholds an unanchored document, so the verifikat view and the list can no longer state opposite things about the same row. Rejected the alternative (extend the WORM deletion guard to protect docs referenced by a supplier invoice, then drop the anchoring requirement everywhere): it touches an enforcement trigger for a strictly larger blast radius and leaves the document outside the guard until the trigger ships. [2026-07-27] Claude Code plugin `homepage` points at the GitHub README, not /docs/api/connect-claude: next.config.ts redirects /docs/api/* to the separate docs.gnubok.se repo, where the connect-claude page was never ported, so the documented URL 404s and the in-repo page is dead code. Submission to the Claude plugin directory cannot wait on a cross-repo docs port. [2026-07-27] Applied to anthropics/claude-plugins-community, not claude-plugins-official: the official marketplace is curated by Anthropic at its own discretion with no application process, and the submission form explicitly does not feed it. Community listing is the only route we control. +[2026-07-27] support_feedback_submitted now reports BOTH channels (email + ticket) with a derived `lost` flag, not just email delivery. Both channels fail silently from the user's side: email is the guarantee so the UI still shows success when only the ticket failed, and a ticket that never opened leaves nothing in PostHog Support to look at either. Answering "did the ticket open?" previously required reproducing it with devtools open, which is exactly what happened the first time Support shipped. ticket: 'unavailable' is kept distinct from 'failed' because unavailable is the expected steady state (Support off, analytics off) while failed means conversations were live and the call still did not land; only the second is worth alerting on. `lost` (neither channel worked) is the single property to alert on. Still carries no message body, pinned by a test. +[2026-07-27] support_feedback_submitted reports both channels (email + ticket) with a derived `lost` flag, and the ticket call runs concurrently with a 4s cap instead of being awaited after the email: both channels fail silently from the user's side, so "did the ticket open?" was previously only answerable by reproducing the submission with devtools open, and awaiting the ticket sequentially let a hung sendMessage hold the confirmation dialog open despite the code comment claiming it could not. ticket: 'unavailable' stays distinct from 'failed' and 'timeout' because unavailable is the expected steady state (Support off, analytics off, self-hosted) while the other two mean conversations were live and the call still did not land; only those deserve an alert. `lost` (neither channel worked) is the single property to alert on. Still carries no message body, pinned by a test. diff --git a/lib/support/__tests__/submit-feedback.test.ts b/lib/support/__tests__/submit-feedback.test.ts index d06b4df0..6c982208 100644 --- a/lib/support/__tests__/submit-feedback.test.ts +++ b/lib/support/__tests__/submit-feedback.test.ts @@ -94,6 +94,9 @@ describe('submitFeedback', () => { expect(captureMock).toHaveBeenCalledWith('support_feedback_submitted', { subject: 'Hjälpsida', delivered: true, + email: 'ok', + ticket: 'ok', + lost: false, }) // Free text is user content: it must never ride along as an event property. expect(JSON.stringify(captureMock.mock.calls)).not.toContain('känslig text') @@ -180,4 +183,93 @@ describe('submitFeedback', () => { expect(sendMessageMock).not.toHaveBeenCalled() }) }) + + describe('breadcrumb channel outcomes', () => { + it('reports ticket: ok when the ticket opened', async () => { + stubFetchOk() + await submitFeedback({ message: 'msg' }) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ ticket: 'ok', lost: false }) + ) + }) + + // 'unavailable' is the expected steady state (Support off, analytics off); + // 'failed' means conversations were live and the call still did not land. + // Only the second deserves an alert, so they must not collapse. + it("reports ticket: unavailable when conversations are not available", async () => { + isAvailableMock.mockReturnValue(false) + stubFetchOk() + await submitFeedback({ message: 'msg' }) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ ticket: 'unavailable' }) + ) + }) + + it('reports ticket: failed when sendMessage throws', async () => { + sendMessageMock.mockRejectedValueOnce(new Error('boom')) + stubFetchOk() + await submitFeedback({ message: 'msg' }) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ ticket: 'failed' }) + ) + }) + + it('reports email: failed but not lost when the ticket still opened', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, json: async () => ({}) })) + await submitFeedback({ message: 'msg' }) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ email: 'failed', ticket: 'ok', lost: false }) + ) + }) + + // The alerting signal: the user's message reached nobody at all. + it('sets lost when email failed and the ticket was unavailable', async () => { + isAvailableMock.mockReturnValue(false) + vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('down'))) + await submitFeedback({ message: 'msg' }) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ email: 'failed', ticket: 'unavailable', lost: true }) + ) + }) + + it('sets lost when email failed and the ticket genuinely errored', async () => { + sendMessageMock.mockRejectedValueOnce(new Error('boom')) + vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('down'))) + await submitFeedback({ message: 'msg' }) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ email: 'failed', ticket: 'failed', lost: true }) + ) + }) + + // A hung sendMessage must not hold the confirmation dialog open: the ticket + // is capped and reported as 'timeout', while email still decides ok. + it('does not let a hanging ticket call block the user', async () => { + vi.useFakeTimers() + sendMessageMock.mockImplementationOnce(() => new Promise(() => {})) + stubFetchOk() + const pending = submitFeedback({ message: 'msg' }) + await vi.advanceTimersByTimeAsync(5000) + const result = await pending + vi.useRealTimers() + + expect(result.ok).toBe(true) + expect(result.channels).toEqual(['email']) + expect(captureMock).toHaveBeenCalledWith( + 'support_feedback_submitted', + expect.objectContaining({ email: 'ok', ticket: 'timeout', lost: false }) + ) + }) + + it('still carries no message body', async () => { + stubFetchOk() + await submitFeedback({ subject: 'Moms', message: 'hemlig fritext om bolaget' }) + expect(JSON.stringify(captureMock.mock.calls)).not.toContain('hemlig fritext') + }) + }) }) diff --git a/lib/support/submit-feedback.ts b/lib/support/submit-feedback.ts index 28ea88c6..ba8d9d0d 100644 --- a/lib/support/submit-feedback.ts +++ b/lib/support/submit-feedback.ts @@ -46,19 +46,47 @@ async function submitViaEmail( } } +/** Outcome of each channel, for the analytics breadcrumb. */ +type ChannelOutcome = 'ok' | 'failed' | 'unavailable' | 'timeout' + +/** How long the ticket call may run before we stop waiting on it. The user is + * waiting on this dialog, and the ticket is a complement, not the delivery. */ +const TICKET_TIMEOUT_MS = 4000 + /** * Breadcrumb on the user's PostHog timeline so a support message is visible * next to the session replay that led to it: the genuinely useful half of what * the Recapt channel provided. NOT a delivery channel, and deliberately * carries no message body: free text is user content and would be PII in an * event property. Email remains the only thing that actually delivers. + * + * Both channels are reported, because both fail silently from the user's side. + * A ticket that never opened is invisible in the UI (email is the guarantee, + * so the user still sees success) and invisible in PostHog Support (no ticket + * exists to look at). Without `ticket` here, the only way to answer "did the + * ticket open?" is to reproduce it with devtools open, which is what happened + * the first time this shipped. + * + * 'unavailable' is kept distinct from 'failed' on purpose: unavailable is the + * expected steady state when Support is off or analytics is disabled, whereas + * failed means conversations were live and the call still did not land. Only + * the second is worth alerting on. */ -function noteInAnalytics({ subject }: SubmitFeedbackInput, delivered: boolean): void { +function noteInAnalytics( + { subject }: SubmitFeedbackInput, + outcomes: { email: boolean; ticket: ChannelOutcome } +): void { if (!isAnalyticsEnabled()) return try { posthog.capture('support_feedback_submitted', { subject: subject ?? null, - delivered, + // Kept for continuity: existing insights filter on `delivered`. + delivered: outcomes.email, + email: outcomes.email ? 'ok' : 'failed', + ticket: outcomes.ticket, + // True only when the user's message reached neither channel. This is the + // one that deserves an alert. + lost: !outcomes.email && outcomes.ticket !== 'ok', }) } catch { // Telemetry must never affect whether the user's message went out. @@ -76,15 +104,15 @@ function noteInAnalytics({ subject }: SubmitFeedbackInput, delivered: boolean): * Never throws and never blocks: if conversations are unavailable (support * disabled, no analytics, older SDK) the user still gets the email path. */ -async function submitViaTicket({ message, subject }: SubmitFeedbackInput): Promise { - if (!isAnalyticsEnabled()) return false +async function submitViaTicket({ message, subject }: SubmitFeedbackInput): Promise { + if (!isAnalyticsEnabled()) return 'unavailable' try { const conversations = posthog.conversations - if (!conversations?.isAvailable?.()) return false + if (!conversations?.isAvailable?.()) return 'unavailable' await conversations.sendMessage(composeTicketBody(message, subject)) - return true + return 'ok' } catch { - return false + return 'failed' } } @@ -92,21 +120,41 @@ function composeTicketBody(message: string, subject?: string): string { return subject ? `[${subject}]\n\n${message}` : message } -export async function submitFeedback(input: SubmitFeedbackInput): Promise { - // Email first and awaited on its own: it is the delivery guarantee, and a - // slow or failing ticket call must never delay or affect it. - const emailResult = await submitViaEmail(input) - const ticketOk = await submitViaTicket(input) +/** Resolve to `fallback` if the promise has not settled in time. Never rejects: + * submitViaTicket already swallows its own errors. */ +function withTimeout( + promise: Promise, + ms: number, + fallback: ChannelOutcome +): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(fallback), ms) + void promise.then((value) => { + clearTimeout(timer) + resolve(value) + }) + }) +} - noteInAnalytics(input, emailResult.ok) +export async function submitFeedback(input: SubmitFeedbackInput): Promise { + // Both channels start together, so the user waits max(email, ticket) rather + // than the sum. Email is the delivery guarantee and decides `ok`; the ticket + // is a complement, so it is additionally capped: a hung sendMessage must + // never hold the confirmation dialog open. It resolves to 'timeout' instead, + // which is reported rather than silently rounded to 'failed'. + const ticketPromise = submitViaTicket(input) + const emailResult = await submitViaEmail(input) + const ticket = await withTimeout(ticketPromise, TICKET_TIMEOUT_MS, 'timeout') + + noteInAnalytics(input, { email: emailResult.ok, ticket }) if (emailResult.ok) { - return { ok: true, channels: ticketOk ? ['email', 'ticket'] : ['email'] } + return { ok: true, channels: ticket === 'ok' ? ['email', 'ticket'] : ['email'] } } return { ok: false, - channels: ticketOk ? ['ticket'] : [], + channels: ticket === 'ok' ? ['ticket'] : [], error: emailResult.error, } }