chore: api status-page locale

[mxkaske]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
openstatus-dashboard	Ready	Preview, Comment	Mar 24, 2026 10:35am
openstatus-status-page	Ready	Preview, Comment	Mar 24, 2026 10:35am

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
openstatus-web	Skipped		Mar 24, 2026 10:35am

[cubic-dev-ai]

3 issues found across 7 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/proto/api/openstatus/status_page/v1/service.proto">

<violation number="1" location="packages/proto/api/openstatus/status_page/v1/service.proto:199">
P2: `repeated Locale locales` in an update message cannot distinguish "don't change" from "clear all locales" — both are an empty list on the wire in proto3. Consider wrapping it in a message (e.g., `message LocaleList { repeated Locale locales = 1; }`) and using `optional LocaleList locales` so that presence is trackable, or adopt a `FieldMask` pattern.</violation>
</file>

<file name="apps/server/src/routes/rpc/services/status-page/index.ts">

<violation number="1" location="apps/server/src/routes/rpc/services/status-page/index.ts:287">
P2: Once `locales` are set on a status page, they can never be cleared via `updateStatusPage`. An empty `req.locales` array is silently ignored instead of resetting the field to `null`, unlike the create path and the pattern used for other nullable fields like `homepageUrl`.</violation>
</file>

<file name="apps/server/src/routes/rpc/services/status-page/__tests__/status-page.test.ts">

<violation number="1" location="apps/server/src/routes/rpc/services/status-page/__tests__/status-page.test.ts:588">
P2: These two tests share the slug `${TEST_PREFIX}-locale-create-slug`. If the first test fails after page creation but before its inline cleanup, the second test will also fail with a 409 slug conflict. Use a distinct slug for the second test (e.g., `${TEST_PREFIX}-locale-default-slug`) to isolate them.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[mxkaske]

@claude review

[claude]

Claude finished @mxkaske's task in 1m 24s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze and post review

---

This PR adds defaultLocale and locales fields to the status page API (proto, TypeScript server, and tests). The implementation is clean and consistent with existing patterns. The three issues flagged by cubic-dev-ai are all valid — details and additional findings below.

---

Issues

1. repeated Locale locales in UpdateStatusPageRequest — ambiguous semantics (P2)

packages/proto/api/openstatus/status_page/v1/service.proto:199

In proto3, repeated fields have no field presence: an empty list on the wire is indistinguishable from a field that was never set. This means a caller cannot express "clear all locales" — it looks the same as "don't touch locales".

The simplest fix consistent with how optional scalar fields work elsewhere in this file is to wrap locales in an optional wrapper message:

message LocaleList {
  repeated Locale locales = 1;
}
// In UpdateStatusPageRequest:
optional LocaleList locales = 8;

When locales is absent → don't update; when present but empty → clear to null; when present with values → set to those values. Fix this →

---

2. Locales can never be cleared via updateStatusPage (P2)

apps/server/src/routes/rpc/services/status-page/index.ts:287

if (req.locales.length > 0) {
  updateValues.locales = req.locales.map(protoLocaleToDb);
}

An empty req.locales is silently ignored rather than setting locales = null. This is inconsistent with:

• The create path, which explicitly sets locales: req.locales.length > 0 ? req.locales.map(protoLocaleToDb) : null
• Other nullable fields like homepageUrl which correctly distinguish presence from empty value

