diff --git a/CHANGELOG.md b/CHANGELOG.md index 788c3e3..cb0dc62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable user-visible changes are recorded here. Versions follow [Semantic Ve ## Unreleased +### Added + +- A feedback form inside the web app. It opens from the account menu and the + Account page, and from the moments where something can go wrong: starting a + session, the session password gate, a session that ended, the vault setup + and unlock screens, error notices, the empty sessions list, and the delete + account form. Messages are kept by the service and, with `FEEDBACK_TO` + set, forwarded by email. Nothing from a terminal is attached. + ### Fixed - Widened the vault password fields on the vault setup, unlock, and Account diff --git a/app/.env.example b/app/.env.example index eeb0975..ccecd7f 100644 --- a/app/.env.example +++ b/app/.env.example @@ -21,3 +21,6 @@ MAIL_PROVIDER=sendgrid MAIL_API_KEY= MAIL_FROM=shell.online # MAIL_API_URL=https://api.example.com/emails + +# Where feedback sent from the app is forwarded. Empty keeps it in the database only. +FEEDBACK_TO= diff --git a/app/README.md b/app/README.md index ebd09a2..88c038a 100644 --- a/app/README.md +++ b/app/README.md @@ -41,6 +41,7 @@ The client reads `VITE_FIREBASE_*` at build time. The server uses: | `RELAY_URL` | Relay proxied through `/relay/*` | | `MAIL_API_KEY`, `MAIL_FROM` | Optional invitation email | | `MAIL_PROVIDER`, `MAIL_API_URL` | Optional non-SendGrid JSON provider | +| `FEEDBACK_TO` | Optional address that feedback sent from the app is forwarded to | | `TRUST_PROXY` | Set to `1` only behind a trusted proxy | See [`.env.example`](.env.example) for the complete development configuration. @@ -78,6 +79,8 @@ npx wrangler deploy --config wrangler.deploy.jsonc ``` Set `MAIL_API_KEY` with `wrangler secret put` if invitation email is enabled. +Set `FEEDBACK_TO` as a Worker variable to have feedback sent from the app +forwarded by email; without it, feedback is kept in the `feedback` table only. Keep Hyperdrive query caching disabled because the app depends on read-after-write consistency. diff --git a/app/docker-compose.yml b/app/docker-compose.yml index 79f510a..4a18847 100644 --- a/app/docker-compose.yml +++ b/app/docker-compose.yml @@ -45,6 +45,8 @@ services: # Only true behind a proxy that rewrites X-Forwarded-For; believing it # otherwise lets any caller pick a new address per request. TRUST_PROXY: ${TRUST_PROXY:-0} + # Where feedback sent from the app is forwarded. Empty keeps it in the database only. + FEEDBACK_TO: ${FEEDBACK_TO:-} ports: - "${PORT:-8080}:8080" restart: unless-stopped diff --git a/app/server/app.test.ts b/app/server/app.test.ts index 14e66f4..545254d 100644 --- a/app/server/app.test.ts +++ b/app/server/app.test.ts @@ -2526,3 +2526,58 @@ describe("team audit key", () => { }); }); }); + +describe("feedback", () => { + const message = { + kind: "problem", + body: "The gate never opened.", + surface: "session-gate", + route: "/sessions?open=s1#k3y", + app_version: "0.15.1", + can_reply: true, + context: { host: "laptop", status: "ended" }, + }; + + it("keeps a message with who sent it and where from", async () => { + const posted = await call("POST", "/api/feedback", { auth: await idToken(), body: message }); + expect(posted.status).toBe(201); + expect(posted.body.feedback.id).toMatch(/^fbk_/); + const [kept] = await store.feedback(); + expect(kept).toMatchObject({ + uid: "uid-1", + email: "ana@example.com", + kind: "problem", + body: "The gate never opened.", + surface: "session-gate", + route: "/sessions", + appVersion: "0.15.1", + canReply: true, + context: { host: "laptop", status: "ended" }, + }); + expect(kept.orgId).toBeTruthy(); + }); + + it("refuses without a signed-in user", async () => { + const posted = await call("POST", "/api/feedback", { body: message }); + expect(posted.status).toBe(401); + expect(await store.feedback()).toEqual([]); + }); + + it("refuses an empty message", async () => { + const posted = await call("POST", "/api/feedback", { auth: await idToken(), body: { ...message, body: " " } }); + expect(posted.status).toBe(400); + expect(posted.body.error).toBe("write something first"); + }); + + it("stops a flood from one person", async () => { + for (let index = 0; index < 5; index += 1) { + expect((await call("POST", "/api/feedback", { auth: await idToken(), body: message })).status).toBe(201); + } + const sixth = await call("POST", "/api/feedback", { auth: await idToken(), body: message }); + expect(sixth.status).toBe(429); + expect(Number(sixth.headers["Retry-After"])).toBeGreaterThan(0); + /* Another person is not the flood. */ + const other = await call("POST", "/api/feedback", { auth: await idToken({ sub: "uid-2", email: "bo@example.com" }), body: message }); + expect(other.status).toBe(201); + }); +}); diff --git a/app/server/app.ts b/app/server/app.ts index f814ee8..a074a34 100644 --- a/app/server/app.ts +++ b/app/server/app.ts @@ -44,6 +44,7 @@ import { import { recordAudit, assignSession, auditCsv, SEALED_KINDS } from "./routes/audit"; import { addComment, inbox, notifyAssigned, notifySessionStarted } from "./routes/social"; import { deleteAccount } from "./routes/account"; +import { submitFeedback } from "./routes/feedback"; import { callerAddress, rateLimiter } from "./lib/rate-limit"; import { logMailer, type Mailer } from "./lib/mail"; @@ -70,6 +71,11 @@ export interface AppOptions { * an invite whose link is perfectly good. */ mailer?: Mailer; + /** + * Where feedback sent from the app is forwarded. Absent keeps it in the + * store only, which is still the record; the mail is for whoever reads it. + */ + feedbackTo?: string; /** * Serves the built client for anything that is not an API route. Present * only in a deployment that serves the app and the API together; in @@ -121,6 +127,12 @@ const CREDENTIAL_ROUTES = new Set([ ]); const CREDENTIAL_BUCKET = { burst: 12, perSecond: 0.2 }; const GENERAL_BUCKET = { burst: 240, perSecond: 40 }; +/* + * Feedback, per person rather than per address. The form is one click from + * most screens, and a stuck key or a script must not fill the inbox it + * forwards to. Five, then one more every twelve minutes. + */ +const FEEDBACK_BUCKET = { burst: 5, perSecond: 5 / 3600 }; /* * This service answers JSON to a known origin and serves no markup of its own, @@ -307,6 +319,7 @@ export function createApp(options: AppOptions) { const webOrigin = options.webOrigin ?? allowedOrigins[0] ?? ""; const credentialLimit = rateLimiter(CREDENTIAL_BUCKET); const generalLimit = rateLimiter(GENERAL_BUCKET); + const feedbackLimit = rateLimiter(FEEDBACK_BUCKET); async function sessionsForMember(membership: Membership, sessions: SessionRecord[]) { const states = options.sessionLiveness @@ -1424,6 +1437,29 @@ export function createApp(options: AppOptions) { return send(response, 201, { comment: result.value }); } + /* ---- Feedback ---- */ + + if (route === "POST /api/feedback") { + const membership = await requireMember(request); + if (!membership) return send(response, 401, { error: "sign in first" }); + const allowance = feedbackLimit.take(membership.uid); + if (!allowance.ok) { + response.setHeader("Retry-After", String(Math.ceil(allowance.retryAfterMs / 1000))); + return send(response, 429, { error: "That is plenty for now. Try again in a little while." }); + } + const result = await submitFeedback( + store, + mailer, + membership, + await readBody(request), + String(request.headers["user-agent"] ?? ""), + options.feedbackTo, + log, + ); + if (!result.ok) return send(response, result.status, { error: result.error }); + return send(response, 201, { feedback: { id: result.value.id, at: result.value.at } }); + } + /* ---- Inbox ---- */ if (route === "GET /api/notifications") { diff --git a/app/server/import-json.ts b/app/server/import-json.ts index 9899d18..c4295e8 100644 --- a/app/server/import-json.ts +++ b/app/server/import-json.ts @@ -89,6 +89,10 @@ async function main(): Promise { await copy(`notification ${notification.id}`, () => target.putNotification(notification)); } + for (const feedback of await source.feedback(Number.MAX_SAFE_INTEGER)) { + await copy(`feedback ${feedback.id}`, () => target.putFeedback(feedback)); + } + console.log(`import: ${copied} records copied from ${file}, ${skipped} already present`); } finally { await target.close(); diff --git a/app/server/index.ts b/app/server/index.ts index 262d2c0..12bdd52 100644 --- a/app/server/index.ts +++ b/app/server/index.ts @@ -49,6 +49,7 @@ const server = createAccountsServer({ webOrigin: config.webOrigin, trustProxy: config.trustProxy, mailer: createMailer(config.mail), + feedbackTo: config.feedbackTo, serveClient: config.clientDir ? staticFiles(config.clientDir) : undefined, relay: forward ?? undefined, sessionLiveness: config.relayUrl ? relaySessionLiveness(config.relayUrl) : undefined, diff --git a/app/server/lib/config.ts b/app/server/lib/config.ts index da02a2c..1a1280c 100644 --- a/app/server/lib/config.ts +++ b/app/server/lib/config.ts @@ -45,6 +45,11 @@ export interface Config { * so an unconfigured deployment still creates a perfectly good invite. */ mail: { provider?: "sendgrid" | "json"; apiUrl?: string; apiKey?: string; from?: string }; + /** + * Where feedback sent from the app is forwarded by email. Absent keeps it + * in the database only, which is still the record. + */ + feedbackTo?: string; } export class ConfigError extends Error {} @@ -169,5 +174,6 @@ export function readConfig(env: NodeJS.ProcessEnv = process.env): Config { apiKey: env.MAIL_API_KEY?.trim() || undefined, from: env.MAIL_FROM?.trim() || undefined, }, + feedbackTo: env.FEEDBACK_TO?.trim() || undefined, }; } diff --git a/app/server/lib/mail-feedback.test.ts b/app/server/lib/mail-feedback.test.ts new file mode 100644 index 0000000..9a2a304 --- /dev/null +++ b/app/server/lib/mail-feedback.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from "vitest"; +import { feedbackMessage } from "./mail"; +import type { Feedback } from "./types"; + +const feedback: Feedback = { + id: "fbk_1", + uid: "uid-1", + email: "ana@example.com", + orgId: "org_1", + kind: "problem", + body: "The gate never opened & I gave up.", + surface: "session-gate", + route: "/sessions", + appVersion: "0.15.1", + userAgent: "Chrome 129 on macOS", + canReply: false, + context: { host: "laptop" }, + at: Date.UTC(2026, 8, 14, 12, 0, 0), +}; + +describe("feedbackMessage", () => { + it("says what kind of message it is and where it came from", () => { + const message = feedbackMessage(feedback, "team@example.com"); + expect(message.to).toBe("team@example.com"); + expect(message.subject).toBe("[shell.online feedback] Problem: The gate never opened & I gave up."); + expect(message.text).toContain("session-gate on /sessions"); + expect(message.text).toContain("host: laptop"); + expect(message.text).toContain("asked not to be written to"); + }); + + it("escapes what the sender typed before it is rendered as HTML", () => { + const message = feedbackMessage(feedback, "team@example.com"); + expect(message.html).not.toContain("never"); + expect(message.html).toContain("<b>never</b>"); + expect(message.html).toContain("& I gave up."); + }); + + it("shortens a long first line in the subject", () => { + const message = feedbackMessage({ ...feedback, body: `${"word ".repeat(30)}end` }, "team@example.com"); + expect(message.subject.length).toBeLessThan(110); + expect(message.subject.endsWith("…")).toBe(true); + }); +}); diff --git a/app/server/lib/mail.ts b/app/server/lib/mail.ts index 9fc96a8..839b7bf 100644 --- a/app/server/lib/mail.ts +++ b/app/server/lib/mail.ts @@ -1,3 +1,5 @@ +import type { Feedback } from "./types"; + /** * Sending an invitation by email. * @@ -322,3 +324,67 @@ export function invitationMessage(invitation: Invitation, now = Date.now()): Mes return { to: invitation.to, subject, html, text }; } + +/* ---- Feedback, forwarded to whoever reads it ---- */ + +const KIND_WORD: Record = { + problem: "Problem", + idea: "Idea", + question: "Question", +}; + +/** The first line of a message, short enough for a subject. */ +function summarize(body: string, limit = 60): string { + const flat = body.replace(/\s+/g, " ").trim(); + return flat.length > limit ? `${flat.slice(0, limit).trimEnd()}…` : flat; +} + +/** + * One message per piece of feedback, as plain as a forwarded note. + * + * Everything in it was typed by the sender or attached by their browser, so + * all of it is escaped. The reply address is in the body rather than in a + * Reply-To header: the sender chose whether to be written back to, and a + * header the mail client acts on by itself would make that choice for them. + */ +export function feedbackMessage(feedback: Feedback, to: string): Message { + const kind = KIND_WORD[feedback.kind]; + const subject = `[shell.online feedback] ${kind}: ${summarize(feedback.body)}`; + const reply = feedback.canReply + ? `${feedback.email} said it is fine to reply.` + : `${feedback.email} asked not to be written to about this.`; + const facts: [string, string][] = [ + ["From", reply], + ["Where", `${feedback.surface} on ${feedback.route || "/"}`], + ["App", `${feedback.appVersion || "unknown"} · ${feedback.userAgent || "unknown browser"}`], + ["Sent", new Date(feedback.at).toISOString()], + ...Object.entries(feedback.context).map(([key, value]): [string, string] => [key, value]), + ["Id", feedback.id], + ]; + + const text = [ + `${kind} from ${feedback.email}`, + "", + feedback.body, + "", + ...facts.map(([key, value]) => `${key}: ${value}`), + ].join("\n"); + + const rows = facts + .map( + ([key, value]) => + `${escape(key)}${escape(value)}`, + ) + .join(""); + const html = ` + +${escape(subject)} + +

${escape(kind)} sent from the shell.online app

+ 
${escape(feedback.body)}
+ ${rows}
+ +`; + + return { to, subject, html, text }; +} diff --git a/app/server/lib/migrations/011_feedback.sql b/app/server/lib/migrations/011_feedback.sql new file mode 100644 index 0000000..3106d61 --- /dev/null +++ b/app/server/lib/migrations/011_feedback.sql @@ -0,0 +1,27 @@ +-- What people tell us from inside the app. +-- +-- One row per message, with where in the app it was written: the control that +-- opened the form, the route, the build and the browser, and a few facts the +-- control attached, such as which kind of session was being started. Nothing +-- from a terminal: the app never has the plaintext, and the form says so. +-- +-- A message belongs to the service rather than to a team, so deleting the +-- account that sent it clears who sent it and keeps the words, the same way the +-- activity trail keeps what was typed. +CREATE TABLE IF NOT EXISTS feedback ( + id TEXT PRIMARY KEY, + uid TEXT NOT NULL, + email TEXT NOT NULL, + org_id TEXT, + kind TEXT NOT NULL CHECK (kind IN ('problem', 'idea', 'question')), + body TEXT NOT NULL, + surface TEXT NOT NULL, + route TEXT NOT NULL, + app_version TEXT NOT NULL, + user_agent TEXT NOT NULL, + can_reply BOOLEAN NOT NULL DEFAULT FALSE, + context JSONB NOT NULL DEFAULT '{}'::jsonb, + at BIGINT NOT NULL +); + +CREATE INDEX IF NOT EXISTS feedback_at ON feedback (at DESC); diff --git a/app/server/lib/store-conformance.test.ts b/app/server/lib/store-conformance.test.ts index 918ec33..712c816 100644 --- a/app/server/lib/store-conformance.test.ts +++ b/app/server/lib/store-conformance.test.ts @@ -2,7 +2,7 @@ import { afterAll, beforeEach, describe, expect, it } from "vitest"; import { MemoryStore } from "./store-memory"; import { PostgresStore } from "./store-postgres"; import { DELETED_ACCOUNT_MEMORY_MS, DELETED_ACTOR_EMAIL, type Store } from "./store"; -import type { AgentCommand, AuditEvent, CliToken, Notification, SessionRecord } from "./types"; +import type { AgentCommand, AuditEvent, CliToken, Feedback, Notification, SessionRecord } from "./types"; import type { Invite, Membership, Organization } from "./orgs"; /** @@ -127,7 +127,27 @@ function notification(overrides: Partial = {}): Notification { type Implementation = { name: string; open: () => Promise; reset: (store: Store) => Promise }; +function feedback(overrides: Partial = {}): Feedback { + return { + id: "fbk_1", + uid: "uid-1", + email: "ana@example.com", + orgId: "org_1", + kind: "problem", + body: "The gate did not open", + surface: "session-gate", + route: "/sessions", + appVersion: "0.15.1", + userAgent: "Chrome 129 on macOS", + canReply: true, + context: { host: "laptop" }, + at: 1000, + ...overrides, + }; +} + const TABLES = [ + "feedback", "deleted_accounts", "account_keys", "session_key_shares", @@ -870,6 +890,22 @@ for (const implementation of implementations) { }); }); + describe("feedback", () => { + it("keeps a message with where it came from, newest first", async () => { + await store.putFeedback(feedback()); + await store.putFeedback(feedback({ id: "fbk_2", kind: "idea", at: 3000, context: {} })); + const listed = await store.feedback(); + expect(listed.map((entry) => entry.id)).toEqual(["fbk_2", "fbk_1"]); + expect(listed[1]).toEqual(feedback()); + expect(await store.feedback(1)).toHaveLength(1); + }); + + it("orders messages sent in the same millisecond the same way every time", async () => { + for (const id of ["fbk_b", "fbk_c", "fbk_a"]) await store.putFeedback(feedback({ id })); + expect((await store.feedback()).map((entry) => entry.id)).toEqual(["fbk_c", "fbk_b", "fbk_a"]); + }); + }); + describe("claiming an organization on first sight", () => { /* * Signing in fires several requests at once. On a new account none of @@ -991,6 +1027,8 @@ for (const implementation of implementations) { }); await store.putNotification(notification()); await store.putNotification(notification({ id: "ntf_2", uid: "uid-1", actorUid: "uid-2" })); + await store.putFeedback(feedback()); + await store.putFeedback(feedback({ id: "fbk_bo", uid: "uid-2", email: "bo@example.com" })); await store.deleteAccount("uid-1", { orgId: "org_1", dissolve: false, successorUid: "uid-3" }, 5000); @@ -1005,6 +1043,12 @@ for (const implementation of implementations) { expect(await store.notificationsFor("org_1", "uid-1")).toEqual([]); expect(await store.notificationsFor("org_1", "uid-2")).toEqual([]); + /* Feedback is a message to the service: the words stay, the sender does not. */ + const messages = await store.feedback(); + const anas = messages.find((entry) => entry.id === "fbk_1"); + expect(anas).toMatchObject({ uid: "", email: DELETED_ACTOR_EMAIL, canReply: false, body: "The gate did not open" }); + expect(messages.find((entry) => entry.id === "fbk_bo")).toMatchObject({ uid: "uid-2", email: "bo@example.com" }); + const kept = (await store.listOrgSessions("org_1")).find((entry) => entry.id === "s2"); expect(kept?.assigneeUids).toEqual(["uid-3"]); expect(kept?.assigneeUid).toBe("uid-3"); diff --git a/app/server/lib/store-memory.ts b/app/server/lib/store-memory.ts index 0e7dd54..6387f72 100644 --- a/app/server/lib/store-memory.ts +++ b/app/server/lib/store-memory.ts @@ -18,6 +18,7 @@ import type { CliToken, Comment, Device, + Feedback, Notification, SessionKeyShare, SessionRecord, @@ -65,6 +66,7 @@ interface Shape { audit: AuditEvent[]; comments: Comment[]; notifications: Notification[]; + feedback: Feedback[]; accountKeys: AccountKey[]; deletedAccounts: { uid: string; deletedAt: number }[]; teamKeys: TeamKey[]; @@ -74,7 +76,7 @@ interface Shape { const EMPTY: Shape = { codes: [], tokens: [], sessions: [], commands: [], organizations: [], memberships: [], invites: [], audit: [], - comments: [], notifications: [], accountKeys: [], deletedAccounts: [], + comments: [], notifications: [], feedback: [], accountKeys: [], deletedAccounts: [], teamKeys: [], teamKeyShares: [], }; @@ -138,6 +140,7 @@ export class MemoryStore implements Store { audit: parsed.audit ?? [], comments: parsed.comments ?? [], notifications: parsed.notifications ?? [], + feedback: parsed.feedback ?? [], accountKeys: parsed.accountKeys ?? [], deletedAccounts: parsed.deletedAccounts ?? [], teamKeys: parsed.teamKeys ?? [], @@ -341,6 +344,13 @@ export class MemoryStore implements Store { for (const event of data.audit) { if (event.actorUid === uid) event.actorEmail = DELETED_ACTOR_EMAIL; } + /* The words stay, as a message to the service; who sent them does not. */ + for (const entry of data.feedback) { + if (entry.uid !== uid) continue; + entry.uid = ""; + entry.email = DELETED_ACTOR_EMAIL; + entry.canReply = false; + } data.deletedAccounts = [ ...data.deletedAccounts.filter((entry) => entry.uid !== uid), { uid, deletedAt: now }, @@ -837,6 +847,19 @@ export class MemoryStore implements Store { return this.data.organizations; } + /* ---- Feedback ---- */ + + async putFeedback(feedback: Feedback): Promise { + this.data.feedback.push(feedback); + this.flush(); + } + + async feedback(limit = 100): Promise { + return [...this.data.feedback] + .sort(byTime((entry) => entry.at, (entry) => entry.id, true)) + .slice(0, limit); + } + async tokensForImport(): Promise { return this.data.tokens; } diff --git a/app/server/lib/store-postgres.ts b/app/server/lib/store-postgres.ts index c5068b2..7c5ce01 100644 --- a/app/server/lib/store-postgres.ts +++ b/app/server/lib/store-postgres.ts @@ -20,6 +20,7 @@ import type { CliToken, Comment, Device, + Feedback, Notification, SessionKeyShare, SessionRecord, @@ -315,6 +316,24 @@ function toNotification(row: Row): Notification { }) as unknown as Notification; } +function toFeedback(row: Row): Feedback { + return { + id: row.id as string, + uid: row.uid as string, + email: row.email as string, + ...(row.org_id ? { orgId: row.org_id as string } : {}), + kind: row.kind as Feedback["kind"], + body: row.body as string, + surface: row.surface as string, + route: row.route as string, + appVersion: row.app_version as string, + userAgent: row.user_agent as string, + canReply: row.can_reply as boolean, + context: (row.context ?? {}) as Record, + at: row.at as number, + }; +} + /** * The production store. * @@ -992,6 +1011,11 @@ export class PostgresStore implements Store { uid, DELETED_ACTOR_EMAIL, ]); + /* The words stay, as a message to the service; who sent them does not. */ + await client.query("UPDATE feedback SET uid = '', email = $2, can_reply = FALSE WHERE uid = $1", [ + uid, + DELETED_ACTOR_EMAIL, + ]); await client.query( `INSERT INTO deleted_accounts (uid, deleted_at) VALUES ($1, $2) ON CONFLICT (uid) DO UPDATE SET deleted_at = EXCLUDED.deleted_at`, @@ -1519,6 +1543,38 @@ export class PostgresStore implements Store { return result.rowCount ?? 0; } + /* ---- Feedback ---- */ + + async putFeedback(feedback: Feedback): Promise { + await this.pool.query( + `INSERT INTO feedback (id, uid, email, org_id, kind, body, surface, route, app_version, user_agent, can_reply, context, at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13)`, + [ + feedback.id, + feedback.uid, + feedback.email, + feedback.orgId ?? null, + feedback.kind, + feedback.body, + feedback.surface, + feedback.route, + feedback.appVersion, + feedback.userAgent, + feedback.canReply, + JSON.stringify(feedback.context), + feedback.at, + ], + ); + } + + async feedback(limit = 100): Promise { + const rows = await this.rows( + 'SELECT * FROM feedback ORDER BY at DESC, id COLLATE "C" DESC LIMIT $1', + [limit], + ); + return rows.map(toFeedback); + } + /* ---- Housekeeping ---- */ async purgeExpired(now = Date.now()): Promise { diff --git a/app/server/lib/store.ts b/app/server/lib/store.ts index 100f5c2..0545cfc 100644 --- a/app/server/lib/store.ts +++ b/app/server/lib/store.ts @@ -7,6 +7,7 @@ import type { CliToken, Comment, Device, + Feedback, Notification, SessionKeyShare, SessionRecord, @@ -229,6 +230,14 @@ export interface Store { markNotificationRead(orgId: string, uid: string, id: string, now?: number): Promise; markAllNotificationsRead(orgId: string, uid: string, now?: number): Promise; + /* ---- Feedback ---- */ + putFeedback(feedback: Feedback): Promise; + /** + * Newest first. For an operator's export and for tests; the app never lists + * it. Deleting an account keeps its messages and clears who sent them. + */ + feedback(limit?: number): Promise; + /* ---- Housekeeping ---- */ purgeExpired(now?: number): Promise; close(): Promise; diff --git a/app/server/lib/types.ts b/app/server/lib/types.ts index 85604e8..b301cc4 100644 --- a/app/server/lib/types.ts +++ b/app/server/lib/types.ts @@ -169,6 +169,33 @@ export interface Notification { readAt?: number; } +/** + * Something a person told us from inside the app. + * + * Kept as it was sent, with where in the app it was written, so a report can + * be read without asking the reporter which screen they meant. Nothing from a + * terminal is in it: the app never has the plaintext, and the form says so. + */ +export interface Feedback { + id: string; + uid: string; + email: string; + orgId?: string; + kind: "problem" | "idea" | "question"; + body: string; + /** Which control opened the form: "new-session", "session-gate", and so on. */ + surface: string; + /** The app route it was sent from, path only. */ + route: string; + appVersion: string; + userAgent: string; + /** Whether the sender is happy to be written to about it. */ + canReply: boolean; + /** A few facts the surface attached, such as the kind of session being started. */ + context: Record; + at: number; +} + export interface SessionRecord { id: string; uid: string; diff --git a/app/server/routes/feedback.test.ts b/app/server/routes/feedback.test.ts new file mode 100644 index 0000000..49e4c1c --- /dev/null +++ b/app/server/routes/feedback.test.ts @@ -0,0 +1,139 @@ +import { describe, expect, it, vi } from "vitest"; +import { MemoryStore } from "../lib/store-memory"; +import type { Membership } from "../lib/orgs"; +import type { Message } from "../lib/mail"; +import { MAX_FEEDBACK, readFeedback, submitFeedback } from "./feedback"; + +const ana: Membership = { + orgId: "org_1", + uid: "uid-1", + email: "ana@example.com", + name: "Ana", + role: "owner", + joinedAt: 1000, +}; + +const valid = { + kind: "idea", + body: " Let me pin a session. ", + surface: "account-menu", + route: "/account?tab=vault#nope", + app_version: "0.15.1", + can_reply: true, + context: { host: "laptop", count: 3, "Bad Key": "x", empty: " " }, +}; + +function mailbox() { + const sent: Message[] = []; + return { sent, mailer: { send: async (message: Message) => void sent.push(message) } }; +} + +describe("readFeedback", () => { + it("keeps the message and the place it came from, and nothing else", () => { + const read = readFeedback(valid); + expect(read.ok).toBe(true); + if (!read.ok) return; + expect(read.value).toEqual({ + kind: "idea", + body: "Let me pin a session.", + surface: "account-menu", + route: "/account", + appVersion: "0.15.1", + canReply: true, + context: { host: "laptop" }, + }); + }); + + it("needs a kind it knows", () => { + const read = readFeedback({ ...valid, kind: "rant" }); + expect(read).toMatchObject({ ok: false, status: 400 }); + }); + + it("needs something written", () => { + const read = readFeedback({ ...valid, body: " " }); + expect(read).toMatchObject({ ok: false, status: 400, error: "write something first" }); + }); + + it("caps the length", () => { + const read = readFeedback({ ...valid, body: "x".repeat(MAX_FEEDBACK + 1) }); + expect(read).toMatchObject({ ok: false, status: 400 }); + expect(readFeedback({ ...valid, body: "x".repeat(MAX_FEEDBACK) }).ok).toBe(true); + }); + + it("keeps the attached facts small", () => { + const context: Record = {}; + for (let index = 0; index < 12; index += 1) context[`key_${index}`] = "v".repeat(500); + const read = readFeedback({ ...valid, context }); + if (!read.ok) throw new Error("expected ok"); + expect(Object.keys(read.value.context)).toHaveLength(8); + expect(read.value.context.key_0).toHaveLength(200); + }); + + it("names an unknown surface rather than trusting one", () => { + const read = readFeedback({ ...valid, surface: "Weird Surface!" }); + if (!read.ok) throw new Error("expected ok"); + expect(read.value.surface).toBe("unknown"); + }); + + it("treats a missing reply choice as no", () => { + const read = readFeedback({ ...valid, can_reply: "yes" }); + if (!read.ok) throw new Error("expected ok"); + expect(read.value.canReply).toBe(false); + }); +}); + +describe("submitFeedback", () => { + it("stores the message and forwards it when an address is set", async () => { + const store = MemoryStore.memory(); + const { sent, mailer } = mailbox(); + const log = vi.fn(); + const result = await submitFeedback(store, mailer, ana, valid, "Mozilla/5.0 Chrome/129", "team@example.com", log, 5000); + expect(result.ok).toBe(true); + const [kept] = await store.feedback(); + expect(kept).toMatchObject({ + uid: "uid-1", + email: "ana@example.com", + orgId: "org_1", + kind: "idea", + body: "Let me pin a session.", + surface: "account-menu", + route: "/account", + userAgent: "Mozilla/5.0 Chrome/129", + canReply: true, + context: { host: "laptop" }, + at: 5000, + }); + expect(kept.id).toMatch(/^fbk_/); + expect(sent).toHaveLength(1); + expect(sent[0].to).toBe("team@example.com"); + expect(sent[0].subject).toContain("Idea"); + expect(sent[0].text).toContain("ana@example.com"); + expect(sent[0].text).toContain("fine to reply"); + expect(log).not.toHaveBeenCalled(); + }); + + it("stores without forwarding when no address is set", async () => { + const store = MemoryStore.memory(); + const { sent, mailer } = mailbox(); + await submitFeedback(store, mailer, ana, valid, "", undefined, vi.fn()); + expect(await store.feedback()).toHaveLength(1); + expect(sent).toEqual([]); + }); + + it("keeps the message when the mail provider fails", async () => { + const store = MemoryStore.memory(); + const log = vi.fn(); + const mailer = { send: async () => { throw new Error("sendgrid returned 401"); } }; + const result = await submitFeedback(store, mailer, ana, valid, "", "team@example.com", log); + expect(result.ok).toBe(true); + expect(await store.feedback()).toHaveLength(1); + expect(log).toHaveBeenCalledOnce(); + }); + + it("refuses what it cannot read without writing anything", async () => { + const store = MemoryStore.memory(); + const result = await submitFeedback(store, mailbox().mailer, ana, { kind: "idea", body: "" }, "", undefined, vi.fn()); + expect(result).toMatchObject({ ok: false, status: 400 }); + expect(await store.feedback()).toEqual([]); + }); +}); diff --git a/app/server/routes/feedback.ts b/app/server/routes/feedback.ts new file mode 100644 index 0000000..8f000dd --- /dev/null +++ b/app/server/routes/feedback.ts @@ -0,0 +1,136 @@ +import type { Feedback, Store } from "../lib/store"; +import { newId, type Membership } from "../lib/orgs"; +import { feedbackMessage, type Mailer } from "../lib/mail"; +import type { Outcome } from "./social"; + +export const MAX_FEEDBACK = 4000; +export const FEEDBACK_KINDS = ["problem", "idea", "question"] as const; +export type FeedbackKind = (typeof FEEDBACK_KINDS)[number]; + +/* How much of the app's own bookkeeping one message may carry. */ +const MAX_ROUTE = 200; +const MAX_VERSION = 40; +const MAX_USER_AGENT = 300; +const MAX_CONTEXT_ENTRIES = 8; +const MAX_CONTEXT_VALUE = 200; + +/** What the browser sends: the message, and where in the app it was written. */ +export interface FeedbackInput { + kind: FeedbackKind; + body: string; + surface: string; + route: string; + appVersion: string; + canReply: boolean; + context: Record; +} + +function isKind(value: unknown): value is FeedbackKind { + return typeof value === "string" && (FEEDBACK_KINDS as readonly string[]).includes(value); +} + +/* + * The route the message came from, path only. A query string can name a + * session (?open=...) and a hash is where a share link keeps its key, so + * neither is kept even when the client sends them. + */ +function readRoute(value: unknown): string { + if (typeof value !== "string") return ""; + const path = value.split(/[?#]/, 1)[0].trim(); + return path.startsWith("/") ? path.slice(0, MAX_ROUTE) : ""; +} + +/* + * Small facts the surface attached: which kind of session was being started, + * which machine, what the error said. Bounded in count and size, and keyed by + * identifiers rather than free text, so the column stays a handful of labels + * and never becomes a second message body. + */ +function readContext(value: unknown): Record { + const context: Record = {}; + if (typeof value !== "object" || value === null) return context; + let kept = 0; + for (const [key, raw] of Object.entries(value as Record)) { + if (kept >= MAX_CONTEXT_ENTRIES) break; + if (!/^[a-z][a-z0-9_]{0,31}$/.test(key) || typeof raw !== "string") continue; + const trimmed = raw.trim(); + if (!trimmed) continue; + context[key] = trimmed.slice(0, MAX_CONTEXT_VALUE); + kept += 1; + } + return context; +} + +export function readFeedback(body: unknown): Outcome { + const given = (typeof body === "object" && body !== null ? body : {}) as Record; + if (!isKind(given.kind)) { + return { ok: false, status: 400, error: "say whether this is a problem, an idea or a question" }; + } + const text = typeof given.body === "string" ? given.body.trim() : ""; + if (!text) return { ok: false, status: 400, error: "write something first" }; + if (text.length > MAX_FEEDBACK) { + return { + ok: false, + status: 400, + error: `keep it under ${MAX_FEEDBACK} characters; you can send more than one`, + }; + } + /* The surface is an identifier the app chose, so anything else is "unknown" rather than kept. */ + const surface = + typeof given.surface === "string" && /^[a-z][a-z0-9-]{0,63}$/.test(given.surface) + ? given.surface + : "unknown"; + return { + ok: true, + value: { + kind: given.kind, + body: text, + surface, + route: readRoute(given.route), + appVersion: + typeof given.app_version === "string" ? given.app_version.trim().slice(0, MAX_VERSION) : "", + canReply: given.can_reply === true, + context: readContext(given.context), + }, + }; +} + +/** + * Keeps a message and, when an address is configured, forwards it. + * + * The row is the record; the email is a convenience for whoever reads them. + * So the mail is best effort, like an invitation: a provider having a bad + * afternoon must not turn a message somebody took the time to write into an + * error they have to read. + */ +export async function submitFeedback( + store: Store, + mailer: Mailer, + sender: Membership, + body: unknown, + userAgent: string, + forwardTo: string | undefined, + log: (message: string, error?: unknown) => void, + now = Date.now(), +): Promise> { + const read = readFeedback(body); + if (!read.ok) return read; + const feedback: Feedback = { + id: newId("fbk"), + uid: sender.uid, + email: sender.email, + orgId: sender.orgId, + ...read.value, + userAgent: userAgent.trim().slice(0, MAX_USER_AGENT), + at: now, + }; + await store.putFeedback(feedback); + if (forwardTo) { + try { + await mailer.send(feedbackMessage(feedback, forwardTo)); + } catch (error) { + log(`accounts: could not forward feedback ${feedback.id}`, error); + } + } + return { ok: true, value: feedback }; +} diff --git a/app/src/App.tsx b/app/src/App.tsx index 20f085e..a979ee8 100644 --- a/app/src/App.tsx +++ b/app/src/App.tsx @@ -16,6 +16,7 @@ import { Audit } from "./routes/Audit"; import { CliAuthorize } from "./routes/CliAuthorize"; import { VaultProvider } from "./vault/VaultProvider"; import { TeamKeyProvider } from "./vault/TeamKeyProvider"; +import { FeedbackProvider } from "./feedback/FeedbackProvider"; export default function App() { return ( @@ -23,6 +24,7 @@ export default function App() { + } /> } /> } /> + diff --git a/app/src/components/AppShell.tsx b/app/src/components/AppShell.tsx index d8b070a..4bb677c 100644 --- a/app/src/components/AppShell.tsx +++ b/app/src/components/AppShell.tsx @@ -1,12 +1,13 @@ import { useEffect, useRef, useState, type ReactNode } from "react"; import { Link, NavLink, useNavigate } from "react-router-dom"; import { - Terminal, Desktop, User, UsersThree, ClockCounterClockwise, SignOut, Copy, Check, Warning, + Terminal, Desktop, User, UsersThree, ClockCounterClockwise, SignOut, Copy, Check, Warning, ChatCircleDots, } from "@phosphor-icons/react"; import { Inbox } from "./Inbox"; import { Avatar } from "./Avatar"; import { Wordmark } from "./Wordmark"; import { useAuth } from "../auth/AuthProvider"; +import { useFeedback } from "../feedback/context"; import { COPY_FAILED, useCopy } from "../lib/clipboard"; const NAV = [ @@ -52,6 +53,7 @@ function AccountMenu() { const [open, setOpen] = useState(false); const wrapper = useRef(null); const navigate = useNavigate(); + const feedback = useFeedback(); useEffect(() => { if (!open) return; @@ -96,6 +98,18 @@ function AccountMenu() { Account + {/* Always one click away, from the rail and from the phone's top bar alike. */} + + {/* The one moment a reason is on the tip of the tongue. */} +

+ + Tell us why you are leaving + +

); } diff --git a/app/src/components/NewSessionModal.tsx b/app/src/components/NewSessionModal.tsx index 95f5eda..d34e358 100644 --- a/app/src/components/NewSessionModal.tsx +++ b/app/src/components/NewSessionModal.tsx @@ -3,6 +3,7 @@ import { createPortal } from "react-dom"; import { X, Warning } from "@phosphor-icons/react"; import { Button } from "./Button"; import { Alert } from "./Alert"; +import { FeedbackLink } from "../feedback/FeedbackLink"; import { SESSION_KINDS, sessionName, @@ -320,6 +321,28 @@ export function NewSessionModal({ No linked machine is available. Run shell login on one.

)} + + {/* + * Under the buttons, where somebody who could not start a session + * ends up. The facts a report needs most go with it: what was being + * started, on which machine, whether it was reachable, and what the + * form said if it refused. + */} +
+ + Something off with starting a session? Tell us + +
diff --git a/app/src/components/SignedInModal.tsx b/app/src/components/SignedInModal.tsx index 67651e1..d8d9831 100644 --- a/app/src/components/SignedInModal.tsx +++ b/app/src/components/SignedInModal.tsx @@ -2,6 +2,7 @@ import { useEffect, useRef } from "react"; import { createPortal } from "react-dom"; import { CheckCircle } from "@phosphor-icons/react"; import { Button } from "./Button"; +import { FeedbackLink } from "../feedback/FeedbackLink"; interface SignedInModalProps { onClose(): void; @@ -59,6 +60,13 @@ export function SignedInModal({ onClose }: SignedInModalProps) { + {/* The first thing a new machine's owner sees; the first place to hear about it. */} + + Anything confusing about linking? Tell us + , diff --git a/app/src/feedback/FeedbackLink.tsx b/app/src/feedback/FeedbackLink.tsx new file mode 100644 index 0000000..39d3b2f --- /dev/null +++ b/app/src/feedback/FeedbackLink.tsx @@ -0,0 +1,31 @@ +import type { ReactNode } from "react"; +import { ChatCircleDots } from "@phosphor-icons/react"; +import { useFeedback, type FeedbackRequest } from "./context"; + +interface FeedbackLinkProps extends FeedbackRequest { + children?: ReactNode; + className?: string; +} + +/** + * The line that opens the feedback sheet, from wherever something might have + * gone wrong. Quiet on purpose: it sits under a form or a notice and must not + * compete with the action the person came for. + */ +export function FeedbackLink({ + children = "Something off? Tell us", + className, + ...request +}: FeedbackLinkProps) { + const { open } = useFeedback(); + return ( + + ); +} diff --git a/app/src/feedback/FeedbackProvider.tsx b/app/src/feedback/FeedbackProvider.tsx new file mode 100644 index 0000000..05b80b7 --- /dev/null +++ b/app/src/feedback/FeedbackProvider.tsx @@ -0,0 +1,39 @@ +import { useCallback, useMemo, useState, type ReactNode } from "react"; +import { useLocation } from "react-router-dom"; +import { useAuth } from "../auth/AuthProvider"; +import { sendFeedback } from "../lib/api"; +import { routeForFeedback } from "../lib/feedback"; +import { FeedbackContext, type FeedbackRequest } from "./context"; +import { FeedbackSheet } from "./FeedbackSheet"; + +/** + * One sheet for the whole app, opened from wherever a FeedbackLink sits. + * + * Rendered here rather than by the link that opened it, so it outlives a + * modal that closes underneath it and reads the same on every screen. It + * needs a signed-in person: the message is recorded against the account, + * which is what makes a reply possible. + */ +export function FeedbackProvider({ children }: { children: ReactNode }) { + const { user } = useAuth(); + const location = useLocation(); + const [request, setRequest] = useState(null); + const open = useCallback((next: FeedbackRequest) => setRequest(next), []); + const close = useCallback(() => setRequest(null), []); + const value = useMemo(() => ({ open }), [open]); + + return ( + + {children} + {request && user && ( + + )} + + ); +} diff --git a/app/src/feedback/FeedbackSheet.tsx b/app/src/feedback/FeedbackSheet.tsx new file mode 100644 index 0000000..04c9c1c --- /dev/null +++ b/app/src/feedback/FeedbackSheet.tsx @@ -0,0 +1,234 @@ +import { useEffect, useRef, useState, type FormEvent } from "react"; +import { createPortal } from "react-dom"; +import { CheckCircle, X } from "@phosphor-icons/react"; +import { Button } from "../components/Button"; +import { Alert } from "../components/Alert"; +import { + APP_VERSION, + FEEDBACK_KINDS, + MAX_FEEDBACK, + describeBrowser, + surfaceLabel, + trimContext, + type FeedbackKind, + type FeedbackPayload, +} from "../lib/feedback"; +import type { FeedbackRequest } from "./context"; + +export interface FeedbackSheetProps { + /** Whose message it is, shown next to the reply choice so nothing is implied. */ + email: string; + /** The route it is sent from, path only. */ + route: string; + request: FeedbackRequest; + /** Defaults to this browser's. A parameter so the sheet can be rendered elsewhere. */ + userAgent?: string; + onSend(payload: FeedbackPayload): Promise; + onClose(): void; +} + +/** + * The form itself: a kind, a message, what travels with it, and whether a + * reply is welcome. + * + * Everything sent is on the screen. The "sent with your message" list is the + * actual context, not a summary of it, so nobody has to wonder what the app + * attached on their behalf. Nothing from a terminal is ever in it: the app + * never has the plaintext, and the line under the list says so. + */ +export function FeedbackSheet({ + email, + route, + request, + userAgent = navigator.userAgent, + onSend, + onClose, +}: FeedbackSheetProps) { + const [kind, setKind] = useState(request.kind ?? "problem"); + const [text, setText] = useState(""); + const [canReply, setCanReply] = useState(true); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(""); + const [sent, setSent] = useState(false); + const box = useRef(null); + const done = useRef(null); + const context = trimContext(request.context); + const option = FEEDBACK_KINDS.find((candidate) => candidate.id === kind) ?? FEEDBACK_KINDS[0]; + + useEffect(() => { + /* + * Capture phase, so Escape closes this sheet and stops there. It opens on + * top of other dialogs, which listen for the same key on the document, + * and one press must not close the form underneath as well. + */ + const onKey = (event: KeyboardEvent) => { + if (event.key !== "Escape") return; + event.stopPropagation(); + onClose(); + }; + document.addEventListener("keydown", onKey, true); + /* Restoring the literal earlier value, not "", so a nested open is safe. */ + const previous = document.body.style.overflow; + document.body.style.overflow = "hidden"; + return () => { + document.removeEventListener("keydown", onKey, true); + document.body.style.overflow = previous; + }; + }, [onClose]); + + /* The box first, then the one button left once the message has gone. */ + useEffect(() => { + const frame = requestAnimationFrame(() => (sent ? done.current : box.current)?.focus()); + return () => cancelAnimationFrame(frame); + }, [sent]); + + async function handleSubmit(event: FormEvent) { + event.preventDefault(); + const body = text.trim(); + if (!body || busy) return; + setBusy(true); + setError(""); + try { + await onSend({ + kind, + body, + surface: request.surface, + route, + app_version: APP_VERSION, + can_reply: canReply, + context, + }); + setSent(true); + } catch (caught) { + setError(caught instanceof Error ? caught.message : "Could not send that. Try again."); + } finally { + setBusy(false); + } + } + + return createPortal( +
event.target === event.currentTarget && onClose()} + > +
+ {sent ? ( +
+ +

Sent. Thank you.

+

+ {canReply ? ( + <> + If we have a question, we will write to {email}. + + ) : ( + "We will read it and, as you asked, not write back." + )} +

+ +
+ ) : ( + <> +
+

Tell us

+ +
+ +
+ {request.prompt &&

{request.prompt}

} + +
+ {FEEDBACK_KINDS.map((candidate) => ( + + ))} +
+ +
+ +