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
21 changes: 16 additions & 5 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -358,16 +358,27 @@ reviews:
table under the checkbox is the explanation and does not change the declaration.

Treat a change as breaking if the diff does any of the following:
- Renames, removes, or retypes a field in an API request or response shape, or changes
an emitted value (for example `EN` becoming `en-US`).
- Removes or renames an endpoint or route, or changes a webhook payload.
- Renames, removes, or retypes a field in an API request or response shape, requires a
field that was optional, or changes an emitted value (for example `EN` becoming
`en-US`).
- Removes or renames an endpoint or route, changes a webhook payload, or changes the
HTTP status code returned for an existing request/response case.
- Changes a default value, or removes or renames an environment variable or config key —
including keys in `charts/formbricks/values.yaml` and `.env.example`. Adding one counts
only when it is required, or has no default that keeps existing installs working.
- Adds a database migration that needs manual action, drops or renames a column or
table, or adds a NOT NULL column with no default and no backfill.
- Changes an exported signature of the public SDK surface in `packages/js-core` or
`packages/surveys`.
- Changes an exported signature of the public SDK surface in `packages/js-core`,
`packages/survey-ui`, or `packages/surveys`.

Nothing else is breaking. In particular, a purely additive change — a new optional request
or response field, a new endpoint or route, an env var whose default keeps existing installs
working — is not, and neither is any change to code no external consumer reaches: internal
functions, modules, types, and workspace-package exports other than the SDK surface above,
however they change. "Internal API" means internal to this repo, and it is outside this
check. A change that matches one of the bullets above is breaking regardless of how
confident the author is; genuine uncertainty about which bullet applies is what the
inconclusive case below is for.

FAIL if the diff contains at least one of the above and the checkbox is unticked, or the
section is empty or still holds the unedited template comment. Name the specific file and
Expand Down
25 changes: 22 additions & 3 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,28 @@ to grasp without opening files. -->
## Breaking changes

<!-- REQUIRED. Tick the box below if — and only if — this PR breaks something for API/SDK consumers or
self-hosters. It is breaking if it changes an API/SDK shape or emitted value (e.g. `EN` → `en-US`),
removes or renames an endpoint, route, env var or config key, changes a default or webhook payload, or
needs manual migration action.
self-hosters. The test is whether someone outside this repo has to change something to keep working.

**Breaking:** renaming, removing or retyping a field in a public API request or response shape,
requiring one that was optional, or changing an emitted value (e.g. `EN` → `en-US`); removing or
renaming an endpoint, route, env var or config key; changing a default, a webhook payload, or the
HTTP status code returned for an existing case; adding an env var or config key that is required, or
whose default does not keep existing installs working; changing an exported signature of the SDK
shipped from `packages/js-core`, `packages/survey-ui`, or `packages/surveys`; or needing manual
action on upgrade, including a migration that drops or renames a column or table.

**Not breaking:** anything purely additive, and anything internal. Adding an optional field to a
request or response, a new endpoint or route, or an env var whose default keeps existing installs
working leaves every existing consumer working unchanged. So does any change to internal code —
functions, modules, types, workspace-package exports other than the SDK surface above — that no
external consumer reaches, additive or not: "internal API" means internal to this repo, and this
section is not about it.

Uncertain? That means the change isn't clearly one of the bullets above — leave it unticked and say
why below. If it does match a bullet, tick it regardless of how you feel about it: the tick is the
only thing that writes a migration entry, so ticking it "to be safe" when nothing above applies
invents an upgrade step for self-hosters that does not exist; a reviewer who disagrees can still ask
for it.

The checkbox alone drives the label: ticking it applies `breaking-change`, which feeds the release notes
and the self-hoster migration guide. Leave its wording alone — `pr-label-sync.yml` finds it by that text,
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/api-v3-contract-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ jobs:
--health-timeout=5s
--health-retries=5
valkey:
image: valkey/valkey@sha256:12ba4f45a7c3e1d0f076acd616cb230834e75a77e8516dde382720af32832d6d
image: valkey/valkey@sha256:e0eb7c480958d32bdc4357a74bdd70653ae15f2f9b4c93c4a5a9fad1dc471c84
ports:
- 6379:6379
steps:
Expand Down
16 changes: 15 additions & 1 deletion .github/workflows/docker-build-validation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ jobs:
--health-retries 5

