Fix frontend, backend, and deployment bugs (deep review)

CLAUDE.md

@@ -388,17 +388,23 @@ When a search doesn't turn anything up: the wiring is usually in
 The full deployment runbook is [docs/DEPLOYMENT.md](docs/DEPLOYMENT.md);
 the Railway readiness audit is [docs/RAILWAY_READINESS.md](docs/RAILWAY_READINESS.md).
 
-Production = **Railway**. Two services:
+Production = **Railway**. Three deployed services (plus an optional
+fourth):
 
-- `web` — Vite `build` → `vite preview` on `$PORT`, behind a Railway
-  domain. Static SPA + nothing else.
+- `web` — Vite `build` → static `dist/` served by the `serve` package
+  on `$PORT` (`apps/web/Dockerfile`), behind a Railway domain.
 - `server` — `apps/server/Dockerfile` (Node 20 slim), Fastify on
   `$PORT`. Mounted Postgres add-on for the DB. S3-compatible object
   storage add-on for recordings/uploads.
-
-`railway.json` at the repo root encodes the build + start commands per
-service. The `apps/server/Dockerfile` is the only Dockerfile in the
-tree — `apps/web` ships as a Vite preview server, not a container.
+- `sample-app` — same static-`serve` shape as `web`
+  (`apps/sample-app/Dockerfile`).
+- `shotter` — optional headless-Chromium screenshot service
+  (`apps/shotter/Dockerfile`); not deployed today.
+
+Railway config-as-code is service-scoped (one file per service), so
+each app carries its own `apps/<app>/railway.json`; point each Railway
+service's "Config file path" setting at its file. See
+docs/DEPLOYMENT.md §9.
 
 The MCP server (`apps/mcp`) **does not deploy**. It runs on the user's
 laptop (spawned by Claude Code, or via `npm run dev:mcp`). It opens a

apps/sample-app/railway.json

@@ -0,0 +1,11 @@
+{
+  "$schema": "https://railway.app/railway.schema.json",
+  "build": {
+    "builder": "DOCKERFILE",
+    "dockerfilePath": "apps/sample-app/Dockerfile"
+  },
+  "deploy": {
+    "restartPolicyType": "ON_FAILURE",
+    "restartPolicyMaxRetries": 10
+  }
+}

apps/sample-app/serve.json

