fix: clarify customer communication and delivery outcomes#563
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Unkey Deploy
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
25 issues found across 262 files
Confidence score: 2/5
packages/notifications/src/providers/slack.tsinbuildSlackBlockscan generate more than Slack’s 50-block limit, which may cause webhook rejection and noisy retry loops that drop/duplicate alert delivery — cap total blocks (or truncate/summarize metadata) before merging.apps/dashboard/app/(main)/billing/components/topup-card.tsxcurrently describes pricing as if all usage re-prices at the next tier, but billing is graduated; this can mislead customers on cost and trigger trust/support issues — update the copy to match marginal-tier behavior before release.apps/api/src/routes/webhooks/autumn.tsandapps/api/src/routes/public/flags.tsintroduce concrete behavior regressions: multi-org billing events can be acknowledged without handling whenentity_idis missing, and emptykeysnow returns zero evaluations instead of all flags — restore fallback/selection behavior and add regression tests for these edge cases before merging.apps/dashboard/lib/query-client.ts,apps/dashboard/app/(auth)/register/page.tsx, andpackages/email/src/emails/usage-email-utils.tseach weaken reliability/security posture (global mutation errors can be hidden by local handlers, auth callback params now take user-controlled query input, andformatResetDatecan throw on out-of-range finite timestamps) — re-enable global error surfacing, constrain callback URLs to trusted targets, and guard date bounds before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/dashboard/app/(main)/websites/[id]/errors/_components/error-summary-stats.tsx">
<violation number="1" location="apps/dashboard/app/(main)/websites/[id]/errors/_components/error-summary-stats.tsx:103">
P2: The title 'Sessions with errors' reads as a count, but the displayed value is still a percentage formatted from `errorSummary.errorRate`. Because neighboring cards ('Affected Sessions', 'Affected Users', 'Total Errors') all show absolute counts, users may misread this card as a session count rather than a rate. Consider a label that preserves the percentage semantics, such as 'Error Rate (sessions)' or '% of sessions with errors' to avoid dashboard misinterpretation during incident triage.</violation>
</file>
<file name="packages/email/src/emails/auth-email-expiry.ts">
<violation number="1" location="packages/email/src/emails/auth-email-expiry.ts:10">
P2: The expiry labels here are a second, independent copy of the durations used in `AUTH_EMAIL_EXPIRY_SECONDS`. Because the email package uses the labels in customer-facing templates while the auth package uses the seconds for real token TTLs, these two objects can silently drift apart when someone updates a token lifetime but forgets the matching label. This is especially risky given the PR focus on tightening customer communication accuracy. Consider deriving the labels from the seconds values (for example with a small human-duration formatter) or using a single keyed definition so the copy shown to users is always tied to the actual enforced expiry.</violation>
</file>
<file name="apps/dashboard/app/(main)/websites/[id]/errors/_components/error-table-columns.tsx">
<violation number="1" location="apps/dashboard/app/(main)/websites/[id]/errors/_components/error-table-columns.tsx:41">
P2: The pluralization branch uses `errorsPerUser === 1` on the raw float while the displayed value is rounded with `toFixed(1)`. Values like `0.96` or `1.04` display as `1.0` but still take the plural branch (`errors`). Match the grammar decision to the rounded display (e.g. compare `parseFloat(errorsPerUser.toFixed(1)) === 1` or check the formatted string) so the UI stays consistent.</violation>
</file>
<file name="packages/notifications/src/client.ts">
<violation number="1" location="packages/notifications/src/client.ts:98">
P2: The `channels` array is referenced directly across an async boundary (`Promise.allSettled`), which creates a race condition if the underlying array is mutated while sends are in-flight. If entries are added or removed before the `await` resolves, the post-await `channels.map(...)` loop can misalign, drop, or fabricate results. Prefer cloning `channels` at the point of dispatch (e.g., with a spread `[...channels]`) so the iteration snapshot is stable regardless of caller-side mutations.</violation>
</file>
<file name="apps/docs/content/docs/sdk/nuxt.mdx">
<violation number="1" location="apps/docs/content/docs/sdk/nuxt.mdx:156">
P2: The `trackPerformance` option was removed from the Nuxt module options docs, but `packages/nuxt/src/module.ts` still accepts it as a deprecated compatibility alias (mapped to `trackWebVitals`). Without any mention in the docs, users who previously configured `trackPerformance` have no migration guidance and may not realize the alias still works. Consider restoring the row with a deprecation note or adding a short migration callout so the docs stay in sync with the runtime schema.</violation>
</file>
<file name="apps/dashboard/lib/databunny-usage.ts">
<violation number="1" location="apps/dashboard/lib/databunny-usage.ts:1">
P2: These customer-facing constants live only in `apps/dashboard/lib/databunny-usage.ts`, while the same copy is hardcoded in `packages/email`, `apps/api`, `packages/rpc`, and other surfaces. Since the PR goal is cross-surface wording consistency, this creates drift risk: a future edit to the dashboard constants won't update emails or API responses. Consider moving them to `packages/shared` (or another shared copy source) so all surfaces consume the same strings.</violation>
</file>
<file name="apps/docs/content/docs/Integrations/gtm.mdx">
<violation number="1" location="apps/docs/content/docs/Integrations/gtm.mdx:10">
P2: The new TL;DR says page views are automatic and `data-track-screen-views` has been removed from all setup snippets, but the "Page View Tracking" trigger section (lines 169–176) is still present with no associated tag or explanation. This creates a documentation contradiction: readers may think they still need to configure a page-view trigger for Databuddy, and could end up attaching it to the main script tag (firing twice) or creating a redundant manual page-view tag. Consider removing this obsolete trigger section, or if it is meant for SPA/enhanced page views, clarify the purpose and show the tag that should use this trigger.</violation>
</file>
<file name="apps/dashboard/lib/autumn/customer-plan-name.ts">
<violation number="1" location="apps/dashboard/lib/autumn/customer-plan-name.ts:1">
P2: This introduces a local hardcoded plan-ID-to-name mapping (`CUSTOMER_PLAN_NAMES`) that duplicates the existing Autumn catalog already defined in `apps/dashboard/autumn.config.ts` (e.g., `scale` → "Enterprise"). Because unmapped IDs silently fall back to `fallbackName`, any drift between this overlay and the central catalog will degrade unnoticed. Consider deriving display names directly from the canonical `autumn.config.ts` plan objects or adding a cross-reference comment/breadcrumb so future edits to one side prompt updates to the other.</violation>
</file>
<file name="apps/status/app/error.tsx">
<violation number="1" location="apps/status/app/error.tsx:23">
P2: This error boundary hardcodes a 503 / "Status unavailable" message for every error it catches. In Next.js, `error.tsx` is a catch-all boundary that handles client-side exceptions, render bugs, and any unexpected server errors—not just monitor-data fetch failures. Labeling all of these as a 503 misclassifies the failure and undermines the PR goal of making status messaging evidence-bounded. Consider keeping a generic error code/title or inspecting the error to only show 503 when the failure is actually an availability/unreachable issue.</violation>
</file>
<file name="apps/basket/src/index.ts">
<violation number="1" location="apps/basket/src/index.ts:106">
P2: The `x-request-id` header is accepted directly from the client without validation or sanitization and then propagated into error telemetry, the response payload, and the response header. A malicious or buggy client can send arbitrarily long or high-cardinality values, which increases log cardinality, pollutes observability data, and enables correlation spoofing. Consider validating the header against a safe pattern (e.g., UUID or alphanumeric with a length cap, such as `^[a-zA-Z0-9_-]{1,64}$`) and falling back to `crypto.randomUUID()` when it fails validation.</violation>
</file>
<file name="packages/notifications/src/providers/slack.ts">
<violation number="1" location="packages/notifications/src/providers/slack.ts:68">
P1: The `buildSlackBlocks` helper chunks metadata fields into `section` blocks without capping the total block count. Slack incoming webhooks reject any message with more than 50 blocks, and because the provider retries on failure, an oversized payload wastes retries before the notification is lost. With the two fixed blocks plus metadata sections (and an optional priority block), as few as 480–490 metadata entries will exceed the limit. Cap the number of metadata sections so the total stays within Slack's 50-block limit while leaving room for the header, body, and optional priority block.</violation>
</file>
<file name="apps/dashboard/app/(main)/billing/components/topup-card.tsx">
<violation number="1" location="apps/dashboard/app/(main)/billing/components/topup-card.tsx:262">
P1: The nudge says "every usage unit drops to" the next tier's rate, but this component uses graduated pricing where each unit is billed by the tier it falls into — only the marginal units above the threshold get the lower rate. The pricing-details copy already clarifies this: "only the first 100 cost $X — every unit above that drops to a cheaper rate." The nudge should use similar wording ("units above that drop to") to avoid contradicting the graduated model and misleading buyers about their effective per-unit cost.</violation>
</file>
<file name="apps/docs/content/docs/sdk/configuration.mdx">
<violation number="1" location="apps/docs/content/docs/sdk/configuration.mdx:142">
P2: `trackWebVitals` is described inconsistently in the same docs page: the React props table says it only tracks Core Web Vitals, while the data-attributes table says it tracks both page performance and Core Web Vitals. Since `trackPerformance` was removed, users need a single authoritative description of what `trackWebVitals` actually captures. Align whichever description is stale with the SDK behavior.</violation>
</file>
<file name="apps/dashboard/components/feature-gate.tsx">
<violation number="1" location="apps/dashboard/components/feature-gate.tsx:138">
P2: When `canUserUpgrade` is false, the fallback UI now unconditionally links to `/organizations/members` and tells users to ask an organization owner or billing admin to upgrade. However, `canUserUpgrade` is also set to `false` by the server in non-permission cases such as public-workspace contexts and missing billing customers, and `isOrganizationBilling` may be `false`. In those cases the organization-members link is irrelevant and can be misleading. Consider checking `isOrganizationBilling` (already available from `useBillingContext`) before rendering the `/organizations/members` CTA, or provide a more generic fallback for non-organization states.</violation>
</file>
<file name="packages/tracker/src/core/utils.ts">
<violation number="1" location="packages/tracker/src/core/utils.ts:72">
P2: `sanitizePageUrl` does not restrict the URL protocol, so non-HTTP(S) schemes such as `mailto:`, `javascript:`, or `data:` produce malformed tracker metadata because `new URL(...).origin` is the string `"null"` for those schemes. Since this helper is used for outgoing-link `href`s, `document.referrer`, and exit paths, it should reject (return `""`) anything that is not `http:` or `https:`.</violation>
</file>
<file name="apps/dashboard/app/(auth)/login/magic/page.tsx">
<violation number="1" location="apps/dashboard/app/(auth)/login/magic/page.tsx:20">
P2: The `callback` query parameter is parsed without validation (`parseAsString.withDefault("/websites")`) and this change widens its propagation to additional client-side surfaces: the `/login` back-link, the post-send redirect to `/login/magic-sent`, and the `errorCallbackURL` passed to the auth client. Previously the back-link dropped the callback entirely. If the auth backend does not strictly validate `callbackURL`/`errorCallbackURL` against an allowlist of internal paths, this increases open-redirect and phishing exposure. Consider validating that `callback` is a relative internal path before encoding it into any URL or auth option.</violation>
</file>
<file name="apps/dashboard/lib/query-client.ts">
<violation number="1" location="apps/dashboard/lib/query-client.ts:100">
P1: Global mutation error reporting is now silently suppressed whenever a mutation defines its own `onError`, even if that local handler only performs partial work like optimistic-rollback or state cleanup and never surfaces a toast or calls `trackError`. There are already mutations in the codebase (e.g. `useDeleteWebsite`, `useOrganizations`, `useLinks`) whose `onError` handlers do exactly that. Previously those mutations still benefited from global `toast.error` and `trackError`; with this guard they will fail silently for both users and monitoring. The codebase already has an explicit opt-out via `meta.suppressGlobalErrorToast` — use that instead of blanket suppression by `onError` presence.</violation>
</file>
<file name="packages/rpc/src/routers/status-page-health.ts">
<violation number="1" location="packages/rpc/src/routers/status-page-health.ts:5">
P2: The `granularity` parameter is typed as an unconstrained `string`, but `deriveMonitorFreshness` relies on an exact key match against the hardcoded `GRANULARITY_MS` map. A mismatch from upstream (e.g., a new or renamed granularity in the DB schema) silently produces `undefined`, which falls back to `"unknown"` freshness and degrades the derived status. Consider constraining the type to the known keys (`keyof typeof GRANULARITY_MS`) or adding explicit validation/documentation that links the map to the upstream schema so drift is caught at compile time or in tests rather than silently in production.</violation>
</file>
<file name="apps/dashboard/app/(auth)/register/page.tsx">
<violation number="1" location="apps/dashboard/app/(auth)/register/page.tsx:138">
P1: The `resendVerificationEmail` and `handleSocialLogin` flows now pass user-controlled query input into authentication callback parameters. Previously `sendVerificationEmail` and `newUserCallbackURL` were hardcoded to `/onboarding`; after this change they use `getCallbackUrl()`, which falls back to the raw `callback` query string parsed by `useQueryState` without validation or normalization. If an attacker crafts a registration URL with `?callback=https://evil.com`, that value propagates into `authClient.sendVerificationEmail({ callbackURL: ... })` and `authClient.signIn.social({ newUserCallbackURL: ... })`, creating a potential open-redirect or phishing vector when server-side enforcement is absent or misconfigured. Consider validating or sanitizing the `callback` parameter before passing it to auth client methods, or document the server-side allowlist that is expected to block arbitrary URLs.</violation>
</file>
<file name="apps/api/src/routes/webhooks/autumn.ts">
<violation number="1" location="apps/api/src/routes/webhooks/autumn.ts:153">
P1: When `entity_id` is missing and a user owns multiple organizations, `resolveBillingOrganization` returns `null` because `candidates.length !== 1`. Both `handleLimitReached` and `handleUsageAlert` then return `success: false`, and the route handler maps that to HTTP 502. Since the payload itself is unambiguously unresolvable, retries from the webhook provider will never succeed — this creates a permanent retry loop for affected customers and wastes resources.
Consider returning `success: true` (since the webhook was received and the email skip is intentional business logic), or map organization-resolution failures to a non-retry status like 422 so providers stop retrying.</violation>
</file>
<file name="packages/email/src/emails/usage-email-utils.ts">
<violation number="1" location="packages/email/src/emails/usage-email-utils.ts:11">
P2: `formatUsageNumber` passes its input directly to `Intl.NumberFormat` without validating that the value is finite, so `NaN`, `Infinity`, or `-Infinity` would show up in customer-facing emails as the literal strings `NaN`, `∞`, or `-∞`. The `formatUsagePercentage` function in the same file already guards against this with `Number.isFinite()` and returns a safe fallback (`—`), so adding the same check to `formatUsageNumber` keeps the formatting utilities consistent and prevents malformed telemetry from reaching billing/usage emails.</violation>
<violation number="2" location="packages/email/src/emails/usage-email-utils.ts:22">
P2: `formatResetDate` assumes its `timestamp` parameter is in milliseconds because it passes the value directly to `new Date()`. Without a docstring or parameter name like `timestampMs`, callers from backend code that pass Unix seconds will silently produce dates around 1970 in customer-facing billing emails. Consider adding a JSDoc comment (`@param timestamp - milliseconds since Unix epoch`) or renaming the parameter to `timestampMs` to make the contract explicit.</violation>
<violation number="3" location="packages/email/src/emails/usage-email-utils.ts:22">
P1: `formatResetDate` can throw `RangeError` when `timestamp` is finite but outside the valid Date range (e.g., very large epoch numbers). `Number.isFinite` does not guarantee `new Date(timestamp)` will be valid. A safer pattern is to construct the Date, verify it with `Number.isNaN(date.getTime())`, and return `undefined` on invalid input.</violation>
</file>
<file name="apps/api/src/routes/public/flags.ts">
<violation number="1" location="apps/api/src/routes/public/flags.ts:113">
P1: The new POST `/bulk` route accepts flag keys in a JSON body array without any size or length limits. Because the per-client rate-limit check only runs inside `evaluateBulkFlags` after Elysia has already parsed the body, a large or malicious payload can exhaust CPU and memory before being rejected. Consider adding `maxItems` to the array and `maxLength` to the key strings (and ensuring an upstream body-size limit is in place) to close this allocation path.</violation>
<violation number="2" location="apps/api/src/routes/public/flags.ts:786">
P1: When the `keys` parameter is empty (e.g. `?keys=` or `{"keys":[]}`), the bulk flag endpoint now evaluates zero flags instead of all flags. This happens because the shared `evaluateBulkFlags` helper only checks `input.keys` for truthiness; an empty array `[""]` or `[]` passes that test, and after `.trim().filter(Boolean)` produces an empty Set. Because the Set object itself is truthy, the downstream filter `allFlags.filter((flag) => requestedKeys.has(flag.key))` returns nothing.
Normalize the trimmed key list so that an empty result is treated the same as a missing `keys` parameter (i.e. `null`), preserving the prior behavior where no filter means "evaluate all flags."</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 200 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| ), | ||
| })) | ||
| : []; | ||
| for ( |
There was a problem hiding this comment.
P1: The buildSlackBlocks helper chunks metadata fields into section blocks without capping the total block count. Slack incoming webhooks reject any message with more than 50 blocks, and because the provider retries on failure, an oversized payload wastes retries before the notification is lost. With the two fixed blocks plus metadata sections (and an optional priority block), as few as 480–490 metadata entries will exceed the limit. Cap the number of metadata sections so the total stays within Slack's 50-block limit while leaving room for the header, body, and optional priority block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/notifications/src/providers/slack.ts, line 68:
<comment>The `buildSlackBlocks` helper chunks metadata fields into `section` blocks without capping the total block count. Slack incoming webhooks reject any message with more than 50 blocks, and because the provider retries on failure, an oversized payload wastes retries before the notification is lost. With the two fixed blocks plus metadata sections (and an optional priority block), as few as 480–490 metadata entries will exceed the limit. Cap the number of metadata sections so the total stays within Slack's 50-block limit while leaving room for the header, body, and optional priority block.</comment>
<file context>
@@ -5,6 +5,92 @@ import type {
+ ),
+ }))
+ : [];
+ for (
+ let index = 0;
+ index < metadataFields.length;
</file context>
| {nudge.unitsUntilNextTier.toLocaleString()} | ||
| </span>{" "} | ||
| more and every credit drops to{" "} | ||
| more and every usage unit drops to{" "} |
There was a problem hiding this comment.
P1: The nudge says "every usage unit drops to" the next tier's rate, but this component uses graduated pricing where each unit is billed by the tier it falls into — only the marginal units above the threshold get the lower rate. The pricing-details copy already clarifies this: "only the first 100 cost $X — every unit above that drops to a cheaper rate." The nudge should use similar wording ("units above that drop to") to avoid contradicting the graduated model and misleading buyers about their effective per-unit cost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/app/(main)/billing/components/topup-card.tsx, line 262:
<comment>The nudge says "every usage unit drops to" the next tier's rate, but this component uses graduated pricing where each unit is billed by the tier it falls into — only the marginal units above the threshold get the lower rate. The pricing-details copy already clarifies this: "only the first 100 cost $X — every unit above that drops to a cheaper rate." The nudge should use similar wording ("units above that drop to") to avoid contradicting the graduated model and misleading buyers about their effective per-unit cost.</comment>
<file context>
@@ -247,7 +259,7 @@ function NudgeSlot({ blendedRate, nudge, quantity, savings }: NudgeSlotProps) {
{nudge.unitsUntilNextTier.toLocaleString()}
</span>{" "}
- more and every credit drops to{" "}
+ more and every usage unit drops to{" "}
<span className="font-medium text-foreground tabular-nums">
${nudge.nextRate.toFixed(3)}
</file context>
Suggested change
| more and every usage unit drops to{" "} | |
| + more and units above that drop to{" "} |
| isSilencedError(error) || | ||
| mutation.meta?.suppressGlobalErrorToast | ||
| mutation.meta?.suppressGlobalErrorToast || | ||
| mutation.options.onError |
There was a problem hiding this comment.
P1: Global mutation error reporting is now silently suppressed whenever a mutation defines its own onError, even if that local handler only performs partial work like optimistic-rollback or state cleanup and never surfaces a toast or calls trackError. There are already mutations in the codebase (e.g. useDeleteWebsite, useOrganizations, useLinks) whose onError handlers do exactly that. Previously those mutations still benefited from global toast.error and trackError; with this guard they will fail silently for both users and monitoring. The codebase already has an explicit opt-out via meta.suppressGlobalErrorToast — use that instead of blanket suppression by onError presence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/lib/query-client.ts, line 100:
<comment>Global mutation error reporting is now silently suppressed whenever a mutation defines its own `onError`, even if that local handler only performs partial work like optimistic-rollback or state cleanup and never surfaces a toast or calls `trackError`. There are already mutations in the codebase (e.g. `useDeleteWebsite`, `useOrganizations`, `useLinks`) whose `onError` handlers do exactly that. Previously those mutations still benefited from global `toast.error` and `trackError`; with this guard they will fail silently for both users and monitoring. The codebase already has an explicit opt-out via `meta.suppressGlobalErrorToast` — use that instead of blanket suppression by `onError` presence.</comment>
<file context>
@@ -95,7 +96,8 @@ export function makeQueryClient() {
isSilencedError(error) ||
- mutation.meta?.suppressGlobalErrorToast
+ mutation.meta?.suppressGlobalErrorToast ||
+ mutation.options.onError
) {
return;
</file context>
| try { | ||
| const { error } = await authClient.sendVerificationEmail({ | ||
| email: formData.email, | ||
| callbackURL: getCallbackUrl(), |
There was a problem hiding this comment.
P1: The resendVerificationEmail and handleSocialLogin flows now pass user-controlled query input into authentication callback parameters. Previously sendVerificationEmail and newUserCallbackURL were hardcoded to /onboarding; after this change they use getCallbackUrl(), which falls back to the raw callback query string parsed by useQueryState without validation or normalization. If an attacker crafts a registration URL with ?callback=https://evil.com, that value propagates into authClient.sendVerificationEmail({ callbackURL: ... }) and authClient.signIn.social({ newUserCallbackURL: ... }), creating a potential open-redirect or phishing vector when server-side enforcement is absent or misconfigured. Consider validating or sanitizing the callback parameter before passing it to auth client methods, or document the server-side allowlist that is expected to block arbitrary URLs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/app/(auth)/register/page.tsx, line 138:
<comment>The `resendVerificationEmail` and `handleSocialLogin` flows now pass user-controlled query input into authentication callback parameters. Previously `sendVerificationEmail` and `newUserCallbackURL` were hardcoded to `/onboarding`; after this change they use `getCallbackUrl()`, which falls back to the raw `callback` query string parsed by `useQueryState` without validation or normalization. If an attacker crafts a registration URL with `?callback=https://evil.com`, that value propagates into `authClient.sendVerificationEmail({ callbackURL: ... })` and `authClient.signIn.social({ newUserCallbackURL: ... })`, creating a potential open-redirect or phishing vector when server-side enforcement is absent or misconfigured. Consider validating or sanitizing the `callback` parameter before passing it to auth client methods, or document the server-side allowlist that is expected to block arbitrary URLs.</comment>
<file context>
@@ -132,34 +132,38 @@ function RegisterPageContent() {
+ try {
+ const { error } = await authClient.sendVerificationEmail({
+ email: formData.email,
+ callbackURL: getCallbackUrl(),
+ });
+ if (error) {
</file context>
| }, | ||
| }); | ||
|
|
||
| const candidates = entityId |
There was a problem hiding this comment.
P1: When entity_id is missing and a user owns multiple organizations, resolveBillingOrganization returns null because candidates.length !== 1. Both handleLimitReached and handleUsageAlert then return success: false, and the route handler maps that to HTTP 502. Since the payload itself is unambiguously unresolvable, retries from the webhook provider will never succeed — this creates a permanent retry loop for affected customers and wastes resources.
Consider returning success: true (since the webhook was received and the email skip is intentional business logic), or map organization-resolution failures to a non-retry status like 422 so providers stop retrying.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/routes/webhooks/autumn.ts, line 153:
<comment>When `entity_id` is missing and a user owns multiple organizations, `resolveBillingOrganization` returns `null` because `candidates.length !== 1`. Both `handleLimitReached` and `handleUsageAlert` then return `success: false`, and the route handler maps that to HTTP 502. Since the payload itself is unambiguously unresolvable, retries from the webhook provider will never succeed — this creates a permanent retry loop for affected customers and wastes resources.
Consider returning `success: true` (since the webhook was received and the email skip is intentional business logic), or map organization-resolution failures to a non-retry status like 422 so providers stop retrying.</comment>
<file context>
@@ -118,8 +136,105 @@ const getUserData = cacheable(
+ },
+ });
+
+ const candidates = entityId
+ ? memberships.filter((row) => row.organizationId === entityId)
+ : memberships;
</file context>
| return `${origin}${maskPathname(pathname, patterns)}`; | ||
| } | ||
|
|
||
| export function sanitizePageUrl(value: string): string { |
There was a problem hiding this comment.
P2: sanitizePageUrl does not restrict the URL protocol, so non-HTTP(S) schemes such as mailto:, javascript:, or data: produce malformed tracker metadata because new URL(...).origin is the string "null" for those schemes. Since this helper is used for outgoing-link hrefs, document.referrer, and exit paths, it should reject (return "") anything that is not http: or https:.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tracker/src/core/utils.ts, line 72:
<comment>`sanitizePageUrl` does not restrict the URL protocol, so non-HTTP(S) schemes such as `mailto:`, `javascript:`, or `data:` produce malformed tracker metadata because `new URL(...).origin` is the string `"null"` for those schemes. Since this helper is used for outgoing-link `href`s, `document.referrer`, and exit paths, it should reject (return `""`) anything that is not `http:` or `https:`.</comment>
<file context>
@@ -61,6 +61,26 @@ export function maskPathname(
+ return `${origin}${maskPathname(pathname, patterns)}`;
+}
+
+export function sanitizePageUrl(value: string): string {
+ if (!value) {
+ return "";
</file context>
Suggested change
| export function sanitizePageUrl(value: string): string { | |
| export function sanitizePageUrl(value: string): string { | |
| if (!value) { | |
| return ""; | |
| } | |
| try { | |
| const url = new URL(value); | |
| if (url.protocol !== "http:" && url.protocol !== "https:") { | |
| return ""; | |
| } | |
| return `${url.origin}${url.pathname}`; | |
| } catch { | |
| return ""; | |
| } | |
| } |
| ); | ||
| const [email, setEmail] = useState(""); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const loginHref = `/login?callback=${encodeURIComponent(callback)}`; |
There was a problem hiding this comment.
P2: The callback query parameter is parsed without validation (parseAsString.withDefault("/websites")) and this change widens its propagation to additional client-side surfaces: the /login back-link, the post-send redirect to /login/magic-sent, and the errorCallbackURL passed to the auth client. Previously the back-link dropped the callback entirely. If the auth backend does not strictly validate callbackURL/errorCallbackURL against an allowlist of internal paths, this increases open-redirect and phishing exposure. Consider validating that callback is a relative internal path before encoding it into any URL or auth option.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/dashboard/app/(auth)/login/magic/page.tsx, line 20:
<comment>The `callback` query parameter is parsed without validation (`parseAsString.withDefault("/websites")`) and this change widens its propagation to additional client-side surfaces: the `/login` back-link, the post-send redirect to `/login/magic-sent`, and the `errorCallbackURL` passed to the auth client. Previously the back-link dropped the callback entirely. If the auth backend does not strictly validate `callbackURL`/`errorCallbackURL` against an allowlist of internal paths, this increases open-redirect and phishing exposure. Consider validating that `callback` is a relative internal path before encoding it into any URL or auth option.</comment>
<file context>
@@ -17,6 +17,7 @@ function MagicLinkPage() {
);
const [email, setEmail] = useState("");
const [isLoading, setIsLoading] = useState(false);
+ const loginHref = `/login?callback=${encodeURIComponent(callback)}`;
const handleMagicLinkLogin = async (e: React.FormEvent) => {
</file context>
| export type MonitorFreshness = "fresh" | "stale" | "unknown"; | ||
| export type OverallStatus = "operational" | "degraded" | "outage" | "unknown"; | ||
|
|
||
| const GRANULARITY_MS: Record<string, number> = { |
There was a problem hiding this comment.
P2: The granularity parameter is typed as an unconstrained string, but deriveMonitorFreshness relies on an exact key match against the hardcoded GRANULARITY_MS map. A mismatch from upstream (e.g., a new or renamed granularity in the DB schema) silently produces undefined, which falls back to "unknown" freshness and degrades the derived status. Consider constraining the type to the known keys (keyof typeof GRANULARITY_MS) or adding explicit validation/documentation that links the map to the upstream schema so drift is caught at compile time or in tests rather than silently in production.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rpc/src/routers/status-page-health.ts, line 5:
<comment>The `granularity` parameter is typed as an unconstrained `string`, but `deriveMonitorFreshness` relies on an exact key match against the hardcoded `GRANULARITY_MS` map. A mismatch from upstream (e.g., a new or renamed granularity in the DB schema) silently produces `undefined`, which falls back to `"unknown"` freshness and degrades the derived status. Consider constraining the type to the known keys (`keyof typeof GRANULARITY_MS`) or adding explicit validation/documentation that links the map to the upstream schema so drift is caught at compile time or in tests rather than silently in production.</comment>
<file context>
@@ -0,0 +1,128 @@
+export type MonitorFreshness = "fresh" | "stale" | "unknown";
+export type OverallStatus = "operational" | "degraded" | "outage" | "unknown";
+
+const GRANULARITY_MS: Record<string, number> = {
+ minute: 60_000,
+ five_minutes: 5 * 60_000,
</file context>
| timeZone: "UTC", | ||
| }); | ||
|
|
||
| export function formatUsageNumber(value: number): string { |
There was a problem hiding this comment.
P2: formatUsageNumber passes its input directly to Intl.NumberFormat without validating that the value is finite, so NaN, Infinity, or -Infinity would show up in customer-facing emails as the literal strings NaN, ∞, or -∞. The formatUsagePercentage function in the same file already guards against this with Number.isFinite() and returns a safe fallback (—), so adding the same check to formatUsageNumber keeps the formatting utilities consistent and prevents malformed telemetry from reaching billing/usage emails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/email/src/emails/usage-email-utils.ts, line 11:
<comment>`formatUsageNumber` passes its input directly to `Intl.NumberFormat` without validating that the value is finite, so `NaN`, `Infinity`, or `-Infinity` would show up in customer-facing emails as the literal strings `NaN`, `∞`, or `-∞`. The `formatUsagePercentage` function in the same file already guards against this with `Number.isFinite()` and returns a safe fallback (`—`), so adding the same check to `formatUsageNumber` keeps the formatting utilities consistent and prevents malformed telemetry from reaching billing/usage emails.</comment>
<file context>
@@ -0,0 +1,27 @@
+ timeZone: "UTC",
+});
+
+export function formatUsageNumber(value: number): string {
+ return usageNumberFormatter.format(value);
+}
</file context>
| return `${Math.round((usage / limit) * 100)}%`; | ||
| } | ||
|
|
||
| export function formatResetDate(timestamp?: number | null): string | undefined { |
There was a problem hiding this comment.
P2: formatResetDate assumes its timestamp parameter is in milliseconds because it passes the value directly to new Date(). Without a docstring or parameter name like timestampMs, callers from backend code that pass Unix seconds will silently produce dates around 1970 in customer-facing billing emails. Consider adding a JSDoc comment (@param timestamp - milliseconds since Unix epoch) or renaming the parameter to timestampMs to make the contract explicit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/email/src/emails/usage-email-utils.ts, line 22:
<comment>`formatResetDate` assumes its `timestamp` parameter is in milliseconds because it passes the value directly to `new Date()`. Without a docstring or parameter name like `timestampMs`, callers from backend code that pass Unix seconds will silently produce dates around 1970 in customer-facing billing emails. Consider adding a JSDoc comment (`@param timestamp - milliseconds since Unix epoch`) or renaming the parameter to `timestampMs` to make the contract explicit.</comment>
<file context>
@@ -0,0 +1,27 @@
+ return `${Math.round((usage / limit) * 100)}%`;
+}
+
+export function formatResetDate(timestamp?: number | null): string | undefined {
+ if (timestamp == null || !Number.isFinite(timestamp)) {
+ return;
</file context>
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
Audits and tightens customer-facing communication across transactional email, billing and authentication states, API errors, SDK/tracker outcomes, Slack, redirects, status pages, onboarding, and public documentation.
The main user-visible changes:
Scope
Verification
Verified from an isolated clean worktree at 386a858:
The repo-wide lint command still reports 27 pre-existing diagnostics in untouched basket/ClickHouse files. Every changed JS/TS/JSON file is clean.
Pre-landing review
Fixed all identified P1 findings:
Fixed the two email-copy P2 findings around misleading reachability claims and duplicated/raw uptime metadata.
Known follow-up: mixed-destination uptime alarms still need a durable per-destination outbox. Today, one successful destination can commit the transition while another failed destination is not retried independently.
Release and rollout
This is intentionally a draft until the SDK release path is chosen. The branch currently includes breaking SDK behavior:
Choose one before merge:
Rollout order:
Documentation
Test plan
AI-assisted with OpenAI Codex. All committed changes were reviewed and verified in an isolated worktree.
Summary by cubic
Clarifies customer communication and delivery outcomes across emails, billing, auth, API errors, flags, Slack, status pages, and docs. Adds request IDs and safer, clearer messages; preserves redirect destinations; honors privacy signals; and isolates flag state per identity.
New Features
X-Request-IDand return it in error payloads; rate-limit headers exposed.callbackthrough auth; improved empty/error states.Bug Fixes
requestId; blocked-traffic alerts guard missing email config.Written for commit 386a858. Summary will update on new commits.