fix: Release channel unique constraint #205
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
| .refine((name) => { | ||
| return !releaseChannels.some((rc) => rc.name === name); | ||
| }, "Release channel name must be unique"), |
There was a problem hiding this comment.
| .refine((name) => { | |
| return !releaseChannels.some((rc) => rc.name === name); | |
| }, "Release channel name must be unique"), | |
| .refine((name) => | |
| !releaseChannels.some((rc) => rc.name === name) | |
| , "Release channel name must be unique"), |
| @@ -25,6 +25,7 @@ export const POST = request() | |||
| db | |||
| .insert(SCHEMA.releaseChannel) | |||
| .values(body) | |||
| .onConflictDoNothing() | |||
There was a problem hiding this comment.
you should be throwing a 409 status code
![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
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (2)
21-23: Consider adding error handling for the queryWhile the query setup and data handling are correct, consider adding error handling to improve user experience when the query fails.
const releaseChannelsQ = api.deployment.releaseChannel.list.byDeploymentId.useQuery(deploymentId); const releaseChannels = releaseChannelsQ.data ?? []; + if (releaseChannelsQ.error) { + // Consider showing an error toast or inline error message + console.error('Failed to fetch release channels:', releaseChannelsQ.error); + }
35-39: Consider showing a loading stateWhile the loading check prevents premature rendering, the current implementation simply shows nothing during loading. Consider showing a loading indicator or skeleton to improve user experience.
- {isReleaseChannelsActive && !releaseChannelsQ.isLoading && ( + {isReleaseChannelsActive && ( + releaseChannelsQ.isLoading ? ( + <Button size="sm" variant="secondary" disabled> + Loading... + </Button> + ) : ( <CreateReleaseChannelDialog deploymentId={deploymentId} releaseChannels={releaseChannels} > <Button size="sm" variant="secondary"> New Release Channel </Button> </CreateReleaseChannelDialog> + ) )}apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (2)
55-69: Consider enhancing the name validation.While the uniqueness validation is correctly implemented, consider these improvements:
- Add case-insensitive comparison to prevent confusion (e.g., "prod" vs "PROD")
- Provide a more specific error message indicating the conflicting name
const schema = z.object({ name: z .string() .min(1) .max(50) .refine((name) => { - return !releaseChannels.some((rc) => rc.name === name); + return !releaseChannels.some( + (rc) => rc.name.toLowerCase() === name.toLowerCase() + ); }, "Release channel name must be unique"),
Line range hint
82-89: Enhance error handling in form submission.The mutation error handling could be improved to provide better user feedback. Consider adding error handling and removing the unnecessary form reset.
onSubmit = form.handleSubmit((data) => { const filter = getFinalFilter(data.releaseFilter); createReleaseChannel .mutateAsync({ ...data, deploymentId, releaseFilter: filter }) - .then(() => form.reset(data)) .then(() => router.refresh()) - .then(() => setOpen(false)); + .then(() => setOpen(false)) + .catch((error) => { + // Handle specific error cases (e.g., concurrent creation causing uniqueness violation) + console.error('Failed to create release channel:', error); + }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx(2 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/release-channels/route.ts(1 hunks)apps/webservice/src/app/api/v1/release-channels/route.ts(1 hunks)packages/db/drizzle/0033_abnormal_microchip.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/release.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/db/drizzle/0033_abnormal_microchip.sql
- packages/db/drizzle/meta/_journal.json
🔇 Additional comments (6)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx (2)
9-9: LGTM: Clean import addition
The tRPC API import is correctly added and follows the standard pattern.
35-39: Verify uniqueness constraint implementation
The changes pass releaseChannels to the dialog for uniqueness validation. Let's verify the implementation in the CreateReleaseChannelDialog component.
✅ Verification successful
Let me try different commands to locate and analyze the CreateReleaseChannelDialog component.
From the search results, I found the CreateReleaseChannelDialog component and its uniqueness validation implementation. I can now generate the final response.
Uniqueness constraint is properly implemented
The CreateReleaseChannelDialog component implements name uniqueness validation through a Zod schema that checks if the entered name already exists in the releaseChannels array:
.refine((name) => {
return !releaseChannels.some((rc) => rc.name === name);
})🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify uniqueness validation in CreateReleaseChannelDialog
# Search for name validation logic in the dialog component
rg -A 10 "CreateReleaseChannelDialog" --type tsx
# Search for any uniqueness-related validation
ast-grep --pattern 'name === $_'
Length of output: 506
Script:
#!/bin/bash
# Search for CreateReleaseChannelDialog component with proper file extension
rg -A 10 "CreateReleaseChannelDialog" --type ts --type tsx --type-add 'tsx:*.{tsx,ts}'
# Search for validation logic using name/channel patterns
rg -A 5 "releaseChannels?.find" --type ts --type tsx --type-add 'tsx:*.{tsx,ts}'
rg -A 5 "channel.*exists|exists.*channel" --type ts --type tsx --type-add 'tsx:*.{tsx,ts}'
# Try to find the component definition using ast-grep
ast-grep --pattern 'const CreateReleaseChannelDialog = $_'
Length of output: 16469
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx (2)
3-3: LGTM: Type-safe integration of release channels.
The addition of the SCHEMA import and the updated props type properly integrate the release channels data needed for uniqueness validation.
Also applies to: 44-47
Line range hint 82-82: Verify API endpoint error handling.
Ensure that the API endpoint properly handles uniqueness constraint violations and returns appropriate error responses that can be handled by the frontend.
✅ Verification successful
API endpoint has proper uniqueness constraint handling
The verification confirms that the API endpoint is properly designed to handle uniqueness constraints:
- The database schema enforces a unique constraint on
(deploymentId, name)combination through a unique index - The TRPC mutation uses Drizzle ORM which will automatically handle and throw errors for uniqueness violations
- The frontend can safely handle these database constraint errors through TRPC's error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the API endpoint implementation to verify error handling
ast-grep --pattern 'export const releaseChannelRouter = createTRPCRouter({
$$$
create: protectedProcedure
.input($_)
.mutation($_)
$$$
})'
Length of output: 154
Script:
#!/bin/bash
# Let's try a broader search for the release channel router
rg -l "releaseChannelRouter"
# Also search for any file containing release channel creation logic
rg -l "createReleaseChannel"
# Search for trpc router files that might contain the endpoint
fd "router.ts|router.tsx"
Length of output: 720
Script:
#!/bin/bash
# Let's examine the deployment router implementation for release channel creation
rg "createReleaseChannel" packages/api/src/router/deployment.ts -A 10 -B 2
# Also check the schema definition for any unique constraints
rg "createReleaseChannel|ReleaseChannel" packages/db/src/schema/release.ts -A 5 -B 2
Length of output: 1590
Script:
#!/bin/bash
# Let's check the schema definition for the release channel table structure and constraints
rg "releaseChannel = pgTable" packages/db/src/schema/release.ts -A 15 -B 0
# Also check if there are any unique indexes or constraints defined
rg "createIndex|unique" packages/db/src/schema/release.ts
Length of output: 923
packages/db/src/schema/release.ts (2)
Line range hint 89-108: Verify the scope: Additional unique constraint on releaseDependency table.
While the unique index on (releaseId, deploymentId) is logically sound to prevent duplicate dependencies, this change appears to be outside the scope of the PR objectives which focused on release channel constraints. Please clarify if this additional constraint was intentional.
Let's check if this constraint is related to any reported issues:
57-71: LGTM! Unique constraint properly implemented.
The unique index on (deploymentId, name) ensures that release channel names remain unique within each deployment, which is the correct approach.
Let's verify there are no existing duplicate records that might conflict with this constraint:
✅ Verification successful
Unique constraint implementation verified and properly migrated
The migration files confirm that:
- The release_channel table was initially created without the unique constraint (0026_blushing_kang.sql)
- The unique constraint was properly added later via a dedicated migration (0033_abnormal_microchip.sql) using
CREATE UNIQUE INDEX IF NOT EXISTS "release_channel_deployment_id_name_index"
This matches the schema definition in the code and ensures a clean upgrade path for existing databases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing duplicate release channel names within the same deployment
# that would violate the new unique constraint
# Search for SQL migration file that adds this constraint
echo "Searching for related migration file..."
fd --type f --extension sql . | xargs grep -l "release_channel_deployment_id_name"
# Look for potential duplicate checks in the codebase
echo "Checking for duplicate validation in the codebase..."
rg "duplicate.*release.*channel" --type ts
Length of output: 410
Script:
#!/bin/bash
# Let's try a different approach to find migration files and duplicate checks
# Search for migration files related to release_channel
echo "Searching for release channel migration files..."
fd -e sql . | xargs grep -l "release_channel"
# Search for any unique constraint related code for release channels
echo "Checking for unique constraint related code..."
rg "unique.*release.*channel" -t ts
# Search for any validation logic around release channel names
echo "Checking for validation logic..."
ast-grep --pattern 'function $FUNC($_) {
$$$
release$_channel$_
$$$
unique$_
$$$
}'
Length of output: 555
Script:
#!/bin/bash
# Let's examine the migration files we found to understand the constraint implementation
echo "Checking migration file content..."
cat packages/db/drizzle/0026_blushing_kang.sql
cat packages/db/drizzle/0033_abnormal_microchip.sql
# Also search for any error handling related to duplicate channels
echo "Checking for error handling..."
rg "duplicate.*channel" -A 3 -B 3 -t ts
Length of output: 5338
| @@ -25,6 +25,7 @@ export const POST = request() | |||
| db | |||
| .insert(SCHEMA.releaseChannel) | |||
| .values(body) | |||
| .onConflictDoNothing() | |||
| .returning() | |||
| .then(takeFirst) | |||
| .then((releaseChannel) => NextResponse.json(releaseChannel)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with specific error codes.
The current implementation returns a 500 status code for all errors, which doesn't distinguish between different types of errors (e.g., validation errors, database constraints, server errors).
Consider implementing more specific error handling:
- .catch((error) => NextResponse.json({ error }, { status: 500 })),
+ .catch((error) => {
+ if (error.code === '23505') { // PostgreSQL unique violation
+ return NextResponse.json(
+ { error: 'Release channel with this name already exists' },
+ { status: 409 }
+ );
+ }
+ console.error('Failed to create release channel:', error);
+ return NextResponse.json(
+ { error: 'Internal server error' },
+ { status: 500 }
+ );
+ }),Committable suggestion skipped: line range outside the PR's diff.
| @@ -25,6 +25,7 @@ | |||
| db | |||
| .insert(SCHEMA.releaseChannel) | |||
| .values(body) | |||
| .onConflictDoNothing() | |||
There was a problem hiding this comment.
Replace .onConflictDoNothing() with proper conflict handling.
The current approach of silently ignoring conflicts could lead to inconsistent UI state and poor user experience. Even though the UI performs pre-validation, race conditions could still occur, making it important to handle conflicts explicitly.
Consider this alternative approach:
- .onConflictDoNothing()
+ .returning()
+ .then(takeFirst)
+ .then((releaseChannel) => {
+ if (!releaseChannel) {
+ return NextResponse.json(
+ { error: 'Release channel with this name already exists' },
+ { status: 409 }
+ );
+ }
+ return NextResponse.json(releaseChannel);
+ })Committable suggestion skipped: line range outside the PR's diff.
| @@ -29,6 +29,7 @@ export const POST = request() | |||
| const releaseChannel = await ctx.db | |||
| .insert(schema.releaseChannel) | |||
| .values({ ...ctx.body, deploymentId: extra.params.deploymentId }) | |||
| .onConflictDoNothing() | |||
There was a problem hiding this comment.
Consider explicit conflict handling instead of silent failure.
The current implementation using .onConflictDoNothing() silently fails when a duplicate release channel name exists, which may lead to a confusing user experience. Instead, consider explicitly handling the conflict and returning appropriate error responses.
Here's a suggested approach:
- .onConflictDoNothing()
- .returning();
+ .returning()
+ .catch((error) => {
+ if (error.code === '23505') { // PostgreSQL unique violation code
+ throw new Error('A release channel with this name already exists in this deployment');
+ }
+ throw error;
+ });
// Update the error handling middleware to return appropriate status codes
// 409 Conflict for unique constraint violations
// 500 Internal Server Error for other errorsThis change will:
- Provide clear feedback when a duplicate name is encountered
- Allow the client to handle the error appropriately
- Maintain proper REST semantics by returning appropriate status codes
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/api/v1/release-channels/route.ts (1)
25-49: Consider potential race condition.There's a potential race condition between the existence check and the insert operation. While the database's unique constraint provides ultimate protection, concurrent requests could lead to confusing error messages.
Consider using a transaction to make the check and insert atomic:
async ({ db, body }) => { + return await db.transaction(async (trx) => { const releaseChannel = await db .select() .from(SCHEMA.releaseChannel) .where( and( eq(SCHEMA.releaseChannel.deploymentId, body.deploymentId), eq(SCHEMA.releaseChannel.name, body.name), ), ) .then(takeFirstOrNull); if (releaseChannel) return NextResponse.json( { error: "Release channel already exists" }, { status: 409 }, ); return db .insert(SCHEMA.releaseChannel) .values(body) .returning() .then(takeFirst) .then((releaseChannel) => NextResponse.json(releaseChannel)) .catch((error) => NextResponse.json({ error }, { status: 500 })); + }); },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx(2 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/release-channels/route.ts(2 hunks)apps/webservice/src/app/api/v1/release-channels/route.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx
- apps/webservice/src/app/api/v1/deployments/[deploymentId]/release-channels/route.ts
🔇 Additional comments (3)
apps/webservice/src/app/api/v1/release-channels/route.ts (3)
4-4: LGTM: Import additions are appropriate.
The new imports support the uniqueness check functionality.
25-34: LGTM: Well-implemented uniqueness check.
The query correctly validates uniqueness using both deploymentId and name.
36-40: LGTM: Proper conflict response.
Correctly implements 409 status code with a clear error message as suggested in previous reviews.
| .insert(SCHEMA.releaseChannel) | ||
| .values(body) | ||
| .returning() | ||
| .then(takeFirst) | ||
| .then((releaseChannel) => NextResponse.json(releaseChannel)) | ||
| .catch((error) => NextResponse.json({ error }, { status: 500 })), | ||
| .catch((error) => NextResponse.json({ error }, { status: 500 })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with logging and specific error codes.
The current error handling could be more robust:
- Add error logging for debugging
- Handle specific database errors (e.g., constraint violations)
- .catch((error) => NextResponse.json({ error }, { status: 500 }));
+ .catch((error) => {
+ console.error('Failed to create release channel:', error);
+ if (error.code === '23505') { // PostgreSQL unique violation
+ return NextResponse.json(
+ { error: 'Release channel with this name already exists' },
+ { status: 409 }
+ );
+ }
+ return NextResponse.json(
+ { error: 'Internal server error' },
+ { 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.
| .catch((error) => NextResponse.json({ error }, { status: 500 })); | |
| .catch((error) => { | |
| console.error('Failed to create release channel:', error); | |
| if (error.code === '23505') { // PostgreSQL unique violation | |
| return NextResponse.json( | |
| { error: 'Release channel with this name already exists' }, | |
| { status: 409 } | |
| ); | |
| } | |
| return NextResponse.json( | |
| { error: 'Internal server error' }, | |
| { status: 500 } | |
| ); | |
| }); |
Not adding it to the trpc endpoint. if you add a
Summary by CodeRabbit
Release Notes
New Features
CreateReleaseChannelDialogcomponent to enforce unique names for release channels, improving data integrity.NavigationMenuActioncomponent.Bug Fixes
CreateReleaseChannelDialogto ensure it displays only when data is ready.Chores
releaseChannelandreleaseDependencytables to maintain data integrity.