feat: page-subscriptions by components #1848
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mxkaske
changed the title
mxkaske
requested review from
Copilot and
thibaultleouay
and removed request for
thibaultleouay
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive multi-channel subscription system with component-level granularity, migrating from the simple page_subscriber table to a new page_subscription architecture. It introduces a new @openstatus/subscriptions package that centralizes subscription logic, adds support for webhook channels alongside email, and enables users to subscribe to specific page components rather than entire pages.
Changes:
- Created new
@openstatus/subscriptionspackage with service layer, dispatcher, and channel abstractions for email and webhook notifications - Added database schema with
page_subscriptionand junction table for component associations, including migration SQL that preserves existing subscriber tokens - Refactored API endpoints from
statusPagerouter to newpageSubscriptionrouter with improved public and protected procedures
Reviewed changes
Copilot reviewed 48 out of 51 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/subscriptions/* | New package implementing service layer, dispatcher, channels (email/webhook), and comprehensive tests |
| packages/db/src/schema/page_subscriptions/* | New database schema with multi-channel support, component junction table, CHECK constraints, and validation schemas |
| packages/db/drizzle/0056_clammy_tombstone.sql | Migration SQL creating new tables and migrating existing page_subscriber data with token preservation |
| packages/api/src/router/pageSubscription.ts | New tRPC router with public endpoints for upsert/verify/unsubscribe and protected endpoints for list/delete |
| packages/api/src/router/statusPage.ts | Removed deprecated subscription endpoints that were migrated to new router |
| packages/api/src/router/email/index.ts | Updated to use new dispatcher and added sendPageSubscriptionVerification endpoint |
| packages/proto/* | Added PageSubscription proto message while keeping PageSubscriber for backward compatibility |
| packages/emails/* | Renamed page subscription email template and client method for clarity |
| apps/server/src/routes/rpc/services/* | Updated gRPC services to use new dispatcher functions instead of direct email sending |
| apps/dashboard/src/components/data-table/subscribers/* | Updated UI to show subscription scope (entire page vs components) and use new API |
| apps/status-page/src/components/nav/header.tsx | Updated subscription flow to use new upsert and verification endpoints |
| apps/status-page/src/app//(public)/ | Updated verify and unsubscribe pages to use new pageSubscription router |
| pnpm-lock.yaml | Updated with new dependencies for subscriptions package |
| apps/server/Dockerfile, dofigen.yml | Added subscriptions package to Docker build process |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pageSlug: pageData.slug, | ||
| customDomain: pageData.customDomain, | ||
| workspaceId: existing.workspaceId, | ||
| channelType: "email", |
There was a problem hiding this comment.
The channelType is hardcoded as "email" in the return object. This should use a const from the function signature or the existing.channelType to maintain consistency and type safety. Using the actual channel type from the database ensures that if the schema evolves or if there are any edge cases, the returned data accurately reflects what's stored.
| // Validate RESEND_API_KEY exists | ||
| if (!process.env.RESEND_API_KEY) { | ||
| throw new Error( | ||
| "RESEND_API_KEY environment variable is required for email notifications", | ||
| ); | ||
| } |
There was a problem hiding this comment.
The environment variable validation at the module level will cause the module to fail to load if RESEND_API_KEY is not set. This could break the application during development or testing where email functionality might not be needed. Consider moving this validation into the functions that actually use it, or making it conditional based on NODE_ENV.
| token: pageSubscription.token, | ||
| verifiedAt: pageSubscription.verifiedAt, | ||
| // Aggregate component IDs as comma-separated string (NULL if empty) | ||
| componentIds: sql<string>`GROUP_CONCAT(${pageSubscriptionToPageComponent.pageComponentId})`, |
There was a problem hiding this comment.
Using GROUP_CONCAT with comma separator could potentially break if component IDs themselves contain commas (though unlikely with integer IDs). More critically, GROUP_CONCAT in SQLite has a default limit of 1MB for the result string. While this is unlikely to be hit with typical component counts, consider documenting this limitation or using a more robust aggregation approach for production systems with many components.
| email: z.email(), | ||
| pageId: z.number().int().positive(), | ||
| componentIds: z.array(z.number().int().positive()).optional(), | ||
| groupId: z.number().int().positive().optional(), |
There was a problem hiding this comment.
The upsert endpoint accepts a groupId parameter in the input schema but never uses it. This could be confusing for API consumers and suggests incomplete implementation. Either remove the groupId parameter from the input schema or implement group-based subscription functionality.
| groupId: z.number().int().positive().optional(), |
| upsert: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| email: z.email(), | ||
| pageId: z.number().int().positive(), | ||
| componentIds: z.array(z.number().int().positive()).optional(), | ||
| groupId: z.number().int().positive().optional(), | ||
| }), | ||
| ) | ||
| .mutation(async (opts) => { | ||
| try { | ||
| const subscription = await upsertEmailSubscription({ | ||
| email: opts.input.email, | ||
| pageId: opts.input.pageId, | ||
| componentIds: opts.input.componentIds, | ||
| }); | ||
|
|
||
| return { | ||
| success: true, | ||
| subscription: { | ||
| id: subscription.id, | ||
| token: subscription.token, | ||
| verifiedAt: subscription.verifiedAt, | ||
| componentIds: subscription.componentIds, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: error.message, | ||
| }); | ||
| } | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Failed to create subscription", | ||
| }); | ||
| } | ||
| }), |
There was a problem hiding this comment.
The upsert endpoint is PUBLIC and lacks rate limiting or workspace limits validation. A malicious actor could spam subscription creation requests, potentially exhausting database resources or flooding email systems. Consider adding rate limiting, CAPTCHA, or at minimum checking workspace limits before allowing subscription creation.
| <span className="font-semibold"> | ||
| {subscriberQuery.data.maskedEmail} | ||
| </span>{" "} | ||
| <span className="font-semibold">{subscriberQuery.data.email}</span>{" "} |
There was a problem hiding this comment.
The unsubscribe page now displays the full email address instead of a masked version (e.g., "j***@example.com"). While the old implementation had email masking for privacy, the new getByToken endpoint returns the full email. This is a privacy regression that could expose email addresses to shoulder-surfing or screenshot leaks. Consider either returning a masked email from the API or masking it client-side for display purposes.
| const { report: newReport, pageId } = await db.transaction(async (tx) => { | ||
| // Validate page component IDs inside transaction to prevent TOCTOU race condition | ||
| const validatedComponents = await validatePageComponentIds( | ||
| req.pageComponentIds, | ||
| workspaceId, | ||
| tx, | ||
| ); | ||
|
|
||
| // Validate that provided pageId matches the components' page | ||
| const derivedPageId = validatedComponents.pageId; | ||
| const providedPageId = req.pageId?.trim(); | ||
| if ( | ||
| derivedPageId !== null && | ||
| providedPageId && | ||
| providedPageId !== "" && | ||
| Number(providedPageId) !== derivedPageId | ||
| ) { | ||
| throw pageIdComponentMismatchError( | ||
| providedPageId, | ||
| String(derivedPageId), | ||
| ); | ||
| } | ||
|
|
||
| // Use the derived pageId from components (null if no components) | ||
| const pageId = Number(providedPageId); | ||
|
|
||
| // Create the status report | ||
| const report = await tx | ||
| .insert(statusReport) | ||
| .values({ | ||
| workspaceId, | ||
| pageId, | ||
| title: req.title, | ||
| status: protoStatusToDb(req.status), | ||
| }) | ||
| .returning() | ||
| .get(); | ||
|
|
||
| if (!report) { | ||
| throw statusReportCreateFailedError(); | ||
| } | ||
|
|
||
| // Create page component associations | ||
| await updatePageComponentAssociations( | ||
| report.id, | ||
| validatedComponents.componentIds, | ||
| tx, | ||
| ); | ||
|
|
||
| // Create the initial update | ||
| const newUpdate = await tx | ||
| .insert(statusReportUpdate) | ||
| .values({ | ||
| statusReportId: report.id, | ||
| status: protoStatusToDb(req.status), | ||
| date, | ||
| message: req.message, | ||
| }) | ||
| .returning() | ||
| .get(); | ||
|
|
||
| if (!newUpdate) { | ||
| throw statusReportCreateFailedError(); | ||
| } | ||
|
|
||
| return { report, pageId }; | ||
| return { report, pageId, updateId: newUpdate.id }; | ||
| }); | ||
|
|
||
| // Send notifications if requested (outside transaction) | ||
| if (req.notify) { | ||
| await sendStatusReportNotification({ | ||
| statusReportId: newReport.id, | ||
| pageId, | ||
| reportTitle: newReport.title, | ||
| status: protoStatusToDb(req.status), | ||
| message: req.message, | ||
| date, | ||
| limits: rpcCtx.workspace.limits, | ||
| }); | ||
| if (req.notify && rpcCtx.workspace.limits["status-subscribers"]) { | ||
| await dispatchStatusReportUpdate(newReport.id); |
There was a problem hiding this comment.
The transaction returns an updateId on line 243, but the destructuring on line 178 only captures report and pageId, ignoring the updateId. Then line 248 calls dispatchStatusReportUpdate with newReport.id (the status report ID) instead of the updateId. The dispatcher expects a statusReportUpdateId, not a statusReportId. This should destructure as { report: newReport, pageId, updateId } and call dispatchStatusReportUpdate(updateId).
| sendPageSubscriptionVerification: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| id: z.number().int().positive(), | ||
| }), | ||
| ) | ||
| .mutation(async (opts) => { | ||
| const subscription = await opts.ctx.db.query.pageSubscription.findFirst({ | ||
| where: eq(pageSubscription.id, opts.input.id), | ||
| with: { | ||
| page: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (!subscription) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Subscription not found", | ||
| }); | ||
| } | ||
|
|
||
| if (!subscription.email) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "No email associated with this subscription", | ||
| }); | ||
| } | ||
|
|
||
| if (subscription.verifiedAt) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Subscription already verified", | ||
| }); | ||
| } | ||
|
|
||
| // Build verification URL | ||
| const verifyUrl = subscription.page.customDomain | ||
| ? `https://${subscription.page.customDomain}/verify/${subscription.token}` | ||
| : `https://${subscription.page.slug}.openstatus.dev/verify/${subscription.token}`; | ||
|
|
||
| // Get email channel and send verification | ||
| const channel = getChannel("email"); | ||
|
|
||
| if (!channel) { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Email channel not found", | ||
| }); | ||
| } | ||
|
|
||
| await channel.sendVerification?.( | ||
| { | ||
| id: subscription.id, | ||
| pageId: subscription.pageId, | ||
| pageName: subscription.page.title, | ||
| pageSlug: subscription.page.slug, | ||
| customDomain: subscription.page.customDomain, | ||
| workspaceId: subscription.workspaceId, | ||
| componentIds: [], // Not needed for verification email | ||
| channelType: "email", | ||
| email: subscription.email, | ||
| token: subscription.token, | ||
| verifiedAt: subscription.verifiedAt ?? undefined, | ||
| }, | ||
| verifyUrl, | ||
| ); | ||
|
|
||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
The sendPageSubscriptionVerification endpoint is marked as PUBLIC but lacks workspace limits validation. A malicious actor could spam verification emails by repeatedly calling this endpoint with valid subscription IDs. Consider adding rate limiting or making this a protected endpoint, or at minimum checking workspace limits before sending emails.
| if (statusReport.notifySubscribers) { | ||
| await sendStatusReportUpdateMutation.mutateAsync({ | ||
| id: statusReport.id, | ||
| id: statusReport.statusReportId, |
There was a problem hiding this comment.
The code sends a notification using statusReport.statusReportId, but this field name seems suspicious. Based on the API endpoint expecting just 'id', this should likely be statusReport.id or the first statusReportUpdateId. Verify that the create mutation returns the correct field name, as this could cause notifications to fail silently or send for the wrong update.
| id: statusReport.statusReportId, | |
| id: statusReport.id, |
| "dependencies": { | ||
| "@openstatus/db": "workspace:*", | ||
| "@openstatus/emails": "workspace:*", | ||
| "zod": "^3.24.1" |
There was a problem hiding this comment.
The zod dependency version is ^3.24.1, but the pnpm-lock.yaml shows it resolves to version 3.25.76. Meanwhile, other packages in the monorepo use zod 4.1.13. This version inconsistency could lead to type compatibility issues or unexpected behavior when different parts of the codebase interact. Consider standardizing on a single zod version across the monorepo.
|
@claude review this and nit pick |
|
Claude finished @thibaultleouay's task in 2m 26s —— View job Code Review - Nitpick Mode ActivatedConducted a thorough, nitpicky code review of the page subscriptions refactor. Overall, this is a well-architected feature with good separation of concerns. Here are my findings organized by severity: 🔴 Critical Issues1. Incorrect parameter passed to dispatcher (
|
2 participants
Type of change
Description
A picture tells a thousand words (if any)
Before this PR
{Please add a screenshot here}
After this PR
{Please add a screenshot here}
Related Issue (optional)