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>
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