Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ephemeral envs init #187

Merged
merged 8 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/jobs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
"@ctrlplane/logger": "workspace:*",
"@ctrlplane/validators": "workspace:*",
"cron": "^3.1.7",
"lodash": "^4.17.21",
"ts-is-present": "^1.2.2",
"zod": "catalog:"
},
"devDependencies": {
Expand Down
60 changes: 60 additions & 0 deletions apps/jobs/src/ephemeral-env-checker/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import _ from "lodash";
import { isPresent } from "ts-is-present";

import { and, eq, inArray, isNotNull, lte } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import * as SCHEMA from "@ctrlplane/db/schema";

type QueryRow = {
environment: SCHEMA.Environment;
deployment: SCHEMA.Deployment;
};

const groupByEnvironment = (rows: QueryRow[]) =>
_.chain(rows)
.groupBy((e) => e.environment.id)
.map((env) => ({
...env[0]!.environment,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using non-null assertion to prevent runtime errors.

The use of the non-null assertion operator ! in env[0]! can lead to runtime exceptions if env is an empty array. Consider adding a check or using optional chaining to handle the possibility of env being empty.

Apply this diff to safely access env[0]:

-        ...env[0]!.environment,
+        ...(env[0]?.environment ?? {}),
📝 Committable suggestion

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

Suggested change
...env[0]!.environment,
...(env[0]?.environment ?? {}),

deployments: env.map((e) => e.deployment),
}))
.value();

export const run = async () => {
const ephemeralEnvironments = await db
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const ephemeralEnvironments = await db
const expiredEnvironments = await db

.select()
.from(SCHEMA.environment)
.innerJoin(
SCHEMA.deployment,
eq(SCHEMA.deployment.systemId, SCHEMA.environment.systemId),
)
.where(
and(
isNotNull(SCHEMA.environment.expiresAt),
lte(SCHEMA.environment.expiresAt, new Date()),
Copy link
Member

Choose a reason for hiding this comment

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

do you need the null check?

),
)
.then(groupByEnvironment);
if (ephemeralEnvironments.length === 0) return;

const targetPromises = ephemeralEnvironments
.filter((env) => isPresent(env.targetFilter))
.map(async (env) => {
const targets = await db
.select()
.from(SCHEMA.target)
.where(SCHEMA.targetMatchesMetadata(db, env.targetFilter));

return { environmentId: env.id, targets };
});
const associatedTargets = await Promise.all(targetPromises);

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize target fetching by batching database queries.

Fetching targets for each ephemeral environment individually may lead to performance issues. Consider batching the queries or modifying the database query to fetch all required targets in a single operation to improve performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we cannot batch the queries due to the limitation of env.targetFilter, this needs to be extracted from the database first before it can be used

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityachoudhari26, understood. Thank you for the clarification regarding env.targetFilter. Since the filter needs to be extracted from the database first, batching the queries is not feasible in this case.

🐇🤔


✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#187
File: apps/jobs/src/ephemeral-env-checker/index.ts:39-0
Timestamp: 2024-10-30T23:09:31.909Z
Learning: Due to the limitations of `env.targetFilter`, batching queries is not feasible because the filter needs to be extracted from the database first before it can be used.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

for (const { environmentId, targets } of associatedTargets)
console.log(
`[${targets.length}] targets are associated with ephemeral environment [${environmentId}]`,
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use a structured logging mechanism instead of console.log.

Using console.log for logging is not ideal in production environments. Consider utilizing a structured logging library to handle log levels and formats more effectively.

Copy link
Member

Choose a reason for hiding this comment

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

@adityachoudhari26 you can use the logger lib

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

);

const envIds = ephemeralEnvironments.map((env) => env.id);
await db
.delete(SCHEMA.environment)
.where(inArray(SCHEMA.environment.id, envIds));
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure cascading deletes for associated deployments.

When deleting ephemeral environments, the associated deployments may remain in the database, leading to orphaned records. Consider deleting associated deployments or configuring the database to cascade deletes to maintain data integrity.

Would you like assistance in updating the code to handle deletion of associated deployments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want to keep the deployments, they are decoupled from the environments

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityachoudhari26 Understood, thank you for the clarification that deployments are decoupled from environments and should be retained even when environments are deleted.

(•◡•)?


✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#187
File: apps/jobs/src/ephemeral-env-checker/index.ts:57-0
Timestamp: 2024-10-30T23:10:58.576Z
Learning: In the codebase, deployments are decoupled from environments. When deleting environments (e.g., in `apps/jobs/src/ephemeral-env-checker/index.ts`), associated deployments should not be deleted.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for asynchronous operations.

The run function performs several asynchronous database operations without error handling. To ensure reliability, consider wrapping database calls in try-catch blocks or handling promise rejections to manage potential errors gracefully.

5 changes: 5 additions & 0 deletions apps/jobs/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,18 @@ import { z } from "zod";

import { logger } from "@ctrlplane/logger";

import { run as ephemeralEnvChecker } from "./ephemeral-env-checker/index.js";
import { run as jobPolicyChecker } from "./policy-checker/index.js";

const jobs: Record<string, { run: () => Promise<void>; schedule: string }> = {
"policy-checker": {
run: jobPolicyChecker,
schedule: "* * * * *", // Default: Every minute
},
"ephemeral-env-checker": {
run: ephemeralEnvChecker,
schedule: "* * * * *", // Default: Every minute
},
};

const jobSchema = z.object({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export const EnvironmentDrawer: React.FC = () => {
const environmentQ = api.environment.byId.useQuery(environmentId ?? "", {
enabled: isOpen,
});
const environmentQError = environmentQ.error;
const environment = environmentQ.data;

const { workspaceSlug } = useParams<{ workspaceSlug: string }>();
Expand All @@ -75,17 +76,24 @@ export const EnvironmentDrawer: React.FC = () => {
showBar={false}
className="left-auto right-0 top-0 mt-0 h-screen w-2/3 max-w-6xl overflow-auto rounded-none focus-visible:outline-none"
>
<DrawerTitle className="flex items-center gap-2 border-b p-6">
<div className="flex-shrink-0 rounded bg-green-500/20 p-1 text-green-400">
<IconPlant className="h-4 w-4" />
<DrawerTitle className="flex flex-col gap-2 border-b p-6">
<div className="flex items-center gap-2">
<div className="flex-shrink-0 rounded bg-green-500/20 p-1 text-green-400">
<IconPlant className="h-4 w-4" />
</div>
{environment?.name}
{environment != null && (
<EnvironmentDropdownMenu environment={environment}>
<Button variant="ghost" size="icon" className="h-6 w-6">
<IconDotsVertical className="h-4 w-4" />
</Button>
</EnvironmentDropdownMenu>
)}
</div>
{environment?.name}
{environment != null && (
<EnvironmentDropdownMenu environment={environment}>
<Button variant="ghost" size="icon" className="h-6 w-6">
<IconDotsVertical className="h-4 w-4" />
</Button>
</EnvironmentDropdownMenu>
{environmentQError != null && (
<div className="text-xs text-red-500">
{environmentQError.message}
</div>
)}
</DrawerTitle>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import type * as SCHEMA from "@ctrlplane/db/schema";
import { addDays, addHours, addMinutes, format } from "date-fns";
import { z } from "zod";

import { Button } from "@ctrlplane/ui/button";
import { Checkbox } from "@ctrlplane/ui/checkbox";
import {
Form,
FormControl,
Expand All @@ -11,34 +13,94 @@ import {
useForm,
} from "@ctrlplane/ui/form";
import { Input } from "@ctrlplane/ui/input";
import { Label } from "@ctrlplane/ui/label";
import {
Select,
SelectContent,
SelectItem,
SelectTrigger,
SelectValue,
} from "@ctrlplane/ui/select";
import { Textarea } from "@ctrlplane/ui/textarea";

import { api } from "~/trpc/react";

const schema = z.object({
name: z.string().min(1).max(100),
description: z.string().max(1000).nullable(),
durationNumber: z.number().min(0).nullable(),
durationUnit: z.enum(["minutes", "hours", "days"]).nullable(),
removeExpiration: z.boolean(),
});

type OverviewProps = {
environment: SCHEMA.Environment;
};

const getExpiresAt = (
expiresAt: Date | null,
durationNumber: number,
durationUnit: "minutes" | "hours" | "days",
) => {
const currExpiresAt = expiresAt ?? new Date();
if (durationUnit === "minutes")
return addMinutes(currExpiresAt, durationNumber);
if (durationUnit === "hours") return addHours(currExpiresAt, durationNumber);
return addDays(currExpiresAt, durationNumber);
Copy link
Member

@jsbroks jsbroks Oct 30, 2024

Choose a reason for hiding this comment

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

Suggested change
const currExpiresAt = expiresAt ?? new Date();
if (durationUnit === "minutes")
return addMinutes(currExpiresAt, durationNumber);
if (durationUnit === "hours") return addHours(currExpiresAt, durationNumber);
return addDays(currExpiresAt, durationNumber);
const currExpiresAt = expiresAt ?? new Date();
return add(currExpiresAt, { [durationUnit]: durationNumber });

};

export const Overview: React.FC<OverviewProps> = ({ environment }) => {
const form = useForm({ schema, defaultValues: environment });
const defaultValues = {
...environment,
durationNumber: null,
durationUnit: "hours" as const,
removeExpiration: false,
};
const form = useForm({ schema, defaultValues });
const update = api.environment.update.useMutation();
const envOverride = api.job.trigger.create.byEnvId.useMutation();

const utils = api.useUtils();

const { id, systemId } = environment;
const onSubmit = form.handleSubmit((data) =>
const onSubmit = form.handleSubmit((data) => {
const { durationNumber, durationUnit, removeExpiration } = data;
const expiresAt = removeExpiration
? null
: durationNumber != null && durationUnit != null
? getExpiresAt(environment.expiresAt, durationNumber, durationUnit)
: environment.expiresAt;

const envData = { ...data, expiresAt };

const resetValues = {
...data,
durationNumber: null,
removeExpiration: false,
};
update
.mutateAsync({ id, data })
.then(() => form.reset(data))
.mutateAsync({ id, data: envData })
.then(() => form.reset(resetValues))
.then(() => utils.environment.bySystemId.invalidate(systemId))
.then(() => utils.environment.byId.invalidate(id)),
);
.then(() => utils.environment.byId.invalidate(id));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider extracting expiration calculation logic.

The form submission logic handles multiple concerns. Consider extracting the expiration calculation into a separate function for better maintainability.

+const calculateNewExpiration = (
+  currentExpiresAt: Date | null,
+  formData: {
+    durationNumber: number | null;
+    durationUnit: "minutes" | "hours" | "days" | null;
+    removeExpiration: boolean;
+  }
+) => {
+  const { durationNumber, durationUnit, removeExpiration } = formData;
+  return removeExpiration
+    ? null
+    : durationNumber != null && durationUnit != null
+      ? getExpiresAt(currentExpiresAt, durationNumber, durationUnit)
+      : currentExpiresAt;
+};

 const onSubmit = form.handleSubmit((data) => {
-  const { durationNumber, durationUnit, removeExpiration } = data;
-  const expiresAt = removeExpiration
-    ? null
-    : durationNumber != null && durationUnit != null
-      ? getExpiresAt(environment.expiresAt, durationNumber, durationUnit)
-      : environment.expiresAt;
+  const expiresAt = calculateNewExpiration(environment.expiresAt, data);

   const envData = { ...data, expiresAt };
📝 Committable suggestion

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

Suggested change
const defaultValues = {
...environment,
durationNumber: null,
durationUnit: "hours" as const,
removeExpiration: false,
};
const form = useForm({ schema, defaultValues });
const update = api.environment.update.useMutation();
const envOverride = api.job.trigger.create.byEnvId.useMutation();
const utils = api.useUtils();
const { id, systemId } = environment;
const onSubmit = form.handleSubmit((data) =>
const onSubmit = form.handleSubmit((data) => {
const { durationNumber, durationUnit, removeExpiration } = data;
const expiresAt = removeExpiration
? null
: durationNumber != null && durationUnit != null
? getExpiresAt(environment.expiresAt, durationNumber, durationUnit)
: environment.expiresAt;
const envData = { ...data, expiresAt };
const resetValues = {
...data,
durationNumber: null,
removeExpiration: false,
};
update
.mutateAsync({ id, data })
.then(() => form.reset(data))
.mutateAsync({ id, data: envData })
.then(() => form.reset(resetValues))
.then(() => utils.environment.bySystemId.invalidate(systemId))
.then(() => utils.environment.byId.invalidate(id)),
);
.then(() => utils.environment.byId.invalidate(id));
});
const defaultValues = {
...environment,
durationNumber: null,
durationUnit: "hours" as const,
removeExpiration: false,
};
const form = useForm({ schema, defaultValues });
const update = api.environment.update.useMutation();
const envOverride = api.job.trigger.create.byEnvId.useMutation();
const utils = api.useUtils();
const calculateNewExpiration = (
currentExpiresAt: Date | null,
formData: {
durationNumber: number | null;
durationUnit: "minutes" | "hours" | "days" | null;
removeExpiration: boolean;
}
) => {
const { durationNumber, durationUnit, removeExpiration } = formData;
return removeExpiration
? null
: durationNumber != null && durationUnit != null
? getExpiresAt(currentExpiresAt, durationNumber, durationUnit)
: currentExpiresAt;
};
const { id, systemId } = environment;
const onSubmit = form.handleSubmit((data) => {
const expiresAt = calculateNewExpiration(environment.expiresAt, data);
const envData = { ...data, expiresAt };
const resetValues = {
...data,
durationNumber: null,
removeExpiration: false,
};
update
.mutateAsync({ id, data: envData })
.then(() => form.reset(resetValues))
.then(() => utils.environment.bySystemId.invalidate(systemId))
.then(() => utils.environment.byId.invalidate(id));
});


const currExpiresAt = environment.expiresAt;
const { durationNumber, durationUnit, removeExpiration } = form.watch();

const currentExpiration =
currExpiresAt != null
? format(currExpiresAt, "MMM d, yyyy h:mm a")
: "never";

const newExpiration = removeExpiration
? "never"
: durationNumber != null && durationUnit != null
? format(
getExpiresAt(currExpiresAt, durationNumber, durationUnit),
"MMM d, yyyy h:mm a",
)
: null;

return (
<Form {...form}>
Expand Down Expand Up @@ -72,6 +134,89 @@ export const Overview: React.FC<OverviewProps> = ({ environment }) => {
)}
/>

<div className="space-y-4">
<Label>Environment expiration</Label>
<div className="flex flex-col gap-1 text-sm text-muted-foreground">
<span>Current expiration: {currentExpiration}</span>
<span>New expiration: {newExpiration ?? "No change"}</span>
</div>

<div className="flex items-center gap-2 text-sm">
{currExpiresAt == null && <span>Environment expires in: </span>}
{currExpiresAt != null && <span>Extend expiration by: </span>}
<FormField
control={form.control}
name="durationNumber"
render={({ field: { value, onChange } }) => (
<FormItem className="w-16">
<FormControl>
<Input
type="number"
value={value ?? ""}
className="appearance-none [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none"
onChange={(e) => {
const num = e.target.valueAsNumber;
if (Number.isNaN(num)) {
onChange(null);
return;
}
onChange(num);
}}
/>
</FormControl>
</FormItem>
)}
/>
<FormField
control={form.control}
name="durationUnit"
render={({ field: { value, onChange } }) => (
<FormItem className="w-24">
<FormControl>
<Select
value={value ?? undefined}
onValueChange={onChange}
defaultValue="hours"
>
<SelectTrigger>
<SelectValue placeholder="Select duration unit..." />
</SelectTrigger>
<SelectContent>
<SelectItem value="minutes">Minutes</SelectItem>
<SelectItem value="hours">Hours</SelectItem>
<SelectItem value="days">Days</SelectItem>
</SelectContent>
</Select>
</FormControl>
</FormItem>
)}
/>
</div>

<FormField
control={form.control}
name="removeExpiration"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormControl>
<div className="flex items-center gap-2">
<Checkbox
checked={value}
onCheckedChange={(v) => {
onChange(v);
if (v) form.setValue("durationNumber", null);
}}
/>
<label htmlFor="removeExpiration" className="text-sm">
Remove expiration
</label>
</div>
</FormControl>
</FormItem>
)}
/>
</div>

<div className="flex items-center gap-2">
<Button
type="submit"
Expand Down
27 changes: 18 additions & 9 deletions apps/webservice/src/app/api/v1/environments/route.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
import type { PermissionChecker } from "@ctrlplane/auth/utils";
import type { User } from "@ctrlplane/db/schema";
import type { z } from "zod";
import { NextResponse } from "next/server";
import { isPresent } from "ts-is-present";
import { z } from "zod";

import { takeFirst } from "@ctrlplane/db";
import * as schema from "@ctrlplane/db/schema";
import { Permission } from "@ctrlplane/validators/auth";

import { authn, authz } from "../auth";
import { parseBody } from "../body-parser";
import { request } from "../middleware";

const body = schema.createEnvironment;
const body = schema.createEnvironment.extend({
expiresAt: z.coerce.date().optional(),
});
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add date validation constraints

While the date coercion works, consider adding validation to ensure:

  1. The expiration date is in the future
  2. There's a reasonable maximum duration (e.g., 1 year)

Consider updating the schema like this:

 const body = schema.createEnvironment.extend({
-  expiresAt: z.coerce.date().optional(),
+  expiresAt: z.coerce
+    .date()
+    .min(new Date(), { message: "Expiration date must be in the future" })
+    .max(new Date(Date.now() + 365 * 24 * 60 * 60 * 1000), {
+      message: "Expiration date cannot be more than 1 year in the future",
+    })
+    .optional(),
 });
📝 Committable suggestion

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

Suggested change
const body = schema.createEnvironment.extend({
expiresAt: z.coerce.date().optional(),
});
const body = schema.createEnvironment.extend({
expiresAt: z.coerce
.date()
.min(new Date(), { message: "Expiration date must be in the future" })
.max(new Date(Date.now() + 365 * 24 * 60 * 60 * 1000), {
message: "Expiration date cannot be more than 1 year in the future",
})
.optional(),
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the suggestion for the min, but since you can have null epxiresAt which mens the environment lives forever, I don't think we really need a limit for the max

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityachoudhari26, understood. Since expiresAt can be null, representing environments that live forever, a maximum date constraint isn't necessary.

Here's the updated schema with only the min constraint:

const body = schema.createEnvironment.extend({
  expiresAt: z.coerce
    .date()
    .min(new Date(), { message: "Expiration date must be in the future" })
    .optional(),
});

(_/)
( •_•)


✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#187
File: apps/webservice/src/app/api/v1/environments/route.ts:15-17
Timestamp: 2024-10-30T23:07:27.355Z
Learning: When validating `expiresAt` for environments, since `expiresAt` can be `null` (representing environments that live indefinitely), there's no need to enforce a maximum expiration date.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


export const POST = request()
.use(authn)
Expand All @@ -23,12 +27,17 @@ export const POST = request()
),
)
.handle<{ user: User; can: PermissionChecker; body: z.infer<typeof body> }>(
async (ctx) => {
const environment = await ctx.db
async (ctx) =>
ctx.db
.insert(schema.environment)
.values(ctx.body)
.returning();

return NextResponse.json({ environment });
},
.values({
...ctx.body,
expiresAt: isPresent(ctx.body.expiresAt)
? new Date(ctx.body.expiresAt)
: undefined,
})
.returning()
.then(takeFirst)
.then((environment) => NextResponse.json({ environment }))
.catch((error) => NextResponse.json({ error }, { status: 500 })),
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling

The current error handling exposes raw error details to the client, which could be a security concern. Consider implementing structured error responses.

-        .catch((error) => NextResponse.json({ error }, { status: 500 })),
+        .catch((error) => {
+          console.error('Failed to create environment:', error);
+          return NextResponse.json(
+            { message: 'Failed to create environment' },
+            { status: 500 }
+          );
+        }),
📝 Committable suggestion

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

Suggested change
.then((environment) => NextResponse.json({ environment }))
.catch((error) => NextResponse.json({ error }, { status: 500 })),
.then((environment) => NextResponse.json({ environment }))
.catch((error) => {
console.error('Failed to create environment:', error);
return NextResponse.json(
{ message: 'Failed to create environment' },
{ status: 500 }
);
}),

);
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { NextRequest } from "next/server";
import { NextResponse } from "next/server";

import { and, eq, isNull, notInArray } from "@ctrlplane/db";
import { and, eq, notInArray } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import {
deployment,
Expand Down Expand Up @@ -40,7 +40,6 @@ export const GET = async (
"completed",
"invalid_job_agent",
]),
isNull(environment.deletedAt),
),
)
.then((rows) =>
Expand Down
Loading