Skip to content

Conversation

samejr
Copy link
Member

@samejr samejr commented Sep 7, 2025

Updates these emails sent from the app:

  • Welcome email
  • Invite email
  • Magic link login email

With these improvements:

  • Updates them all to use Tailwind
  • Updates react-email to the latest
  • Improved styling and layout
  • Fixes some very outdated links that were 404s

New layouts (without images and dynamic content loaded):

Welcome email
CleanShot 2025-09-07 at 15 11 46@2x

Login email
CleanShot 2025-09-07 at 15 11 31@2x

Invite email
CleanShot 2025-09-07 at 15 11 16@2x

Copy link

changeset-bot bot commented Sep 7, 2025

⚠️ No Changeset found

Latest commit: 03e6bce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Sep 7, 2025

Walkthrough

This PR applies broad cross-cutting changes: multiple email templates migrated to Tailwind-based @react-email components and package updates; email transports (SMTP/AWS SES) now await rendered HTML; invite schema and several email props updated. It adds a Redis-backed magic-link rate limiter and integrates rate-limiting into the login magic flow. The run-engine gains repair/reporting features, new RunQueue concurrency/clear APIs, worker repairSnapshot, and many internals updated (span/event APIs, span presenters, event repository). Webapp routes and presenters were tightened to enforce membership checks; env schema gained LOGIN_RATE_LIMITS_ENABLED; personal access token revoke now requires userId. Helm/docker images and chart/values updated for legacy Bitnami images and new webapp init/tokenSyncer images. Miscellaneous type and import refinements across the codebase.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description outlines which emails were updated and the nature of the improvements, but it does not follow the repository’s required template sections: it lacks the “Closes #” reference, the ✅ Checklist, a Testing section, a Changelog entry, and the formal Screenshots heading. Please update the description to match the template by adding “Closes #,” completing the ✅ Checklist, adding a “Testing” section with your test steps, a “Changelog” summary, and placing the screenshots under the “## Screenshots” heading.
Docstring Coverage ⚠️ Warning Docstring coverage is 3.23% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title “chore(webapp): improvements to app emails” clearly reflects that the primary change is improving the application’s email templates, follows the project’s conventional commit style, and is concise enough for teammates to understand the core update at a glance.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch nicer-app-emails

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@samejr samejr changed the title chore(webapp)Improvements to app emails chore(webapp):Improvements to app emails Sep 7, 2025
@samejr samejr changed the title chore(webapp):Improvements to app emails chore(webapp): improvements to app emails Sep 7, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal-packages/emails/emails/invite.tsx (1)

26-31: Follow repo guideline: avoid default exports.

Prefer a named export to align with “No default exports” guidance.

Apply:

