fix: Ephemeral envs init #187
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several key changes across multiple files, primarily focusing on the addition of new dependencies, enhancements to job management functionalities, and modifications to various components for improved user experience. Notable updates include the introduction of a new job ( 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
packages/db/drizzle/0028_peaceful_otto_octavius.sql (1)
1-1: Consider adding a database trigger for expired environments.With the shift from policy-based
ephemeral_durationto directexpires_attimestamps, consider implementing a database trigger or scheduled cleanup job to automatically handle expired environments.Would you like me to help create a database trigger or cleanup job implementation?
Also applies to: 3-3
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)
Line range hint
15-65: Consider implementing pagination for better performance.The endpoint retrieves all running jobs without pagination, which could lead to performance issues if there are many jobs. Consider implementing pagination to limit the result set size.
Example implementation:
export const GET = async ( req: NextRequest, { params }: { params: { agentId: string } }, ) => { + const { searchParams } = new URL(req.url); + const limit = parseInt(searchParams.get('limit') ?? '50'); + const offset = parseInt(searchParams.get('offset') ?? '0'); const user = await getUser(req); if (!user) return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); const je = await db .select() .from(job) .innerJoin(releaseJobTrigger, eq(releaseJobTrigger.jobId, job.id)) .leftJoin(environment, eq(environment.id, releaseJobTrigger.environmentId)) .leftJoin(target, eq(target.id, releaseJobTrigger.targetId)) .leftJoin(release, eq(release.id, releaseJobTrigger.releaseId)) .leftJoin(deployment, eq(deployment.id, release.deploymentId)) .where( and( eq(job.jobAgentId, params.agentId), notInArray(job.status, [ "failure", "cancelled", "skipped", "completed", "invalid_job_agent", ]), ), ) + .limit(limit) + .offset(offset) .then((rows) => rows.map((row) => ({ ...row.job, config: row.release_job_trigger, environment: row.environment, target: row.target, deployment: row.deployment, release: row.release, })), ); - return NextResponse.json(je); + return NextResponse.json({ + data: je, + pagination: { + limit, + offset, + }, + }); };apps/jobs/src/index.ts (1)
Line range hint
71-79: Consider enhancing job execution monitoring.The error handling is good, but consider these improvements for better observability:
- Add job duration tracking
- Add logic to prevent concurrent executions of the same job
- Add metrics for successful/failed job runs
Example implementation:
const runJob = async (job: string) => { logger.info(`Running job: ${job}`); + const startTime = Date.now(); try { await jobs[job]?.run(); - logger.info(`Job ${job} completed successfully`); + const duration = Date.now() - startTime; + logger.info(`Job ${job} completed successfully in ${duration}ms`); } catch (error: any) { logger.error(`Error running job ${job}: ${error.message}`, error); } };packages/job-dispatch/src/policies/release-string-check.ts (1)
104-104: Document and verify the implications of including deleted environmentsThe simplified where clause now includes all environments, including potentially deleted ones. This change needs to be:
- Documented in the function's JSDoc to make this behavior explicit
- Verified that it aligns with the ephemeral environment initialization requirements
Add this documentation to the function's JSDoc:
/** + * Note: This function includes all environments in the query, including deleted ones. + * This behavior was modified as part of PR #187 to support ephemeral environment initialization. * * @param db * @param wfConsider implementing a consistent pattern across the codebase for handling deleted environments. This could be:
- A reusable query builder that makes the inclusion/exclusion of deleted records configurable
- A clear documentation of when deleted records should be included/excluded
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (2)
79-96: LGTM with enhancement suggestions!The new drawer title structure with error handling is well-implemented. Consider these minor improvements:
- Add an error icon next to the error message for better visual indication
- Consider adding text wrapping control for long error messages
{environmentQError != null && ( - <div className="text-xs text-red-500"> + <div className="flex items-center gap-1 text-xs text-red-500"> + <IconExclamationCircle className="h-3 w-3" /> {environmentQError.message} </div> )}
Line range hint
102-104: Consider enhancing the loading state feedback.While the loading spinner is good, users might benefit from knowing what's being loaded. Consider adding a descriptive message.
<div className="flex h-full items-center justify-center"> + <div className="flex flex-col items-center gap-2"> <IconLoader2 className="h-8 w-8 animate-spin" /> + <p className="text-sm text-muted-foreground">Loading environment details...</p> + </div> </div>packages/db/src/schema/environment.ts (2)
51-53: Consider adding a custom error message to the refinement.The validation logic is solid, but adding a custom error message would improve the developer experience.
Consider updating the refinement to include a descriptive error message:
targetFilter: targetCondition .optional() - .refine((filter) => filter == null || isValidTargetCondition(filter)), + .refine( + (filter) => filter == null || isValidTargetCondition(filter), + "Target filter must be either null or a valid target condition" + ),
45-45: Consider additional schema enhancements for ephemeral environments.While the
expiresAtfield addition is a good start, consider these additional enhancements for robust ephemeral environment support:
- Add an index on
expiresAtto optimize queries for cleanup jobs- Consider adding a boolean flag
isEphemeralfor quick filtering- Think about adding
createdByto track ephemeral environment ownershipExample index addition:
CREATE INDEX idx_environment_expires_at ON environment (expires_at) WHERE expires_at IS NOT NULL;apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx (2)
40-50: Consider adding input validation to getExpiresAt.While the function works correctly, consider adding validation for negative duration numbers and handling edge cases.
const getExpiresAt = ( expiresAt: Date | null, durationNumber: number, durationUnit: "minutes" | "hours" | "days", ) => { + if (durationNumber < 0) { + throw new Error("Duration number must be positive"); + } const currExpiresAt = expiresAt ?? new Date(); if (durationUnit === "minutes") return addMinutes(currExpiresAt, durationNumber); if (durationUnit === "hours") return addHours(currExpiresAt, durationNumber); return addDays(currExpiresAt, durationNumber); };
137-218: Enhance accessibility for form controls.While the UI implementation is clean, consider adding ARIA labels and descriptions for better screen reader support.
<Input type="number" value={value ?? ""} + aria-label="Duration number" className="appearance-none [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none" onChange={(e) => {Also, consider adding helper text to explain the expiration behavior:
<div className="space-y-4"> <Label>Environment expiration</Label> + <p className="text-sm text-muted-foreground" id="expiration-help"> + Set how long this environment should exist before automatic cleanup. + </p> <div className="flex flex-col gap-1 text-sm text-muted-foreground">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
apps/jobs/package.json(1 hunks)apps/jobs/src/ephemeral-env-checker/index.ts(1 hunks)apps/jobs/src/index.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx(3 hunks)apps/webservice/src/app/api/v1/environments/route.ts(2 hunks)apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(2 hunks)packages/api/src/router/environment-policy.ts(0 hunks)packages/api/src/router/environment.ts(1 hunks)packages/api/src/router/job.ts(2 hunks)packages/db/drizzle/0028_peaceful_otto_octavius.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/environment.ts(2 hunks)packages/job-dispatch/src/__test__/job-variables-deployment.test.ts(7 hunks)packages/job-dispatch/src/policies/gradual-rollout.ts(2 hunks)packages/job-dispatch/src/policies/manual-approval.ts(2 hunks)packages/job-dispatch/src/policies/release-string-check.ts(2 hunks)packages/job-dispatch/src/policies/release-window.ts(2 hunks)packages/job-dispatch/src/policies/success-rate-criteria-passing.ts(2 hunks)packages/job-dispatch/src/policy-create.ts(1 hunks)packages/job-dispatch/src/release-job-trigger.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/api/src/router/environment-policy.ts
- packages/job-dispatch/src/release-job-trigger.ts
✅ Files skipped from review due to trivial changes (1)
- packages/job-dispatch/src/test/job-variables-deployment.test.ts
🧰 Additional context used
📓 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#183
File: apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx:55-57
Timestamp: 2024-10-29T23:49:24.855Z
Learning: In `EnvironmentDrawer.tsx`, we prefer to fetch all data upfront and show a loading indicator until everything is fetched, unless performance becomes an issue.
🔇 Additional comments (30)
packages/db/drizzle/0028_peaceful_otto_octavius.sql (2)
1-3: LGTM! Migration structure looks good.
The migration correctly implements the schema changes for ephemeral environments with proper statement breakpoints and IF EXISTS clauses.
2-2: Verify data migration for deleted environments.
Before dropping the deleted_at column, ensure that any relevant data has been properly migrated or archived, as this change could result in data loss for soft-deleted environments.
✅ Verification successful
No data migration needed - Column replacement is safe
The deleted_at column is being replaced with expires_at as part of a transition to ephemeral environments. The codebase shows:
- No active usage of
deleted_atin any migrations between 0000 and 0028 - A new ephemeral environment checker (apps/jobs/src/ephemeral-env-checker/index.ts) that handles environment cleanup using the new
expires_atfield - Clean environment deletion logic in the router that doesn't rely on soft deletes
The migration is safe to proceed as it's part of a feature transition from soft-delete to expiration-based cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any environments with deleted_at values that might need migration
echo "Searching for references to deleted_at in the codebase..."
rg -l "deleted_at" --type sql
# Look for any environment-related cleanup or archive logic
echo "Checking for environment cleanup implementations..."
rg -l "cleanup|archive.*environment" --type typescript
Length of output: 454
Script:
#!/bin/bash
# Check the initial migration that added deleted_at
echo "Checking initial migration that added deleted_at..."
cat packages/db/drizzle/0000_busy_firebird.sql
# Check the current migration dropping deleted_at
echo "Checking current migration dropping deleted_at..."
cat packages/db/drizzle/0028_peaceful_otto_octavius.sql
# Look for environment-related code with proper file type
echo "Checking for environment cleanup implementations..."
rg -l "environment.*delete|delete.*environment" --type ts --type js
# Check for any environment model/schema definitions
echo "Looking for environment model definitions..."
rg -l "environment.*schema|schema.*environment" --type ts --type js
Length of output: 41782
Script:
#!/bin/bash
# Check for ephemeral environment handling
echo "Checking ephemeral environment implementation..."
cat apps/jobs/src/ephemeral-env-checker/index.ts
# Check environment router for deletion logic
echo "Checking environment router implementation..."
cat packages/api/src/router/environment.ts
# Check if there are any migrations between 0000 and 0028 that modify deleted_at
echo "Checking for intermediate migrations affecting deleted_at..."
rg -l "deleted_at" packages/db/drizzle/00[0-1][0-9]_*.sql packages/db/drizzle/002[0-7]_*.sql
Length of output: 13614
apps/jobs/package.json (1)
21-22: LGTM! Dependencies are well-chosen for the use case.
The additions of lodash and ts-is-present are appropriate for managing ephemeral environments and type validation. The caret ranges allow for safe minor and patch updates while maintaining compatibility.
Let's verify these are the latest compatible versions:
✅ Verification successful
Dependencies are using the latest available versions
The installed versions of both dependencies match their latest available versions:
This confirms the package.json is using up-to-date versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the versions of new dependencies
# Expected: These should be the latest compatible versions
echo "Checking latest versions..."
npm view lodash version
npm view ts-is-present version
Length of output: 345
packages/job-dispatch/src/policy-create.ts (2)
Line range hint 5-11: LGTM: Imports are well-organized and necessary
The added SQL query builder imports are appropriate for the query construction below.
Line range hint 13-42: Verify: Intentional inclusion of deleted environments
The removal of the isNull(environment.deletedAt) condition means that policies will be created for deleted environments. While this aligns with similar changes across the codebase, please confirm if this is intentional as it could have security implications.
Consider the following scenarios:
- Could this lead to unnecessary approval records for deleted environments?
- Is there a risk of unauthorized access if approvals exist for deleted environments?
Let's verify the broader pattern of this change:
✅ Verification successful
Let's gather more specific information about the deletedAt usage patterns.
Let's try a different approach to understand the environment and policy relationships.
Based on the evidence gathered, I can now provide a final response:
Removal of deletedAt check is intentional and correct
The codebase has undergone a schema change where the deleted_at column was explicitly dropped from the environment table (as seen in the migration file 0028_peaceful_otto_octavius.sql). Instead, the schema now uses an expires_at column for managing environment lifecycles. Therefore, the removal of isNull(environment.deletedAt) check is not only intentional but necessary as that column no longer exists.
Key findings:
- Environment table schema has replaced
deleted_atwithexpires_atfor lifecycle management - The change aligns with the PR's objective of handling ephemeral environments
- No security implications as environment deletion is now handled through expiration rather than soft deletion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of environment.deletedAt usage
# to understand the broader context of this change
# Search for any remaining deletedAt checks
rg -l "deletedAt|deleted_at" --type ts
# Search for environment policy related files
rg -l "environmentPolicy|environment_policy" --type ts
Length of output: 1619
Script:
#!/bin/bash
# Check for deletedAt conditions in environment-related queries
rg "deletedAt|deleted_at" -B 2 -A 2 packages/db/src/schema/environment.ts packages/job-dispatch/src/job-creation.ts packages/api/src/router/environment.ts
# Look for any recent changes related to deletedAt in git history
git log -p --grep="deletedAt\|deleted_at" -n 5
Length of output: 205
Script:
#!/bin/bash
# Look for environment schema definition
ast-grep --pattern 'export const environment = pgTable($$$)'
# Search for environment-related queries in policy files
rg "from\(.*environment\)" -B 2 -A 2 packages/job-dispatch/src/policies/
# Check for environment deletion handling
rg "environment.*delete" -B 2 -A 2
Length of output: 5323
apps/webservice/src/app/api/v1/environments/route.ts (2)
4-4: LGTM: Clean import additions
The new imports enhance type safety (isPresent) and database operation handling (takeFirst).
Also applies to: 7-7
33-38: LGTM: Clean date handling implementation
The implementation correctly:
- Preserves existing fields using spread operator
- Safely handles date conversion with type checking
- Properly sets undefined when expiresAt is not present
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (2)
Line range hint 4-11: LGTM! Import statements are properly organized.
The new import for database operations is correctly structured and grouped with other database-related imports.
Line range hint 15-65: Verify the impact of removed deletedAt check.
The removal of isNull(environment.deletedAt) check might allow querying jobs associated with deleted environments. While this aligns with the schema changes removing the deletedAt column, we should verify if this is the intended behavior.
Let's verify the schema changes and related queries:
✅ Verification successful
The removal of deletedAt check is correct as the column has been removed
From the migration files, we can see that the deleted_at column was explicitly dropped from the environment table with the SQL statement: ALTER TABLE "environment" DROP COLUMN IF EXISTS "deleted_at";. This confirms that the removal of the isNull(environment.deletedAt) check from the query is the correct approach as the column no longer exists in the schema.
The schema has moved away from soft deletion for environments, so there's no need to check for deleted records anymore. All environment records in the database are now considered active.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schema changes and related queries
# Check if deletedAt column has been removed from environment table
echo "Checking environment table schema..."
rg -A 10 "environment.*=.*createTable" packages/db/src/schema/
# Check for other instances of deletedAt in queries
echo "Checking for other deletedAt references..."
rg "deletedAt.*environment" --type ts
# Check for migration files
echo "Checking migration files..."
rg "deletedAt" packages/db/drizzle/
Length of output: 383
Script:
#!/bin/bash
# Let's try a different approach to verify the schema changes
# Check the environment table schema definition
echo "Checking environment table schema..."
fd "schema.ts" packages/db/src/ --exec cat {}
# Check for any migrations that might have removed deletedAt
echo "Checking recent migration files..."
fd "meta.json" packages/db/drizzle/ --exec cat {}
# Look for any other queries involving environment table
echo "Checking queries involving environment table..."
rg "from\(environment\)" --type ts -A 5
Length of output: 4054
Script:
#!/bin/bash
# Let's check the schema definition more specifically
# Look for environment table definition using ast-grep
echo "Checking environment table definition..."
ast-grep --pattern 'export const environment = createTable("$_", {
$$$
});'
# Check for any migration files with drop column
echo "Checking migration files for deletedAt removal..."
fd "0" packages/db/drizzle/ -e sql --exec cat {}
# Look for soft delete related code
echo "Checking for soft delete patterns..."
rg "softDelete|isDeleted" --type ts
Length of output: 58150
packages/job-dispatch/src/policies/manual-approval.ts (2)
3-3: LGTM! Import cleanup looks good.
The imports have been properly cleaned up to only include the functions that are actually used in the code.
40-42: Verify the removal of deletedAt check.
The removal of the isNull(environment.deletedAt) check means this policy will now evaluate deleted environments. While this aligns with the PR's focus on ephemeral environments, we should verify this is intentional.
Let's check if this pattern is consistent across other policy files:
✅ Verification successful
Let me try a different search strategy to verify this change.
Let me try one final verification to check for any related changes in environment handling.
Based on the git diff and other findings, I can now provide a final response:
Removal of deletedAt check is intentional and aligned with ephemeral environment changes
The removal of isNull(environment.deletedAt) is part of a broader change implementing ephemeral environments. The git diff shows new functionality being added to handle environment expiration through expiresAt field, replacing the previous deletedAt check. This is a deliberate architectural change where environments are now managed through expiration rather than soft deletion.
Key evidence:
- New
expiresAtfield handling added for ephemeral environments - Implementation of
groupByEnvironmentfunction for managing environment lifecycles - Direct environment deletion through
delete(SCHEMA.environment)after expiration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar changes in other policy files
# and verify if deletedAt checks have been consistently removed
# Look for other policy files that might have similar patterns
echo "Checking other policy files for deletedAt patterns..."
rg -l "deletedAt" packages/job-dispatch/src/policies/
# Look for any remaining isNull checks on deletedAt
echo "Checking for any remaining deletedAt null checks..."
ast-grep --pattern 'isNull($_.deletedAt)'
Length of output: 327
Script:
#!/bin/bash
# Search for any deletedAt related conditions in policy files and their tests
echo "Searching for deletedAt in policy files and tests..."
rg -A 3 "deletedAt" packages/job-dispatch/src/policies/
# Check git history for the specific file to understand the context of removal
echo -e "\nChecking git history for manual-approval.ts..."
git log -p packages/job-dispatch/src/policies/manual-approval.ts | grep -A 5 -B 5 "deletedAt"
# Look for any environment-related queries in other policy files
echo -e "\nChecking environment queries in policy files..."
rg -A 3 "environment\." packages/job-dispatch/src/policies/
Length of output: 909
Script:
#!/bin/bash
# Look for environment-related changes in the PR
echo "Checking for environment-related changes in the PR..."
git diff HEAD~1 | grep -A 5 -B 5 "environment\."
# Search for any environment validation or checks in the codebase
echo -e "\nSearching for environment validation patterns..."
rg -A 3 "environment.*validation|validateEnvironment|checkEnvironment" packages/job-dispatch/src/
# Check for any environment-related tests
echo -e "\nChecking environment-related tests..."
rg -A 3 "environment" packages/job-dispatch/src/**/*test*
Length of output: 5856
packages/job-dispatch/src/policies/gradual-rollout.ts (1)
4-4: Verify the removal of deletedAt check for ephemeral environments.
The removal of the deletedAt check means that environments marked for deletion will still be included in the rollout policy. While this might be intentional for ephemeral environments to continue processing jobs until actual termination, it could lead to jobs being dispatched to environments that should be terminated.
Let's verify the impact and consistency of this change:
Also applies to: 41-43
packages/job-dispatch/src/policies/release-window.ts (2)
4-4: LGTM: Import addition aligns with query changes
The addition of inArray import is consistent with its usage in the modified query.
42-44: Verify the impact of removing deletedAt check
The removal of the deletedAt null check from the where clause could now include deleted environments in the release window policy evaluation. While this appears intentional as part of the ephemeral environments implementation, we should verify that this behavior is expected and consistent across the codebase.
Let's verify the pattern of deletedAt check removal across related files:
✅ Verification successful
Based on the evidence gathered from the shell scripts, particularly the SQL migrations showing that deleted_at was explicitly removed from the environment table and replaced with expires_at, and the lack of any deletedAt checks in environment queries, I can now generate the final response:
Removal of deletedAt is part of the ephemeral environments implementation
The removal of the deletedAt check is intentional and consistent with the codebase changes. The system has moved from soft deletion (deletedAt) to explicit expiration (expires_at) for environments, which aligns with the ephemeral environments implementation. This is evidenced by:
- Migration explicitly removing
deleted_atcolumn and addingexpires_at - No other
deletedAtchecks found in environment-related queries - Consistent handling across the codebase with no legacy soft-delete patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar changes in related policy files and environment queries
# Test 1: Look for other policy files that might have similar changes
echo "=== Checking other policy files ==="
fd -e ts -e js "policy" -x rg -l "deletedAt|isNull.*environment" {}
# Test 2: Check for environment-related queries to ensure consistent handling
echo "=== Checking environment queries ==="
rg -l "from.*environment.*where"
# Test 3: Look for related tests that might need updating
echo "=== Checking related tests ==="
fd -e test.ts -e spec.ts -x rg -l "release.*window.*policy" {}
Length of output: 406
Script:
#!/bin/bash
# Let's try a different approach to verify the impact
# Check for environment-related queries with deletedAt conditions
echo "=== Checking environment queries with deletedAt ==="
rg -A 3 "from.*environment"
# Look for release window policy implementations
echo "=== Checking release window policy implementations ==="
ast-grep --pattern 'class $_ implements ReleaseWindowPolicy {
$$$
}'
# Check for environment-related type definitions and interfaces
echo "=== Checking environment-related types ==="
rg -A 5 "interface.*Environment|type.*Environment"
# Look for environment deletion handling
echo "=== Checking environment deletion handling ==="
rg -A 3 "deleteEnvironment|softDelete.*environment"
Length of output: 45736
Script:
#!/bin/bash
# Let's check for the specific changes in the release window policy implementation
# Check the release window policy file content
echo "=== Checking release window policy implementation ==="
rg -A 10 "class.*implements.*ReleaseWindowPolicy"
# Look for deletedAt references in environment-related queries
echo "=== Checking deletedAt in environment queries ==="
rg -A 5 "deletedAt.*environment|isNull.*environment"
# Check for environment soft deletion implementation
echo "=== Checking environment soft delete implementation ==="
rg -A 5 "softDelete|deletedAt.*schema"
# Look for environment-related migrations
echo "=== Checking environment migrations ==="
fd -e sql -x cat {}
Length of output: 58748
packages/job-dispatch/src/policies/success-rate-criteria-passing.ts (2)
4-4: LGTM: Import statement cleanup
The consolidated import statement improves code organization by grouping related imports from @ctrlplane/db.
81-83: Verify the removal of deletedAt check
The removal of the deletedAt check means that the policy will now evaluate environments regardless of their deletion status. While this aligns with the changes across other files for ephemeral environments, please ensure this won't cause any unintended side effects.
Let's verify the broader impact:
✅ Verification successful
Let me gather more specific information about the deletedAt checks in the codebase.
Removal of deletedAt check is intentional and consistent
The git history shows that the deletedAt check was deliberately removed from the success rate criteria policy. This aligns with the changes for ephemeral environments, and there's no evidence of similar checks in other policy files. The change simplifies the logic to only check if the release job trigger ID is in the array, without considering the environment's deletion status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining deletedAt checks in environment-related policies
# and verify consistency across the codebase
# Check for any remaining deletedAt checks in policy files
echo "Checking for remaining deletedAt checks in policies..."
rg -l "deletedAt" "packages/job-dispatch/src/policies"
# Check for environment deletion handling patterns
echo "Checking for environment deletion handling patterns..."
ast-grep --pattern 'environment.deletedAt'
Length of output: 25851
Script:
#!/bin/bash
# Search for deletedAt checks specifically in success-rate-criteria-passing.ts and related files
echo "Checking for deletedAt in success-rate-criteria-passing.ts and related files..."
rg -A 5 -B 5 "deletedAt" "packages/job-dispatch/src/policies/success-rate-criteria-passing.ts"
# Check for any git history of deletedAt changes in this file
echo "Checking git history for deletedAt changes..."
git log -p packages/job-dispatch/src/policies/success-rate-criteria-passing.ts | grep -A 5 -B 5 "deletedAt"
# Look for any other policy files that might be handling environment deletion status
echo "Checking other policy files for environment deletion handling..."
rg -A 5 "environment.*deletedAt" "packages/job-dispatch/src/policies/"
Length of output: 1026
apps/jobs/src/index.ts (2)
18-18: LGTM!
The import statement follows the established pattern and correctly uses the .js extension for ESM imports in TypeScript.
26-29: Consider documenting the job's purpose and verifying the schedule frequency.
Running the ephemeral environment checker every minute might be unnecessarily frequent. Please consider:
- Adding a comment explaining the job's purpose and why this frequency was chosen
- Evaluating if a less frequent schedule (e.g., every 5 or 15 minutes) would be sufficient
Let's check the implementation to understand the job's requirements:
✅ Verification successful
Let me gather more information about the ephemeral environment checker implementation.
Let me check the actual implementation of the ephemeral environment checker to understand its purpose and timing requirements.
Running every minute is appropriate for this job
The implementation shows that this job checks and removes expired ephemeral environments. Running every minute is justified because:
- It only processes environments where
expiresAtis in the past - The operation is lightweight (single query + cleanup)
- Timely cleanup is important for resource management
Consider adding this comment above the schedule:
schedule: "* * * * *", // Run every minute to ensure timely cleanup of expired environments🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of ephemeral-env-checker to understand its requirements
# and potential impact of running every minute
# Look for the implementation file
fd --type f "ephemeral-env-checker/index.ts"
# Search for any configuration or environment variables that might affect the schedule
rg -i "schedule|interval|frequency|period" --type ts
Length of output: 5237
Script:
#!/bin/bash
# Search for the ephemeral environment checker implementation
rg -g "!*.test.*" -g "!*.spec.*" "ephemeralEnvChecker" -A 10
# Look for any environment-related configuration
rg -g "!*.test.*" -g "!*.spec.*" "ephemeral.*env" -i
Length of output: 1930
Script:
#!/bin/bash
# Get the full implementation of the ephemeral environment checker
cat apps/jobs/src/ephemeral-env-checker/index.ts
Length of output: 1783
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (2)
79-79: Verify data access patterns and security implications.
The simplified job filtering logic (eq(job.id, params.jobId)) now retrieves jobs regardless of their environment's deletion status. Please ensure this aligns with:
- Data access control requirements
- Security boundaries between environments
- The ephemeral environment initialization objectives
#!/bin/bash
# Check for similar patterns in other job-related endpoints
echo "Checking job-related endpoints for consistent filtering patterns..."
rg -A 5 "from\(job\).*where" --type ts
# Look for potential security implications in tests
echo "Checking test files for deletion-related scenarios..."
rg -l "deletedAt.*test" --type ts4-4: Verify the implications of removing environment deletion checks.
The removal of isNull import and subsequent removal of environment.deletedAt checks represents a significant change in how jobs are filtered. This could result in retrieving jobs from soft-deleted environments, which might be intentional but needs verification.
packages/db/drizzle/meta/_journal.json (1)
200-206: LGTM! Migration entry follows established patterns.
The new migration entry is properly structured with sequential indexing, consistent versioning, and follows the naming convention.
Let's verify the corresponding migration file exists with the expected changes:
✅ Verification successful
Migration file verified and matches expected changes ✅
The migration file 0028_peaceful_otto_octavius.sql exists and contains the expected schema changes:
- Adds
expires_atcolumn toenvironmenttable - Removes
deleted_atcolumn fromenvironmenttable - Additionally removes
ephemeral_durationfromenvironment_policytable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify migration file exists and contains expected changes
# Test: Check if migration file exists and contains expires_at addition and deleted_at removal
# Find and display the migration file content
fd "0028_peaceful_otto_octavius.sql" --type f --exec cat {}
Length of output: 337
packages/job-dispatch/src/policies/release-string-check.ts (1)
5-5: Verify removal of deleted environment filtering
The removal of isNull import suggests that deleted environments are no longer being filtered out. This could lead to processing of deleted environments, which might not be the intended behavior.
Let's verify if this change is consistent with other environment queries in the codebase:
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/EnvironmentDrawer.tsx (1)
53-53: LGTM: Good error handling improvement!
The addition of error extraction from the environment query enhances user feedback capabilities.
packages/db/src/schema/environment.ts (2)
18-21: LGTM: Import changes are well-structured.
The new imports for target validation are properly scoped and align with the schema changes.
45-45: Verify data migration for deletedAt removal.
The addition of expiresAt and removal of deletedAt looks good. However, ensure there's a migration plan for existing soft-deleted records.
Let's verify if there are any remaining references to deletedAt:
apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx (2)
2-2: LGTM: Well-structured schema and imports.
The new schema fields and imports are well-organized and properly constrained for managing environment expiration.
Also applies to: 6-6, 16-23, 31-33
66-86: Verify database integration for expiration handling.
The form submission updates align with the database schema changes. Let's verify the integration:
✅ Verification successful
Based on the verification results, I can now provide a final response:
Database integration for expiration handling is properly implemented
The code changes align with the database schema, as evidenced by:
- Migration
0028_peaceful_otto_octavius.sqladdsexpires_atcolumn to the environment table - The migration also removes legacy
deleted_atandephemeral_durationcolumns, showing proper schema evolution - The form submission code correctly handles the
expiresAtfield using proper date handling and null values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify database schema changes and related code
# Check for environment table schema
rg -A 10 "CREATE TABLE.*environment" --glob "*.sql"
# Check for any other components handling expires_at
rg "expires_at" --type typescript
Length of output: 5799
Script:
#!/bin/bash
# Let's verify the database schema changes and related code with corrected commands
# Check for environment table schema changes related to expires_at
rg -A 10 "CREATE TABLE.*environment" --glob "*.sql" -B 5
# Check for any other components handling expires_at in TypeScript files
rg "expiresAt" --type ts -A 2
# Check for environment schema/model definitions
rg "environment.*schema" --type ts -A 10
Length of output: 31099
Script:
#!/bin/bash
# Let's check for the database migration that added expires_at to the environment table
# Check for migrations adding expires_at
rg "expires_at" --glob "*.sql" -A 2 -B 2
# Check for environment model/schema definition
rg "environment.*=.*{" --type ts -A 15
Length of output: 1697
packages/api/src/router/environment.ts (1)
152-152: Verify the removal of deleted environment filtering.
The removal of the isNull(environment.deletedAt) condition means that bySystemId will now return all environments, including deleted ones. This change could impact downstream components that assume deleted environments are filtered out.
Let's verify the consistency of this change across the codebase:
Please confirm:
- Is this change intentional?
- Has the deletion strategy changed from soft delete to physical delete?
- Have all UI components been updated to handle deleted environments appropriately?
packages/api/src/router/job.ts (2)
Line range hint 644-654: LGTM with a note about deleted environments.
The query implementation looks good:
- Proper pagination with a reasonable limit
- Correct ordering by both job and trigger creation dates ensures consistent results
- However, like the
byDeploymentIdprocedure, this will now include jobs from deleted environments
Let's verify if there are any performance implications:
✅ Verification successful
Let me search for indexes specifically related to the tables and columns used in the query to ensure efficient execution.
Performance impact of the query changes is minimal
The query implementation is efficient because:
- The
target.idcolumn is the primary key and has multiple supporting indexes - The
job.createdAtandreleaseJobTrigger.createdAtare timestamp columns with default values, and while they don't have dedicated indexes, the limit of 1000 records with proper joins will ensure reasonable performance - The removal of the
isNull(environment.deletedAt)check won't significantly impact performance due to:- The 1000 record limit
- The ordering by creation dates which ensures we get the most recent records first
- The small proportion of deleted environments in typical usage
No additional indexes are required as the query's performance characteristics remain acceptable even with the inclusion of deleted environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any indexes on the relevant columns to support this query efficiently
# Search for index definitions
rg -A 3 "CREATE.*INDEX" --type sql
# Search for migration files that might contain index definitions
fd -e sql -x cat {} \; | rg -A 3 "CREATE.*INDEX"
Length of output: 16024
Script:
#!/bin/bash
# Search for indexes on job.createdAt, releaseJobTrigger.createdAt, and target.id
rg "CREATE.*INDEX.*job.*created_at" --type sql
rg "CREATE.*INDEX.*release_job_trigger.*created_at" --type sql
rg "CREATE.*INDEX.*target.*id" --type sql
# Let's also check the table definitions to understand the relationships
fd -e sql -x cat {} \; | rg -A 10 "CREATE TABLE.*(job|release_job_trigger|target)"
Length of output: 8554
Line range hint 311-315: Verify the implications of including deleted environments in job queries.
The removal of the isNull(environment.deletedAt) check means this query will now return job triggers for both active and deleted environments. Please verify:
- If this change is intentional for maintaining historical records
- That downstream consumers can handle jobs from deleted environments correctly
Let's verify the impact:
✅ Verification successful
Let me gather more context about the job-related queries and environment handling:
Let me search for more specific patterns related to job queries and environment handling:
Based on the collected evidence from all the searches, I can now provide a final response:
Removal of environment.deletedAt check is intentional and safe
The removal of the isNull(environment.deletedAt) check from the job query is part of a consistent pattern where:
- Job triggers are meant to maintain historical records, including those from deleted environments
- Job status and execution logic (as seen in
apps/webservice/src/app/api/github/webhook/workflow/handler.ts) operates based on job status rather than environment status - No other queries in the codebase enforce environment deletion status for job-related operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other places in the codebase that might assume jobs are only from active environments
# Search for environment.deletedAt checks in job-related queries
rg -A 3 "environment\.deletedAt" --type ts
# Search for job processing logic that might need to handle deleted environments
ast-grep --pattern 'job.status === $_'
Length of output: 304
Script:
#!/bin/bash
# Search for environment-related joins in job queries
rg -A 5 "from.*environment" --type ts
# Look for other job trigger queries to understand the pattern
rg -A 5 "release_job_trigger" --type ts
# Check if there are any environment validations elsewhere
ast-grep --pattern 'where(eq(environment.deletedAt, null))'
Length of output: 40816
Script:
#!/bin/bash
# Look for job-related queries that might handle deleted environments
rg -A 5 "from.*job.*where" --type ts
# Check for environment-related conditions in job queries
rg -A 5 "environment.*where" --type ts
# Look for job status handling with environment conditions
ast-grep --pattern 'select($$$).from(job).where($$$environment$$$)'
Length of output: 1052
| .then((environment) => NextResponse.json({ environment })) | ||
| .catch((error) => NextResponse.json({ error }, { status: 500 })), |
There was a problem hiding this comment.
🛠️ 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.
| .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 } | |
| ); | |
| }), |
| const body = schema.createEnvironment.extend({ | ||
| expiresAt: z.coerce.date().optional(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add date validation constraints
While the date coercion works, consider adding validation to ensure:
- The expiration date is in the future
- 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.
| 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(), | |
| }); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
| 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)); | ||
| }); |
There was a problem hiding this comment.
🛠️ 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.
| 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 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); | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
| export const run = async () => { | ||
| const ephemeralEnvironments = 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()), | ||
| ), | ||
| ) | ||
| .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); | ||
|
|
||
| for (const { environmentId, targets } of associatedTargets) | ||
| console.log( | ||
| `[${targets.length}] targets are associated with ephemeral environment [${environmentId}]`, | ||
| ); | ||
|
|
||
| const envIds = ephemeralEnvironments.map((env) => env.id); | ||
| await db | ||
| .delete(SCHEMA.environment) | ||
| .where(inArray(SCHEMA.environment.id, envIds)); | ||
| }; |
There was a problem hiding this comment.
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.
| await db | ||
| .delete(SCHEMA.environment) | ||
| .where(inArray(SCHEMA.environment.id, envIds)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
we want to keep the deployments, they are decoupled from the environments
There was a problem hiding this comment.
@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.
|
|
||
| for (const { environmentId, targets } of associatedTargets) | ||
| console.log( | ||
| `[${targets.length}] targets are associated with ephemeral environment [${environmentId}]`, |
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
@adityachoudhari26 you can use the logger lib
There was a problem hiding this comment.
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!
| _.chain(rows) | ||
| .groupBy((e) => e.environment.id) | ||
| .map((env) => ({ | ||
| ...env[0]!.environment, |
There was a problem hiding this comment.
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.
| ...env[0]!.environment, | |
| ...(env[0]?.environment ?? {}), |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/db/drizzle/0029_boring_frank_castle.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/drizzle/meta/_journal.json
🔇 Additional comments (2)
packages/db/drizzle/0029_boring_frank_castle.sql (2)
1-3: LGTM! Migration structure looks good.
The changes appropriately shift from deletion tracking to expiration-based management for ephemeral environments. The use of IF EXISTS clauses provides safe rollout.
1-2: Verify data migration requirements.
Please verify if there's any existing data in the deleted_at column that needs to be migrated to expires_at before dropping the column.
| @@ -0,0 +1,3 @@ | |||
| ALTER TABLE "environment" ADD COLUMN "expires_at" timestamp with time zone DEFAULT NULL;--> statement-breakpoint | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding an index on expires_at.
Since this column will likely be used to query for expired environments, an index would improve query performance.
ALTER TABLE "environment" ADD COLUMN "expires_at" timestamp with time zone DEFAULT NULL;--> statement-breakpoint
+ CREATE INDEX idx_environment_expires_at ON "environment"("expires_at");--> statement-breakpoint📝 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.
| ALTER TABLE "environment" ADD COLUMN "expires_at" timestamp with time zone DEFAULT NULL;--> statement-breakpoint | |
| ALTER TABLE "environment" ADD COLUMN "expires_at" timestamp with time zone DEFAULT NULL;--> statement-breakpoint | |
| CREATE INDEX idx_environment_expires_at ON "environment"("expires_at");--> statement-breakpoint |
| .value(); | ||
|
|
||
| export const run = async () => { | ||
| const ephemeralEnvironments = await db |
There was a problem hiding this comment.
| const ephemeralEnvironments = await db | |
| const expiredEnvironments = await db |
| isNotNull(SCHEMA.environment.expiresAt), | ||
| lte(SCHEMA.environment.expiresAt, new Date()), |
| const currExpiresAt = expiresAt ?? new Date(); | ||
| if (durationUnit === "minutes") | ||
| return addMinutes(currExpiresAt, durationNumber); | ||
| if (durationUnit === "hours") return addHours(currExpiresAt, durationNumber); | ||
| return addDays(currExpiresAt, durationNumber); |
There was a problem hiding this comment.
| 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 }); |
| .then(takeFirst) | ||
| .then((environment) => NextResponse.json({ environment })) | ||
| .catch((error) => { | ||
| console.error(error); |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
apps/webservice/src/app/api/v1/environments/route.ts (1)
45-51: Consider enhancing error loggingWhile the error handling is secure, consider adding more context to the error log for better debugging.
- console.error(error); + console.error('Failed to create environment:', { + error, + systemId: ctx.body.systemId, + userId: ctx.user.id + });packages/ui/src/calendar.tsx (1)
12-17: Consider adding JSDoc comments for better documentation.The props structure is clean and well-organized. Consider adding JSDoc comments to document the purpose of each prop and the component itself.
+/** + * A customizable calendar component wrapping react-day-picker + * @param className - Additional classes to apply to the calendar + * @param classNames - Custom class names for various calendar elements + * @param showOutsideDays - Whether to show days from previous/next months + */ function Calendar({packages/ui/src/datetime-picker.tsx (1)
478-486: Consider making the timeout duration configurableThe hardcoded timeout of 2000ms in the
useEffecthook might not suit all use cases. Making it configurable improves flexibility.Suggestion:
+ const TIMEOUT_DURATION = 2000; React.useEffect(() => { if (flag) { const timer = setTimeout(() => { setFlag(false); - }, 2000); + }, TIMEOUT_DURATION); return () => clearTimeout(timer); } }, [flag]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/jobs/package.json(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx(3 hunks)apps/webservice/src/app/api/v1/environments/route.ts(2 hunks)packages/ui/package.json(1 hunks)packages/ui/src/calendar.tsx(1 hunks)packages/ui/src/datetime-picker.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/jobs/package.json
- apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Overview.tsx
🧰 Additional context used
📓 Learnings (1)
apps/webservice/src/app/api/v1/environments/route.ts (1)
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.632Z
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.
🔇 Additional comments (7)
apps/webservice/src/app/api/v1/environments/route.ts (3)
4-4: LGTM! Clean import additions
The new imports are well-organized and serve clear purposes:
isPresentprovides type-safe null checkstakeFirstsimplifies database result handling
Also applies to: 7-7
15-20: LGTM! Well-implemented schema extension
The schema extension properly handles the expiresAt field with appropriate validation:
- Coerces input to Date type
- Validates future dates
- Correctly handles optional/null case for indefinite environments
33-44: LGTM! Clean database operation implementation
The implementation correctly:
- Handles the optional
expiresAtfield - Uses type-safe null checks
- Simplifies result handling with
takeFirst
packages/ui/package.json (2)
41-41: LGTM: Minor version update for @radix-ui/react-slot
The update from ^1.0.2 to ^1.1.0 is a minor version bump that should maintain backward compatibility.
48-48: Verify the usage of new date-related dependencies
The addition of date-fns and react-day-picker appears to be for the new Calendar component implementation.
Let's verify their usage in the codebase:
Also applies to: 52-52
✅ Verification successful
Dependencies are properly utilized in the codebase
The verification confirms that both date-fns and react-day-picker are actively used:
date-fnsis used indatetime-picker.tsxfor date formatting, locale handling, and date manipulationreact-day-pickeris integrated in bothcalendar.tsxanddatetime-picker.tsxcomponents for the calendar UI implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new dependencies are being used appropriately
# Test 1: Check for Calendar component implementation
echo "Checking for Calendar component implementation..."
fd -e tsx -e ts "calendar" packages/ui/src
# Test 2: Verify date-fns usage (should be used with react-day-picker)
echo "Checking date-fns usage..."
rg "from 'date-fns'" packages/ui/src
# Test 3: Verify react-day-picker usage
echo "Checking react-day-picker usage..."
rg "from 'react-day-picker'" packages/ui/src
Length of output: 466
Script:
#!/bin/bash
# Let's examine the content of the calendar files to verify the usage
echo "Checking calendar.tsx content..."
cat packages/ui/src/calendar.tsx
echo -e "\nChecking date-time-picker/calendar.tsx content..."
cat packages/ui/src/date-time-picker/calendar.tsx
# Let's also check for any imports of these packages with different patterns
echo -e "\nChecking for date-fns imports with different patterns..."
rg "date-fns" packages/ui/src
echo -e "\nChecking for react-day-picker imports with different patterns..."
rg "react-day-picker" packages/ui/src
Length of output: 9031
packages/ui/src/calendar.tsx (2)
1-10: LGTM! Imports and type definitions are well structured.
The imports are properly organized and the type definition correctly extends DayPicker's props.
71-73: LGTM! Component export follows React best practices.
The displayName is properly set and the component is correctly exported.
| components={{ | ||
| Chevron: ({ ...props }) => ( | ||
| <ChevronLeftIcon className="h-4 w-4" {...props} /> | ||
| ), | ||
| }} |
There was a problem hiding this comment.
Add ChevronRightIcon for consistent navigation.
The navigation buttons currently only customize the left chevron. Consider adding the right chevron for visual consistency.
components={{
Chevron: ({ ...props }) => (
<ChevronLeftIcon className="h-4 w-4" {...props} />
),
+ ChevronNext: ({ ...props }) => (
+ <ChevronRightIcon className="h-4 w-4" {...props} />
+ ),
}}📝 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.
| components={{ | |
| Chevron: ({ ...props }) => ( | |
| <ChevronLeftIcon className="h-4 w-4" {...props} /> | |
| ), | |
| }} | |
| components={{ | |
| Chevron: ({ ...props }) => ( | |
| <ChevronLeftIcon className="h-4 w-4" {...props} /> | |
| ), | |
| ChevronNext: ({ ...props }) => ( | |
| <ChevronRightIcon className="h-4 w-4" {...props} /> | |
| ), | |
| }} |
| classNames={{ | ||
| months: "flex flex-col sm:flex-row space-y-4 sm:space-x-4 sm:space-y-0", | ||
| month: "space-y-4", | ||
| caption: "flex justify-center pt-1 relative items-center", | ||
| caption_label: "text-sm font-medium", | ||
| nav: "space-x-1 flex items-center", | ||
| nav_button: cn( | ||
| buttonVariants({ variant: "outline" }), | ||
| "h-7 w-7 bg-transparent p-0 opacity-50 hover:opacity-100", | ||
| ), | ||
| nav_button_previous: "absolute left-1", | ||
| nav_button_next: "absolute right-1", | ||
| table: "w-full border-collapse space-y-1", | ||
| head_row: "flex", | ||
| head_cell: | ||
| "text-muted-foreground rounded-md w-8 font-normal text-[0.8rem]", | ||
| row: "flex w-full mt-2", | ||
| cell: cn( | ||
| "relative p-0 text-center text-sm focus-within:relative focus-within:z-20 [&:has([aria-selected])]:bg-accent [&:has([aria-selected].day-outside)]:bg-accent/50 [&:has([aria-selected].day-range-end)]:rounded-r-md", | ||
| props.mode === "range" | ||
| ? "[&:has(>.day-range-end)]:rounded-r-md [&:has(>.day-range-start)]:rounded-l-md first:[&:has([aria-selected])]:rounded-l-md last:[&:has([aria-selected])]:rounded-r-md" | ||
| : "[&:has([aria-selected])]:rounded-md", | ||
| ), | ||
| day: cn( | ||
| buttonVariants({ variant: "ghost" }), | ||
| "h-8 w-8 p-0 font-normal aria-selected:opacity-100", | ||
| ), | ||
| day_range_start: "day-range-start", | ||
| day_range_end: "day-range-end", | ||
| day_selected: | ||
| "bg-primary text-primary-foreground hover:bg-primary hover:text-primary-foreground focus:bg-primary focus:text-primary-foreground", | ||
| day_today: "bg-accent text-accent-foreground", | ||
| day_outside: | ||
| "day-outside text-muted-foreground opacity-50 aria-selected:bg-accent/50 aria-selected:text-muted-foreground aria-selected:opacity-30", | ||
| day_disabled: "text-muted-foreground opacity-50", | ||
| day_range_middle: | ||
| "aria-selected:bg-accent aria-selected:text-accent-foreground", | ||
| day_hidden: "invisible", | ||
| ...classNames, | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting class configurations for better maintainability.
The classNames configuration is comprehensive but could be more maintainable by extracting the class strings into constants.
+const dayClasses = {
+ selected: "bg-primary text-primary-foreground hover:bg-primary hover:text-primary-foreground focus:bg-primary focus:text-primary-foreground",
+ today: "bg-accent text-accent-foreground",
+ outside: "day-outside text-muted-foreground opacity-50 aria-selected:bg-accent/50 aria-selected:text-muted-foreground aria-selected:opacity-30",
+ disabled: "text-muted-foreground opacity-50",
+};
classNames={{
// ... other classes
day_selected: dayClasses.selected,
day_today: dayClasses.today,
day_outside: dayClasses.outside,
day_disabled: dayClasses.disabled,
// ... remaining classes
}}Committable suggestion was skipped due to low confidence.
packages/ui/src/datetime-picker.tsx
Outdated
| const MONTHS = React.useMemo(() => { | ||
| let locale: Pick<Locale, "options" | "localize" | "formatLong"> = enUS; | ||
| const { options, localize, formatLong } = props.locale ?? {}; | ||
| if (options && localize && formatLong) { | ||
| locale = { | ||
| options, | ||
| localize, | ||
| formatLong, | ||
| }; | ||
| } | ||
| return genMonths(locale); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor locale initialization to remove duplication
The locale initialization logic is repeated in both the Calendar and DateTimePicker components. Refactoring this into a utility function enhances maintainability and reduces code duplication.
Suggestion:
Create a utility function initializeLocale:
function initializeLocale(locale?: Locale): Locale {
if (locale && locale.options && locale.localize && locale.formatLong) {
return {
...enUS,
options: locale.options,
localize: locale.localize,
formatLong: locale.formatLong,
};
}
return enUS;
}Then update the components:
// In Calendar component
- let locale: Pick<Locale, "options" | "localize" | "formatLong"> = enUS;
- const { options, localize, formatLong } = props.locale ?? {};
- if (options && localize && formatLong) {
- locale = {
- options,
- localize,
- formatLong,
- };
- }
+ const locale = initializeLocale(props.locale);
// In DateTimePicker component
- let loc = enUS;
- const { options, localize, formatLong } = locale;
- if (options && localize && formatLong) {
- loc = {
- ...enUS,
- options,
- localize,
- formatLong,
- };
- }
+ const loc = initializeLocale(locale);Also applies to: 754-763
packages/ui/src/datetime-picker.tsx
Outdated
| const YEARS = React.useMemo(() => genYears(yearRange), []); | ||
|
|
There was a problem hiding this comment.
Add missing dependencies to useMemo hooks
Both MONTHS and YEARS use useMemo hooks without including all necessary dependencies, which can lead to stale values when props change.
Ensure all dependencies are included:
// For MONTHS
}, []);
+ }, [props.locale]);
// For YEARS
}, []);
+ }, [yearRange]);Also applies to: 253-265
Include yearRange in the useMemo dependency array
The YEARS calculation depends on yearRange, but the dependency array is empty. This means YEARS won't update if yearRange changes.
Suggestion:
const YEARS = React.useMemo(() => genYears(yearRange), []);
+ }, [yearRange]);Committable suggestion was skipped due to low confidence.
packages/ui/src/datetime-picker.tsx
Outdated
| @@ -0,0 +1,822 @@ | |||
| /* eslint-disable react-hooks/exhaustive-deps */ | |||
There was a problem hiding this comment.
Avoid disabling react-hooks/exhaustive-deps globally
Disabling react-hooks/exhaustive-deps at the top of the file suppresses important warnings about missing dependencies in hooks, which can lead to unintended side effects or bugs. It's better to address the specific instances where dependencies are missing.
Suggestion:
- /* eslint-disable react-hooks/exhaustive-deps */Ensure that all hooks have the correct dependency arrays.
📝 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.
| /* eslint-disable react-hooks/exhaustive-deps */ |
packages/ui/src/datetime-picker.tsx
Outdated
| const handleSelect = (newDay: Date | undefined) => { | ||
| if (!newDay) return; | ||
| if (!value) { | ||
| onChange?.(newDay); | ||
| setMonth(newDay); | ||
| return; | ||
| } | ||
| const diff = newDay.getTime() - value.getTime(); | ||
| const diffInDays = diff / (1000 * 60 * 60 * 24); | ||
| const newDateFull = add(value, { days: Math.ceil(diffInDays) }); | ||
| onChange?.(newDateFull); | ||
| setMonth(newDateFull); | ||
| }; |
There was a problem hiding this comment.
Update month state when value prop changes
The month state is initialized with value, but doesn't update when value changes. This can cause the calendar to display the wrong month if the value prop is updated externally.
Suggestion:
- const [month, setMonth] = React.useState<Date>(value ?? new Date());
+ const [month, setMonth] = React.useState<Date>(value ?? new Date());
+ React.useEffect(() => {
+ if (value) {
+ setMonth(value);
+ }
+ }, [value]);Committable suggestion was skipped due to low confidence.
packages/ui/src/datetime-picker.tsx
Outdated
| const MONTHS = React.useMemo(() => { | ||
| let locale: Pick<Locale, "options" | "localize" | "formatLong"> = enUS; | ||
| const { options, localize, formatLong } = props.locale ?? {}; | ||
| if (options && localize && formatLong) { | ||
| locale = { | ||
| options, | ||
| localize, | ||
| formatLong, | ||
| }; | ||
| } | ||
| return genMonths(locale); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Include props.locale in the useMemo dependency array
The MONTHS calculation depends on props.locale, but the dependency array is empty. If props.locale changes, MONTHS won't recompute, potentially causing incorrect month labels.
Suggestion:
const MONTHS = React.useMemo(() => {
// existing code
- }, []);
+ }, [props.locale]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MONTHS = React.useMemo(() => { | |
| let locale: Pick<Locale, "options" | "localize" | "formatLong"> = enUS; | |
| const { options, localize, formatLong } = props.locale ?? {}; | |
| if (options && localize && formatLong) { | |
| locale = { | |
| options, | |
| localize, | |
| formatLong, | |
| }; | |
| } | |
| return genMonths(locale); | |
| }, []); | |
| const MONTHS = React.useMemo(() => { | |
| let locale: Pick<Locale, "options" | "localize" | "formatLong"> = enUS; | |
| const { options, localize, formatLong } = props.locale ?? {}; | |
| if (options && localize && formatLong) { | |
| locale = { | |
| options, | |
| localize, | |
| formatLong, | |
| }; | |
| } | |
| return genMonths(locale); | |
| }, [props.locale]); | |
packages/ui/src/datetime-picker.tsx
Outdated
| function display12HourValue(hours: number) { | ||
| if (hours === 0 || hours === 12) return "12"; | ||
| if (hours >= 22) return `${hours - 12}`; | ||
| if (hours % 12 > 9) return `${hours}`; | ||
| return `0${hours % 12}`; | ||
| } |
There was a problem hiding this comment.
Correct the logic in display12HourValue function
The display12HourValue function may not correctly convert 24-hour times to 12-hour format for all hour values. The special cases and conditions can result in incorrect hour strings.
Apply this diff to fix the logic:
function display12HourValue(hours: number) {
if (hours === 0 || hours === 12) return "12";
- if (hours >= 22) return `${hours - 12}`;
- if (hours % 12 > 9) return `${hours}`;
- return `0${hours % 12}`;
+ const hour12 = hours % 12;
+ return hour12 < 10 ? `0${hour12}` : `${hour12}`;
}📝 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.
| function display12HourValue(hours: number) { | |
| if (hours === 0 || hours === 12) return "12"; | |
| if (hours >= 22) return `${hours - 12}`; | |
| if (hours % 12 > 9) return `${hours}`; | |
| return `0${hours % 12}`; | |
| } | |
| function display12HourValue(hours: number) { | |
| if (hours === 0 || hours === 12) return "12"; | |
| const hour12 = hours % 12; | |
| return hour12 < 10 ? `0${hour12}` : `${hour12}`; | |
| } |
Summary by CodeRabbit
New Features
expired-env-checker, to manage expired environments.Improvements
EnvironmentDrawerandOverviewcomponents.Bug Fixes
Chores