redis:
image: valkey/valkey@sha256:12ba4f45a7c3e1d0f076acd616cb230834e75a77e8516dde382720af32832d6d
image: valkey/valkey@sha256:e0eb7c480958d32bdc4357a74bdd70653ae15f2f9b4c93c4a5a9fad1dc471c84
ports:
- 6379:6379

Expand All @@ -53,6 +53,20 @@ jobs:
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.11.1

- name: Verify Valkey Image Platforms
shell: bash
run: |
set -euo pipefail

valkey_image="$(docker compose -f docker/docker-compose.yml config --format json | jq -r '.services.redis.image')"
manifest="$(docker buildx imagetools inspect --raw "$valkey_image")"

jq -e '
.mediaType == "application/vnd.oci.image.index.v1+json" and
any(.manifests[]; .platform.os == "linux" and .platform.architecture == "amd64") and
any(.manifests[]; .platform.os == "linux" and .platform.architecture == "arm64")
' <<<"$manifest"

# 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
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ jobs:
--health-timeout=5s
--health-retries=5
valkey:
image: valkey/valkey@sha256:12ba4f45a7c3e1d0f076acd616cb230834e75a77e8516dde382720af32832d6d
image: valkey/valkey@sha256:e0eb7c480958d32bdc4357a74bdd70653ae15f2f9b4c93c4a5a9fad1dc471c84
ports:
- 6379:6379
steps:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/integration-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ jobs:
--health-timeout=5s
--health-retries=5
valkey:
image: valkey/valkey@sha256:12ba4f45a7c3e1d0f076acd616cb230834e75a77e8516dde382720af32832d6d
image: valkey/valkey@sha256:e0eb7c480958d32bdc4357a74bdd70653ae15f2f9b4c93c4a5a9fad1dc471c84
ports:
- 6379:6379
steps:
Expand Down
7 changes: 5 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,10 @@ Do not:

- Keep code DRY and small; remove dead code and unused imports.
- Follow React hooks rules, keep effects focused, and avoid unnecessary `useMemo`/`useCallback`.
- Prefer type inference, avoid `any`, and use shared types from `@formbricks/types`.
- Prefer type inference, avoid `any`, and use shared types from `@formbricks/types`. This is enforced:
`@typescript-eslint/no-explicit-any` is an error in `packages/*` and a warning in `apps/web`, where the
typescript-eslint baseline is being ratcheted to error rule by rule (ENG-2264). Never add new `any`s —
a warning today becomes an error once its rule's backlog is cleared.
- Keep components focused, avoid deep nesting, and ensure basic accessibility.

