feat: expose project build settings #2507
Conversation
|
WalkthroughAdds Build Settings end-to-end. Database: migration adding a nullable JSONB column Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (8)
internal-packages/database/prisma/schema.prisma (1)
398-398: Add a brief field doc and clarify null vs empty-object semantics.To avoid ambiguity downstream, document that
nullmeans “unset/defaults” and{}means “user explicitly set, but no overrides.” This also helps future migrations and UI behavior.connectedGithubRepository ConnectedGithubRepository? - buildSettings Json? + /// Build settings for the project. Use NULL to indicate "unset/defaults". + buildSettings Json?apps/webapp/app/v3/buildSettings.ts (1)
3-7: Normalize inputs: trim, collapse empty strings to undefined, and cap length.Prevents storing whitespace-only values and overly long commands/paths. Keeps the UI forgiving (empty input → unset).
-export const BuildSettingsSchema = z.object({ - rootDirectory: z.string().optional(), - installCommand: z.string().optional(), - triggerConfigFile: z.string().optional(), -}); +const optionalSanitizedString = z.preprocess( + (v) => { + if (typeof v !== "string") return v; + const t = v.trim(); + return t === "" ? undefined : t; + }, + z.string().max(512).optional() +); + +export const BuildSettingsSchema = z + .object({ + rootDirectory: optionalSanitizedString, + installCommand: optionalSanitizedString, + triggerConfigFile: optionalSanitizedString, + }) + .strip();apps/webapp/app/services/projectSettings.server.ts (1)
1-1: Import Prisma to explicitly set DB NULL for “unset” build settings.We’ll use
Prisma.DbNullto store NULL (vs JSON null) when all fields are empty after normalization.-import { type PrismaClient } from "@trigger.dev/database"; +import { Prisma, type PrismaClient } from "@trigger.dev/database";apps/webapp/app/services/projectSettingsPresenter.server.ts (2)
30-49: Remove redundant null check; getProject() already guarantees a project or an error.Simplifies flow; no behavior change.
- return getProject().andThen((project) => { - if (!project) { - return err({ type: "project_not_found" as const }); - } - - const buildSettingsOrFailure = BuildSettingsSchema.safeParse(project.buildSettings); + return getProject().andThen((project) => { + const buildSettingsOrFailure = BuildSettingsSchema.safeParse(project.buildSettings); const buildSettings = buildSettingsOrFailure.success ? buildSettingsOrFailure.data : undefined;
36-49: DRY the BuildSettings parsing.Extract a tiny helper to avoid repeating
safeParseand ensure consistent behavior.- const buildSettingsOrFailure = BuildSettingsSchema.safeParse(project.buildSettings); - const buildSettings = buildSettingsOrFailure.success - ? buildSettingsOrFailure.data - : undefined; + const buildSettings = getBuildSettings(project.buildSettings);Add this helper near the top of the file:
function getBuildSettings(raw: unknown) { const r = BuildSettingsSchema.safeParse(raw); return r.success ? r.data : undefined; }Also applies to: 135-139, 159-161
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (3)
65-65: Avoid default React import; prefer fragment shorthand.No need to import the default React object when using the automatic JSX runtime. Import Fragment or use <>…</> instead.
Apply:
-import React, { useEffect, useState } from "react"; +import { useEffect, useState, Fragment } from "react";And replace React.Fragment usages accordingly:
- <React.Fragment> + <Fragment> ... - </React.Fragment> + </Fragment>
388-412: Move normalization into the schema; stop coalescing in the action.Once the schema coerces blanks to undefined, this extra “|| undefined” is unnecessary and can mask intent.
Apply:
- const resultOrFail = await projectSettingsService.updateBuildSettings(projectId, { - rootDirectory: rootDirectory || undefined, - installCommand: installCommand || undefined, - triggerConfigFile: triggerConfigFile || undefined, - }); + const resultOrFail = await projectSettingsService.updateBuildSettings(projectId, { + rootDirectory, + installCommand, + triggerConfigFile, + });
1087-1196: Tighten UX copy and placeholders for clarity; avoid suggesting “/” root.“/” reads as an absolute path; these should be repo-relative. Also simplify placeholders.
Apply:
- <Input + <Input {...conform.input(fields.rootDirectory, { type: "text" })} defaultValue={buildSettings?.rootDirectory || ""} - placeholder="/" + placeholder="e.g., packages/app" onChange={(e) => { setBuildSettingsValues((prev) => ({ ...prev, rootDirectory: e.target.value, })); }} /> - <Hint>The directory that contains your code.</Hint> + <Hint>Relative to your repository root. Leave blank to use the repo root.</Hint>- <Input + <Input {...conform.input(fields.installCommand, { type: "text" })} defaultValue={buildSettings?.installCommand || ""} - placeholder="e.g., `npm install`, or `bun install`" + placeholder="e.g., npm install or bun install" onChange={(e) => { setBuildSettingsValues((prev) => ({ ...prev, installCommand: e.target.value, })); }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx(9 hunks)apps/webapp/app/services/projectSettings.server.ts(2 hunks)apps/webapp/app/services/projectSettingsPresenter.server.ts(5 hunks)apps/webapp/app/v3/buildSettings.ts(1 hunks)internal-packages/database/prisma/migrations/20250915141201_add_build_settings_to_project/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)
🧰 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:
apps/webapp/app/v3/buildSettings.tsapps/webapp/app/services/projectSettings.server.tsapps/webapp/app/services/projectSettingsPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.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/v3/buildSettings.tsapps/webapp/app/services/projectSettings.server.tsapps/webapp/app/services/projectSettingsPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.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/v3/buildSettings.tsapps/webapp/app/services/projectSettings.server.tsapps/webapp/app/services/projectSettingsPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
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/v3/buildSettings.tsapps/webapp/app/services/projectSettings.server.tsapps/webapp/app/services/projectSettingsPresenter.server.ts
{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/services/projectSettings.server.tsapps/webapp/app/services/projectSettingsPresenter.server.ts
🧠 Learnings (3)
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to trigger.config.ts : Declare build options and extensions (external, jsx, conditions, extensions) via the build block in trigger.config.ts rather than custom scripts
Applied to files:
apps/webapp/app/v3/buildSettings.ts
📚 Learning: 2025-07-18T17:49:24.468Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T17:49:24.468Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : We use zod a lot in packages/core and in the webapp
Applied to files:
apps/webapp/app/v3/buildSettings.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use schemaTask({ schema, run, ... }) to validate payloads when input validation is required
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
🧬 Code graph analysis (3)
apps/webapp/app/services/projectSettings.server.ts (1)
apps/webapp/app/v3/buildSettings.ts (1)
BuildSettings(9-9)
apps/webapp/app/services/projectSettingsPresenter.server.ts (1)
apps/webapp/app/v3/buildSettings.ts (1)
BuildSettingsSchema(3-7)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (2)
apps/webapp/app/models/message.server.ts (2)
redirectBackWithErrorMessage(200-207)redirectBackWithSuccessMessage(209-216)apps/webapp/app/v3/buildSettings.ts (1)
BuildSettings(9-9)
⏰ 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 / webapp / 🧪 Unit Tests: Webapp (8, 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 (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
internal-packages/database/prisma/migrations/20250915141201_add_build_settings_to_project/migration.sql (1)
1-1: LGTM: additive, nullable JSONB column.No data backfill required; safe forward migration.
apps/webapp/app/services/projectSettings.server.ts (2)
248-263: No buildSettings values are being interpolated into shell commands — advisory onlySearches for buildSettings/installCommand/rootDirectory/triggerConfigFile (including template interpolation/concatenation) and for child_process/execa usage found no instances where buildSettings values are passed into exec/spawn/execa. Relevant locations: apps/webapp/app/services/projectSettings.server.ts (updateBuildSettings), apps/webapp/app/v3/buildSettings.ts (schema), apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (form + action). Child_process/execa usages exist (e.g., packages/docker-provider/src/index.ts, scripts/*) but they do not reference buildSettings. If these fields are ever used to run shell commands, pass values as argument arrays (execFile/spawn/execa with args) or strictly whitelist/validate/escape them.
248-263: Normalize and store DB NULL for empty buildSettings.Trim string fields and drop empty strings; if the normalized object is empty, persist Prisma.DbNull instead of {}. The generated Prisma client was not found at internal-packages/database/src/generated/prisma — verify that Prisma.DbNull is exported by @trigger.dev/database (or import Prisma from @prisma/client) before applying.
- updateBuildSettings(projectId: string, buildSettings: BuildSettings) { - return fromPromise( - this.#prismaClient.project.update({ - where: { - id: projectId, - }, - data: { - buildSettings: buildSettings, - }, - }), - (error) => ({ - type: "other" as const, - cause: error, - }) - ); - } + updateBuildSettings(projectId: string, buildSettings: BuildSettings) { + const normalized = Object.fromEntries( + Object.entries(buildSettings) + .filter(([_, v]) => typeof v === "string") + .map(([k, v]) => [k, (v as string).trim()]) + .filter(([_, v]) => (v as string).length > 0) + ) as BuildSettings; + + const data = + Object.keys(normalized).length === 0 + ? { buildSettings: Prisma.DbNull } + : { buildSettings: normalized }; + + return fromPromise( + this.#prismaClient.project.update({ + where: { id: projectId }, + data, + }), + (error) => ({ type: "other" as const, cause: error }) + ); + }apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (3)
123-138: Good: loader surfaces buildSettings with other project metadata.This keeps the page render simple and consistent with existing loader patterns.
199-200: LGTM: discriminated union extended with update-build-settings.Keeps action parsing consistent with existing patterns.
426-427: LGTM: loader data consumption typed via useTypedLoaderData.Props stay aligned with loader return type.
...p/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
Show resolved
Hide resolved
...p/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
Show resolved
Hide resolved
...p/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
Outdated
Show resolved
Hide resolved
myftija
force-pushed
the
project-build-settings
branch
from
9d3adb5 to
9d9ce24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (3)
80-80: Good: reusing the shared BuildSettings type.This avoids local type drift. Matches prior feedback.
574-600: Don’t gate Build settings behind GitHub enablement.Users should edit build settings without connecting GitHub. Render Build settings unconditionally; keep Git settings conditional.
Apply:
- {githubAppEnabled && ( - <React.Fragment> - <div> - <Header2 spacing>Git settings</Header2> - <div className="w-full rounded-sm border border-grid-dimmed p-4"> - {connectedGithubRepository ? ( - <ConnectedGitHubRepoForm connectedGitHubRepo={connectedGithubRepository} /> - ) : ( - <GitHubConnectionPrompt - gitHubAppInstallations={githubAppInstallations ?? []} - organizationSlug={organization.slug} - projectSlug={project.slug} - environmentSlug={environment.slug} - openGitHubRepoConnectionModal={openGitHubRepoConnectionModal} - /> - )} - </div> - </div> - - <div> - <Header2 spacing>Build settings</Header2> - <div className="w-full rounded-sm border border-grid-dimmed p-4"> - <BuildSettingsForm buildSettings={buildSettings ?? {}} /> - </div> - </div> - </React.Fragment> - )} + {githubAppEnabled && ( + <div> + <Header2 spacing>Git settings</Header2> + <div className="w-full rounded-sm border border-grid-dimmed p-4"> + {connectedGithubRepository ? ( + <ConnectedGitHubRepoForm connectedGitHubRepo={connectedGithubRepository} /> + ) : ( + <GitHubConnectionPrompt + gitHubAppInstallations={githubAppInstallations ?? []} + organizationSlug={organization.slug} + projectSlug={project.slug} + environmentSlug={environment.slug} + openGitHubRepoConnectionModal={openGitHubRepoConnectionModal} + /> + )} + </div> + </div> + )} + + <div> + <Header2 spacing>Build settings</Header2> + <div className="w-full rounded-sm border border-grid-dimmed p-4"> + <BuildSettingsForm buildSettings={buildSettings ?? {}} /> + </div> + </div>
160-188: Harden path validation (Windows/UNC, backslashes, traversal) and normalize empty strings.Current rules strip leading “/” but allow “C:\…”, “\server\…”, “..\…”, and “\…”. Also, rely on action to coerce empty → undefined. Tighten here to reduce bad states.
Apply:
-const UpdateBuildSettingsFormSchema = z.object({ +const UpdateBuildSettingsFormSchema = z.object({ action: z.literal("update-build-settings"), triggerConfigFilePath: z - .string() - .trim() - .optional() - .transform((val) => (val ? val.replace(/^\/+/, "") : val)) + .string() + .trim() + .optional() + .transform((val) => (val && val.length > 0 ? val.replace(/^[/\\]+/, "") : undefined)) + .refine((val) => !val || !/^(?:[a-zA-Z]:\\|\\\\)/.test(val), { + message: "Use a relative path (no drive letters or UNC paths)", + }) + .refine((val) => !val || !/(^|[\\/])\.\.(?:[\\/]|$)/.test(val), { + message: "Parent directory traversal '..' is not allowed", + }) .refine((val) => !val || val.length <= 255, { message: "Config file path must not exceed 255 characters", }), installDirectory: z - .string() - .trim() - .optional() - .transform((val) => (val ? val.replace(/^\/+/, "") : val)) + .string() + .trim() + .optional() + .transform((val) => (val && val.length > 0 ? val.replace(/^[/\\]+/, "") : undefined)) + .refine((val) => !val || !/^(?:[a-zA-Z]:\\|\\\\)/.test(val), { + message: "Use a relative path (no drive letters or UNC paths)", + }) + .refine((val) => !val || !/(^|[\\/])\.\.(?:[\\/]|$)/.test(val), { + message: "Parent directory traversal '..' is not allowed", + }) .refine((val) => !val || val.length <= 255, { message: "Install directory must not exceed 255 characters", }), installCommand: z .string() .trim() - .optional() + .optional() .refine((val) => !val || !val.includes("\n"), { message: "Install command must be a single line", }) .refine((val) => !val || val.length <= 500, { message: "Install command must not exceed 500 characters", }), });Optional: Import and
.and(BuildSettingsInputSchema)to avoid duplication if you add it in v3/buildSettings.ts.
🧹 Nitpick comments (6)
apps/webapp/app/v3/buildSettings.ts (1)
3-9: Share a stricter input schema (trim, empty→undefined, block absolute/traversal) to avoid drift across layers.Right now the base schema allows raw strings and the route re-implements normalization. Export a reusable “input” schema here and consume it in the route to keep behavior consistent and safer.
Apply:
import { z } from "zod"; export const BuildSettingsSchema = z.object({ triggerConfigFilePath: z.string().optional(), installDirectory: z.string().optional(), installCommand: z.string().optional(), }); export type BuildSettings = z.infer<typeof BuildSettingsSchema>; + +// Optional but recommended: shared, stricter input schema for UI/API payloads +const relPath = z + .string() + .trim() + .transform((s) => (s === "" ? undefined : s)) + // strip leading slashes/backslashes + .transform((s) => (s ? s.replace(/^[/\\]+/, "") : s)) + // disallow absolute Windows/UNC, and parent traversal + .refine((v) => !v || !/^(?:[a-zA-Z]:\\|\\\\)/.test(v), { + message: "Use a relative path (no drive letters or UNC paths).", + }) + .refine((v) => !v || !/(^|[\\/])\.\.(?:[\\/]|$)/.test(v), { + message: "Parent directory traversal '..' is not allowed.", + }) + .refine((v) => !v || v.length <= 255, { message: "Max 255 characters." }); + +export const BuildSettingsInputSchema = z.object({ + triggerConfigFilePath: relPath.optional(), + installDirectory: relPath.optional(), + installCommand: z + .string() + .trim() + .transform((s) => (s === "" ? undefined : s)) + .refine((v) => !v || !/[\r\n]/.test(v), { message: "Command must be a single line." }) + .refine((v) => !v || v.length <= 500, { message: "Max 500 characters." }) + .optional(), +}); + +export type BuildSettingsInput = z.infer<typeof BuildSettingsInputSchema>;apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (5)
65-65: Unnecessary React default import (modern JSX).You can drop the default
Reactimport; keep only named hooks. Safe in TS/Remix with react-jsx.-import React, { useEffect, useState } from "react"; +import { useEffect, useState } from "react";
190-191: Avoid type/value name collision; rename inferred type.Prevents confusion with the schema constant.
-type UpdateBuildSettingsFormSchema = z.infer<typeof UpdateBuildSettingsFormSchema>; +type UpdateBuildSettingsValues = z.infer<typeof UpdateBuildSettingsFormSchema>;
414-438: Pass normalized values directly; drop manual “|| undefined”.If you adopt the schema change (empty → undefined), you can forward
submission.valueas-is.- const { installDirectory, installCommand, triggerConfigFilePath } = submission.value; - - const resultOrFail = await projectSettingsService.updateBuildSettings(projectId, { - installDirectory: installDirectory || undefined, - installCommand: installCommand || undefined, - triggerConfigFilePath: triggerConfigFilePath || undefined, - }); + const { installDirectory, installCommand, triggerConfigFilePath } = submission.value; + const resultOrFail = await projectSettingsService.updateBuildSettings(projectId, { + installDirectory, + installCommand, + triggerConfigFilePath, + });
1107-1217: Mark fields optional in UI and mirror server limits with maxLength.Improves UX consistency and prevents oversized inputs client‑side.
- <Label htmlFor={fields.triggerConfigFilePath.id}>Trigger config file</Label> + <Label htmlFor={fields.triggerConfigFilePath.id} required={false}> + Trigger config file + </Label> <Input {...conform.input(fields.triggerConfigFilePath, { type: "text" })} defaultValue={buildSettings?.triggerConfigFilePath || ""} placeholder="trigger.config.ts" + maxLength={255} onChange={(e) => { ... - <Label htmlFor={fields.installCommand.id}>Install command</Label> + <Label htmlFor={fields.installCommand.id} required={false}>Install command</Label> <Input {...conform.input(fields.installCommand, { type: "text" })} defaultValue={buildSettings?.installCommand || ""} - placeholder="e.g., `npm install`, or `bun install`" + placeholder="e.g., npm install or bun install" + maxLength={500} onChange={(e) => { ... - <Label htmlFor={fields.installDirectory.id}>Install directory</Label> + <Label htmlFor={fields.installDirectory.id} required={false}> + Install directory + </Label> <Input {...conform.input(fields.installDirectory, { type: "text" })} defaultValue={buildSettings?.installDirectory || ""} - placeholder="" + placeholder="e.g., apps/webapp" + maxLength={255} onChange={(e) => {
160-188: Optional: reuse shared input schema to eliminate duplication.If you add
BuildSettingsInputSchema, import and use it here:-const UpdateBuildSettingsFormSchema = z.object({ - action: z.literal("update-build-settings"), - ... -}); +import { BuildSettingsInputSchema } from "~/v3/buildSettings"; +const UpdateBuildSettingsFormSchema = z + .object({ action: z.literal("update-build-settings") }) + .and(BuildSettingsInputSchema);Also applies to: 414-438, 1107-1217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx(10 hunks)apps/webapp/app/v3/buildSettings.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
apps/webapp/app/v3/buildSettings.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.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/v3/buildSettings.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.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/v3/buildSettings.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
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/v3/buildSettings.ts
🧬 Code graph analysis (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (8)
apps/webapp/app/models/message.server.ts (2)
redirectBackWithErrorMessage(200-207)redirectBackWithSuccessMessage(209-216)apps/webapp/app/components/primitives/Headers.tsx (1)
Header2(52-70)apps/webapp/app/v3/buildSettings.ts (1)
BuildSettings(9-9)apps/webapp/app/components/primitives/Label.tsx (1)
Label(25-41)apps/webapp/app/components/primitives/Hint.tsx (1)
Hint(3-9)apps/webapp/app/components/primitives/FormButtons.tsx (1)
FormButtons(3-22)apps/webapp/app/components/primitives/Buttons.tsx (1)
Button(296-331)apps/webapp/app/components/primitives/Spinner.tsx (1)
SpinnerWhite(76-78)
⏰ 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). (22)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- 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: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (2)
124-139: Loader correctly exposes buildSettings.Shape looks consistent with presenter/service. LGTM.
160-188: Verify server-side consumers guard against command/path misuse.Checked: UI validation in apps/webapp/.../route.tsx trims input, strips leading slashes, and limits length/newlines; BuildSettingsSchema (apps/webapp/app/v3/buildSettings.ts) is permissive and projectSettings.server.updateBuildSettings simply persists the JSON. I found no direct usage of installCommand/installDirectory/triggerConfigFilePath being executed, but exec helpers exist (packages/core/src/v3/apps/exec.ts, apps/coordinator/src/exec.ts, apps/docker-provider/src/index.ts).
Action required: ensure any runner that consumes these fields (installCommand / paths) runs in an isolated sandbox/container, uses safe path-joining and confinement (realpath/whitelist, forbid ".."), never shell-interpolates user strings (use execFile/execa with arg arrays), and enforces timeouts and resource limits.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (2)
160-188: Reuse BuildSettingsSchema; harden normalization (Windows abs paths, CRLF) and coerce blanks to undefined.Avoid schema drift and enforce stronger constraints at the edge.
-import { type BuildSettings } from "~/v3/buildSettings"; +import { type BuildSettings, BuildSettingsSchema } from "~/v3/buildSettings";-const UpdateBuildSettingsFormSchema = z.object({ - action: z.literal("update-build-settings"), - triggerConfigFilePath: z - .string() - .trim() - .optional() - .transform((val) => (val ? val.replace(/^\/+/, "") : val)) - .refine((val) => !val || val.length <= 255, { - message: "Config file path must not exceed 255 characters", - }), - installDirectory: z - .string() - .trim() - .optional() - .transform((val) => (val ? val.replace(/^\/+/, "") : val)) - .refine((val) => !val || val.length <= 255, { - message: "Install directory must not exceed 255 characters", - }), - installCommand: z - .string() - .trim() - .optional() - .refine((val) => !val || !val.includes("\n"), { - message: "Install command must be a single line", - }) - .refine((val) => !val || val.length <= 500, { - message: "Install command must not exceed 500 characters", - }), -}); +const UpdateBuildSettingsFormSchema = z + .object({ action: z.literal("update-build-settings") }) + .and( + BuildSettingsSchema + // Normalize and coerce blanks to undefined + .transform((v) => ({ + ...v, + triggerConfigFilePath: + v.triggerConfigFilePath && v.triggerConfigFilePath.trim() !== "" + ? v.triggerConfigFilePath.replace(/^[/\\]+/, "") + : undefined, + installDirectory: + v.installDirectory && v.installDirectory.trim() !== "" + ? v.installDirectory.replace(/^[/\\]+/, "") + : undefined, + installCommand: + v.installCommand && v.installCommand.trim() !== "" + ? v.installCommand.trim() + : undefined, + })) + // Disallow absolute paths (POSIX and Windows) + .refine( + (v) => + !v.triggerConfigFilePath || + !/^(?:[a-zA-Z]:[\\/]|[\\/])/.test(v.triggerConfigFilePath), + { path: ["triggerConfigFilePath"], message: "Use a relative path (no leading slash/drive)." } + ) + .refine( + (v) => !v.installDirectory || !/^(?:[a-zA-Z]:[\\/]|[\\/])/.test(v.installDirectory), + { path: ["installDirectory"], message: "Use a relative path (no leading slash/drive)." } + ) + // Enforce single-line commands (handle CRLF and LF) + .refine((v) => !v.installCommand || !/[\r\n]/.test(v.installCommand), { + path: ["installCommand"], + message: "Command must be a single line.", + }) + );
574-600: Ungate Build settings from GitHub app enablement.Users should edit build settings regardless of GitHub connection.
- {githubAppEnabled && ( - <Fragment> - <div> - <Header2 spacing>Git settings</Header2> - <div className="w-full rounded-sm border border-grid-dimmed p-4"> - {connectedGithubRepository ? ( - <ConnectedGitHubRepoForm connectedGitHubRepo={connectedGithubRepository} /> - ) : ( - <GitHubConnectionPrompt - gitHubAppInstallations={githubAppInstallations ?? []} - organizationSlug={organization.slug} - projectSlug={project.slug} - environmentSlug={environment.slug} - openGitHubRepoConnectionModal={openGitHubRepoConnectionModal} - /> - )} - </div> - </div> - - <div> - <Header2 spacing>Build settings</Header2> - <div className="w-full rounded-sm border border-grid-dimmed p-4"> - <BuildSettingsForm buildSettings={buildSettings ?? {}} /> - </div> - </div> - </Fragment> - )} + {githubAppEnabled && ( + <div> + <Header2 spacing>Git settings</Header2> + <div className="w-full rounded-sm border border-grid-dimmed p-4"> + {connectedGithubRepository ? ( + <ConnectedGitHubRepoForm connectedGitHubRepo={connectedGithubRepository} /> + ) : ( + <GitHubConnectionPrompt + gitHubAppInstallations={githubAppInstallations ?? []} + organizationSlug={organization.slug} + projectSlug={project.slug} + environmentSlug={environment.slug} + openGitHubRepoConnectionModal={openGitHubRepoConnectionModal} + /> + )} + </div> + </div> + )} + + <div> + <Header2 spacing>Build settings</Header2> + <div className="w-full rounded-sm border border-grid-dimmed p-4"> + <BuildSettingsForm buildSettings={buildSettings ?? {}} /> + </div> + </div>
🧹 Nitpick comments (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (3)
65-65: Drop default React import; use Fragment shorthand.Slight cleanup; no behavior change.
-import React, { useEffect, useState } from "react"; +import { Fragment, useEffect, useState } from "react";- <React.Fragment> + <Fragment> ... - </React.Fragment> + </Fragment>Also applies to: 574-575, 599-599
190-191: Avoid identifier shadowing between value and type.Rename the inferred type to reduce confusion.
-type UpdateBuildSettingsFormSchema = z.infer<typeof UpdateBuildSettingsFormSchema>; +type UpdateBuildSettingsFormValues = z.infer<typeof UpdateBuildSettingsFormSchema>;
1107-1217: Polish the form UX: maxLength, spellCheck, and clearer placeholders.Keeps client-side consistent with schema and avoids autocorrect surprises.
- <Input + <Input {...conform.input(fields.triggerConfigFilePath, { type: "text" })} defaultValue={buildSettings?.triggerConfigFilePath || ""} placeholder="trigger.config.ts" + spellCheck={false} + maxLength={255} onChange={(e) => { ... - <Input + <Input {...conform.input(fields.installCommand, { type: "text" })} defaultValue={buildSettings?.installCommand || ""} - placeholder="e.g., `npm install`, or `bun install`" + placeholder="e.g., npm install or bun install" + spellCheck={false} + maxLength={500} onChange={(e) => { ... - <Input + <Input {...conform.input(fields.installDirectory, { type: "text" })} defaultValue={buildSettings?.installDirectory || ""} - placeholder="" + placeholder="e.g., . or apps/webapp" + spellCheck={false} + maxLength={255} onChange={(e) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx(10 hunks)apps/webapp/app/services/projectSettings.server.ts(2 hunks)apps/webapp/app/services/projectSettingsPresenter.server.ts(5 hunks)apps/webapp/app/v3/buildSettings.ts(1 hunks)internal-packages/database/prisma/migrations/20250915141201_add_build_settings_to_project/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/webapp/app/services/projectSettings.server.ts
- internal-packages/database/prisma/schema.prisma
- internal-packages/database/prisma/migrations/20250915141201_add_build_settings_to_project/migration.sql
- apps/webapp/app/v3/buildSettings.ts
- apps/webapp/app/services/projectSettingsPresenter.server.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to trigger.config.ts : Declare build options and extensions (external, jsx, conditions, extensions) via the build block in trigger.config.ts rather than custom scripts
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use schemaTask({ schema, run, ... }) to validate payloads when input validation is required
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (7)
apps/webapp/app/models/message.server.ts (2)
redirectBackWithErrorMessage(200-207)redirectBackWithSuccessMessage(209-216)apps/webapp/app/v3/buildSettings.ts (1)
BuildSettings(9-9)apps/webapp/app/components/primitives/Label.tsx (1)
Label(25-41)apps/webapp/app/components/primitives/Hint.tsx (1)
Hint(3-9)apps/webapp/app/components/primitives/FormButtons.tsx (1)
FormButtons(3-22)apps/webapp/app/components/primitives/Buttons.tsx (1)
Button(296-331)apps/webapp/app/components/primitives/Spinner.tsx (1)
SpinnerWhite(76-78)
⏰ 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: units / webapp / 📊 Merge Reports
🔇 Additional comments (6)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (6)
80-81: Good: use shared BuildSettings type.
124-139: Loader plumbs buildSettings to client — LGTM.Keep presenter output JSON-safe. Strings-only here are fine.
225-225: Good: union includes the new action.
414-438: Server action for update-build-settings — LGTM.Blank-to-undefined coercion on write path is correct.
452-453: Client consumes buildSettings — LGTM.
1165-1198: Align naming: code uses installDirectory; PR uses "root directory"BuildSettingsSchema defines installDirectory (apps/webapp/app/v3/buildSettings.ts) and projectSettings.server.ts exposes updateBuildSettings; UI shows "Install directory" in apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (lines 1165–1198). Update the PR description to "install directory" or rename the schema/UI to "rootDirectory" if that is the intended product term.
Likely an incorrect or invalid review comment.
2 participants
This PR enables setting project build settings in the settings page:
root directory, install command and trigger config file path.
For most cases there should be no need to set these explicitly.