-export default function Email({
+export function InviteEmail({
   orgName,
   inviterName,
   inviterEmail,
   inviteLink,
 }: z.infer<typeof InviteEmailSchema>) {

Follow-up: update any import sites (e.g., index/routers) to import { InviteEmail } ....

Run to find consumers:

#!/bin/bash
rg -nP --type=ts --type=tsx -C2 'from\s+["'\'']\./emails/invite["'\'']|require\(\s*["'\'']\./emails/invite["'\'']\s*\)'
🧹 Nitpick comments (4)
internal-packages/emails/emails/invite.tsx (2)

16-16: Avoid mixing style objects with Tailwind; inline via className for consistency.

You can drop the styles import and express paragraphLight as Tailwind classes.

Apply:

-import { anchor, container, h1, main, paragraphLight } from "./components/styles";
+// Tailwind used throughout; local style tokens not needed here.
-              <Text style={paragraphLight}>
+              <Text className="text-[14px] leading-6 text-[#878C99]">
                 {inviterName ?? inviterEmail} has invited you to join their organization on
                 Trigger.dev.
               </Text>

Also applies to: 47-50


62-66: Add rel attribute when using target="_blank".

Harmless in emails and good hygiene.

Apply:

-              <Link
+              <Link
                 href={inviteLink}
-                target="_blank"
+                target="_blank"
+                rel="noopener noreferrer"
                 className="text-[#6366F1] text-[14px] no-underline"
               >
internal-packages/emails/emails/magic-link.tsx (2)

46-53: Prevent long-link overflow and keep link affordance.

Long magic links can break layout in some clients; also underline improves recognizability.

-              <Link
-                href={magicLink}
-                target="_blank"
-                className="text-[#6366F1] text-[14px] no-underline"
-              >
+              <Link
+                href={magicLink}
+                target="_blank"
+                rel="noopener noreferrer"
+                className="text-[#6366F1] text-[14px] underline break-all"
+              >

22-24: Minor: drop redundant body margins.

Email clients ignore/override body margins; Container already centers content.

-        <Body className="bg-[#121317] my-auto mx-auto font-sans">
+        <Body className="bg-[#121317] font-sans">
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 89b1d8b and a42c799.

⛔ Files ignored due to path filters (1)
  • apps/webapp/public/emails/logo-triangle.png is excluded by !**/*.png
📒 Files selected for processing (6)
  • internal-packages/emails/emails/components/Footer.tsx (1 hunks)
  • internal-packages/emails/emails/components/styles.ts (0 hunks)
  • internal-packages/emails/emails/invite.tsx (2 hunks)
  • internal-packages/emails/emails/magic-link.tsx (1 hunks)
  • internal-packages/emails/emails/welcome.tsx (1 hunks)
  • internal-packages/emails/package.json (1 hunks)
💤 Files with no reviewable changes (1)
  • internal-packages/emails/emails/components/styles.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/emails/emails/magic-link.tsx
  • internal-packages/emails/emails/invite.tsx
  • internal-packages/emails/emails/welcome.tsx
  • internal-packages/emails/emails/components/Footer.tsx
🪛 GitHub Actions: 🤖 PR Checks
internal-packages/emails/package.json

[error] 1-1: Lockfile specifiers do not match package.json: lockfile lists @react-email/components 0.0.16, @react-email/render ^0.0.12, and react-email ^2.1.1 (among others) whereas package.json requires @react-email/components 0.4.0, @react-email/render ^1.1.4, and react-email ^4.2.7; also @react-email/tailwind is present in package.json but missing from the lockfile. Run 'pnpm install' to update.

🔇 Additional comments (5)
internal-packages/emails/emails/components/Footer.tsx (1)

8-13: LGTM on the presentational refactor.

Consistent text sizing/color and keeping the hr tokenized via styles looks good.

internal-packages/emails/emails/invite.tsx (1)

35-73: Sanity-check Tailwind inlining with the new renderer.

Given multiple arbitrary values (e.g., bg-[#121317]), render one sample locally to ensure styles inline as expected with @react-email/render ^1.1.4.

#!/bin/bash
# Find any remaining incorrect Tailwind imports across the repo
rg -nPU '(?s)import\s*{\s*[^}]*Tailwind[^}]*}\s*from\s*"@react-email/components"'

# (manual step) locally run the email preview if you use react-email CLI
# pnpm --filter emails dev
internal-packages/emails/emails/welcome.tsx (1)

18-69: LGTM on the content and Tailwind migration.

Clear copy, sensible defaults, consistent tokens, and Footer integration look good.

internal-packages/emails/emails/magic-link.tsx (2)

26-31: Verify logo URL resolution in production emails.

Relative paths often break in email clients/CDNs. Confirm your Image component rewrites to an absolute URL (e.g., CDN/base URL).

#!/bin/bash
# Inspect Image component(s) to confirm absolute URL handling
fd -t f Image.tsx | while read -r f; do
  echo "== $f"; rg -nP -C2 'src|path|baseUrl|cdn|process\.env|absolute' "$f"
done

55-60: Confirm the 30-minute expiry matches backend policy.

If the TTL differs, this is user-facing misinformation.

#!/bin/bash
# Search for magic link TTL/expiry to validate the copy
rg -nPI -C2 '(magic[_ -]?link|login).*expir|expiresIn|ttl|maxAge|token' \
  -g '!**/node_modules/**' -g '!**/dist/**' --type=ts --type=tsx --type=tsv --type=js

import { Hr, Link, Text } from "@react-email/components";
import React from "react";
import { footer, footerAnchor, hr } from "./styles";
import { Hr, Link, Tailwind, Text } from "@react-email/components";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Tailwind is imported from the wrong package and unused here.

Footer doesn’t use Tailwind; drop it from the import (and if needed elsewhere, import from @react-email/tailwind, not components).

Apply:

-import { Hr, Link, Tailwind, Text } from "@react-email/components";
+import { Hr, Link, Text } from "@react-email/components";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { Hr, Link, Tailwind, Text } from "@react-email/components";
import { Hr, Link, Text } from "@react-email/components";
🤖 Prompt for AI Agents
internal-packages/emails/emails/components/Footer.tsx lines 1-1: the import
currently brings in Tailwind from the wrong package and it is unused in this
file; remove Tailwind from the import list (keep Hr, Link, Text) so Footer only
imports what it uses, and if you need Tailwind elsewhere import it from
@react-email/tailwind rather than @react-email/components.

import { anchor, container, h1, main, paragraphLight } from "./components/styles";
import { hr } from "./components/styles";

export default function Email({ magicLink }: { magicLink: string }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Switch to a named export per repo guidelines; also use a clearer component name.

Avoid default exports. Rename to a named export to align with “No default exports, use function declarations”.

-export default function Email({ magicLink }: { magicLink: string }) {
+export function MagicLinkEmail({ magicLink }: { magicLink: string }) {

Run to find and update call sites that import the default:

#!/bin/bash
# Find default imports of magic-link and re-export patterns using default
rg -nP -C1 '\bimport\s+Email\s+from\s+["\'][^"\']*magic-link' --type=ts --type=tsx
rg -nP -C1 'export\s*{\s*default\s+as\s+[^}]+}\s*from\s*["\'][^"\']*magic-link' --type=ts --type=tsx
🤖 Prompt for AI Agents
In internal-packages/emails/emails/magic-link.tsx around line 17, the component
is exported as a default and named Email; change this to a named function export
with a clearer name (e.g., MagicLinkEmail) and export it as a named export
(export function MagicLinkEmail(...)). Update the component name internally if
needed but keep the same props signature, remove the default export, and ensure
the file exports the component by name. Run the provided ripgrep commands to
find and update all call sites to import { MagicLinkEmail } from
'.../magic-link' (or adjust to the new name) and replace any re-exports of
default with corresponding named re-exports. Finally run TypeScript build/tests
to validate no remaining default-import references.

import { anchor, bullets, footerItalic, main, paragraphLight } from "./components/styles";

export default function Email({ name }: { name?: string }) {
return (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use a named export per repo guideline (no default exports).

Switch to a named function export; consider a descriptive name.

Apply:

-export default function Email({ name }: { name?: string }) {
+export function WelcomeEmail({ name }: { name?: string }) {

Update import sites accordingly.

#!/bin/bash
rg -nP --type=ts --type=tsx -C2 'export\s+default\s+function\s+Email\(|from\s+["'\'']\./emails/welcome["'\'']'
🤖 Prompt for AI Agents
internal-packages/emails/emails/welcome.tsx around line 13: this file currently
uses a default export for the Email component; change it to a named export
(e.g., export function WelcomeEmail({ name }: { name?: string }) { ... }) and
update all import sites to use the new named import (import { WelcomeEmail }
from './emails/welcome'). Run the provided ripgrep command to find and update
usages and ensure TypeScript/ESLint passes after renaming.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
internal-packages/emails/emails/invite.tsx (1)

1-11: Correct Tailwind import path (resolved prior warning).

Tailwind is now imported from @react-email/tailwind, not from @react-email/components. Looks good.

🧹 Nitpick comments (5)
internal-packages/emails/emails/invite.tsx (5)

17-23: Tighten schema: validate inviterEmail and restrict inviteLink host.

Prevents malformed emails and accidental/phishing domains if data ever becomes user-influenced.

Apply:

 export const InviteEmailSchema = z.object({
   email: z.literal("invite"),
   orgName: z.string(),
   inviterName: z.string().optional(),
-  inviterEmail: z.string(),
-  inviteLink: z.string().url(),
+  inviterEmail: z.string().email(),
+  inviteLink: z
+    .string()
+    .url()
+    .refine((u) => {
+      try {
+        return new URL(u).hostname.endsWith("trigger.dev");
+      } catch {
+        return false;
+      }
+    }, { message: "Invite link must be a trigger.dev URL" }),
 });

Would you like me to update the other email schemas similarly?


35-37: Minor: drop auto margins on Body.

Email clients often ignore body auto-centering; the Container already centers.

-        <Body className="bg-[#121317] my-auto mx-auto font-sans">
+        <Body className="bg-[#121317] font-sans">
           <Container className="my-[40px] mx-auto p-[20px] max-w-[600px]">

61-67: Harden external link and improve long-URL wrapping.

Add rel to mitigate opener leaks; wrap long URLs to prevent layout overflow.

-              <Link
-                href={inviteLink}
-                target="_blank"
-                className="text-[#6366F1] text-[14px] no-underline"
-              >
+              <Link
+                href={inviteLink}
+                target="_blank"
+                rel="noopener noreferrer"
+                className="text-[#6366F1] text-[14px] no-underline break-all"
+              >
                 {inviteLink}
               </Link>

25-31: Avoid default export; use a named export per repo guidelines.

Keeps imports consistent and avoids refactor hazards.

-export default function Email({
+export function InviteEmail({
   orgName,
   inviterName,
   inviterEmail,
   inviteLink,
 }: z.infer<typeof InviteEmailSchema>) {

After this change, update call sites importing the default. To find them:

#!/bin/bash
rg -nP -C2 'import\s+Email\s+from\s+["\'][^"\']*invite["\']|import\s+[^}]*\*\s+as\s+Email\s+from\s+["\'][^"\']*invite'

38-45: Image props: use numeric dimensions and confirm absolute URL resolution.

@react-email Image-like components typically expect numeric width/height; also ensure path resolves to an absolute URL in prod emails.

-              <Image
-                path="/emails/logo.png"
-                width="180px"
-                height="32px"
+              <Image
+                path="/emails/logo.png"
+                width={180}
+                height={32}
                 alt="Trigger.dev"
                 className="mt-0 mb-12"
               />

To verify the Image component API and URL handling:

#!/bin/bash
fd -a '^Image\.(tsx|jsx)$' internal-packages | xargs -I{} sh -c 'echo "== {} =="; sed -n "1,160p" "{}"'
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a42c799 and d3a7fcc.

📒 Files selected for processing (4)
  • internal-packages/emails/emails/invite.tsx (2 hunks)
  • internal-packages/emails/emails/magic-link.tsx (1 hunks)
  • internal-packages/emails/emails/welcome.tsx (1 hunks)
  • internal-packages/emails/package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal-packages/emails/emails/welcome.tsx
  • internal-packages/emails/emails/magic-link.tsx
  • internal-packages/emails/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/emails/emails/invite.tsx

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
internal-packages/emails/src/transports/smtp.ts (1)

25-33: Add plaintext fallback using toPlainText
Render your HTML once, then pass a text field generated via toPlainText (the { plainText: true } option is deprecated). For example:

+ import { toPlainText } from '@react-email/render';
  // …
- const html = await render(react);
+ const html = await render(react);
  await this.#client.sendMail({
    from,
    to,
    replyTo,
    subject,
-   html: html,
+   html: html,
+   text: toPlainText(html),
  });
internal-packages/emails/src/transports/aws-ses.ts (3)

14-21: Allow SES region/creds injection for env parity and testing

Let consumers pass a prebuilt SESClient or config (region, credentials). This avoids relying on ambient env and eases testing.

Apply this diff:

 export type AwsSesMailTransportOptions = {
-  type: "aws-ses";
+  type: "aws-ses";
+  client?: awsSes.SESClient;
+  clientConfig?: awsSes.SESClientConfig;
 };
 
 export class AwsSesMailTransport implements MailTransport {
   #client: nodemailer.Transporter;
 
   constructor(options: AwsSesMailTransportOptions) {
-    const ses = new awsSes.SESClient();
+    const ses = options.client ?? new awsSes.SESClient(options.clientConfig);
 
     this.#client = nodemailer.createTransport({
       SES: {
         aws: awsSes,
-        ses,
-      },
-    });
+        ses,
+      },
+    });
   }

33-43: Log unknown errors too (nit)

If a non-Error is thrown, we skip logging. Add a minimal fallback log for easier incident triage.

-    } catch (error) {
+    } catch (error) {
       if (error instanceof Error) {
         console.error(
           `Failed to send email to ${to}, ${subject}. Error ${error.name}: ${error.message}`
         );
         throw new EmailError(error);
       } else {
-        throw error;
+        console.error(`Failed to send email to ${to}, ${subject}. Unknown error:`, error);
+        throw error;
       }
     }

24-33: Include plain text alternative in AWS SES transport
Add a text field using toPlainText from @react-email/render alongside html for better inbox placement. Example diff:

--- a/internal-packages/emails/src/transports/aws-ses.ts
+++ b/internal-packages/emails/src/transports/aws-ses.ts
@@ -1,5 +1,6 @@
-import { render } from "@react-email/render";
+import { render, toPlainText } from "@react-email/render";
 import { EmailError, MailMessage, MailTransport, PlainTextMailMessage } from "./index";
 import nodemailer from "nodemailer";
 import * as awsSes from "@aws-sdk/client-ses";
@@ -28,7 +29,10 @@ export class AwsSesMailTransport implements MailTransport {
   async send({ to, from, replyTo, subject, react }: MailMessage): Promise<void> {
+    const html = await render(react);
+    const text = toPlainText(html);
     try {
       await this.#client.sendMail({
         from,
         to,
         replyTo,
         subject,
-        html: await render(react),
+        html,
+        text,
       });
     } catch (error) {
       // ...

Use toPlainText(html) since the plainText option on render is deprecated (react.email)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3a7fcc and 6677844.

📒 Files selected for processing (2)
  • internal-packages/emails/src/transports/aws-ses.ts (3 hunks)
  • internal-packages/emails/src/transports/smtp.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/emails/src/transports/smtp.ts
  • internal-packages/emails/src/transports/aws-ses.ts
🧬 Code graph analysis (1)
internal-packages/emails/src/transports/aws-ses.ts (1)
internal-packages/emails/src/transports/index.ts (3)
  • MailTransport (23-26)
  • MailMessage (7-13)
  • PlainTextMailMessage (15-21)
⏰ 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). (2)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
internal-packages/emails/src/transports/smtp.ts (1)

32-32: Async render usage looks correct

Awaiting render(react) aligns with newer react-email versions and is safe even if render is sync.

internal-packages/emails/src/transports/aws-ses.ts (3)

3-4: Imports LGTM

Switch to explicit * as awsSes and keeping nodemailer import is fine for SES v3 transport.


7-8: Literal type formatting change is harmless

The string-literal type "aws-ses" is consistent. No behavior change.


45-54: Plaintext path unchanged — LGTM

sendPlainText remains straightforward and consistent with the transport interface.

nicktrn and others added 9 commits September 24, 2025 14:33
* fix: project scoping for runs

* Apply some 🐰 suggestions
* fix: org invite scoping

Fixes some scoping issues with team invites.

* Fix invite flow changes
…sage (#2549)

* fix: org scoping in the select plan flow

Adds proper org scoping in the loader and action in the plans page.

* Fix billing alerts scope

* Fix org usage page scope

* Fix token revoking flow scope check

* Throw error for failed PAT revokes instead of silent failure
myftija and others added 13 commits October 4, 2025 08:05
* fix: use higher entropy invite tokens

We currently use CUIDs for invite tokens, which are generated using
a pattern and are not cryptographically secure. This PR switches to
a higher entropy string generated with `nanoid`.

* Dedupe the invite emails in the application
…NISHED execution status (#2564)

* feat(server): add two admin endpoints for queue and environment concurrency debugging and repairing
feat(run-engine): ability to repair runs in QUEUED, SUSPENDED, and FINISHED execution status

* Handle FINISHED snapshot in the repair
Sets `Referrer-Policy`, `X-Content-Type-Options` and `Permissions-Policy` headers.
Relevant against certain types of attacks.
* feat(webapp): rate limit magic-link login attempts

Adds a simple rate limiter to the login with magic link flow. Similar implementation to the MFA rate limits.

* Fix error message

* Add an env var feature flags for login rate limiting

* Use BoolEnv instead of `0`/`1`

* Parse xff properly
* feat(helm): support topology spread constraints

* chore(helm): update topology env var

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* chore(helm): limit spread constraints to webapp for now

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…utility images (#2574)

* chore(docker): use bitnami legacy repo

* chore(helm): use bitnami legacy repo

* Make Helm webapp chart images configurable

Adds configurability for init and token syncer container images through
new values in the Helm chart configuration

* chore(helm): refactor utility image config

* chore(helm): bump chart version to 4.0.3

---------

Co-authored-by: LeoKaynan <[email protected]>
Co-authored-by: mintlify[bot] <109931778+mintlify[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal-packages/run-engine/src/run-queue/index.ts (1)

933-957: Thread the optional concurrency key through the clear helper.

To make the engine fix effective, clearMessageFromConcurrencySets must accept the run’s concurrencyKey and use it when computing the Redis keys (both queue and dequeued variants). Right now we always build the “no concurrency key” variant and miss entries for runs that were partitioned by concurrency key, so nothing is removed. Please add the optional argument and forward it in the key helpers.

-  public async clearMessageFromConcurrencySets(params: {
-    runId: string;
-    orgId: string;
-    queue: string;
-    env: RunQueueKeyProducerEnvironment;
-  }) {
+  public async clearMessageFromConcurrencySets(params: {
+    runId: string;
+    orgId: string;
+    queue: string;
+    concurrencyKey?: string;
+    env: RunQueueKeyProducerEnvironment;
+  }) {
     return this.#callClearMessageFromConcurrencySets(params);
   }
…
-  async #callClearMessageFromConcurrencySets({
-    runId,
-    orgId,
-    queue,
-    env,
-  }: {
+  async #callClearMessageFromConcurrencySets({
+    runId,
+    orgId,
+    queue,
+    concurrencyKey,
+    env,
+  }: {
     runId: string;
     orgId: string;
     queue: string;
+    concurrencyKey?: string;
     env: RunQueueKeyProducerEnvironment;
   }) {
     const messageId = runId;
     const messageKey = this.keys.messageKey(orgId, messageId);
-    const queueCurrentConcurrencyKey = this.keys.queueCurrentConcurrencyKey(env, queue);
+    const queueCurrentConcurrencyKey = this.keys.queueCurrentConcurrencyKey(
+      env,
+      queue,
+      concurrencyKey
+    );
     const envCurrentConcurrencyKey = this.keys.envCurrentConcurrencyKey(env);
-    const queueCurrentDequeuedKey = this.keys.queueCurrentDequeuedKey(env, queue);
+    const queueCurrentDequeuedKey = this.keys.queueCurrentDequeuedKey(env, queue, concurrencyKey);
     const envCurrentDequeuedKey = this.keys.envCurrentDequeuedKey(env);
…
     return this.redis.clearMessageFromConcurrencySets(
       queueCurrentConcurrencyKey,
       envCurrentConcurrencyKey,
       queueCurrentDequeuedKey,
       envCurrentDequeuedKey,
       messageId
     );
   }
🧹 Nitpick comments (2)
apps/webapp/app/models/member.server.ts (1)

260-299: LGTM! Excellent security improvement.

The membership verification (checking that the user is a member of the organization via orgSlug) before allowing invite revocation prevents unauthorized users from revoking invites.

Optional: Consider atomic delete operation.

The current implementation uses findFirst followed by a separate delete, which is not atomic. While this is acceptable for invite revocation (low risk of race conditions), you could optionally combine these into a single delete with the full where clause:

const invite = await prisma.orgMemberInvite.delete({
  where: {
    id: inviteId,
    organization: {
      slug: orgSlug,
      members: {
        some: {
          userId,
        },
      },
    },
  },
  select: {
    id: true,
    email: true,
    organization: true,
  },
});

if (!invite) {
  throw new Error("Invite not found");
}

This would make the operation atomic and slightly more efficient by removing the extra query.

apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts (1)

69-95: Add pagination metadata or remove incomplete pagination.

The loader applies skip/take pagination but still returns only the report body, so callers have no way to discover the total queue count or next page. Either expose pagination metadata (e.g., total, page, perPage) alongside the report or drop pagination to always return the full queue set.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6677844 and 03e6bce.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (42)
  • apps/webapp/app/env.server.ts (1 hunks)
  • apps/webapp/app/models/member.server.ts (7 hunks)
  • apps/webapp/app/presenters/v3/RunPresenter.server.ts (1 hunks)
  • apps/webapp/app/presenters/v3/SpanPresenter.server.ts (4 hunks)
  • apps/webapp/app/root.tsx (1 hunks)
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx (0 hunks)
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx (2 hunks)
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing/route.tsx (2 hunks)
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (1 hunks)
  • apps/webapp/app/routes/account.tokens/route.tsx (5 hunks)
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts (1 hunks)
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts (1 hunks)
  • apps/webapp/app/routes/invite-accept.tsx (1 hunks)
  • apps/webapp/app/routes/invite-resend.tsx (2 hunks)
  • apps/webapp/app/routes/invite-revoke.tsx (2 hunks)
  • apps/webapp/app/routes/invites.tsx (4 hunks)
  • apps/webapp/app/routes/login.magic/route.tsx (3 hunks)
  • apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts (1 hunks)
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (2 hunks)
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (2 hunks)
  • apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts (3 hunks)
  • apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts (1 hunks)
  • apps/webapp/app/services/magicLinkRateLimiter.server.ts (1 hunks)
  • apps/webapp/app/services/personalAccessToken.server.ts (1 hunks)
  • apps/webapp/app/v3/eventRepository.server.ts (7 hunks)
  • docker/dev-compose.yml (1 hunks)
  • docker/docker-compose.yml (1 hunks)
  • docs/idempotency.mdx (0 hunks)
  • hosting/docker/webapp/docker-compose.yml (2 hunks)
  • hosting/k8s/helm/Chart.yaml (1 hunks)
  • hosting/k8s/helm/templates/_helpers.tpl (1 hunks)
  • hosting/k8s/helm/templates/webapp.yaml (3 hunks)
  • hosting/k8s/helm/values.yaml (7 hunks)
  • internal-packages/run-engine/package.json (1 hunks)
  • internal-packages/run-engine/src/engine/index.ts (12 hunks)
  • internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1 hunks)
  • internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (1 hunks)
  • internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (8 hunks)
  • internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (2 hunks)
  • internal-packages/run-engine/src/engine/types.ts (2 hunks)
  • internal-packages/run-engine/src/engine/workerCatalog.ts (1 hunks)
  • internal-packages/run-engine/src/run-queue/index.ts (15 hunks)
💤 Files with no reviewable changes (2)
  • docs/idempotency.mdx
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
✅ Files skipped from review due to trivial changes (1)
  • apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/run-engine/src/engine/types.ts
  • apps/webapp/app/presenters/v3/RunPresenter.server.ts
  • internal-packages/run-engine/src/engine/workerCatalog.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx
  • apps/webapp/app/services/magicLinkRateLimiter.server.ts
  • apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts
  • apps/webapp/app/routes/invite-resend.tsx
  • apps/webapp/app/root.tsx
  • apps/webapp/app/routes/invite-accept.tsx
  • apps/webapp/app/routes/invites.tsx
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts
  • internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts
  • apps/webapp/app/routes/login.magic/route.tsx
  • apps/webapp/app/services/personalAccessToken.server.ts
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts
  • apps/webapp/app/routes/account.tokens/route.tsx
  • apps/webapp/app/presenters/v3/SpanPresenter.server.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing/route.tsx
  • internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts
  • apps/webapp/app/models/member.server.ts
  • apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx
  • internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
  • apps/webapp/app/v3/eventRepository.server.ts
  • internal-packages/run-engine/src/engine/systems/dequeueSystem.ts
  • internal-packages/run-engine/src/run-queue/index.ts
  • internal-packages/run-engine/src/engine/index.ts
  • apps/webapp/app/routes/invite-revoke.tsx
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/presenters/v3/RunPresenter.server.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx
  • apps/webapp/app/services/magicLinkRateLimiter.server.ts
  • apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts
  • apps/webapp/app/routes/invite-resend.tsx
  • apps/webapp/app/root.tsx
  • apps/webapp/app/routes/invite-accept.tsx
  • apps/webapp/app/routes/invites.tsx
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts
  • apps/webapp/app/routes/login.magic/route.tsx
  • apps/webapp/app/services/personalAccessToken.server.ts
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts
  • apps/webapp/app/routes/account.tokens/route.tsx
  • apps/webapp/app/presenters/v3/SpanPresenter.server.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing/route.tsx
  • apps/webapp/app/models/member.server.ts
  • apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx
  • apps/webapp/app/v3/eventRepository.server.ts
  • apps/webapp/app/routes/invite-revoke.tsx
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/presenters/v3/RunPresenter.server.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx
  • apps/webapp/app/services/magicLinkRateLimiter.server.ts
  • apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts
  • apps/webapp/app/routes/invite-resend.tsx
  • apps/webapp/app/root.tsx
  • apps/webapp/app/routes/invite-accept.tsx
  • apps/webapp/app/routes/invites.tsx
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts
  • apps/webapp/app/routes/login.magic/route.tsx
  • apps/webapp/app/services/personalAccessToken.server.ts
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts
  • apps/webapp/app/routes/account.tokens/route.tsx
  • apps/webapp/app/presenters/v3/SpanPresenter.server.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing/route.tsx
  • apps/webapp/app/models/member.server.ts
  • apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx
  • apps/webapp/app/v3/eventRepository.server.ts
  • apps/webapp/app/routes/invite-revoke.tsx
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Files:

  • apps/webapp/app/presenters/v3/RunPresenter.server.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/services/magicLinkRateLimiter.server.ts
  • apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts
  • apps/webapp/app/services/personalAccessToken.server.ts
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts
  • apps/webapp/app/presenters/v3/SpanPresenter.server.ts
  • apps/webapp/app/models/member.server.ts
  • apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts
  • apps/webapp/app/v3/eventRepository.server.ts
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

  • apps/webapp/app/presenters/v3/RunPresenter.server.ts
  • apps/webapp/app/env.server.ts
  • apps/webapp/app/services/magicLinkRateLimiter.server.ts
  • apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts
  • apps/webapp/app/services/personalAccessToken.server.ts
  • apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts
  • apps/webapp/app/presenters/v3/SpanPresenter.server.ts
  • apps/webapp/app/models/member.server.ts
  • apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts
  • apps/webapp/app/v3/eventRepository.server.ts
🧠 Learnings (3)
📚 Learning: 2025-06-25T13:18:44.103Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/Chart.yaml:1-18
Timestamp: 2025-06-25T13:18:44.103Z
Learning: For the Trigger.dev Helm chart, the chart name should be "trigger" (not "trigger-v4") since this is the first official chart release. Helper templates should use the actual chart name from .Chart.Name rather than hardcoded prefixes.

Applied to files:

  • hosting/k8s/helm/Chart.yaml
📚 Learning: 2025-06-25T13:20:17.174Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/values.yaml:22-51
Timestamp: 2025-06-25T13:20:17.174Z
Learning: In the Trigger.dev Helm chart values.yaml, the maintainer prefers to use explicit comprehensive warnings for security-sensitive default values rather than implementing secure-by-default behavior that would fail installation. The project uses deterministic default secrets with clear "TESTING ONLY" warnings and instructions for production deployment.

Applied to files:

  • hosting/k8s/helm/Chart.yaml
📚 Learning: 2025-06-25T13:18:04.827Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/templates/extra-manifests.yaml:1-4
Timestamp: 2025-06-25T13:18:04.827Z
Learning: In the Trigger.dev v4 Helm chart, the user prefers to rely on documentation and examples in values files rather than implementing defensive coding in templates, particularly for features like extraManifests where proper usage is documented.

Applied to files:

  • hosting/k8s/helm/Chart.yaml
🧬 Code graph analysis (22)
apps/webapp/app/env.server.ts (2)
apps/supervisor/src/envUtil.ts (1)
  • BoolEnv (15-17)
apps/webapp/app/utils/boolEnv.ts (1)
  • BoolEnv (12-14)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx (4)
apps/webapp/app/presenters/OrganizationsPresenter.server.ts (1)
  • userId (126-168)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/utils/pathBuilder.ts (2)
  • OrganizationParamsSchema (18-20)
  • organizationPath (92-94)
apps/webapp/app/features.server.ts (1)
  • featuresForRequest (22-25)
apps/webapp/app/services/magicLinkRateLimiter.server.ts (2)
apps/webapp/app/services/rateLimiter.server.ts (2)
  • createRedisRateLimitClient (74-105)
  • RateLimiter (21-72)
apps/webapp/app/env.server.ts (1)
  • env (1185-1185)
apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts (3)
apps/webapp/app/utils/pathBuilder.ts (1)
  • ProjectParamSchema (22-24)
apps/webapp/app/routes/resources.taskruns.$runParam.debug.ts (1)
  • loader (13-157)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/routes/invite-accept.tsx (1)
apps/webapp/app/models/message.server.ts (1)
  • redirectWithSuccessMessage (162-179)
apps/webapp/app/routes/invites.tsx (1)
apps/webapp/app/services/session.server.ts (1)
  • requireUser (39-62)
apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.ts (5)
apps/webapp/app/services/personalAccessToken.server.ts (1)
  • authenticateApiRequestWithPersonalAccessToken (110-119)
internal-packages/run-engine/src/engine/index.ts (1)
  • environment (1327-1381)
apps/webapp/app/v3/engineVersion.server.ts (1)
  • determineEngineVersion (17-76)
apps/webapp/app/db.server.ts (1)
  • $replica (103-106)
apps/webapp/app/v3/runEngine.server.ts (1)
  • engine (9-9)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
packages/core/src/v3/schemas/common.ts (2)
  • TaskRunError (200-205)
  • TaskRunError (207-207)
apps/webapp/app/routes/login.magic/route.tsx (2)
apps/webapp/app/env.server.ts (1)
  • env (1185-1185)
apps/webapp/app/services/magicLinkRateLimiter.server.ts (4)
  • checkMagicLinkIpRateLimit (89-96)
  • checkMagicLinkEmailRateLimit (71-78)
  • checkMagicLinkEmailDailyRateLimit (80-87)
  • MagicLinkRateLimitError (6-13)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (1)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.ts (4)
apps/webapp/app/services/personalAccessToken.server.ts (1)
  • authenticateApiRequestWithPersonalAccessToken (110-119)
apps/webapp/app/v3/engineVersion.server.ts (1)
  • determineEngineVersion (17-76)
apps/webapp/app/db.server.ts (1)
  • $replica (103-106)
apps/webapp/app/v3/runEngine.server.ts (1)
  • engine (9-9)
apps/webapp/app/routes/account.tokens/route.tsx (1)
apps/webapp/app/services/personalAccessToken.server.ts (1)
  • revokePersonalAccessToken (82-96)
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (2)
apps/webapp/app/v3/taskEventStore.server.ts (2)
  • getTaskEventStoreTableForRun (56-60)
  • TaskEventStoreTable (54-54)
apps/webapp/app/v3/eventRepository.server.ts (1)
  • eventRepository (1791-1791)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing/route.tsx (2)
apps/webapp/app/presenters/OrganizationsPresenter.server.ts (1)
  • userId (126-168)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts (2)
apps/webapp/app/presenters/OrganizationsPresenter.server.ts (1)
  • userId (126-168)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (3)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/utils/pathBuilder.ts (2)
  • OrganizationParamsSchema (18-20)
  • organizationPath (92-94)
apps/webapp/app/features.server.ts (1)
  • featuresForRequest (22-25)
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (1)
packages/core/src/utils.ts (1)
  • tryCatch (5-18)
apps/webapp/app/v3/eventRepository.server.ts (2)
apps/webapp/app/v3/taskEventStore.server.ts (1)
  • TaskEventStoreTable (54-54)
apps/webapp/app/v3/tracer.server.ts (1)
  • startActiveSpan (113-142)
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (2)
internal-packages/run-engine/src/run-queue/index.ts (5)
  • message (1458-1506)
  • message (1763-1818)
  • message (1859-1907)
  • message (1909-1937)
  • message (1958-1960)
packages/core/src/v3/logger/taskLogger.ts (1)
  • message (75-101)
internal-packages/run-engine/src/run-queue/index.ts (4)
internal-packages/run-engine/src/shared/index.ts (1)
  • MinimalAuthenticatedEnvironment (8-19)
internal-packages/run-engine/src/run-queue/types.ts (1)
  • RunQueueKeyProducerEnvironment (49-52)
internal-packages/run-engine/src/run-queue/keyProducer.ts (4)
  • queueCurrentConcurrencyKey (157-165)
  • envCurrentConcurrencyKey (199-217)
  • queueCurrentDequeuedKey (149-155)
  • envCurrentDequeuedKey (231-247)
internal-packages/run-engine/src/engine/index.ts (3)
  • runId (1230-1262)
  • runId (1383-1660)
  • runId (1662-1774)
internal-packages/run-engine/src/engine/index.ts (5)
internal-packages/run-engine/src/run-queue/index.ts (3)
  • options (1126-1142)
  • RunQueue (163-2721)
  • runId (1820-1857)
internal-packages/run-engine/src/run-queue/fairQueueSelectionStrategy.ts (5)
  • FairQueueSelectionStrategy (92-625)
  • queue (618-624)
  • snapshot (170-209)
  • queues (265-313)
  • queues (452-499)
internal-packages/run-engine/src/shared/index.ts (1)
  • AuthenticatedEnvironment (4-6)
internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (1)
  • getLatestExecutionSnapshot (95-113)
internal-packages/run-engine/src/engine/types.ts (1)
  • ReportableQueue (155-161)
apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (1)
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (5)
  • prisma (763-918)
  • prisma (920-950)
  • prisma (952-979)
  • prisma (981-1014)
  • prisma (1016-1094)
⏰ 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). (1)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (37)
apps/webapp/app/routes/resources.taskruns.$runParam.cancel.ts (3)

2-2: LGTM! Clean TypeScript imports.

The type-only import for ActionFunction and the addition of requireUserId follow TypeScript best practices and set up the authentication layer.

Also applies to: 7-7


19-19: Critical security improvement: authentication enforced.

Retrieving userId upfront ensures that only authenticated users can access this cancellation endpoint. This prevents anonymous or unauthenticated requests from proceeding.


30-42: Critical security improvement: authorization properly enforced.

The migration from findUnique to findFirst with nested organization membership checks ensures that users can only cancel runs belonging to organizations they are members of. This prevents privilege escalation where a user might cancel runs in organizations they don't have access to.

The pattern members: { some: { userId } } is consistent with authorization checks elsewhere in the codebase (e.g., OrganizationsPresenter.server.ts), and returning "Run not found" when access is denied correctly prevents information leakage about run existence.

apps/webapp/app/routes/invite-revoke.tsx (1)

2-2: LGTM!

The type-only import for ActionFunction and the parameter rename to orgSlug correctly align with the updated revokeInvite signature.

Also applies to: 25-29

apps/webapp/app/routes/invite-resend.tsx (1)

2-2: LGTM!

The type-only import for ActionFunction and the addition of the userId parameter correctly align with the updated resendInvite signature.

Also applies to: 26-29

apps/webapp/app/routes/invite-accept.tsx (1)

21-25: LGTM! Excellent security improvement.

The early authentication guard prevents unauthenticated users from proceeding with invite acceptance, providing a clear user-facing message to log in first.

apps/webapp/app/services/personalAccessToken.server.ts (1)

82-96: LGTM! Excellent security and error handling improvements.

Adding userId to the revocation check ensures users can only revoke their own tokens, preventing unauthorized access. The explicit count check provides clear feedback when the token is not found or already revoked.

apps/webapp/app/routes/invites.tsx (1)

3-3: LGTM! Good refactor to use user objects.

The shift from requireUserId to requireUser and passing a user object instead of just userId makes the data dependencies more explicit and aligns with the updated acceptInvite and declineInvite signatures.

Also applies to: 39-39, 50-53, 65-68

apps/webapp/app/routes/account.tokens/route.tsx (1)

6-7: LGTM! Excellent type safety and cleanup.

The type-only imports, removal of the unused params parameter, addition of userId for secure token revocation, and the satisfies never check all represent best practices for TypeScript and security.

Also applies to: 55-55, 115-115, 127-127

apps/webapp/app/models/member.server.ts (4)

1-6: LGTM! Good token generation setup.

Using nanoid with a custom alphabet (excluding confusing characters like 0 and l) and a 40-character length provides strong security for invite tokens.


102-115: LGTM! Excellent security improvement.

Generating a unique token for each invite (instead of reusing tokens) strengthens security and enables better tracking of invite usage.


159-212: LGTM! Good refactor to user object pattern.

Using a user object { id, email } instead of separate parameters makes the required data explicit and improves type safety across the invite acceptance and decline flows.

Also applies to: 214-242


244-258: LGTM! Good security improvement.

Requiring userId and verifying inviterId matches ensures only the original inviter can resend an invite.

internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (1)

219-220: Verify pagination semantics
Ordering by createdAt: "desc" with take: 50 then reversing returns the latest 50 snapshots, skipping any older snapshots if more than 50 exist. Only caller found at internal-packages/run-engine/src/engine/index.ts:1124. If you intend to page forward from the cursor, use orderBy: { createdAt: "asc" } and drop the reverse().

hosting/k8s/helm/Chart.yaml (1)

5-5: Helm chart version bump is appropriate.

The version increment from 4.0.1 to 4.0.3 correctly reflects the patch-level changes in this release, which primarily introduce legacy Bitnami image support and topology constraints without altering core functionality.

docker/docker-compose.yml (1)

79-79: Image update is consistent with related files.

The ClickHouse image migration to bitnamilegacy/clickhouse:latest aligns with the same change in docker/dev-compose.yml and hosting/docker/webapp/docker-compose.yml.

hosting/docker/webapp/docker-compose.yml (1)

142-142: Image migrations are consistent and use parameterized tags.

Both ClickHouse and MinIO image updates to bitnamilegacy namespace use environment variables for tags (${CLICKHOUSE_IMAGE_TAG:-latest} and ${MINIO_IMAGE_TAG:-latest}), providing deployment flexibility while defaulting to latest.

Also applies to: 186-186

hosting/k8s/helm/templates/webapp.yaml (3)

70-78: Template-driven image resolution improves maintainability.

The initContainer now uses {{ include "trigger-v4.webapp.volumePermissions.image" . }} for image resolution, centralizing configuration in values.yaml and enabling easier image registry overrides.


80-82: Token syncer image now template-driven.

Consistent with the initContainer change, the token-syncer sidecar now uses {{ include "trigger-v4.webapp.tokenSyncer.image" . }} for centralized image management.


424-427: Topology spread constraints provide pod distribution control.

The optional topologySpreadConstraints block enables users to configure pod distribution across failure domains. Using tpl allows dynamic templating of constraint values (e.g., referencing {{ .Values.webapp.fullname }}), which is appropriate for this use case.

hosting/k8s/helm/templates/_helpers.tpl (1)

98-124: Image helper templates follow established patterns.

The new trigger-v4.webapp.volumePermissions.image and trigger-v4.webapp.tokenSyncer.image helpers correctly implement the registry resolution hierarchy (global.imageRegistry with component fallback) and follow the same structure as existing helpers like trigger-v4.image.

hosting/k8s/helm/values.yaml (3)

51-65: Init and sidecar image configurations are well-structured.

The webapp.volumePermissions and webapp.tokenSyncer image blocks follow Helm best practices with explicit registry, repository, tag, and pullPolicy fields. The busybox image for volume permissions is a standard choice, and kubectl for token syncing is appropriate.


91-92: Topology spread constraints provide deployment flexibility.

The empty topologySpreadConstraints: [] list is a good default that allows users to optionally configure pod distribution across failure domains without requiring changes to templates.


383-386: Legacy Bitnami image repositories are consistently configured.

All subchart image repositories (PostgreSQL, Redis, ClickHouse, MinIO/S3) are consistently updated to use bitnamilegacy/* repositories. The commented examples showing specific image tags (e.g., docker.io/bitnamilegacy/postgresql:17.5.0-debian-12-r12) are helpful for users.

Based on learnings: The Trigger.dev Helm chart maintainers prefer explicit comprehensive warnings for security-sensitive defaults rather than implementing secure-by-default behavior that would fail installation, as seen in previous feedback on the chart.

Also applies to: 438-441, 533-536, 600-603

docker/dev-compose.yml (1)

51-51: Validate Bitnami legacy image usage and plan migration

bitnamilegacy is Bitnami’s official read-only legacy namespace (images moved from bitnami in Aug-Sep 2025). These tags receive no further updates or support—ensure you migrate to actively maintained images (e.g., Bitnami Secure Images or alternatives).

internal-packages/run-engine/package.json (1)

34-34: LGTM - p-map dependency addition.

The addition of p-map at version ^6.0.0 is appropriate for the parallel processing needs described in the AI summary. The version constraint allows for minor and patch updates within the v6 line, which is suitable for this use case.

internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (3)

1143-1143: LGTM - batchId propagation.

The addition of the optional batchId parameter to tryNackAndRequeue and its propagation to createExecutionSnapshot is consistent with the broader batch handling enhancements in this PR.

Also applies to: 1163-1163, 1212-1212


1901-1912: Verify truncation limits don't lose critical debugging information.

The error truncation implementation looks correct, but the limits warrant verification:

  • name: 1024 characters
  • message: 16 KB (16,384 characters)
  • stackTrace: 16 KB (16,384 characters)

While these limits will prevent database bloat and protect against malicious payloads, ensure they're sufficient for typical error scenarios. Consider whether truncated errors should include an indicator (e.g., "... [truncated]") so developers know information was lost.

Would you like me to generate a test to verify these limits work well with common error patterns?

Also applies to: 1923-1929


1482-1483: LGTM - consistent error truncation applied.

The truncated error is correctly applied in all relevant locations:

  • Updating taskRun error field (line 1492)
  • Waitpoint output during failure (line 1554)
  • Run failure notifications (via the truncated error passed to event emission)

Also applies to: 1492-1492, 1554-1554

internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)

146-153: LGTM - improved observability.

The logging statement captures essential context about dequeued messages at an appropriate log level. This aligns with the broader observability and reporting enhancements introduced in this PR.

apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (1)

81-81: LGTM - consistent membership enforcement.

The addition of requireUserId and passing userId to the presenter aligns with the broader pattern in this PR of enforcing organization membership at the route level. This ensures only authorized users can access span data.

Also applies to: 84-84, 95-95

apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.usage/route.tsx (1)

49-49: LGTM - security improvement with membership enforcement.

The change from findUnique to findFirst with a membership filter (members: { some: { userId } }) properly restricts access to organizations where the user is a member. This is a security improvement that prevents unauthorized access to organization usage data.

Also applies to: 57-59

apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing-alerts/route.tsx (2)

50-50: LGTM - consistent membership enforcement.

The membership-based access control follows the same secure pattern as other settings routes in this PR, ensuring only organization members can manage billing alerts.

Also applies to: 58-60


184-184: Verify the new alert level thresholds are intentional.

The alert levels now include 200% and 500% thresholds. While this provides more flexibility for monitoring overage, these high percentages seem unusual for billing alerts where users typically want to be notified before or shortly after exceeding their budget.

Confirm that:

  1. Users actually need these high threshold alerts
  2. The UI clearly communicates what 200% and 500% mean (e.g., 2x and 5x the set amount)
  3. There are no unintended consequences of these large multipliers
apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.runs.$runParam.ts (2)

6-6: LGTM - unused import removed.

The removal of v3DeploymentPath is correct as it's no longer referenced in this file.


13-13: LGTM - membership-based access control.

The query now properly scopes access by requiring:

  1. The project matches the slug
  2. The organization matches the slug
  3. The user is a member of the organization

This prevents unauthorized access to run data and aligns with the security improvements throughout this PR.

Also applies to: 21-28

apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.billing/route.tsx (1)

31-31: LGTM - consistent membership enforcement.

The membership-based access control matches the security pattern applied to other organization settings routes in this PR, ensuring only authorized members can access billing information.

Also applies to: 44-46

import { parse } from "@conform-to/zod";
import { ActionFunction, json } from "@remix-run/server-runtime";
import { type ActionFunction, json } from "@remix-run/server-runtime";
import { env } from "process";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix environment variable import to comply with coding guidelines.

This file imports env from process, which violates the coding guidelines. Files under apps/webapp/app/routes/**/*.ts should access environment variables only via the env export from app/env.server.ts.

Apply this diff:

-import { env } from "process";
+import { env } from "~/env.server";

Based on coding guidelines.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { env } from "process";
import { env } from "~/env.server";
🤖 Prompt for AI Agents
In apps/webapp/app/routes/invite-resend.tsx around line 3, the file imports env
from node's process which violates the guideline; replace that import with the
project's centralized env export (import { env } from "app/env.server") and
update any uses to reference that env export. Ensure you remove the import from
"process", add the import from "app/env.server", and run a quick typecheck to
confirm no other references need adjustment.

Comment on lines +172 to +177
const extractClientIp = (xff: string | null) => {
if (!xff) return null;

const parts = xff.split(",").map((p) => p.trim());
return parts[parts.length - 1]; // take last item, ALB appends the real client IP by default
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use the first X-Forwarded-For entry for the client IP

X-Forwarded-For is ordered left→right. The first value is the original client, while later entries are intermediate proxies (ALB/Nginx/etc.). Taking the last element means you rate-limit on the load balancer’s IP, so every user shares the same quota. Please switch to the first trimmed entry (and guard against empty strings).

Apply this diff to fix the bug:

-  const parts = xff.split(",").map((p) => p.trim());
-  return parts[parts.length - 1]; // take last item, ALB appends the real client IP by default
+  const parts = xff.split(",").map((p) => p.trim()).filter(Boolean);
+  return parts[0] ?? null; // the original client IP is the left-most entry
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const extractClientIp = (xff: string | null) => {
if (!xff) return null;
const parts = xff.split(",").map((p) => p.trim());
return parts[parts.length - 1]; // take last item, ALB appends the real client IP by default
};
const extractClientIp = (xff: string | null) => {
if (!xff) return null;
const parts = xff.split(",").map((p) => p.trim()).filter(Boolean);
return parts[0] ?? null; // the original client IP is the left-most entry
};
🤖 Prompt for AI Agents
In apps/webapp/app/routes/login.magic/route.tsx around lines 172 to 177, the
extractClientIp function currently returns the last X-Forwarded-For entry
(giving the proxy/ALB IP) instead of the original client IP; update it to split
and trim the header, then return the first non-empty entry (guarding against
empty strings and returning null if none exist) so rate-limiting uses the true
client IP.

Comment on lines +74 to +95
if (!result.success) {
const retryAfter = new Date(result.reset).getTime() - Date.now();
throw new MagicLinkRateLimitError(retryAfter);
}
}

export async function checkMagicLinkEmailDailyRateLimit(identifier: string): Promise<void> {
const result = await magicLinkEmailDailyRateLimiter.limit(identifier);

if (!result.success) {
const retryAfter = new Date(result.reset).getTime() - Date.now();
throw new MagicLinkRateLimitError(retryAfter);
}
}

export async function checkMagicLinkIpRateLimit(ip: string): Promise<void> {
const result = await magicLinkIpRateLimiter.limit(ip);

if (!result.success) {
const retryAfter = new Date(result.reset).getTime() - Date.now();
throw new MagicLinkRateLimitError(retryAfter);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix retryAfter calculation from Upstash reset timestamp

Ratelimit.limit() returns reset as a Unix timestamp (seconds). Wrapping it in new Date(reset) yields a date in 1970, so subtracting Date.now() produces a large negative number. Any consumer of MagicLinkRateLimitError.retryAfter would emit bogus values (e.g. a negative Retry-After header). Convert the value to milliseconds before subtracting, and clamp at ≥0. Please apply the same fix in the other two helpers.

Apply this diff to fix the bug:

-    const retryAfter = new Date(result.reset).getTime() - Date.now();
+    const resetValue =
+      result.reset instanceof Date ? result.reset.getTime() : Number(result.reset);
+    const resetMs = resetValue < 1_000_000_000_000 ? resetValue * 1000 : resetValue;
+    const retryAfter = Math.max(0, resetMs - Date.now());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!result.success) {
const retryAfter = new Date(result.reset).getTime() - Date.now();
throw new MagicLinkRateLimitError(retryAfter);
}
}
export async function checkMagicLinkEmailDailyRateLimit(identifier: string): Promise<void> {
const result = await magicLinkEmailDailyRateLimiter.limit(identifier);
if (!result.success) {
const retryAfter = new Date(result.reset).getTime() - Date.now();
throw new MagicLinkRateLimitError(retryAfter);
}
}
export async function checkMagicLinkIpRateLimit(ip: string): Promise<void> {
const result = await magicLinkIpRateLimiter.limit(ip);
if (!result.success) {
const retryAfter = new Date(result.reset).getTime() - Date.now();
throw new MagicLinkRateLimitError(retryAfter);
}
if (!result.success) {
const resetValue =
result.reset instanceof Date ? result.reset.getTime() : Number(result.reset);
const resetMs = resetValue < 1_000_000_000_000 ? resetValue * 1000 : resetValue;
const retryAfter = Math.max(0, resetMs - Date.now());
throw new MagicLinkRateLimitError(retryAfter);
}
}
export async function checkMagicLinkEmailDailyRateLimit(identifier: string): Promise<void> {
const result = await magicLinkEmailDailyRateLimiter.limit(identifier);
if (!result.success) {
const resetValue =
result.reset instanceof Date ? result.reset.getTime() : Number(result.reset);
const resetMs = resetValue < 1_000_000_000_000 ? resetValue * 1000 : resetValue;
const retryAfter = Math.max(0, resetMs - Date.now());
throw new MagicLinkRateLimitError(retryAfter);
}
}
export async function checkMagicLinkIpRateLimit(ip: string): Promise<void> {
const result = await magicLinkIpRateLimiter.limit(ip);
if (!result.success) {
const resetValue =
result.reset instanceof Date ? result.reset.getTime() : Number(result.reset);
const resetMs = resetValue < 1_000_000_000_000 ? resetValue * 1000 : resetValue;
const retryAfter = Math.max(0, resetMs - Date.now());
throw new MagicLinkRateLimitError(retryAfter);
}
}
🤖 Prompt for AI Agents
In apps/webapp/app/services/magicLinkRateLimiter.server.ts around lines 74 to
95, the retryAfter is computed from result.reset (a Unix timestamp in seconds)
by passing it to new Date(), which yields a 1970 date and a negative value;
convert the reset value to milliseconds (e.g. Number(result.reset) * 1000)
before subtracting Date.now(), and clamp the result to a minimum of 0
(Math.max(0, ...)) so MagicLinkRateLimitError.retryAfter is always a
non-negative millisecond duration; apply the same conversion and clamping to the
other two helper blocks in this range.

Comment on lines +5 to +7
security:
# Required when using bitnami legacy images
allowInsecureImages: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Clarify security implications of allowInsecureImages.

The global.security.allowInsecureImages: true setting is introduced with a comment stating it's "Required when using bitnami legacy images," but the security implications are not explained. This flag suggests a security trade-off.

Please clarify:

  1. Why are Bitnami legacy images considered "insecure"?
  2. What specific security controls are being bypassed?
  3. What risks does this introduce for production deployments?
  4. Is there documentation or a Bitnami advisory explaining the legacy image situation?

Consider adding a more detailed warning comment in the values file if this is a known security concern.


🌐 Web query:

What are Bitnami legacy Docker images and why would they require allowInsecureImages flag in Kubernetes?

💡 Result:

Short answer

  • Bitnami "legacy" Docker images are the archived/versioned Bitnami images moved out of the public/main catalog into a legacy repository (e.g., docker.io/bitnamilegacy). They are frozen — no updates, security patches, or support — and intended only as a temporary fallback during migrations. [1]

  • Some Kubernetes operators/products expose an allowInsecureImages (or similarly named) security option that you must enable when switching to Bitnami Legacy. That flag tells the operator to accept/use images from the legacy/archived location (or from registries/images that are not in the operator’s approved/signed catalog). Without it, the operator blocks or refuses to pull/run those images, which would cause pods to fail to start (ImagePullBackOff) after the Bitnami catalog change. [2][3]

Why this matters (quick bullets)

  • If your manifests/Helm charts still reference docker.io/bitnami/* after Bitnami moved most versioned tags to the legacy repo, image pulls will fail unless you update references or permit the legacy source. [1][3]
  • Products that provide a safety switch (allowInsecureImages) require you to explicitly opt in to using the archived/unsupported legacy images (or unsigned/untrusted registries) to avoid silently running unmaintained images. [2]

References

  • Bitnami GitHub / FAQ about the Legacy repository and what “legacy” means. [1]
  • Example vendor guidance showing the allowInsecureImages patch/workaround to let their Kubernetes operator use Bitnami Legacy images (Digital.ai operator docs). [2]
  • Kubermatic blog explaining impact of Bitnami catalog changes on clusters and why image references must change. [3]

If you want, I can: 1) scan your cluster/manifests for bitnami image references and produce an inventory, or 2) show the exact kubectl patch to set allowInsecureImages for a specific operator. Which would you prefer?


Clarify security trade-offs of allowInsecureImages
Add a detailed warning above security.allowInsecureImages: true explaining that it permits pulling unmaintained Bitnami legacy images from the archived registry (docker.io/bitnamilegacy), bypasses the operator’s trusted/signed image catalog, and exposes unpatched, unsupported images to production. Link to the official Bitnami Legacy FAQ (https://github.com/bitnami/bitnami-docker#legacy-repository) and suggest migrating to maintained images where possible.

Comment on lines +1738 to +1767

if (!taskRun) {
this.logger.error(
"RunEngine.handleRepairSnapshot SUSPENDED/FINISHED task run not found",
{
runId,
snapshotId,
}
);
return;
}

// We need to clear this run from the current concurrency sets
await this.runQueue.clearMessageFromConcurrencySets({
runId,
orgId: latestSnapshot.organizationId,
queue: taskRun.queue,
env: {
id: latestSnapshot.environmentId,
type: latestSnapshot.environmentType,
project: {
id: latestSnapshot.projectId,
},
organization: {
id: latestSnapshot.organizationId,
},
},
});

break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Include the concurrency key when clearing Redis sets.

clearMessageFromConcurrencySets builds the key using queueCurrentConcurrencyKey(env, queue, concurrencyKey?). We fetch only queue, so the generated key is <…:queue:{queue}>. For runs dispatched with a non-null concurrencyKey, the actual key in Redis is <…:queue:{queue}:concurrency:{key}>, so the SREM misses the entry and the stuck run remains. Please hydrate concurrencyKey in the Prisma query and pass it through to clearMessageFromConcurrencySets (and teach that helper to forward the concurrency key). Without this, repairing queues with concurrency-enabled runs is a no-op.

-      const taskRun = await this.prisma.taskRun.findFirst({
-        where: { id: runId },
-        select: {
-          queue: true,
-        },
-      });
+      const taskRun = await this.prisma.taskRun.findFirst({
+        where: { id: runId },
+        select: {
+          queue: true,
+          concurrencyKey: true,
+        },
+      });-      await this.runQueue.clearMessageFromConcurrencySets({
-        runId,
-        orgId: latestSnapshot.organizationId,
-        queue: taskRun.queue,
-        env: {
-          id: latestSnapshot.environmentId,
-          type: latestSnapshot.environmentType,
-          project: {
-            id: latestSnapshot.projectId,
-          },
-          organization: {
-            id: latestSnapshot.organizationId,
-          },
-        },
-      });
+      await this.runQueue.clearMessageFromConcurrencySets({
+        runId,
+        orgId: latestSnapshot.organizationId,
+        queue: taskRun.queue,
+        concurrencyKey: taskRun.concurrencyKey ?? undefined,
+        env: {
+          id: latestSnapshot.environmentId,
+          type: latestSnapshot.environmentType,
+          project: {
+            id: latestSnapshot.projectId,
+          },
+          organization: {
+            id: latestSnapshot.organizationId,
+          },
+        },
+      });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!taskRun) {
this.logger.error(
"RunEngine.handleRepairSnapshot SUSPENDED/FINISHED task run not found",
{
runId,
snapshotId,
}
);
return;
}
// We need to clear this run from the current concurrency sets
await this.runQueue.clearMessageFromConcurrencySets({
runId,
orgId: latestSnapshot.organizationId,
queue: taskRun.queue,
env: {
id: latestSnapshot.environmentId,
type: latestSnapshot.environmentType,
project: {
id: latestSnapshot.projectId,
},
organization: {
id: latestSnapshot.organizationId,
},
},
});
break;
const taskRun = await this.prisma.taskRun.findFirst({
where: { id: runId },
select: {
queue: true,
concurrencyKey: true,
},
});
if (!taskRun) {
this.logger.error(
"RunEngine.handleRepairSnapshot SUSPENDED/FINISHED task run not found",
{
runId,
snapshotId,
}
);
return;
}
// We need to clear this run from the current concurrency sets
await this.runQueue.clearMessageFromConcurrencySets({
runId,
orgId: latestSnapshot.organizationId,
queue: taskRun.queue,
concurrencyKey: taskRun.concurrencyKey ?? undefined,
env: {
id: latestSnapshot.environmentId,
type: latestSnapshot.environmentType,
project: {
id: latestSnapshot.projectId,
},
organization: {
id: latestSnapshot.organizationId,
},
},
});
break;
🤖 Prompt for AI Agents
In internal-packages/run-engine/src/engine/index.ts around lines 1738 to 1767,
the call to clearMessageFromConcurrencySets only passes queue and therefore
builds a Redis key without the concurrency suffix; hydrate the task run query to
include concurrencyKey (fetch it from taskRun), then pass that concurrencyKey
into clearMessageFromConcurrencySets; also update the
clearMessageFromConcurrencySets helper signature/implementation to accept an
optional concurrencyKey and include it when composing/forwarding the
queueCurrentConcurrencyKey so the SREM targets the correct Redis key
(…:queue:{queue}:concurrency:{key}).

});

return snapshots.map(enhanceExecutionSnapshot);
return snapshots.reverse().map(enhanceExecutionSnapshot);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Reversing after DESC order skips intermediate snapshots.

The reverse() call compensates for the descending order, returning snapshots in ascending chronological order. However, when combined with take: 50, this approach skips intermediate snapshots if more than 50 exist after sinceSnapshotId.

If the repair mechanism only needs a bounded recent batch, consider renaming the function to reflect this (e.g., getRecentExecutionSnapshotsAfter) and document the skip behavior. Alternatively, add pagination metadata (e.g., hasMore, nextCursor) so callers can detect truncated results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants