New environments page view #384
Conversation
WalkthroughThis pull request introduces several enhancements across multiple components related to environments and insights. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ED as EnvironmentDropdown Component
participant CB as Clipboard API
participant T as Toast Notification
U->>ED: Click "Copy ID"
ED->>CB: Execute copyEnvironmentId()
CB-->>ED: Confirm copy action
ED->>T: Trigger success toast
T-->>U: Display notification
sequenceDiagram
participant U as User
participant EC as EnvironmentCard Component
participant AR as API Resource Query
participant AS as API Health Stats Query
U->>EC: Request rendering of Environment Card
EC->>AR: Fetch resource data
AR-->>EC: Return resource information
EC->>AS: Fetch health stats
AS-->>EC: Return health metrics
EC->>U: Render card with metrics & failure rates
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
still need to connect to trpc endpoints |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (1)
44-50: Add error handling to clipboard operation.The clipboard operation might fail in some browsers or contexts. Consider adding error handling to provide feedback when copying fails.
const copyEnvironmentId = () => { - navigator.clipboard.writeText(environment.id); - toast.success("Environment ID copied", { - description: environment.id, - duration: 2000, - }); + navigator.clipboard.writeText(environment.id) + .then(() => { + toast.success("Environment ID copied", { + description: environment.id, + duration: 2000, + }); + }) + .catch((error) => { + toast.error("Failed to copy Environment ID", { + description: "Please try again or copy manually", + duration: 2000, + }); + }); };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (4)
222-234: Add error handling to clipboard operation.Similar to the
EnvironmentDropdowncomponent, this clipboard operation should include error handling. Consider using Promise-based error handling for consistent user feedback when copying fails.onClick={useCallback( (e: React.MouseEvent<HTMLButtonElement>) => { e.preventDefault(); - navigator.clipboard.writeText(environment.id); - toast.success("Environment ID copied to clipboard", { - description: environment.id, - duration: 2000, - icon: <IconCheck className="h-4 w-4" />, - }); + navigator.clipboard.writeText(environment.id) + .then(() => { + toast.success("Environment ID copied to clipboard", { + description: environment.id, + duration: 2000, + icon: <IconCheck className="h-4 w-4" />, + }); + }) + .catch((error) => { + toast.error("Failed to copy Environment ID", { + description: "Please try again or copy manually", + duration: 2000, + }); + }); }, [environment.id], )}
98-335: Consider breaking down the EnvironmentCard component.The
EnvironmentCardcomponent is quite large (over 200 lines). Consider extracting some logical sections into separate components for better maintainability:
- VersionDistribution
- ResourceHealthIndicator
- EnvironmentStatusBadge
This would improve code readability and make future maintenance easier.
317-327: Document the reasoning for 6 segments in health bar.The resource health bar uses a static count of 6 segments without explanation. Consider adding a comment explaining why 6 segments were chosen and how the health calculation works for future maintainers.
<div className="flex gap-1"> + {/* Using 6 segments to provide a granular visualization of health ratio */} {Array.from({ length: 6 }).map((_, i) => ( <div key={i} className={cn( "h-1.5 w-4 rounded-full", i < (healthyCount * 6) / totalCount ? "bg-green-500" : "bg-red-500", )} /> ))} </div>
44-50: DRY violation: Duplicate clipboard functionality.The clipboard functionality is implemented separately in both
EnvironmentDropdownandEnvironmentCardcomponents. Consider extracting this to a shared utility function.Create a shared utility for clipboard operations:
// utils/clipboard.ts import { toast } from "@ctrlplane/ui/toast"; import { IconCheck } from "@tabler/icons-react"; export const copyToClipboard = ( text: string, successMessage: string = "Copied to clipboard", description?: string ) => { navigator.clipboard.writeText(text) .then(() => { toast.success(successMessage, { description: description || text, duration: 2000, icon: <IconCheck className="h-4 w-4" />, }); }) .catch((error) => { toast.error("Failed to copy to clipboard", { description: "Please try again or copy manually", duration: 2000, }); }); };Then use it in both components:
// In EnvironmentDropdown import { copyToClipboard } from "~/utils/clipboard"; const copyEnvironmentId = () => { copyToClipboard( environment.id, "Environment ID copied", environment.id ); }; // In EnvironmentCard import { copyToClipboard } from "~/utils/clipboard"; // Inside onClick callback e.preventDefault(); copyToClipboard( environment.id, "Environment ID copied to clipboard", environment.id );Also applies to: 222-234
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsx (3)
9-9: Component import updated appropriately.The import change from
EnvironmentRowtoEnvironmentCardaligns with the UI upgrade in this PR.
31-35: Well-structured responsive grid layout.The grid layout implementation with responsive breakpoints (md and lg) provides a good visual experience across different screen sizes. The gap and padding settings create appropriate spacing between cards.
36-42: Good empty state handling.The empty state message with styled border provides clear feedback to users when no environments exist, with a helpful prompt to create one.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (1)
55-108: Well-organized dropdown menu structure.The dropdown menu is well-organized with logical grouping of related actions. The use of icons enhances usability by providing visual cues for each action. The cursor style and gap settings create a consistent, polished appearance.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (2)
119-128: Replace mock data with real API data.The component is using mock data for version distribution and failure rate. This should be replaced with actual API calls before production deployment.
Can you confirm if there's an API endpoint planned for fetching this data, or if this is intended to remain as mock data for the initial implementation?
83-91: Good color selection for distribution visualization.The color palette chosen for the version distribution provides good visual distinction between versions while maintaining aesthetic appeal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (1)
50-56: Add error handling for clipboard operationsThe clipboard API might fail in some browsers or environments. Consider adding a try/catch block to handle potential errors when copying to clipboard.
const copyEnvironmentId = () => { + try { navigator.clipboard.writeText(environment.id); toast.success("Environment ID copied", { description: environment.id, duration: 2000, }); + } catch (error) { + toast.error("Failed to copy environment ID", { + description: "Please copy manually: " + environment.id, + duration: 3000, + }); + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx(2 hunks)apps/webservice/src/app/urls.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/urls.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (2)
44-48: URL construction looks goodUsing the URL builder pattern with route parameters ensures consistent and maintainable navigation throughout the application.
61-135: Well-structured dropdown menu organizationThe dropdown menu is logically organized with appropriate grouping, icons, and separators. The navigation options provide a comprehensive set of actions for environment management.
apps/webservice/src/app/urls.ts (1)
86-86: LGTM: Settings URL method follows established patternThe added
settingsmethod follows the consistent pattern used throughout the file, properly utilizing thebuildUrlfunction with the correct base path segments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (4)
201-212: Move useCallback outside of JSX definitionThe
useCallbackhook is being used directly within the JSX, which is not the recommended pattern. Instead, define the callback at the component level to improve readability and performance.export const EnvironmentCard: React.FC<{ environment: Environment; }> = ({ environment }) => { const { workspaceSlug, systemSlug } = useParams<{ workspaceSlug: string; systemSlug: string; }>(); const allResourcesQ = api.resource.byWorkspaceId.list.useQuery( { workspaceId: environment.system.workspaceId, filter: environment.resourceFilter ?? undefined, limit: 0, }, { enabled: environment.resourceFilter != null }, ); const unhealthyResourcesQ = api.environment.stats.unhealthyResources.useQuery( environment.id, ); + const handleCopyId = useCallback((e: React.MouseEvent<HTMLButtonElement>) => { + e.preventDefault(); + navigator.clipboard.writeText(environment.id); + toast.success("Environment ID copied to clipboard", { + description: environment.id, + duration: 2000, + icon: <IconCheck className="h-4 w-4" />, + }); + }, [environment.id]); // ...rest of componentThen use
handleCopyIdin the button's onClick prop.
90-111: Consider reusing the existing EnvironmentHealth componentThe
EnvironmentCardduplicates queries and logic from the existingEnvironmentHealthcomponent. Consider reusing theEnvironmentHealthor at least extracting the shared logic into a custom hook to avoid duplication.export const EnvironmentCard: React.FC<{ environment: Environment; }> = ({ environment }) => { const { workspaceSlug, systemSlug } = useParams<{ workspaceSlug: string; systemSlug: string; }>(); - const allResourcesQ = api.resource.byWorkspaceId.list.useQuery( - { - workspaceId: environment.system.workspaceId, - filter: environment.resourceFilter ?? undefined, - limit: 0, - }, - { enabled: environment.resourceFilter != null }, - ); - - const unhealthyResourcesQ = api.environment.stats.unhealthyResources.useQuery( - environment.id, - ); + // Custom hook to share logic with EnvironmentHealth + const { + healthyCount, + totalCount, + unhealthyCount, + isLoading + } = useEnvironmentHealthMetrics(environment);
224-232: Add loading state for the release failure rateThe failure rate rendering doesn't account for a loading state or potential errors when the data would come from an actual API call rather than hardcoded data.
<span className="text-sm font-medium"> - {latestReleaseFailureRate > 5 ? ( + {releaseStatsQuery.isLoading ? ( + <Skeleton className="h-4 w-20 rounded-full" /> + ) : releaseStatsQuery.error ? ( + <span className="text-muted-foreground">Error loading data</span> + ) : latestReleaseFailureRate > 5 ? ( <span className="text-red-400"> {latestReleaseFailureRate}% failure rate </span> ) : ( <span className="text-green-400"> {latestReleaseFailureRate}% failure rate </span> )} </span>
82-239: Error handling for clipboard operationsThe code attempts to copy the environment ID to the clipboard but doesn't handle potential errors that could occur during this operation. While clipboard operations are generally reliable in modern browsers, it's still a good practice to handle potential failures.
const handleCopyId = useCallback((e: React.MouseEvent<HTMLButtonElement>) => { e.preventDefault(); - navigator.clipboard.writeText(environment.id); - toast.success("Environment ID copied to clipboard", { - description: environment.id, - duration: 2000, - icon: <IconCheck className="h-4 w-4" />, - }); + navigator.clipboard.writeText(environment.id) + .then(() => { + toast.success("Environment ID copied to clipboard", { + description: environment.id, + duration: 2000, + icon: <IconCheck className="h-4 w-4" />, + }); + }) + .catch((err) => { + console.error("Failed to copy: ", err); + toast.error("Failed to copy environment ID", { + duration: 2000, + }); + }); }, [environment.id]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (1)
120-239: The new EnvironmentCard component looks well-structured and provides a rich UIThe card-based layout provides a more comprehensive view of environment status than the previous row implementation. It includes key metrics like resource counts, health status, environment ID (with copy functionality), and failure rates, making the interface more informative and user-friendly.
...aceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (2)
84-95: Consider refactoring duplicated API queriesThe EnvironmentCard component duplicates the same API queries that are already defined in the EnvironmentHealth component. Consider extracting these queries into a custom hook or shared utility to avoid duplicating this logic.
// Create a custom hook +export function useEnvironmentResources(environment: Environment) { + const allResourcesQ = api.resource.byWorkspaceId.list.useQuery( + { + workspaceId: environment.system.workspaceId, + filter: environment.resourceFilter ?? undefined, + limit: 0, + }, + { enabled: environment.resourceFilter != null }, + ); + + const unhealthyResourcesQ = api.environment.stats.unhealthyResources.useQuery( + environment.id, + ); + + const unhealthyCount = unhealthyResourcesQ.data?.length ?? 0; + const totalCount = allResourcesQ.data?.total ?? 0; + const healthyCount = totalCount - unhealthyCount; + + return { + allResourcesQ, + unhealthyResourcesQ, + totalCount, + healthyCount, + unhealthyCount, + isLoading: unhealthyResourcesQ.isLoading || allResourcesQ.isLoading + }; +} // Then in your components: -const allResourcesQ = api.resource.byWorkspaceId.list.useQuery(...) -const unhealthyResourcesQ = api.environment.stats.unhealthyResources.useQuery(...) -const unhealthyCount = unhealthyResourcesQ.data?.length ?? 0; -const totalCount = allResourcesQ.data?.total ?? 0; -const healthyCount = totalCount - unhealthyCount; +const { totalCount, healthyCount, unhealthyCount, isLoading } = useEnvironmentResources(environment);
214-228: Consider conditionally rendering the latest release sectionThe component always shows the latest release failure rate. Consider conditionally rendering this section only when the data is available to handle cases where there might not be any release data.
<div className="flex justify-between"> <span className="text-sm text-muted-foreground"> Latest Release </span> <span className="text-sm font-medium"> - {latestReleaseFailureRate > 5 ? ( + {latestReleaseFailureRate !== undefined ? ( + latestReleaseFailureRate > 5 ? ( <span className="text-red-400"> {latestReleaseFailureRate}% failure rate </span> ) : ( <span className="text-green-400"> {latestReleaseFailureRate}% failure rate </span> + ) + ) : ( + <span className="text-neutral-400">No data available</span> )} </span> </div>apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (3)
22-32: Consider adding error handling for API callsThe data fetching looks good, but there's no error handling for failed API calls. Consider adding error states to provide feedback to users when data fetching fails.
- const { data: system, isLoading: isLoadingSystem } = api.system.byId.useQuery(systemId); - const { data: resources, isLoading: isLoadingResources } = api.system.resources.useQuery({ + const { data: system, isLoading: isLoadingSystem, error: systemError } = api.system.byId.useQuery(systemId); + const { data: resources, isLoading: isLoadingResources, error: resourcesError } = api.system.resources.useQuery({ systemId, limit: 100, }); - const { data: environments, isLoading: isLoadingEnvironments } = api.environment.bySystemId.useQuery(systemId); + const { data: environments, isLoading: isLoadingEnvironments, error: environmentsError } = api.environment.bySystemId.useQuery(systemId); const [healthData, setHealthData] = useState<any[]>([]); const [isLoading, setIsLoading] = useState(true); + const [error, setError] = useState<string | null>(null);
103-108: Consider adding responsive margin adjustmentsThe chart margins are fixed values that might not scale well on different screen sizes. Consider making the margins responsive based on container width.
margin={{ top: 10, right: 10, left: 10, - bottom: 20, + bottom: 30, // Increase bottom margin for better label visibility }}
123-129: Tooltip formatting enhancementThe tooltip implementation is good but could be enhanced to show the total count for context.
<Tooltip formatter={(value, name) => [ value, - name === "healthy" ? "Healthy" : "Unhealthy" + name === "healthy" ? "Healthy Resources" : "Unhealthy Resources" ]} + labelFormatter={(label) => `Environment: ${label}`} contentStyle={{ borderRadius: '4px', fontSize: '12px' }} />apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx (3)
18-22: Consider strengthening type definitionsThe
deploymentsarray is usingany[]type, which loses type safety. Consider defining a more specific type for the deployment objects.+ type Deployment = { + id: string; + name: string; + systemName: string; + systemSlug: string; + slug: string; + totalJobs?: number; + successRate?: number; + p50?: number; + p90?: number; + lastRunAt?: string; + }; type DeploymentPerformanceProps = { - deployments: any[]; + deployments: Deployment[]; startDate: Date; endDate: Date; };
43-46: Consider adding error handling for navigationThe navigation function looks good, but consider adding error handling in case the navigation fails.
const handleRowClick = (systemSlug: string, deploymentSlug: string) => { // Navigate to the deployment details - router.push(`/${systemSlug}/deployments/${deploymentSlug}`); + try { + router.push(`/${systemSlug}/deployments/${deploymentSlug}`); + } catch (error) { + console.error("Navigation failed:", error); + // Optionally show a user-facing error message + } };
80-122: Enhance accessibility of clickable rowsThe table rows are clickable but lack keyboard accessibility indicators. Consider enhancing the accessibility for keyboard users.
<TableRow key={deployment.id} - className="cursor-pointer hover:bg-muted/10" + className="cursor-pointer hover:bg-muted/10 focus-within:bg-muted/20" onClick={() => handleRowClick(deployment.systemSlug, deployment.slug)} + tabIndex={0} + role="button" + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleRowClick(deployment.systemSlug, deployment.slug); + } + }} >apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx (3)
35-61: Consider consolidating query parameter updates
BothhandleSystemChangeandhandleTimeRangeChangerebuild the URL params with similar logic. You might encapsulate this into a single helper function for better maintainability, reducing code repetition and potential mistakes when adding more query parameters.
63-79: Minor UI Enhancement
It is sometimes helpful to provide a visual placeholder (e.g., “Select a time range” or “Select a system”) rather than only defaulting to "Last 30 days" or "All Systems". This can clarify user intent.
81-96: Validation ofsystemsarray
If there is an emptysystemsarray, the user won't see system options to select. You might want to handle that scenario—either display a fallback UI, or disable the dropdown.apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx (1)
28-39: Enhance Accessibility for Color-Coded Text
The dynamic text color tied to success rate is helpful but could pose difficulty for color-blind users. Consider adding an accessible label (e.g., viaaria-label) or including a textual indicator (e.g., "High", "Medium", "Low") to reinforce the color signal.apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (2)
45-46: Robust parsing oftimeRange
IfparseInt(timeRange, 10)fails or receives an invalid string, the fallback is30. This is fine, but logging or gracefully handling erroneous values (e.g., negative integers) could improve resilience.
91-102: Header and filters
The integration of<InsightsFilters>in the header is a neat approach. Consider adding a brief descriptive text near the filters to help new users understand how these selections drive the displayed data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (4)
76-233: Good implementation of EnvironmentCard component with a clean, informative UIThe new EnvironmentCard component presents environment data in a clear, well-structured card format with good visual indicators for status. The use of color coding for health status and the layout of information make it easy to understand the environment state at a glance.
97-101: Replace mock data with actual API dataThe code uses hardcoded mock data for the latest release failure rate. As indicated by the comments, this should be replaced with actual data from an API call before deployment to production.
195-206: Good use of useCallback for clipboard operationThe clipboard functionality is properly implemented with useCallback to prevent unnecessary re-renders, and it includes prevention of the default link behavior with e.preventDefault(). The toast notification provides good user feedback.
105-112: Good status calculation logicThe status calculation logic is clear and provides intuitive feedback about the environment's health state. The use of descriptive status labels like "Healthy", "Issues Detected", and "No Resources" makes the status easy to understand.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx (1)
23-32: Improved UI layout and formattingThe changes to the card structure and content presentation provide a cleaner, more consistent UI with improved readability. Using
toLocaleString()for number formatting is a good practice, especially for potentially large numbers. The flex column layout separates the label from the value effectively.apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx (1)
26-35: Good UI consistency with other card componentsThe changes maintain visual consistency with other card components in the insights section. The skeleton dimensions have been appropriately adjusted, and the conditional rendering is clean. Using
toLocaleString()for the job count improves readability for larger numbers.apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (3)
13-21: Clean component structure with appropriate propsThe component props are well-defined and the functional component is properly typed with React.FC.
67-80: Loading state well-implementedThe loading state is well implemented with a skeleton UI to indicate data is being fetched, which provides good user feedback.
33-65:❓ Verification inconclusive
Hardcoded implementation detail in resource filtering
The comment on lines 46-47 mentions "simplified check" with a
return truethat includes all resources in every environment. This appears to be a placeholder implementation that needs to be replaced with actual logic.Replace the simplified check with actual logic to filter resources by environment:
const envResources = resources.items.filter(resource => { - // This is a simplified check - you'd need to implement logic to properly - // check if a resource belongs to an environment based on filter criteria - return true; + // Filter resources that belong to this environment + return resource.environmentId === env.id; });Please verify if there's an appropriate field in the resource object that can be used to associate it with an environment.
Below is a new shell script to verify whether the resource objects in our repository include an
environmentIdproperty:#!/bin/bash # Verify if resource objects include an 'environmentId' field. rg "environmentId" -n .
Action Required: Validate Resource Filtering Logic
The current implementation always includes all resources by returning
truein the filter. As a fix, replace the placeholder with a condition that compares the resource’s environment identifier to the current environment’s id:const envResources = resources.items.filter(resource => { - // This is a simplified check - you'd need to implement logic to properly - // check if a resource belongs to an environment based on filter criteria - return true; + // Filter resources that belong to this environment + return resource.environmentId === env.id; });This change assumes that the
resourceobject includes anenvironmentIdproperty correctly used for environment association. Please verify with the script above that such a field exists (or adjust to use the appropriate property if it does not).apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx (2)
31-41: Well-implemented duration formatting functionThe
formatDurationfunction is well-implemented with appropriate handling of edge cases (null/undefined values) and proper formatting logic.
90-104: Well-implemented conditional styling for success rateThe conditional styling for success rates is well-implemented with appropriate color coding based on the success percentage, which provides good visual feedback.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (1)
47-49: Missing system-based filtering implementation
Despite the comment "Filter by system if systemId is provided," there's no code filteringfilteredResourcesbysystemId. Consider applying actual filtering logic, for example:- let filteredResources = resources.items; + let filteredResources = systemId + ? resources.items.filter(item => item.systemId === systemId) + : resources.items;apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (2)
40-41: Confirm alignment of default time range
You defaulttimeRangeto"30". Verify that this matches user expectations or any UI defaults that might differ.
175-182: DeploymentPerformance data scope
DeploymentPerformancedisplays data based on eithersystemIdorworkspaceId. Double-check that the user won't accidentally query massive amounts of data when no system is selected in a large workspace, which might affect performance.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (1)
73-86: Consider simplifying the deployment stats parameters.The code sets
orderByandorderindeploymentStatsParams, but then overrides them with the same values when making the API call.const deploymentStatsParams = { startDate, endDate, timezone, orderBy: "last-run-at", order: "desc", ...(systemId ? { systemId } : { workspaceId: workspace.id }), + orderBy: "last-run-at" as StatsColumn, + order: "desc" as StatsOrder, }; const deploymentStats = await api.deployment.stats.byWorkspaceId({ ...deploymentStatsParams, - orderBy: "last-run-at" as StatsColumn, - order: "desc" as StatsOrder, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (4)
37-37: Good error handling for searchParams!The optional chaining (
??) ensures the code works even if searchParams is undefined, and the default value for timeRange is correctly set.
42-45: Proper date range calculation based on user selection.The code correctly calculates the date range using the timeRange parameter, with appropriate parsing and default value handling.
156-177: Explicit environment forSystemHealthOverviewWhen no
systemIdis selected, you display a helpful placeholder. In some cases, you might also want to show aggregated or cross-system information rather than an empty placeholder—depending on the product requirements.
89-191: Well-structured layout with clear section organization.The page layout is well-organized with meaningful section headings and responsive grid layouts. The conditional rendering for SystemHealthOverview and the new components like InsightsFilters and DeploymentPerformance enhance user experience with improved filtering and data visualization.
adityachoudhari26
force-pushed
the
switch-to-environment-cards
branch
from
e8837a9 to
e461bef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (2)
76-113: Well-designed status calculation logic for environment health.The status calculation provides a clear representation of environment health based on resource counts, with appropriate status text and color coding.
However, there's some duplication with the existing
EnvironmentHealthcomponent logic. Consider extracting the health calculation into a shared utility function to maintain DRY principles.+// Add to a shared utility file: +export const calculateEnvironmentHealth = (totalCount: number, unhealthyCount: number) => { + const healthyCount = totalCount - unhealthyCount; + const status = + totalCount > 0 + ? unhealthyCount === 0 + ? "Healthy" + : "Issues Detected" + : "No Resources"; + const statusColor = + totalCount > 0 ? (unhealthyCount === 0 ? "green" : "red") : "neutral"; + + return { healthyCount, status, statusColor }; +}; // Then in both components: -const unhealthyCount = unhealthyResourcesQ.data?.length ?? 0; -const totalCount = allResourcesQ.data?.total ?? 0; -const healthyCount = totalCount - unhealthyCount; -const status = - totalCount > 0 - ? unhealthyCount === 0 - ? "Healthy" - : "Issues Detected" - : "No Resources"; -const statusColor = - totalCount > 0 ? (unhealthyCount === 0 ? "green" : "red") : "neutral"; +const unhealthyCount = unhealthyResourcesQ.data?.length ?? 0; +const totalCount = allResourcesQ.data?.total ?? 0; +const { healthyCount, status, statusColor } = calculateEnvironmentHealth(totalCount, unhealthyCount);
213-228: Implement consistent color threshold for failure rate.The code uses a hardcoded threshold (5%) for determining if a failure rate is good or bad. Consider extracting this to a constant or configuration value for consistency across the application.
+// At the top of the file or in a constants file +const FAILURE_RATE_THRESHOLD = 5; // percentage // Then in the component -{latestReleaseFailureRate > 5 ? ( +{latestReleaseFailureRate > FAILURE_RATE_THRESHOLD ? (apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (3)
30-30: Consider using a more specific type for healthData state.The
any[]type is used for thehealthDatastate, which may lead to type safety issues. Consider defining a specific interface for this data structure.- const [healthData, setHealthData] = useState<any[]>([]); + type HealthDataItem = { + name: string; + healthy: number; + unhealthy: number; + total: number; + }; + const [healthData, setHealthData] = useState<HealthDataItem[]>([]);
52-53: Use optional chaining and nullish coalescing for safer property access.When filtering resources by status, consider that the status property might be undefined or null.
- const healthy = envResources.filter(r => r.status === "healthy").length || 0; + const healthy = envResources.filter(r => r.status === JobStatus.Healthy).length ?? 0;
112-117: Consider adding truncation for long environment names in the X-axis.For environments with long names, you might want to truncate or handle the text to ensure it displays properly in the chart.
<XAxis dataKey="name" axisLine={false} tickLine={false} fontSize={12} + tickFormatter={(value) => value.length > 15 ? `${value.substring(0, 12)}...` : value} />apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx (1)
14-18: Consider importing the System type from a shared types file.The
Systemtype is defined locally but might be reused across other components. Consider moving it to a shared types file to maintain consistency.-type System = { - id: string; - name: string; - slug: string; -}; +import type { System } from "~/types/system";apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (1)
74-86: Ensure proper type safety for API parameters.The deployment stats API call uses type casting for
orderByandorderparameters. Consider defining these parameters with their correct types from the beginning to avoid casting.const deploymentStatsParams = { startDate, endDate, timezone, - orderBy: "last-run-at", - order: "desc", + orderBy: "last-run-at" as StatsColumn, + order: "desc" as StatsOrder, ...(systemId ? { systemId } : { workspaceId: workspace.id }), }; const deploymentStats = await api.deployment.stats.byWorkspaceId({ ...deploymentStatsParams, - orderBy: "last-run-at" as StatsColumn, - order: "desc" as StatsOrder, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx(4 hunks)apps/webservice/src/app/urls.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/webservice/src/app/urls.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict en...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (13)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (5)
3-13: Well-organized imports for enhanced functionality.The added imports for navigation hooks and icons are properly organized by source, providing all the necessary icons for the dropdown menu items.
38-48: Good implementation of URL builder pattern.Using the URL builder pattern from the
urlsutility is a clean approach for constructing environment URLs consistently. This makes navigation more maintainable and less prone to errors.
50-56: Effective clipboard interaction with user feedback.The
copyEnvironmentIdfunction provides a good user experience by copying the environment ID to clipboard and showing a toast notification for confirmation.
61-86: Well-structured navigation options in first group.The first dropdown menu group is well-organized with logical navigation options (View Details, Deployments, Resources) and consistent styling. Each item has an appropriate icon and clear purpose.
90-122: Good organization of environment management options.The second dropdown menu group effectively organizes environment management options with consistent styling and appropriate icons for each action.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (3)
97-100: Replace mock data with actual API dataThe code uses hardcoded mock data for the latest release failure rate. As indicated by the comments, this should be replaced with actual data from an API call before deployment to production.
119-157: Good card header design with status indicators.The card header provides a well-designed visual representation of the environment status with appropriate color coding and layout. The environment name and status badge are clearly visible, and the dropdown menu is accessible.
191-211: Optimized clipboard interaction with useCallback.The implementation of the copy functionality:
- Correctly uses
useCallbackto memoize the handler- Prevents the default link behavior when clicking the copy button
- Provides user feedback through a toast notification
- Shows the full environment ID in the description
This creates a smooth user experience.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx (2)
35-47: LGTM! Well-implemented callback with proper dependencies.The
handleSystemChangefunction is well-implemented with appropriate URL parameter management and proper dependencies in the useCallback hook.
63-97: Clean and well-structured UI components.The UI components are well-organized with appropriate sizing and spacing. The selects have clear labels and the component provides good usability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (3)
42-45: LGTM! Good approach to dynamic date range calculation.The implementation for calculating the date range based on the
timeRangeparameter is clean and well-structured.
156-177: Good conditional rendering for SystemHealthOverview component.The approach to conditionally render either the SystemHealthOverview component or a placeholder based on whether a systemId is selected is good. The placeholder provides clear instructions to the user.
117-143: Well-structured activity trends section with responsive grid layout.The activity trends section is well-organized using a responsive grid layout that adapts to different screen sizes. The cards have consistent styling and provide valuable information about jobs and resources over time.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (1)
195-241: Error handling for clipboard operation.The clipboard operation lacks error handling. While modern browsers widely support the Clipboard API, it's best practice to handle potential failures.
- onClick={useCallback( - (e: React.MouseEvent<HTMLButtonElement>) => { - e.preventDefault(); - navigator.clipboard.writeText(environment.id); - toast.success("Environment ID copied to clipboard", { - description: environment.id, - duration: 2000, - icon: <IconCheck className="h-4 w-4" />, - }); - }, - [environment.id], - )} + onClick={useCallback( + (e: React.MouseEvent<HTMLButtonElement>) => { + e.preventDefault(); + navigator.clipboard.writeText(environment.id) + .then(() => { + toast.success("Environment ID copied to clipboard", { + description: environment.id, + duration: 2000, + icon: <IconCheck className="h-4 w-4" />, + }); + }) + .catch(() => { + toast.error("Failed to copy to clipboard", { + duration: 2000, + }); + }); + }, + [environment.id], + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx(5 hunks)apps/webservice/src/app/urls.ts(1 hunks)packages/api/src/router/environment-stats.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/urls.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsxpackages/api/src/router/environment-stats.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx
🧬 Code Definitions (3)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (4)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx (1) (1)
InsightsFilters(27:99)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (1) (1)
ResourceTypeBreakdown(31:151)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (1) (1)
SystemHealthOverview(18:155)apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx (1) (1)
DeploymentPerformance(24:130)
packages/api/src/router/environment-stats.ts (4)
packages/db/src/schema/job.ts (1) (1)
JobStatus(132:132)packages/db/src/schema/job.ts (1) (1)
JobStatus(132:132)packages/db/src/schema/job.ts (1) (1)
JobStatus(132:132)packages/db/src/schema/job.ts (1) (1)
JobStatus(132:132)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (6)
apps/webservice/src/app/urls.ts (1) (1)
urls(193:193)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (1) (1)
EnvironmentDropdown(33:138)apps/webservice/src/app/urls.ts (1) (1)
urls(193:193)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (1) (1)
EnvironmentDropdown(33:138)apps/webservice/src/app/urls.ts (1) (1)
urls(193:193)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentDropdown.tsx (1) (1)
EnvironmentDropdown(33:138)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
packages/api/src/router/environment-stats.ts (3)
7-11: LGTM: New database imports correctly added.The additions of
gt,inArray, andlteoperators from the database library are appropriate for the new query functionality.
97-141: Well-structured procedure with proper authorization.The
failureRateprocedure implementation is well-structured with:
- Clear input validation using Zod
- Proper authorization checks
- Efficient SQL query with nullif handling to prevent division by zero
- Appropriate date range filtering
The procedure calculates the job failure rate percentage by counting unsuccessful jobs divided by total jobs for a specific environment within a date range.
18-18:✅ Verification successful
Ensure JobStatus is properly typed.
The import of
analyticsStatusesandJobStatusprovides necessary constants for the query filters.
🏁 Script executed:
#!/bin/bash # Verify that JobStatus is properly defined and imported # Check the import source cat packages/db/src/schema/job.ts | grep -A 5 "export type JobStatus" # Check for analyticsStatuses definition grep -r "export const analyticsStatuses" --include="*.ts" .Length of output: 367
Verified: JobStatus is correctly typed and imported
The verification confirms that
JobStatusis defined as an alias toJob["status"]inpackages/db/src/schema/job.tsand is properly imported alongsideanalyticsStatusesfrom the validators package. No further changes are needed.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/environments/EnvironmentRow.tsx (3)
4-8: Appropriate imports for new functionality.Added imports for React hooks, icons, and date utilities to support the new component functionality.
13-15: UI components imported for card-based layout.The addition of Card components provides a structured layout for the new EnvironmentCard.
78-241: Well-designed EnvironmentCard component with comprehensive environment information.The new EnvironmentCard component provides a detailed view of the environment with:
- Visual status indicators (green for healthy, red for issues)
- Resource counts and health metrics
- Environment ID with copy functionality
- Failure rate statistics over the past week
Implementation uses appropriate error states and loading indicators. The copy-to-clipboard functionality correctly prevents default navigation behavior.
One minor improvement to consider:
- const failureRateQ = api.environment.stats.failureRate.useQuery({ + // Use one week timeframe for failure rate statistics + const failureRateQ = api.environment.stats.failureRate.useQuery({apps/webservice/src/app/[workspaceSlug]/(app)/insights/page.tsx (8)
1-4: Type imports correctly added for deployment statistics.These imported types provide proper typing for the deployment statistics query.
14-20: Appropriate component imports for new insights sections.The imports of new components align with the enhanced page structure.
42-46: Search parameters type correctly defined.The interface for search parameters supports the new filtering capabilities.
51-59: Improved date calculation based on timeRange parameter.The implementation correctly uses the timeRange parameter to calculate the start date, with a sensible default of 30 days.
61-96: Well-structured data fetching with system filtering.The code properly fetches systems for the filter dropdown and handles optional system filtering for statistical queries. The restructuring of parameters improves code organization and readability.
131-157: Well-designed Activity Trends section with responsive layout.The Activity Trends section provides a clear visualization of activity over time with:
- Jobs per day chart
- Resources over time chart
- Responsive grid layout
The implementation properly displays the selected time range in the title.
159-194: Conditional rendering for system health overview.The Distribution & Health section intelligently handles both cases:
- When a system is selected: Shows the SystemHealthOverview component
- When no system is selected: Shows a helpful placeholder with instructions
This provides good user experience and clear guidance.
196-205: Performance Metrics section with deployment statistics.The Performance Metrics section properly integrates the DeploymentPerformance component, passing the required props including date range.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx (1)
3-3: Unused imports detected.The component imports
CardHeaderandCardTitlefrom "@ctrlplane/ui/card" but doesn't use them anymore after the redesign.-import { Card, CardContent, CardHeader, CardTitle } from "@ctrlplane/ui/card"; +import { Card, CardContent } from "@ctrlplane/ui/card";apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (2)
64-66: Add error handling for potential undefined values.The code assumes that resources will always have a status property, but this might not be the case. Add defensive coding to handle potential undefined values.
const total = envResources.length; -const healthy = - envResources.filter((r) => r.status === "healthy").length || 0; +const healthy = + envResources.filter((r) => r?.status === "healthy").length || 0; const unhealthy = total - healthy;
146-151: Add type safety for the Tooltip formatter function.The current implementation doesn't type-check the formatter arguments, which could lead to errors if the recharts API changes.
<Tooltip - formatter={(value, name) => [ + formatter={(value: number, name: string) => [ value, name === "healthy" ? "Healthy" : "Unhealthy", ]} contentStyle={{ borderRadius: "4px", fontSize: "12px" }} />apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (2)
61-68: Add error handling for malformed resource objects.The code could crash if a resource object doesn't have the expected structure. Add null checks to handle potential undefined values.
let key; if (groupBy === "kind") { - key = resource.kind || "Unknown"; + key = resource?.kind || "Unknown"; } else if (groupBy === "provider") { - key = resource.provider || "Unknown"; + key = resource?.provider || "Unknown"; } else if (groupBy === "apiVersion") { - key = resource.apiVersion || "Unknown"; + key = resource?.apiVersion || "Unknown"; }
144-148: Improve label rendering logic for better readability.The current implementation truncates labels that are longer than 12 characters and doesn't display labels for small segments. Consider adding a tooltip or other solution to ensure visibility of all data.
label={({ name, percent }) => - percent > 0.05 - ? `${name.length > 12 ? name.substring(0, 12) + "..." : name}` - : "" + percent > 0.03 + ? `${name.length > 15 ? name.substring(0, 15) + "..." : name}` + : "" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx (3)
26-26: Improved card styling with shadow.The change from a full-width card with background inheritance to using a subtle shadow creates better visual hierarchy and depth, which helps distinguish this component from its surroundings.
27-27: Consistent padding improves card content layout.The explicit padding values (px-6 pt-6) create proper spacing around the content, enhancing readability and visual appeal.
28-39: Improved content structure and visual hierarchy.The redesigned content presentation with:
- A cleaner conditional rendering using ternary operator
- Appropriately sized skeleton loader (h-8 w-20)
- Semantic structure with label and value separation
- Proper typography hierarchy using text-sm for labels and text-3xl for values
- Number formatting with toLocaleString() for better readability
This creates a more intuitive and accessible card that follows good UI design principles.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (1)
57-61: Implement proper environment resource filtering.The current implementation includes a placeholder filter that always returns true. This means all resources are associated with each environment, which might not be accurate.
You need to implement the actual filtering logic based on your application's data model to properly associate resources with their respective environments.
const envResources = resources.items.filter(resource => { - // This is a simplified check - you'd need to implement logic to properly - // check if a resource belongs to an environment based on filter criteria - return true; + // Example implementation - adjust based on your actual data model: + return resource.environmentId === env.id || resource.labels?.environment === env.name; });apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx (1)
1-99: LGTM! Well-structured component with clean logic.This component is well-implemented with proper TypeScript typing, good separation of concerns, and clear, maintainable code. The handlers for system and time range changes are implemented efficiently with appropriate dependency arrays in the useCallback hooks.
| const { data: environments, isLoading: isLoadingEnvironments } = | ||
| api.environment.bySystemId.useQuery(systemId); | ||
|
|
||
| const [healthData, setHealthData] = useState<any[]>([]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type and define a proper interface.
Using any[] type for healthData loses TypeScript's type safety benefits. Consider creating a proper interface for the health data structure.
-const [healthData, setHealthData] = useState<any[]>([]);
+interface EnvironmentHealth {
+ name: string;
+ healthy: number;
+ unhealthy: number;
+ total: number;
+}
+
+const [healthData, setHealthData] = useState<EnvironmentHealth[]>([]);📝 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 [healthData, setHealthData] = useState<any[]>([]); | |
| interface EnvironmentHealth { | |
| name: string; | |
| healthy: number; | |
| unhealthy: number; | |
| total: number; | |
| } | |
| const [healthData, setHealthData] = useState<EnvironmentHealth[]>([]); |
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (3)
144-148: Move label function outside of JSX to prevent unnecessary re-renders.Defining functions directly in JSX can lead to new function instances on each render, potentially causing unnecessary re-renders.
+const renderPieLabel = ({ name, percent }: {name: string, percent: number}) => + percent > 0.05 + ? `${name.length > 12 ? `${name.substring(0, 12)}...` : name}` + : ""; // Then in JSX <Pie // other props - label={({ name, percent }: {name: string, percent: number}) => - percent > 0.05 - ? `${name.length > 12 ? `${name.substring(0, 12)}...` : name}` - : "" - } + label={renderPieLabel} >
171-173: Move formatter function outside of JSX to prevent unnecessary re-renders.Similar to the previous comment, extracting the formatter function will help optimize rendering.
+const formatLegendValue = (value: string) => + value.length > 20 ? `${value.substring(0, 20)}...` : value; // Then in JSX <Legend // other props - formatter={(value: string) => - value.length > 20 ? `${value.substring(0, 20)}...` : value - } + formatter={formatLegendValue} />
44-48: Consider implementing pagination or virtualization for performance.Fetching 500 resources at once might impact performance, especially on slower connections or devices. Consider implementing pagination, virtualization, or incremental loading for large datasets.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (5)
22-30: Ensure consistent prop usage between type definition and implementation.The
workspaceIdprop is defined in theSystemHealthOverviewPropstype but commented out in the component implementation. This creates an inconsistency between the type definition and actual usage.Either uncomment and use the prop if it's needed, or remove it from the type definition if it's not required.
type SystemHealthOverviewProps = { systemId: string; - workspaceId: string; + workspaceId?: string; // Make optional if not currently needed but planned for future }; export const SystemHealthOverview: React.FC<SystemHealthOverviewProps> = ({ systemId, - // workspaceId, + workspaceId, // Uncomment if needed }) => {
31-41: Consider adding error handling for API queries.While the loading states are well handled, there's no error handling for the API queries. Consider adding error states to handle potential API failures gracefully.
const { data: system, isLoading: isLoadingSystem } = api.system.byId.useQuery(systemId); const { data: resources, isLoading: isLoadingResources } = api.system.resources.useQuery({ systemId, limit: 100, }); const { data: environments, isLoading: isLoadingEnvironments } = api.environment.bySystemId.useQuery(systemId); +const [error, setError] = useState<string | null>(null); +useEffect(() => { + if (!system && !isLoadingSystem) setError("Failed to load system data"); + if (!resources && !isLoadingResources) setError("Failed to load resources data"); + if (!environments && !isLoadingEnvironments) setError("Failed to load environments data"); +}, [system, resources, environments, isLoadingSystem, isLoadingResources, isLoadingEnvironments]);Then update the render method to show an error state:
if (error) { return ( <Card className="shadow-sm"> <CardHeader className="pb-2"> <CardTitle className="text-base">System Health Overview</CardTitle> </CardHeader> <CardContent className="h-[350px]"> <div className="flex h-full w-full items-center justify-center"> <p className="text-sm text-red-500">{error}</p> </div> </CardContent> </Card> ); }
87-100: Consider using a single loading check.The loading check at line 87 could be simplified since you're already tracking all loading states in a single variable.
-if (isLoading === true) { +if (isLoading) { return ( // ... ); }
145-151: Consider adding more context to tooltip.The current tooltip only shows the value and whether it's "Healthy" or "Unhealthy". Consider enhancing it with percentage information for better context.
<Tooltip formatter={(value, name, entry) => { + const item = entry.payload; + const total = item.healthy + item.unhealthy; + const percentage = Math.round((value as number / total) * 100); return [ - value, + `${value} (${percentage}%)`, name === "healthy" ? "Healthy" : "Unhealthy", ]; }} contentStyle={{ borderRadius: "4px", fontSize: "12px" }} />
45-85: Consider optimizing the effect dependencies.The effect has many dependencies which could cause unnecessary recalculations. Consider restructuring to minimize re-executions.
useEffect(() => { // Wait for all data to load if (isLoadingSystem || isLoadingResources || isLoadingEnvironments) { setIsLoading(true); return; } setIsLoading(false); // Format data for the environments health chart if (environments && resources) { const envHealthData = environments.map((env) => { // ... calculation logic }); setHealthData(envHealthData); } -}, [ - system, - resources, - environments, - isLoadingSystem, - isLoadingResources, - isLoadingEnvironments, -]); +}, [isLoadingSystem, isLoadingResources, isLoadingEnvironments, environments, resources]);The
systemdependency isn't used within the effect body for calculations, so it can be removed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/WorkspaceResources.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/TotalJobs.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/overview-cards/SuccessRate.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/InsightsFilters.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (9)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (6)
50-50: Define a proper type for chart data instead of using 'any'.Using
any[]for the chart data loses TypeScript's type safety benefits. Create a specific interface for the chart data structure.-const [chartData, setChartData] = useState<any[]>([]); +interface ResourceChartData { + name: string; + value: number; +} + +const [chartData, setChartData] = useState<ResourceChartData[]>([]);
55-56: Implement system filtering logic.The comment indicates that resources should be filtered by
systemIdif provided, but the actual filtering logic is missing. This will lead to inaccurate visualization when a specific system is selected.// Filter by system if systemId is provided -const filteredResources = resources.items; +let filteredResources = resources.items; +if (systemId) { + filteredResources = filteredResources.filter(resource => + resource.systemId === systemId || resource.system?.id === systemId + ); +}
58-85: The resource processing logic looks good.The implementation for grouping resources by different attributes and transforming them into chart data is well structured. The dependency array correctly includes all variables used within the effect.
87-98: Loading state is properly handled.The component appropriately displays a skeleton loader while data is being fetched. This provides good feedback to users during the loading phase.
125-131: Empty state is properly handled.The component gracefully handles the case when no data is available, providing clear feedback to users instead of displaying an empty chart.
158-165: Tooltip implementation is well done.The tooltip configuration provides clear information about the data point being hovered, with good formatting and styling.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (3)
42-42: Avoid usinganytype and define a proper interface.Using
any[]type forhealthDataloses TypeScript's type safety benefits. Consider creating a proper interface for the health data structure.-const [healthData, setHealthData] = useState<any[]>([]); +interface EnvironmentHealth { + name: string; + healthy: number; + unhealthy: number; + total: number; +} + +const [healthData, setHealthData] = useState<EnvironmentHealth[]>([]);
57-61: Implement proper environment resource filtering.The current implementation includes a placeholder filter that always returns true. This means all resources are associated with each environment, which might not be accurate.
const envResources = resources.items.filter((_resource) => { // This is a simplified check - you'd need to implement logic to properly // check if a resource belongs to an environment based on filter criteria - return true as boolean; + // Example implementation - adjust based on your actual data model: + return _resource.environmentId === env.id || _resource.labels?.environment === env.name; });
122-172: The chart implementation is well-structured and responsive.The BarChart implementation using Recharts is well done with appropriate configuration for responsiveness, styling, and data presentation. The use of color coding for healthy/unhealthy resources and the tooltip formatting enhances usability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (1)
58-62:⚠️ Potential issueImplement proper environment resource filtering.
The current implementation includes a placeholder filter that always returns true. This means all resources are associated with each environment, which might not be accurate.
Implement the actual filtering logic based on your application's data model:
-const envResources = resources.items.filter((_resource) => { - // This is a simplified check - you'd need to implement logic to properly - // check if a resource belongs to an environment based on filter criteria - return true as boolean; -}); +const envResources = resources.items.filter((resource) => { + // Filter resources that belong to this environment + return resource.environmentId === env.id || resource.labels?.environment === env.name; +});apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (1)
55-56:⚠️ Potential issueImplement system filtering logic.
The comment indicates that resources should be filtered by
systemIdif provided, but the actual filtering logic is missing. This will lead to inaccurate visualization when a specific system is selected.// Filter by system if systemId is provided -const filteredResources = resources.items; +let filteredResources = resources.items; +if (systemId) { + filteredResources = filteredResources.filter(resource => + resource.systemId === systemId || resource.system?.id === systemId + ); +}
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (4)
87-101: Extract loading state skeleton to a separate component.The loading skeleton could be extracted into a separate component for better code organization and potential reuse across similar components.
+const SystemHealthSkeletonLoader = () => ( + <Card className="shadow-sm"> + <CardHeader className="pb-2"> + <CardTitle className="text-base">System Health Overview</CardTitle> + </CardHeader> + <CardContent className="h-[350px]"> + <div className="flex h-full w-full items-center justify-center"> + <Skeleton className="h-4/5 w-4/5" /> + </div> + </CardContent> + </Card> +); export const SystemHealthOverview: React.FC<SystemHealthOverviewProps> = ({ // ... rest of component }) => { // ... existing code if (isLoading === true) { - return ( - <Card className="shadow-sm"> - <CardHeader className="pb-2"> - <CardTitle className="text-base">System Health Overview</CardTitle> - </CardHeader> - <CardContent className="h-[350px]"> - <div className="flex h-full w-full items-center justify-center"> - <Skeleton className="h-4/5 w-4/5" /> - </div> - </CardContent> - </Card> - ); + return <SystemHealthSkeletonLoader />; }
88-88: Simplify boolean comparison.No need to explicitly compare with
truein a boolean check.-if (isLoading === true) { +if (isLoading) {
146-152: Extract tooltip formatter for better readability.Consider extracting the tooltip formatter logic into a named function for improved readability and maintainability.
+const tooltipFormatter = (value: number, name: string) => [ + value, + name === "healthy" ? "Healthy" : "Unhealthy", +]; // ...within the component <Tooltip - formatter={(value, name) => [ - value, - name === "healthy" ? "Healthy" : "Unhealthy", - ]} + formatter={tooltipFormatter} contentStyle={{ borderRadius: "4px", fontSize: "12px" }} />
156-158: Simplify Legend formatter function.The Legend formatter function can be simplified for better readability.
<Legend wrapperStyle={{ fontSize: "12px" }} iconSize={10} - formatter={(value) => { - return value === "healthy" ? "Healthy" : "Unhealthy"; - }} + formatter={(value) => value === "healthy" ? "Healthy" : "Unhealthy"} />apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (3)
59-77: Extract resource grouping logic to a separate function.The resource grouping logic is complex and would benefit from being extracted into a separate function for better readability and maintainability.
+const groupResourcesByAttribute = ( + resources: any[], + groupBy: string +): Record<string, number> => { + return resources.reduce( + (acc, resource) => { + let key; + if (groupBy === "kind") { + key = resource.kind || "Unknown"; + } else if (groupBy === "provider") { + key = resource.provider ?? "Unknown"; + } else if (groupBy === "apiVersion") { + key = resource.apiVersion || "Unknown"; + } + + if (!acc[key]) { + acc[key] = 0; + } + acc[key]++; + return acc; + }, + {} as Record<string, number> + ); +}; // In useEffect useEffect(() => { if (!resources) return; // Filter by system if systemId is provided const filteredResources = resources.items; // Group resources by the selected attribute - const groupedResources = filteredResources.reduce( - (acc, resource) => { - let key; - if (groupBy === "kind") { - key = resource.kind || "Unknown"; - } else if (groupBy === "provider") { - key = resource.provider ?? "Unknown"; - } else if (groupBy === "apiVersion") { - key = resource.apiVersion || "Unknown"; - } - - if (!acc[key]) { - acc[key] = 0; - } - acc[key]++; - return acc; - }, - {} as Record<string, number>, - ); + const groupedResources = groupResourcesByAttribute(filteredResources, groupBy);
144-154: Extract label formatting logic to a separate function.The label formatting logic uses complex inline conditional expressions that could be extracted to a separate function for better readability.
+const formatPieLabel = ({ name, percent }: { name: string; percent: number }): string => { + if (percent <= 0.05) return ""; + return name.length > 12 ? `${name.substring(0, 12)}...` : name; +}; // In the component <Pie data={chartData} cx="50%" cy="45%" labelLine={false} outerRadius={130} innerRadius={60} fill="#8884d8" dataKey="value" paddingAngle={1} - label={({ - name, - percent, - }: { - name: string; - percent: number; - }) => - percent > 0.05 - ? `${name.length > 12 ? `${name.substring(0, 12)}...` : name}` - : "" - } + label={formatPieLabel} >
177-179: Simplify legend formatter function.The legend formatter function can be simplified using a more concise syntax for better readability.
<Legend layout="horizontal" verticalAlign="bottom" align="center" wrapperStyle={{ paddingTop: "10px", fontSize: "12px" }} - formatter={(value: string) => - value.length > 20 ? `${value.substring(0, 20)}...` : value - } + formatter={(value: string) => value.length > 20 ? `${value.substring(0, 20)}...` : value} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (1)
43-43: Avoid usinganytype and define a proper interface.Using
any[]type forhealthDataloses TypeScript's type safety benefits. Consider creating a proper interface for the health data structure.-const [healthData, setHealthData] = useState<any[]>([]); +interface EnvironmentHealth { + name: string; + healthy: number; + unhealthy: number; + total: number; +} + +const [healthData, setHealthData] = useState<EnvironmentHealth[]>([]);apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (1)
50-50: Define a proper type for chart data instead of using 'any'.Using
any[]for the chart data loses TypeScript's type safety benefits. Create a specific interface for the chart data structure.-const [chartData, setChartData] = useState<any[]>([]); +interface ResourceChartData { + name: string; + value: number; +} + +const [chartData, setChartData] = useState<ResourceChartData[]>([]);
|
|
||
| export const SystemHealthOverview: React.FC<SystemHealthOverviewProps> = ({ | ||
| systemId, | ||
| // workspaceId, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the need for commented-out prop.
The workspaceId prop is defined in the component interface but commented out in the parameter list. If it's not needed, consider removing it from the props type definition as well to maintain consistency.
🏁 Script executed:
#!/bin/bash
# Check if workspaceId is used elsewhere in the codebase for this component
rg -A 2 -B 2 "SystemHealthOverview.*workspaceId" --type tsxLength of output: 89
Below is an updated verification step. It appears the original command failed because ripgrep did not recognize the custom file type flag for TSX files. Please run the following script to properly search for any non-commented usage of "workspaceId" within the SystemHealthOverview component:
Once you verify that no uncommented usage exists for workspaceId in that file (or elsewhere if needed), you can proceed to remove it from the props type definition for consistency.
Action: Verify if the commented-out workspaceId prop is required.
• If it isn’t used anywhere (as confirmed by the above search), please remove it from both the parameter list and the component’s props interface.
🏁 Script executed:
#!/bin/bash
# Search within files named SystemHealthOverview.tsx for any occurrence of "workspaceId"
rg "workspaceId" -g "SystemHealthOverview.tsx"Length of output: 367
Action: Remove the unused workspaceId property for consistency
The search confirmed that workspaceId is declared in the component's prop interface but remains commented out in the parameter list. If this property isn’t being used or planned for future use, please remove it from the props type definition as well as the commented section in the component declaration to avoid confusion and maintain consistency.
• Location:
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx- Remove the
workspaceId: string;line from the interface if unused. - Delete the commented
// workspaceId,from the parameter list.
- Remove the
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (1)
58-62:⚠️ Potential issueImplement actual resource to environment filtering logic.
The current implementation includes a placeholder filter that always returns true, meaning all resources are associated with each environment. This doesn't accurately represent the relationship between resources and environments.
const envResources = resources.items.filter((_resource) => { - // This is a simplified check - you'd need to implement logic to properly - // check if a resource belongs to an environment based on filter criteria - return true as boolean; + // Implement proper filtering based on your data model, for example: + return _resource.environmentId === env.id || + _resource.labels?.environment === env.name; });
🧹 Nitpick comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (4)
44-49: Add error handling for the API call.The current implementation doesn't handle API errors, which could lead to unexpected behavior if the request fails.
-const { data: resources, isLoading } = +const { data: resources, isLoading, error } = api.resource.byWorkspaceId.list.useQuery({ workspaceId, limit: 500, }); +// Add error handling in the rendering sectionAnd then handle the error in the rendering section:
if (error) { return ( <Card className="rounded-md"> <CardHeader> <CardTitle>Resource Breakdown</CardTitle> </CardHeader> <CardContent className="h-[400px]"> <div className="flex h-full items-center justify-center"> <p className="text-sm text-muted-foreground"> Error loading resource data: {error.message} </p> </div> </CardContent> </Card> ); }
154-162: Improve accessibility for the pie chart.The current implementation might not be fully accessible for users with visual impairments or color perception difficulties.
Consider adding:
- ARIA labels to the chart elements
- A color-blind friendly palette or patterns in addition to colors
{chartData.map((entry, index) => ( <Cell key={`cell-${index}`} fill={COLORS[index % COLORS.length]} strokeWidth={0.5} + role="graphics-symbol" + aria-label={`${entry.name}: ${entry.value} resources`} /> ))}
132-180: Consider adding a download or export option for the chart data.Adding the ability to export the chart data would enhance the user experience, especially for users who need to include this data in reports.
You could add a button in the card header to export the data as CSV or image:
<div className="flex items-center justify-between"> <CardTitle className="text-base">Resource Breakdown</CardTitle> <div className="flex items-center gap-2"> <Button variant="outline" size="sm" onClick={() => handleExportCSV(chartData)} className="h-7 px-3 text-xs" > Export CSV </Button> <Tabs defaultValue="kind" onValueChange={setGroupBy} className="w-auto" > {/* existing tabs code */} </Tabs> </div> </div>
98-184: Consider adding a responsive design approach for smaller screens.The current implementation might not render well on smaller screens, especially with the legend and labels.
You could add responsive adjustments for the chart size, legend position, and label display:
<ResponsiveContainer width="100%" height="100%"> <PieChart> <Pie data={chartData} cx="50%" cy="45%" labelLine={false} - outerRadius={130} + outerRadius={window.innerWidth < 768 ? 100 : 130} innerRadius={60} fill="#8884d8" dataKey="value" paddingAngle={1} label={({ name, percent, }: { name: string; percent: number; }) => percent > 0.05 - ? `${name.length > 12 ? `${name.substring(0, 12)}...` : name}` + ? `${name.length > (window.innerWidth < 768 ? 8 : 12) ? `${name.substring(0, window.innerWidth < 768 ? 8 : 12)}...` : name}` : "" } > {/* ... */} </Pie> <Legend layout="horizontal" - verticalAlign="bottom" + verticalAlign={window.innerWidth < 768 ? "top" : "bottom"} align="center" wrapperStyle={{ paddingTop: "10px", fontSize: "12px" }} formatter={(value: string) => - value.length > 20 ? `${value.substring(0, 20)}...` : value + value.length > (window.innerWidth < 768 ? 15 : 20) ? `${value.substring(0, window.innerWidth < 768 ? 15 : 20)}...` : value } /> {/* ... */} </PieChart> </ResponsiveContainer>To properly implement this, you should use a window resize listener or media queries in a React hook instead of directly using
window.innerWidth.apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (6)
19-19: Remove unused import comment.The commented import for
JobStatusappears to be unused in this component. If it's not needed, consider removing it to keep the code clean.
37-38: Consider making the resource limit configurable.The hard-coded limit of 100 resources might be insufficient for larger systems. Consider making this configurable or implementing pagination to handle systems with many resources.
87-100: Improve loading state display.The loading skeleton takes up significant space but doesn't give users any indication of what's loading. Consider adding a loading message or making the skeleton more representative of the chart that will be displayed.
<CardContent className="h-[350px]"> <div className="flex h-full w-full items-center justify-center"> - <Skeleton className="h-4/5 w-4/5" /> + <div className="w-4/5 space-y-4"> + <Skeleton className="h-4 w-3/4" /> + <Skeleton className="h-[200px] w-full" /> + <div className="flex justify-between"> + <Skeleton className="h-4 w-20" /> + <Skeleton className="h-4 w-20" /> + </div> + </div> </div> </CardContent>
102-102: Add fallback for missing system name.When system data isn't available, you're using a fallback of "Selected System". Consider using a more descriptive fallback that indicates the system data couldn't be loaded.
-const systemName = system?.name ?? "Selected System"; +const systemName = system?.name ?? "System Details Unavailable";
145-151: Improve tooltip accessibility.The current tooltip doesn't provide enough context about what the numbers represent. Consider adding more descriptive labels that include the total count and percentage.
<Tooltip - formatter={(value, name) => [ - value, - name === "healthy" ? "Healthy" : "Unhealthy", - ]} + formatter={(value, name, entry) => { + const item = entry.payload; + const percentage = Math.round((value / item.total) * 100); + return [`${value} (${percentage}%)`, name === "healthy" ? "Healthy" : "Unhealthy"]; + }} contentStyle={{ borderRadius: "4px", fontSize: "12px" }} />
113-121: Add a more informative empty state message.The current message "No health data available" doesn't provide users with next steps. Consider adding guidance on how to get health data.
<div className="flex h-full items-center justify-center"> <div className="text-center"> - <p className="text-sm text-muted-foreground"> - No health data available - </p> + <p className="text-sm text-muted-foreground"> + No health data available for the selected system + </p> + <p className="mt-2 text-xs text-muted-foreground"> + Add resources to environments to see health metrics + </p> </div> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/layout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/insights/DeploymentPerformance.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsxapps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsxapps/webservice/src/app/[workspaceSlug]/(app)/layout.tsx
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/layout.tsx (1)
2-2: Confirm if hiding the Insights navigation is intentionalI noticed you've removed the
IconChartBarimport and commented out the Insights navigation link. While this keeps the code clean by avoiding unused imports, it removes user access to the Insights page. Is this change intentional as part of the new environments page view implementation?If this is temporary during development, consider adding a TODO comment explaining when this will be re-enabled, or if there's a new navigation path planned for Insights.
Also applies to: 37-41
apps/webservice/src/app/[workspaceSlug]/(app)/insights/ResourceTypeBreakdown.tsx (2)
50-50: Replaceany[]type with a proper interface for chart data.Using
any[]for the chart data loses TypeScript's type safety benefits. Define a specific interface for the chart data structure.-const [chartData, setChartData] = useState<any[]>([]); +interface ResourceChartData { + name: string; + value: number; +} + +const [chartData, setChartData] = useState<ResourceChartData[]>([]);
55-57: Implement system filtering logic.The comment indicates that resources should be filtered by
systemIdif provided, but the actual filtering logic is missing. This will lead to inaccurate visualization when a specific system is selected.// Filter by system if systemId is provided -const filteredResources = resources.items; +let filteredResources = resources.items; +if (systemId) { + filteredResources = filteredResources.filter(resource => + resource.systemId === systemId || resource.system?.id === systemId + ); +}apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx (2)
23-26: Remove unused workspaceId property for consistency.The
workspaceIdproperty is defined in the props interface but commented out in the component parameter list (line 30). If this property isn't being used, it should be removed from the interface definition to maintain consistency.
43-43: Avoid usinganytype and define a proper interface.Using
any[]type forhealthDataloses TypeScript's type safety benefits. Consider creating a proper interface for the health data structure.-const [healthData, setHealthData] = useState<any[]>([]); +interface EnvironmentHealth { + name: string; + healthy: number; + unhealthy: number; + total: number; +} + +const [healthData, setHealthData] = useState<EnvironmentHealth[]>([]);
| if (!acc[key as string]) { | ||
| acc[key as string] = (acc[key as string] ?? 0) + 1; | ||
| } |
There was a problem hiding this comment.
Fix resource count accumulation logic.
The current implementation has an issue with how counts are accumulated. The condition is always checking if the key doesn't exist, but then using the nullish coalescing operator which would be unnecessary in that case.
-if (!acc[key as string]) {
- acc[key as string] = (acc[key as string] ?? 0) + 1;
-}
+// Increment count for the key
+acc[key as string] = (acc[key as string] ?? 0) + 1;📝 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.
| if (!acc[key as string]) { | |
| acc[key as string] = (acc[key as string] ?? 0) + 1; | |
| } | |
| // Increment count for the key | |
| acc[key as string] = (acc[key as string] ?? 0) + 1; |
| const total = envResources.length; | ||
| const healthy = 0; | ||
| const unhealthy = total - healthy; |
There was a problem hiding this comment.
Implement actual health status determination logic.
The current implementation hardcodes the healthy count to 0, which means all resources are counted as unhealthy. This doesn't provide a meaningful health overview.
const total = envResources.length;
-const healthy = 0;
+const healthy = envResources.filter(resource =>
+ resource.status === 'healthy' || resource.health === 'ok'
+).length;
const unhealthy = total - healthy;📝 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 total = envResources.length; | |
| const healthy = 0; | |
| const unhealthy = total - healthy; | |
| const total = envResources.length; | |
| const healthy = envResources.filter(resource => | |
| resource.status === 'healthy' || resource.health === 'ok' | |
| ).length; | |
| const unhealthy = total - healthy; |
| // import { JobStatus } from "@ctrlplane/validators/jobs"; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Connect to TRPC endpoints as mentioned in PR comments.
According to the PR comments, there's still a need to connect to TRPC endpoints. Ensure all necessary TRPC connections are implemented before merging this PR.
🏁 Script executed:
#!/bin/bash
# Check if there are any TODO comments related to TRPC in the file
grep -i "todo.*trpc\|trpc.*todo" apps/webservice/src/app/\[workspaceSlug\]/\(app\)/insights/SystemHealthOverview.tsxLength of output: 116
TRPC Endpoints Integration Required
It appears that after scanning the file there’s no evidence of any TRPC connection being implemented. Based on the PR comments, the TRPC endpoints still need to be integrated. Please verify that the necessary TRPC endpoints (and any related API hooks or configurations) are properly connected. For example:
- File:
apps/webservice/src/app/[workspaceSlug]/(app)/insights/SystemHealthOverview.tsx
– No TRPC-related imports or code blocks (beyond the removed comment) are present.
Before merging, ensure that the file (or an appropriate shared module) includes the TRPC connection as specified in earlier PR comments.
Summary by CodeRabbit
New Features
Bug Fixes