feat(services): migrate status-report domain (PR 1/N)#2101
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 25 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/api/src/router/statusReport.ts">
<violation number="1" location="packages/api/src/router/statusReport.ts:202">
P2: `statusReport.list` now silently truncates to 100 reports because it hard-codes `limit: 100` with no pagination path. Workspaces with >100 reports will not receive the full list.</violation>
</file>
<file name="apps/server/src/routes/rpc/adapter.ts">
<violation number="1" location="apps/server/src/routes/rpc/adapter.ts:42">
P2: Map `LIMIT_EXCEEDED` to `Code.ResourceExhausted` instead of `Code.PermissionDenied` to preserve correct RPC error semantics.</violation>
</file>
<file name="biome.jsonc">
<violation number="1" location="biome.jsonc:89">
P2: Use Biome's `includes` key here; `include` prevents this override from applying.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
Review response (Cubic findings)Addressed in 539c821 + also fixes the dashboard build regression that the Vercel preview turned up. 1. ✅
|
4 tasks
This was referenced Apr 23, 2026
First domain migration on top of the services scaffold (PR 0). Freezes
the `ServiceContext` shape and the single-object `{ ctx, input }`
argument convention across all three surfaces — tRPC / Connect / Slack.
## Services
- `packages/services/src/status-report/` — create, add-update, resolve
(convenience wrapper over add-update), update (metadata+associations),
update-update (edit a single update row), delete, delete-update, list,
get, notify.
- Shared internals: `validatePageComponentIds`,
`updatePageComponentAssociations`, `getReportInWorkspace`,
`getReportUpdateInWorkspace`, `getUpdatesForReport`,
`getPageComponentIdsForReport`.
- All mutations run inside `withTransaction`, emit an `emitAudit` row
before returning, and throw the typed `ServiceError` hierarchy.
- Zod schemas are the contract; `.parse()` runs at every service entry
as defence-in-depth.
- `notifyStatusReport` is the separate-call notification dispatch that
the dashboard invokes as its second mutation — preserves today's
two-call pattern required by the Edge runtime.
## Surfaces
- **tRPC** (`packages/api/src/router/statusReport.ts`): every procedure
now calls a service. External contract is preserved bit-for-bit —
`create` still returns `{ ...initialUpdate, notifySubscribers }`,
`list` still returns the enriched `{updates,pageComponents,page}` shape
the dashboard consumes, etc. `emailRouter.sendStatusReport` becomes a
thin wrapper over `notifyStatusReport`.
- **Connect RPC** (`apps/server/src/routes/rpc/services/status-report/`):
handler extracts proto → parse → service call → convert back. Domain
helpers previously exported from this folder (and consumed by Slack)
are gone; callers go through services directly.
- **Slack** (`apps/server/src/routes/slack/interactions.ts`): the four
status-report branches (create / add-update / update / resolve) call
services. The maintenance branch still writes to db directly — that
migrates in PR 2.
## Adapters
- `packages/api/src/service-adapter.ts` — `toServiceCtx(ctx)` +
`toTRPCError(err)` (ServiceError → TRPCError).
- `apps/server/src/routes/rpc/adapter.ts` — `toServiceCtx(rpcCtx)` +
`toConnectError(err)`. Synthesises `actor.keyId` as `ws:<workspaceId>`
until the auth interceptor captures the real key id (called out in
the plan's open questions).
- `apps/server/src/routes/slack/service-adapter.ts` — refetches the full
workspace for the `PendingAction`'s workspaceId and builds a Slack
actor context; `toSlackMessage(err)` renders human-readable failures.
## Enforcement
- Biome `noRestrictedImports` (nursery rule) scoped to
`packages/api/src/router/statusReport.ts` +
`apps/server/src/routes/rpc/services/status-report/**`. Tests are
ignored since setup/cleanup legitimately manipulates db state.
- Subpath export `@openstatus/services/status-report` so surfaces
import the domain barrel directly.
- `packages/api` / `apps/server` now declare `@openstatus/services` as
a dependency.
## Tests
- Integration tests in `packages/services/src/status-report/__tests__/`
cover happy paths, workspace isolation, cascading deletes, the
`ForbiddenError` / `NotFoundError` paths, and the slack-actor audit
branch. `expectAuditRow` asserts on the in-memory audit buffer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- **Dashboard build**: `data-table-row-actions.tsx:35` accesses `row.original.page.customDomain`, which the old tRPC zod schema made non-nullable. My migrated service returned `page: Page | null`, breaking the type. Narrow `page` to non-null in the tRPC `get` / `list` wrappers via a tiny `narrowPage` helper that throws on the shouldn't- happen case (status report without its page) — matches the old zod parse behaviour. - **`listStatusReports` silent truncation (Cubic finding)**: the tRPC wrapper hard-coded `limit: 100`, silently dropping reports beyond that ceiling for workspaces with >100 reports. Service now allows an unbounded `limit` (min 1, no max); tRPC passes a `10_000` sentinel and Connect keeps its external `max=100` cap in the handler. - **`LIMIT_EXCEEDED` Connect mapping (Cubic finding)**: map to `Code.ResourceExhausted` rather than `Code.PermissionDenied` — quota/rate-limit is the documented meaning of ResourceExhausted in Connect/gRPC, while PermissionDenied means "authorised but not entitled". Correct semantics on the wire. Biome `include` key (Cubic's third finding) is verified correct for Biome 1.8.3 — OverridePattern schema documents `include` as the glob selector, and our override fires as expected (it flags the Connect test file in the scoped dir). No change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e PR Applies to status-report since the same patterns were copy-pasted across both domains: - **Dedupe pageComponentIds** in `validatePageComponentIds` — duplicate ids in the input would hit the composite-PK constraint on `status_report_to_page_component` during insert. - **Batch list enrichment** — `listStatusReports` previously fanned out 3 queries per row (updates / component associations / page + page components). Dashboard calls it with `limit: 10_000`, so the N+1 was real. Rewritten as three batched `IN` queries regardless of row count; pages are deduplicated so reports sharing a page share the Page object. `getStatusReport` reuses the batch path with a singleton. - **Idempotent `delete`** — the old tRPC `statusReport.delete` silently succeeded when the row was missing; my service throws `NotFoundError`. Catch and swallow in the tRPC wrapper only. Connect still returns 404 on missing — external API semantics preserved. - **Connect numeric-id error** — `parsePageComponentIds` was throwing `invalidDateFormatError` for non-numeric input. Replaced with an inline `ConnectError(Code.InvalidArgument)` with the correct message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `packages/services/package.json` and `packages/importers/package.json`
to the `install` stage bind list so `bun install --frozen-lockfile` can
resolve the dep graph inside the Docker build.
Without this, the moment main receives:
- `@openstatus/services` as a dep of `apps/server` (landing via the
status-report PR that this branch targets), bun install fails with
a missing workspace package.
- `@openstatus/importers` becomes a transitive dep of
`@openstatus/services` later in the stack (via the import-domain
migration), so we bind it proactively in the same commit rather
than landing a second dofigen touch-up.
`packages/importers` already exists in the repo and isn't used by
apps/server directly at this point, so binding it early is a no-op
for bun until it actually becomes a dep. Alternative was two sequential
fix PRs; one covers the whole stack cleaner.
`Dockerfile` + `dofigen.lock` are regenerated from `dofigen.yml` via
`dofigen update` — both are generated outputs that must stay in sync
with the yml source.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mxkaske
force-pushed
the
feat/services-status-report
branch
from
3dfbbb8 to
db59d2f
Compare
Member
Author
|
@claude review |
`packages/services/src/status-report/notify.ts` imports from `@openstatus/subscriptions`, which transitively pulls in `@openstatus/emails`. The emails package validates `RESEND_API_KEY` via `@t3-oss/env-core` at module-load time and throws on missing — which killed `@openstatus/services#test` in CI (neither the test.yml workflow nor local `pnpm test` sets the var). Adds a bun test preload that stubs the env var with a dummy value before any test file is imported. Tests never exercise the real Resend dispatch path, so this is scoped to unblocking the import graph rather than mocking the dispatcher itself. Added: - packages/services/bunfig.toml — points bun:test at the preload. - packages/services/test/preload.ts — sets the dummy env var. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CI failures surfaced after the services migration changed error
shapes for status-report writes:
- **tRPC `statusReport.create` / `.updateStatus` cross-workspace tests**
expected `FORBIDDEN` when a caller passed pageComponentIds belonging
to another workspace. The service's `validatePageComponentIds` now
throws `NotFoundError` for the same case — intentionally, so cross-
workspace lookups don't leak resource existence. Updated both tests
to assert `NOT_FOUND` and left a comment pointing at the migration
rationale.
- **Connect `CreateStatusReport` page-mismatch test** asserted the
error message contained "does not match the page ID". The service
throws `ConflictError("pageId X does not match the page (Y) of the
selected components.")` — no "ID" in the wording. Loosened the
assertion to "does not match the page" so it matches the actual
service string.
Both are pure test updates — the new service behavior is the intended
contract; the tests had drifted from the pre-migration wording.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five findings from the PR review, fixed in one commit: **P1 — tRPC `LIMIT_EXCEEDED` → `TOO_MANY_REQUESTS`** `packages/api/src/service-adapter.ts` mapped `LIMIT_EXCEEDED` to `FORBIDDEN`. `FORBIDDEN` means "authenticated but not allowed to do this on this resource" — plan quota exhaustion is semantically different. Mirrors the Cubic fix on the Connect side (which moved from `PermissionDenied` to `ResourceExhausted`). **P2 — fragile Drizzle join result key in `enrichReportsBatch`** `packages/services/src/status-report/list.ts` read `row.status_report_to_page_component.statusReportId` — Drizzle auto-derives that key from the schema variable name at import time, so a rename would silently produce `undefined` at runtime. Replaced with explicit-column select (`reportId: statusReportsToPageComponents.statusReportId`) which is typed and rename-safe. **P2 — Slack `NOT_FOUND` messages leak internal row IDs** `NotFoundError` formats as `"<entity> <id> not found"` (e.g. `"page_component 17 not found"`). For API consumers that's fine — they're developers. For Slack workspace users those IDs are meaningless noise and surface internals. `toSlackMessage` now returns a fixed `"Couldn't find what you were looking for."` for `NOT_FOUND`; other `ServiceError` codes keep their hand-written messages (they're sentence-style to start with). **P3 — test cleanup without try/finally** `packages/services/src/status-report/__tests__/status-report.test.ts` had 13 tests that did inline `db.delete(statusReport)` cleanup at the end of their body — skipped on assertion failure. Replaced with a shared `createdReportIds` array + `afterEach` drain so cleanup runs regardless of test outcome. **P3 — hoisted `toServiceCtx` out of Slack `executeAction` switch** Every service-backed branch loaded the workspace row via `toServiceCtx`. Hoisted the call above the switch — halves db lookups per Slack action execution. The `createMaintenance` branch (still on db until PR 2) doesn't use the ctx but the extra lookup there is negligible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two server-side tests were failing after the migration: - `updates pageId when changing components to different page` - `clears pageId when removing all components` Both expected `report.pageId` to track the association set: - Swap components to a different page → `pageId` moves. - Empty the association set → `pageId` becomes null. The service was enforcing a stricter invariant — it *threw* a `ConflictError` when the new components' page differed from the report's current page, and left `pageId` untouched when associations were cleared. That was an overcautious choice in the initial migration; no caller today wants a "report pinned to page X regardless of its components" guarantee. Simplified to: whenever `pageComponentIds` is supplied, `updateValues.pageId = validated.pageId`. The zero-component case sets `pageId = null`; the one-page case moves the report to that page. Mixed-page inputs are still rejected by `validatePageComponentIds`'s `componentPageIds.size > 1` check, so the only reachable states here are "one page" or "no components". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@claude review and check again |
|
Claude finished @mxkaske's task in 2m 1s —— View job PR Re-Review: feat(services): migrate status-report domain (PR 1/N)Tasks:
Follow-up review — all previous findings addressedCommit P1 — tRPC P2 — Fragile Drizzle join key in P2 — Slack P3 — Test cleanup without try/finally ✅ P3 — Multiple Bonus fix — Remaining minor notes (non-blocking, no action needed)
VerdictAll P1/P2 issues resolved, P3 fixes solid, bonus behavioral fix correct. The service layer contract is clean and the enforcement mechanism (Biome |
mxkaske
added a commit
that referenced
this pull request
Apr 23, 2026
Three CI failures on #2103: **Slack `toSlackMessage` — revert to uniform generic message.** The P3 fix on #2101 kept hand-written `ServiceError` messages for `FORBIDDEN` / `CONFLICT` / `VALIDATION` / `LIMIT_EXCEEDED` — but the existing `does not leak internal error details to user` test expects every error to surface as a single `"Something went wrong. Please try again."` string. The intent is stronger than just "strip row IDs": Slack users aren't developers, so none of the service error wording is appropriate for them. Reduced the adapter to a one-liner that always returns the generic message; detailed error context still flows to logtape/Sentry via the catch site upstream. **Handler test — "does not match the page ID" → "does not match".** `ConflictError` wording drifted during the migration (same as the status-report fix). Loosened to the stable fragment. **Handler test — "Page not found" → "not found".** `NotFoundError("page", id)` formats as `"page <id> not found"` (lowercase `page`). Pre-migration handler emitted capital-P `"Page not found"`. Loosened the substring so both forms pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mxkaske
added a commit
that referenced
this pull request
Apr 24, 2026
* feat(services): migrate maintenance domain onto service layer
Second domain migration, stacked on the status-report PR. Same shape as
PR 1 — every write path and read path for `maintenance` now goes through
`@openstatus/services/maintenance`; tRPC / Connect / Slack are thin
adapters.
## Services (`packages/services/src/maintenance/`)
- `createMaintenance`, `updateMaintenance`, `deleteMaintenance`,
`listMaintenances`, `getMaintenance`, `notifyMaintenance`.
- Date range validated at the Zod refine + again inside `updateMaintenance`
when partial updates could cross the invariant.
- All mutations inside `withTransaction`, emit `emitAudit` before returning.
- Internal helpers duplicated from status-report (`validatePageComponentIds`,
`updatePageComponentAssociations`, `getMaintenanceInWorkspace`,
`getPageComponentIdsForMaintenance`, `getPageComponentsForMaintenance`).
A third consumer should trigger the shared-helper extraction.
## Surfaces
- **tRPC** (`packages/api/src/router/maintenance.ts`): all four procedures
(`delete` / `list` / `new` / `update`) now call services.
`emailRouter.sendMaintenance` becomes a thin wrapper over
`notifyMaintenance`.
- **Connect RPC** (`apps/server/src/routes/rpc/services/maintenance/`):
handler rewritten as proto → parse → service → convert.
External contract preserved.
- **Slack** (`interactions.ts`): `createMaintenance` branch now calls
services. Also extracts `getPageUrl`/`getReportUrl` into
`apps/server/src/routes/slack/page-urls.ts` — pure transport-layer URL
formatting, left on db directly but isolated so `interactions.ts` itself
becomes db-free.
## Enforcement
- Biome `noRestrictedImports` override now includes:
`packages/api/src/router/maintenance.ts`,
`apps/server/src/routes/rpc/services/maintenance/**`, and
`apps/server/src/routes/slack/interactions.ts`.
(`page-urls.ts`, `service-adapter.ts`, `confirmation-store.ts`, and
`workspace-resolver.ts` remain outside scope — they legitimately need db
access for transport/URL/auth concerns.)
- Subpath export `@openstatus/services/maintenance`.
## Tests
- Integration tests in `packages/services/src/maintenance/__tests__/`
cover happy paths, workspace isolation, cascade deletes, cross-workspace
`NotFoundError` / `ForbiddenError`, the Zod date-range refine, the slack
actor audit branch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/maintenance): address Cubic review
Mirrors the symmetric fixes applied to status-report on the parent
branch:
- **Dedupe `pageComponentIds`** in `validatePageComponentIds` — duplicate
ids would violate the composite PK on `maintenance_to_page_component`.
- **Batch list enrichment** — `listMaintenances` previously ran 2 extra
queries per row; rewritten as one IN query regardless of list size,
pairing well with the 10_000 sentinel tRPC passes.
- **Idempotent tRPC `delete`** — swallow `NotFoundError` in the wrapper
to preserve the old drizzle-returning behaviour; Connect still
returns 404.
- **Connect numeric-id error** — replace `invalidDateFormatError` with an
inline `ConnectError(Code.InvalidArgument)` for malformed page component
ids. Correct error message on the wire.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/maintenance): parity with status-report post-review fixes
Three issues mirror what landed on status-report after its PR-review
pass — this branch was behind because it hadn't been rebased onto the
updated status-report yet.
**`update.ts` — pageId follows components**
The service was enforcing a strict invariant: throw `ConflictError`
when new components belonged to a different page, and leave `pageId`
untouched when components were cleared. The Connect
`UpdateMaintenance > clears pageId when removing all components`
test fails against this — it expects `pageId` to null out on empty
components. Same reasoning as the status-report fix: `pageId` should
follow the association set. Mixed-page inputs still rejected
upstream by `validatePageComponentIds`.
**Handler test — error message wording**
Two assertions expected the old handler's `"Start time (from) must
be before end time"` message. Service throws `"End date must be
after start date."` via `ConflictError`. Updated both assertions to
match service wording — consistent with how the status-report
message drift was resolved.
**Service test — cleanup without try/finally**
7 tests did inline `db.delete(maintenance)` at the end of their body;
a failing assertion skipped cleanup and orphaned rows. Replaced with
a shared `createdMaintenanceIds` array drained in `afterEach`,
matching the status-report pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): Slack error opacity + maintenance test drift
Three CI failures on #2103:
**Slack `toSlackMessage` — revert to uniform generic message.**
The P3 fix on #2101 kept hand-written `ServiceError` messages for
`FORBIDDEN` / `CONFLICT` / `VALIDATION` / `LIMIT_EXCEEDED` — but the
existing `does not leak internal error details to user` test expects
every error to surface as a single `"Something went wrong. Please try
again."` string. The intent is stronger than just "strip row IDs":
Slack users aren't developers, so none of the service error wording
is appropriate for them. Reduced the adapter to a one-liner that
always returns the generic message; detailed error context still
flows to logtape/Sentry via the catch site upstream.
**Handler test — "does not match the page ID" → "does not match".**
`ConflictError` wording drifted during the migration (same as the
status-report fix). Loosened to the stable fragment.
**Handler test — "Page not found" → "not found".**
`NotFoundError("page", id)` formats as `"page <id> not found"`
(lowercase `page`). Pre-migration handler emitted capital-P
`"Page not found"`. Loosened the substring so both forms pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate incident domain (PR 3/N) (#2104)
* feat(services): migrate incident domain onto service layer
Third domain migration, stacked on maintenance. tRPC-only — no Connect
handler, no Slack path — so this PR mostly exercises the
`ServiceContext` actor variants without surface-specific adapters.
## Services (`packages/services/src/incident/`)
- `acknowledgeIncident`, `resolveIncident`, `deleteIncident`,
`listIncidents`, `getIncident`.
- Both `acknowledge` / `resolve` stamp the acting user's id onto
`acknowledged_by` / `resolved_by` via the new `tryGetActorUserId`
helper on `ServiceContext`. Non-user actors (system, webhook, API keys
without a linked userId) stamp `null`.
- Idempotent `ConflictError` when an incident is already acknowledged
or resolved — matches existing tRPC's `BAD_REQUEST` semantics.
- All mutations wrapped in `withTransaction`, emit `emitAudit`.
## list.ts
- Batch-enriches monitors via a single IN query against distinct
`monitorId`s — pairs well with the 10_000 sentinel tRPC passes.
Avoids the N+1 Cubic flagged on the maintenance PR.
## Surfaces
- **tRPC** (`packages/api/src/router/incident.ts`): all four procedures
call services. `delete` catches `NotFoundError` to preserve the
pre-migration idempotent behaviour. `list` narrows `monitor` to
non-null in the return type — every incident is expected to have an
associated monitor via the FK. `acknowledge` / `resolve` still return
`true` to match the old contract.
## Context helper
- `tryGetActorUserId(actor)` in `packages/services/src/context.ts` —
returns the openstatus user id for user/apiKey/slack actors when
available, `null` for system/webhook. Exported from the root barrel.
## Enforcement
- Biome `noRestrictedImports` scope adds
`packages/api/src/router/incident.ts`.
- Subpath export `@openstatus/services/incident`.
## Tests
- `packages/services/src/incident/__tests__/incident.test.ts` — happy
paths for acknowledge / resolve / delete, already-acknowledged and
already-resolved `ConflictError`, workspace isolation across all
mutations, list workspace isolation, and the batch monitor enrichment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/incident): address Cubic review
- **TOCTOU on acknowledge / resolve** — the old read-then-update allowed
two concurrent acknowledgers (or resolvers) to both succeed. Replaced
with a conditional update (`WHERE acknowledged_at IS NULL` /
`WHERE resolved_at IS NULL`); the loser of the race gets no row back
and we throw the same `ConflictError` the pre-read would have raised.
Dropped the now-unreachable `InternalServiceError` branch.
- **Monitor enrichment workspace scope** — `enrichIncidentsBatch` now
filters `monitor.workspaceId = ctx.workspace.id` alongside the
`inArray` on monitor id. Defence-in-depth: the `incident.monitorId`
column has no FK against workspace ownership, so a cross-workspace
pointer (however unlikely) no longer leaks the other workspace's
monitor row.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate monitor domain tRPC (PR 4/N) (#2105)
* feat(services): migrate monitor domain tRPC onto service layer
Fourth domain, the largest so far. Migrates the **tRPC** monitor router
(all 14 procedures) onto `@openstatus/services/monitor`. The Connect
handler (`apps/server/src/routes/rpc/services/monitor/`) and the v1 REST
`apps/server/src/routes/v1/monitors/*` endpoints stay untouched for now
— they are separate external-API surfaces and warrant their own
follow-up PRs with their own test suites.
- `createMonitor`, `cloneMonitor`, `deleteMonitor`, `deleteMonitors`
(bulk soft-delete), `getMonitor`, `listMonitors`.
- `updateMonitorGeneral` — preserves the existing tRPC `updateGeneral`
behaviour including jobType switching (HTTP ↔ TCP ↔ DNS). Called out
as a code smell in the explore, but preserved intentionally since
it's the dashboard's current edit flow. Kept jobType-agnostic rather
than split into 3 separate update methods.
- `updateMonitorRetry`, `updateMonitorFollowRedirects`,
`updateMonitorOtel`, `updateMonitorPublic`,
`updateMonitorResponseTime` — field-specific setters.
- `updateMonitorSchedulingRegions` — enforces plan limits
(periodicity / region-count / region-access) + validates the
private-location ids before replacing the association set.
- `updateMonitorTags`, `updateMonitorNotifiers` — validate and
replace the tag / notifier association sets.
- `bulkUpdateMonitors` — batched toggle of `public` / `active`
across multiple monitor ids. Matches the old `updateMonitors`
procedure.
- All mutations run inside `withTransaction`, emit `emitAudit`.
- Cascade deletes: `monitor_tag_to_monitor`,
`notifications_to_monitors`, `page_component` rows are torn down on
delete (matches pre-migration behaviour — bypasses FK cascades
because some rows reference the monitor without cascade).
- `validateTagIds` / `validateNotificationIds` / `validatePrivateLocationIds`
— workspace-scoped validators with dedupe.
- `pickDefaultRegions(workspace)` — ports the old "randomly pick 4
free / 6 paid regions excluding deprecated" logic.
- `serialiseAssertions` / `headersToDbJson` — assertion / header
serialisation moved out of the tRPC router.
- `countMonitorsInWorkspace` for quota checks.
`listMonitors` does two IN queries (tags + incidents) regardless of
list size. `getMonitor` reuses the same path with the richer
`{ notifications, privateLocations }` toggle and a singleton. Same
pattern as status-report's batched fix.
- **tRPC** (`packages/api/src/router/monitor.ts`): every procedure is a
thin wrapper. `delete` catches `NotFoundError` for idempotency.
`new` / `updateGeneral` keep the `testHttp` / `testTcp` / `testDns`
pre-save check at the tRPC layer — services unconditionally save,
callers decide whether to pre-validate.
- Connect RPC handler at `apps/server/src/routes/rpc/services/monitor/`
— still uses its own helpers. Will be migrated in a follow-up
(4b).
- v1 REST endpoints at `apps/server/src/routes/v1/monitors/*` — yet
another external surface; dedicated PR later.
- `packages/api/src/service/import.ts` monitor writes — covered by
the plan's dedicated PR 9.
- Biome `noRestrictedImports` scope adds
`packages/api/src/router/monitor.ts`. The Connect monitor handler
stays out of scope until 4b.
- Subpath export `@openstatus/services/monitor`.
- `packages/services/src/monitor/__tests__/monitor.test.ts` —
create (http / tcp / dns), delete cascade, bulk delete, clone,
tags / notifiers validation (forbidden when cross-workspace),
list / get with workspace isolation + soft-delete hiding,
updateMonitorGeneral round-trip.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): declare @openstatus/regions + @openstatus/assertions deps
The monitor domain pulls `regionDict` from `@openstatus/regions` (for
the default-region picker) and assertion classes / validators from
`@openstatus/assertions`. Both were missing from
`packages/services/package.json`, so Vercel's Next.js build for
`apps/status-page` — which transitively imports the services package
via `@openstatus/api` — failed with "Module not found" on
`packages/services/src/monitor/internal.ts:20` and `schemas.ts:1`.
Local pnpm workspaces resolved the imports via hoisting, which masked
the missing declarations until an actual Next.js build forced strict
resolution.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/monitor): address Cubic review
- **`timeout` / `degradedAfter` bounds** (schemas.ts) — mirror the
0–60_000 ms cap from `insertMonitorSchema`. Values outside the range
are rejected before the UPDATE instead of being silently persisted.
- **`jsonBody` assertion mapping** (internal.ts) — the serialiser was
silently dropping `jsonBody`-typed input because the runtime class
wasn't wired up. Added the `JsonBodyAssertion` branch; the class has
existed in `@openstatus/assertions` the whole time, this was just a
missing case.
- **Clone resets `status`** (clone.ts) — cloning no longer inherits the
source's current `error`/`degraded` health. Freshly cloned monitors
start at `"active"` and settle on their first check.
- **Delete filters soft-deleted** (delete.ts) — the pre-check now
includes `isNull(monitor.deletedAt)`, so a repeat delete returns
`NotFoundError` (preserved as idempotent at the tRPC layer) instead
of re-running the cascades and emitting duplicate audits.
- **List/get workspace scope on relations** (list.ts) — `enrichMonitorsBatch`
takes `workspaceId` and scopes the incident / tag / notification /
private-location IN queries to the caller's workspace. Defence-in-depth
against inconsistent FK data (none of those tables enforce workspace
ownership at the FK level).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate notification CRUD (PR 5/N) (#2106)
* feat(services): migrate notification CRUD onto service layer
Fifth domain. Migrates the four CRUD procedures — `list`, `new`,
`updateNotifier`, `delete` — of the notification tRPC router onto
`@openstatus/services/notification`. The three integration-helper
procedures (`sendTest`, `createTelegramToken`, `getTelegramUpdates`)
stay inline at the tRPC layer and are explicitly out of scope for this
migration.
- `createNotification` — enforces plan limits on both the channel count
(`notification-channels`) and the provider itself (`sms` / `pagerduty`
/ `opsgenie` / `grafana-oncall` / `whatsapp` require plan flags);
validates the loose `data` payload against `NotificationDataSchema`;
validates monitor ids are in-workspace and not soft-deleted.
- `updateNotification` — replaces name / data / monitor associations in
a single transaction with the same validation rules as create.
- `deleteNotification` — hard-delete (FK cascade clears associations).
The tRPC wrapper swallows `NotFoundError` to preserve the old
idempotent behaviour.
- `listNotifications`, `getNotification` — batched IN query enriches
monitors per notification. Monitor enrichment is workspace-scoped and
filters soft-deleted monitors for defence-in-depth.
- All mutations run inside `withTransaction`, emit `emitAudit`.
- **tRPC** (`packages/api/src/router/notification.ts`): `list` / `new` /
`updateNotifier` / `delete` become thin service wrappers. `sendTest`
+ `createTelegramToken` + `getTelegramUpdates` are unchanged.
- **`sendTest` migration** — the dispatch switch imports from 10
`@openstatus/notification-*` packages. Moving it into services would
pull those as direct deps; the plan's phrasing ("Channel CRUD +
test-dispatch") allows this as a later extraction.
- **`createTelegramToken` / `getTelegramUpdates`** — redis + external
Telegram API helpers; transport UX, not domain operations.
- **Biome scope for `notification.ts`** — the file still imports
`@openstatus/db/src/schema` for the `sendTest` provider data schemas.
Will land with the sendTest migration follow-up.
- **Connect RPC notification handler** — stays on its own helpers;
follow-up aligned with PR 4's Connect deferral.
- `__tests__/notification.test.ts` covers create (including
`ValidationError` on malformed data, `LimitExceededError` on gated
provider, `ForbiddenError` on cross-workspace monitor), update
(association replacement, cross-workspace `NotFoundError`), delete,
list/get workspace isolation + monitor enrichment scope.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/notification): address Cubic review
- **Update flow plan gate** (update.ts) — `updateNotification` was
skipping `assertProviderAllowed`, so a user who downgraded their plan
could still edit a notification configured with a now-restricted
provider. Re-check against the stored `existing.provider` to match
the create-time gate.
- **Provider / data match** (internal.ts) — `NotificationDataSchema` is
a union, so `{ provider: "discord", data: { slack: "…" } }` passed
the union check even though the payload key doesn't match the
provider. `validateNotificationData` now takes the provider and
asserts `provider in data` after the top-level parse. Applied in
both `create` and `update` (update uses the stored provider since
the API doesn't allow provider changes).
Added a test for the mismatch case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/notification): tighten data validation against provider schema
Cubic's follow-up on the previous fix was right: checking only
`provider in data` isn't enough. `NotificationDataSchema` is a union,
so a payload like `{ discord: "not-a-url", slack: "valid-url" }` passes
because the union matches the slack variant — the extra `discord` key
is ignored, and my key-presence check sees `"discord"` and lets it
through.
Replaced the union parse + key check with a provider-specific schema
lookup (`providerDataSchemas[provider].safeParse(data)`). Each
canonical channel schema is keyed by its provider name and validates
the shape / content of the value, so the new check catches both the
mismatched-provider and malformed-payload cases in one pass.
Added a test covering the exact case Cubic flagged — invalid `discord`
URL alongside a valid `slack` URL now rejects with ValidationError.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate page-component domain (PR 6/N) (#2107)
* feat(services): migrate page-component domain onto service layer
Sixth domain, tRPC-only. Migrates `list`, `delete`, and `updateOrder`
onto `@openstatus/services/page-component`.
- `listPageComponents` — workspace-scoped filter + optional pageId
filter. Batched enrichment in four IN queries (monitors, groups,
status reports via join, maintenances via join). All relation queries
scoped to the caller's workspace for defence-in-depth.
- `deletePageComponent` — hard-delete. Cascade clears the
`status_report_to_page_component` / `maintenance_to_page_component`
associations. The tRPC wrapper swallows `NotFoundError` to preserve
the pre-migration idempotent behaviour.
- `updatePageComponentOrder` — the complex one. Mirrors the existing
diff-and-reconcile pass faithfully (≈220 lines → a single transaction):
1. Assert the page is in the workspace.
2. Enforce the workspace's `page-components` plan cap.
3. Validate every monitor id in the input set.
4. Remove monitor components whose monitorId isn't in the input;
remove static components based on whether the input carries ids.
5. Clear `groupId` before dropping groups (FK safety), then recreate
groups.
6. Upsert monitor components via `onConflictDoUpdate` on the
`(pageId, monitorId)` unique constraint (preserves ids).
7. Update existing static components by id; insert new ones.
Audit: `page_component.update_order` / `page_component.delete`.
- **tRPC** (`packages/api/src/router/pageComponent.ts`): all three
procedures call services. `delete` catches `NotFoundError` and
returns the old `drizzle.returning()`-shaped empty array. The
pre-existing `pageComponent.test.ts` (tests cross-workspace monitorId
→ `TRPCError(FORBIDDEN)`) is untouched and still valid — my services
throw `ForbiddenError`, which `toTRPCError` maps to the same code.
- Biome `noRestrictedImports` scope adds
`packages/api/src/router/pageComponent.ts`.
- Subpath export `@openstatus/services/page-component`.
- `__tests__/page-component.test.ts` covers `updatePageComponentOrder`
happy path (creates monitor + static + grouped components), rejects
cross-workspace monitorId and cross-workspace pageId, `list`
workspace isolation, `delete` cross-workspace `NotFoundError`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): Connect RPC notification handler catch-up (#2108)
* feat(services): Connect RPC notification handler onto services (catch-up)
Follow-up to PR 5 — noticed on review that my PRs from PR 4 onwards had
been narrowing scope to tRPC only and deferring Connect handlers, which
was piling up. This closes the notification Connect gap.
`apps/server/src/routes/rpc/services/notification/index.ts` — the five
CRUD methods now delegate to `@openstatus/services/notification`:
- `createNotification` → `createNotification` service (handles the
plan-count limit, per-provider plan gate, and data-schema validation
internally — the Connect-side `checkNotificationLimit` /
`checkProviderAllowed` / `validateProviderDataConsistency` calls are
gone).
- `getNotification`, `listNotifications`, `updateNotification`,
`deleteNotification` — thin proto-to-service-to-proto wrappers.
- `updateNotification` reads the existing record via the service and
fills in missing fields (Connect's update is partial; the service
expects a full payload), then applies the update.
Left inline:
- `sendTestNotification` — calls `test-providers.ts` (external HTTP).
- `checkNotificationLimit` RPC method — returns the count info via
`./limits.ts` helpers (pure queries, no domain mutation).
The local Connect helpers (`validateProviderDataConsistency`,
`checkNotificationLimit`, `checkProviderAllowed`, and the ad-hoc
`validateMonitorIds` / `updateMonitorAssociations` / `getMonitorById` /
`getMonitorCountForNotification` / `getMonitorIdsForNotification`) are
no longer imported by `index.ts`; they remain in their files because
`test-providers.ts` and the unmigrated Connect monitor handler still
reference some of them.
Added `apps/server/src/routes/rpc/services/notification/index.ts` to
the `noRestrictedImports` scope. The directory-level glob isn't a fit
because `limits.ts` and `test-providers.ts` legitimately need direct
db access until their own follow-up migrations.
- **Connect monitor handler** (~880 lines, 6 jobType-specific
create/update methods + 3 external-integration methods) — requires a
much bigger refactor. Flagged as dedicated PR 4b; tracked separately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): dedupe monitor ids in Connect createNotification response
Cubic's P2 catch: the service dedupes `monitors` before the insert
(via `validateMonitorIds` in the services package), but the Connect
handler echoed `req.monitorIds` verbatim back in the response. For an
input like `["1", "1", "2"]` the DB stored `[1, 2]` while the response
claimed `["1", "1", "2"]` — caller state diverges from persistence.
Echo `Array.from(new Set(req.monitorIds))` instead so the response
matches what's actually stored.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate page authoring (PR 7/N) (#2109)
* feat(services): migrate page (status-page authoring) onto service layer
Seventh domain. Migrates the 13 authoring procedures in
`pageRouter` onto `@openstatus/services/page`. Deliberately scoped to
authoring CRUD only:
- `statusPage.ts` — public viewer endpoints (subscribe / get / uptime /
report / verify / unsubscribe) are a separate surface that doesn't
use the authenticated `ServiceContext`; dedicated follow-up.
- Connect `apps/server/src/routes/rpc/services/status-page/**` — ~1500
lines with 18 methods (page CRUD + components + groups + subscribers
+ view). Too big for this PR; dedicated follow-up, same shape as the
Connect monitor deferral.
- `createPage` / `newPage` — full vs minimal create; both enforce the
`status-pages` plan cap and (for `createPage`) the per-access-type
plan gates (password-protection, email-domain-protection, ip-
restriction, no-index).
- `deletePage` — FK cascade clears components / groups / reports /
subscribers.
- `listPages` — batched enrichment with `statusReports`.
- `getPage` — enriched with `maintenances` / `pageComponents` /
`pageComponentGroups`.
- `getSlugAvailable` — pure check against `subdomainSafeList` + DB.
- `updatePageGeneral` — with slug-uniqueness re-check on change.
- `updatePageCustomDomain` — persists the DB change and returns the
previous domain so the caller can diff. Vercel add/remove stays at
the tRPC layer (external integration).
- `updatePagePasswordProtection` — re-applies the same plan gates
the `create` path uses.
- `updatePageAppearance`, `updatePageLinks`, `updatePageLocales`
(gated on `i18n` plan flag), `updatePageConfiguration`.
- Audit action emitted for every mutation.
All 13 procedures are thin wrappers. `delete` catches `NotFoundError`
for idempotency. `updateCustomDomain` orchestrates:
1. `getPage` (via service) to read the existing domain.
2. `addDomainToVercel` / `removeDomainFromVercel` as needed.
3. `updatePageCustomDomain` (via service) to persist.
- Biome scope adds `packages/api/src/router/page.ts`. The router
imports `insertPageSchema` via the services re-export
(`CreatePageInput`) so the db-import ban applies cleanly.
- Subpath export `@openstatus/services/page`.
- `__tests__/page.test.ts` covers `newPage` happy / reserved /
duplicate, `createPage` monitor attachment + cross-workspace monitor,
`updatePageGeneral` rename + duplicate-slug conflict + cross-workspace,
`updatePageLocales` plan gate, list / get / slug-available workspace
isolation, delete cross-workspace NotFoundError.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate Connect status-page page CRUD onto services
Extends PR #2109 to cover the Connect RPC status-page handler's page
CRUD surface (create / get / list / update / delete), matching the
migration that landed for tRPC's `pageRouter`. The other 13 methods
(components, groups, subscribers, viewer) still read the db directly —
they're separate domains that'll need their own services in follow-ups.
- create / get / delete call into `@openstatus/services/page` and
preserve the granular Connect errors (`statusPageNotFoundError`,
`slugAlreadyExistsError`) by pre-checking before the service call or
catching `NotFoundError` → re-throwing the richer variant.
- list fetches via the service and paginates in-memory; status-page
quota is bounded per workspace so the extra enrichment is negligible.
- update loads the existing page via the service, then orchestrates the
per-section updates (`updatePageGeneral`, `updatePageLinks`,
`updatePageAppearance`, `updatePageCustomDomain`, `updatePageLocales`,
`updatePagePasswordProtection`) inside a shared transaction so a
partial failure can't leave the page half-updated. Each service's
internal `withTransaction` detects the pre-opened tx and skips
nesting.
- Proto-specific format validations (https icon URL, custom-domain
regex, IPv4 CIDR, email-domain shape) and the i18n PermissionDenied
path stay at the handler — they don't exist in the zod insert schema
and their error codes would change if deferred to the service.
- `Page` from the service parses `authEmailDomains` / `allowedIpRanges`
into arrays, while the converters (still used by the unmigrated
methods) expect the comma-joined string form. `serviceToConverterPage`
bridges the two shapes at the call sites that need it.
Biome scope deliberately unchanged: the file still imports from
`@openstatus/db` for the 13 legacy methods, so the override would
light up the whole file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/page): address Cubic review on #2109
Four issues flagged across two Cubic reviews:
- `createPage` skipped `assertSlugAvailable`, so full-form creates
could bypass reserved/duplicate slug validation and either create a
duplicate or fail late on a DB constraint instead of the clean
`ConflictError`. Added the check alongside the existing quota gate.
- `createPage` passed `passwordProtected` / `allowedIpRanges` but not
`allowIndex` to `assertAccessTypeAllowed`, bypassing the `no-index`
plan gate on create. Now forwarded.
- `UpdatePagePasswordProtectionInput.allowedIpRanges` accepted arbitrary
strings. Mirrored the CIDR validation from `insertPageSchema` — bare
IPs get `/32` appended, everything pipes through `z.cidrv4()`.
- `updatePagePasswordProtection` wrote `authEmailDomains:
input.authEmailDomains?.join(",")`, which evaluates to `undefined`
when the caller clears the field. Drizzle treats `undefined` as
"skip this column" on `.set()`, so stale email domains survived an
access-type switch. Added the `?? null` fallback to match the
neighboring `allowedIpRanges` line. This fixes the Connect
`updateStatusPage` path where switching away from AUTHENTICATED sets
`nextAuthEmailDomains = undefined` expecting the column to clear.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate workspace / user / invitation / api-key (PR 8/N) (#2110)
* feat(services): migrate workspace / user / invitation / api-key (PR 8/N)
Stacked on PR #2109. Eighth migration — four small domains consolidated into
one PR because each is narrow (roughly two to five procedures) and they
share no structural dependencies beyond already-migrated infrastructure.
**workspace** — `getWorkspace`, `getWorkspaceWithUsage` (pages + monitors +
notifications + page-components batched via drizzle relations),
`listWorkspaces` (takes `userId` explicitly since `list` runs across every
workspace the user has access to), `updateWorkspaceName`.
**user** — `getUser` (active, non-soft-deleted), `deleteAccount` (the paid-
plan guardrail stays; removes non-owned memberships, sessions, OAuth
accounts and blanks the PII columns inside a single tx).
**invitation** — `createInvitation` (plan gate counts pending invites
against the members cap so two outstanding invites can't both accept past
the limit), `deleteInvitation`, `listInvitations`, `getInvitationByToken`
(scoped by token **and** accepting email to prevent token-sharing),
`acceptInvitation` (stamps acceptedAt + inserts membership atomically).
**api-key** — `createApiKey` (returns plaintext token once), `revokeApiKey`
(workspace-scoped existence check inside the tx so concurrent revokes
resolve to a consistent NotFound rather than a silent no-op),
`listApiKeys` (replaces the legacy per-row `Promise.all` fan-out with a
single IN query for creator enrichment), `verifyApiKey` +
`updateApiKeyLastUsed` (no ctx required — the verify path runs before
workspace resolution and callers pass an optional `db` override).
All 14 procedures become thin `try { return await serviceFn(...) } catch
{ toTRPCError }` wrappers. Router shapes stay identical so the dashboard
needs no changes. Connect + Slack don't expose these domains today;
migrating their consumers is a follow-up.
Biome `noRestrictedImports` override adds the four router files. Subpath
exports `@openstatus/services/{workspace,user,invitation,api-key}` added
to the services package.
Deletes `packages/api/src/service/apiKey.ts` and its tests — fully
superseded by `packages/services/src/api-key/`. The auth middleware in
`apps/server` has its own inline apiKey verification and is unaffected.
- **`domain.ts`** — pure Vercel-API proxy with no DB usage; not part of
the migration surface. Stays as-is.
- **`packages/api/src/service/{import,telegram-updates}.ts`** — import
migration is PR 9; telegram-updates stays for a follow-up.
Per-domain `__tests__/*.test.ts` covers: workspace rename + audit, usage
counts, members cap hit on free plan, invitation token-mismatch rejection,
accept idempotency, api-key creation returning a bcrypt hash, list creator
enrichment, revoke NotFoundError on unknown ids, verifyApiKey happy / bad-
format / wrong-body paths, lastUsed debounce.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address Cubic review on #2110
Four issues flagged on PR 8:
- **P1 — `invitation/accept.ts`**: the read-then-write pattern let two
concurrent accepts both pass the `isNull(acceptedAt)` check and race
through the membership insert. Replaced with a conditional UPDATE that
re-asserts `isNull(acceptedAt)` in the WHERE clause and checks
`.returning()` rowcount. The loser gets `ConflictError`, the tx aborts
before membership inserts run.
- **P2 — `api-key/create.ts`**: `createdById` was taken from input and
the router spliced in `ctx.user.id`. Since that column is attribution
data (who owns the key, who the audit row blames), trusting input
would let any caller forge ownership. Derived from `ctx.actor` via
`tryGetActorUserId`; actors without a resolvable user id (system /
webhook / unlinked api-key) now get `UnauthorizedError` instead of a
silent NULL write. `createdById` removed from the input schema.
- **P2 — `invitation/delete.ts`**: audit row was emitted even when the
DELETE matched zero rows (unknown id / wrong workspace). Switched to
`.returning({ id })` and short-circuit before the audit emit so the
log only reflects actual deletions.
- **P2 — `invitation/list.ts`**: the `if (!input.email)` →
`UnauthorizedError` branch in `getInvitationByToken` was unreachable
because `z.email()` already rejects empty / malformed emails at
`.parse()`. Removed the dead branch; the router keeps its own
pre-call check for `ctx.user.email`, so the transport-level
UnauthorizedError path is preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services): migrate import domain (PR 9/N) (#2111)
* feat(services): migrate import domain (PR 9/N)
Stacked on PR #2110. Ninth and final domain — lifts the ~1,000-line
`packages/api/src/service/import.ts` orchestrator into the services
package as its own `@openstatus/services/import` domain.
Split into focused files:
- **`schemas.ts`** — `PreviewImportInput` / `RunImportInput` zod. Provider
discriminator + per-provider page-id fields live here; options schema
is separately exported for callers that want to pre-validate.
- **`provider.ts`** — `createProvider` factory + `buildProviderConfig`
reshape helper, isolated from the orchestrator so adding a provider is
a one-file change.
- **`limits.ts`** — `addLimitWarnings` (shared by preview + run). Pure
mutation on the `ImportSummary` argument; no writes.
- **`utils.ts`** — `clampPeriodicity` + `computePhaseStatus` helpers.
- **`phase-writers.ts`** — the seven phase writers (page / component
groups / components / incidents / maintenances / monitors /
subscribers). Each takes a `DB` explicitly so callers can thread a
pre-opened tx; failing resources get `status: "failed"` with an error
string rather than throwing.
- **`preview.ts`** — dry-run only; validates credentials, runs the
provider with `dryRun: true`, emits warnings.
- **`run.ts`** — the orchestrator. Now owns the `pageId` ownership
check (previously duplicated in the tRPC router) and emits exactly
**one** `import.run` audit row regardless of outcome so partial /
failed runs still show up in the audit signal. Deliberately *not*
wrapped in `withTransaction` — imports can span minutes across dozens
of writes and the existing UX is phase-level recovery.
124 lines → 28 lines. The router is now a thin `previewImport` /
`runImport` wrapper; the input schemas and all validation live in the
service. The router-level `TRPCError`-throwing `pageId` ownership check
moved into `runImport` so non-tRPC callers (Slack / future) get the
same guard.
- Provider validation failure: `TRPCError("BAD_REQUEST")` →
`ValidationError` → `TRPCError("BAD_REQUEST")`. Net-same.
- Unknown / wrong-workspace `pageId`: `TRPCError("NOT_FOUND")` →
`NotFoundError` → `TRPCError("NOT_FOUND")`. Net-same.
- Unit tests for `addLimitWarnings` / `clampPeriodicity` /
`computePhaseStatus` move to `packages/services/src/import/__tests__/`.
- Router integration tests (`packages/api/src/router/import.test.ts`)
that previously called `previewImport` / `runImport` directly to
override workspace limits now route through `makeCaller(limitsOverride)`
with an explicit `provider: "statuspage"` field. This also fixes four
pre-existing TypeScript errors where those calls were missing the
(required) provider discriminator.
- Biome `noRestrictedImports` override adds `packages/api/src/router/import.ts`.
- Subpath export `@openstatus/services/import` added.
- `@openstatus/importers` added to services deps; services `tsconfig.json`
bumped to `moduleResolution: "bundler"` so the importers package-exports
map resolves (same setting `packages/api` already uses).
Deletes `packages/api/src/service/import.ts` (1042 lines) and its test
file (463 lines). Only `telegram-updates.ts` remains in
`packages/api/src/service/` — that's slated for a follow-up PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(services/import): per-resource audit + Cubic fixes on #2111
Two changes folded together:
Every phase writer now emits one `emitAudit` row per *created*
resource, matching what the domain services emit for normal CRUD:
| Phase | Audit action
| --- | ---
| page | `page.create`
| componentGroups | `page_component_group.create`
| components | `page_component.create`
| monitors | `monitor.create`
| incidents | `status_report.create` + `status_report.add_update` per update
| maintenances | `maintenance.create`
| subscribers | `page_subscriber.create`
Skipped resources don't emit (their original create audit already
exists); failed resources don't emit (nothing was written); link-table
rows (statusReportsToPageComponents etc.) don't emit (edges, not
entities). Metadata always carries `source: "import"` + `provider:
<name>` + `sourceId: <provider-id>` so the audit trail traces back to
the source system.
The rollup `import.run` audit still fires at the end — the per-resource
rows give forensic granularity, the run-level row gives "this bulk
operation happened" without scanning the full summary blob.
For the change, phase writers now take a shared `PhaseContext = { ctx,
tx, provider }` instead of `(db, workspaceId, limits)` — the orchestrator
builds one `PhaseContext` per run and threads it through, giving each
writer access to `ctx.actor` for audit attribution. `statusReportUpdate`
writes now use `.returning({ id })` so the per-update audit can
attribute the right row.
- **`run.ts:130`** — phases after `page` kept their provider-assigned
status when `targetPageId` was falsy but the user option wasn't
`false`. Replaced the narrow `else if (option === false)` branches
with a plain `else → phase.status = "skipped"`, matching what
`subscribers` already did.
- **`run.ts:147`** — when the `components` phase hit `remaining <= 0`,
the phase was marked `"failed"` but individual resource statuses were
left stale with no error string. Each resource is now marked
`"skipped"` with `"Skipped: component limit reached (N)"`, matching
`writeMonitorsPhase`. Phase-level status becomes `"skipped"` too
(was `"failed"` — failed implied a writer error, this is really a
plan-limit pre-check).
- **`provider.ts`** — both `createProvider` and `buildProviderConfig`
had a `default:` that silently ran the Statuspage adapter for any
unknown provider name, which would mask a typo by handing a non-
Statuspage api key to the wrong adapter. Replaced with exhaustive
`case "statuspage"` + `never`-typed default throw.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(services): rename rpc/services → rpc/handlers (PR 10/N) (#2112)
The symbolic deliverable from the plan's "close the loop" PR. Renames
`apps/server/src/routes/rpc/services/` → `apps/server/src/routes/rpc/handlers/`
so the distinction between "the services layer" (owns business logic,
lives in `packages/services`) and "Connect transport handlers" (thin
proto → service → proto wrappers) is permanent and visible in the path.
Keeping the old name invites the next developer to "just add one small
thing" to a file under a `services/` folder months later; the rename
makes the layering explicit.
- `git mv` of the six domain subdirectories + their tests
(health / maintenance / monitor / notification / status-page /
status-report).
- `router.ts` import paths updated from `./services/*` to `./handlers/*`.
- Biome `overrides.include` paths updated to the new location.
- Added `apps/server/src/routes/rpc/handlers/health/**` to the scope —
the health handler has no db usage today; including it locks in that
invariant.
Rather than pretending the full "close the loop" deliverable is possible
today, the biome.jsonc comment now enumerates exactly what remains
unmigrated:
- `packages/api/src/router/statusPage.ts` — public viewer endpoints
under `publicProcedure`, no authed `ServiceContext`.
- `packages/api/src/router/{member,integration,monitorTag,
pageSubscriber,privateLocation,checker,feedback,stripe,tinybird,
email}.ts` — small domains not yet lifted.
- `apps/server/src/routes/rpc/handlers/monitor/**` — 6 jobType-specific
methods still on db.
- `apps/server/src/routes/rpc/handlers/status-page/**` — page CRUD is
migrated (PR 7), but components / groups / subscribers / viewer (13
methods) still import db, so the whole file stays out of scope.
- `apps/server/src/routes/v1/**` — the public HTTP API surface.
- `apps/server/src/routes/slack/**` except `interactions.ts` — tools,
handler, oauth, workspace-resolver still on db.
- `apps/server/src/routes/public/**` — public-facing HTTP routes.
Each of the above is its own PR-sized migration. The final consolidation
(broadening to `router/**` + dropping `@openstatus/db` from
`packages/api` and `apps/server`) is conditional on all of them
landing first.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/import): use ctx workspaceId for page insert
`writePagePhase` was inserting with `data.workspaceId` — the value the
provider package round-tripped into resource data. Every other phase
writer (monitor / components / subscriber) already reads `workspaceId`
from `ctx.workspace.id`; this lines the page insert up with that
pattern. Defends against the (unlikely) case where a provider mapper
serialises the wrong workspace id into its output, since `ctx` is the
authoritative source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address Claude review findings on #2110
Six findings from Claude's review pass — five code/doc fixes, one
documentation-only note.
**P2 — `acceptInvitation` derives userId from `ctx.actor`.**
Was taking it from input: the email scoped *which* invitation could
be accepted, but not *who* the membership was inserted for. A caller
with the right token+email could insert a membership under an
arbitrary user id. Removed `userId` from `AcceptInvitationInput`;
derived from `tryGetActorUserId(ctx.actor)`, throws
`UnauthorizedError` for non-user actors. Mirrors the same pattern
applied to `createApiKey.createdById` in the Cubic pass. Router and
test updated accordingly.
**P2 — `getWorkspace` throws `NotFoundError` explicitly.**
`findFirst` + `selectWorkspaceSchema.parse(undefined)` was throwing
`ZodError` (→ `BAD_REQUEST`) instead of the `NotFoundError` shape
every other service uses. Unreachable in practice (ctx.workspace is
resolved upstream) but the error shape was the only outlier;
consistency matters for callers pattern-matching on error codes.
**P3 — `listApiKeys` filters null `createdById` before the IN query.**
The new `createApiKey` path enforces a non-null creator, but legacy
rows may have null. SQL's `x IN (NULL)` is `UNKNOWN` — technically
safe — but drizzle types model the array as `number[]`. Filtering
upfront keeps the types honest and sidesteps any future surprise.
**P3 — `deleteInvitation` guards `acceptedAt IS NULL`.**
The WHERE previously allowed hard-deleting *accepted* invitations,
wiping the "user was invited on X" breadcrumb. Added the
`isNull(acceptedAt)` guard + doc comment explaining the audit-trail
preservation intent.
**Doc-only — `deleteAccount` orphan comment.**
Non-owner memberships are removed, but owner memberships + owned
workspaces survive. Matches legacy behavior. Added a scope-note
docblock flagging that workspace cleanup is explicitly out of scope
(belongs to a future admin / scheduled job).
**Doc-only — `createInvitation` role comment.**
The invite insert lets `role` fall through to the schema default
(`member`). Matches legacy (which also only picked `email`).
Comment added so the absence reads as deliberate rather than
overlooked.
Minor — the concurrent-accept race test is covered by the conditional
UPDATE + `ConflictError` path from the earlier P1 fix; mocking it
reliably against SQLite is noisy and not worth the test complexity.
Documented in the related code comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address Claude re-review findings on #2110
Four issues surfaced after the first round of fixes on this PR:
**P2 — `listApiKeys` crashes on all-legacy keys.**
After the null filter added in the previous commit, workspaces whose
keys all pre-date the services migration (every `createdById` null)
end up with `creatorIds === []`. Drizzle throws "At least one value
must be provided" on an empty `inArray`, taking the whole endpoint
down. Added an early return that maps `createdBy: undefined` when
there are no non-null creator ids to look up.
**P2 — `getWorkspaceWithUsage` ZodError on missing row.**
Same `findFirst` + `selectWorkspaceSchema.parse(result)` pattern as
`getWorkspace`, but without the `NotFoundError` guard that got added
in the earlier pass. Added the guard. Also cleaned up the usage
block — no longer needs optional chaining once the narrowing fires.
**P2 — `deleteAccount` took `userId` from input.**
Completing the `createApiKey` / `acceptInvitation` pattern: account
deletion must target `ctx.actor`, never an arbitrary id. Dropped
`userId` from `DeleteAccountInput` (now an empty forward-compat
shape), derived inside the service via `tryGetActorUserId`, throws
`UnauthorizedError` for non-user actors. Router updated to stop
passing it.
**P3 — `createInvitation` dev-token log could leak in tests.**
Tightened the comment around the `process.env.NODE_ENV === "development"`
guard to flag that strict equality is load-bearing — bun:test sets
`NODE_ENV=test` and CI leaves it undefined, both of which correctly
skip the log. No behavior change, just a clearer contract so the
next reader doesn't loosen it.
Cubic's two findings on this review pass point at `packages/api/src/
router/import.ts` and `packages/services/src/import/limits.ts` — both
live in the next PR up the stack (#2111 / feat/services-import) and
will be addressed there.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address latest Cubic pass on #2110
Four findings from the third Cubic review (now that #2111's import
domain is included in the #2110 diff via the stack):
**P2 — biome.jsonc notification handler scope.**
Only `notification/index.ts` was in the `noRestrictedImports` override.
Sibling files (`errors.ts`, `test-providers.ts`) were outside the
migration guard, so new db imports could land in them without the
lint failing. Broadened to `notification/**` and moved the two files
that *legitimately* still read db (`limits.ts` querying workspace
quotas, `converters.ts` needing db enum shapes for proto round-trip)
into the `ignore` list. Future siblings are enforced by default
rather than silently slipping through.
**P2 — `clampPeriodicity` unknown values returned too fast.**
`PERIODICITY_ORDER.indexOf("unknown") === -1` → `Math.max(-1, 0) === 0`
→ walk started at `"30s"` (the fastest tier). Could return an
interval faster than requested, violating the
"never-faster-than-requested" invariant. Short-circuits now to the
slowest allowed tier when the requested value isn't a known
periodicity. Added unit tests covering the unknown-value + empty-
allowed fallback paths.
**P2 — component/monitor limit warnings counted total resources, not
quota-consuming inserts.**
If the import contained 4 components and 3 already existed (would be
skipped as duplicates), the warning claimed `"Only X of 4 can be
imported"` — but actually zero quota would be consumed by the 3
skips, so the real new-creation count might fit entirely. Reworded
to `"Only N new components may be created … some of the M in the
import may already exist and be skipped"`. Same treatment for the
monitors warning. Preview stays DB-light (no per-resource existence
checks); the warning now honestly conveys worst-case without
misleading users about what will actually happen. Test assertions
updated to match the new wording with substring matches that aren't
tied to the exact fraction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/page): address Claude review on #2109
Six items from Claude's review, going with the calls I leaned toward
in the question-back:
**P2 — tRPC `updateCustomDomain` wasteful `getPage` read.**
Was calling `getPage(id)` (fires 3 batched relation queries:
maintenances + components + groups) just to grab `customDomain`
before the Vercel add/remove calls. Added a narrow
`getPageCustomDomain` service helper — single indexed lookup,
workspace-scoped, returns the string directly. Router swapped over.
Service-layer authority preserved; no db reads leak into the router.
**P2 — Connect `updateStatusPage` slug-race code drift.**
Handler pre-checks slug to surface `slugAlreadyExistsError`
(`Code.AlreadyExists`). The `updatePageGeneral` service call
re-validates via `assertSlugAvailable` → `ConflictError` →
`Code.InvalidArgument` in the race where two callers both clear the
pre-check. Wrapped the call in `try/catch (ConflictError)` and
rethrow as `slugAlreadyExistsError(req.slug)` so gRPC clients keying
on the code get a consistent `AlreadyExists` whether they lose at
the pre-check or at the inner tx.
**P2 — Connect `createStatusPage` / `updateStatusPage` customDomain
without Vercel sync.** Pre-existing behaviour (the direct-db handler
had the same gap). Added a top-of-impl comment so it doesn't go
unnoticed — the fix is a shared transport-layer helper the Connect
handlers can reuse, out of scope for this migration PR to keep the
behavioural blast radius small for external API consumers.
**P3 — double cast `row as unknown as Page` in `create.ts`.**
The drizzle insert-returning type and the `Page` type diverge on
`authEmailDomains` / `allowedIpRanges` (raw comma-joined string vs
parsed `string[]`). Replaced the double casts with
`selectPageSchema.parse(row)` which normalises the row into the shape
callers expect. Cast-drift is now impossible to introduce silently.
**P3 — `void ConflictError;` workaround.**
Import was unused in `create.ts`; the `void` line was silencing the
unused-import warning rather than fixing the cause. Removed both.
**P3 — deprecated `passwordProtected` column.**
Added a doc block on `updatePagePasswordProtection` flagging that
the deprecated boolean column is intentionally not written here (the
v1 REST read path derives it from `accessType` via
`normalizePasswordProtected`). Prevents a future reader from
mistaking the omission for an oversight and writing two sources of
truth for the same signal.
Test coverage for the 5 untested update services
(`updatePagePasswordProtection`, `updatePageCustomDomain`,
`updatePageAppearance`, `updatePageLinks`, `updatePageConfiguration`)
deferred to a follow-up per Claude's "not blocking" marker — the
failing-edge behaviour is the critical bit, and
`updatePagePasswordProtection` already has indirect coverage through
the Connect handler tests on this branch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address Claude review on #2108
Four items from Claude's review of the Connect notification handler
backfill:
**P3 — `protoDataToServiceInput` swallowed parse failures.**
`try { JSON.parse } catch { return {} }` was hiding any malformed
output from `protoDataToDb` (which would be a programmer error, not
user-input) behind a generic empty-object fallback. The downstream
`validateNotificationData` then failed with a far less specific
error. Let the throw propagate — `toConnectError` maps it to
`Code.Internal`, which is the signal we want for "the helper itself
misbehaved."
**P3 — `createNotification` response approximated the monitor IDs.**
Was echoing `Array.from(new Set(req.monitorIds))` on the happy path
(correct, since the service validates + throws on invalid) but the
approximation diverged from `updateNotification`'s re-fetch pattern.
Now re-fetches via `getNotification` after create so the response
reflects what's actually in the DB — one extra IN query per create,
eliminates the approximation entirely, makes both handlers
structurally identical.
**P3 — `sendTestNotification` bypassed `toConnectError`.**
Only handler in the impl without a `try { … } catch { toConnectError
}` wrap, so any thrown `ServiceError` / `ZodError` from
`test-providers.ts` fell through to the interceptor's generic catch
and surfaced with a less precise gRPC status. Wrapped for symmetry.
**P3 — `JSON.parse(existing.data)` null-unsafe.**
Drizzle infers `notification.data` as `string | null` (the column has
`default("{}")` but no `.notNull()`). A legacy row with `NULL` in the
column would crash `updateNotification` with `SyntaxError` during
the partial-update read-modify-write. Added `?? "{}"` fallback and a
comment pointing at the schema.
Cubic's single finding from the earlier pass (dedupe of
`req.monitorIds` in the create response) was already applied in
`b69ad13` and has now been superseded by the re-fetch above.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address latest Cubic pass on #2108
Five findings from Cubic's second review cycle on this PR, all on
files that entered this branch via the #2109 (status-page) and #2111
(import) squash-merges stacked on top. Fixing here so the cumulative
state reaching main is clean.
**P1 — `page/create.ts` double-encoded JSON configuration.**
`page.configuration` is a drizzle `text("…", { mode: "json" })`
column — drizzle serialises objects automatically. Calling
`JSON.stringify(configuration)` first stored a raw JSON string in
the column, breaking any downstream read that expects an object
(e.g. the appearance merge at `update.ts:185`). Dropped the wrap;
drizzle handles it.
**P2 — `page/schemas.ts` slug + customDomain validation weaker than
insert schema.**
`NewPageInput.slug`, `GetSlugAvailableInput.slug`, and
`UpdatePageGeneralInput.slug` were `z.string().toLowerCase()` — no
regex, no min-length. `UpdatePageCustomDomainInput.customDomain`
was `z.string().toLowerCase()` — no format check. Meant the service
would accept malformed slugs / URLs that `createPage` would then
reject via `insertPageSchema`, or — worse — that `getSlugAvailable`
would confidently return "available" for garbage. Exported the
canonical `slugSchema` + `customDomainSchema` from
`@openstatus/db/src/schema/pages/validation` and reused them across
all four service inputs; db validation is now the single source of
truth for page slug/domain shape.
**P2 — `api/router/import.ts` nullish → optional contract narrowing.**
The service's `PreviewImportInput`/`RunImportInput` used `.optional()`
for the three provider page-id fields, which dropped the `null`
acceptance the legacy router had via `.nullish()`. Existing clients
sending `null` would have started hitting `Invalid input` errors
after the import migration landed. Added a `nullishString` transform
in the service schema that accepts `string | null | undefined` and
normalises to `string | undefined` before it reaches
`buildProviderConfig` — callers keep the broader contract, service
internals stay ignorant of `null`.
**P2 — `page/update.ts` empty array stored "" not null.**
`authEmailDomains?.join(",") ?? null` coerces `null`/`undefined` to
`null`, but `[].join(",")` returns `""` (empty string) which `??`
treats as a value. Callers sending `authEmailDomains: []` to clear
the column were persisting the empty string instead of nulling it —
misleading "present but blank" state. Switched to `|| null` on both
array-join outputs (`authEmailDomains` + `allowedIpRanges`) so the
three clearing inputs — `undefined`, `null`, `[]` — all land on DB
`NULL` while real non-empty joins pass through unchanged.
Test fixtures already use slugs ≥ 3 chars that match the regex, so
the tightened validation doesn't break any existing assertions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/page-component): address Cubic + Claude review on #2107
Three fixes + one test, addressing both the original Cubic finding
and Claude's re-review pass.
**P2 — Discriminated union for `componentInput`.**
The flat `z.object` with `type: z.enum(["monitor", "static"])` and
optional `monitorId` let callers submit a "monitor" component with
no monitor id, or a "static" one with a monitor id attached. The DB
catches it with a `CHECK` constraint, but that surfaces as an opaque
SQLite CHECK failure instead of a clean `ZodError` at the service
boundary. Replaced with a `z.discriminatedUnion("type", [...])` that
requires `monitorId` on the "monitor" arm and omits it on the
"static" arm.
Fallout in `update-order.ts`: `c.monitorId` no longer exists on the
"static" arm after narrowing, so the spreads now use
`monitorId: c.type === "monitor" ? c.monitorId : null`. The defensive
`&& c.monitorId` guards on the already-narrowed monitor branches are
gone (TypeScript enforces the invariant the DB was catching late).
**P2 — Sequential group insert instead of bulk `.returning()`.**
The bulk insert relied on drizzle/SQLite returning rows in the same
order they were inserted, so `newGroups[i]` could line up with
`input.groups[i]` when mapping components to their groups. True on
Turso today, but an implicit coupling — any driver change, batch
split, or upstream sort could silently reorder rows and land
components in the wrong group with no error signal. Switched to a
loop that captures each group id before moving on; the set size is
bounded by the status-page component-group plan cap so the extra
round trips are a rounding error.
**Nit — removed dead `hasStaticComponentsInInput` guard.**
Both the "input has static components but none carry ids" and "input
has no static components at all" branches collapsed to the same
"drop all existing static components" action, so the outer
`hasStaticComponentsInInput` conditional was doing no work. Dropped
the variable and the nested branch.
**Test — upsert idempotency.**
The `onConflictDoUpdate` on `(pageId, monitorId)` was the riskiest
untested path — a regression would silently insert duplicate rows on
every re-invocation. Added a test that calls
`updatePageComponentOrder` twice on the same page with the same
`monitorId`, then asserts there's exactly one matching row and the
second call's values won.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address latest Cubic pass on #2107 + unblock build
Eight Cubic findings from the second review plus one dashboard build
break from my earlier discriminated-union change.
**Build — router shape diverged from service discriminated union.**
`packages/api/src/router/pageComponent.ts` kept its own flat
`z.object({...})` input schema with `type: z.enum(["monitor",
"static"])` and `monitorId: z.number().nullish()`. After the service
switched to `z.discriminatedUnion("type", [...])`, TS couldn't
reconcile the two — dashboard build failed. Replaced the local
schema with the service's exported `UpdatePageComponentOrderInput`
so both layers share the canonical shape.
**P1 — page router: validate customDomain before Vercel call.**
The router input was `z.string().toLowerCase()` (no format check) and
the service's `customDomainSchema` only fired inside
`updatePageCustomDomain`, *after* the Vercel add/remove mutations.
A malformed domain could be added to Vercel, then rejected by the
service, leaving Vercel/db state drifted. Switched the router input
to the service's `UpdatePageCustomDomainInput` so format validation
runs at tRPC input parsing, before any Vercel call.
**P1 — `listApiKeys` leaked `hashedToken`.**
`SELECT *` returned every column including the bcrypt hash of each
key's one-time token, which has no business appearing in a list
response. Replaced with an explicit column select that omits
`hashedToken`. New `PublicApiKey` type (`Omit<ApiKey,
"hashedToken">`) is the return shape; exported from the barrel.
**P2 — `acceptInvitation` eager workspace load + second fetch.**
The initial `findFirst` already loaded the workspace via `with: {
workspace: true }`, but the return value re-fetched it by id. Use
the joined value directly — one round-trip instead of two, and
eliminates the read-skew window where a just-renamed workspace
could appear with a different name in each fetch.
**P2 — `import.run` audit entityId 0.**
`entityId: targetPageId ?? 0` wrote a ghost `page 0` reference to
the audit trail when the page phase failed before producing an id.
Entity attribution now falls back to the workspace (`entityType:
"workspace"`, `entityId: ctx.workspace.id`) when no target page is
in play — real rollback signal, no phantom foreign key.
**P2 — `page-components` limit scoped per-page, not workspace.**
`page-components` is a workspace-wide cap (see
`page-component/update-order.ts` — counts every component across
every page). The import preview and run's component check were
scoping the existing count to `targetPageId`, which understated
pressure and would let imports push past the cap at write time.
Both sites now count workspace-wide.
**P2 — `writeIncidentsPhase` lacked idempotency.**
Every other phase writer checks for an existing row before
inserting (page by slug, monitor by url, component by name,
subscriber by email); `writeIncidentsPhase` inserted
unconditionally. A re-run would duplicate status reports on every
pass. Added an existence check by `(title, pageId, workspaceId)`
matching the convention.
**P2 — `writeMaintenancesPhase` lacked idempotency.**
Same pattern. Added a check by `(title, pageId, from, to,
workspaceId)` — the `from/to` pair is load-bearing because
maintenance titles recur ("DB upgrade") across unrelated windows.
**P2 — `writeComponentsPhase` silent monitor→static fallback…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #2100. First domain migration onto the service layer. Moves every status-report write and read onto
packages/services/src/status-report/and makes tRPC / Connect / Slack thin adapters. Freezes theServiceContextshape and the single-object{ ctx, input }argument convention for every subsequent domain PR.What lands here
Services (
packages/services/src/status-report/)createStatusReport,addStatusReportUpdate,resolveStatusReport(convenience wrapper over add-update with status=resolved),updateStatusReport(metadata + associations),updateStatusReportUpdate(single-update edit),deleteStatusReport,deleteStatusReportUpdate,listStatusReports,getStatusReport,notifyStatusReport.withTransaction, emitemitAuditbefore returning, and throw the typedServiceErrorhierarchy (NotFound / Forbidden / Conflict / …)..parse()runs at every entry as defence-in-depth.validatePageComponentIds,updatePageComponentAssociations,getReportInWorkspace,getReportUpdateInWorkspace,getUpdatesForReport,getPageComponentIdsForReport.notifyStatusReport({ ctx, input })wrapsdispatchStatusReportUpdatewith workspace-scope +status-subscribersplan check, and is invoked as a second awaited call after the mutation — preserving the dashboard's two-call pattern (mandatory under the Edge runtime).Surfaces — external contracts preserved
packages/api/src/router/statusReport.ts): every procedure now calls a service.createstill returns{ ...initialUpdate, notifySubscribers }.listandgetstill return the enriched{ updates, pageComponents, page }shape the dashboard consumes.emailRouter.sendStatusReportbecomes a thin wrapper overnotifyStatusReport.apps/server/src/routes/rpc/services/status-report/): handler is now proto → parse → service call → convert-back. External Connect API unchanged. Helpers previously exported from this folder (consumed by Slack) are gone; callers go through services.apps/server/src/routes/slack/interactions.ts): the four status-report branches (create / add-update / update / resolve) go through services. The maintenance branch still writes to db directly — it migrates in PR 2.Adapters
packages/api/src/service-adapter.ts—toServiceCtx(ctx)+toTRPCError(err)(ServiceError → TRPCError with matching codes; ZodError maps to BAD_REQUEST).apps/server/src/routes/rpc/adapter.ts—toServiceCtx(rpcCtx)+toConnectError(err). Synthesisesactor.keyId = "ws:<workspaceId>"until the auth interceptor captures the real API key id (open question in the plan).apps/server/src/routes/slack/service-adapter.ts— refetches the full workspace for the pending-action'sworkspaceId, builds a Slack actor context, and rendersServiceError→ human-readable Slack messages.Enforcement
noRestrictedImportsscoped topackages/api/src/router/statusReport.ts+apps/server/src/routes/rpc/services/status-report/**. Bans@openstatus/db,@openstatus/db/src/schema,drizzle-orm, and the threeplan/*subpaths. Tests are ignored (setup/cleanup legitimately touch db).@openstatus/services/status-report.packages/api/apps/servernow depend on@openstatus/services.Tests
packages/services/src/status-report/__tests__/status-report.test.ts— happy paths, workspace isolation, cascade deletes, cross-workspaceForbiddenError/NotFoundError, the slack-actor audit branch, and enriched-relations forlist/get.Deliberate behaviour changes (small)
limit). Callers that need clamping can catch.updateStatusReportwithpageComponentIds: []now clears associations (same as old Connect'supdatePageComponentIds: true). Passingundefinedleaves them untouched.Open questions deferred to later PRs
actor.keyId = "ws:<workspaceId>". Fix comes with a small interceptor change in a later PR.Test plan
servicesintegration tests (seeded turso) alongside existing Connect + tRPC suitesstatusReport.create→emailRouter.sendStatusReporttwo-call path still works (no frontend change)CreateStatusReport/AddStatusReportUpdatepreservenotifysemantics (single RPC call externally, two service calls internally)@openstatus/dbsneaks into a scoped fileStack
@openstatus/dbdeps, renamerpc/services/→rpc/handlers/)🤖 Generated with Claude Code