List API for resources, systems, deploys #379
Conversation
WalkthroughThis pull request includes a variety of updates across the codebase. It adds a new entry to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Auth
participant DB
User->>API: GET /v1/workspaces/:workspaceId/deployments
API->>Auth: Run authentication & authorization
Auth-->>API: Return auth result
API->>DB: Execute deployment query (similarly for environments, resources, systems)
DB-->>API: Return queried data
API-->>User: 200 OK with JSON data
sequenceDiagram
participant App
participant EmailClient
participant Transporter
App->>EmailClient: Call sendEmail(payload)
EmailClient->>Transporter: transporter.sendMail({...})
Transporter-->>EmailClient: Return sending result
EmailClient-->>App: Provide confirmation response
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
openapi.v1.json (1)
2294-2811: 🛠️ Refactor suggestionFix path parameter format inconsistency in OpenAPI spec
The new endpoints use a colon prefix for path parameters (
:workspaceId) while existing endpoints in the spec use curly braces ({workspaceId}). This inconsistency could cause issues with API clients and documentation tools.Update the path format to use curly braces for consistency:
- "/v1/workspaces/:workspaceId/deployments": { + "/v1/workspaces/{workspaceId}/deployments": {- "/v1/workspaces/:workspaceId/environments": { + "/v1/workspaces/{workspaceId}/environments": {- "/v1/workspaces/:workspaceId/resources": { + "/v1/workspaces/{workspaceId}/resources": {- "/v1/workspaces/:workspaceId/systems": { + "/v1/workspaces/{workspaceId}/systems": {
🧹 Nitpick comments (23)
packages/emails/src/Welcome.jsx (1)
5-7: Replace placeholder URL with configurable URLThe "https://example.com" URL is a placeholder that should be replaced with a configurable URL. Consider passing the URL as a prop to make the component reusable.
-export const MyEmail = () => { +export const MyEmail = ({ actionUrl = "https://example.com", buttonText = "Click me" }) => { return (<Html> - <Button href="https://example.com" style={{ background: "#000", color: "#fff", padding: "12px 20px" }}> - Click me + <Button href={actionUrl} style={{ background: "#000", color: "#fff", padding: "12px 20px" }}> + {buttonText} </Button> </Html>); };packages/emails/src/client/index.js (1)
1-14: Missing connection with React email componentsThe current implementation doesn't show how to use the React email components (like MyEmail) with the sendEmail function. Consider adding a helper function to render React components to HTML.
import nodemailer from "nodemailer"; +import { render } from "@react-email/render"; import { env } from "./env"; export const transporter = nodemailer.createTransport({ host: env.SMTP_HOST, port: env.SMTP_PORT, secure: env.SMTP_SECURE, auth: { user: env.SMTP_USER, pass: env.SMTP_PASS, }, }); export const sendEmail = (payload) => { return transporter.sendMail({ from: env.SMTP_FROM, ...payload }); }; + +export const sendEmailTemplate = async (component, options) => { + const html = render(component); + return sendEmail({ + html, + ...options, + }); +};packages/emails/src/client/env.js (2)
4-11: Consider making SMTP_USER and SMTP_PASS optionalSome SMTP servers don't require authentication. Consider making SMTP_USER and SMTP_PASS optional to support these scenarios.
export const env = createEnv({ server: { SMTP_HOST: z.string(), SMTP_PORT: z.coerce.number().default(587), - SMTP_USER: z.string(), - SMTP_PASS: z.string(), + SMTP_USER: z.string().optional(), + SMTP_PASS: z.string().optional(), SMTP_FROM: z.string(), SMTP_SECURE: z.coerce.boolean().default(false), },
12-15: Add environment variable documentationAdd comments explaining the purpose of each environment variable and how to configure them, especially for new developers.
export const env = createEnv({ server: { + // SMTP server hostname (e.g., smtp.gmail.com) SMTP_HOST: z.string(), + // SMTP server port (defaults to 587) SMTP_PORT: z.coerce.number().default(587), + // SMTP username for authentication (if required) SMTP_USER: z.string(), + // SMTP password for authentication (if required) SMTP_PASS: z.string(), + // Default sender email address SMTP_FROM: z.string(), + // Whether to use TLS (defaults to false) SMTP_SECURE: z.coerce.boolean().default(false), }, runtimeEnv: process.env,apps/webservice/src/app/api/v1/environments/route.ts (1)
4-4: Unused import detectedThe
httpStatusmodule is imported but not used in this file. Consider either:
- Using the imported module to replace hardcoded status codes (e.g., replace
{ status: 409 }with{ status: httpStatus.CONFLICT })- Removing the unused import
Using named constants from http-status would improve code readability and maintainability.
apps/webservice/src/app/api/v1/deployments/route.ts (2)
4-4: Unused import detectedThe
zlibrary from zod is imported but not used in the current implementation. Consider removing it if not needed, or document if it's intended for future use.
16-16: New logger not used in error handlingThe new contextual logger is defined but not used in the error handling on line 55, which still uses the global logger.
For consistency, update the error handling to use the new contextual logger:
- logger.error("Failed to create deployment", { error }); + log.error("Failed to create deployment", { error });apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/openapi.ts (2)
1-46: Missing schema component referenceThe OpenAPI specification references
#/components/schemas/Environmentbut doesn't define this schema or specify where it's defined. This could cause issues with OpenAPI validation tools or auto-generated clients.Consider either:
- Including the Environment schema definition in a
componentssection- Using an external reference (e.g.,
$ref: "./schemas.ts#/Environment")- Adding a comment to clarify where this schema is defined
10-10: Path parameter format might not be compatible with all OpenAPI toolsThe path uses colon-prefixed parameters (
:workspaceId) which is common in Express-style routing, but many OpenAPI tools expect curly braces ({workspaceId}).Consider using the more widely supported format:
- "/v1/workspaces/:workspaceId/environments": { + "/v1/workspaces/{workspaceId}/environments": {apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/openapi.ts (3)
1-46: Missing schema component referenceSimilar to the environments OpenAPI file, this specification references
#/components/schemas/Resourcebut doesn't define this schema component.Consider adding the Resource schema definition or clarifying where it's defined, such as:
components: { schemas: { Resource: { type: "object", properties: { // Define properties here } } } }
10-10: Path parameter format might not be compatible with all OpenAPI toolsThe path uses colon-prefixed parameters (
:workspaceId) which is common in Express-style routing, but OpenAPI typically uses curly braces for path parameters.Consider using the standard OpenAPI path parameter format:
- "/v1/workspaces/:workspaceId/resources": { + "/v1/workspaces/{workspaceId}/resources": {
1-46: Consider a consistent approach across all OpenAPI filesThis file has the same issues as other OpenAPI specifications in the codebase. This suggests a need for a consistent approach to OpenAPI documentation.
Consider:
- Creating shared schema components that can be referenced by multiple endpoints
- Setting up OpenAPI validation in CI to catch these issues automatically
- Creating a template or helper function for generating consistent OpenAPI specs
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/openapi.ts (1)
1-43: OpenAPI specification looks good but could use query parameters for filtering and paginationThe OpenAPI specification for the systems endpoint is well-structured and follows the 3.0.0 standard correctly. The path parameter for workspaceId is properly defined with the required attributes.
However, list endpoints commonly support filtering, sorting, and pagination capabilities. Consider enhancing this specification to include query parameters such as:
limitandoffsetfor paginationsortfor specifying sort fields- Relevant filter parameters specific to systems
paths: { "/v1/workspaces/:workspaceId/systems": { parameters: [ { name: "workspaceId", in: "path", required: true, schema: { type: "string", }, description: "The ID of the workspace", }, + { + name: "limit", + in: "query", + required: false, + schema: { + type: "integer", + default: 20 + }, + description: "Maximum number of systems to return" + }, + { + name: "offset", + in: "query", + required: false, + schema: { + type: "integer", + default: 0 + }, + description: "Number of systems to skip" + } ],apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/openapi.ts (1)
1-46: OpenAPI specification is correct but missing pagination and filter parametersThe OpenAPI specification for deployments is well-structured and correctly follows the OpenAPI 3.0.0 standard. The path parameter for workspaceId is properly defined.
Similar to the systems endpoint, consider enhancing this specification to include query parameters for pagination and filtering to make the API more versatile:
paths: { "/v1/workspaces/:workspaceId/deployments": { parameters: [ { name: "workspaceId", in: "path", required: true, schema: { type: "string", }, description: "The ID of the workspace", }, + { + name: "limit", + in: "query", + required: false, + schema: { + type: "integer", + default: 20 + }, + description: "Maximum number of deployments to return" + }, + { + name: "offset", + in: "query", + required: false, + schema: { + type: "integer", + default: 0 + }, + description: "Number of deployments to skip" + }, + { + name: "status", + in: "query", + required: false, + schema: { + type: "string", + enum: ["active", "failed", "pending"] + }, + description: "Filter deployments by status" + } ],apps/webservice/src/app/api/v1/resources/route.ts (4)
4-4: The logger module name doesn't match the file contextYou've added logging capability, but the logger module name is set to "api/v1/deployments" when this file is handling resources. This inconsistency could lead to confusion when diagnosing issues in logs.
-const log = logger.child({ module: "api/v1/deployments" }); +const log = logger.child({ module: "api/v1/resources" });Also applies to: 7-7, 14-16
16-16: Unused logger variableThe
logvariable is defined but not used anywhere in the file. Either use it for logging key operations or remove it.-const log = logger.child({ module: "api/v1/deployments" });Alternatively, add meaningful log entries at key points in the request handling:
async (ctx) => { + log.info(`Processing upsert for ${ctx.body.resources.length} resources in workspace ${ctx.body.workspaceId}`); if (ctx.body.resources.length === 0)
7-7: Import formatting is inconsistentThe import statement for createResource and resource has inconsistent spacing. Follow a consistent import style throughout the codebase.
-import {createResource, resource} from "@ctrlplane/db/schema"; +import { createResource, resource } from "@ctrlplane/db/schema";
7-7: Unused import: 'resource'The 'resource' import is added but not used anywhere in this file.
-import {createResource, resource} from "@ctrlplane/db/schema"; +import { createResource } from "@ctrlplane/db/schema";apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/route.ts (5)
37-38: Remove commented-out console.dirThere's a commented-out console.dir statement in the error handling block. This should be removed or replaced with proper logging.
} catch (error) { - //console.dir(error); + // Use proper logging instead + console.error('Error fetching deployments:', error);Better yet, use the logger pattern established in the resources route:
} catch (error) { - //console.dir(error); + const log = logger.child({ module: "api/v1/deployments" }); + log.error({ err: error }, "Error fetching deployments");
22-24: Type the return value for better type safetyThe return type of the handle function isn't fully typed. Consider adding explicit return type annotation for better type checking and documentation.
- .handle<unknown, Promise<ListRequestParams>>( + .handle<unknown, Promise<ListRequestParams>, NextResponse>(
11-11: Improve ListRequestParams type definitionThe type definition for
ListRequestParamscould be more descriptive to indicate that it represents route parameters. Consider renaming it to be more specific.-type ListRequestParams = {params: {workspaceId: string}}; +type WorkspaceRouteParams = {params: {workspaceId: string}};And update its usage:
-.handle<unknown, Promise<ListRequestParams>>( +.handle<unknown, Promise<WorkspaceRouteParams>>(
16-20: Verify workspaceId exists before proceedingThe current implementation assumes the workspaceId is valid, but doesn't verify if it actually corresponds to an existing workspace. Consider adding a check to ensure the workspace exists.
async (ctx, extra) => { try { const { workspaceId } = (await extra).params; + + // Verify that the workspace exists + const workspaceExists = await ctx.db + .select({ count: count() }) + .from(workspace) + .where(eq(workspace.id, workspaceId)) + .then(result => result[0].count > 0); + + if (!workspaceExists) { + return NextResponse.json( + { error: "Workspace not found" }, + { status: httpStatus.NOT_FOUND } + ); + } + const deployments = await ctx.db
39-45: Consider using a structured error handling approachThe error handling approach is good but could be improved by using a more structured approach for consistent error handling across endpoints.
Consider creating a utility function for error handling:
+ // In a shared utilities file + export function handleApiError(error: unknown): NextResponse { + const message = error instanceof Error ? error.message : "Unknown error"; + const status = isHttpStatusError(error) ? error.statusCode : httpStatus.INTERNAL_SERVER_ERROR; + + return NextResponse.json( + { + error: httpStatus[status], + message + }, + { status } + ); + } // Then in your route handler: } catch (error) { - return NextResponse.json( - { - error: "Internal Server Error", - message: error instanceof Error ? error.message : "Unknown error", - }, - { status: httpStatus.INTERNAL_SERVER_ERROR }, - ); + return handleApiError(error); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitignore(1 hunks)README.md(1 hunks)apps/webservice/src/app/api/v1/deployments/route.ts(2 hunks)apps/webservice/src/app/api/v1/environments/route.ts(1 hunks)apps/webservice/src/app/api/v1/middleware.ts(1 hunks)apps/webservice/src/app/api/v1/resources/route.ts(1 hunks)apps/webservice/src/app/api/v1/systems/route.ts(2 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/route.ts(1 hunks)openapi.v1.json(2 hunks)packages/emails/src/Welcome.jsx(1 hunks)packages/emails/src/client/env.js(1 hunks)packages/emails/src/client/index.js(1 hunks)packages/emails/tailwind.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/middleware.tsapps/webservice/src/app/api/v1/resources/route.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/route.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/openapi.tsapps/webservice/src/app/api/v1/deployments/route.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/route.tsapps/webservice/src/app/api/v1/systems/route.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/openapi.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/openapi.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/route.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/openapi.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.tsapps/webservice/src/app/api/v1/environments/route.ts
🔇 Additional comments (9)
packages/emails/tailwind.config.js (1)
7-17:❓ Verification inconclusive
Verify sidebar colors relevance for emails
The configuration extends theme with sidebar-related colors, which seems unusual for an email package since emails typically don't have sidebars. Consider if these color definitions are necessary or if they were mistakenly copied from another configuration.
🏁 Script executed:
#!/bin/bash # Search for sidebar class usage in email components echo "Checking for sidebar class usage in email components:" grep -r "sidebar" --include="*.jsx" --include="*.tsx" ./packages/emails/src/Length of output: 193
Review Comment: Verify and Possibly Remove Unused Sidebar Colors
Based on the grep search for "sidebar" in our email package source files, there were no matches indicating any usage of sidebar-related classes. Since email templates typically don’t utilize sidebars, these custom color definitions in the Tailwind configuration seem unnecessary. Please manually verify that these colors aren’t being referenced indirectly or through shared theme mechanisms before removing them from
packages/emails/tailwind.config.js(lines 7–17). If they’re confirmed unused, cleaning them up would simplify the configuration..gitignore (1)
58-60: LGTM - Good addition to ignore IDE-specific filesAdding the JetBrains IDE configuration folder to .gitignore is a good practice to keep the repository clean of developer-specific settings.
apps/webservice/src/app/api/v1/systems/route.ts (1)
14-14: Improved logging context - LGTMUpdating the logger module name to include the workspace context provides better traceability in logs. This change aligns with the new API structure that includes workspace-scoped endpoints.
README.md (1)
83-85: Improved development setup instructionsThe updated setup instructions reflect important changes to the development workflow:
- Using a dedicated development Docker Compose file (
docker-compose.dev.yaml) which is good practice for separating development and production configurations- More explicit database migration process that correctly navigates to the db package directory
These changes will help developers set up their environment correctly.
apps/webservice/src/app/api/v1/middleware.ts (1)
15-15: Inconsistent type constraint between Handler and handle methodWhile you've updated the Handler type to accept
object|Promise<object>, thehandlemethod on line 35 still only acceptsExtra extends object = object. This creates a potential type mismatch when handlers expect Promise but the implementation only accepts plain objects.Consider updating the handle method signature to match:
- const handle = - <TContext = object, Extra extends object = object>( - handler: Handler<TContext & BaseContext, Extra>, - ) + const handle = + <TContext = object, Extra extends object|Promise<object> = object>( + handler: Handler<TContext & BaseContext, Extra>, + )Or verify if the current implementation correctly handles Promise objects through testing.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/route.ts (1)
25-35: SQL query misses potential error caseThe SQL query joins the deployment and system tables, but doesn't handle the case where a deployment might exist without a valid systemId. Consider whether this is a valid scenario in your data model.
Does the deployment table enforce a foreign key constraint on systemId? If not, you might want to add a check to ensure valid results:
const deployments = await ctx.db .select() .from(deployment) .leftJoin(system, eq(system.id, deployment.systemId)) - .where(eq(system.workspaceId, workspaceId)) + .where( + and( + eq(system.workspaceId, workspaceId), + isNotNull(deployment.systemId) + ) + ) .orderBy(deployment.slug);apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.ts (1)
1-49: Good implementation of the environments list endpointThe implementation correctly:
- Uses authentication and authorization middleware
- Extracts the workspaceId from request parameters
- Queries environments with a left join to the system table
- Orders results by environment name
- Handles errors appropriately
One minor suggestion: consider removing the commented out debug statement on line 38.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/route.ts (1)
1-44: Well-structured systems list endpointThe implementation follows good practices for authentication, authorization, and error handling. The database query correctly filters systems by workspaceId and orders by slug.
Minor improvement: consider removing the commented out console.dir(error) on line 33.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/route.ts (1)
1-52: Clean implementation of resources list endpointThe endpoint is properly secured with authentication and authorization middleware, and follows the established pattern for listing entities by workspace.
Some minor cleanup suggestions:
- Remove empty lines 4-5, 10-11, and 15 for consistency
- Remove the commented debug statement on line 41
| import * as React from "react"; | ||
| import { Button, Html } from "@react-email/components"; | ||
| export const MyEmail = () => { | ||
| return (<Html> | ||
| <Button href="https://example.com" style={{ background: "#000", color: "#fff", padding: "12px 20px" }}> | ||
| Click me | ||
| </Button> | ||
| </Html>); | ||
| }; | ||
| export default MyEmail; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Component name doesn't match file purpose
The filename "Welcome.jsx" suggests a welcome email template, but the component is named "MyEmail" and contains just a basic button. Consider renaming the component to match its purpose (e.g., "WelcomeEmail") and enhancing it with proper welcome content.
-export const MyEmail = () => {
+export const WelcomeEmail = () => {
return (<Html>
+ <h1>Welcome to Our Platform!</h1>
+ <p>We're excited to have you on board.</p>
<Button href="https://example.com" style={{ background: "#000", color: "#fff", padding: "12px 20px" }}>
- Click me
+ Get Started
</Button>
</Html>);
};
-export default MyEmail;
+export default WelcomeEmail;📝 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.
| import * as React from "react"; | |
| import { Button, Html } from "@react-email/components"; | |
| export const MyEmail = () => { | |
| return (<Html> | |
| <Button href="https://example.com" style={{ background: "#000", color: "#fff", padding: "12px 20px" }}> | |
| Click me | |
| </Button> | |
| </Html>); | |
| }; | |
| export default MyEmail; | |
| import * as React from "react"; | |
| import { Button, Html } from "@react-email/components"; | |
| export const WelcomeEmail = () => { | |
| return (<Html> | |
| <h1>Welcome to Our Platform!</h1> | |
| <p>We're excited to have you on board.</p> | |
| <Button href="https://example.com" style={{ background: "#000", color: "#fff", padding: "12px 20px" }}> | |
| Get Started | |
| </Button> | |
| </Html>); | |
| }; | |
| export default WelcomeEmail; |
packages/emails/tailwind.config.js
Outdated
| @@ -0,0 +1,21 @@ | |||
| import baseConfig from "@ctrlplane/tailwind-config/web"; | |||
| export default { | |||
| content: ["./src/**/*.tsx"], | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Content path only includes TSX files
The content configuration only includes TSX files, but your component uses JSX extension. This may cause Tailwind to miss your styles in JSX files.
- content: ["./src/**/*.tsx"],
+ content: ["./src/**/*.{jsx,tsx}"],📝 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.
| content: ["./src/**/*.tsx"], | |
| content: ["./src/**/*.{jsx,tsx}"], |
| import { authn, authz } from "../auth"; | ||
| import { parseBody } from "../body-parser"; | ||
| import { request } from "../middleware"; | ||
| import * as schema from "@ctrlplane/db/schema"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate schema import with different naming convention
There's already an import for the same module on line 7 as * as SCHEMA. Having two imports for the same module with different casing conventions can lead to confusion.
Consider consolidating to a single import:
- import * as SCHEMA from "@ctrlplane/db/schema";
- import * as schema from "@ctrlplane/db/schema";
+ import * as schema from "@ctrlplane/db/schema";Then update all references to SCHEMA in the file.
📝 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.
| import * as schema from "@ctrlplane/db/schema"; | |
| - import * as SCHEMA from "@ctrlplane/db/schema"; | |
| - import * as schema from "@ctrlplane/db/schema"; | |
| + import * as schema from "@ctrlplane/db/schema"; |
| authz(async ({ can, extra: { params } }) => | ||
| can | ||
| .perform(Permission.DeploymentList) | ||
| .on({ type: "workspace", id: (await params).workspaceId }), |
There was a problem hiding this comment.
Do we need to await this? I don't think we do it in any other endpoints
There was a problem hiding this comment.
this framework is a custom built one, if nxj 15 requires awaits we might need to update all endpoints? and I would think the lint/typescript would pick this up
There was a problem hiding this comment.
I mistook the warning "Error message" for an actual error while I was debugging. Eventually, we would need to await params but it is only warned for now. Nevertheless, we should await it, per the docs: https://nextjs.org/docs/messages/sync-dynamic-apis
Error: Route "/api/v1/workspaces/[workspaceId]/environments" used `params.workspaceId`. `params` should be awaited before using its properties. Learn more: https://nextjs.org/docs/messages/sync-dynamic-apis at createParamsAccessError (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:26:5107) at /home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:26:2694 at syncIODev (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:26:3762) at Object.get (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:26:29324) at /home/jonathan/code/wandb/ctrlplane/apps/webservice/.next/server/chunks/[root of the server]__dfa64689._.js:5259:17 at next (/home/jonathan/code/wandb/ctrlplane/apps/webservice/.next/server/chunks/[root of the server]__dfa64689._.js:5217:57) at /home/jonathan/code/wandb/ctrlplane/apps/webservice/.next/server/chunks/[root of the server]__dfa64689._.js:5179:20 at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async AppRouteRouteModule.do (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:26:33891) at async AppRouteRouteModule.handle (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:26:41254) at async doRender (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/base-server.js:1513:42) at async DevServer.renderToResponseWithComponentsImpl (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/base-server.js:1915:28) at async DevServer.renderPageComponent (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/base-server.js:2393:24) at async DevServer.renderToResponseImpl (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/base-server.js:2430:32) at async DevServer.pipeImpl (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/base-server.js:1003:25) at async NextNodeServer.handleCatchallRenderRequest (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/next-server.js:304:17) at async DevServer.handleRequestImpl (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/base-server.js:895:17) at async /home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/dev/next-dev-server.js:371:20 at async Span.traceAsyncFn (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/trace/trace.js:157:20) at async DevServer.handleRequest (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/dev/next-dev-server.js:368:24) at async invokeRender (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/lib/router-server.js:234:21) at async handleRequest (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/lib/router-server.js:422:24) at async requestHandlerImpl (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/lib/router-server.js:446:13) at async Server.requestListener (/home/jonathan/code/wandb/ctrlplane/node_modules/next/dist/server/lib/start-server.js:158:13) {stack: "Error: Route "/api/v1/workspaces/[workspaceId]/env…ules/next/dist/server/lib/start-server.js:158:13)", message: "Route "/api/v1/workspaces/[workspaceId]/environmen…ttps://nextjs.org/docs/messages/sync-dynamic-apis", __NEXT_ERROR_CODE: "E307"}```
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.ts (3)
36-36: Remove commented debugging codeThere's a commented out debugging statement that should be removed before merging to production.
- //console.dir(error);
35-45: Error handling is adequate but could be enhancedThe error handling captures all exceptions and returns appropriate responses. However, consider adding more specific error handling for different error types (database errors, validation errors, etc.) to provide more helpful responses to API consumers.
23-31: Consider implementing paginationFor endpoints that return lists of resources, pagination is usually necessary as the dataset grows. Consider implementing pagination parameters (limit/offset or cursor-based) to ensure the API remains performant with large datasets.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/webservice/src/app/(auth)/sign-up/SignUpCard.tsx(7 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/SearchDialog.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/(deploy)/(sidebar)/systems/SystemsPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(appv2)/TopNav.tsx(2 hunks)apps/webservice/src/app/api/v1/deployments/route.ts(1 hunks)apps/webservice/src/app/api/v1/middleware.ts(1 hunks)apps/webservice/src/app/api/v1/resources/route.ts(1 hunks)apps/webservice/src/app/api/v1/systems/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/route.ts(1 hunks)apps/webservice/src/app/terminal/_components/Terminal.tsx(1 hunks)packages/emails/src/Welcome.jsx(1 hunks)packages/emails/src/client/env.js(1 hunks)packages/emails/src/client/index.js(1 hunks)packages/emails/tailwind.config.js(1 hunks)packages/ui/src/button.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/ui/src/button.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/SearchDialog.tsx
- apps/webservice/src/app/terminal/_components/Terminal.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/TopNav.tsx
- apps/webservice/src/app/(auth)/sign-up/SignUpCard.tsx
- apps/webservice/src/app/[workspaceSlug]/(appv2)/(deploy)/(sidebar)/systems/SystemsPageContent.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/webservice/src/app/api/v1/systems/route.ts
- apps/webservice/src/app/api/v1/deployments/route.ts
- apps/webservice/src/app/api/v1/resources/route.ts
- packages/emails/tailwind.config.js
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/openapi.ts
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/openapi.ts
- apps/webservice/src/app/api/v1/middleware.ts
- packages/emails/src/Welcome.jsx
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/route.ts
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/deployments/openapi.ts
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/route.ts
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/openapi.ts
- packages/emails/src/client/index.js
- packages/emails/src/client/env.js
- apps/webservice/src/app/api/v1/workspaces/[workspaceId]/systems/route.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.ts
🔇 Additional comments (2)
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/environments/route.ts (2)
13-21: Authentication and authorization setup looks good!The request handler correctly implements authentication and authorization middleware, ensuring that only users with proper permissions can access environment data for a workspace. The code awaits
paramswhich addresses the need mentioned in the PR objectives for NJS 15+ compatibility.
22-35: API implementation follows best practicesThe database query is well-structured, using a left join with the system table to capture all environment records, even those with null relationships - which aligns with the PR objective of "utilizing left joins to capture null metadata combinations." The response format is clean and consistent with REST API standards.
2 participants
A key fix is to
middleware.ts-- to enable the handler to safely type the "extra"as either a Promise of the object or just the object.
paramsmust be awaitedwith NJS 15+
Summary by CodeRabbit
New Features
Documentation