[pull] main from formbricks:main

.coderabbit.yaml

@@ -86,15 +86,22 @@ reviews:
   path_instructions:
     - path: "**/*.test.tsx"
       instructions: |
-        AGENTS.md forbids React component unit tests in this repo — components are covered by
-        Playwright specs in `apps/web/playwright/`. If this file is newly added, say so and ask for
-        the coverage to move to a Playwright `.spec.ts` instead. Do not raise this on edits to the
-        handful of `.test.tsx` files that already exist.
+        AGENTS.md forbids React component unit tests in this repo. If this file is newly added, say
+        so and ask for the logic under test to move into a `.ts` module with a unit test. Do not ask
+        for a Playwright spec instead: deleting a `.test.tsx` creates no E2E obligation, and a
+        component is not an E2E subject. Do not raise this on edits to the handful of `.test.tsx`
+        files that already exist.
 
     - path: "**/*.tsx"
       instructions: |
-        - Never suggest adding unit tests for `.tsx` files. AGENTS.md forbids them; UI behaviour is
-          covered by Playwright specs in `apps/web/playwright/`. Suggesting an E2E spec is fine.
+        - Never suggest adding unit tests for `.tsx` files — AGENTS.md forbids them, and their
+          absence is not a coverage gap to fill somewhere else. Ask for a Playwright spec only when
+          the diff changes a feature's happy path or a journey across several surfaces, and then ask
+          for it in that feature's existing spec — or, if the diff opens a feature area the suite does
+          not cover yet, for one happy-path spec named for that area. Never ask for one to cover
+          component detail (a
+          label, a breadcrumb, an ARIA attribute, a keystroke inside one widget, a field's
+          validation message) — that belongs in a `.ts` unit test or in manual QA.
         - New client data flows go through an `/api/v3` route with TanStack Query, not a new Server
           Action. Server data lives in the query cache only — flag it being mirrored into
           `useState` or Jotai, and flag `router.refresh()` used as a data-refresh mechanism.
@@ -122,9 +129,27 @@ reviews:
 
     - path: "apps/web/playwright/**/*.spec.ts"
       instructions: |
