feat(Notifications): Platform notifications for CLI and Dashboard #3254
feat(Notifications): Platform notifications for CLI and Dashboard #3254
Conversation
|
WalkthroughAdds a platform notifications system: database migration and Prisma schema for notifications and interactions; server services for creating, querying, and recording interactions (web and CLI); multiple Remix routes (admin CRUD UI, public API, resource polling, and interaction beacons); admin notifications UI and webapp NotificationPanel plus changelog polling and Help popover updates; CLI client and commands to fetch/display notifications with discovery and color-markup utilities and tests; Redis-backed per-user CLI request counter; and a package dependency addition for react-markdown. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
0ski
force-pushed
the
oskar/feat-platform-notifications
branch
from
7e4eac9 to
dc9619f
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 CLI notification fetch has no changeset for the new cli-v3 package changes
Per CONTRIBUTING.md and CLAUDE.md, changes to packages in /packages should include a changeset (pnpm run changeset:add). This PR modifies packages/cli-v3/src/apiClient.ts, packages/cli-v3/src/commands/dev.ts, packages/cli-v3/src/commands/login.ts, and adds new utility files. These are published package changes that would need a changeset for the version bump and changelog entry.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🚩 Server-changes file may be needed for webapp changes
Per CONTRIBUTING.md, if the PR changes server components (apps/webapp/) with no package changes, a .server-changes/ file should be added. Since this PR changes both packages (packages/cli-v3) and server (apps/webapp/), a changeset alone should suffice per the documented rules. However, if a changeset is not added, a .server-changes/ file would be needed for the webapp side.
Was this helpful? React with 👍 or 👎 to provide feedback.
| include: { | ||
| _count: { | ||
| select: { interactions: true }, | ||
| }, | ||
| interactions: { | ||
| select: { | ||
| webappDismissedAt: true, | ||
| webappClickedAt: true, | ||
| cliDismissedAt: true, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🚩 getAdminNotificationsList fetches ALL interactions per notification for stats
The admin list query at apps/webapp/app/services/platformNotifications.server.ts:68-88 includes all interactions records per notification (not limited to a sample) to compute click/dismiss stats. For a global notification seen by thousands of users, this could return thousands of interaction rows per notification × page_size notifications. The _count aggregation is efficient, but the interactions include for filtering webappClickedAt/cliDismissedAt materializes all rows. Consider using Prisma _count with where filters for clicked/dismissed counts instead of loading all interactions.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
apps/webapp/app/routes/resources.platform-notifications.$id.clicked.tsx (1)
6-17: Consider adding request method validation.The action handler will respond to any HTTP method. Consider validating that the request is a POST to follow REST conventions and prevent unintended behavior from GET requests (e.g., browser prefetching or link previews).
🛡️ Suggested validation
export async function action({ request, params }: ActionFunctionArgs) { + if (request.method !== "POST") { + return json({ success: false }, { status: 405 }); + } + const userId = await requireUserId(request);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/resources.platform-notifications`.$id.clicked.tsx around lines 6 - 17, The action currently accepts any HTTP method; update the action({ request, params }: ActionFunctionArgs) to validate request.method === "POST" and return an appropriate 405 Method Not Allowed (or 400/JSON error) when it's not POST, before calling requireUserId or recordNotificationClicked; ensure the validation happens early, preserve existing behavior for POST (including requireUserId and recordNotificationClicked with params.id), and include a clear error JSON response for non-POST requests.packages/cli-v3/src/commands/login.ts (1)
292-295: The notification display is already resilient to failures.The implementation is already well-designed for error resilience:
fetchPlatformNotificationwraps its API call intryCatchand returnsundefinedon any error, so it never throws. This means the notification promise itself cannot fail—it always resolves to aPlatformNotificationorundefined. The only potential throw source is the logging call indisplayPlatformNotification, which is extremely unlikely.This pattern is consistent with how it's used elsewhere in the codebase (e.g.,
dev.ts), and the existing implementation ensures that notification failures do not affect the login flow as intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-v3/src/commands/login.ts` around lines 292 - 295, The notification fetch is already resilient because fetchPlatformNotification (used with new CliApiClient and indexResult.token) returns undefined on errors, so don't add redundant try/catch or promise handling around the notificationPromise; keep the creation of notificationPromise and the subsequent call to displayPlatformNotification as-is (using fetchPlatformNotification and CliApiClient) so login flow remains unaffected.apps/webapp/app/components/navigation/NotificationPanel.tsx (2)
42-44: Type assertion on hook return value.Consider typing
usePlatformNotificationsto return{ notifications: Notification[] }at the source rather than asserting here. This would catch type mismatches at the hook definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 42 - 44, The code is using a type assertion at the callsite of usePlatformNotifications in NotificationPanel (casting its result to { notifications: Notification[] }); instead, update the hook's own type signature so usePlatformNotifications returns { notifications: Notification[] } (or a proper interface) so callers like NotificationPanel can use the typed return directly; adjust the hook's definition (and any related types) to export/declare Notification and the return shape, and remove the cast in NotificationPanel to rely on the hook's typed return value.
55-65: Missing fetcher dependencies in useCallback hooks.The callbacks reference
dismissFetcher,clickFetcher, andseenFetcherbut have empty dependency arrays. While Remix'suseFetcherreturns a stable reference, explicitly including them satisfies the exhaustive-deps lint rule and documents the dependency.🔧 Optional fix
- const handleDismiss = useCallback((id: string) => { + const handleDismiss = useCallback((id: string) => { setDismissedIds((prev) => new Set(prev).add(id)); dismissFetcher.submit(/* ... */); - }, []); + }, [dismissFetcher]); - const fireClickBeacon = useCallback((id: string) => { + const fireClickBeacon = useCallback((id: string) => { // ... - }, []); + }, [clickFetcher]); - const fireSeenBeacon = useCallback((n: Notification) => { + const fireSeenBeacon = useCallback((n: Notification) => { // ... - }, []); + }, [seenFetcher]);Also applies to: 67-78, 81-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 55 - 65, Update the useCallback dependency arrays to include the fetcher variables referenced inside each callback: add dismissFetcher to the dependency array for handleDismiss, add clickFetcher to the dependency array for the click handler (e.g., handleClick), and add seenFetcher to the dependency array for the seen handler (e.g., handleSeen); keep other stable functions like setDismissedIds as-is since state setters are stable, but explicitly listing the corresponding fetcher in each useCallback satisfies exhaustive-deps and documents the dependency.apps/webapp/app/routes/admin.api.v1.platform-notifications.ts (1)
45-46: Validate request body with Zod at route handler level.Currently, the body is cast without validation at the route boundary. While
createPlatformNotificationvalidates internally using Zod, validating at the entry point provides early error catching and clearer API contracts.🔧 Suggested approach
+import { CreatePlatformNotificationSchema } from "~/services/platformNotifications.server"; + // In action function: - const body = await request.json(); - const result = await createPlatformNotification(body as CreatePlatformNotificationInput); + const rawBody = await request.json(); + const parseResult = CreatePlatformNotificationSchema.safeParse(rawBody); + if (!parseResult.success) { + return json({ error: "Invalid request body", details: parseResult.error.issues }, { status: 400 }); + } + const result = await createPlatformNotification(parseResult.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/admin.api.v1.platform-notifications.ts` around lines 45 - 46, The route handler reads the body with request.json() and casts it to CreatePlatformNotificationInput without schema validation; update this handler to validate the incoming payload using the same Zod schema used by createPlatformNotification (or its exported Zod schema) before calling createPlatformNotification — parse/validate the body via the Zod schema (e.g., schema.parse or schema.safeParse) and return a 4xx error on validation failure, then pass the validated value to createPlatformNotification instead of the raw cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/components/navigation/NotificationPanel.tsx`:
- Around line 277-287: The markdown link anchor is allowing click events to
bubble to the parent card's onClick (which calls handleCardClick and may dismiss
the card when dismissOnAction is true); update the markdown link click handler
(onLinkClick used by the inline anchor component) to accept the click event and
call event.stopPropagation() before performing link behavior so the parent
card's onClick (and handleCardClick) are not triggered by link clicks.
In `@apps/webapp/app/routes/admin.notifications.tsx`:
- Around line 715-718: The inline JSX comment inside the image preview branch is
creating an extra sibling expression and breaking TSX parsing; remove the
comment from inside the parentheses of the conditional (the expression using
image && (...)) and either place the lgtm comment immediately above the
conditional as a standalone JSX comment or delete it, leaving the <img
src={image} alt="" className="mt-1.5 rounded px-2 pb-2" /> expression as the
sole child of the parentheses so the image preview (image && (...)) parses
correctly.
In `@apps/webapp/app/routes/resources.platform-notifications.tsx`:
- Around line 35-54: The initial-load guard (hasInitiallyFetched) in
usePlatformNotifications never resets when organizationId/projectId change,
causing stale initial context; replace the boolean guard with a ref that stores
the lastRequestedUrl (or store the computed url as the key) and only skip load
if lastRequestedUrl.current === url and fetcher.state === "idle"; on a new url
set lastRequestedUrl.current = url and call fetcher.load(url); keep the interval
logic unchanged but use the same url variable and the ref check to avoid
blocking the first immediate load for new organizationId/projectId.
In `@apps/webapp/app/services/platformNotifications.server.ts`:
- Around line 277-294: getRecentChangelogs currently ignores audience and
timing, so USER/org/project scoped changelogs and archived/future/expired rows
leak into the result; update getRecentChangelogs to accept audience/context
params (e.g., scope, userId, organizationId, projectId) and apply them in the
prisma.platformNotification.findMany where clause (filter by scope and matching
userId/organizationId/projectId as appropriate), and also restrict to
published/non-archived rows and current time window (e.g., startAt <= now and
(endAt IS NULL OR endAt >= now) and archived = false or status = 'published')
before mapping and parsing with PayloadV1Schema. Ensure the function signature
(getRecentChangelogs) and the prisma query are updated accordingly so only
visible, published, in-scope changelogs are returned.
- Around line 298-319: isCliNotificationExpired currently treats
interaction.showCount as the total request attempts, but elsewhere the code
increments showCount before applying the cliShowEvery modulo, causing suppressed
requests to consume the max-show budget; change the logic so the "show" counter
only increments when the notification is actually displayed (or introduce a
separate request counter like requestCount) and leave interaction.showCount to
represent displayed shows checked by isCliNotificationExpired (which uses
interaction.showCount and notification.cliMaxShowCount). Update the code paths
that currently increment interaction.showCount before the cliShowEvery check to
instead increment after passing the modulo/display decision (and mirror the same
fix in the other similar block mentioned), ensuring isCliNotificationExpired
(and any checks against notification.cliMaxDaysAfterFirstSeen) still operate on
the displayed-show counter.
- Around line 8-25: The schema currently allows storing arbitrary regexes via
DiscoverySchema.contentPattern which can lead to catastrophic backtracking when
later executed; update the schema to require an explicit flag (e.g., add a
boolean useRegex) and only accept contentPattern as a regex when that flag is
true, and when accepting regexes apply strict safety checks: limit pattern
length (e.g., <= 200 chars), reject patterns containing known dangerous
constructs (nested quantifiers like (.+)+, (.*){n,}, backreferences like \1, and
lookarounds ?= / ?! / ?<= / ?<!), and still attempt to compile the pattern as a
final validate step; if useRegex is false always treat contentPattern as a
literal substring (no RegExp compile). Ensure you modify DiscoverySchema and the
contentPattern refine logic accordingly and keep matchBehavior behavior
consistent.
- Around line 503-523: validateSurfaceFields currently only blocks CLI-only
numeric fields but doesn't check the notification payload type, allowing
incompatible combinations (e.g., surface: "CLI" with type "card"/"changelog" or
surface: "WEBAPP" with type "log"); update validateSurfaceFields to also read
the payload.data.type (payload.data.type) and add ctx.addIssue when surface and
type are mismatched: reject surface === "CLI" if type is "card" or "changelog"
and reject surface === "WEBAPP" if type is "log" (use the same issue shape as
existing checks and reference CLI_ONLY_FIELDS, validateSurfaceFields, and
payload.data.type when locating where to add the new checks).
---
Nitpick comments:
In `@apps/webapp/app/components/navigation/NotificationPanel.tsx`:
- Around line 42-44: The code is using a type assertion at the callsite of
usePlatformNotifications in NotificationPanel (casting its result to {
notifications: Notification[] }); instead, update the hook's own type signature
so usePlatformNotifications returns { notifications: Notification[] } (or a
proper interface) so callers like NotificationPanel can use the typed return
directly; adjust the hook's definition (and any related types) to export/declare
Notification and the return shape, and remove the cast in NotificationPanel to
rely on the hook's typed return value.
- Around line 55-65: Update the useCallback dependency arrays to include the
fetcher variables referenced inside each callback: add dismissFetcher to the
dependency array for handleDismiss, add clickFetcher to the dependency array for
the click handler (e.g., handleClick), and add seenFetcher to the dependency
array for the seen handler (e.g., handleSeen); keep other stable functions like
setDismissedIds as-is since state setters are stable, but explicitly listing the
corresponding fetcher in each useCallback satisfies exhaustive-deps and
documents the dependency.
In `@apps/webapp/app/routes/admin.api.v1.platform-notifications.ts`:
- Around line 45-46: The route handler reads the body with request.json() and
casts it to CreatePlatformNotificationInput without schema validation; update
this handler to validate the incoming payload using the same Zod schema used by
createPlatformNotification (or its exported Zod schema) before calling
createPlatformNotification — parse/validate the body via the Zod schema (e.g.,
schema.parse or schema.safeParse) and return a 4xx error on validation failure,
then pass the validated value to createPlatformNotification instead of the raw
cast.
In `@apps/webapp/app/routes/resources.platform-notifications`.$id.clicked.tsx:
- Around line 6-17: The action currently accepts any HTTP method; update the
action({ request, params }: ActionFunctionArgs) to validate request.method ===
"POST" and return an appropriate 405 Method Not Allowed (or 400/JSON error) when
it's not POST, before calling requireUserId or recordNotificationClicked; ensure
the validation happens early, preserve existing behavior for POST (including
requireUserId and recordNotificationClicked with params.id), and include a clear
error JSON response for non-POST requests.
In `@packages/cli-v3/src/commands/login.ts`:
- Around line 292-295: The notification fetch is already resilient because
fetchPlatformNotification (used with new CliApiClient and indexResult.token)
returns undefined on errors, so don't add redundant try/catch or promise
handling around the notificationPromise; keep the creation of
notificationPromise and the subsequent call to displayPlatformNotification as-is
(using fetchPlatformNotification and CliApiClient) so login flow remains
unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36c87d64-2905-4d8b-a703-7e8e4d50cb86
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/routes/admin.tsxapps/webapp/app/routes/api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/routes/resources.platform-notifications.$id.clicked.tsxapps/webapp/app/routes/resources.platform-notifications.$id.dismiss.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.tsapps/webapp/package.jsoninternal-packages/database/prisma/migrations/20260223123615_add_platform_notification_tables/migration.sqlinternal-packages/database/prisma/schema.prismapackages/cli-v3/src/apiClient.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/commands/login.tspackages/cli-v3/src/utilities/colorMarkup.test.tspackages/cli-v3/src/utilities/colorMarkup.tspackages/cli-v3/src/utilities/discoveryCheck.test.tspackages/cli-v3/src/utilities/discoveryCheck.tspackages/cli-v3/src/utilities/platformNotifications.ts
| const DiscoverySchema = z.object({ | ||
| filePatterns: z.array(z.string().min(1)).min(1), | ||
| contentPattern: z | ||
| .string() | ||
| .optional() | ||
| .refine( | ||
| (val) => { | ||
| if (!val) return true; | ||
| try { | ||
| new RegExp(val); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }, | ||
| { message: "contentPattern must be a valid regular expression" } | ||
| ), | ||
| matchBehavior: z.enum(["show-if-found", "show-if-not-found"]), |
There was a problem hiding this comment.
Don't persist arbitrary regexes for discovery.
This only proves contentPattern compiles. A catastrophically backtracking pattern can still be saved here and later run by every CLI client against local files, which can hang dev/login on a single notification. Either constrain discovery to literal matching or add a safety check before accepting regex mode.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 16-16: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(val)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 8 -
25, The schema currently allows storing arbitrary regexes via
DiscoverySchema.contentPattern which can lead to catastrophic backtracking when
later executed; update the schema to require an explicit flag (e.g., add a
boolean useRegex) and only accept contentPattern as a regex when that flag is
true, and when accepting regexes apply strict safety checks: limit pattern
length (e.g., <= 200 chars), reject patterns containing known dangerous
constructs (nested quantifiers like (.+)+, (.*){n,}, backreferences like \1, and
lookarounds ?= / ?! / ?<= / ?<!), and still attempt to compile the pattern as a
final validate step; if useRegex is false always treat contentPattern as a
literal substring (no RegExp compile). Ensure you modify DiscoverySchema and the
contentPattern refine logic accordingly and keep matchBehavior behavior
consistent.
| export async function getRecentChangelogs({ limit = 2 }: { limit?: number } = {}) { | ||
| const notifications = await prisma.platformNotification.findMany({ | ||
| where: { | ||
| surface: "WEBAPP", | ||
| payload: { path: ["data", "type"], equals: "changelog" }, | ||
| }, | ||
| orderBy: [{ createdAt: "desc" }], | ||
| take: limit, | ||
| }); | ||
|
|
||
| return notifications | ||
| .map((n) => { | ||
| const parsed = PayloadV1Schema.safeParse(n.payload); | ||
| if (!parsed.success) return null; | ||
| return { id: n.id, title: parsed.data.data.title, actionUrl: parsed.data.data.actionUrl }; | ||
| }) | ||
| .filter(Boolean) as Array<{ id: string; title: string; actionUrl?: string }>; | ||
| } |
There was a problem hiding this comment.
Apply audience filtering before populating “What’s new”.
This helper ignores scope, userId, organizationId, and projectId, so a USER-scope preview or an org/project changelog will be returned to every signed-in user. If this popover is meant to show published changelogs, it should also exclude archived/future/expired rows here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 277 -
294, getRecentChangelogs currently ignores audience and timing, so
USER/org/project scoped changelogs and archived/future/expired rows leak into
the result; update getRecentChangelogs to accept audience/context params (e.g.,
scope, userId, organizationId, projectId) and apply them in the
prisma.platformNotification.findMany where clause (filter by scope and matching
userId/organizationId/projectId as appropriate), and also restrict to
published/non-archived rows and current time window (e.g., startAt <= now and
(endAt IS NULL OR endAt >= now) and archived = false or status = 'published')
before mapping and parsing with PayloadV1Schema. Ensure the function signature
(getRecentChangelogs) and the prisma query are updated accordingly so only
visible, published, in-scope changelogs are returned.
| function isCliNotificationExpired( | ||
| interaction: { firstSeenAt: Date; showCount: number } | null, | ||
| notification: { cliMaxDaysAfterFirstSeen: number | null; cliMaxShowCount: number | null } | ||
| ): boolean { | ||
| if (!interaction) return false; | ||
|
|
||
| if ( | ||
| notification.cliMaxShowCount !== null && | ||
| interaction.showCount >= notification.cliMaxShowCount | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (notification.cliMaxDaysAfterFirstSeen !== null) { | ||
| const daysSinceFirstSeen = | ||
| (Date.now() - interaction.firstSeenAt.getTime()) / (1000 * 60 * 60 * 24); | ||
| if (daysSinceFirstSeen > notification.cliMaxDaysAfterFirstSeen) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
cliShowEvery is currently burning through cliMaxShowCount while suppressed.
You increment the interaction before the modulo check, so non-displayed requests still consume showCount. With cliShowEvery = 3 and cliMaxShowCount = 5, the notification only renders once before isCliNotificationExpired() hides it forever. This needs a separate request counter, or the max-show behavior won't match the CLI controls.
Also applies to: 410-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 298 -
319, isCliNotificationExpired currently treats interaction.showCount as the
total request attempts, but elsewhere the code increments showCount before
applying the cliShowEvery modulo, causing suppressed requests to consume the
max-show budget; change the logic so the "show" counter only increments when the
notification is actually displayed (or introduce a separate request counter like
requestCount) and leave interaction.showCount to represent displayed shows
checked by isCliNotificationExpired (which uses interaction.showCount and
notification.cliMaxShowCount). Update the code paths that currently increment
interaction.showCount before the cliShowEvery check to instead increment after
passing the modulo/display decision (and mirror the same fix in the other
similar block mentioned), ensuring isCliNotificationExpired (and any checks
against notification.cliMaxDaysAfterFirstSeen) still operate on the
displayed-show counter.
| function validateSurfaceFields( | ||
| data: { | ||
| surface: string; | ||
| cliMaxDaysAfterFirstSeen?: number; | ||
| cliMaxShowCount?: number; | ||
| cliShowEvery?: number; | ||
| }, | ||
| ctx: z.RefinementCtx | ||
| ) { | ||
| if (data.surface !== "WEBAPP") return; | ||
|
|
||
| for (const field of CLI_ONLY_FIELDS) { | ||
| if (data[field] !== undefined) { | ||
| ctx.addIssue({ | ||
| code: "custom", | ||
| message: `${field} is not allowed for WEBAPP surface`, | ||
| path: [field], | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate payload.data.type against surface.
validateSurfaceFields() blocks the CLI-only numeric fields on WEBAPP, but it still allows surface: "CLI" with type: "card"/"changelog" and vice versa. Those combinations can come through the admin API and later make the CLI index log[...] with an unsupported level.
💡 Suggested fix
function validateSurfaceFields(
data: {
surface: string;
+ payload: PayloadV1;
cliMaxDaysAfterFirstSeen?: number;
cliMaxShowCount?: number;
cliShowEvery?: number;
},
ctx: z.RefinementCtx
) {
+ const allowedTypes =
+ data.surface === "CLI"
+ ? new Set(["info", "warn", "error", "success"])
+ : new Set(["card", "changelog"]);
+
+ if (!allowedTypes.has(data.payload.data.type)) {
+ ctx.addIssue({
+ code: "custom",
+ message: `payload.data.type ${data.payload.data.type} is not allowed for ${data.surface}`,
+ path: ["payload", "data", "type"],
+ });
+ }
+
if (data.surface !== "WEBAPP") return;
for (const field of CLI_ONLY_FIELDS) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 503 -
523, validateSurfaceFields currently only blocks CLI-only numeric fields but
doesn't check the notification payload type, allowing incompatible combinations
(e.g., surface: "CLI" with type "card"/"changelog" or surface: "WEBAPP" with
type "log"); update validateSurfaceFields to also read the payload.data.type
(payload.data.type) and add ctx.addIssue when surface and type are mismatched:
reject surface === "CLI" if type is "card" or "changelog" and reject surface ===
"WEBAPP" if type is "log" (use the same issue shape as existing checks and
reference CLI_ONLY_FIELDS, validateSurfaceFields, and payload.data.type when
locating where to add the new checks).
dc9619f to
abc723f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| {/* lgtm[js/xss-through-dom] React JSX sets src via setAttribute, not raw HTML interpolation — safe from XSS */} | ||
| {image && ( | ||
| <img src={image} alt="" className="mt-1.5 rounded px-2 pb-2" /> |
Check failure
Code scanning / CodeQL
DOM text reinterpreted as HTML High
Copilot Autofix
AI about 16 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/cli-v3/src/utilities/discoveryCheck.ts (1)
81-120: Deduplicate matched files before content scanning.When multiple patterns resolve to the same file, the file is scanned repeatedly. Convert
matchedto aSetto avoid duplicate I/O and duplicate regex checks.Suggested refactor
async function resolveFilePatterns( patterns: string[], projectRoot: string ): Promise<string[]> { - const matched: string[] = []; + const matched = new Set<string>(); @@ - matched.push(...files); + for (const file of files) matched.add(file); @@ if (exists) { - matched.push(absolutePath); + matched.add(absolutePath); } @@ - return matched; + return [...matched]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-v3/src/utilities/discoveryCheck.ts` around lines 81 - 120, The current discovery loop accumulates file paths into the matched array causing duplicate scans; change matched from an array to a Set (e.g., matchedSet) and use matchedSet.add(...) when processing both glob results (iterate files and add each) and literal path hits, then return Array.from(matchedSet) at the end; update any variable references from matched to matchedSet in the function (the loop that processes patterns, the branches for glob and literal checks, and the final return) so duplicates are eliminated before content scanning.apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx (1)
162-188: Consider addingdata-actionattributes for analytics consistency.Individual changelog entries lack
data-actionattributes, while "Full changelog" hasdata-action="full-changelog". Consider adding tracking attributes to individual items for consistent analytics.♻️ Optional: Add data-action for individual changelog entries
{changelogs.map((entry) => ( <SideMenuItem key={entry.id} name={entry.title} icon={GrayDotIcon} trailingIcon={ArrowUpRightIcon} trailingIconClassName="text-text-dimmed" inactiveIconColor="text-text-dimmed" activeIconColor="text-text-dimmed" to={entry.actionUrl ?? "https://trigger.dev/changelog"} target="_blank" + data-action={`changelog-${entry.id}`} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx` around lines 162 - 188, Changelogs list items rendered by the SideMenuItem inside the changelogs.map lack a data-action attribute while the standalone "Full changelog" has data-action="full-changelog"; update the map to pass a consistent analytics attribute (e.g. data-action={`changelog-${entry.id}`} or a stable slug) into each SideMenuItem so each entry (referenced by changelogs, entry.id, entry.title and the SideMenuItem component) emits a data-action for tracking, ensuring the attribute is included alongside existing props like to and target.apps/webapp/app/routes/resources.platform-changelogs.tsx (1)
28-43: Missingfetcherin useEffect dependencies may cause stale closure issues.The
useEffectreferencesfetcher.stateandfetcher.loadbut has an empty dependency array. While this works becausefetcherobject identity is stable in Remix, the missing dependency will trigger ESLint warnings. Consider adding a comment to suppress or restructure.♻️ Optional: Suppress ESLint warning with comment
return () => clearInterval(interval); + // eslint-disable-next-line react-hooks/exhaustive-deps -- fetcher reference is stable }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/resources.platform-changelogs.tsx` around lines 28 - 43, The useEffect uses fetcher.state and fetcher.load (and hasInitiallyFetched/POLL_INTERVAL_MS) but has an empty dependency array, causing an ESLint react-hooks/exhaustive-deps warning and potential stale-closure problems; update the effect to either include fetcher in the dependency array (e.g., useEffect(..., [fetcher])) or, if fetcher identity is guaranteed stable in Remix, add a single-line suppression comment for the rule above the useEffect to silence the warning (keep hasInitiallyFetched and POLL_INTERVAL_MS unchanged) while ensuring fetcher.load and fetcher.state are referenced safely.apps/webapp/app/components/navigation/NotificationPanel.tsx (2)
55-65: Consider adding fetchers to useCallback dependencies.The
handleDismiss,fireClickBeacon, andfireSeenBeaconcallbacks usedismissFetcher,clickFetcher, andseenFetcherrespectively but have empty dependency arrays. While this works because fetcher references are stable in Remix, it may trigger ESLint warnings.Also applies to: 67-78, 81-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 55 - 65, The callbacks handleDismiss, fireClickBeacon, and fireSeenBeacon currently have empty useCallback dependency arrays but reference dismissFetcher, clickFetcher, and seenFetcher; update each useCallback to include its respective fetcher (dismissFetcher for handleDismiss, clickFetcher for fireClickBeacon, seenFetcher for fireSeenBeacon) in the dependency array so ESLint no longer warns and the closures capture the correct references; if you intentionally rely on Remix-stable fetcher identities, alternatively add an explicit eslint-disable-next-line comment for the exhaustive-deps rule on those lines and document why.
42-44: Consider removing type assertion in favor of typed hook.The type assertion
as { notifications: Notification[] }bypasses type safety. If the hook's return type doesn't match, this could cause runtime issues.♻️ Suggested approach
Consider updating
usePlatformNotificationsto return a properly typed result, or at minimum add a runtime check:- const { notifications } = usePlatformNotifications(organizationId, projectId) as { - notifications: Notification[]; - }; + const { notifications } = usePlatformNotifications(organizationId, projectId); + // Ensure the hook returns properly typed notificationsThis would require updating the hook's return type in
resources.platform-notifications.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 42 - 44, Remove the unsafe type assertion in NotificationPanel.tsx (the "as { notifications: Notification[] }" on usePlatformNotifications) and instead make the hook return a properly typed result; update the return type in resources.platform-notifications.tsx so usePlatformNotifications returns { notifications: Notification[] } (or a discriminated union) and adjust the call in NotificationPanel to const { notifications } = usePlatformNotifications(organizationId, projectId); if you cannot change the hook immediately add a minimal runtime check after the call to assert notifications is an array of Notification objects before using it.apps/webapp/app/services/platformNotifications.server.ts (1)
396-399: Redundant sort after Prisma-ordered query.The Prisma query on lines 383-397 already orders by
[{ priority: "desc" }, { createdAt: "desc" }]. Creating a copy and re-sorting withcompareNotifications(which applies the same ordering logic) is unnecessary overhead.♻️ Proposed simplification
- const sorted = [...notifications].sort(compareNotifications); - - for (const n of sorted) { + for (const n of notifications) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 396 - 399, The code copies the Prisma-returned notifications into sorted and re-sorts with compareNotifications even though the Prisma query already uses orderBy [{ priority: "desc" }, { createdAt: "desc" }]; remove the redundant copy-and-sort: stop creating sorted and use the original notifications result directly (or if a copy is required for immutability, simply spread into a variable but do not call .sort with compareNotifications). Update any downstream uses of sorted to reference notifications (or the immutable copy) and remove the compareNotifications call here.apps/webapp/app/routes/admin.notifications.tsx (1)
174-186: Consider adding existence check before archive update.If
notificationIdreferences a non-existent record, Prisma will throw aRecordNotFounderror that surfaces as a 500. For admin tooling with low concurrency risk this is acceptable, but afindUniquecheck would provide a cleaner 400 response.💡 Optional improvement
async function handleArchiveAction(formData: FormData) { const notificationId = formData.get("notificationId") as string; if (!notificationId) { return typedjson({ error: "Missing notificationId" }, { status: 400 }); } + const exists = await prisma.platformNotification.findUnique({ + where: { id: notificationId }, + select: { id: true }, + }); + if (!exists) { + return typedjson({ error: "Notification not found" }, { status: 400 }); + } + await prisma.platformNotification.update({ where: { id: notificationId }, data: { archivedAt: new Date() }, }); return typedjson({ success: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/admin.notifications.tsx` around lines 174 - 186, The handleArchiveAction function should verify the notification exists before calling prisma.platformNotification.update: use prisma.platformNotification.findUnique or findFirst to look up the record by the notificationId, return a typedjson error (e.g., 400 or 404) if not found, and only then call prisma.platformNotification.update to set archivedAt; reference the notificationId variable and prisma.platformNotification.update/findUnique in your change so the lookup and early return are added immediately before the update call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/routes/admin.api.v1.platform-notifications.ts`:
- Around line 45-46: Wrap the call to request.json() in a try-catch inside the
route handler so malformed JSON doesn't throw uncaught and cause a 500; catch
JSON parse errors (SyntaxError) and return a 400 Bad Request with a clear
message, then only call createPlatformNotification(body as
CreatePlatformNotificationInput) when JSON parsing succeeds, rethrow or
propagate unexpected errors as appropriate.
In `@packages/cli-v3/src/apiClient.ts`:
- Around line 61-86: The CliPlatformNotificationResponseSchema's inner data.type
enum is missing the "card" and "changelog" notification types which causes Zod
validation to drop server-sent notifications; update the enum in
CliPlatformNotificationResponseSchema (the z.enum([...]) inside payload.data) to
include "card" and "changelog" so it matches the server's CardDataV1Schema, then
create a public-package changeset for packages/cli-v3 by running pnpm run
changeset:add and describe the schema change.
In `@packages/cli-v3/src/utilities/discoveryCheck.ts`:
- Around line 127-146: The code currently constructs new RegExp(contentPattern)
inside the files loop which risks ReDoS and runtime exceptions; pre-compile and
validate the pattern once before iterating: if useFastPath is false, wrap new
RegExp(contentPattern) in a try/catch, log and return/reject on invalid
patterns, store the compiled regex (e.g. compiledPattern) and then inside the
for loop use compiledPattern.test(content) instead of constructing a new RegExp
each iteration; keep the original content.includes(contentPattern) branch for
useFastPath.
---
Nitpick comments:
In `@apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx`:
- Around line 162-188: Changelogs list items rendered by the SideMenuItem inside
the changelogs.map lack a data-action attribute while the standalone "Full
changelog" has data-action="full-changelog"; update the map to pass a consistent
analytics attribute (e.g. data-action={`changelog-${entry.id}`} or a stable
slug) into each SideMenuItem so each entry (referenced by changelogs, entry.id,
entry.title and the SideMenuItem component) emits a data-action for tracking,
ensuring the attribute is included alongside existing props like to and target.
In `@apps/webapp/app/components/navigation/NotificationPanel.tsx`:
- Around line 55-65: The callbacks handleDismiss, fireClickBeacon, and
fireSeenBeacon currently have empty useCallback dependency arrays but reference
dismissFetcher, clickFetcher, and seenFetcher; update each useCallback to
include its respective fetcher (dismissFetcher for handleDismiss, clickFetcher
for fireClickBeacon, seenFetcher for fireSeenBeacon) in the dependency array so
ESLint no longer warns and the closures capture the correct references; if you
intentionally rely on Remix-stable fetcher identities, alternatively add an
explicit eslint-disable-next-line comment for the exhaustive-deps rule on those
lines and document why.
- Around line 42-44: Remove the unsafe type assertion in NotificationPanel.tsx
(the "as { notifications: Notification[] }" on usePlatformNotifications) and
instead make the hook return a properly typed result; update the return type in
resources.platform-notifications.tsx so usePlatformNotifications returns {
notifications: Notification[] } (or a discriminated union) and adjust the call
in NotificationPanel to const { notifications } =
usePlatformNotifications(organizationId, projectId); if you cannot change the
hook immediately add a minimal runtime check after the call to assert
notifications is an array of Notification objects before using it.
In `@apps/webapp/app/routes/admin.notifications.tsx`:
- Around line 174-186: The handleArchiveAction function should verify the
notification exists before calling prisma.platformNotification.update: use
prisma.platformNotification.findUnique or findFirst to look up the record by the
notificationId, return a typedjson error (e.g., 400 or 404) if not found, and
only then call prisma.platformNotification.update to set archivedAt; reference
the notificationId variable and prisma.platformNotification.update/findUnique in
your change so the lookup and early return are added immediately before the
update call.
In `@apps/webapp/app/routes/resources.platform-changelogs.tsx`:
- Around line 28-43: The useEffect uses fetcher.state and fetcher.load (and
hasInitiallyFetched/POLL_INTERVAL_MS) but has an empty dependency array, causing
an ESLint react-hooks/exhaustive-deps warning and potential stale-closure
problems; update the effect to either include fetcher in the dependency array
(e.g., useEffect(..., [fetcher])) or, if fetcher identity is guaranteed stable
in Remix, add a single-line suppression comment for the rule above the useEffect
to silence the warning (keep hasInitiallyFetched and POLL_INTERVAL_MS unchanged)
while ensuring fetcher.load and fetcher.state are referenced safely.
In `@apps/webapp/app/services/platformNotifications.server.ts`:
- Around line 396-399: The code copies the Prisma-returned notifications into
sorted and re-sorts with compareNotifications even though the Prisma query
already uses orderBy [{ priority: "desc" }, { createdAt: "desc" }]; remove the
redundant copy-and-sort: stop creating sorted and use the original notifications
result directly (or if a copy is required for immutability, simply spread into a
variable but do not call .sort with compareNotifications). Update any downstream
uses of sorted to reference notifications (or the immutable copy) and remove the
compareNotifications call here.
In `@packages/cli-v3/src/utilities/discoveryCheck.ts`:
- Around line 81-120: The current discovery loop accumulates file paths into the
matched array causing duplicate scans; change matched from an array to a Set
(e.g., matchedSet) and use matchedSet.add(...) when processing both glob results
(iterate files and add each) and literal path hits, then return
Array.from(matchedSet) at the end; update any variable references from matched
to matchedSet in the function (the loop that processes patterns, the branches
for glob and literal checks, and the final return) so duplicates are eliminated
before content scanning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b401b83d-7e14-4f2e-a805-680507d6a0d6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/routes/admin.tsxapps/webapp/app/routes/api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/routes/resources.platform-notifications.$id.clicked.tsxapps/webapp/app/routes/resources.platform-notifications.$id.dismiss.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.tsapps/webapp/package.jsoninternal-packages/database/prisma/migrations/20260223123615_add_platform_notification_tables/migration.sqlinternal-packages/database/prisma/schema.prismapackages/cli-v3/src/apiClient.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/commands/login.tspackages/cli-v3/src/utilities/colorMarkup.test.tspackages/cli-v3/src/utilities/colorMarkup.tspackages/cli-v3/src/utilities/discoveryCheck.test.tspackages/cli-v3/src/utilities/discoveryCheck.tspackages/cli-v3/src/utilities/platformNotifications.ts
✅ Files skipped from review due to trivial changes (6)
- apps/webapp/app/routes/admin.tsx
- apps/webapp/app/routes/resources.platform-notifications.$id.clicked.tsx
- packages/cli-v3/src/utilities/colorMarkup.ts
- apps/webapp/package.json
- apps/webapp/app/routes/api.v1.platform-notifications.ts
- internal-packages/database/prisma/migrations/20260223123615_add_platform_notification_tables/migration.sql
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/webapp/app/routes/resources.platform-notifications.$id.dismiss.tsx
- packages/cli-v3/src/utilities/colorMarkup.test.ts
- packages/cli-v3/src/commands/dev.ts
- packages/cli-v3/src/utilities/platformNotifications.ts
- packages/cli-v3/src/utilities/discoveryCheck.test.ts
- apps/webapp/app/routes/resources.platform-notifications.tsx
- internal-packages/database/prisma/schema.prisma
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: For apps and internal packages (apps/*,internal-packages/*), usepnpm run typecheck --filter <package>for verification, never usebuildas it proves almost nothing about correctness
Use testcontainers helpers (redisTest,postgresTest,containerTestfrom@internal/testcontainers) for integration tests with Redis and PostgreSQL instead of mocking
When writing Trigger.dev tasks, always import from@trigger.dev/sdk- never use@trigger.dev/sdk/v3or deprecatedclient.defineJob
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxpackages/cli-v3/src/apiClient.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxpackages/cli-v3/src/commands/login.tsapps/webapp/app/services/platformNotifications.server.tspackages/cli-v3/src/utilities/discoveryCheck.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Use pnpm for package management in this monorepo (version 10.23.0) with Turborepo for orchestration - run commands from root withpnpm run
Add crumbs as you write code for debug tracing using//@Crumbscomments or `// `#region` `@crumbsblocks - they stay on the branch throughout development and are stripped viaagentcrumbs stripbefore merge
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxpackages/cli-v3/src/apiClient.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxpackages/cli-v3/src/commands/login.tsapps/webapp/app/services/platformNotifications.server.tspackages/cli-v3/src/utilities/discoveryCheck.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxpackages/cli-v3/src/apiClient.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxpackages/cli-v3/src/commands/login.tsapps/webapp/app/services/platformNotifications.server.tspackages/cli-v3/src/utilities/discoveryCheck.ts
apps/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying only server components (
apps/webapp/,apps/supervisor/, etc.) with no package changes, add a.server-changes/file instead of a changeset
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
apps/webapp/app/**/*.{ts,tsx,server.ts}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Access environment variables via
envexport fromapp/env.server.ts. Never useprocess.envdirectly
Files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/cli-v3/src/apiClient.tsapps/webapp/app/routes/admin.api.v1.platform-notifications.tspackages/cli-v3/src/commands/login.tsapps/webapp/app/services/platformNotifications.server.tspackages/cli-v3/src/utilities/discoveryCheck.ts
packages/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/**/*.{ts,tsx,js,jsx}: For public packages (packages/*), usepnpm run build --filter <package>for verification
Add a changeset viapnpm run changeset:addwhen modifying any public package (packages/*orintegrations/*) - default to patch for bug fixes and minor changes
Files:
packages/cli-v3/src/apiClient.tspackages/cli-v3/src/commands/login.tspackages/cli-v3/src/utilities/discoveryCheck.ts
apps/webapp/app/routes/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Use Remix flat-file route convention with dot-separated segments where
api.v1.tasks.$taskId.trigger.tsmaps to/api/v1/tasks/:taskId/trigger
Files:
apps/webapp/app/routes/admin.api.v1.platform-notifications.ts
packages/cli-v3/src/commands/**/*
📄 CodeRabbit inference engine (packages/cli-v3/CLAUDE.md)
CLI command definitions should be located in
src/commands/
Files:
packages/cli-v3/src/commands/login.ts
packages/cli-v3/src/commands/login.ts
📄 CodeRabbit inference engine (packages/cli-v3/CLAUDE.md)
Implement
login.tscommand insrc/commands/for authentication
Files:
packages/cli-v3/src/commands/login.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/platformNotifications.server.ts
🧠 Learnings (34)
📚 Learning: 2026-03-22T13:45:34.388Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/navigation/SideMenu.tsx:460-489
Timestamp: 2026-03-22T13:45:34.388Z
Learning: In triggerdotdev/trigger.dev, sidebar navigation items (SideMenu.tsx) are intentionally NOT gated behind feature-flag or permission checks at the nav level. Authorization is enforced at the route/loader level instead. Hiding nav items based on access checks is considered confusing UX. This applies to items like "AI Metrics" (v3BuiltInDashboardPath) and other dashboard links — they are always rendered in the sidebar regardless of hasQueryAccess or similar flags.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxpackages/cli-v3/src/apiClient.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxpackages/cli-v3/src/commands/login.tsapps/webapp/app/services/platformNotifications.server.tspackages/cli-v3/src/utilities/discoveryCheck.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxpackages/cli-v3/src/apiClient.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.notifications.tsxpackages/cli-v3/src/commands/login.tsapps/webapp/app/services/platformNotifications.server.tspackages/cli-v3/src/utilities/discoveryCheck.ts
📚 Learning: 2026-03-23T06:24:25.016Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:25.016Z
Learning: Applies to apps/webapp/app/routes/**/*.ts : Use Remix flat-file route convention with dot-separated segments where `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`
Applied to files:
apps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2026-03-23T06:24:14.555Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.555Z
Learning: Applies to packages/**/*.{ts,tsx,js,jsx} : Add a changeset via `pnpm run changeset:add` when modifying any public package (`packages/*` or `integrations/*`) - default to patch for bug fixes and minor changes
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/src/build/**/* : Build system in `src/build/` should use configuration from `trigger.config.ts` in user projects to determine bundling, build extensions, and output structure
Applied to files:
packages/cli-v3/src/apiClient.tsapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/.claude/skills/trigger-dev-tasks/**/* : Update `.claude/skills/trigger-dev-tasks/` in parallel with `rules/` when SDK features change
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/src/commands/dev.ts : Implement `dev.ts` command in `src/commands/` for local development mode
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/rules/**/* : Update `rules/` directory with versioned SDK documentation when SDK features change
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/src/commands/promote.ts : Implement `promote.ts` command in `src/commands/` for deployment promotion
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/src/dev/**/* : Dev mode code should be located in `src/dev/` and runs tasks locally in the user's Node.js process without containers
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `task()` function from `trigger.dev/sdk/v3` to define tasks with id and run properties
Applied to files:
packages/cli-v3/src/apiClient.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Use `useRun`, `useRealtimeRun` and other SWR/realtime hooks from `trigger.dev/react-hooks` for data fetching
Applied to files:
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx
📚 Learning: 2026-01-28T16:57:47.620Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 2964
File: apps/webapp/app/components/AskAI.tsx:121-141
Timestamp: 2026-01-28T16:57:47.620Z
Learning: In the trigger.dev webapp codebase, the Button component (apps/webapp/app/components/primitives/Buttons) does not spread unknown props to the underlying <button> element—it only passes specific props (type, disabled, onClick, name, value, ref, form, autoFocus). When using Radix UI's TooltipTrigger with asChild, a span wrapper around the Button is necessary to receive Radix props (aria-describedby, onPointerEnter, onPointerLeave, data-state) while the Button handles its own behavior. Directly making the Button the child of TooltipTrigger with asChild will break accessibility and tooltip functionality.
Applied to files:
apps/webapp/app/components/navigation/NotificationPanel.tsx
📚 Learning: 2026-03-13T13:45:39.411Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.missing.$model.tsx:19-21
Timestamp: 2026-03-13T13:45:39.411Z
Learning: In `apps/webapp/app/routes/admin.llm-models.missing.$model.tsx`, the `decodeURIComponent(params.model ?? "")` call is intentionally unguarded. Remix route params are decoded at the routing layer before reaching the loader, so malformed percent-encoding is rejected upstream. The page is also admin-only, so the risk is minimal and no try-catch is warranted.
Applied to files:
apps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2026-03-13T13:42:25.092Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.new.tsx:65-91
Timestamp: 2026-03-13T13:42:25.092Z
Learning: In `apps/webapp/app/routes/admin.llm-models.new.tsx`, sequential Prisma writes for model/tier creation are intentionally not wrapped in a transaction. The form is admin-only with low concurrency risk, and the blast radius is considered minimal for admin tooling.
Applied to files:
apps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2026-03-22T19:26:46.247Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: internal-packages/emails/emails/alert-error-group.tsx:58-58
Timestamp: 2026-03-22T19:26:46.247Z
Learning: In the triggerdotdev/trigger.dev codebase, email template files under `internal-packages/emails/emails/` must use `export default function Email(...)` (default export) because the React Email previewer requires a default export to discover and render templates. Do not flag default exports in these files as violations of the "use named exports" coding guideline.
Applied to files:
apps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2026-03-23T06:24:14.555Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.555Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Add crumbs as you write code for debug tracing using `// crumbs` comments or `// `#region` crumbs` blocks - they stay on the branch throughout development and are stripped via `agentcrumbs strip` before merge
Applied to files:
apps/webapp/app/routes/admin.notifications.tsx
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/src/commands/login.ts : Implement `login.ts` command in `src/commands/` for authentication
Applied to files:
packages/cli-v3/src/commands/login.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/services/**/*.server.{ts,tsx} : Separate testable services from configuration files; follow the pattern of `realtimeClient.server.ts` (testable service) and `realtimeClientGlobal.server.ts` (configuration) in the webapp
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-02T12:43:37.906Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/core/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:37.906Z
Learning: Exercise caution with changes to trigger.dev/core as they affect both the customer-facing SDK and server-side webapp - breaking changes can impact deployed user tasks and the platform simultaneously
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-23T06:24:14.555Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.555Z
Learning: Applies to apps/**/*.{ts,tsx,js,jsx} : When modifying only server components (`apps/webapp/`, `apps/supervisor/`, etc.) with no package changes, add a `.server-changes/` file instead of a changeset
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-23T06:24:25.016Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:25.016Z
Learning: Applies to apps/webapp/app/v3/services/**/*.server.ts : Only modify V2 code paths when editing services that branch on `RunEngineVersion` to support both V1 and V2 (e.g., `cancelTaskRun.server.ts`, `batchTriggerV3.server.ts`)
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-10T12:44:19.869Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3200
File: docs/config/config-file.mdx:353-368
Timestamp: 2026-03-10T12:44:19.869Z
Learning: In the triggerdotdev/trigger.dev repository, docs PRs are often written as companions to implementation PRs (e.g., PR `#3200` documents features being added in PR `#3196`). When reviewing docs PRs, the documented features may exist in a companion/companion PR branch rather than main. Always check companion PRs referenced in the PR description before flagging missing implementations.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `trigger.dev/sdk/v3` for all imports in Trigger.dev tasks
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-23T06:24:14.555Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.555Z
Learning: Applies to apps/webapp/app/v3/**/*.{ts,tsx} : In the webapp v3 directory, only modify V2 code paths when encountering V1/V2 branching in services - all new work uses Run Engine 2.0 (`internal/run-engine`) and redis-worker, not legacy V1 engine code
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-01-28T14:15:15.011Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2953
File: apps/webapp/app/components/runs/v3/SharedFilters.tsx:441-452
Timestamp: 2026-01-28T14:15:15.011Z
Learning: In apps/webapp/app/components/runs/v3/SharedFilters.tsx, the maxPeriodDays limit for date ranges should only check the from date (via dateRangeToDays(fromValue)) because it enforces data retention limits—how far back in history queries can reach. The to date is irrelevant for retention-based limits.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T19:27:26.469Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts:104-112
Timestamp: 2026-03-22T19:27:26.469Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts`, the `#scheduleErrorAlertEvaluation` helper intentionally uses the same job id (`evaluateErrorAlerts:${projectId}`) as the evaluator's periodic self-chain. The deduplication is desired: if a future run is already queued, the immediate enqueue becomes a no-op, preventing two evaluations firing in quick succession. Do not flag this as a bug or suggest a unique/timestamped id.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T19:32:14.433Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx:82-151
Timestamp: 2026-03-22T19:32:14.433Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts` and `apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx`, the `errorAlertConfig` field on `ProjectAlertChannel` is intentionally `Json?` (nullable). The `ErrorAlertEvaluator.computeMinInterval()` in `apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts` uses `ErrorAlertConfig.safeParse(ch.errorAlertConfig)` and falls back to `DEFAULT_INTERVAL_MS = 300_000` when `errorAlertConfig` is null. No UI currently collects this value — it is scaffolding for a future per-channel evaluation interval feature. Do not flag the absence of `errorAlertConfig` in `CreateAlertChannelOptions` or the action handler as a bug; null configs are safe and expected.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
🪛 ast-grep (0.41.1)
apps/webapp/app/services/platformNotifications.server.ts
[warning] 16-16: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(val)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
packages/cli-v3/src/utilities/discoveryCheck.ts
[warning] 144-144: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(contentPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Check: CodeQL
apps/webapp/app/routes/admin.notifications.tsx
[failure] 717-717: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
🔇 Additional comments (22)
apps/webapp/app/components/navigation/NotificationPanel.tsx (3)
277-287: Nested anchor click propagates to parent card click handler.When clicking a markdown link inside the card, the event bubbles up to the card's
onClick(line 214), potentially triggeringhandleCardClick(which may dismiss the card ifdismissOnActionis true). Consider stopping propagation in the markdown link handler.🔧 Suggested fix (from past review)
a: ({ href, children }: { href?: string; children?: React.ReactNode }) => ( <a href={href} target="_blank" rel="noopener noreferrer" className="text-indigo-400 underline hover:text-indigo-300 transition-colors" - onClick={onLinkClick} + onClick={(e) => { + e.stopPropagation(); + onLinkClick(); + }} > {children} </a> ),
12-29: LGTM on type definition.Clean type definition using
typeinstead ofinterfaceper coding guidelines.
94-99: LGTM on seen beacon effect.Correctly fires the seen beacon when a notification is present and no incident is active. The dependency on
notification?.idensures it re-fires only when the notification changes.apps/webapp/app/components/navigation/SideMenu.tsx (1)
55-55: LGTM!Clean integration of the
NotificationPanelinto the side menu. The component is correctly placed after theV3DeprecationPaneland receives the necessary props (isCollapsed,hasIncident,organizationId,projectId) from existing state.Also applies to: 689-694
apps/webapp/app/routes/resources.platform-notifications.$id.seen.tsx (1)
6-16: LGTM!The action handler correctly authenticates the user, validates the notification ID parameter, and calls the service function. The pattern is clean and follows Remix conventions.
packages/cli-v3/src/commands/login.ts (2)
321-321: LGTM!The notification display is appropriately placed after successful login completion but before returning the result, giving the user relevant platform updates at login time.
292-295: The notification fetch pattern is correctly implemented with proper error handling.fetchPlatformNotificationusestryCatchto catch all errors and returnsundefinedrather than throwing, with a 7-second timeout to prevent hangs. Errors are logged at debug level and don't interrupt the login flow.awaitAndDisplayPlatformNotificationsafely handles theundefinedcase. No changes needed.apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx (1)
196-202: LGTM!Simple, focused helper component for the visual dot indicator.
packages/cli-v3/src/apiClient.ts (1)
570-587: LGTM on the method implementation.Good defensive pattern returning
{ success: true, data: { notification: null } }when no access token is present, preventing errors in unauthenticated contexts. Thesignalparameter enables timeout control from the caller.apps/webapp/app/routes/admin.api.v1.platform-notifications.ts (1)
13-32: LGTM on authentication logic.The
authenticateAdminfunction properly separates concerns and returns appropriate error codes: 401 for missing/invalid tokens and missing users, 403 for non-admin authenticated users.apps/webapp/app/routes/resources.platform-changelogs.tsx (1)
14-20: LGTM on loader implementation.Clean authentication and data fetching with proper typed JSON response.
apps/webapp/app/routes/admin.notifications.tsx (4)
715-718: CodeQL flag is a false positive.The static analysis warning about "DOM text reinterpreted as HTML" does not apply here. React's JSX sets the
srcattribute viasetAttribute, not innerHTML/dangerouslySetInnerHTML. Additionally,imgtags do not executejavascript:URIs in theirsrcattribute in modern browsers. The existing lgtm comment correctly documents this reasoning.
42-74: LGTM! Loader and action structure is well-organized.Admin access validation is correctly applied in both loader and action. The search params validation using Zod and the action discriminator pattern (
_action) are clean.
76-172: LGTM! Create action handles form data and error mapping correctly.The handler properly distinguishes validation errors (400) from database errors (500) and handles preview mode appropriately with user-scoped, time-limited notifications.
644-722: LGTM! Preview card implementation with fixed JSX.The
NotificationPreviewCardcomponent correctly implements the live markdown preview with proper overflow detection viauseLayoutEffect. The JSX comment placement issue from the previous review has been addressed—the lgtm comment is now correctly positioned outside the conditional expression.apps/webapp/app/services/platformNotifications.server.ts (7)
8-25: ReDoS risk with arbitrary regex patterns persisted to database.This concern was raised in a previous review and remains unaddressed. The schema validates that
contentPatterncompiles as a regex, but does not guard against catastrophically backtracking patterns (e.g.,(a+)+$) that could hang CLI clients when executed against local files.
277-294: Audience filtering missing in changelog query.This concern was raised in a previous review and remains unaddressed.
getRecentChangelogsignores scope, archived status, and time window constraints, potentially exposing USER-scoped previews or expired/archived changelogs to all users.
298-319:showCountincrements before display check, consuming max-show budget.This concern was raised in a previous review and remains unaddressed. With
cliShowEvery=3andcliMaxShowCount=5, the notification expires after 5 requests even though it was only displayed ~2 times.
503-523: Missing validation ofpayload.data.typeagainstsurface.This concern was raised in a previous review and remains unaddressed. The current validation allows
surface: "CLI"withtype: "card"/"changelog"and vice versa, which will fail Zod parsing on the CLI client (which only accepts["info", "warn", "error", "success"]).
57-120: LGTM! Admin list with interaction stats is well-structured.The pagination, optional archive filtering, and stats aggregation (seen/clicked/dismissed) are cleanly implemented. Gracefully handles invalid payloads by returning
nullfor derived fields.
124-200: LGTM! Active notifications query with scope filtering.Proper filtering by surface, time window, archival status, and scope hierarchy (GLOBAL → ORGANIZATION → PROJECT → USER). The dismissal check and payload validation before inclusion are correct.
448-579: LGTM! Creation schema with comprehensive cross-field validation.The
superRefinevalidators correctly enforce scope-FK relationships, surface-specific fields, and startsAt constraints. UsingResultAsyncfrom neverthrow provides clean error discrimination between validation and DB failures.
| const useFastPath = !REGEX_METACHARACTERS.test(contentPattern); | ||
|
|
||
| logger.debug("Discovery: checking content pattern", { | ||
| contentPattern, | ||
| useFastPath, | ||
| fileCount: files.length, | ||
| }); | ||
|
|
||
| for (const filePath of files) { | ||
| const [error, content] = await tryCatch(readFile(filePath)); | ||
|
|
||
| if (error) { | ||
| logger.debug("Discovery: failed to read file, skipping", { filePath, error }); | ||
| continue; | ||
| } | ||
|
|
||
| const matches = useFastPath | ||
| ? content.includes(contentPattern) | ||
| : new RegExp(contentPattern).test(content); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate discovery-related files =="
fd 'discoveryCheck\.ts$|discoveryCheck\.test\.ts$|platformNotifications\.ts$|.*notification.*\.ts$'
echo
echo "== Inspect discovery/contentPattern schema and validation =="
rg -n -C4 'contentPattern|discovery|zod|z\.string|max\(|regex\('
echo
echo "== Find all dynamic RegExp constructions in CLI package =="
rg -n -C2 'new RegExp\(' packages/cli-v3Repository: triggerdotdev/trigger.dev
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full implementation of checkContentPattern ==="
sed -n '120,160p' packages/cli-v3/src/utilities/discoveryCheck.ts
echo
echo "=== DiscoverySpec type and contentPattern definition ==="
sed -n '1,50p' packages/cli-v3/src/utilities/discoveryCheck.ts
echo
echo "=== Look for upstream DiscoverySpec schema definition ==="
rg -A10 'interface DiscoverySpec|type DiscoverySpec' packages/cli-v3
echo
echo "=== Check if contentPattern is validated upstream ==="
rg -B5 -A5 'contentPattern' packages/cli-v3/src/utilities/discoveryCheck.ts | head -60Repository: triggerdotdev/trigger.dev
Length of output: 5956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Test file to understand contentPattern usage ==="
cat packages/cli-v3/src/utilities/discoveryCheck.test.ts | head -100
echo
echo "=== Check where/how contentPattern gets set (callers) ==="
rg -B3 -A3 'checkContentPattern' packages/cli-v3 | head -80Repository: triggerdotdev/trigger.dev
Length of output: 4386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for DiscoverySpec creation/parsing (especially from network/files) ==="
rg -B5 -A10 'DiscoverySpec|contentPattern' packages/cli-v3 --type ts --type tsx | grep -E '(platform|notification|spec|eval|check|fetch|parse|API|server)' -A3 -B3 | head -150
echo
echo "=== Check platformNotifications file which was mentioned ==="
grep -n 'DiscoverySpec\|contentPattern\|evaluateDiscovery' packages/cli-v3/src/utilities/platformNotifications.ts
echo
echo "=== Look for any schema validation on discovery specs ==="
rg -n 'zod\|schema\|validate' packages/cli-v3/src/utilities/discoveryCheck.tsRepository: triggerdotdev/trigger.dev
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if contentPattern is ever user-provided or comes from API ==="
rg -B10 'evaluateDiscovery' packages/cli-v3 --type ts --type tsx | head -100Repository: triggerdotdev/trigger.dev
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for DiscoverySpec creation/parsing (especially from network/files) ==="
rg -B5 -A10 'DiscoverySpec|contentPattern' packages/cli-v3 | grep -E '(platform|notification|spec|eval|check|fetch|parse|API|server)' -A3 -B3 | head -150
echo
echo "=== Check platformNotifications file which was mentioned ==="
grep -n 'DiscoverySpec\|contentPattern\|evaluateDiscovery' packages/cli-v3/src/utilities/platformNotifications.ts
echo
echo "=== Look for any schema validation on discovery specs ==="
rg -n 'zod\|schema\|validate' packages/cli-v3/src/utilities/discoveryCheck.ts
echo
echo "=== Check if contentPattern is ever user-provided or comes from API ==="
rg -B10 'evaluateDiscovery' packages/cli-v3 | head -150Repository: triggerdotdev/trigger.dev
Length of output: 11534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check platformNotifications.ts ==="
cat packages/cli-v3/src/utilities/platformNotifications.ts
echo
echo "=== Find all usages of evaluateDiscovery ==="
rg 'evaluateDiscovery' packages/cli-v3 -B5 -A5Repository: triggerdotdev/trigger.dev
Length of output: 22091
Move regex compilation outside the file loop and validate pattern early.
At line 145, new RegExp(contentPattern) is constructed from externally-provided content inside the file iteration. A malicious pattern from the API can trigger catastrophic backtracking (ReDoS), causing the CLI to hang. Compile the regex once before the loop and catch invalid patterns with early rejection.
Suggested fix
async function checkContentPattern(
files: string[],
contentPattern: string
): Promise<boolean> {
const useFastPath = !REGEX_METACHARACTERS.test(contentPattern);
+ let compiledPattern: RegExp | undefined;
+
+ if (!useFastPath) {
+ try {
+ compiledPattern = new RegExp(contentPattern);
+ } catch (error) {
+ logger.debug("Discovery: invalid regex pattern, skipping discovery", {
+ contentPattern,
+ error,
+ });
+ return false;
+ }
+ }
logger.debug("Discovery: checking content pattern", {
contentPattern,
useFastPath,
fileCount: files.length,
});
for (const filePath of files) {
const [error, content] = await tryCatch(readFile(filePath));
if (error) {
logger.debug("Discovery: failed to read file, skipping", { filePath, error });
continue;
}
const matches = useFastPath
? content.includes(contentPattern)
- : new RegExp(contentPattern).test(content);
+ : compiledPattern!.test(content);
logger.debug("Discovery: content check result", { filePath, matches });
if (matches) {
return true;
}
}🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 144-144: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(contentPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli-v3/src/utilities/discoveryCheck.ts` around lines 127 - 146, The
code currently constructs new RegExp(contentPattern) inside the files loop which
risks ReDoS and runtime exceptions; pre-compile and validate the pattern once
before iterating: if useFastPath is false, wrap new RegExp(contentPattern) in a
try/catch, log and return/reject on invalid patterns, store the compiled regex
(e.g. compiledPattern) and then inside the for loop use
compiledPattern.test(content) instead of constructing a new RegExp each
iteration; keep the original content.includes(contentPattern) branch for
useFastPath.
Add PlatformNotification and PlatformNotificationInteraction Prisma models, including enums PlatformNotificationSurface and PlatformNotificationScope, indexes, timestamps, and fields for scoping (user, project, organization), lifecycle (startsAt, endsAt, archivedAt) and delivery behavior (CLI-specific fields, priority). Wire new relations into User, Organization, Project, OrgMember and Workspace models so notifications and interactions can be queried from those entities. Add SQL migration to create the new enums and adjust schema constraints and indexes required for the migration. The change enables admin-created, scoped platform notifications with per-user interaction tracking for both webapp and CLI surfaces. feat(notifications): Add a way to notify users on platform feat(notifications): Enforce notifications expired date, add CLI improvements CLI colors CLI show every n-th notification feat(webapp): Platform notifications ui/ux improvements feat(CLI): CLI notifications v1 feat(webapp): add dashboard platform notifications service & UI Introduce a new server-side service to read and record platform notifications targeted at the webapp. - Add payload schema (v1) using zod and typed PayloadV1. - Define PlatformNotificationWithPayload type and scope priority map. - Implement getActivePlatformNotifications to: - query active WEBAPP notifications with scope/org/project/user filters, - include user interactions and validate payloads, - filter dismissed items, compute unreadCount, and return sorted results. - Add helper functions: - findInteraction to match global/org interactions, - compareNotifications to sort by scope, priority, then recency. - Implement upsertInteraction to create or update platform notification interactions, handling GLOBAL-scoped interactions per organization. These changes centralize notification read/write logic, enforce payload validation, and provide deterministic ordering and unread counts for the webapp UI.
Introduce a new server-side service to read and record platform notifications targeted at the webapp. - Add payload schema (v1) using zod and typed PayloadV1. - Define PlatformNotificationWithPayload type and scope priority map. - Implement getActivePlatformNotifications to: - query active WEBAPP notifications with scope/org/project/user filters, - include user interactions and validate payloads, - filter dismissed items, compute unreadCount, and return sorted results. - Add helper functions: - findInteraction to match global/org interactions, - compareNotifications to sort by scope, priority, then recency. - Implement upsertInteraction to create or update platform notification interactions, handling GLOBAL-scoped interactions per organization. These changes centralize notification read/write logic, enforce payload validation, and provide deterministic ordering and unread counts for the webapp UI.
0ski
force-pushed
the
oskar/feat-platform-notifications
branch
from
abc723f to
1980052
Compare
There was a problem hiding this comment.
🚩 Help & Feedback popover removed Discord, Slack, and "Book a call" support options
The PR removes the entire "Need help?" section from the Help & Feedback popover, including the Slack Connect dialog for Pro subscribers, the "Ask in our Discord" link, and the "Book a 15 min call" link. Only the "Contact us" feedback button remains. This is likely intentional (replaced by the new notification system + changelogs section), but reviewers should confirm these support channels are accessible elsewhere, as Pro users may expect to find Slack access in this menu.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
packages/cli-v3/src/utilities/discoveryCheck.ts (1)
123-146:⚠️ Potential issue | 🟠 MajorPre-compile and validate
contentPatternregex before the file loop.At Line 145,
new RegExp(contentPattern)is built from API-provided input inside the per-file loop. This repeats compilation, and invalid/expensive patterns can throw or stall matching work unnecessarily. Compile once (withtry/catch) before iterating files, then reuse the compiled regex.Proposed fix
async function checkContentPattern( files: string[], contentPattern: string ): Promise<boolean> { const useFastPath = !REGEX_METACHARACTERS.test(contentPattern); + let compiledPattern: RegExp | undefined; + + if (!useFastPath) { + try { + compiledPattern = new RegExp(contentPattern); + } catch (error) { + logger.debug("Discovery: invalid regex pattern, skipping discovery", { + contentPattern, + error, + }); + return false; + } + } logger.debug("Discovery: checking content pattern", { contentPattern, useFastPath, fileCount: files.length, }); for (const filePath of files) { const [error, content] = await tryCatch(readFile(filePath)); if (error) { logger.debug("Discovery: failed to read file, skipping", { filePath, error }); continue; } const matches = useFastPath ? content.includes(contentPattern) - : new RegExp(contentPattern).test(content); + : compiledPattern!.test(content); logger.debug("Discovery: content check result", { filePath, matches });#!/bin/bash set -euo pipefail echo "=== checkContentPattern implementation ===" sed -n '123,160p' packages/cli-v3/src/utilities/discoveryCheck.ts echo echo "=== verify dynamic regex is inside file loop ===" rg -n -C3 'for \(const filePath of files\)|new RegExp\(contentPattern\)|try \{' packages/cli-v3/src/utilities/discoveryCheck.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-v3/src/utilities/discoveryCheck.ts` around lines 123 - 146, In checkContentPattern, precompile and validate the contentPattern regex once before iterating files: if useFastPath is false, wrap new RegExp(contentPattern) in a try/catch, assign the result to a local contentRegex (or null if invalid), and on error log via logger.debug/error with the pattern and return false (or decide no matches) so you don't compile per file; then inside the for loop replace new RegExp(contentPattern).test(content) with contentRegex.test(content) (or skip testing when contentRegex is null), keeping useFastPath logic unchanged.packages/cli-v3/src/apiClient.ts (1)
61-86:⚠️ Potential issue | 🟠 MajorAdd missing notification types to prevent validation failures.
The
typeenum is missing"card"and"changelog"which the server'sCardDataV1Schemaincludes. When the server sends notifications with these types, Zod validation will fail and silently drop them.Suggested fix
data: z.object({ - type: z.enum(["info", "warn", "error", "success"]), + type: z.enum(["card", "info", "warn", "error", "success", "changelog"]), title: z.string(), description: z.string(),Since this is in a public package (
packages/cli-v3), add a changeset viapnpm run changeset:add. As per coding guidelines: "Add a changeset viapnpm run changeset:addwhen modifying any public package (packages/*orintegrations/*)".,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-v3/src/apiClient.ts` around lines 61 - 86, Update the CliPlatformNotificationResponseSchema so its notification.payload.data.type enum includes the missing "card" and "changelog" values to match the server's CardDataV1Schema (locate the z.enum([...]) in CliPlatformNotificationResponseSchema) and run pnpm run changeset:add to create a changeset for the public package packages/cli-v3 as required by the project guidelines.apps/webapp/app/components/navigation/NotificationPanel.tsx (1)
277-287:⚠️ Potential issue | 🟡 MinorNested anchor propagation may trigger parent card click.
When clicking a markdown link inside the card, the event bubbles up to the card's
onClick(line 214), potentially triggeringhandleCardClick(which may dismiss the card ifdismissOnActionis true). Consider stopping propagation.Suggested fix
a: ({ href, children }: { href?: string; children?: React.ReactNode }) => ( <a href={href} target="_blank" rel="noopener noreferrer" className="text-indigo-400 underline hover:text-indigo-300 transition-colors" - onClick={onLinkClick} + onClick={(e) => { + e.stopPropagation(); + onLinkClick(); + }} > {children} </a> ),,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 277 - 287, Nested link clicks inside NotificationPanel currently bubble to the card's onClick (handleCardClick) and can trigger dismissal when dismissOnAction is true; update the anchor render (the inline component using onLinkClick) to stop event propagation before invoking onLinkClick (e.g., onClick={e => { e.stopPropagation(); onLinkClick?.(e); }}), so link clicks won't bubble to handleCardClick while preserving existing link behavior.apps/webapp/app/services/platformNotifications.server.ts (4)
277-294:⚠️ Potential issue | 🟠 MajorScope and time-filter the “What’s new” feed.
This helper ignores audience and lifecycle state, so USER/org/project changelogs and archived/future/expired rows can leak into the shared Help popover. It needs the same targeting and active-window filters as
getActivePlatformNotifications().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 277 - 294, getRecentChangelogs currently fetches changelogs without applying audience or lifecycle time-window filters, so update it to reuse the same targeting and active-window constraints as getActivePlatformNotifications() — either by calling getActivePlatformNotifications() and filtering for payload type "changelog" and taking the requested limit, or by copying the same where-clause logic into getRecentChangelogs' prisma.platformNotification.findMany call (include audience targeting and lifecycle active-window checks used by getActivePlatformNotifications). Keep the existing payload parsing via PayloadV1Schema and return shape (id, title, actionUrl) after applying those additional filters.
10-24:⚠️ Potential issue | 🟠 MajorDon't persist arbitrary discovery regexes.
Compiling
contentPatternhere only proves it parses. A catastrophically backtracking pattern can still be saved and later executed by every CLI client against local files, which can hangdev/login; regex mode needs an explicit opt-in plus safety checks instead of treating every value as a raw pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 10 - 24, The current schema is accepting any string as contentPattern after compile-check in the refine, which lets clients persist catastrophic regexes; change the validation to require an explicit opt-in flag (e.g., a boolean like regexMode or contentPatternIsRegex) and only allow a pattern when that flag is true, and add a safety check instead of just new RegExp — use a deterministic/safe-regex check (e.g., integrate safe-regex or validate against a whitelist and max length/complexity) before accepting the pattern in the contentPattern refine; update the schema to reference the opt-in flag and reject patterns that fail the safety check or exceed allowed complexity/length.
410-423:⚠️ Potential issue | 🟠 Major
cliShowEveryis burning visibility budget while hidden.The upsert happens before the modulo gate, so suppressed requests still increment
showCountand can startfirstSeenAt. That letscliMaxShowCount/cliMaxDaysAfterFirstSeenexpire a notification before it has actually been rendered the configured number of times.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 410 - 423, The upsert increments showCount and sets firstSeenAt before the cliShowEvery gate, so suppressed views still consume visibility and can trigger expiry rules; change the flow in platformNotifications.server.ts to first read the existing interaction (prisma.platformNotificationInteraction.findUnique for notificationId_userId) and compute nextShowCount = (existing?.showCount ?? 0) + 1, apply the n.cliShowEvery modulo check against nextShowCount and continue if it should be suppressed, and only then perform the upsert (or create) to increment showCount and set firstSeenAt = now when creating/first-seen; reference prisma.platformNotificationInteraction.upsert, updated.showCount, n.cliShowEvery, firstSeenAt, cliMaxShowCount, and cliMaxDaysAfterFirstSeen when making these changes.
503-523:⚠️ Potential issue | 🟠 MajorValidate
payload.data.typeagainstsurface.This currently blocks the CLI-only numeric fields on
WEBAPP, but it still allowssurface: "CLI"with card/changelog payloads andsurface: "WEBAPP"with log-style payloads. Those combinations fall through to unsupported renderers later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 503 - 523, validateSurfaceFields currently only blocks CLI-only numeric fields but doesn't ensure the payload shape matches the surface; update validateSurfaceFields to read data.payload?.data?.type and add ctx.addIssue when the type is incompatible with the surface (e.g., if data.surface === "WEBAPP" and payload.data.type is a log-style type, add an error on path ["payload","data","type"], and if data.surface === "CLI" and payload.data.type is a card/changelog type, add an error similarly); reference the validateSurfaceFields function, CLI_ONLY_FIELDS constant and the payload.data.type path when implementing these checks so unsupported surface/type combos are rejected early.
🧹 Nitpick comments (2)
packages/cli-v3/src/utilities/discoveryCheck.ts (1)
77-121: Consider deduplicatingmatchedFilesbefore content scanning.Overlapping literal/glob patterns can add the same path multiple times, causing repeated reads and duplicate debug entries. Returning unique paths from
resolveFilePatterns()would trim unnecessary I/O.Refactor sketch
- return matched; + return [...new Set(matched)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-v3/src/utilities/discoveryCheck.ts` around lines 77 - 121, resolveFilePatterns may return the same absolute path multiple times (from overlapping globs and literals); update resolveFilePatterns to normalize every absolute path (use path.normalize on values from glob results and the computed absolutePath) before pushing to matched and then return a deduplicated list (e.g., Array.from(new Set(matched)) or [...new Set(matched)]) so consumers only scan each file once; keep the rest of the logic and logging intact and reference the matched variable and resolveFilePatterns function.apps/webapp/app/routes/admin.notifications.tsx (1)
76-172: Consider extracting payload building into a helper.The
handleCreateActionfunction is quite long (~100 lines) with complex conditional logic for building the payload. While functional, extracting the payload construction into a separate function would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/admin.notifications.tsx` around lines 76 - 172, Extract the notification payload construction out of handleCreateAction into a helper function (e.g., buildNotificationPayload or constructPlatformNotificationPayload) that takes the FormData-derived values (surface, payloadType, adminLabel, title, description, actionUrl, image, dismissOnAction, discoveryFilePatterns, discoveryContentPattern, discoveryMatchBehavior, startsAt, endsAt, priority, cli fields, isPreview, userId) and returns the object passed to createPlatformNotification; move the discovery assembly and the surface/CLI-specific merges (including startsAt/endsAt logic and cliMax* handling) into that helper, keep handleCreateAction responsible only for validation, calling the helper, invoking createPlatformNotification, and handling result/error mapping, and update calls to createPlatformNotification to use the helper's return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/services/platformNotifications.server.ts`:
- Around line 68-88: The current findMany (prisma.platformNotification.findMany)
hydrates the full interactions relation for each notification; remove the
include: { interactions: ... } and instead run a separate aggregated query that
counts clicks/dismissals per notification id (e.g., using
prisma.platformNotification.groupBy or prisma.interaction.groupBy / aggregate to
_count with where clauses like webappClickedAt != null and webappDismissedAt !=
null) and then merge those aggregated counts into the notifications array before
returning; update the code around prisma.platformNotification.findMany and the
mapping/return logic so admin stats come from the aggregated counts rather than
loading every interaction row.
- Around line 359-381: The current fallback uses prisma.orgMember.findFirst and
only carries one membership into scopeFilter, collapsing multi-org users;
replace the findFirst call with prisma.orgMember.findMany to load all
memberships, then iterate the returned list and push a scopeFilter entry for
each membership.organizationId (e.g., scopeFilter.push({ scope: "ORGANIZATION",
organizationId })) while avoiding duplicates; update the logic around
organizationId/membership to no longer assume a single orgId so PROJECT and
GLOBAL entries remain unchanged.
- Around line 448-475: CreatePlatformNotificationSchema currently allows endsAt
to be <= startsAt (or <= now if startsAt is omitted), producing impossible
notification windows; add a superRefine check in the existing .superRefine
callback (or inside validateStartsAt) that computes const start = data.startsAt
?? new Date() and then if (data.endsAt <= start) call ctx.addIssue for the
"endsAt" path with a clear message (e.g., "endsAt must be after startsAt/now");
ensure you compare Date objects (not strings) so the fields endsAt and startsAt
from the schema are validated as chronological and reject endsAt <= start.
---
Duplicate comments:
In `@apps/webapp/app/components/navigation/NotificationPanel.tsx`:
- Around line 277-287: Nested link clicks inside NotificationPanel currently
bubble to the card's onClick (handleCardClick) and can trigger dismissal when
dismissOnAction is true; update the anchor render (the inline component using
onLinkClick) to stop event propagation before invoking onLinkClick (e.g.,
onClick={e => { e.stopPropagation(); onLinkClick?.(e); }}), so link clicks won't
bubble to handleCardClick while preserving existing link behavior.
In `@apps/webapp/app/services/platformNotifications.server.ts`:
- Around line 277-294: getRecentChangelogs currently fetches changelogs without
applying audience or lifecycle time-window filters, so update it to reuse the
same targeting and active-window constraints as getActivePlatformNotifications()
— either by calling getActivePlatformNotifications() and filtering for payload
type "changelog" and taking the requested limit, or by copying the same
where-clause logic into getRecentChangelogs'
prisma.platformNotification.findMany call (include audience targeting and
lifecycle active-window checks used by getActivePlatformNotifications). Keep the
existing payload parsing via PayloadV1Schema and return shape (id, title,
actionUrl) after applying those additional filters.
- Around line 10-24: The current schema is accepting any string as
contentPattern after compile-check in the refine, which lets clients persist
catastrophic regexes; change the validation to require an explicit opt-in flag
(e.g., a boolean like regexMode or contentPatternIsRegex) and only allow a
pattern when that flag is true, and add a safety check instead of just new
RegExp — use a deterministic/safe-regex check (e.g., integrate safe-regex or
validate against a whitelist and max length/complexity) before accepting the
pattern in the contentPattern refine; update the schema to reference the opt-in
flag and reject patterns that fail the safety check or exceed allowed
complexity/length.
- Around line 410-423: The upsert increments showCount and sets firstSeenAt
before the cliShowEvery gate, so suppressed views still consume visibility and
can trigger expiry rules; change the flow in platformNotifications.server.ts to
first read the existing interaction
(prisma.platformNotificationInteraction.findUnique for notificationId_userId)
and compute nextShowCount = (existing?.showCount ?? 0) + 1, apply the
n.cliShowEvery modulo check against nextShowCount and continue if it should be
suppressed, and only then perform the upsert (or create) to increment showCount
and set firstSeenAt = now when creating/first-seen; reference
prisma.platformNotificationInteraction.upsert, updated.showCount,
n.cliShowEvery, firstSeenAt, cliMaxShowCount, and cliMaxDaysAfterFirstSeen when
making these changes.
- Around line 503-523: validateSurfaceFields currently only blocks CLI-only
numeric fields but doesn't ensure the payload shape matches the surface; update
validateSurfaceFields to read data.payload?.data?.type and add ctx.addIssue when
the type is incompatible with the surface (e.g., if data.surface === "WEBAPP"
and payload.data.type is a log-style type, add an error on path
["payload","data","type"], and if data.surface === "CLI" and payload.data.type
is a card/changelog type, add an error similarly); reference the
validateSurfaceFields function, CLI_ONLY_FIELDS constant and the
payload.data.type path when implementing these checks so unsupported
surface/type combos are rejected early.
In `@packages/cli-v3/src/apiClient.ts`:
- Around line 61-86: Update the CliPlatformNotificationResponseSchema so its
notification.payload.data.type enum includes the missing "card" and "changelog"
values to match the server's CardDataV1Schema (locate the z.enum([...]) in
CliPlatformNotificationResponseSchema) and run pnpm run changeset:add to create
a changeset for the public package packages/cli-v3 as required by the project
guidelines.
In `@packages/cli-v3/src/utilities/discoveryCheck.ts`:
- Around line 123-146: In checkContentPattern, precompile and validate the
contentPattern regex once before iterating files: if useFastPath is false, wrap
new RegExp(contentPattern) in a try/catch, assign the result to a local
contentRegex (or null if invalid), and on error log via logger.debug/error with
the pattern and return false (or decide no matches) so you don't compile per
file; then inside the for loop replace new RegExp(contentPattern).test(content)
with contentRegex.test(content) (or skip testing when contentRegex is null),
keeping useFastPath logic unchanged.
---
Nitpick comments:
In `@apps/webapp/app/routes/admin.notifications.tsx`:
- Around line 76-172: Extract the notification payload construction out of
handleCreateAction into a helper function (e.g., buildNotificationPayload or
constructPlatformNotificationPayload) that takes the FormData-derived values
(surface, payloadType, adminLabel, title, description, actionUrl, image,
dismissOnAction, discoveryFilePatterns, discoveryContentPattern,
discoveryMatchBehavior, startsAt, endsAt, priority, cli fields, isPreview,
userId) and returns the object passed to createPlatformNotification; move the
discovery assembly and the surface/CLI-specific merges (including
startsAt/endsAt logic and cliMax* handling) into that helper, keep
handleCreateAction responsible only for validation, calling the helper, invoking
createPlatformNotification, and handling result/error mapping, and update calls
to createPlatformNotification to use the helper's return value.
In `@packages/cli-v3/src/utilities/discoveryCheck.ts`:
- Around line 77-121: resolveFilePatterns may return the same absolute path
multiple times (from overlapping globs and literals); update resolveFilePatterns
to normalize every absolute path (use path.normalize on values from glob results
and the computed absolutePath) before pushing to matched and then return a
deduplicated list (e.g., Array.from(new Set(matched)) or [...new Set(matched)])
so consumers only scan each file once; keep the rest of the logic and logging
intact and reference the matched variable and resolveFilePatterns function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2afbbf07-3db0-46d0-925a-613eca6a0408
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/admin.notifications.tsxapps/webapp/app/routes/admin.tsxapps/webapp/app/routes/api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-changelogs.tsxapps/webapp/app/routes/resources.platform-notifications.$id.clicked.tsxapps/webapp/app/routes/resources.platform-notifications.$id.dismiss.tsxapps/webapp/app/routes/resources.platform-notifications.$id.seen.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.tsapps/webapp/package.jsoninternal-packages/database/prisma/migrations/20260223123615_add_platform_notification_tables/migration.sqlinternal-packages/database/prisma/schema.prismapackages/cli-v3/src/apiClient.tspackages/cli-v3/src/commands/dev.tspackages/cli-v3/src/commands/login.tspackages/cli-v3/src/utilities/colorMarkup.test.tspackages/cli-v3/src/utilities/colorMarkup.tspackages/cli-v3/src/utilities/discoveryCheck.test.tspackages/cli-v3/src/utilities/discoveryCheck.tspackages/cli-v3/src/utilities/platformNotifications.ts
✅ Files skipped from review due to trivial changes (7)
- apps/webapp/package.json
- apps/webapp/app/routes/admin.tsx
- apps/webapp/app/routes/resources.platform-notifications.$id.clicked.tsx
- apps/webapp/app/routes/resources.platform-notifications.$id.dismiss.tsx
- apps/webapp/app/routes/api.v1.platform-notifications.ts
- packages/cli-v3/src/utilities/colorMarkup.test.ts
- packages/cli-v3/src/utilities/colorMarkup.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/webapp/app/routes/resources.platform-notifications.$id.seen.tsx
- packages/cli-v3/src/utilities/discoveryCheck.test.ts
- apps/webapp/app/routes/admin.api.v1.platform-notifications.ts
- apps/webapp/app/routes/resources.platform-notifications.tsx
- packages/cli-v3/src/utilities/platformNotifications.ts
- apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx
| const [notifications, total] = await Promise.all([ | ||
| prisma.platformNotification.findMany({ | ||
| where, | ||
| orderBy: [{ createdAt: "desc" }], | ||
| skip: (page - 1) * pageSize, | ||
| take: pageSize, | ||
| include: { | ||
| _count: { | ||
| select: { interactions: true }, | ||
| }, | ||
| interactions: { | ||
| select: { | ||
| webappDismissedAt: true, | ||
| webappClickedAt: true, | ||
| cliDismissedAt: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| prisma.platformNotification.count({ where }), | ||
| ]); |
There was a problem hiding this comment.
Avoid hydrating every interaction row for admin stats.
This query pulls the full interactions collection for each notification just to count clicks and dismissals. One high-reach notification can turn a single admin page load into a huge result set; these counts should be aggregated in the database instead of loaded into Node.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 68 -
88, The current findMany (prisma.platformNotification.findMany) hydrates the
full interactions relation for each notification; remove the include: {
interactions: ... } and instead run a separate aggregated query that counts
clicks/dismissals per notification id (e.g., using
prisma.platformNotification.groupBy or prisma.interaction.groupBy / aggregate to
_count with where clauses like webappClickedAt != null and webappDismissedAt !=
null) and then merge those aggregated counts into the notifications array before
returning; update the code around prisma.platformNotification.findMany and the
mapping/return logic so admin stats come from the aggregated counts rather than
loading every interaction row.
| // If no projectRef or project not found, get org from membership | ||
| if (!organizationId) { | ||
| const membership = await prisma.orgMember.findFirst({ | ||
| where: { userId }, | ||
| select: { organizationId: true }, | ||
| }); | ||
| if (membership) { | ||
| organizationId = membership.organizationId; | ||
| } | ||
| } | ||
|
|
||
| const scopeFilter: Array<Record<string, unknown>> = [ | ||
| { scope: "GLOBAL" }, | ||
| { scope: "USER", userId }, | ||
| ]; | ||
|
|
||
| if (organizationId) { | ||
| scopeFilter.push({ scope: "ORGANIZATION", organizationId }); | ||
| } | ||
|
|
||
| if (projectId) { | ||
| scopeFilter.push({ scope: "PROJECT", projectId }); | ||
| } |
There was a problem hiding this comment.
Don't collapse multi-org users to a single organization.
When projectRef is absent, this fallback carries only one membership forward into scopeFilter. Users in multiple orgs will get arbitrary org-scoped CLI notices while the other orgs' notifications are never considered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 359 -
381, The current fallback uses prisma.orgMember.findFirst and only carries one
membership into scopeFilter, collapsing multi-org users; replace the findFirst
call with prisma.orgMember.findMany to load all memberships, then iterate the
returned list and push a scopeFilter entry for each membership.organizationId
(e.g., scopeFilter.push({ scope: "ORGANIZATION", organizationId })) while
avoiding duplicates; update the logic around organizationId/membership to no
longer assume a single orgId so PROJECT and GLOBAL entries remain unchanged.
| export const CreatePlatformNotificationSchema = z | ||
| .object({ | ||
| title: z.string().min(1), | ||
| payload: PayloadV1Schema, | ||
| surface: z.enum(["WEBAPP", "CLI"]), | ||
| scope: z.enum(["USER", "PROJECT", "ORGANIZATION", "GLOBAL"]), | ||
| userId: z.string().optional(), | ||
| organizationId: z.string().optional(), | ||
| projectId: z.string().optional(), | ||
| startsAt: z | ||
| .string() | ||
| .datetime() | ||
| .transform((s) => new Date(s)) | ||
| .optional(), | ||
| endsAt: z | ||
| .string() | ||
| .datetime() | ||
| .transform((s) => new Date(s)), | ||
| priority: z.number().int().default(0), | ||
| cliMaxDaysAfterFirstSeen: z.number().int().positive().optional(), | ||
| cliMaxShowCount: z.number().int().positive().optional(), | ||
| cliShowEvery: z.number().int().min(2).optional(), | ||
| }) | ||
| .superRefine((data, ctx) => { | ||
| validateScopeForeignKeys(data, ctx); | ||
| validateSurfaceFields(data, ctx); | ||
| validateStartsAt(data, ctx); | ||
| }); |
There was a problem hiding this comment.
Reject inverted notification windows.
Nothing here prevents endsAt <= startsAt (or <= now when startsAt is omitted), which creates records that can never satisfy the active query and just disappear.
Possible fix
export const CreatePlatformNotificationSchema = z
.object({
@@
})
.superRefine((data, ctx) => {
validateScopeForeignKeys(data, ctx);
validateSurfaceFields(data, ctx);
validateStartsAt(data, ctx);
+ validateEndsAt(data, ctx);
});
+
+function validateEndsAt(data: { startsAt?: Date; endsAt: Date }, ctx: z.RefinementCtx) {
+ const effectiveStart = data.startsAt ?? new Date();
+
+ if (data.endsAt <= effectiveStart) {
+ ctx.addIssue({
+ code: "custom",
+ message: "endsAt must be after startsAt",
+ path: ["endsAt"],
+ });
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 448 -
475, CreatePlatformNotificationSchema currently allows endsAt to be <= startsAt
(or <= now if startsAt is omitted), producing impossible notification windows;
add a superRefine check in the existing .superRefine callback (or inside
validateStartsAt) that computes const start = data.startsAt ?? new Date() and
then if (data.endsAt <= start) call ctx.addIssue for the "endsAt" path with a
clear message (e.g., "endsAt must be after startsAt/now"); ensure you compare
Date objects (not strings) so the fields endsAt and startsAt from the schema are
validated as chronological and reject endsAt <= start.
There was a problem hiding this comment.
♻️ Duplicate comments (6)
apps/webapp/app/services/platformNotifications.server.ts (6)
513-533:⚠️ Potential issue | 🟠 MajorSurface/type mismatch is not validated.
validateSurfaceFieldsblocks CLI-only numeric fields for WEBAPP surface but doesn't validate that the payload type matches the surface. For example,surface: "CLI"withtype: "card"or"changelog"(webapp-only types) would be accepted.Consider adding type validation to ensure CLI surfaces only allow CLI types (
info,warn,error,success) and WEBAPP surfaces only allow webapp types (card,changelog).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 513 - 533, validateSurfaceFields currently blocks CLI-only numeric fields for WEBAPP but doesn't enforce that the payload "type" matches the "surface"; update validateSurfaceFields to also validate data.type against allowed sets depending on data.surface: define CLI_ALLOWED_TYPES = ["info","warn","error","success"] and WEBAPP_ALLOWED_TYPES = ["card","changelog"] and when data.surface === "WEBAPP" ensure data.type is one of WEBAPP_ALLOWED_TYPES (otherwise call ctx.addIssue with code "custom", path ["type"], and a clear message), and when data.surface === "CLI" ensure data.type is one of CLI_ALLOWED_TYPES (similarly report via ctx.addIssue); keep the existing CLI_ONLY_FIELDS checks intact so both field-level and type-level mismatches are reported.
9-27:⚠️ Potential issue | 🟠 MajorReDoS vulnerability in user-supplied regex patterns.
The
contentPatternrefinement only validates that the regex compiles but doesn't prevent catastrophically backtracking patterns. A malicious or poorly written regex can hang CLI clients when executed against local files.Consider constraining discovery to literal matching, limiting pattern complexity, or using a safe-regex validation library.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 9 - 27, The contentPattern refinement in DiscoverySchema only checks that user-supplied regexes compile (symbol: contentPattern inside DiscoverySchema) but allows ReDoS-prone patterns; replace this unsafe behavior by either (a) switching contentPattern to a literal substring match (e.g., rename to contentLiteral and validate as plain string), or (b) validate/whitelist the pattern using a safe-regex checker (e.g., integrate a library such as safe-regex or use an RE2 binding) and reject/limit patterns that are too complex (max length, disallow nested quantifiers/backreferences), or (c) refuse regexes entirely and require user-provided globs/strings; update DiscoverySchema validation and any code paths that execute the pattern (search/match logic that reads contentPattern) to use the chosen safe approach so runtime matching cannot be hijacked by catastrophic backtracking.
363-372:⚠️ Potential issue | 🟠 MajorMulti-org users only receive notifications from one arbitrary organization.
When
projectRefis absent,findFirstreturns a single membership, so users in multiple organizations will only see ORG-scoped notifications from whichever org is returned first. Consider usingfindManyand including all organization IDs in the scope filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 363 - 372, The code uses prisma.orgMember.findFirst which picks a single organization for multi-org users; change this to prisma.orgMember.findMany to fetch all membership records (e.g., assign to memberships) and derive an array of organizationIds instead of a single organizationId, then update the subsequent notification scope/filter logic to use that array (e.g., pass organizationIds or use where organizationId IN organizationIds) so ORG-scoped notifications are evaluated against all user organizations; adjust any variable names (membership -> memberships) and downstream uses (organizationId -> organizationIds) in the surrounding functions (including platformNotifications.server.ts scope/filter logic) accordingly.
472-485:⚠️ Potential issue | 🟡 MinorAllow creating notifications with
endsAt <= startsAt.The schema doesn't validate that
endsAtis afterstartsAt(or afternowwhenstartsAtis omitted). This allows creating notification windows that can never be active.Add a refinement to ensure
endsAt > (startsAt ?? now).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 472 - 485, The schema's superRefine currently calls validateScopeForeignKeys, validateSurfaceFields, and validateStartsAt but doesn't ensure endsAt is after startsAt (or now when startsAt is missing), allowing invalid windows; inside the existing .superRefine((data, ctx) => { ... }) add a check that compares data.endsAt (a Date from the transform) to (data.startsAt ?? new Date()), and if endsAt <= baseTime call ctx.addIssue (ZodIssueCode.custom) with a clear message and path ['endsAt'] so the schema rejects creations where endsAt is not strictly greater than the effective start time.
69-89:⚠️ Potential issue | 🟠 MajorAvoid loading all interaction rows for aggregate stats.
The query includes the full
interactionsarray (lines 79-85) only to count clicks and dismissals in memory (lines 110-113). For high-reach notifications, this could load thousands of rows per admin page load.Consider using Prisma's
_countwith filtered relations or a separate aggregation query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 69 - 89, The current prisma.platformNotification.findMany loads full interactions (the interactions include select block) just to compute click/dismiss counts and risks O(N) rows per notification; change it to avoid loading interaction rows by removing the interactions include and instead fetch aggregated counts—either extend the query to use prisma.platformNotification.findMany with relation counts (use _count where possible) or run a separate aggregation using prisma.notificationInteraction.count or groupBy filtered by notificationId and flags (e.g., webappClickedAt not null / webappDismissedAt not null) for the page’s notification IDs, then map those aggregated counts back to notifications; update the code that currently reads interactions (the places using interactions to compute clicks/dismissals) to read from the aggregated counts instead.
278-298:⚠️ Potential issue | 🟡 MinorScope filtering is missing for changelogs.
While the comment explains the intentional omission of time/archived filters, scoped changelogs (USER, ORGANIZATION, PROJECT) will still be visible to all users. If changelogs are only meant to be GLOBAL, consider adding a
scope: "GLOBAL"filter; otherwise, add scope filtering similar togetActivePlatformNotifications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotifications.server.ts` around lines 278 - 298, getRecentChangelogs currently queries prisma.platformNotification.findMany without scoping, so changelogs targeted to USER/ORGANIZATION/PROJECT will leak to everyone; update the where clause in getRecentChangelogs to include a scope filter (either scope: "GLOBAL" if changelogs should be global-only, or accept a scope parameter and mirror the scope filtering logic used in getActivePlatformNotifications) to restrict results appropriately while keeping PayloadV1Schema.safeParse and the existing mapping logic intact.
🧹 Nitpick comments (4)
apps/webapp/app/components/navigation/NotificationPanel.tsx (2)
55-65: AdddismissFetcherto the dependency array or document the intentional omission.The
handleDismisscallback usesdismissFetcher.submitbut has an empty dependency array. WhileuseFetcherprovides stable references, this may trigger lint warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 55 - 65, The useCallback for handleDismiss captures dismissFetcher but currently has an empty dependency array which can trigger lint warnings; update the dependencies to include dismissFetcher (and setDismissedIds if not stable) so the array becomes [dismissFetcher] or, if you intentionally rely on dismissFetcher being stable from useFetcher, add a brief inline comment and an eslint-disable-next-line react-hooks/exhaustive-deps above the useCallback to document the omission; locate the handleDismiss definition and update its dependency array or add the explanatory eslint-disable and comment accordingly.
94-99: AddfireSeenBeaconto the effect's dependency array.The effect calls
fireSeenBeaconbut it's not listed in the dependencies. SincefireSeenBeaconis memoized withuseCallback, adding it won't cause extra effect runs but will satisfy exhaustive-deps linting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/components/navigation/NotificationPanel.tsx` around lines 94 - 99, The useEffect that calls fireSeenBeacon when mounting references fireSeenBeacon but doesn't include it in the dependency array; update the dependency array for the useEffect (the effect that checks notification and hasIncident) to include fireSeenBeacon alongside notification?.id and hasIncident so exhaustive-deps is satisfied — locate the useEffect block in NotificationPanel.tsx (the one using notification, hasIncident and calling fireSeenBeacon) and add fireSeenBeacon to its dependencies.apps/webapp/app/services/platformNotificationCounter.server.ts (1)
9-25: Consider sharing the existing Redis client instead of creating a duplicate connection.This initialization is nearly identical to
inputStreamWaitpointCache.server.ts(same host, port, credentials,keyPrefix: "tr:", and TLS settings). Each singleton creates a separate connection to the same Redis instance.Consider extracting a shared cache Redis client or reusing the existing one to reduce connection overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/platformNotificationCounter.server.ts` around lines 9 - 25, The current initializeRedis function (used to create the singleton("platformNotificationCounter", initializeRedis) client) duplicates the same connection settings used elsewhere (e.g., inputStreamWaitpoint cache); extract the Redis creation into a single shared factory and reuse that singleton instead of creating another one. Specifically, move the Redis initialization logic (host/port/username/password/keyPrefix/tls/enableAutoPipelining and connectionName selection) into a shared module (e.g., getSharedRedisClient or sharedRedisSingleton) and have platformNotificationCounter.server.ts call that shared factory (or accept the shared client via injection) so both consumers reuse the same Redis instance rather than instantiating a new client. Ensure the shared factory preserves the keyPrefix and TLS conditional and uses a descriptive singleton key to avoid duplicate connections.apps/webapp/app/routes/resources.platform-notifications.tsx (1)
39-54: Consider addingfetcherto the dependency array.The
useEffectreferencesfetcher.stateandfetcher.loadbut doesn't includefetcherin the dependency array. WhileuseFetcherreturns a stable reference in Remix, this may trigger ESLint'sexhaustive-depswarning.If adding
fetchercauses unwanted interval recreation, you can suppress with// eslint-disable-next-line react-hooks/exhaustive-depswith a comment explaining the stable reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/resources.platform-notifications.tsx` around lines 39 - 54, The effect uses fetcher.state and fetcher.load but omits fetcher from the dependency array; update the useEffect dependency list to include fetcher (alongside organizationId and projectId) or, if you rely on Remix's stable useFetcher reference and want to avoid recreating the interval, add a clear comment and suppress the exhaustive-deps rule with // eslint-disable-next-line react-hooks/exhaustive-deps; make the change where useEffect is defined (referencing useEffect, lastLoadedUrl.current, fetcher.load, fetcher.state, and POLL_INTERVAL_MS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/webapp/app/services/platformNotifications.server.ts`:
- Around line 513-533: validateSurfaceFields currently blocks CLI-only numeric
fields for WEBAPP but doesn't enforce that the payload "type" matches the
"surface"; update validateSurfaceFields to also validate data.type against
allowed sets depending on data.surface: define CLI_ALLOWED_TYPES =
["info","warn","error","success"] and WEBAPP_ALLOWED_TYPES =
["card","changelog"] and when data.surface === "WEBAPP" ensure data.type is one
of WEBAPP_ALLOWED_TYPES (otherwise call ctx.addIssue with code "custom", path
["type"], and a clear message), and when data.surface === "CLI" ensure data.type
is one of CLI_ALLOWED_TYPES (similarly report via ctx.addIssue); keep the
existing CLI_ONLY_FIELDS checks intact so both field-level and type-level
mismatches are reported.
- Around line 9-27: The contentPattern refinement in DiscoverySchema only checks
that user-supplied regexes compile (symbol: contentPattern inside
DiscoverySchema) but allows ReDoS-prone patterns; replace this unsafe behavior
by either (a) switching contentPattern to a literal substring match (e.g.,
rename to contentLiteral and validate as plain string), or (b)
validate/whitelist the pattern using a safe-regex checker (e.g., integrate a
library such as safe-regex or use an RE2 binding) and reject/limit patterns that
are too complex (max length, disallow nested quantifiers/backreferences), or (c)
refuse regexes entirely and require user-provided globs/strings; update
DiscoverySchema validation and any code paths that execute the pattern
(search/match logic that reads contentPattern) to use the chosen safe approach
so runtime matching cannot be hijacked by catastrophic backtracking.
- Around line 363-372: The code uses prisma.orgMember.findFirst which picks a
single organization for multi-org users; change this to
prisma.orgMember.findMany to fetch all membership records (e.g., assign to
memberships) and derive an array of organizationIds instead of a single
organizationId, then update the subsequent notification scope/filter logic to
use that array (e.g., pass organizationIds or use where organizationId IN
organizationIds) so ORG-scoped notifications are evaluated against all user
organizations; adjust any variable names (membership -> memberships) and
downstream uses (organizationId -> organizationIds) in the surrounding functions
(including platformNotifications.server.ts scope/filter logic) accordingly.
- Around line 472-485: The schema's superRefine currently calls
validateScopeForeignKeys, validateSurfaceFields, and validateStartsAt but
doesn't ensure endsAt is after startsAt (or now when startsAt is missing),
allowing invalid windows; inside the existing .superRefine((data, ctx) => { ...
}) add a check that compares data.endsAt (a Date from the transform) to
(data.startsAt ?? new Date()), and if endsAt <= baseTime call ctx.addIssue
(ZodIssueCode.custom) with a clear message and path ['endsAt'] so the schema
rejects creations where endsAt is not strictly greater than the effective start
time.
- Around line 69-89: The current prisma.platformNotification.findMany loads full
interactions (the interactions include select block) just to compute
click/dismiss counts and risks O(N) rows per notification; change it to avoid
loading interaction rows by removing the interactions include and instead fetch
aggregated counts—either extend the query to use
prisma.platformNotification.findMany with relation counts (use _count where
possible) or run a separate aggregation using
prisma.notificationInteraction.count or groupBy filtered by notificationId and
flags (e.g., webappClickedAt not null / webappDismissedAt not null) for the
page’s notification IDs, then map those aggregated counts back to notifications;
update the code that currently reads interactions (the places using interactions
to compute clicks/dismissals) to read from the aggregated counts instead.
- Around line 278-298: getRecentChangelogs currently queries
prisma.platformNotification.findMany without scoping, so changelogs targeted to
USER/ORGANIZATION/PROJECT will leak to everyone; update the where clause in
getRecentChangelogs to include a scope filter (either scope: "GLOBAL" if
changelogs should be global-only, or accept a scope parameter and mirror the
scope filtering logic used in getActivePlatformNotifications) to restrict
results appropriately while keeping PayloadV1Schema.safeParse and the existing
mapping logic intact.
---
Nitpick comments:
In `@apps/webapp/app/components/navigation/NotificationPanel.tsx`:
- Around line 55-65: The useCallback for handleDismiss captures dismissFetcher
but currently has an empty dependency array which can trigger lint warnings;
update the dependencies to include dismissFetcher (and setDismissedIds if not
stable) so the array becomes [dismissFetcher] or, if you intentionally rely on
dismissFetcher being stable from useFetcher, add a brief inline comment and an
eslint-disable-next-line react-hooks/exhaustive-deps above the useCallback to
document the omission; locate the handleDismiss definition and update its
dependency array or add the explanatory eslint-disable and comment accordingly.
- Around line 94-99: The useEffect that calls fireSeenBeacon when mounting
references fireSeenBeacon but doesn't include it in the dependency array; update
the dependency array for the useEffect (the effect that checks notification and
hasIncident) to include fireSeenBeacon alongside notification?.id and
hasIncident so exhaustive-deps is satisfied — locate the useEffect block in
NotificationPanel.tsx (the one using notification, hasIncident and calling
fireSeenBeacon) and add fireSeenBeacon to its dependencies.
In `@apps/webapp/app/routes/resources.platform-notifications.tsx`:
- Around line 39-54: The effect uses fetcher.state and fetcher.load but omits
fetcher from the dependency array; update the useEffect dependency list to
include fetcher (alongside organizationId and projectId) or, if you rely on
Remix's stable useFetcher reference and want to avoid recreating the interval,
add a clear comment and suppress the exhaustive-deps rule with //
eslint-disable-next-line react-hooks/exhaustive-deps; make the change where
useEffect is defined (referencing useEffect, lastLoadedUrl.current,
fetcher.load, fetcher.state, and POLL_INTERVAL_MS).
In `@apps/webapp/app/services/platformNotificationCounter.server.ts`:
- Around line 9-25: The current initializeRedis function (used to create the
singleton("platformNotificationCounter", initializeRedis) client) duplicates the
same connection settings used elsewhere (e.g., inputStreamWaitpoint cache);
extract the Redis creation into a single shared factory and reuse that singleton
instead of creating another one. Specifically, move the Redis initialization
logic (host/port/username/password/keyPrefix/tls/enableAutoPipelining and
connectionName selection) into a shared module (e.g., getSharedRedisClient or
sharedRedisSingleton) and have platformNotificationCounter.server.ts call that
shared factory (or accept the shared client via injection) so both consumers
reuse the same Redis instance rather than instantiating a new client. Ensure the
shared factory preserves the keyPrefix and TLS conditional and uses a
descriptive singleton key to avoid duplicate connections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a236ddf-3f0c-4c16-b168-96ce9e94a4d1
📒 Files selected for processing (5)
apps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/services/platformNotifications.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/admin.api.v1.platform-notifications.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: For apps and internal packages (apps/*,internal-packages/*), usepnpm run typecheck --filter <package>for verification, never usebuildas it proves almost nothing about correctness
Use testcontainers helpers (redisTest,postgresTest,containerTestfrom@internal/testcontainers) for integration tests with Redis and PostgreSQL instead of mocking
When writing Trigger.dev tasks, always import from@trigger.dev/sdk- never use@trigger.dev/sdk/v3or deprecatedclient.defineJob
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Use pnpm for package management in this monorepo (version 10.23.0) with Turborepo for orchestration - run commands from root withpnpm run
Add crumbs as you write code for debug tracing using//@Crumbscomments or `// `#region` `@crumbsblocks - they stay on the branch throughout development and are stripped viaagentcrumbs stripbefore merge
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/services/platformNotifications.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/services/platformNotifications.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
apps/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying only server components (
apps/webapp/,apps/supervisor/, etc.) with no package changes, add a.server-changes/file instead of a changeset
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
apps/webapp/app/**/*.{ts,tsx,server.ts}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Access environment variables via
envexport fromapp/env.server.ts. Never useprocess.envdirectly
Files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
🧠 Learnings (25)
📚 Learning: 2026-01-12T17:18:09.451Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2870
File: apps/webapp/app/services/redisConcurrencyLimiter.server.ts:56-66
Timestamp: 2026-01-12T17:18:09.451Z
Learning: In `apps/webapp/app/services/redisConcurrencyLimiter.server.ts`, the query concurrency limiter will not be deployed with Redis Cluster mode, so multi-key operations (keyKey and globalKey in different hash slots) are acceptable and will function correctly in standalone Redis mode.
Applied to files:
apps/webapp/app/services/platformNotificationCounter.server.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/services/**/*.server.{ts,tsx} : Separate testable services from configuration files; follow the pattern of `realtimeClient.server.ts` (testable service) and `realtimeClientGlobal.server.ts` (configuration) in the webapp
Applied to files:
apps/webapp/app/services/platformNotificationCounter.server.ts
📚 Learning: 2026-03-23T06:24:14.566Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.566Z
Learning: Applies to apps/**/*.{ts,tsx,js,jsx} : When modifying only server components (`apps/webapp/`, `apps/supervisor/`, etc.) with no package changes, add a `.server-changes/` file instead of a changeset
Applied to files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-23T06:24:25.029Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:25.029Z
Learning: Applies to apps/webapp/app/v3/services/**/*.server.ts : Only modify V2 code paths when editing services that branch on `RunEngineVersion` to support both V1 and V2 (e.g., `cancelTaskRun.server.ts`, `batchTriggerV3.server.ts`)
Applied to files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/services/platformNotificationCounter.server.tsapps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-01-28T16:57:47.620Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 2964
File: apps/webapp/app/components/AskAI.tsx:121-141
Timestamp: 2026-01-28T16:57:47.620Z
Learning: In the trigger.dev webapp codebase, the Button component (apps/webapp/app/components/primitives/Buttons) does not spread unknown props to the underlying <button> element—it only passes specific props (type, disabled, onClick, name, value, ref, form, autoFocus). When using Radix UI's TooltipTrigger with asChild, a span wrapper around the Button is necessary to receive Radix props (aria-describedby, onPointerEnter, onPointerLeave, data-state) while the Button handles its own behavior. Directly making the Button the child of TooltipTrigger with asChild will break accessibility and tooltip functionality.
Applied to files:
apps/webapp/app/components/navigation/NotificationPanel.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/components/navigation/NotificationPanel.tsxapps/webapp/app/routes/resources.platform-notifications.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/resources.platform-notifications.tsxapps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-23T06:24:25.029Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:25.029Z
Learning: Applies to apps/webapp/app/routes/**/*.ts : Use Remix flat-file route convention with dot-separated segments where `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`
Applied to files:
apps/webapp/app/routes/resources.platform-notifications.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/resources.platform-notifications.tsx
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : Use zod for validation in packages/core and apps/webapp
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-02T12:43:37.906Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/core/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:37.906Z
Learning: Exercise caution with changes to trigger.dev/core as they affect both the customer-facing SDK and server-side webapp - breaking changes can impact deployed user tasks and the platform simultaneously
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-02T12:43:34.140Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/cli-v3/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:34.140Z
Learning: Applies to packages/cli-v3/src/build/**/* : Build system in `src/build/` should use configuration from `trigger.config.ts` in user projects to determine bundling, build extensions, and output structure
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-10T12:44:19.869Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3200
File: docs/config/config-file.mdx:353-368
Timestamp: 2026-03-10T12:44:19.869Z
Learning: In the triggerdotdev/trigger.dev repository, docs PRs are often written as companions to implementation PRs (e.g., PR `#3200` documents features being added in PR `#3196`). When reviewing docs PRs, the documented features may exist in a companion/companion PR branch rather than main. Always check companion PRs referenced in the PR description before flagging missing implementations.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-23T06:24:14.566Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.566Z
Learning: Applies to apps/webapp/app/v3/**/*.{ts,tsx} : In the webapp v3 directory, only modify V2 code paths when encountering V1/V2 branching in services - all new work uses Run Engine 2.0 (`internal/run-engine`) and redis-worker, not legacy V1 engine code
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T19:26:49.299Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: internal-packages/emails/emails/alert-error-group.tsx:58-58
Timestamp: 2026-03-22T19:26:49.299Z
Learning: In the triggerdotdev/trigger.dev codebase, email template files under `internal-packages/emails/emails/` must use `export default function Email(...)` (default export) because the React Email previewer requires a default export to discover and render templates. Do not flag default exports in these files as violations of the "use named exports" coding guideline.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-01-28T14:15:15.011Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2953
File: apps/webapp/app/components/runs/v3/SharedFilters.tsx:441-452
Timestamp: 2026-01-28T14:15:15.011Z
Learning: In apps/webapp/app/components/runs/v3/SharedFilters.tsx, the maxPeriodDays limit for date ranges should only check the from date (via dateRangeToDays(fromValue)) because it enforces data retention limits—how far back in history queries can reach. The to date is irrelevant for retention-based limits.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T19:27:29.014Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts:104-112
Timestamp: 2026-03-22T19:27:29.014Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts`, the `#scheduleErrorAlertEvaluation` helper intentionally uses the same job id (`evaluateErrorAlerts:${projectId}`) as the evaluator's periodic self-chain. The deduplication is desired: if a future run is already queued, the immediate enqueue becomes a no-op, preventing two evaluations firing in quick succession. Do not flag this as a bug or suggest a unique/timestamped id.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-22T19:32:16.231Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx:82-151
Timestamp: 2026-03-22T19:32:16.231Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts` and `apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx`, the `errorAlertConfig` field on `ProjectAlertChannel` is intentionally `Json?` (nullable). The `ErrorAlertEvaluator.computeMinInterval()` in `apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts` uses `ErrorAlertConfig.safeParse(ch.errorAlertConfig)` and falls back to `DEFAULT_INTERVAL_MS = 300_000` when `errorAlertConfig` is null. No UI currently collects this value — it is scaffolding for a future per-channel evaluation interval feature. Do not flag the absence of `errorAlertConfig` in `CreateAlertChannelOptions` or the action handler as a bug; null configs are safe and expected.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-02-11T16:50:14.167Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:14.167Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-10T17:56:26.581Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3201
File: apps/webapp/app/v3/services/setSeatsAddOn.server.ts:25-29
Timestamp: 2026-03-10T17:56:26.581Z
Learning: In the `triggerdotdev/trigger.dev` webapp, service classes such as `SetSeatsAddOnService` and `SetBranchesAddOnService` do NOT need to perform their own userId-to-organizationId authorization checks. Auth is enforced at the route layer: `requireUserId(request)` authenticates the user, and the `_app.orgs.$organizationSlug` layout route enforces that the authenticated user is a member of the org. Any `userId` and `organizationId` reaching these services from org-scoped routes are already validated. This is the consistent pattern used across all org-scoped services in the codebase.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
📚 Learning: 2026-03-13T13:42:59.104Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts:40-43
Timestamp: 2026-03-13T13:42:59.104Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts` and `apps/webapp/app/routes/admin.api.v1.llm-models.ts`, the `startDate` field in `UpdateModelSchema` and `CreateModelSchema` intentionally uses `z.string().optional()` (or `.nullable().optional()`) without strict ISO datetime validation. Invalid date strings are rejected at the Prisma/DB layer. This is acceptable because these are admin-only API routes protected by Personal Access Token (PAT) authentication and are not user-facing.
Applied to files:
apps/webapp/app/services/platformNotifications.server.ts
🪛 ast-grep (0.41.1)
apps/webapp/app/services/platformNotifications.server.ts
[warning] 17-17: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(val)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (8)
apps/webapp/app/services/platformNotificationCounter.server.ts (1)
28-45: LGTM!The counter logic is straightforward. The potential race between
incrandsetduring wrap-around is acceptable for this throttling use case since the worst case is multiple near-simultaneous resets, which doesn't affect correctness.apps/webapp/app/services/platformNotifications.server.ts (4)
29-45: LGTM!The payload schema structure is well-defined with appropriate optional fields and URL validation.
125-191: LGTM!The webapp notification retrieval logic correctly filters by scope, time window, and dismissal state. Safe payload parsing ensures malformed payloads don't crash the service.
205-274: LGTM!The interaction upsert pattern is clean and reusable across seen, dismiss, and click operations.
405-442: LGTM on the showCount fix.The separation of
requestCounter(global per-user request cadence) fromshowCount(per-notification display count) correctly ensures thatcliMaxShowCounttracks actual displays rather than API encounters. Good implementation.apps/webapp/app/routes/resources.platform-notifications.tsx (1)
18-31: LGTM!The loader correctly requires authentication, handles missing
organizationIdgracefully, and delegates to the service layer.apps/webapp/app/components/navigation/NotificationPanel.tsx (2)
168-269: LGTM!The
NotificationCardcomponent handles dismiss, expand/collapse, and click interactions cleanly with proper event propagation. The conditional wrapper pattern for links is well-implemented.
272-299: LGTM!The markdown components are well-styled and the anchor click handler correctly stops propagation to prevent triggering the parent card's click handler.
|
|
||
| /** Increment and return the user's CLI request counter (1-based, wraps at 1001→1). */ | ||
| export async function incrementCliRequestCounter(userId: string): Promise<number> { | ||
| if (!redis) return 1; |
There was a problem hiding this comment.
🟡 cliShowEvery notifications permanently suppressed when Redis is unavailable
When Redis is not configured (e.g., self-hosted instances without CACHE_REDIS_HOST), incrementCliRequestCounter at apps/webapp/app/services/platformNotificationCounter.server.ts:29 always returns 1. Since the CreatePlatformNotificationSchema enforces cliShowEvery to be at minimum 2 (z.number().int().min(2).optional() at apps/webapp/app/services/platformNotifications.server.ts:479), the modulo check 1 % N is always non-zero for any N ≥ 2, causing the continue on line 421 to always fire and permanently skip CLI notifications that use cliShowEvery. The same issue occurs on Redis errors (the catch block also returns 1). The fix is to return 0 as the fallback (since 0 % N === 0 for any N, notifications would always show — a sensible "fail open" for a rate-limiting feature), or to skip the cliShowEvery check entirely when the counter is unreliable.
| if (!redis) return 1; | |
| if (!redis) return 0; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Code reviewFound 1 issue:
trigger.dev/apps/webapp/app/components/navigation/NotificationPanel.tsx Lines 94 to 100 in a6f6bb1 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
1 participant
✅ Checklist
Testing
Spawning new CLI / Dashboard notifications, check MVP, check if failures not produce any problems with CLI/Dashboard
Changelog
Added notifications mechanism for Dashboard and CLI
Screenshots
💯