feat(Notifications): Platform notifications for CLI and Dashboard #3254
feat(Notifications): Platform notifications for CLI and Dashboard #3254
Conversation
🦋 Changeset detectedLatest commit: fd57fff The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a platform notifications feature: DB migration and Prisma models for notifications and interactions; server services for creating, querying, and recording interactions (web and CLI); multiple Remix routes for admin UI, public APIs, resource polling, and interaction beacons; admin notifications UI and a webapp NotificationPanel plus changelog polling and Help popover updates; CLI client/commands to fetch and display notifications with discovery and color-markup utilities and tests; a Redis-backed per-user CLI request counter; and adds react-markdown dependency. 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0ski
force-pushed
the
oskar/feat-platform-notifications
branch
from
dc9619f to
abc723f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0ski
force-pushed
the
oskar/feat-platform-notifications
branch
from
abc723f to
1980052
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 👎. |
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
2 times, most recently
from
6be61b0 to
2360973
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0ski
force-pushed
the
oskar/feat-platform-notifications
branch
from
2360973 to
c9c44c1
Compare
| const interval = setInterval(() => { | ||
| if (fetcher.state === "idle") { | ||
| fetcher.load(url); | ||
| } | ||
| }, POLL_INTERVAL_MS); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
🟡 Stale fetcher reference makes fetcher.state === "idle" guard permanently true in polling interval
In useRecentChangelogs, the useEffect has an empty dependency array [] but captures fetcher from useFetcher() in its closure. In Remix 2.x, useFetcher() returns a new object reference on each render; the captured reference's .state property never updates from its initial "idle" value. This means the fetcher.state === "idle" guard inside the setInterval callback at line 37 is always true, defeating the intent to skip polling when a previous request is still in flight. In practice, Remix handles concurrent fetches on the same key by aborting the old one, so this doesn't crash — but it results in unnecessary aborted requests every poll interval.
Was this helpful? React with 👍 or 👎 to provide feedback.
0ski
force-pushed
the
oskar/feat-platform-notifications
branch
from
c9c44c1 to
fd57fff
Compare
| const notificationPromise = options.skipPlatformNotifications | ||
| ? undefined | ||
| : fetchPlatformNotification({ | ||
| apiClient: new CliApiClient(options.login.auth.apiUrl, options.login.auth.accessToken), | ||
| projectRef: options.projectRef, | ||
| }); | ||
|
|
||
| await printStandloneInitialBanner(true, options.profile); | ||
|
|
||
| await awaitAndDisplayPlatformNotification(notificationPromise); |
There was a problem hiding this comment.
🟡 Double notification fetch/display when dev triggers browser login flow
When a user runs trigger dev and needs to authenticate (no stored token), the login function's browser auth flow fetches and displays a platform notification (login.ts:293-321), then control returns to startDev in dev.ts:220-229 which fetches and displays another notification. This causes two API calls to getNextCliNotification, which increments the per-user requestCounter (platformNotificationCounter.server.ts:33) and the per-notification showCount (platformNotifications.server.ts:428) twice. The user sees the same notification printed twice, and notifications with cliMaxShowCount limits expire at double the expected rate during first-login scenarios.
Prompt for agents
The dev command at packages/cli-v3/src/commands/dev.ts:220-229 fetches and displays a platform notification, but the login function at packages/cli-v3/src/commands/login.ts:293-321 also fetches and displays a notification when the browser auth flow is used. When dev calls login with embedded: true, both execute sequentially, causing duplicate notification display and double-counting of showCount/requestCounter.
Fix options:
1. In packages/cli-v3/src/commands/login.ts, skip the notification fetch when opts.embedded is true (around line 293), since the caller (dev.ts) will handle its own notification fetch.
2. Alternatively, have login.ts return the notification promise/result so dev.ts can reuse it instead of fetching again.
3. Or add a skipPlatformNotifications option to the login function so dev.ts can suppress it when it handles notifications itself.
Option 1 is simplest: wrap lines 293-295 and 321 in login.ts with if (!opts.embedded) { ... }.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
For human reviewer:
✅ 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
💯