Concurrency self serve #2681
Concurrency self serve #2681
Conversation
|
WalkthroughAdds an end-to-end concurrency management feature: UI components (ConcurrencyIcon, InputNumberStepper), SideMenu entry and route for environment concurrency, a ManageConcurrencyPresenter, services for allocating concurrency and setting concurrency add‑ons, platform helpers for per‑environment defaults and billing, route loader/action for concurrency page, DB migration and model field (maximumProjectCount), path helper Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts (1)
103-114: Good addition of error handling! Consider differentiating status codes.The introduction of
tryCatchis a positive change that prevents unhandled promise rejections. However, all errors currently return a 400 status code, which may not be semantically appropriate for all error types.Consider differentiating status codes based on error type for better API semantics:
const [error, project] = await tryCatch( createProject({ organizationSlug: organization.slug, name: parsedBody.data.name, userId: authenticationResult.userId, version: "v3", }) ); if (error) { - return json({ error: error.message }, { status: 400 }); + // Determine appropriate status code based on error type + let status = 400; + if (error.message.includes("permission")) { + status = 403; + } else if (error.message.includes("maximum number of projects")) { + status = 429; // or 402 for payment required + } else if (error.message.includes("Unable to create project")) { + status = 500; + } + return json({ error: error.message }, { status }); }Alternatively, you could check error types if
createProjectthrows custom error classes (e.g.,ExceededProjectLimitError).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/webapp/app/assets/icons/ConcurrencyIcon.tsx(1 hunks)apps/webapp/app/components/Feedback.tsx(3 hunks)apps/webapp/app/components/navigation/SideMenu.tsx(4 hunks)apps/webapp/app/components/primitives/Input.tsx(1 hunks)apps/webapp/app/components/primitives/InputNumberStepper.tsx(1 hunks)apps/webapp/app/components/primitives/Toast.tsx(5 hunks)apps/webapp/app/models/message.server.ts(3 hunks)apps/webapp/app/models/organization.server.ts(2 hunks)apps/webapp/app/models/project.server.ts(2 hunks)apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx(4 hunks)apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.ts(1 hunks)apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts(2 hunks)apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency.ts(0 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx(1 hunks)apps/webapp/app/routes/storybook.input-fields/route.tsx(1 hunks)apps/webapp/app/routes/storybook.stepper/route.tsx(1 hunks)apps/webapp/app/routes/storybook/route.tsx(1 hunks)apps/webapp/app/services/platform.v3.server.ts(4 hunks)apps/webapp/app/utils/environmentSort.ts(1 hunks)apps/webapp/app/utils/pathBuilder.ts(1 hunks)apps/webapp/app/v3/services/allocateConcurrency.server.ts(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(1 hunks)apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(1 hunks)apps/webapp/package.json(1 hunks)internal-packages/database/prisma/migrations/20251113152235_maximum_project_count/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-08T11:48:12.327Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
Applied to files:
apps/webapp/package.json
📚 Learning: 2025-09-02T11:37:42.902Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2461
File: packages/core/src/v3/runEngineWorker/supervisor/consumerPool.ts:315-317
Timestamp: 2025-09-02T11:37:42.902Z
Learning: In packages/core/src/v3/runEngineWorker/supervisor/scalingStrategies.ts, the ScalingStrategy base class already handles clamping to min/max bounds in the public calculateTargetCount method, and the individual strategy implementations handle rounding internally using Math.round, Math.floor, and Math.ceil as appropriate.
Applied to files:
apps/webapp/app/v3/services/createBackgroundWorker.server.ts
🧬 Code graph analysis (17)
apps/webapp/app/routes/storybook.stepper/route.tsx (2)
apps/webapp/app/components/primitives/Headers.tsx (2)
Header2(52-70)Header3(72-90)apps/webapp/app/components/primitives/InputNumberStepper.tsx (1)
InputNumberStepper(13-220)
apps/webapp/app/components/primitives/InputNumberStepper.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts (4)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (1)
ManageConcurrencyPresenter(33-132)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx (1)
action(133-211)apps/webapp/app/services/platform.v3.server.ts (1)
setConcurrencyAddOn(402-416)apps/webapp/app/utils/plain.server.ts (1)
sendToPlain(12-59)
apps/webapp/app/components/primitives/Toast.tsx (4)
apps/webapp/app/models/message.server.ts (1)
ToastMessageAction(13-25)apps/webapp/app/components/primitives/Headers.tsx (1)
Header2(52-70)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)apps/webapp/app/components/primitives/Buttons.tsx (2)
LinkButton(335-401)Button(296-329)
apps/webapp/app/models/project.server.ts (1)
apps/webapp/app/db.server.ts (1)
prisma(101-101)
apps/webapp/app/components/Feedback.tsx (2)
apps/webapp/app/hooks/useSearchParam.ts (1)
useSearchParams(7-64)apps/webapp/app/routes/resources.feedback.ts (1)
FeedbackType(21-21)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (2)
apps/webapp/app/services/platform.v3.server.ts (3)
getCurrentPlan(193-228)getDefaultEnvironmentLimitFromPlan(284-302)getPlans(323-337)apps/webapp/app/utils/environmentSort.ts (1)
sortEnvironments(15-35)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx (10)
apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)apps/webapp/app/utils/pathBuilder.ts (3)
EnvironmentParamSchema(26-28)concurrencyPath(466-472)v3BillingPath(482-486)apps/webapp/app/models/project.server.ts (1)
findProjectBySlug(136-147)apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (3)
ManageConcurrencyPresenter(33-132)ConcurrencyResult(10-21)EnvironmentWithConcurrency(23-31)apps/webapp/app/services/platform.v3.server.ts (1)
getPlans(323-337)apps/webapp/app/models/message.server.ts (2)
redirectWithErrorMessage(201-218)redirectWithSuccessMessage(182-199)apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
AllocateConcurrencyService(22-91)apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts (1)
SetConcurrencyAddOnService(26-143)apps/webapp/app/routes/_app.orgs.$organizationSlug/route.tsx (1)
useCurrentPlan(22-29)apps/webapp/app/hooks/useOrganizations.ts (1)
useOrganization(39-43)
apps/webapp/app/models/organization.server.ts (1)
apps/webapp/app/services/platform.v3.server.ts (1)
getDefaultEnvironmentConcurrencyLimit(258-282)
apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (1)
ManageConcurrencyPresenter(33-132)
apps/webapp/app/models/message.server.ts (2)
apps/webapp/app/components/primitives/Buttons.tsx (1)
ButtonVariant(166-166)apps/webapp/app/routes/resources.feedback.ts (1)
FeedbackType(21-21)
apps/webapp/app/components/navigation/SideMenu.tsx (4)
apps/webapp/app/hooks/useFeatures.ts (1)
useFeatures(5-9)apps/webapp/app/components/navigation/SideMenuItem.tsx (1)
SideMenuItem(7-53)apps/webapp/app/assets/icons/ConcurrencyIcon.tsx (1)
ConcurrencyIcon(1-13)apps/webapp/app/utils/pathBuilder.ts (1)
concurrencyPath(466-472)
apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts (1)
apps/webapp/app/models/project.server.ts (1)
createProject(26-134)
apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (1)
apps/webapp/app/services/platform.v3.server.ts (1)
setPlan(339-400)
apps/webapp/app/services/platform.v3.server.ts (4)
apps/webapp/app/database-types.ts (1)
RuntimeEnvironmentType(49-54)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/models/message.server.ts (2)
redirectWithErrorMessage(201-218)redirectWithSuccessMessage(182-199)apps/webapp/app/utils/pathBuilder.ts (1)
newProjectPath(129-133)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx (3)
apps/webapp/app/components/primitives/Buttons.tsx (1)
LinkButton(335-401)apps/webapp/app/utils/pathBuilder.ts (1)
concurrencyPath(466-472)apps/webapp/app/assets/icons/ConcurrencyIcon.tsx (1)
ConcurrencyIcon(1-13)
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (4)
apps/webapp/app/models/project.server.ts (1)
ExceededProjectLimitError(19-24)apps/webapp/app/models/message.server.ts (1)
redirectWithErrorMessage(201-218)apps/webapp/app/utils/pathBuilder.ts (1)
newProjectPath(129-133)apps/webapp/app/components/Feedback.tsx (1)
Feedback(29-177)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts (1)
7-7: LGTM! Clean addition of error handling utility.The import of
tryCatchenables graceful error handling for thecreateProjectcall below.apps/webapp/app/components/Feedback.tsx (2)
70-73: LGTM!The
handleOpenChangehandler correctly delegates to both the local state update and the optional callback using safe optional chaining.
26-26: LGTM!The
onOpenChangeprop addition is well-implemented:
- Properly typed as an optional callback
- Cleanly integrated without breaking existing usage
- Enables parent components to respond to dialog state changes
Also applies to: 29-29, 76-76
apps/webapp/package.json (1)
117-117: Verify timeline and rationale for beta dependency with team.The @trigger.dev/platform version 1.0.20-beta.2 is not found in public release information; the latest publicly available stable version is 1.0.15. While the beta is locked in your pnpm-lock.yaml (indicating intentional selection), there is no documentation in the codebase explaining why a pre-release was chosen over the stable version or when this will be upgraded to a stable release.
Production dependencies on pre-release versions introduce uncertainty around stability and breaking changes. Please verify with your team:
- The rationale for selecting this beta version
- Expected timeline for upgrading to a stable 1.0.20 release
- Any known limitations or stability considerations with 1.0.20-beta.2
apps/webapp/app/routes/storybook.stepper/route.tsx (1)
21-66: Demo masked rounding regressionBecause the stepper’s
handleStep*isn’t emitting changes, these storybook examples don’t update their state when you click the buttons. Fixing the component resolves the demo, but make sure to rerun and confirm after the stepper fix lands.internal-packages/database/prisma/schema.prisma (1)
207-207: Schema default must match migration
maximumProjectCount Int @default(10)matches the migration’s default, so no issue.internal-packages/database/prisma/migrations/20251113152235_maximum_project_count/migration.sql (1)
1-3: LGTM! Clean migration adding project limit.The migration adds a sensible organization-level project limit with a default of 10. The NOT NULL constraint with DEFAULT ensures existing organizations are handled correctly.
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.ts (1)
77-77: PREVIEW environments now share staging limit allocation.This change treats PREVIEW environments the same as STAGING for concurrency limit allocation. Verify this is the intended behavior for your concurrency model.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx (1)
416-423: LGTM! Clean navigation to dedicated concurrency management.The replacement of the Feedback component with a LinkButton to the new concurrency page is a good UX improvement. The amber styling and ConcurrencyIcon clearly communicate the purpose.
apps/webapp/app/models/organization.server.ts (1)
99-108: Excellent refactoring to centralized limit calculation.Replacing the hardcoded
organization.maximumConcurrencyLimit / 3withgetDefaultEnvironmentConcurrencyLimitprovides better flexibility. This function handles both plan-based limits (managed cloud) and organization-based limits (self-hosted), making the environment creation logic more maintainable.apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
362-365: Correct alignment with new concurrency allocation model.Removing
organization.maximumConcurrencyLimitfrom the clamping operation is appropriate. Since environment limits are now derived from organization limits (viagetDefaultEnvironmentConcurrencyLimitduring creation), the environment'smaximumConcurrencyLimitis the authoritative cap. This change aligns with the allocation-based concurrency model introduced in this PR.apps/webapp/app/utils/pathBuilder.ts (1)
466-472: LGTM! Clean path builder following established patterns.The new
concurrencyPathfunction is consistent with other path builders in the file and properly constructs the concurrency management route.apps/webapp/app/components/navigation/SideMenu.tsx (1)
319-327: LGTM! Properly gated feature integration.The Concurrency menu item is correctly gated behind the
isManagedCloudfeature flag and follows the established pattern for SideMenu items. The placement in the Manage section is appropriate.apps/webapp/app/utils/environmentSort.ts (1)
15-22: Excellent flexibility enhancement with backward compatibility.Adding the optional
sortOrderparameter makes the function more reusable while preserving the default behavior. The fallback toenvironmentSortOrderensures no breaking changes for existing callers.apps/webapp/app/components/primitives/Toast.tsx (1)
25-39:ephemeralnow defaults to permanent toasts.
Across the codebaseoptions.ephemeralis optional. With this ternary,undefinedbecomes falsy, so any toast without an explicit flag (the common case) now sticks around for the full 24‑hour “permanent” duration. Can you double-check that this regression is intentional? If not, we should keep the old behaviour by defaulting totruewhen no action is present.- const ephemeral = options.action ? false : options.ephemeral; + const ephemeral = options.action ? false : options.ephemeral ?? true;
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx
Show resolved
Hide resolved
...outes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx
Show resolved
Hide resolved
| const previousExtra = result.environments.reduce( | ||
| (acc, e) => Math.max(0, e.maximumConcurrencyLimit - e.planConcurrencyLimit) + acc, | ||
| 0 | ||
| ); | ||
| const newExtra = environments.reduce((acc, e) => e.amount + acc, 0); | ||
| const change = newExtra - previousExtra; | ||
|
|
||
| const totalExtra = result.extraAllocatedConcurrency + change; | ||
|
|
||
| if (change > result.extraUnallocatedConcurrency) { | ||
| return { | ||
| success: false, | ||
| error: `You don't have enough unallocated concurrency available. You requested ${totalExtra} but only have ${result.extraUnallocatedConcurrency}.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Client can bypass the extra concurrency cap by omitting environments.
newExtra only sums the environments supplied in the payload. A crafted request can leave out existing environments that already have extra concurrency, making change appear small (or even negative) and slipping past the extraUnallocatedConcurrency guard. The untouched environments keep their previous extra allocation, so the org ends up consuming more than it purchased. Please derive the post-update total from the authoritative list (result.environments), filling in the submitted overrides where present and carrying forward the existing extras for everything else, e.g.:
- const newExtra = environments.reduce((acc, e) => e.amount + acc, 0);
- const change = newExtra - previousExtra;
+ const requested = new Map(environments.map((e) => [e.id, e.amount]));
+ const newExtra = result.environments.reduce((acc, env) => {
+ const targetExtra =
+ requested.has(env.id)
+ ? Math.max(0, requested.get(env.id)!)
+ : Math.max(0, env.maximumConcurrencyLimit - env.planConcurrencyLimit);
+ return acc + targetExtra;
+ }, 0);
+ const change = newExtra - previousExtra;This keeps the guard honest even if the client payload is partial.
📝 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.
| const previousExtra = result.environments.reduce( | |
| (acc, e) => Math.max(0, e.maximumConcurrencyLimit - e.planConcurrencyLimit) + acc, | |
| 0 | |
| ); | |
| const newExtra = environments.reduce((acc, e) => e.amount + acc, 0); | |
| const change = newExtra - previousExtra; | |
| const totalExtra = result.extraAllocatedConcurrency + change; | |
| if (change > result.extraUnallocatedConcurrency) { | |
| return { | |
| success: false, | |
| error: `You don't have enough unallocated concurrency available. You requested ${totalExtra} but only have ${result.extraUnallocatedConcurrency}.`, | |
| }; | |
| } | |
| const previousExtra = result.environments.reduce( | |
| (acc, e) => Math.max(0, e.maximumConcurrencyLimit - e.planConcurrencyLimit) + acc, | |
| 0 | |
| ); | |
| const requested = new Map(environments.map((e) => [e.id, e.amount])); | |
| const newExtra = result.environments.reduce((acc, env) => { | |
| const targetExtra = | |
| requested.has(env.id) | |
| ? Math.max(0, requested.get(env.id)!) | |
| : Math.max(0, env.maximumConcurrencyLimit - env.planConcurrencyLimit); | |
| return acc + targetExtra; | |
| }, 0); | |
| const change = newExtra - previousExtra; | |
| const totalExtra = result.extraAllocatedConcurrency + change; | |
| if (change > result.extraUnallocatedConcurrency) { | |
| return { | |
| success: false, | |
| error: `You don't have enough unallocated concurrency available. You requested ${totalExtra} but only have ${result.extraUnallocatedConcurrency}.`, | |
| }; | |
| } |
matt-aitken
force-pushed
the
concurrency-self-serve
branch
from
d948d24 to
36f6e48
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/models/message.server.ts (1)
57-70: Keep toasts with actions onscreen.The new
actionadds a button the user needs to click, but we still defaultephemeraltotrue, so the toast disappears before they can interact (e.g., “Request more projects” in the new-project flow). Please defaultephemeraltofalsewhenever an action is present, unless the caller overrides it explicitly.export function setSuccessMessage( session: Session, message: string, options?: ToastMessageOptions ) { + const hasAction = Boolean(options?.action); session.flash("toastMessage", { message, type: "success", options: { ...options, - ephemeral: options?.ephemeral ?? true, + ephemeral: options?.ephemeral ?? (hasAction ? false : true), }, } as ToastMessage); } export function setErrorMessage(session: Session, message: string, options?: ToastMessageOptions) { + const hasAction = Boolean(options?.action); session.flash("toastMessage", { message, type: "error", options: { ...options, - ephemeral: options?.ephemeral ?? true, + ephemeral: options?.ephemeral ?? (hasAction ? false : true), }, } as ToastMessage); }Without this, the new CTA can’t realistically be used.
♻️ Duplicate comments (8)
apps/webapp/app/models/project.server.ts (1)
57-68: Project cap check still races under load.The count and the insert happen outside a transaction, so two concurrent
createProjectcalls can both observeprojectCount < maximumProjectCountand create rows, leaving the org over its limit. Please wrap the check+create in a serializable transaction (or useSELECT … FOR UPDATE) so only one request can pass the guard at a time.- const projectCount = await prisma.project.count({ - where: { - organizationId: organization.id, - deletedAt: null, - }, - }); - - if (projectCount >= organization.maximumProjectCount) { - throw new ExceededProjectLimitError( - `This organization has reached the maximum number of projects (${organization.maximumProjectCount}).` - ); - } - - const project = await prisma.project.create({ + const project = await prisma.$transaction( + async (tx) => { + const projectCount = await tx.project.count({ + where: { + organizationId: organization.id, + deletedAt: null, + }, + }); + + if (projectCount >= organization.maximumProjectCount) { + throw new ExceededProjectLimitError( + `This organization has reached the maximum number of projects (${organization.maximumProjectCount}).` + ); + } + + return tx.project.create({ data: { name, slug: uniqueProjectSlug, @@ - include: { - organization: { - include: { - members: true, - }, - }, - }, - }); + include: { + organization: { + include: { + members: true, + }, + }, + }, + }); + }, + { isolationLevel: "Serializable" } + );This ensures the limit can’t be exceeded even under bursty traffic.
apps/webapp/app/components/Feedback.tsx (1)
57-68: Validate thefeedbackPanelparam and avoid shadowing state.The
const open = …shadow still collides with theopenstate, and we’re coercing whatever string is in the query param intoFeedbackTypewithout validation. A crafted URL (e.g.?feedbackPanel=foo) will push an invalid value into state, leaving the select without a matching option. Please keep the state name distinct and ensure the param is one of the known keys before using it; you can also lean on ouruseSearchParamshelper to handle deletion without manual cloning, as mentioned previously.- useEffect(() => { - const open = searchParams.get("feedbackPanel"); - if (open) { - setType(open as FeedbackType); - setOpen(true); - // Clone instead of mutating in place - const next = new URLSearchParams(searchParams); - next.delete("feedbackPanel"); - setSearchParams(next); - } - }, [searchParams]); + useEffect(() => { + const feedbackPanelParam = searchParams.get("feedbackPanel"); + if (feedbackPanelParam && feedbackPanelParam in feedbackTypeLabel) { + setType(feedbackPanelParam as FeedbackType); + setOpen(true); + const next = new URLSearchParams(searchParams); + next.delete("feedbackPanel"); + setSearchParams(next); + } + }, [searchParams, setSearchParams]);This keeps state consistent and prevents invalid types from slipping through.
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (1)
217-218: Provide a real trigger element forFeedback.Passing an empty fragment into
DialogTrigger(withasChild) still triggers Radix’s runtime error (“asChild expects a single child that can accept a ref”), so the page breaks when the component mounts. Please render an actual focusable element—even a visually hidden<button>works—so the dialog opens safely.- <Feedback button={<></>} /> + <Feedback + button={<button type="button" className="sr-only" aria-hidden />} + />This already came up in the previous review; we still need to address it so the page doesn’t crash.
apps/webapp/app/services/platform.v3.server.ts (2)
275-281: Add plan fallback instead of throwing when per-environment limits are absent.This still throws “No plan found” whenever the billing plan omits
concurrentRunsfor a given environment (e.g. free tiers without staging). We previously flagged this; please fall back to the organization’smaximumConcurrencyLimit(same as the no-client branch) so environment creation doesn’t crash. For example:- const limit = getDefaultEnvironmentLimitFromPlan(environmentType, result); - if (!limit) throw new Error("No plan found"); + const limit = + getDefaultEnvironmentLimitFromPlan(environmentType, result) ?? + result.v3Subscription?.plan?.limits.concurrentRuns.number ?? + org.maximumConcurrencyLimit;Make sure
org.maximumConcurrencyLimitis retrieved once so you can reuse it in both branches.
402-415: Surface concurrency add-on failures to callers.Returning
undefinedcovers “no client” and real platform errors, so callers can’t distinguish success from failure—exactly the issue we already discussed. Please throw or return a discriminated result that includes the platform error so the UI can react appropriately.-export async function setConcurrencyAddOn(organizationId: string, amount: number) { - if (!client) return undefined; +export async function setConcurrencyAddOn( + organizationId: string, + amount: number +): Promise<{ success: true } | { success: false; reason: "no_client" | "platform_error"; error?: string }> { + if (!client) { + return { success: false, reason: "no_client" }; + } @@ - if (!result.success) { - logger.error("Error setting concurrency add on - no success", { error: result.error }); - return undefined; - } - return result; + if (!result.success) { + logger.error("Error setting concurrency add on - no success", { error: result.error }); + return { success: false, reason: "platform_error", error: result.error }; + } + return { success: true }; } catch (e) { logger.error("Error setting concurrency add on - caught error", { error: e }); - return undefined; + return { + success: false, + reason: "platform_error", + error: e instanceof Error ? e.message : String(e), + }; } }Update callers accordingly so they can handle the failure cases.
apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
41-86: Recompute total extra concurrency from all environments, not just the payload.
newExtraonly sums the environments included in the request, so omitting an environment with existing extra capacity makeschangenegative and bypasses the guard while its previous allocation persists. This is the same bypass we warned about earlier. Derive the post-update total from the authoritative list, merging submitted overrides with existing data:- const newExtra = environments.reduce((acc, e) => e.amount + acc, 0); - const change = newExtra - previousExtra; + const requested = new Map(environments.map((e) => [e.id, Math.max(0, e.amount)])); + const newExtra = result.environments.reduce((acc, env) => { + const targetExtra = requested.has(env.id) + ? requested.get(env.id)! + : Math.max(0, env.maximumConcurrencyLimit - env.planConcurrencyLimit); + return acc + targetExtra; + }, 0); + const change = newExtra - previousExtra;This keeps the guard honest even when the payload is partial.
apps/webapp/app/components/primitives/InputNumberStepper.tsx (2)
31-63: Stepper clicks never reach consumers
CallingdispatchEvent(new Event("change"))is swallowed by React’s synthetic event system, so parents never see updates when the +/- buttons are used. This breaks every controlled usage of the component. Emit an"input"event and mark it as simulated (or callonChangedirectly) so React processes it.- const event = new Event("change", { bubbles: true }); + const event = new Event("input", { bubbles: true }); + // @ts-expect-error React inspects this flag to avoid dedupe + event.simulated = true; inputRef.current.dispatchEvent(event);
138-181: Preserve rounding logic when parents add handlers
Spreading...propsafter definingonBlur/onKeyDownoverwrites your rounding handlers anytime a consumer passes its own callbacks. Extract those handlers first, then invoke them inside your logic. Otherwise rounding is silently disabled in common usage.- ...props - onBlur={(e) => { - if (round) commitRoundedFromInput(); - }} - onKeyDown={(e) => { + const { onBlur: propsOnBlur, onKeyDown: propsOnKeyDown, ...rest } = props; + ... + onBlur={(e) => { + if (round) commitRoundedFromInput(); + propsOnBlur?.(e); + }} + onKeyDown={(e) => { if (e.key === "Enter" && round) { e.preventDefault(); commitRoundedFromInput(); } + propsOnKeyDown?.(e); }} + {...rest}
🧹 Nitpick comments (1)
apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts (1)
48-83: Remove unnecessary optional chaining.After verifying
updatedConcurrencyis not null/undefined at Line 51, the optional chaining at Line 58 (updatedConcurrency?.result) is unnecessary and creates inconsistent code.Apply this diff:
const updatedConcurrency = await setConcurrencyAddOn(organizationId, totalExtraConcurrency); if (!updatedConcurrency) { return { success: false, error: "Failed to update concurrency", }; } - switch (updatedConcurrency?.result) { + switch (updatedConcurrency.result) { case "success": {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/webapp/app/assets/icons/ConcurrencyIcon.tsx(1 hunks)apps/webapp/app/components/Feedback.tsx(3 hunks)apps/webapp/app/components/navigation/SideMenu.tsx(4 hunks)apps/webapp/app/components/primitives/Input.tsx(1 hunks)apps/webapp/app/components/primitives/InputNumberStepper.tsx(1 hunks)apps/webapp/app/components/primitives/Toast.tsx(5 hunks)apps/webapp/app/models/message.server.ts(3 hunks)apps/webapp/app/models/organization.server.ts(2 hunks)apps/webapp/app/models/project.server.ts(2 hunks)apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx(4 hunks)apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.ts(1 hunks)apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts(2 hunks)apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency.ts(0 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx(1 hunks)apps/webapp/app/routes/storybook.input-fields/route.tsx(1 hunks)apps/webapp/app/routes/storybook.stepper/route.tsx(1 hunks)apps/webapp/app/routes/storybook/route.tsx(1 hunks)apps/webapp/app/services/platform.v3.server.ts(4 hunks)apps/webapp/app/utils/environmentSort.ts(1 hunks)apps/webapp/app/utils/pathBuilder.ts(1 hunks)apps/webapp/app/v3/services/allocateConcurrency.server.ts(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(1 hunks)apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(1 hunks)apps/webapp/package.json(1 hunks)internal-packages/database/prisma/migrations/20251113152235_maximum_project_count/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/webapp/app/routes/storybook/route.tsx
- apps/webapp/app/components/primitives/Input.tsx
- internal-packages/database/prisma/schema.prisma
- apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.ts
- apps/webapp/app/assets/icons/ConcurrencyIcon.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx
- apps/webapp/package.json
- apps/webapp/app/routes/storybook.input-fields/route.tsx
- apps/webapp/app/utils/pathBuilder.ts
- internal-packages/database/prisma/migrations/20251113152235_maximum_project_count/migration.sql
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx
- apps/webapp/app/v3/services/triggerTaskV1.server.ts
- apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx
- apps/webapp/app/components/navigation/SideMenu.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:37:42.902Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2461
File: packages/core/src/v3/runEngineWorker/supervisor/consumerPool.ts:315-317
Timestamp: 2025-09-02T11:37:42.902Z
Learning: In packages/core/src/v3/runEngineWorker/supervisor/scalingStrategies.ts, the ScalingStrategy base class already handles clamping to min/max bounds in the public calculateTargetCount method, and the individual strategy implementations handle rounding internally using Math.round, Math.floor, and Math.ceil as appropriate.
Applied to files:
apps/webapp/app/v3/services/createBackgroundWorker.server.ts
🧬 Code graph analysis (13)
apps/webapp/app/models/project.server.ts (1)
apps/webapp/app/db.server.ts (1)
prisma(101-101)
apps/webapp/app/models/message.server.ts (2)
apps/webapp/app/components/primitives/Buttons.tsx (1)
ButtonVariant(166-166)apps/webapp/app/routes/resources.feedback.ts (1)
FeedbackType(21-21)
apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts (1)
apps/webapp/app/models/project.server.ts (1)
createProject(26-134)
apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (1)
ManageConcurrencyPresenter(33-132)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (2)
apps/webapp/app/services/platform.v3.server.ts (3)
getCurrentPlan(193-228)getDefaultEnvironmentLimitFromPlan(284-302)getPlans(323-337)apps/webapp/app/utils/environmentSort.ts (1)
sortEnvironments(15-35)
apps/webapp/app/services/platform.v3.server.ts (4)
apps/webapp/app/database-types.ts (1)
RuntimeEnvironmentType(49-54)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/models/message.server.ts (2)
redirectWithErrorMessage(201-218)redirectWithSuccessMessage(182-199)apps/webapp/app/utils/pathBuilder.ts (1)
newProjectPath(129-133)
apps/webapp/app/models/organization.server.ts (1)
apps/webapp/app/services/platform.v3.server.ts (1)
getDefaultEnvironmentConcurrencyLimit(258-282)
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (4)
apps/webapp/app/models/project.server.ts (1)
ExceededProjectLimitError(19-24)apps/webapp/app/models/message.server.ts (1)
redirectWithErrorMessage(201-218)apps/webapp/app/utils/pathBuilder.ts (1)
newProjectPath(129-133)apps/webapp/app/components/Feedback.tsx (1)
Feedback(29-177)
apps/webapp/app/components/Feedback.tsx (2)
apps/webapp/app/hooks/useSearchParam.ts (1)
useSearchParams(7-64)apps/webapp/app/routes/resources.feedback.ts (1)
FeedbackType(21-21)
apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts (4)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (1)
ManageConcurrencyPresenter(33-132)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx (1)
action(133-211)apps/webapp/app/services/platform.v3.server.ts (1)
setConcurrencyAddOn(402-416)apps/webapp/app/utils/plain.server.ts (1)
sendToPlain(12-59)
apps/webapp/app/components/primitives/InputNumberStepper.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
apps/webapp/app/components/primitives/Toast.tsx (4)
apps/webapp/app/models/message.server.ts (1)
ToastMessageAction(13-25)apps/webapp/app/components/primitives/Headers.tsx (1)
Header2(52-70)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)apps/webapp/app/components/primitives/Buttons.tsx (2)
LinkButton(335-401)Button(296-329)
apps/webapp/app/routes/storybook.stepper/route.tsx (2)
apps/webapp/app/components/primitives/Headers.tsx (2)
Header2(52-70)Header3(72-90)apps/webapp/app/components/primitives/InputNumberStepper.tsx (1)
InputNumberStepper(13-220)
⏰ 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). (17)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
🔇 Additional comments (5)
apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts (2)
1-43: LGTM! Clean error handling for presenter call.The error handling for the presenter call is appropriate, and the use of
tryCatchensures any thrown errors are caught and converted to a result type.
103-136: Verify email failure handling.Based on the
sendToPlainimplementation (apps/webapp/app/utils/plain.server.ts lines 11-58), it logs errors toconsole.errorand returnsundefinedrather than throwing exceptions. ThetryCatchwrapper will only catch thrown errors, not cases wheresendToPlaincompletes but fails to send the email (e.g., whenPLAIN_API_KEYis missing or API calls fail).This means the service could return
{ success: true }even when the email was never sent.Consider one of these approaches:
- Modify
sendToPlainto throw errors instead of logging and returning undefined- Have
sendToPlainreturn a result object and check it here- Accept that email failures are non-blocking and log them separately
Which behavior is intended for quota-increase requests when email delivery fails?
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (3)
1-50: LGTM! Solid plan retrieval and access control setup.The plan retrieval with appropriate error handling and the
canAddConcurrencycheck based on plan limits are well-structured.
74-108: LGTM! Environment filtering logic is sound.The logic correctly:
- Skips parent (branchable) environments
- Only counts non-DEVELOPMENT environment overages toward allocated concurrency (as documented)
- Filters DEVELOPMENT environments by user ownership
- Safely handles null
orgMemberwith optional chainingThe
if (!limit)check at Line 84 appropriately skips environments with no concurrency limit, which is consistent with the concurrency management use case.
110-131: LGTM! Correct concurrency calculations and result assembly.The calculations properly:
- Cap
extraAllocatedatextraConcurrencyto prevent over-allocation- Compute
extraUnallocatedas the remaining purchased concurrency- Extract quota and pricing information from the plan
- Sort environments in a logical display order (PRODUCTION → STAGING → PREVIEW → DEVELOPMENT)
| const ephemeral = options.action ? false : options.ephemeral; | ||
|
|
||
| toast.custom( | ||
| (t) => ( | ||
| <ToastUI | ||
| variant={type} | ||
| message={message} | ||
| t={t as string} | ||
| title={options.title} | ||
| action={options.action} | ||
| /> | ||
| ), | ||
| { | ||
| duration: ephemeral ? defaultToastDuration : permanentToastDuration, | ||
| } |
There was a problem hiding this comment.
Restore default ephemeral behaviour when no action is present.
The new assignment treats an undefined options.ephemeral as falsy, so vanilla toasts now stick around for 24 h instead of dismissing after 5 s. Please preserve the previous default by falling back to true unless an action is attached.
- const ephemeral = options.action ? false : options.ephemeral;
+ const ephemeral = options.action ? false : options.ephemeral ?? true;📝 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.
| const ephemeral = options.action ? false : options.ephemeral; | |
| toast.custom( | |
| (t) => ( | |
| <ToastUI | |
| variant={type} | |
| message={message} | |
| t={t as string} | |
| title={options.title} | |
| action={options.action} | |
| /> | |
| ), | |
| { | |
| duration: ephemeral ? defaultToastDuration : permanentToastDuration, | |
| } | |
| const ephemeral = options.action ? false : options.ephemeral ?? true; | |
| toast.custom( | |
| (t) => ( | |
| <ToastUI | |
| variant={type} | |
| message={message} | |
| t={t as string} | |
| title={options.title} | |
| action={options.action} | |
| /> | |
| ), | |
| { | |
| duration: ephemeral ? defaultToastDuration : permanentToastDuration, | |
| } |
🤖 Prompt for AI Agents
In apps/webapp/app/components/primitives/Toast.tsx around lines 25 to 39, the
ephemeral flag is set with "const ephemeral = options.action ? false :
options.ephemeral;" which causes an undefined options.ephemeral to be treated as
falsy (making toasts permanent). Change the assignment to fall back to true when
options.ephemeral is undefined, e.g. use the nullish coalescing operator so it
reads: const ephemeral = options.action ? false : (options.ephemeral ?? true);
ensuring toasts without an action default to ephemeral.
| export function sortEnvironments<T extends SortType>( | ||
| environments: T[], | ||
| sortOrder?: RuntimeEnvironmentType[] | ||
| ): T[] { | ||
| const order = sortOrder ?? environmentSortOrder; | ||
| return environments.sort((a, b) => { | ||
| const aIndex = environmentSortOrder.indexOf(a.type); | ||
| const bIndex = environmentSortOrder.indexOf(b.type); | ||
| const aIndex = order.indexOf(a.type); | ||
| const bIndex = order.indexOf(b.type); | ||
|
|
There was a problem hiding this comment.
Handle unknown environments when a custom order is partial.
If a caller supplies a sortOrder that doesn’t list every environment type, order.indexOf() returns -1, so those environments float to the top instead of falling back to the default ordering. For example, passing ["PRODUCTION"] pushes PRODUCTION after every other environment, which defeats the purpose of the new override. Please fall back to environmentSortOrder (or a high sentinel) when sortOrder omits a type.
export function sortEnvironments<T extends SortType>(
environments: T[],
sortOrder?: RuntimeEnvironmentType[]
): T[] {
- const order = sortOrder ?? environmentSortOrder;
+ const order = sortOrder ?? environmentSortOrder;
+ const getIndex = (type: RuntimeEnvironmentType) => {
+ const overrideIndex = order.indexOf(type);
+ if (overrideIndex !== -1) return overrideIndex;
+ return environmentSortOrder.indexOf(type);
+ };
return environments.sort((a, b) => {
- const aIndex = order.indexOf(a.type);
- const bIndex = order.indexOf(b.type);
+ const aIndex = getIndex(a.type);
+ const bIndex = getIndex(b.type);This keeps the existing behaviour while letting partial overrides work as expected.
📝 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.
| export function sortEnvironments<T extends SortType>( | |
| environments: T[], | |
| sortOrder?: RuntimeEnvironmentType[] | |
| ): T[] { | |
| const order = sortOrder ?? environmentSortOrder; | |
| return environments.sort((a, b) => { | |
| const aIndex = environmentSortOrder.indexOf(a.type); | |
| const bIndex = environmentSortOrder.indexOf(b.type); | |
| const aIndex = order.indexOf(a.type); | |
| const bIndex = order.indexOf(b.type); | |
| export function sortEnvironments<T extends SortType>( | |
| environments: T[], | |
| sortOrder?: RuntimeEnvironmentType[] | |
| ): T[] { | |
| const order = sortOrder ?? environmentSortOrder; | |
| const getIndex = (type: RuntimeEnvironmentType) => { | |
| const overrideIndex = order.indexOf(type); | |
| if (overrideIndex !== -1) return overrideIndex; | |
| return environmentSortOrder.indexOf(type); | |
| }; | |
| return environments.sort((a, b) => { | |
| const aIndex = getIndex(a.type); | |
| const bIndex = getIndex(b.type); |
🤖 Prompt for AI Agents
In apps/webapp/app/utils/environmentSort.ts around lines 15 to 23, the current
sort uses order.indexOf(a.type)/indexOf(b.type) directly so unknown types in a
partial custom sortOrder yield -1 and float to the top; change the comparison to
treat a missing type as either its index in the default environmentSortOrder
(fallback) or a high sentinel (e.g., Number.MAX_SAFE_INTEGER) so that any type
not present in the supplied sortOrder falls back to the default ordering;
compute aIndex = order.indexOf(a.type); if aIndex === -1 set aIndex =
environmentSortOrder.indexOf(a.type) (or sentinel if not found), do the same for
bIndex, then compare aIndex - bIndex.
| const organization = await this._replica.organization.findFirst({ | ||
| select: { | ||
| title: true, | ||
| }, | ||
| where: { id: organizationId }, | ||
| }); | ||
|
|
||
| const [error, result] = await tryCatch( | ||
| sendToPlain({ | ||
| userId, | ||
| email: user.email, | ||
| name: user.name ?? user.displayName ?? user.email, | ||
| title: `Concurrency quota request: ${totalExtraConcurrency}`, | ||
| components: [ | ||
| uiComponent.text({ | ||
| text: `Org: ${organization?.title} (${organizationId})`, |
There was a problem hiding this comment.
Handle missing organization gracefully.
If the organization is not found, organization?.title will be undefined, resulting in the text "undefined" appearing in the Plain message. Consider handling this case explicitly.
Apply this diff:
const organization = await this._replica.organization.findFirst({
select: {
title: true,
},
where: { id: organizationId },
});
+ if (!organization) {
+ return {
+ success: false,
+ error: "Organization not found.",
+ };
+ }
+
const [error, result] = await tryCatch(
sendToPlain({
userId,
email: user.email,
name: user.name ?? user.displayName ?? user.email,
title: `Concurrency quota request: ${totalExtraConcurrency}`,
components: [
uiComponent.text({
- text: `Org: ${organization?.title} (${organizationId})`,
+ text: `Org: ${organization.title} (${organizationId})`,
}),🤖 Prompt for AI Agents
In apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts around lines 96 to
111, the code uses organization?.title directly which can render "undefined" in
the Plain message if the organization isn't found; update the message to handle
a missing organization by providing a safe fallback (e.g., const orgTitle =
organization?.title ?? 'Unknown organization' or 'N/A') and use that fallback in
the uiComponent.text call so the Plain text reads a sensible value instead of
"undefined".
matt-aitken
force-pushed
the
concurrency-self-serve
branch
from
36f6e48 to
8d2dbbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/routes/storybook.input-fields/route.tsx (1)
2-2: Remove unused import.The
EnvironmentLabelimport is no longer used after the removal of the second Input block mentioned in the summary.Apply this diff to remove the unused import:
import { MagnifyingGlassIcon } from "@heroicons/react/20/solid"; -import { EnvironmentLabel } from "~/components/environments/EnvironmentLabel"; import { Input } from "~/components/primitives/Input";
♻️ Duplicate comments (6)
apps/webapp/app/components/primitives/Toast.tsx (1)
25-40: Restore safe default for ephemeral when no explicit value is providedIf
options.ephemeralis ever omitted (e.g., a toast not created via the helpers, or any future call site that forgets to set it), this line treats it as falsy and makes the toast effectively “permanent” for 24 h. That’s a surprising UX for plain toasts without actions.You can make this more robust (and match the earlier feedback) by falling back to
truewhenoptions.ephemeralis undefined:- const ephemeral = options.action ? false : options.ephemeral; + const ephemeral = options.action ? false : options.ephemeral ?? true;This keeps action toasts non‑ephemeral while ensuring vanilla toasts still dismiss after the default duration.
apps/webapp/app/models/project.server.ts (1)
57-68: Race condition allows exceeding project limit.Because the count check runs outside a transaction, concurrent
createProjectcalls can both read a count belowmaximumProjectCountand proceed to create projects, resulting in more projects than allowed. Wrap the count+create sequence in a Prisma transaction withSERIALIZABLEisolation or useSELECT ... FOR UPDATEto ensure only one request can pass the guard at a time.Apply this diff to fix the race condition:
+ const project = await prisma.$transaction( + async (tx) => { - const projectCount = await prisma.project.count({ + const projectCount = await tx.project.count({ where: { organizationId: organization.id, deletedAt: null, }, }); if (projectCount >= organization.maximumProjectCount) { throw new ExceededProjectLimitError( `This organization has reached the maximum number of projects (${organization.maximumProjectCount}).` ); } - //ensure the slug is globally unique - const uniqueProjectSlug = `${slug(name)}-${nanoid(4)}`; - const projectWithSameSlug = await prisma.project.findFirst({ - where: { slug: uniqueProjectSlug }, - }); - - if (attemptCount > 100) { - throw new Error(`Unable to create project with slug ${uniqueProjectSlug} after 100 attempts`); - } - - if (projectWithSameSlug) { - return createProject( - { - organizationSlug, - name, - userId, - version, - }, - attemptCount + 1 - ); - } - - const project = await prisma.project.create({ + //ensure the slug is globally unique + const uniqueProjectSlug = `${slug(name)}-${nanoid(4)}`; + const projectWithSameSlug = await tx.project.findFirst({ + where: { slug: uniqueProjectSlug }, + }); + + if (attemptCount > 100) { + throw new Error(`Unable to create project with slug ${uniqueProjectSlug} after 100 attempts`); + } + + if (projectWithSameSlug) { + throw new Error("Project slug collision detected"); + } + + return tx.project.create({ data: { name, slug: uniqueProjectSlug, organization: { connect: { slug: organizationSlug, }, }, externalRef: `proj_${externalRefGenerator()}`, version: version === "v3" ? "V3" : "V2", }, include: { organization: { include: { members: true, }, }, }, }); + }, + { isolationLevel: "Serializable" } + ); + + if (!project) { + return createProject({ organizationSlug, name, userId, version }, attemptCount + 1); + }apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (1)
217-217: Empty fragment breaks Radix DialogTrigger.The
Feedbackcomponent wraps itsbuttonprop in a RadixDialogTriggerwithasChild, which requires a single ref-able focusable child. Passing an empty fragment will throw a runtime error ("asChild expects a single child that can accept a ref") and break the page.Apply this diff to provide a valid trigger element:
- <Feedback button={<></>} /> + <Feedback + button={<button type="button" className="sr-only" aria-label="Open feedback dialog" />} + />Alternatively, if no visible trigger is desired, modify the
Feedbackcomponent to conditionally render theDialogTriggeronly when a valid button prop is provided.apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
41-48: Fix extra concurrency totals so partial payloads can’t bypass the cap.
previousExtrasums extras for all environments, butnewExtraonly sums the submittedenvironments. If a client omits an environment that already has extra concurrency, its extra is preserved but excluded fromnewExtra, sochangeunderestimates the real increase and can let the org exceed its purchased concurrency. RecomputenewExtrafrom the authoritative list, overlaying the submitted amounts and clamping negatives:- const previousExtra = result.environments.reduce( - (acc, e) => Math.max(0, e.maximumConcurrencyLimit - e.planConcurrencyLimit) + acc, - 0 - ); - const newExtra = environments.reduce((acc, e) => e.amount + acc, 0); - const change = newExtra - previousExtra; + const previousExtra = result.environments.reduce( + (acc, e) => Math.max(0, e.maximumConcurrencyLimit - e.planConcurrencyLimit) + acc, + 0 + ); + + const requested = new Map(environments.map((e) => [e.id, e.amount])); + const newExtra = result.environments.reduce((acc, env) => { + const targetExtra = requested.has(env.id) + ? Math.max(0, requested.get(env.id)!) + : Math.max(0, env.maximumConcurrencyLimit - env.planConcurrencyLimit); + return acc + targetExtra; + }, 0); + + const change = newExtra - previousExtra;This keeps the guard honest even if the client sends a partial payload.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx (1)
116-131: Allowpurchaseactions to set total extra concurrency to 0.The schema currently requires
amount ≥ 1for bothpurchaseandquota-increase. When the user removes all purchased concurrency (modal sets total to0), the submission fails validation and the action never runs, blocking the “remove all extra concurrency” flow. Split the union members sopurchaseaccepts0whilequota-increasestill requires a positive amount:-const FormSchema = z.discriminatedUnion("action", [ - z.object({ - action: z.enum(["purchase", "quota-increase"]), - amount: z.coerce.number().min(1, "Amount must be greater than 0"), - }), - z.object({ - action: z.enum(["allocate"]), +const FormSchema = z.discriminatedUnion("action", [ + z.object({ + action: z.literal("purchase"), + amount: z.coerce.number().min(0, "Amount must be 0 or more"), + }), + z.object({ + action: z.literal("quota-increase"), + amount: z.coerce.number().min(1, "Amount must be greater than 0"), + }), + z.object({ + action: z.literal("allocate"), // It will only update environments that are passed in environments: z.array( z.object({ id: z.string(), amount: z.coerce.number().min(0, "Amount must be 0 or more"), }) ), }), ]);This matches the UI’s
min={0}and supports full removal of purchased concurrency.apps/webapp/app/services/platform.v3.server.ts (1)
258-282: Fall back to orgmaximumConcurrencyLimitwhen plan limits are missing, not throw.On cloud,
getDefaultEnvironmentConcurrencyLimitcallsgetDefaultEnvironmentLimitFromPlanand throws"No plan found"when it returnsundefined(or other falsy), and also throws on!result.success. For downgraded/free plans that don’t define per-envconcurrentRunslimits, this will crash environment creation instead of behaving like the no‑client path, which already falls back toorganization.maximumConcurrencyLimit. Reuse that fallback in the error/missing cases:export async function getDefaultEnvironmentConcurrencyLimit( organizationId: string, environmentType: RuntimeEnvironmentType ): Promise<number> { - if (!client) { - const org = await $replica.organization.findFirst({ - where: { - id: organizationId, - }, - select: { - maximumConcurrencyLimit: true, - }, - }); - if (!org) throw new Error("Organization not found"); - return org.maximumConcurrencyLimit; - } - - const result = await client.currentPlan(organizationId); - if (!result.success) throw new Error("Error getting current plan"); - - const limit = getDefaultEnvironmentLimitFromPlan(environmentType, result); - if (!limit) throw new Error("No plan found"); - - return limit; + const getOrgMax = async () => { + const org = await $replica.organization.findFirst({ + where: { id: organizationId }, + select: { maximumConcurrencyLimit: true }, + }); + if (!org) throw new Error("Organization not found"); + return org.maximumConcurrencyLimit; + }; + + if (!client) { + return getOrgMax(); + } + + const result = await client.currentPlan(organizationId); + if (!result.success) { + logger.error("Error getting current plan - no success", { + organizationId, + error: result.error, + }); + return getOrgMax(); + } + + const limit = getDefaultEnvironmentLimitFromPlan(environmentType, result); + if (!limit) { + return getOrgMax(); + } + + return limit; }This keeps env creation resilient when billing data is incomplete while still logging platform issues.
🧹 Nitpick comments (6)
apps/webapp/app/routes/storybook.stepper/route.tsx (1)
44-44: Consider consistent disabled prop syntax.Line 44 uses the shorthand
disabledwhile Line 68 uses the explicitdisabled={true}. Both are functionally equivalent, but using a consistent style improves readability.Apply this diff for consistency:
- disabled + disabled={true}Also applies to: 68-68
apps/webapp/app/models/message.server.ts (1)
13-32: ToastMessageAction/options shape looks solid; JSDoc wording could be tightenedThe action and title extensions are consistent with
ToastMessageActionusage inToast.tsxand withButtonVariant/FeedbackType. One nit: the JSDoc onactionhas a typo and reads a bit confusingly about the ephemeral behaviour.Suggested tweak just for clarity:
- /** This display a button and make it not ephemeral, unless ephemeral is explicitlyset to false */ + /** Displays a button and by default makes the toast non‑ephemeral (unless `ephemeral` is explicitly set). */This keeps the comment aligned with the UI logic while fixing the typo.
apps/webapp/app/components/primitives/Toast.tsx (2)
72-84: UI layout and title/action wiring look good; consider setting button type explicitlyThe new vertical layout with
Header2+ dimmedParagraphand theActionslot is clean and matches the new toast metadata.One small defensive tweak: the close button does not currently set
type="button". If this ever ends up rendered inside a<form>, clicking it would submit the form.Suggested change:
- <button + <button + type="button" className="hover:bg-midnight-800 -mr-1 -mt-1 ms-auto rounded p-2 text-text-dimmed transition hover:text-text-bright" onClick={() => toast.dismiss(t)} >Also applies to: 85-90
96-139: Action component wiring is solid; consider preserving existing query params when opening feedbackThe
Actioncomponent correctly handles bothlinkandhelpactions and usesEnvelopeIcon+Button/LinkButtonas expected. One behavioural detail:setSearchParams({ feedbackPanel: feedbackType, });replaces the entire query string, which may drop any existing search params on the current page.
If you want to keep other params intact while toggling
feedbackPanel, you could merge instead:- onClick={() => { - setSearchParams({ - feedbackPanel: feedbackType, - }); - toast.dismiss(toastId); - }} + onClick={() => { + setSearchParams((prev) => { + const params = new URLSearchParams(prev); + params.set("feedbackPanel", feedbackType); + return params; + }); + toast.dismiss(toastId); + }}This keeps all existing filters/sorting/etc. while still opening the feedback panel.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx (2)
92-114:getPlansloader call is ineffective due totryCatchusage.
tryCatch(getPlans())returns a tuple[error, result], soplansis always truthy here and the 404 branch is never hit; the value also isn’t used. Either drop this call (the presenter already fetches plans) or check the actual result:- const plans = await tryCatch(getPlans()); - if (!plans) { - throw new Response(null, { status: 404, statusText: "Plans not found" }); - } + const plans = await getPlans(); + if (!plans) { + throw new Response(null, { status: 404, statusText: "Plans not found" }); + }Or simply remove these lines if you’re happy to rely on
ManageConcurrencyPresenter’s internalgetPlanserror handling.
278-295: Give the allocation form its own Conform form id.Both
UpgradableandPurchaseConcurrencyModaluseuseForm({ id: "purchase-concurrency", ... }), but they represent different forms (allocatevspurchase/quota). Sharing the same Conform id can make validation andlastSubmissionstate bleed between them. Using a distinct id for the allocation form keeps concerns separated:- const [form, { environments: formEnvironments }] = useForm({ - id: "purchase-concurrency", + const [form, { environments: formEnvironments }] = useForm({ + id: "allocate-concurrency",The shared
FormSchemaanduseActionDatacan stay as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/webapp/app/assets/icons/ConcurrencyIcon.tsx(1 hunks)apps/webapp/app/components/Feedback.tsx(3 hunks)apps/webapp/app/components/navigation/SideMenu.tsx(4 hunks)apps/webapp/app/components/primitives/Input.tsx(1 hunks)apps/webapp/app/components/primitives/InputNumberStepper.tsx(1 hunks)apps/webapp/app/components/primitives/Toast.tsx(5 hunks)apps/webapp/app/models/message.server.ts(3 hunks)apps/webapp/app/models/organization.server.ts(2 hunks)apps/webapp/app/models/project.server.ts(2 hunks)apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx(4 hunks)apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.ts(1 hunks)apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts(2 hunks)apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency.ts(0 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx(1 hunks)apps/webapp/app/routes/storybook.input-fields/route.tsx(1 hunks)apps/webapp/app/routes/storybook.stepper/route.tsx(1 hunks)apps/webapp/app/routes/storybook/route.tsx(1 hunks)apps/webapp/app/services/platform.v3.server.ts(4 hunks)apps/webapp/app/utils/environmentSort.ts(1 hunks)apps/webapp/app/utils/pathBuilder.ts(1 hunks)apps/webapp/app/v3/services/allocateConcurrency.server.ts(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(1 hunks)apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(1 hunks)apps/webapp/package.json(1 hunks)internal-packages/database/prisma/migrations/20251113152235_maximum_project_count/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues/route.tsx
- apps/webapp/app/components/primitives/Input.tsx
- apps/webapp/app/v3/services/triggerTaskV1.server.ts
- internal-packages/database/prisma/schema.prisma
- apps/webapp/app/utils/environmentSort.ts
- apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts
- internal-packages/database/prisma/migrations/20251113152235_maximum_project_count/migration.sql
- apps/webapp/app/components/Feedback.tsx
- apps/webapp/app/routes/storybook/route.tsx
- apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts
- apps/webapp/app/routes/api.v1.orgs.$orgParam.projects.ts
- apps/webapp/package.json
- apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.ts
- apps/webapp/app/components/primitives/InputNumberStepper.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:37:42.902Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2461
File: packages/core/src/v3/runEngineWorker/supervisor/consumerPool.ts:315-317
Timestamp: 2025-09-02T11:37:42.902Z
Learning: In packages/core/src/v3/runEngineWorker/supervisor/scalingStrategies.ts, the ScalingStrategy base class already handles clamping to min/max bounds in the public calculateTargetCount method, and the individual strategy implementations handle rounding internally using Math.round, Math.floor, and Math.ceil as appropriate.
Applied to files:
apps/webapp/app/v3/services/createBackgroundWorker.server.ts
🧬 Code graph analysis (11)
apps/webapp/app/routes/storybook.stepper/route.tsx (2)
apps/webapp/app/components/primitives/Headers.tsx (2)
Header2(52-70)Header3(72-90)apps/webapp/app/components/primitives/InputNumberStepper.tsx (1)
InputNumberStepper(13-220)
apps/webapp/app/models/project.server.ts (1)
apps/webapp/app/db.server.ts (1)
prisma(101-101)
apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (1)
apps/webapp/app/services/platform.v3.server.ts (1)
setPlan(339-400)
apps/webapp/app/components/primitives/Toast.tsx (4)
apps/webapp/app/models/message.server.ts (1)
ToastMessageAction(13-25)apps/webapp/app/components/primitives/Headers.tsx (1)
Header2(52-70)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)apps/webapp/app/components/primitives/Buttons.tsx (2)
LinkButton(335-401)Button(296-329)
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (4)
apps/webapp/app/models/project.server.ts (1)
ExceededProjectLimitError(19-24)apps/webapp/app/models/message.server.ts (1)
redirectWithErrorMessage(201-218)apps/webapp/app/utils/pathBuilder.ts (1)
newProjectPath(129-133)apps/webapp/app/components/Feedback.tsx (1)
Feedback(29-177)
apps/webapp/app/models/organization.server.ts (1)
apps/webapp/app/services/platform.v3.server.ts (1)
getDefaultEnvironmentConcurrencyLimit(258-282)
apps/webapp/app/models/message.server.ts (2)
apps/webapp/app/components/primitives/Buttons.tsx (1)
ButtonVariant(166-166)apps/webapp/app/routes/resources.feedback.ts (1)
FeedbackType(21-21)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx (10)
apps/webapp/app/services/session.server.ts (1)
requireUserId(25-35)apps/webapp/app/utils/pathBuilder.ts (3)
EnvironmentParamSchema(26-28)concurrencyPath(466-472)v3BillingPath(482-486)apps/webapp/app/models/project.server.ts (1)
findProjectBySlug(136-147)apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (3)
ManageConcurrencyPresenter(33-132)ConcurrencyResult(10-21)EnvironmentWithConcurrency(23-31)apps/webapp/app/services/platform.v3.server.ts (1)
getPlans(323-337)apps/webapp/app/models/message.server.ts (2)
redirectWithErrorMessage(201-218)redirectWithSuccessMessage(182-199)apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
AllocateConcurrencyService(22-91)apps/webapp/app/v3/services/setConcurrencyAddOn.server.ts (1)
SetConcurrencyAddOnService(26-143)apps/webapp/app/routes/_app.orgs.$organizationSlug/route.tsx (1)
useCurrentPlan(22-29)apps/webapp/app/hooks/useOrganizations.ts (1)
useOrganization(39-43)
apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
apps/webapp/app/presenters/v3/ManageConcurrencyPresenter.server.ts (1)
ManageConcurrencyPresenter(33-132)
apps/webapp/app/components/navigation/SideMenu.tsx (4)
apps/webapp/app/hooks/useFeatures.ts (1)
useFeatures(5-9)apps/webapp/app/components/navigation/SideMenuItem.tsx (1)
SideMenuItem(7-53)apps/webapp/app/assets/icons/ConcurrencyIcon.tsx (1)
ConcurrencyIcon(1-13)apps/webapp/app/utils/pathBuilder.ts (1)
concurrencyPath(466-472)
apps/webapp/app/services/platform.v3.server.ts (4)
apps/webapp/app/database-types.ts (1)
RuntimeEnvironmentType(49-54)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/models/message.server.ts (2)
redirectWithErrorMessage(201-218)redirectWithSuccessMessage(182-199)apps/webapp/app/utils/pathBuilder.ts (1)
newProjectPath(129-133)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
apps/webapp/app/routes/storybook.input-fields/route.tsx (1)
23-25: LGTM!The new outline variant demonstrations are well-structured and align with the Input component's expanded variant support.
apps/webapp/app/routes/storybook.stepper/route.tsx (1)
1-75: LGTM!The InputNumberStepper demonstrations effectively showcase various configurations including different step sizes, min/max bounds, disabled states, and control sizes. State management and onChange handlers are implemented correctly.
apps/webapp/app/models/message.server.ts (1)
57-59: Centralizing the ephemeral default in the helpers is a good improvementDefaulting
ephemeralviaoptions?.ephemeral ?? trueafter spreadingoptionsensures that:
- plain toasts (no explicit options) remain ephemeral by default;
- explicit
falseis respected; and- downstream consumers see a defined boolean.
This also makes the server the single source of truth for toast lifespan defaults, which simplifies the Toast UI logic.
Also applies to: 68-70
apps/webapp/app/assets/icons/ConcurrencyIcon.tsx (1)
1-13: LGTM!The icon component follows the established pattern in the codebase and correctly forwards the className prop to the SVG element. The visual representation with filled and stroked circles effectively conveys the concurrency concept.
apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
362-365: LGTM! Simplified concurrency limit calculation.The change to clamp only against
environment.maximumConcurrencyLimit(removing the organization-level limit) aligns with the new per-environment concurrency management approach introduced in this PR.apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (1)
156-158: LGTM! Improved error handling.Adding
awaitensures that any errors fromsetPlanare properly caught within this function's error handling context, rather than potentially becoming unhandled promise rejections.apps/webapp/app/utils/pathBuilder.ts (1)
466-472: LGTM!The new
concurrencyPathhelper follows the same pattern as existing path builders likebranchesPath, maintaining consistency across the codebase.apps/webapp/app/components/navigation/SideMenu.tsx (1)
27-27: LGTM! Well-integrated feature-flagged menu item.The new Concurrency menu item is properly gated behind the
isManagedCloudfeature flag and follows the established pattern for other menu items in the SideMenu component.Also applies to: 47-47, 127-127, 319-327
apps/webapp/app/models/organization.server.ts (1)
99-100: LGTM! Dynamic concurrency limit calculation.Replacing the hard-coded division by 3 with
getDefaultEnvironmentConcurrencyLimitprovides a more flexible, plan-aware approach to setting per-environment concurrency limits.Also applies to: 108-108
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (1)
119-142: LGTM! Comprehensive error handling.The enhanced error handling provides a good user experience by distinguishing between project limit errors (with a helpful action to request more projects) and other errors, ensuring users receive appropriate feedback.
apps/webapp/app/v3/services/allocateConcurrency.server.ts (1)
57-85: Env update loop and paused check look solid.The per-environment update correctly validates the env id against the presenter result, updates
maximumConcurrencyLimitfrom the plan limit + requested extra, and only callsupdateEnvConcurrencyLimitswhen the environment isn’t paused. That sequencing is good from a consistency and side‑effects standpoint.
| const project = await findProjectBySlug(organizationSlug, projectParam, userId); | ||
| const redirectPath = concurrencyPath( | ||
| { slug: organizationSlug }, | ||
| { slug: projectParam }, | ||
| { slug: envParam } | ||
| ); | ||
|
|
||
| if (!project) { | ||
| throw redirectWithErrorMessage(redirectPath, request, "Project not found"); | ||
| } |
There was a problem hiding this comment.
Don’t throw the Promise from redirectWithErrorMessage.
redirectWithErrorMessage is async and returns a Promise<Response>. throw redirectWithErrorMessage(...) throws the unresolved Promise instead of a Response, which will break Remix’s redirect/error handling. Return the redirect (or throw the awaited result) instead:
- if (!project) {
- throw redirectWithErrorMessage(redirectPath, request, "Project not found");
- }
+ if (!project) {
+ return redirectWithErrorMessage(redirectPath, request, "Project not found");
+ }📝 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.
| const project = await findProjectBySlug(organizationSlug, projectParam, userId); | |
| const redirectPath = concurrencyPath( | |
| { slug: organizationSlug }, | |
| { slug: projectParam }, | |
| { slug: envParam } | |
| ); | |
| if (!project) { | |
| throw redirectWithErrorMessage(redirectPath, request, "Project not found"); | |
| } | |
| const project = await findProjectBySlug(organizationSlug, projectParam, userId); | |
| const redirectPath = concurrencyPath( | |
| { slug: organizationSlug }, | |
| { slug: projectParam }, | |
| { slug: envParam } | |
| ); | |
| if (!project) { | |
| return redirectWithErrorMessage(redirectPath, request, "Project not found"); | |
| } |
🤖 Prompt for AI Agents
In
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.concurrency/route.tsx
around lines 137 to 146, the code currently uses "throw
redirectWithErrorMessage(...)" which throws an unresolved Promise
(redirectWithErrorMessage is async). Replace that by returning the awaited
redirect response or awaiting it and then throwing the resolved Response: either
"return await redirectWithErrorMessage(...)" or "throw await
redirectWithErrorMessage(...)" so the actual Response (not a Promise) is
returned/thrown for Remix to handle.
For Cloud this allows you to add concurrency to your environments by purchasing the add-on and then allocating concurrency.
It also adds a project limit (default 10) to organizations.