-        - Flag timing hacks (`waitForTimeout`, arbitrary sleeps) and dependencies on data another
-          spec created — these are the repo's main source of flake.
-        - Specs must stay small and single-purpose; flag mega-specs. Slow suites need a `@slow` tag.
+        This suite is the critical path of every PR and every test is paid on every PR forever, so
+        cost is a review dimension here rather than a nit (AGENTS.md "Testing Guidelines"):
+        - A new spec file is a finding unless it opens a feature area the suite does not cover yet
+          (the spec filenames in `apps/web/playwright/` are the inventory — check before claiming an
+          area is uncovered); the default is assertions or a `test.step` added to that feature's
+          existing spec. A spec
+          that asserts component-level detail (a label, a breadcrumb, a sidebar's link list, an ARIA
+          attribute, a keystroke inside one widget, a field's validation message) is a finding
+          whatever file it lands in: ask for a `.ts` unit test or manual QA instead.
+        - Flag variant matrices. A second viewport, theme, locale, role or layout needs its own
+          stated reason; "the adjacent spec does it" is not one. A11y coverage for the rendered
+          survey extends `survey-accessibility.spec.ts`; elsewhere it belongs in that feature area's
+          own spec. Neither case is a per-ticket a11y spec.
+        - Flag state clicked into existence through the UI where Prisma or `/api/v3` could seed it
+          (`playwright/utils/accessibility.ts` is the pattern), and a per-test `users.create()` plus
+          login where a worker-scoped fixture would do.
+        - Flag timing hacks (`waitForTimeout`, arbitrary sleeps, `slowMo`) and dependencies on data
+          another spec created — these are the repo's main source of flake.
+        - Specs stay single-purpose per feature area: flag a spec that walks several unrelated
+          features, and a spec that exists for one detail of a covered one. `@slow` is metadata that
+          nothing reads, so a tag is not an answer to a cost finding.
 
     - path: "**/actions.ts"
       instructions: |

.github/pull_request_template.md

@@ -71,6 +71,12 @@ existed; `unit (mutation)` — only fails if you break the fix, because the code
 Any red-on-main or mutation row must carry the command or the mutated `file:line`, so a reviewer can
 rerun it instead of taking the claim on trust. -->
 
+<!-- Pick the cheapest level that can fail on the behaviour (AGENTS.md "Testing Guidelines"): `unit`
+for logic and invariants, a route test for authorization/response shape/scoping, `e2e` only for a
+feature's happy path or a journey across several surfaces, and `manual` — with the screenshot — for
+UI detail inside a feature that already has an e2e. A `manual` row is a complete answer; a new e2e
+spec is paid on every PR forever, so "this row had no automated test" is not a reason to add one. -->
+
 | Behaviour | How | Outcome |
 | --- | --- | --- |
 |  | unit (red on main) / unit (mutation) / unit (guard) / e2e / manual |  |

.github/workflows/docker-build-validation.yml

@@ -53,6 +53,15 @@ jobs:
       - name: Set up Docker Buildx
         uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1
 
+      # NOTE: deliberately no `cache-from`/`cache-to: type=gha`. Measured over four runs, exporting
+      # the cache cost 222-407s per run while the layers it could restore add up to 12s of work: the
+      # only steps that ever reported CACHED were the apk/corepack prelude, because `COPY . .` sits
+      # ahead of `pnpm install` in apps/web/Dockerfile and invalidates everything after it on any
+      # diff. Nothing populates a cache scope this job can read either — no workflow runs on pushes
+      # to main, and merge_group runs write to throwaway `gh-readonly-queue/...` scopes. Restoring
+      # the cache is worth revisiting only together with both halves of the fix: reorder the
+      # Dockerfile so the install layer survives a source change, and give the cache a producer PRs
+      # can read (a push-to-main build, or a registry cache, which is not branch-scoped).
       - name: Build Docker Image
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         env:
@@ -63,8 +72,6 @@ jobs:
           push: false
           load: true
           tags: formbricks-test:${{ env.GITHUB_SHA }}
-          cache-from: type=gha
-          cache-to: type=gha,mode=max
           secrets: |
             database_url=${{ secrets.DUMMY_DATABASE_URL }}
             encryption_key=${{ secrets.DUMMY_ENCRYPTION_KEY }}

AGENTS.md

@@ -21,8 +21,8 @@ Every `packages/*` workspace therefore exposes the standard `lint` / `typecheck`
 `test:coverage` scripts (plus `build` where there is a compile step). Deliberate exceptions:
 `config-*` packages hold only config files (no scripts beyond `clean`); `types` has no runtime logic
 to test; `email`, `types`, and `vite-plugins` are consumed from source, so they have no `build`;
-`apps/storybook` has no unit tests by policy (UI is covered by Playwright). Keep new packages on this
-matrix or document the exception here.
+`apps/storybook` has no unit tests by policy (its components are exercised by the feature journeys in
+`apps/web/playwright`). Keep new packages on this matrix or document the exception here.
 
 ### Shared dependency versions (pnpm catalog)
 
@@ -189,33 +189,74 @@ Always mark React component props as `Readonly<>` (e.g., `({ children }: Readonl
 Principles:
 
 - Confidence over coverage. Test behavior and outcomes; avoid brittle implementation-detail tests.
+- Prove a behavior at the cheapest level that can fail on it. An E2E test is not a stronger unit test; it
+  has a different subject — the journey, not the logic.
+- **An E2E test is paid on every PR, by everyone, forever.** The Playwright job is the critical path of the
+  PR gate (as of Aug 2026: a ~13 min job, of which ~6 min is the Playwright step itself — the rest is
+  install, build and boot — over ~110 tests and ~30 browser-minutes), and its wall clock can never drop
+  below its slowest single test. Weigh that before adding one — sometimes the right answer is no test
+  at this level.
+
+Which level, concretely:
+
+| The change                                                                      | The level                                     |
+| ------------------------------------------------------------------------------- | --------------------------------------------- |
+| A new feature area, or a journey across several surfaces                        | One happy-path E2E + unit tests for its logic |
+| Business logic, invariants, validation, derivation, permissions — anything pure | Unit test on the `.ts`                        |
+| A route's authorization, response shape, or query scoping                       | Unit or integration test on that route        |
+| A UI detail inside a feature that already has a happy-path spec                 | Neither — verify manually, say so in the PR   |
+
+A journey across several surfaces means something like survey list → editor → public survey → response,
+where the behavior only exists once browser, survey bundle, and server are wired together.
+
+The spec filenames in `apps/web/playwright/` are the inventory of covered areas — check there before
+concluding an area has no spec.
+
+This raises a floor as well as lowering a ceiling. Every feature area ships a happy-path E2E, and an area
+with none is a gap rather than a saving (Dashboards and Workflows are the current examples — ENG-2314). A
+bug fix inside a feature that already has one almost never needs a second spec — the level still follows
+the table above: journey behavior extends that spec, logic goes to a unit test, UI detail to manual QA.
 
 Do:
 
-- E2E tests (Playwright): cover critical user flows and regression risks. Extend existing specs or add
-  focused new ones in `apps/web/playwright`, keep tests small and well-named, use descriptive filenames
-  such as `billing.spec.ts`, tag slow suites with `@slow`, and run the suite before opening a PR.
+- E2E tests (Playwright): one spec per **feature area**, not per ticket and not per component. Default to
+  adding assertions or a `test.step` to that area's existing spec in `apps/web/playwright`; a new
+  `*.spec.ts` is for a feature area that has none, and it takes the area's name (`billing.spec.ts`).
+  Follow the suite's own patterns — seed state through Prisma or `/api/v3` instead of clicking it into
+  existence (`playwright/utils/accessibility.ts`), one journey per test with `test.step` phases
+  (`settings-tags.spec.ts`), assertions at feature level (`survey-overview.spec.ts`) — and run the suite
+  before opening a PR.
 - Unit tests: cover stable, high-value logic in `.ts` files, such as validators, transformers,
   evaluators, calculations, and edge cases. Keep assertions on inputs and outputs, colocate specs with
   the code they exercise (`utility.test.ts`), and mock network and storage boundaries through helpers
   from `@formbricks/*`.
 - Manual QA, especially for releases: verify on staging and file bugs. If a bug is critical, backport and
-  re-test.
+  re-test. For UI detail below the journey level, manual verification plus a screenshot in the PR is the
+  expected answer, not a new spec.
 - Run `pnpm test` before opening a PR and `pnpm test:coverage` when touching critical flows.
+- Merging, narrowing, or deleting an E2E spec is legitimate work — record it in the PR's Coverage table
+  like any other change.
 
 Do not:
 
-- Do not write component or UI unit tests for `.tsx` files; React components are covered by Playwright E2E
-  tests instead.
+- Do not write component or UI unit tests for `.tsx` files. **This is not an instruction to write an E2E
+  test instead**: the absence of a component unit test creates no coverage obligation. If a component holds
+  logic worth proving, lift that logic into a `.ts` module and unit-test it there; the rendering is
+  exercised incidentally by the feature journeys that already cross it.
+- Do not E2E a component. A language selector, a breadcrumb, a sidebar's link list, an ARIA attribute on
+  one widget, a keystroke inside one editor, a field's validation message, a search box filtering a list —
+  none of these justify a browser, a login, and a seeded tenant.
+- Do not build a variant matrix. Cover the one case that carries the risk; a second viewport, theme,
+  locale, role, or layout needs its own stated reason, and "the adjacent spec does it" is not one.
+  Accessibility work on the rendered survey extends the existing axe gate
+  (`survey-accessibility.spec.ts`); elsewhere it becomes an assertion in that feature area's own spec.
+  Either way, not a per-ticket a11y spec.
 - Do not add coverage-driven or low-signal tests.
-- Do not write tests that lock implementation details, markup, snapshots, or create churn.
-- Do not create mega or flaky E2E tests; avoid timing hacks and unstable dependencies.
-
-Heuristic:
-
-- User journey risk: E2E.
-- Pure logic or edge cases: unit test.
-- Release readiness: manual QA plus bug/backport loop.
+- Do not write tests that lock implementation details, markup, snapshots, or create churn — an assertion on
+  an exact list of nav labels is churn, not coverage.
+- Do not create mega or flaky E2E tests; avoid timing hacks (`waitForTimeout`, `slowMo`) and unstable
+  dependencies. `@slow` is triage metadata only: nothing in `playwright.config.ts` or CI reads it, so
+  tagging a spec does not make its cost go away.
 
 ## Documentation (apps/docs)
 

apps/web/integration/credential-backfill.integration.test.ts

@@ -1,9 +1,9 @@
-import { createLocalAccountIssuer } from "@better-auth/core/db";
 import { beforeEach, describe, expect, test } from "vitest";
 import { prisma } from "@formbricks/database";
 import { resetDb } from "@/integration/reset-db";
 import { hashSecret } from "@/lib/crypto";
 import { auth } from "@/modules/auth/lib/auth";
+import { canonicalAccountIssuer } from "@/modules/ee/sso/lib/constants";
 // The cutover data migration under test (auto-discovered by the migration runner at the flip).
 import { backfillCredentialAccounts } from "../../../packages/database/migration/20260619120000_eng_1054_credential_account_backfill/migration";
 
@@ -12,15 +12,29 @@ import { backfillCredentialAccounts } from "../../../packages/database/migration
  * (data and schema migrations run in strict timestamp order), always runs BEFORE the schema migration
  * that adds that column — so it genuinely cannot set it, and its rows are inserted with issuer=NULL.
  * In real deployments that's fine: ENG-2343's schema migration runs immediately after this one and
- * backfills every NULL-issuer credential row. A test calling this function standalone has to simulate
- * that follow-up step itself before asserting a real Better Auth sign-in succeeds.
+ * backfills every NULL-issuer row. A test calling this function standalone has to simulate that
+ * follow-up step itself before asserting a real Better Auth sign-in succeeds — using the same canonical
+ * mapping production uses, so the fixture cannot drift from it (ENG-2555).
  */
-const applyEng2343IssuerBackfill = (): Promise<{ count: number }> =>
-  prisma.account.updateMany({
-    where: { provider: "credential", issuer: null },
-    data: { issuer: createLocalAccountIssuer("credential") },
+const applyEng2343IssuerBackfill = async (): Promise<void> => {
+  const rows = await prisma.account.findMany({
+    where: { issuer: null },
+    select: { id: true, provider: true },
   });
 
+  // Per row, because the real backfill is a CASE over `provider` — a single `updateMany` could only
+  // reproduce one arm of it. An earlier version of this helper did exactly that (credential only), which
+  // left the google row below at issuer=NULL and quietly diverged from what production data looks like.
+  await Promise.all(
+    rows.map((row) =>
+      prisma.account.update({
+        where: { id: row.id },
+        data: { issuer: canonicalAccountIssuer(row.provider) },
+      })
+    )
+  );
+};
+
 /**
  * Integration coverage for the cutover credential-account backfill (ENG-1054) against real Postgres.
  * Proves the scariest cutover guarantee: an existing NextAuth-era user (bcrypt hash on User.password,