## Commit & Pull Request Guidelines
Expand All @@ -295,7 +298,7 @@ Commits follow a lightweight Conventional Commit format (`fix:`, `chore:`, `feat

Every PR must use `.github/pull_request_template.md` and follow its inline guidance — the template is the source of truth for PR structure. The ticket line at the top is the only place a magic word (`Fixes`, `Ref`, `Closes`) may sit next to a ticket id: Linear and GitHub scan the whole body, so the same pair written in prose — inside backticks too — links and closes that ticket as well. When you need to name the convention in prose, write it without a resolvable id. All QA for a change happens on its own PR before review: the creator shows that every behaviour the diff changes is covered, and lists what is not under `Open gaps`; the reviewer challenges that list and asks for the missing coverage. There is no separate release QA pass per PR — release review only looks for problems arising from the interplay of several changes. Fill every section from the actual diff on PR open, and re-update it in the same turn on every change (new commits, scope or review fixes) so it never drifts — treat a stale section as a bug.

The checkbox under `## Breaking changes` is a decision you own, not a formality: judge the diff against the template's list of breaking changes and tick it (`- [x]`) when one applies, leave it unticked when none does. It is the only input to the `breaking-change` label, which feeds the release notes and the self-hoster migration guide, so a wrong answer either invents a migration entry or hides one. Re-check it whenever the diff grows. `pr-label-sync.yml` reads nothing but the tick, so the prose below the checkbox cannot change the label — but it is not free-form either: the CodeRabbit `Breaking changes match the diff` check compares the tick against the diff and expects a ticked box to document each breaking change, so explain your answer there in whatever shape fits (table or prose).
The checkbox under `## Breaking changes` is a decision you own, not a formality: judge the diff against the template's list of breaking changes and tick it (`- [x]`) when one applies, leave it unticked when none does. The template also lists what is **not** breaking — purely additive changes, and anything internal to this repo that no external consumer reaches — and an uncertain call is an unticked box with a line of reasoning, never a defensive tick. It is the only input to the `breaking-change` label, which feeds the release notes and the self-hoster migration guide, so a wrong answer either invents a migration entry or hides one. Re-check it whenever the diff grows. `pr-label-sync.yml` reads nothing but the tick, so the prose below the checkbox cannot change the label — but it is not free-form either: the CodeRabbit `Breaking changes match the diff` check compares the tick against the diff and expects a ticked box to document each breaking change, so explain your answer there in whatever shape fits (table or prose).

## Next.js Documentation

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ const mockUser = {
teamUsers: [{ team: { name: "Test Team", id: "team123", workspaceTeams: [{ workspaceId: "proj789" }] } }],
};

// getOrganizationOwnerCount (pulled in via the last-owner guard) request-caches its result with
// React's cache(); mocked to identity so repeated calls across tests re-hit the prisma mock below.
vi.mock("react", () => ({ cache: (fn: Function) => fn }));

vi.mock("@formbricks/database", () => ({
prisma: {
user: {
Expand All @@ -38,6 +42,9 @@ vi.mock("@formbricks/database", () => ({
create: vi.fn(),
delete: vi.fn(),
},
membership: {
count: vi.fn(),
},
$transaction: vi.fn(),
},
}));
Expand Down Expand Up @@ -248,6 +255,65 @@ describe("Users Lib", () => {
expect(result.error.type).toBe("internal_server_error");
}
});

test("blocks demoting the organization's last owner", async () => {
// The re-check runs inside the transaction, so the mock has to actually invoke the
// callback (against the same mocked client, standing in for `tx`) for the guard to run.
(prisma.$transaction as any).mockImplementationOnce((callback: any) => callback(prisma));
(prisma.user.findFirst as any).mockResolvedValueOnce({
...mockUser,
memberships: [{ organizationId: "org456", role: "owner" }],
});
(prisma.membership.count as any).mockResolvedValueOnce(1);

const result = await updateUser({ email: mockUser.email, role: "member" }, "org456");

expect(result.ok).toBe(false);
if (!result.ok) {
expect(result.error.type).toBe("conflict");
}
expect(prisma.user.update).not.toHaveBeenCalled();
});

test("allows demoting an owner when the organization has another owner", async () => {
(prisma.$transaction as any).mockImplementationOnce((callback: any) => callback(prisma));
(prisma.user.findFirst as any).mockResolvedValueOnce({
...mockUser,
memberships: [{ organizationId: "org456", role: "owner" }],
teamUsers: [],
});
(prisma.membership.count as any).mockResolvedValueOnce(2);
(prisma.team.findMany as any).mockResolvedValueOnce([]);
(prisma.user.update as any).mockResolvedValueOnce({
...mockUser,
memberships: [{ organizationId: "org456", role: "member" }],
});

const result = await updateUser({ email: mockUser.email, role: "member" }, "org456");

expect(result.ok).toBe(true);
if (result.ok) {
expect(result.data.role).toBe("member");
}
});

test("allows demoting an already-inactive owner without checking the owner count", async () => {
(prisma.user.findFirst as any).mockResolvedValueOnce({
...mockUser,
isActive: false,
memberships: [{ organizationId: "org456", role: "owner" }],
teamUsers: [],
});
(prisma.team.findMany as any).mockResolvedValueOnce([]);
(prisma.$transaction as any).mockResolvedValueOnce([
{ ...mockUser, isActive: false, memberships: [{ organizationId: "org456", role: "member" }] },
]);

const result = await updateUser({ email: mockUser.email, role: "member" }, "org456");

expect(result.ok).toBe(true);
expect(prisma.membership.count).not.toHaveBeenCalled();
});
});

describe("createUser with teams", () => {
Expand Down
Loading
Loading