Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions app/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ MAIL_PROVIDER=sendgrid
MAIL_API_KEY=
MAIL_FROM=shell.online <no-reply@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=
3 changes: 3 additions & 0 deletions app/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down
2 changes: 2 additions & 0 deletions app/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
55 changes: 55 additions & 0 deletions app/server/app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
36 changes: 36 additions & 0 deletions app/server/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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") {
Expand Down
4 changes: 4 additions & 0 deletions app/server/import-json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ async function main(): Promise<void> {
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();
Expand Down
1 change: 1 addition & 0 deletions app/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 6 additions & 0 deletions app/server/lib/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down Expand Up @@ -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,
};
}
43 changes: 43 additions & 0 deletions app/server/lib/mail-feedback.test.ts
Original file line number Diff line number Diff line change
@@ -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 <b>never</b> 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 <b>never</b> 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("<b>never</b>");
expect(message.html).toContain("&lt;b&gt;never&lt;/b&gt;");
expect(message.html).toContain("&amp; 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);
});
});
66 changes: 66 additions & 0 deletions app/server/lib/mail.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { Feedback } from "./types";

/**
* Sending an invitation by email.
*
Expand Down Expand Up @@ -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<Feedback["kind"], string> = {
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]) =>
`<tr><td style="padding:3px 14px 3px 0;color:${MUTED};white-space:nowrap;vertical-align:top;">${escape(key)}</td><td style="padding:3px 0;color:${QUIET};">${escape(value)}</td></tr>`,
)
.join("");
const html = `<!doctype html>
<html lang="en">
<head><meta charset="utf-8"><meta name="viewport" content="width=device-width,initial-scale=1"><meta name="color-scheme" content="light"><title>${escape(subject)}</title></head>
<body style="margin:0;padding:24px 16px;background:${PAPER};color:${INK};font-family:${FONT};">
<p style="margin:0 0 12px;font-size:13px;color:${MUTED};">${escape(kind)} sent from the shell.online app</p>
<pre style="margin:0 0 18px;padding:16px 18px;background:#ffffff;border:1px solid ${LINE};border-radius:10px;font-family:${FONT};font-size:15px;line-height:1.6;white-space:pre-wrap;word-wrap:break-word;color:${INK};">${escape(feedback.body)}</pre>
<table role="presentation" cellpadding="0" cellspacing="0" border="0" style="font-size:13px;line-height:1.5;">${rows}</table>
</body>
</html>`;

return { to, subject, html, text };
}
27 changes: 27 additions & 0 deletions app/server/lib/migrations/011_feedback.sql
Original file line number Diff line number Diff line change
@@ -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);
Loading
Loading