Fix: show latest release by deployment and environment #169
Conversation
WalkthroughThe changes primarily involve renaming the Changes
Possibly related PRs
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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (6 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/page.tsx (2 hunks)
- packages/api/src/router/release.ts (2 hunks)
🔇 Additional comments (8)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/page.tsx (3)
11-19: LGTM! Clean and well-structured initial setup.The function signature and initial data fetching are well organized, with a clear flow from system to deployments to environments.
47-48: LGTM! Props update is clean and maintains good UX.The component properly handles the enhanced deployment data while maintaining the existing UX patterns.
20-33: 🛠️ Refactor suggestionConsider optimizing the data fetching pattern.
While the implementation works, there are a few areas for improvement:
- The current implementation makes a separate API call for each deployment, which could lead to performance issues with many deployments.
- There's no error handling for failed release fetches.
Let's check if there's a more efficient API endpoint available:
Consider these improvements:
- const deploymentsWithLatestReleases = await Promise.all( - deployments.map(async (deployment) => { - const latestReleases = - await api.release.getLatestByDeploymentAndEnvironments({ - deploymentId: deployment.id, - environmentIds: environments.map((env) => env.id), - }); - - return { - ...deployment, - latestReleases, - }; - }), - ); + const deploymentsWithLatestReleases = await Promise.all( + deployments.map(async (deployment) => { + try { + const latestReleases = + await api.release.getLatestByDeploymentAndEnvironments({ + deploymentId: deployment.id, + environmentIds: environments.map((env) => env.id), + }); + + return { + ...deployment, + latestReleases, + }; + } catch (error) { + console.error( + `Failed to fetch releases for deployment ${deployment.id}:`, + error + ); + return { + ...deployment, + latestReleases: [], + }; + } + }), + );Consider creating a new API endpoint that accepts multiple deployment IDs to fetch all releases in a single request if performance becomes an issue.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (4)
70-70: LGTM: Type enhancement for environment-specific releasesThe addition of
environmentIdto the release type properly supports the requirement to track releases by environment.
Line range hint
95-115: LGTM: Improved conditional rendering logicThe simplified conditional checks make the code more maintainable and easier to understand. The conditions are mutually exclusive and handle all possible states appropriately.
196-196: LGTM: Safe handling of optional releaseThe null coalescing operator ensures safe handling of missing releases while maintaining type safety.
125-131: Verify consumers of DeploymentTable componentThe change from
latestReleasetolatestReleasesarray is a breaking change.Let's verify that all consumers have been updated:
✅ Verification successful
Let me search for more context about the component usage and implementation.
Let me check the content of the page component to see how DeploymentTable is used.
Breaking change has been properly handled
After reviewing the implementation, I can confirm that:
- The
DeploymentTablecomponent's props interface correctly defineslatestReleasesas an array- The page component (
page.tsx) correctly provides the data by fetchinglatestReleasesfor each deployment- The component's internal logic properly handles the array by finding the specific release for each environment using
find()🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DeploymentTable usage to ensure props are updated rg -l "DeploymentTable" | xargs rg "latestRelease[^s]" -A 5Length of output: 1667
Script:
#!/bin/bash # Search for DeploymentTable component definition and props interface ast-grep --pattern 'interface $_ { $$$ latestRelease$_?: $_ $$$ }' # Also check for any other files importing/using DeploymentTable rg -l "import.*DeploymentTable"Length of output: 190
Script:
#!/bin/bash # Check the page component implementation cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/page.tsx" # Also search for any props type definitions in the TableDeployments file cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx"Length of output: 8387
packages/api/src/router/release.ts (1)
45-45: Verify if public access is intended for release information.The addition of
publicProceduremakes the new endpoint publicly accessible without authentication. Consider if this aligns with security requirements, as release information might be sensitive.Consider using
protectedProcedureinstead to maintain consistent authentication across release-related endpoints.✅ Verification successful
Let me check the specific endpoint that uses
publicProcedureto make a complete assessment.
Let me check if this endpoint is used in any public-facing components or features to understand the context better.
Let me check if the page component that uses this endpoint is already protected by authentication at the route level.
Let me check for authentication patterns in the Next.js app structure.
Public endpoint is acceptable due to route-level authentication
The endpoint is used in a page that's already protected by authentication at the workspace layout level (
[workspaceSlug]/layout.tsx). The layout explicitly checks for session and redirects to/loginif not authenticated, making the public procedure safe in this context.
- Authentication is enforced in
[workspaceSlug]/layout.tsxwithif (session == null) redirect("/login")- The endpoint is only used within this authenticated route context
- All parent routes leading to the deployment page are protected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other release-related endpoints use authentication rg -A 1 "protectedProcedure" packages/api/src/router/release.tsLength of output: 526
Script:
#!/bin/bash # Check for the usage of publicProcedure in release.ts rg -A 5 "publicProcedure" packages/api/src/router/release.tsLength of output: 470
Script:
#!/bin/bash # Search for usage of getLatestByDeploymentAndEnvironments across the codebase rg -A 3 "getLatestByDeploymentAndEnvironments" --type tsLength of output: 830
Script:
#!/bin/bash # Check for authentication middleware or protected routes in the app directory rg -B 3 "\[workspaceSlug\]/systems/\[systemSlug\]/deployments" apps/webservice/src/middleware.tsLength of output: 172
Script:
#!/bin/bash # Look for authentication patterns in the app layout or middleware fd -e ts -e tsx middleware rg -A 3 "auth" apps/webservice/src/app/layout.tsx rg -A 3 "auth" apps/webservice/src/app/\[workspaceSlug\]/layout.tsxLength of output: 695
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx
Outdated
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx
Outdated
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/deployment.ts (1)
Line range hint
311-328: Consider extracting common query pattern.The query structure and result mapping in
byWorkspaceIdduplicates the pattern used inbySystemId. Consider extracting this common pattern into a reusable function to improve maintainability.Example refactor:
const getDeploymentsWithLatestRelease = ( db: Tx, whereClause: SQL ) => { const latestRelease = latestActiveReleaseSubQuery(db); return db .select() .from(deployment) .leftJoin( latestRelease, and( eq(latestRelease.deploymentId, deployment.id), eq(latestRelease.rank, 1), ), ) .where(whereClause) .then((r) => r.map((row) => ({ ...row.deployment, latestRelease: row.active_release, })), ); };Then use it in both procedures:
// in bySystemId return getDeploymentsWithLatestRelease( ctx.db, eq(deployment.systemId, input) ); // in byWorkspaceId return getDeploymentsWithLatestRelease( ctx.db, eq(system.workspaceId, input) );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/api/src/router/deployment.ts (6 hunks)
🔇 Additional comments (2)
packages/api/src/router/deployment.ts (2)
Line range hint
207-223: LGTM! Consistent updates to use the new active release query.The changes correctly integrate with the renamed subquery and properly map the results using the new
active_releasealias.
Line range hint
33-49: LGTM! Improved query structure for environment-specific latest releases.The changes correctly implement environment-based filtering and improve naming clarity. The
ROW_NUMBER()partitioning by bothdeploymentIdandenvironmentIdensures we get the correct latest release for each deployment-environment combination.Let's verify if excluding releases without job triggers is intentional:
✅ Verification successful
Let me gather more information about the relationship between releases and job triggers.
Let me check one more thing about the job trigger creation pattern.
Based on the evidence gathered, I can now provide the final response:
Inner join with releaseJobTrigger is intentional and correct
The code shows that job triggers are consistently created alongside releases through the
createReleaseJobTriggersfunction, which is called in all release creation flows:
- New releases via API (
/releasesroute)- Force deployments
- Configuration updates
- New target additions
The inner join in
latestActiveReleaseSubQuerycorrectly filters for releases that should be actively considered for deployment in specific environments, as every valid release-environment combination will have a corresponding job trigger.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any releases without job triggers that might be unintentionally excluded rg -A 5 "INSERT INTO.*release" rg -A 5 "CREATE TABLE.*release"Length of output: 2603
Script:
#!/bin/bash # Check for code patterns that create releases and job triggers ast-grep --pattern 'async function $FUNC($_) { $$$ release$$$ $$$ }' # Also check for direct references to release creation rg -B3 -A3 "insert.*release.*values"Length of output: 1723
Script:
#!/bin/bash # Check the createReleaseJobTriggers function and its usage rg -B5 -A5 "createReleaseJobTriggers" # Also check for any direct job trigger insertions rg -B3 -A3 "insert.*release_job_trigger"Length of output: 8793
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/deployment.ts (1)
333-333: Inconsistent property naming: 'latestRelease' vs 'activeReleases'In the
byWorkspaceIdquery, the property is namedlatestReleaseand holds a singleactive_release, while in thebySystemIdquery, the property isactiveReleasesand contains an array ofactive_releaseobjects. This inconsistency in property names and data structures may cause confusion for consumers of these APIs. For consistency, consider standardizing the property name and structure across both queries.Suggestion:
- If multiple active releases are possible and relevant in both contexts, consider using
activeReleasesas an array in both queries.- If only a single active release is expected, consider using
latestReleaseas a singular property in both queries.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (3 hunks)
- packages/api/src/router/deployment.ts (6 hunks)
🔇 Additional comments (6)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (4)
125-131: LGTM: Type definition for activeReleases is well-structured.The new type structure properly represents the relationship between releases and environments, allowing for multiple active releases across different environments.
182-184: LGTM: Clean implementation of release lookup with proper null handling.The find operation with null coalescing operator provides a clean and safe way to lookup releases by environment.
196-196: LGTM: Release prop passing is correctly updated.The release prop passing aligns with the new activeReleases structure and maintains proper null handling.
125-131: Verify the impact of release structure changes across the codebase.The change from
latestReleasetoactiveReleasesis significant. Let's verify that all components are updated to handle the new structure.Also applies to: 182-184, 196-196
✅ Verification successful
Release structure changes have been properly implemented across the codebase
The verification shows that the transition from
latestReleasetoactiveReleaseshas been handled consistently:
- The API layer (
packages/api/src/router/deployment.ts) has been updated to map the active releases correctly- The only remaining
latestReleasereferences are in different contexts:
job-dispatchpackage uses it for different query logicCreateReleasecomponent uses it for release listingDependencyDiagramuses it for dependency visualizationThe new
activeReleasesstructure is only used in the deployment table component and its corresponding API route, indicating this is an intentional, isolated change for this specific feature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to latestRelease that might need updating rg "latestRelease" --type typescript --type tsx # Search for components that might be consuming the deployment type ast-grep --pattern 'type { Deployment }'Length of output: 125
Script:
#!/bin/bash # Search for latestRelease references with correct file extensions rg "latestRelease" -t ts -t tsx # Search for activeReleases to see current usage rg "activeReleases" -t ts -t tsx # Search for Deployment type definitions and usages rg "type\s+Deployment" -t ts -t tsx rg "interface\s+Deployment" -t ts -t tsxLength of output: 257
Script:
#!/bin/bash # Search for latestRelease references rg "latestRelease" # Search for activeReleases to see current usage rg "activeReleases" # Search for Deployment type definitions and usages rg "type\s+Deployment" rg "interface\s+Deployment"Length of output: 3210
packages/api/src/router/deployment.ts (2)
2-2: Imported lodash correctlyThe addition of
import _ from "lodash";is necessary for utilizing lodash functions in the code below.
Line range hint
35-52: Ensure inner join does not exclude necessary releasesBy introducing an
innerJoinwithreleaseJobTriggerin thelatestActiveReleaseSubQuery, releases without an associatedreleaseJobTriggerwill be excluded from the results. Please verify that all necessary releases have corresponding entries inreleaseJobTrigger, and that this exclusion is intentional.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/deployment.ts (1)
Line range hint
35-52: LGTM! Consider adding SQL comments for clarity.The refactoring improves clarity by:
- Better function naming that reflects its purpose
- Correctly partitioning by both deploymentId and environmentId
- Using inner join to ensure only active releases are returned
Consider adding SQL comments to document the ranking logic for future maintainability.
const latestActiveReleaseSubQuery = (db: Tx) => db .select({ id: release.id, deploymentId: release.deploymentId, version: release.version, createdAt: release.createdAt, name: release.name, config: release.config, environmentId: releaseJobTrigger.environmentId, + // Rank releases by creation date, separately for each deployment-environment pair rank: sql<number>`ROW_NUMBER() OVER (PARTITION BY ${release.deploymentId}, ${releaseJobTrigger.environmentId} ORDER BY ${release.createdAt} DESC)`.as( "rank", ), })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/api/src/router/deployment.ts (6 hunks)
🔇 Additional comments (2)
packages/api/src/router/deployment.ts (2)
222-229:⚠️ Potential issueAdd null safety check and type annotations.
- The existing concern about null safety with
t[0]!is still valid.- Consider adding type annotations to improve code maintainability.
Apply these changes:
- .then((ts) => - _.chain(ts) - .groupBy((t) => t.deployment.id) - .map((t) => ({ - ...t[0]!.deployment, - activeReleases: t.map((a) => a.active_releases).filter(isPresent), - })) - .value(), + .then((ts) => + _.chain(ts) + .groupBy((t) => t.deployment.id) + .map((t): DeploymentWithReleases => ({ + ...(t[0]?.deployment ?? {}), + activeReleases: t.map((a) => a.active_releases).filter(isPresent), + })) + .value(),
Line range hint
35-334: Verify consumers handle the new activeReleases structure.The changes modify how releases are returned (single vs array) and introduce environment-specific filtering. Let's verify that all consumers handle this correctly.
✅ Verification successful
No issues found with the release structure changes
The code inspection shows that all consumers are already properly handling both the
latestReleaseandactiveReleasesstructures:
DependencyDiagram.tsxcorrectly handles nullablelatestReleasewith proper type definitions and null checksTableDeployments.tsxproperly handles the array-basedactiveReleasesstructure with environment-specific filteringCreateRelease.tsxuses the release list API which is unaffected by these changesrelease-job-trigger.tsuses its own query construction and is compatible with both structures🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for consumers of deployment data that might need updates echo "Searching for components using deployment data..." rg -l "latestRelease|activeReleases" --type tsx --type ts echo "Searching for direct SQL queries that might be affected..." rg -l "FROM.*release.*JOIN.*release_job_trigger" --type tsLength of output: 384
Script:
#!/bin/bash # Let's try a more targeted search without file type restrictions echo "Searching for components using deployment data..." rg -l "latestRelease|activeReleases" echo -e "\nSearching for direct SQL queries..." rg -l "FROM.*release.*JOIN.*release_job_trigger" echo -e "\nLooking for deployment-related components..." fd -e tsx -e ts | rg -l "deployment|release"Length of output: 833
Script:
#!/bin/bash # Let's examine how these components handle the release data structure echo "Checking DependencyDiagram.tsx usage..." rg -A 5 -B 5 "latestRelease|activeReleases" "apps/webservice/src/app/[workspaceSlug]/dependencies/DependencyDiagram.tsx" echo -e "\nChecking TableDeployments.tsx usage..." rg -A 5 -B 5 "latestRelease|activeReleases" "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx" echo -e "\nChecking CreateRelease.tsx usage..." rg -A 5 -B 5 "latestRelease|activeReleases" "apps/webservice/src/app/[workspaceSlug]/_components/CreateRelease.tsx" echo -e "\nChecking release-job-trigger.ts usage..." rg -A 5 -B 5 "latestRelease|activeReleases" "packages/job-dispatch/src/release-job-trigger.ts"Length of output: 5136
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/dependencies/page.tsx (1)
22-28: Consider adding TypeScript types for better type safety.The transformation logic is correct, but adding explicit types would make the code more maintainable and catch potential issues at compile time.
Consider adding types:
interface Release { id: string; version: string; } interface Deployment { latestActiveReleases: Release | null; // ... other deployment fields } interface TransformedDeployment { latestActiveRelease: Pick<Release, 'id' | 'version'> | null; // ... other deployment fields }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- apps/webservice/src/app/[workspaceSlug]/dependencies/DependencyDiagram.tsx (5 hunks)
- apps/webservice/src/app/[workspaceSlug]/dependencies/page.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (3 hunks)
- packages/api/src/router/deployment.ts (5 hunks)
- packages/job-dispatch/src/release-job-trigger.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/job-dispatch/src/release-job-trigger.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx
🔇 Additional comments (7)
apps/webservice/src/app/[workspaceSlug]/dependencies/page.tsx (1)
32-32: Verify Diagram component compatibility with the new data structure.Ensure that the Diagram component has been updated to handle the new
latestActiveReleaseproperty instead of the previous structure.✅ Verification successful
Looking at the DependencyDiagram.tsx file, I can see that the component is already properly handling the
latestActiveReleaseproperty in its type definition and implementation. The DeploymentNode component specifically accepts and uses this property:const DeploymentNode: React.FC< NodeProps< Deployment & { latestActiveRelease: { id: string; version: string } | null } > > = ({ data }) => { // ...uses data.latestActiveRelease }And the DependencyDiagram component also accepts the same type:
const DependencyDiagram: React.FC<{ deployments: Array< Deployment & { latestActiveRelease: { id: string; version: string } | null } >; }> = ({ deployments }) => { // ... }The Diagram component is already compatible with the new data structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Diagram component is updated to handle the new structure # Search for the Diagram component definition and its props interface ast-grep --pattern 'interface $props { $$$ deployments: $type $$$ }' # Check for any references to latestRelease in the Diagram component rg -A 5 'latestRelease' --type typescriptLength of output: 153
Script:
#!/bin/bash # Let's try different approaches to find the Diagram component and its usage # Search for Diagram component definition ast-grep --pattern 'const Diagram = $_' # Search for Diagram interface or type definition ast-grep --pattern 'type DiagramProps = { $$$ }' # Search for files containing Diagram component rg -l "Diagram.*props|Diagram.*interface|Diagram.*type" # Find files named Diagram fd -e tsx -e ts "Diagram"Length of output: 685
Script:
#!/bin/bash # Let's examine the DependencyDiagram.tsx since it's in the dependencies folder cat "apps/webservice/src/app/[workspaceSlug]/dependencies/DependencyDiagram.tsx" # Also check for any type definitions or interfaces in this file ast-grep --pattern 'type $_ = { $$$ }' ast-grep --pattern 'interface $_ { $$$ }'Length of output: 103913
apps/webservice/src/app/[workspaceSlug]/dependencies/DependencyDiagram.tsx (4)
157-157: LGTM! State management changes are consistent.The updates to state initialization, dependency checks, and prop passing maintain consistency with the new
latestActiveReleaseproperty.Also applies to: 166-166, 196-196
225-225: LGTM! Type definition is consistent.The type signature update in the
DependencyDiagramcomponent maintains consistency with the newlatestActiveReleaseproperty.
265-265: LGTM! Type definition is consistent.The type signature update in the
Diagramcomponent maintains consistency with the newlatestActiveReleaseproperty.
151-151: Verify the impact of the breaking type change.The type signature change from
latestReleasetolatestActiveReleaseis consistent with the API changes. However, we should ensure all consumers of this component are updated accordingly.✅ Verification successful
After analyzing the file content, I can now generate the final response:
Type change is self-contained and properly handled
The
latestActiveReleasetype change is correctly implemented within the same file where it's used. TheDeploymentNodecomponent and its parent components (DependencyDiagramandDiagram) all consistently use the new type. The component is only used internally within this file, and all its usages have been properly updated to handle the new prop name.
- The type is used consistently in the component props interface (line 151)
- The prop is correctly accessed as
data.latestActiveReleasewithin the component- Parent components
DependencyDiagramandDiagrampass the correct prop type- No external usages of
DeploymentNodeexist as it's a private component defined and used only within this file🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of DeploymentNode to verify they've been updated # to use the new latestActiveRelease prop. rg -l "DeploymentNode" | xargs rg "latestRelease"Length of output: 47
Script:
#!/bin/bash # Let's try a different approach to find usages of DeploymentNode component # First, find the file containing DeploymentNode fd "DependencyDiagram.tsx" # Then let's look at its content to understand the component structure cat $(fd "DependencyDiagram.tsx")Length of output: 7818
packages/api/src/router/deployment.ts (2)
Line range hint
35-52: Well-structured query with improved environment-based filtering!The changes to this query are well thought out:
- The inner join with releaseJobTrigger ensures only releases with active triggers are included
- Partitioning by both deploymentId and environmentId provides correct release ranking per environment
Line range hint
35-52: Verify consumers handle the new release structureThe changes to the release query structure and property names might affect consumers of this API.
Let's verify the consumers:
Also applies to: 210-229, 317-334
| @@ -15,13 +15,21 @@ export default async function Dependencies({ | |||
|
|
|||
| if ( | |||
| deployments.length === 0 || | |||
| deployments.some((d) => d.latestRelease != null) | |||
| deployments.some((d) => d.latestActiveReleases != null) | |||
There was a problem hiding this comment.
Fix incorrect condition logic for showing getting started guide.
The current condition shows the getting started guide when there ARE active releases, which seems backwards. The getting started guide should typically be shown when there are no active releases.
Apply one of these fixes:
- deployments.some((d) => d.latestActiveReleases != null)
+ !deployments.some((d) => d.latestActiveReleases != null)Or alternatively:
- deployments.some((d) => d.latestActiveReleases != null)
+ deployments.every((d) => d.latestActiveReleases == null)📝 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.
| deployments.some((d) => d.latestActiveReleases != null) | |
| !deployments.some((d) => d.latestActiveReleases != null) |
| r.map((row) => ({ | ||
| ...row.deployment, | ||
| latestActiveReleases: row.active_releases, | ||
| })), |
There was a problem hiding this comment.
Fix inconsistent property naming
The property name latestActiveReleases is inconsistent with activeReleases used in the bySystemId procedure. This could cause confusion and maintenance issues.
Apply this diff to maintain consistency:
r.map((row) => ({
...row.deployment,
- latestActiveReleases: row.active_releases,
+ activeReleases: row.active_releases,
})),📝 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.
| r.map((row) => ({ | |
| ...row.deployment, | |
| latestActiveReleases: row.active_releases, | |
| })), | |
| r.map((row) => ({ | |
| ...row.deployment, | |
| activeReleases: row.active_releases, | |
| })), |
| .then((ts) => | ||
| _.chain(ts) | ||
| .groupBy((t) => t.deployment.id) | ||
| .map((t) => ({ | ||
| ...t[0]!.deployment, | ||
| activeReleases: t.map((a) => a.active_releases).filter(isPresent), | ||
| })) | ||
| .value(), |
There was a problem hiding this comment.
Handle potential empty groups to prevent runtime errors
The use of t[0]! assumes that each group has at least one element. This could lead to runtime errors if t is empty.
Apply this diff to add a null check:
.groupBy((t) => t.deployment.id)
.map((t) => ({
- ...t[0]!.deployment,
+ ...(t[0]?.deployment ?? {}),
activeReleases: t.map((a) => a.active_releases).filter(isPresent),
}))📝 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((ts) => | |
| _.chain(ts) | |
| .groupBy((t) => t.deployment.id) | |
| .map((t) => ({ | |
| ...t[0]!.deployment, | |
| activeReleases: t.map((a) => a.active_releases).filter(isPresent), | |
| })) | |
| .value(), | |
| .then((ts) => | |
| _.chain(ts) | |
| .groupBy((t) => t.deployment.id) | |
| .map((t) => ({ | |
| ...(t[0]?.deployment ?? {}), | |
| activeReleases: t.map((a) => a.active_releases).filter(isPresent), | |
| })) | |
| .value(), |
Summary by CodeRabbit
New Features
DeploymentTablecomponent to handle multiple active releases per deployment, improving flexibility in release management.Diagramcomponent to utilize active releases, enhancing rendering logic.Bug Fixes