@@ -1,22 +1,31 @@
 {
   "headers": [
     {
-      "source": "index.html",
+      "source": "**/*.@(js|css|svg|png|jpg|jpeg|gif|webp|ico|woff|woff2|ttf|eot|map)",
       "headers": [
         {
           "key": "Cache-Control",
-          "value": "no-cache, no-store, must-revalidate"
+          "value": "public, max-age=3600"
         }
       ]
     },
     {
-      "source": "**/*.@(js|css|svg|png|jpg|jpeg|gif|webp|ico|woff|woff2|ttf|eot|map)",
+      "source": "assets/**",
       "headers": [
         {
           "key": "Cache-Control",
           "value": "public, max-age=31536000, immutable"
         }
       ]
+    },
+    {
+      "source": "index.html",
+      "headers": [
+        {
+          "key": "Cache-Control",
+          "value": "no-cache, no-store, must-revalidate"
+        }
+      ]
     }
   ],
   "cleanUrls": false,

apps/server/railway.json

@@ -0,0 +1,13 @@
+{
+  "$schema": "https://railway.app/railway.schema.json",
+  "build": {
+    "builder": "DOCKERFILE",
+    "dockerfilePath": "apps/server/Dockerfile"
+  },
+  "deploy": {
+    "healthcheckPath": "/health",
+    "healthcheckTimeout": 30,
+    "restartPolicyType": "ON_FAILURE",
+    "restartPolicyMaxRetries": 10
+  }
+}

apps/server/src/index.ts

@@ -105,10 +105,28 @@ async function main(): Promise<void> {
   // Extra origins from env (comma-separated). Lets production deploys add
   // their canvas/web hostnames without a code change. Localhost is always
   // allowed so `npm run dev` keeps working.
+  // A browser Origin header is always scheme://host[:port] — no path, no
+  // trailing slash. Normalise configured http(s) entries so an operator
+  // writing `https://foldo.dev/` (easy mistake) doesn't silently CORS-block
+  // the entire canvas. Non-http(s) schemes (chrome-extension://,
+  // moz-extension://, capacitor://, …) are passed through verbatim:
+  // `new URL(...).origin` is the literal string 'null' for them, which would
+  // both break the entry AND allowlist the dangerous `Origin: null` that
+  // sandboxed iframes / file:// pages send.
   const extraOrigins = (process.env.FOLDO_WEB_ORIGIN ?? '')
     .split(',')
     .map((s) => s.trim())
-    .filter(Boolean);
+    .filter((s) => Boolean(s) && s !== 'null')
+    .map((s) => {
+      if (/^https?:\/\//i.test(s)) {
+        try {
+          return new URL(s).origin;
+        } catch {
+          /* fall through to the generic trim */
+        }
+      }
+      return s.replace(/\/+$/, '');
+    });
 
   // Locking down the chrome-extension allowlist: before, ANY chrome-extension
   // origin could call the API (lets a malicious extension on a tester's box

apps/server/src/repo/branches.ts

@@ -113,6 +113,14 @@ function rowToCommit(r: CommitRow): Commit {
 }
 
 export async function upsertCommit(c: Commit): Promise<Commit> {
+  // sha is the global PK; the same commit can arrive on multiple branch refs
+  // (merges, pushing an existing commit to a new branch). Updating
+  // message/author/parent on conflict is safe — in Git those are properties
+  // of the sha itself, so a re-delivery carries identical values — and it
+  // matters: POST /api/boards/:id/branches seeds a STUB row (`branch: <name>`
+  // message, the clicking user as author) that the real webhook delivery
+  // must be able to heal. branch_id is deliberately NOT updated: the first
+  // branch attribution wins.
   await exec(
     `INSERT INTO commits (sha, branch_id, message, author_user_id, parent_sha, created_at)
      VALUES ($1, $2, $3, $4, $5, $6)

apps/server/src/routes/comments.ts

@@ -39,7 +39,10 @@ export async function registerCommentRoutes(app: FastifyInstance): Promise<void>
     async (req, reply) => {
       const user = requireUser(req);
       const body = req.body;
-      if (!body?.boardId || !body?.frameId || !body?.text) {
+      // text may legitimately be '' — the pin-drop flow POSTs an empty body
+      // so the pin appears instantly and the user types into the popover
+      // afterwards. Reject only a missing/non-string text.
+      if (!body?.boardId || !body?.frameId || typeof body.text !== 'string') {
         return reply.code(400).send({ error: 'Invalid comment body', code: 'BAD_REQUEST' });
       }
       // Comments require membership; even viewers can leave them in this MVP.

apps/server/src/routes/recordings.ts

@@ -48,7 +48,11 @@ export async function registerRecordingRoutes(
     '/api/recordings/*',
     async (req: FastifyRequest<{ Params: { '*': string } }>, reply: FastifyReply) => {
       const key = decodeURIComponent(req.params['*'] ?? '');
-      if (!key || key.includes('..')) {
+      // All recording keys live under the `recordings/` namespace. Enforcing
+      // the prefix matters on S3 deploys: `signedUrl()` will happily presign
+      // ANY bucket key, so without this check the route doubles as an open
+      // "presign anything" oracle (e.g. /api/recordings/uploads/<id>.png).
+      if (!key.startsWith('recordings/') || key.includes('..')) {
         return reply
           .code(400)
           .send({ error: 'Bad recording key', code: 'BAD_REQUEST' });

apps/server/src/routes/tests.ts

@@ -346,6 +346,21 @@ export async function registerTestRoutes(app: FastifyInstance): Promise<void> {
           url && (mode === 'auto' || mode === 'iframe')
             ? await probeFrameable(url)
             : null;
+      } else if (
+        patch.targetMode !== undefined &&
+        patch.targetMode !== test.targetMode
+      ) {
+        // Mode actually flipped without a new URL (e.g. dom_snapshot →
+        // iframe). The cached probe result may be stale or never computed,
+        // so re-probe against the existing target — otherwise
+        // resolveDeliveryMode keeps serving the wrong delivery mode off a
+        // stale `frameable`. (No-op PATCHes that re-send the current mode
+        // skip the probe; it's a blocking external GET.)
+        const mode = patch.targetMode;
+        patch.frameable =
+          test.targetUrl && (mode === 'auto' || mode === 'iframe')
+            ? await probeFrameable(test.targetUrl)
+            : null;
       }
       if (body.recordingModes !== undefined) {
         const modes = sanitizeRecordingModes(body.recordingModes);

apps/server/src/routes/webhooks.ts

@@ -1,5 +1,5 @@
 import { createHmac, timingSafeEqual } from 'node:crypto';
-import type { FastifyInstance, FastifyRequest, RawServerDefault } from 'fastify';
+import type { FastifyInstance, FastifyRequest } from 'fastify';
 import type { Frame, GithubPushPayload, MarkdownFrameContent } from '@foldo/protocol';
 import { getBoardByRepoSlug } from '../repo/boards.ts';
 import {
@@ -36,24 +36,36 @@ function verifyGithubSignature(
 }
 
 export async function registerWebhookRoutes(app: FastifyInstance): Promise<void> {
-  app.post<{ Body: GithubPushPayload }>(
-    '/api/webhooks/github',
-    {
-      config: { rawBody: true },
-    },
-    async (req, reply) => {
-      const secret = process.env.FOLDO_GITHUB_WEBHOOK_SECRET;
-      // We need raw body for HMAC; Fastify parses JSON before our handler runs,
-      // so reconstitute by re-stringifying. This isn't byte-identical with
-      // exotic encodings but matches GitHub's payload for all practical cases.
-      // For strict verification, register a raw-body content-type-parser.
-      const raw =
-        (req as unknown as { rawBody?: string }).rawBody ?? JSON.stringify(req.body ?? {});
-      if (!verifyGithubSignature(req, raw, secret)) {
-        return reply
-          .code(401)
-          .send({ error: 'Invalid webhook signature', code: 'UNAUTHORIZED' });
-      }
+  // The webhook lives in its own encapsulated scope so we can override the
+  // JSON content-type parser for this route only: GitHub's HMAC is computed
+  // over the exact bytes it sent, so we must keep the verbatim body around —
+  // re-stringifying the parsed object is not byte-identical (key order,
+  // unicode escaping) and makes signature verification fail for real
+  // payloads whenever a secret is configured.
+  await app.register(async (scope) => {
+    // Delegate the actual parse to Fastify's default JSON parser so this
+    // route keeps the secure-json-parse prototype-poisoning protection and
+    // the standard malformed-JSON error shape every other route gets.
+    const defaultJsonParser = scope.getDefaultJsonParser('error', 'error');
+    scope.addContentTypeParser(
+      'application/json',
+      { parseAs: 'string' },
+      (req, body, done) => {
+        (req as unknown as { rawBody?: string }).rawBody = body as string;
+        defaultJsonParser(req, body as string, done);
+      },
+    );
+
+    scope.post<{ Body: GithubPushPayload }>(
+      '/api/webhooks/github',
+      async (req, reply) => {
+        const secret = process.env.FOLDO_GITHUB_WEBHOOK_SECRET;
+        const raw = (req as unknown as { rawBody?: string }).rawBody;
+        if (!verifyGithubSignature(req, raw ?? '', secret)) {
+          return reply
+            .code(401)
+            .send({ error: 'Invalid webhook signature', code: 'UNAUTHORIZED' });
+        }
 
       const body = req.body;
       if (!body?.ref || !body?.after || !body?.repository?.full_name) {
@@ -142,11 +154,7 @@ export async function registerWebhookRoutes(app: FastifyInstance): Promise<void>
       hub.broadcast(board.id, { type: 'frame.added', frame });
 
       return reply.send({ ok: true });
-    },
-  );
-
-  // Suppress unused-type warning for the helper signature on older Fastify type
-  // exports, we don't actually need RawServerDefault but the import keeps the
-  // type imports honest. Side-effect free.
-  void (null as unknown as RawServerDefault);
+      },
+    );
+  });
 }

apps/server/src/ws/browser.ts

@@ -156,36 +156,57 @@ export async function registerBrowserWs(app: FastifyInstance): Promise<void> {
           .connectionsOnBoard(board.id)
           .map((c) => c.presence);
 
-        void Promise.resolve(hub.latestSeq(board.id)).then((latestSeq) => {
-          sendSafe(socket, {
-            type: 'welcome',
-            boardId: board.id,
-            youUserId: user.id,
-            board,
-            users: others,
-            latestSeq,
+        // The protocol guarantees replayed messages arrive immediately AFTER
+        // the welcome, so the SENDS must be ordered — on RedisHub the two hub
+        // reads are independent round-trips that would otherwise race. The
+        // reads themselves are safe to issue in parallel; only the send
+        // order matters.
+        const sinceSeq =
+          typeof msg.sinceSeq === 'number' && msg.sinceSeq > 0
+            ? msg.sinceSeq
+            : null;
+        void Promise.all([
+          Promise.resolve(hub.latestSeq(board.id)),
+          sinceSeq === null
+            ? Promise.resolve(undefined)
+            : Promise.resolve(hub.getMissedSince(board.id, sinceSeq)),
+        ])
+          .then(([latestSeq, missed]) => {
+            sendSafe(socket, {
+              type: 'welcome',
+              boardId: board.id,
+              youUserId: user.id,
+              board,
+              users: others,
+              latestSeq,
+            });
+            // Replay any broadcasts the client missed while it was
+            // disconnected. If sinceSeq is older than our oldest buffered
+            // message getMissedSince returns null and the client falls back
+            // to a fresh REST refetch.
+            if (sinceSeq === null) return;
+            if (missed === null) {
+              wsReplayGaps.inc({ boardId: board.id });
+              sendSafe(socket, {
+                type: 'error',
+                code: 'REPLAY_GAP',
+                message:
+                  'replay buffer no longer contains requested seq; please refetch',
+              });
+            } else if (missed) {
+              for (const m of missed) sendSafe(socket, m);
+            }
+          })
+          .catch((err) => {
+            wsLog.warn(
+              {
+                connId,
+                boardId: board.id,
+                err: err instanceof Error ? err.message : String(err),
+              },
+              'welcome/replay send failed',
+            );
           });
-        });
-
-        // Replay any broadcasts the client missed while it was disconnected.
-        // If sinceSeq is older than our oldest buffered message we return
-        // null and the client falls back to a fresh REST refetch.
-        if (typeof msg.sinceSeq === 'number' && msg.sinceSeq > 0) {
-          void Promise.resolve(hub.getMissedSince(board.id, msg.sinceSeq)).then(
-            (missed) => {
-              if (missed === null) {
-                wsReplayGaps.inc({ boardId: board.id });
-                sendSafe(socket, {
-                  type: 'error',
-                  code: 'REPLAY_GAP',
-                  message: 'replay buffer no longer contains requested seq; please refetch',
-                });
-              } else {
-                for (const m of missed) sendSafe(socket, m);
-              }
-            },
-          );
-        }
 
         // Tell others we joined
         hub.broadcast(board.id, { type: 'presence.join', user: presence }, user.id);

apps/shotter/Dockerfile

@@ -1,6 +1,13 @@
 # syntax=docker/dockerfile:1.6
 # Foldo screenshot service. Headless Chromium via playwright-core's prebuilt
 # image so we don't have to babysit the install dance.
+#
+# IMPORTANT: this base-image tag must match the exact `playwright-core`
+# version pinned in apps/shotter/package.json. The image pre-installs the
+# browser revisions for ITS Playwright version only — a newer playwright-core
+# from the lockfile looks for a Chromium revision that isn't there, and every
+# POST /shot fails at runtime (the container still builds and passes /health).
+# Bump both together.
 
 FROM mcr.microsoft.com/playwright:v1.58.0-jammy
 
@@ -21,8 +28,9 @@ COPY apps/shotter/tsconfig.json ./apps/shotter/
 
 EXPOSE 5180
 
+# node is guaranteed in this image; wget/curl are not.
 HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 \
-  CMD wget -q -O - "http://localhost:${PORT}/health" >/dev/null || exit 1
+  CMD node -e "fetch('http://localhost:'+(process.env.PORT||5180)+'/health').then(r=>{if(!r.ok)process.exit(1)}).catch(()=>process.exit(1))"
 
 WORKDIR /app/apps/shotter
 CMD ["npm", "start"]

apps/shotter/package.json

@@ -10,7 +10,7 @@
   },
   "dependencies": {
     "fastify": "^5.1.0",
-    "playwright-core": "^1.58.0",
+    "playwright-core": "1.58.0",
     "tsx": "^4.19.2"
   },
   "devDependencies": {

apps/shotter/railway.json

@@ -0,0 +1,12 @@
+{
+  "$schema": "https://railway.app/railway.schema.json",
+  "build": {
+    "builder": "DOCKERFILE",
+    "dockerfilePath": "apps/shotter/Dockerfile"
+  },
+  "deploy": {
+    "healthcheckPath": "/health",
+    "restartPolicyType": "ON_FAILURE",
+    "restartPolicyMaxRetries": 10
+  }
+}