Until the proto issue (#1) is resolved this can't be fixed cleanly at the wire level, but once locales becomes an optional wrapper the server can check req.locales !== undefined to distinguish the three states. Fix this →

---

3. Two tests share the same slug — isolation risk (P2)

apps/server/src/routes/rpc/services/status-page/__tests__/status-page.test.ts:588,605

// test 1 (line 588)
slug: `${TEST_PREFIX}-locale-create-slug`,
// test 2 (line 605) — same slug!
slug: `${TEST_PREFIX}-locale-create-slug`,

If test 1 fails after the page is created but before db.delete(...) runs, test 2 will get a 409 slug conflict and fail for the wrong reason. Use a distinct slug for the second test, e.g. ${TEST_PREFIX}-locale-default-slug. Fix this →

---

Additional observations

Missing validation: defaultLocale must be in locales

Neither create nor update validates that defaultLocale is a member of locales when both are provided. A caller can create a page with defaultLocale: LOCALE_FR, locales: [LOCALE_EN, LOCALE_DE], which is semantically inconsistent. Consider adding a buf.validate constraint or a runtime check.

LOCALE_UNSPECIFIED silently maps to "en"

protoLocaleToDb maps LOCALE_UNSPECIFIED (the proto3 zero value, which is also the default when no locale is set) to "en". This is fine operationally, but if LOCALE_UNSPECIFIED is passed inside a locales array it will silently become a duplicate "en" entry. Consider filtering it out or adding a validation constraint on the field.

---

What looks good

• The converter functions (dbLocaleToProto / protoLocaleToDb) are clean and symmetric.
• createStatusPage correctly stores null instead of [] when no locales are provided.
• The GetStatusPage and GetStatusPageContent tests cover the locale round-trip well.
• Proto field numbers are properly sequenced.

[Copilot]

Pull request overview

This PR adds locale support to the Status Page API surface (protobuf + ConnectRPC implementation), allowing a status page to expose a default locale and a set of enabled locales.

Changes:

• Extend StatusPage, CreateStatusPageRequest, and UpdateStatusPageRequest protobuf definitions with default_locale and locales, plus a new Locale enum.
• Implement DB↔proto locale conversions and persist locale fields in the StatusPage RPC service.
• Add RPC-level tests covering create/update/get behaviors for the new locale fields.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/proto/api/openstatus/status_page/v1/status_page.proto	Adds Locale enum and locale fields to StatusPage.
packages/proto/api/openstatus/status_page/v1/service.proto	Adds locale fields to create/update requests.
packages/proto/gen/ts/openstatus/status_page/v1/status_page_pb.ts	Regenerated TS bindings reflecting new locale fields/enum.
packages/proto/gen/ts/openstatus/status_page/v1/service_pb.ts	Regenerated TS bindings reflecting updated request shapes.
apps/server/src/routes/rpc/services/status-page/index.ts	Persists locale settings on create/update.
apps/server/src/routes/rpc/services/status-page/converters.ts	Adds proto/DB locale conversion + includes locale fields in StatusPage responses.
apps/server/src/routes/rpc/services/status-page/tests/status-page.test.ts	Adds test coverage for locale fields across RPC methods.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

UpdateStatusPage only updates locales when req.locales.length > 0, which makes it impossible for clients to clear/reset locales (e.g., set it to empty/null) via the API. If clearing is a supported operation (DB uses null for “no locales configured”), consider adding an explicit way to clear it (e.g., treat an empty list as “clear”, or add a separate boolean/field-mask-style signal).

Suggested change

	if (req.locales.length > 0) {
	updateValues.locales = req.locales.map(protoLocaleToDb);
	if (req.locales !== undefined) {
	updateValues.locales =
	req.locales.length > 0 ? req.locales.map(protoLocaleToDb) : null;

[Copilot]

In createStatusPage, req.defaultLocale ? ... : "en" will treat Locale.UNSPECIFIED (enum value 0) as falsy and silently coerce it to "en". Use an explicit undefined check (or handle UNSPECIFIED) so an explicitly provided enum value doesn’t change behavior due to JS truthiness.

Suggested change

	defaultLocale: req.defaultLocale
	? protoLocaleToDb(req.defaultLocale)
	: "en",
	defaultLocale:
	req.defaultLocale !== undefined
	? protoLocaleToDb(req.defaultLocale)
	: "en",

[Copilot]

Locale fields are written without validating the invariant that defaultLocale must be included in locales when locales is provided (this is enforced in other code paths via packages/db/src/schema/pages/validation.ts). As-is, UpdateStatusPage can persist inconsistent combinations when updating either field independently; add server-side validation/normalization before saving.

[Copilot]

CreateStatusPage also writes defaultLocale/locales without enforcing that the default is contained in the enabled locales list when locales is provided. Consider validating or auto-including the default locale to avoid persisting an inconsistent configuration.

[thibaultleouay]

@claude carefully review this

[claude]

Claude finished @thibaultleouay's task in 1m 50s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze and post review

---

This PR adds defaultLocale and locales fields to the status page API. The previous review issues (duplicate slug, locales-clearing, and proto3 field presence) have all been addressed in b507e7b. The implementation is clean overall. A few remaining items worth discussing:

---

Issues

1. Asymmetric API design: create uses repeated Locale, update uses optional LocaleList (P2)

CreateStatusPageRequest still uses a bare repeated Locale locales = 7, while UpdateStatusPageRequest correctly uses optional LocaleList locales = 8. This means callers need different calling conventions for the two operations:

// Create: flat array
{ locales: ["LOCALE_EN", "LOCALE_DE"] }

// Update: wrapped object
{ locales: { locales: ["LOCALE_EN", "LOCALE_DE"] } }

For create, there's no semantic difference between "empty" and "not provided" (both result in locales: null in the DB), so the current design works. But the inconsistency increases API surface complexity. Consider using LocaleList for create too, or at minimum document this difference. Fix this →

---

2. No test coverage for the validation error path (P2)

apps/server/src/routes/rpc/services/status-page/index.ts:171 and :314

Both createStatusPage and updateStatusPage now validate that defaultLocale ∈ locales and throw Code.InvalidArgument if not. But no test exercises this failure path — the test suite only covers happy paths. If someone removes the validation, tests will still pass.

// Missing test: create with defaultLocale not in locales should return 400
{
  title: "...", slug: "...",
  defaultLocale: "LOCALE_FR",
  locales: ["LOCALE_EN", "LOCALE_DE"], // FR not included
}
// Expected: 400 with "Default locale must be included in the locales list"

---

3. Duplicate locales not filtered (P3)

apps/server/src/routes/rpc/services/status-page/index.ts:166

const validLocales = req.locales.filter((l) => l !== 0);
const locales = validLocales.length > 0 ? validLocales.map(protoLocaleToDb) : null;

A caller sending locales: ["LOCALE_EN", "LOCALE_EN", "LOCALE_DE"] will store ["en", "en", "de"]. This may or may not be valid depending on the DB schema, but it's likely unintentional. Consider adding .filter((v, i, a) => a.indexOf(v) === i) after mapping, or a buf.validate unique constraint. Fix this →

---

What looks good

• The optional LocaleList wrapper for updateStatusPage.locales is the correct proto3 pattern for distinguishing "not provided", "clear", and "set" — well done.
• LOCALE_UNSPECIFIED is properly filtered out (l !== 0) in both create and update paths.
• The update validation correctly handles partial updates: if only defaultLocale changes, it checks against existing pageData.locales; if only locales changes, it checks against pageData.defaultLocale. The logic at index.ts:307-319 is sound.
• Test coverage is comprehensive for the happy paths (set, clear, omit, round-trip via get/get-content).
• dbPageToProto correctly maps null locales to [] in the response.

---

[cubic-dev-ai]

3 issues found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/server/src/routes/rpc/services/status-page/__tests__/status-page.test.ts">

<violation number="1" location="apps/server/src/routes/rpc/services/status-page/__tests__/status-page.test.ts:586">
P2: Test name claims to cover "or empty" but only tests the omitted case. The removed "clears locale settings when empty LocaleList is sent" test has no replacement — consider adding a case that sends `locales: []` to verify whether empty arrays are treated as no-op or as a clear operation.</violation>
</file>

<file name="apps/server/src/routes/rpc/services/status-page/index.ts">

<violation number="1" location="apps/server/src/routes/rpc/services/status-page/index.ts:303">
P2: The condition `req.locales.length > 0` makes it impossible to clear locales. Previously, an empty wrapper message would enter the block and set `locales` to `null`. Now, an empty array skips the block entirely. Since protobuf repeated fields default to `[]`, there's no way for a client to distinguish "not provided" from "clear locales." If clearing locales is a supported operation, this needs a different signaling mechanism (e.g., a separate `clearLocales` boolean field, or reverting to a wrapper message).</violation>
</file>

<file name="packages/proto/api/openstatus/status_page/v1/service.proto">

<violation number="1" location="packages/proto/api/openstatus/status_page/v1/service.proto:199">
P1: Switching from `optional LocaleList` to `repeated Locale` in an update message loses the ability to distinguish "field not sent" (don't change) from "field sent empty" (clear locales). In proto3, a `repeated` field has no presence — both cases deserialize to an empty list. This means an update request that omits `locales` entirely will be interpreted the same as one that explicitly clears them.

Consider keeping a wrapper message (e.g., `optional LocaleList`) or using a field mask to preserve patch semantics, consistent with the `optional` pattern used by every other field in this message.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P1: Switching from optional LocaleList to repeated Locale in an update message loses the ability to distinguish "field not sent" (don't change) from "field sent empty" (clear locales). In proto3, a repeated field has no presence — both cases deserialize to an empty list. This means an update request that omits locales entirely will be interpreted the same as one that explicitly clears them.

Consider keeping a wrapper message (e.g., optional LocaleList) or using a field mask to preserve patch semantics, consistent with the optional pattern used by every other field in this message.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/proto/api/openstatus/status_page/v1/service.proto, line 199:

<comment>Switching from `optional LocaleList` to `repeated Locale` in an update message loses the ability to distinguish "field not sent" (don't change) from "field sent empty" (clear locales). In proto3, a `repeated` field has no presence — both cases deserialize to an empty list. This means an update request that omits `locales` entirely will be interpreted the same as one that explicitly clears them.

Consider keeping a wrapper message (e.g., `optional LocaleList`) or using a field mask to preserve patch semantics, consistent with the `optional` pattern used by every other field in this message.</comment>

<file context>
@@ -196,8 +196,7 @@ message UpdateStatusPageRequest {
   // New enabled locales for the status page (optional).
-  // When absent: don't change. When present but empty: clear locales. When present with values: set locales.
-  optional LocaleList locales = 8;
+  repeated Locale locales = 8;
 }
 
</file context>

[thibaultleouay]

@claude review this again

[claude]

Claude finished @thibaultleouay's task in 3m 6s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze and post review

---

This review covers the current state after all 4 commits. Several issues from the previous reviews have been resolved — the proto design regression introduced in recent commits is the main remaining concern.

---

Issues

1. repeated Locale locales in UpdateStatusPageRequest — proto3 field presence lost (P1)

packages/proto/api/openstatus/status_page/v1/service.proto:199

// Current (broken):
repeated Locale locales = 8;

// Was correct in b507e7b (then reverted):
optional LocaleList locales = 8;

In proto3, repeated fields have no field presence — an omitted field and an explicitly empty list both deserialize to []. This means the server at index.ts:303 has no way to distinguish:

• "client didn't send locales" → don't change
• "client sent locales: []" → clear all locales

The optional LocaleList locales pattern (using a wrapper message) was the correct fix — it was present in b507e7b but was reverted in a subsequent commit. Fix this →

---

2. Clearing locales is impossible via updateStatusPage (P2)

apps/server/src/routes/rpc/services/status-page/index.ts:303

if (req.locales.length > 0) {
  // ...
}

Because of issue #1, req.locales is always [] when no locales are sent. The length > 0 guard means an empty array (which can't be distinguished from "not sent") silently skips the update — making it impossible to ever clear locales once set. This is resolved by reverting to optional LocaleList. Fix this →

---

3. Test name is misleading (P3)

apps/server/src/routes/rpc/services/status-page/__tests__/status-page.test.ts:586

test("does not change locales when field is omitted or empty", async () => {

The test only covers the omitted case (no locales key in the request). The "or empty" part (locales: []) is not tested. With the current proto design, both cases are identical — but the name implies intentional coverage of both. Either:

• Rename to "does not change locales when field is omitted" to accurately reflect what's tested, or
• Add a second request with locales: [] once the proto design allows distinguishing the two cases

---

What looks good (improvements from previous reviews)

• Validation error path is now tested: create returns 400 when defaultLocale is not in locales (line 720) and update returns 400 when defaultLocale is not in locales (line 740) — both missing in the prior review cycle.
• Duplicate slug isolation is fixed: The second create test now uses ${TEST_PREFIX}-locale-default-slug, not the same slug.
• Deduplication is implemented: [...new Set(validLocales.map(protoLocaleToDb))] prevents duplicate locale entries.
• LOCALE_UNSPECIFIED filtering: Both create and update paths filter l !== 0.
• Deduplicate test added: create deduplicates locales at line 759 — good coverage.
• optional is correctly used for default_locale: optional Locale default_locale = 7 preserves presence for scalar fields.
• Partial update validation is sound: The logic at index.ts:311-323 correctly handles the case where only defaultLocale or only locales changes by reading the other value from existing DB state.

---

The core fix needed is to restore optional LocaleList locales = 8 in UpdateStatusPageRequest (and regenerate the TS bindings), then update index.ts to use req.locales !== undefined instead of req.locales.length > 0.