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) <noreply@anthropic.com> * 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] <decision>: <why> shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Opus 5
Claude Opus 5
parent
4dfdb35e13
commit
de461c2cf8
@@ -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')
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<boolean> {
|
||||
if (!isAnalyticsEnabled()) return false
|
||||
async function submitViaTicket({ message, subject }: SubmitFeedbackInput): Promise<ChannelOutcome> {
|
||||
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<SubmitFeedbackResult> {
|
||||
// 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<ChannelOutcome>,
|
||||
ms: number,
|
||||
fallback: ChannelOutcome
|
||||
): Promise<ChannelOutcome> {
|
||||
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<SubmitFeedbackResult> {
|
||||
// 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,
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user