fix: Show analytics on deployment page #299
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant updates to the deployment analytics in both frontend and backend components. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DC as DeploymentsCard
participant DS as DeploymentStatistics
participant API as Backend API
U->>DC: Load Deployments card
DC->>DS: Render deployment statistics (using workspaceSlug)
DS->>API: Request deployment stats (including timezone)
API-->>DS: Return aggregated deployment stats
sequenceDiagram
participant API as DeploymentStatsRouter
participant SC as Schema Module
participant DB as Database
participant AG as Aggregator (Lodash)
API->>SC: Validate request and timezone input
API->>DB: Perform asynchronous query for runStats and join tables
DB-->>API: Return raw deployment run statistics
API->>AG: Aggregate job status counts using analyticsStatuses
API-->>Client: Send combined deployment statistics response
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx (1)
26-46: Clamp successRate to prevent layout overflow
If thesuccessRatevalue ends up greater than 100 or negative, the inline CSS may cause unexpected layout or negative sizing. Consider clamping it to the [0, 100] range.- style={{ height: `${successRate}%` }} + style={{ height: `${Math.max(0, Math.min(successRate ?? 0, 100))}%` }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx(5 hunks)packages/api/src/router/deployment-stats.ts(2 hunks)packages/validators/src/jobs/index.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.
packages/validators/src/jobs/index.tspackages/api/src/router/deployment-stats.tsapps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx (1)
56-61: Double-check off-by-one in last 30 days calculation
Your approach of generating 30 dates fromsubDays(new Date(), 29 - i)should yield exactly 30 entries. Please verify it aligns correctly under all time zone shifts or DST transitions.packages/api/src/router/deployment-stats.ts (3)
25-25: LGTM for added timezone field
Addingtimezoneto the input schema is a solid approach for ensuring accurate date/time calculations.
87-88: Question about the 100-deployment limit
Using.limit(100)might omit deployments if a workspace has more than 100. Double-check that truncating the result is acceptable for your analytics needs.
167-172: Validate merging of runStats and statusCounts
Your merging logic looks correct, but consider verifying there’s no mismatch if either query returns a different set of deployments.packages/validators/src/jobs/index.ts (1)
49-55: Evaluate whether to include additional statuses in analytics
Currently,analyticsStatusesexcludes statuses likecancelledorskipped. Confirm that this omission is intentional if you want a complete picture of job outcomes.
| <td className="p-4"> | ||
| <div className="flex items-center gap-2"> | ||
| <div className="h-2 w-full rounded-full bg-neutral-800"> | ||
| <div | ||
| className="h-full rounded-full bg-white transition-all" | ||
| style={{ | ||
| width: `${((deployment.totalSuccess / deployment.totalJobs) * 100).toFixed(0)}%`, | ||
| }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle division by zero in success rate bar
If deployment.totalJobs is zero, (deployment.totalSuccess / deployment.totalJobs) * 100 will cause an invalid value. Consider adding a fallback to avoid NaN or Infinity.
- width: `${((deployment.totalSuccess / deployment.totalJobs) * 100).toFixed(0)}%`,
+ width: `${
+ deployment.totalJobs > 0
+ ? ((deployment.totalSuccess / deployment.totalJobs) * 100).toFixed(0)
+ : 0
+ }%`,📝 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.
| <td className="p-4"> | |
| <div className="flex items-center gap-2"> | |
| <div className="h-2 w-full rounded-full bg-neutral-800"> | |
| <div | |
| className="h-full rounded-full bg-white transition-all" | |
| style={{ | |
| width: `${((deployment.totalSuccess / deployment.totalJobs) * 100).toFixed(0)}%`, | |
| }} | |
| /> | |
| </div> | |
| <td className="p-4"> | |
| <div className="flex items-center gap-2"> | |
| <div className="h-2 w-full rounded-full bg-neutral-800"> | |
| <div | |
| className="h-full rounded-full bg-white transition-all" | |
| style={{ | |
| width: `${ | |
| deployment.totalJobs > 0 | |
| ? ((deployment.totalSuccess / deployment.totalJobs) * 100).toFixed(0) | |
| : 0 | |
| }%`, | |
| }} | |
| /> | |
| </div> | |
| </div> | |
| </td> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/SkeletonRow.tsx (1)
5-40: LGTM! Well-structured skeleton component with nice fading effect.The component effectively represents the loading state with a consistent table structure and dynamic opacity for visual feedback.
Consider memoizing the component to prevent unnecessary re-renders:
-export const SkeletonRow: React.FC<{ index: number }> = ({ index }) => ( +export const SkeletonRow = React.memo<{ index: number }>(({ index }) => ( <tr className="border-b"> {/* ... existing code ... */} </tr> -); +)); +SkeletonRow.displayName = 'SkeletonRow';apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/TableHeadCell.tsx (1)
9-51: Enhance accessibility of the sortable header cell.While the core sorting functionality is well-implemented, the component needs accessibility improvements.
Apply these changes to improve accessibility:
export const TableHeadCell: React.FC<{ title: string; orderByKey: StatsColumn; }> = ({ title, orderByKey }) => { const { getParam, setParams } = useQueryParams(); const orderByParam = getParam("order-by"); const orderParam = getParam("order"); const isActive = orderByParam === orderByKey; const onOrderByChange = () => { if (orderByParam !== orderByKey) { setParams({ "order-by": orderByKey, order: StatsOrder.Desc }); return; } const newOrder = orderParam === StatsOrder.Asc ? StatsOrder.Desc : StatsOrder.Asc; setParams({ order: newOrder }); }; return ( - <div + <button + type="button" className={cn( "flex cursor-pointer select-none items-center gap-1 text-nowrap", isActive && "font-semibold text-white", )} onClick={onOrderByChange} + aria-sort={ + !isActive + ? undefined + : orderParam === StatsOrder.Asc + ? "ascending" + : "descending" + } > {title} {isActive && ( <IconChevronDown className={cn( orderParam === StatsOrder.Asc && "rotate-180", "h-4 w-4 transition-transform", )} strokeWidth={1.5} + aria-hidden="true" /> )} - </div> + </button> ); };apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentHistoryGraph.tsx (2)
7-17: Consider simplifying the opacity calculation.The opacity calculation can be simplified for better readability.
- style={{ opacity: 1 * (1 - i / 30) }} + style={{ opacity: 1 - i / 30 }}
21-54: Consider enhancing color accessibility.The red/green color scheme might be problematic for colorblind users. Consider adding aria labels and patterns/textures for better accessibility.
<div key={j} - className="relative h-full w-1.5 overflow-hidden rounded-sm" + className="relative h-full w-1.5 overflow-hidden rounded-sm" + role="progressbar" + aria-label={`Success rate: ${successRate ?? 'No data'}%`} > {successRate == null ? ( <div className="absolute bottom-0 h-full w-full bg-neutral-700" /> ) : ( <> - <div className="absolute bottom-0 h-full w-full bg-red-500" /> + <div className="absolute bottom-0 h-full w-full bg-red-500 [background-image:linear-gradient(45deg,rgba(0,0,0,.1)_25%,transparent_25%,transparent_50%,rgba(0,0,0,.1)_50%,rgba(0,0,0,.1)_75%,transparent_75%,transparent)]" /> <div - className="absolute bottom-0 w-full bg-green-500" + className="absolute bottom-0 w-full bg-green-500 [background-image:linear-gradient(-45deg,rgba(255,255,255,.1)_25%,transparent_25%,transparent_50%,rgba(255,255,255,.1)_50%,rgba(255,255,255,.1)_75%,transparent_75%,transparent)]" style={{ height: `${successRate}%` }} /> </> )} </div>apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentRow.tsx (1)
41-50: Consider memoizing formatted durations.The
prettyMillisecondscalculations could be memoized to prevent unnecessary recalculations on re-renders.+const formatDuration = (value: number | null) => { + if (value == null) return "N/A"; + return prettyMilliseconds(Math.round(value) * 1000); +}; <td className="p-4 "> - {deployment.p50 != null - ? prettyMilliseconds(Math.round(deployment.p50) * 1000) - : "N/A"} + {formatDuration(deployment.p50)} </td> <td className="p-4 "> - {deployment.p90 != null - ? prettyMilliseconds(Math.round(deployment.p90) * 1000) - : "N/A"} + {formatDuration(deployment.p90)} </td>apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentTable.tsx (1)
72-81: Consider optimizing list rendering performance.The list rendering could be optimized by using
useMemofor the loading skeleton rows and adding a properkeyprop strategy.+const SKELETON_ROW_COUNT = 3; +const skeletonRows = Array.from({ length: SKELETON_ROW_COUNT }).map((_, i) => ( + <SkeletonRow key={`skeleton-${i}`} index={i} /> +)); <TableBody> {!isLoading && data?.map((deployment) => ( <DeploymentRow key={deployment.id} deployment={deployment} /> ))} - {isLoading && - Array.from({ length: 3 }).map((_, i) => ( - <SkeletonRow key={i} index={i} /> - ))} + {isLoading && skeletonRows} </TableBody>apps/webservice/src/app/[workspaceSlug]/(app)/deployments/AggregateCharts.tsx (2)
6-6: Remove commented-out import.The import statement for
prettyMillisecondsis duplicated and commented out.-// import prettyMilliseconds from "pretty-ms";
42-65: Optimize calculations and improve type safety.The calculations could be optimized by reducing chain operations and improving type safety.
- const totalJobs = data != null ? _.sumBy(data, (d) => d.totalJobs) : 0; + const totalJobs = data?.reduce((sum, d) => sum + d.totalJobs, 0) ?? 0; const totalDuration = - data != null - ? _.chain(data) - .filter((d) => isPresent(d.totalDuration)) - .sumBy((d) => d.totalDuration!) - .value() - : 0; + data?.reduce((sum, d) => sum + (d.totalDuration ?? 0), 0) ?? 0; const totalDurationMs = Math.round(totalDuration * 1000); + const formatDuration = (ms: number) => prettyMilliseconds(ms, { + unitCount: 2, + secondsDecimalDigits: 0, + }); - const totalDurationPretty = prettyMilliseconds(totalDurationMs, { - unitCount: 2, - secondsDecimalDigits: 0, - }); + const totalDurationPretty = formatDuration(totalDurationMs); - const totalSuccess = data != null ? _.sumBy(data, (d) => d.totalSuccess) : 0; + const totalSuccess = data?.reduce((sum, d) => sum + d.totalSuccess, 0) ?? 0; const totalSuccessRate = data != null ? (totalSuccess / totalJobs) * 100 : 0; const totalSuccessRatePretty = `${totalSuccessRate.toFixed(2)}%`; const averageDurationMs = data != null ? Math.round(totalDurationMs / totalJobs) : 0; - const averageDurationPretty = prettyMilliseconds(averageDurationMs, { - unitCount: 2, - secondsDecimalDigits: 0, - }); + const averageDurationPretty = formatDuration(averageDurationMs);apps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx (1)
79-85: Enhance search input accessibility.Consider adding
aria-labelandrole="search"to improve accessibility for screen readers.<Input placeholder="Search..." value={search} onChange={(e) => setSearch(e.target.value)} - className="w-80 pl-8" + className="w-80 pl-8" + aria-label="Search deployments" + role="search" />packages/api/src/router/deployment-stats.ts (1)
160-227: Consider adding caching for historical data.The history query performs complex calculations that don't change frequently. Consider implementing caching to improve performance.
Consider implementing Redis caching with a TTL of 1 hour for the historical data, as it's unlikely to change frequently within that timeframe.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/AggregateCharts.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentHistoryGraph.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/SkeletonRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/TableHeadCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/deployments/page.tsx(1 hunks)packages/api/src/router/deployment-stats.ts(2 hunks)packages/validators/package.json(1 hunks)packages/validators/src/deployments/index.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/[workspaceSlug]/(app)/deployments/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/SkeletonRow.tsxpackages/validators/src/deployments/index.tsapps/webservice/src/app/[workspaceSlug]/(app)/deployments/AggregateCharts.tsxapps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/TableHeadCell.tsxapps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentRow.tsxpackages/api/src/router/deployment-stats.tsapps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentHistoryGraph.tsxapps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
packages/validators/src/deployments/index.ts (1)
1-20: LGTM! Well-structured enums and validators.The enums and Zod validators are well-defined with consistent naming conventions. The column names use kebab-case which is appropriate for URL parameters, and the sort order values follow standard conventions.
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/page.tsx (1)
28-28: LGTM! Clean and focused page structure.The page component is well-structured, follows Next.js conventions, and correctly handles workspace validation.
apps/webservice/src/app/[workspaceSlug]/(app)/deployments/deployment-table/DeploymentHistoryGraph.tsx (1)
56-66: LGTM! Well-implemented lazy loading.The use of
useInViewfor lazy loading is a good performance optimization.apps/webservice/src/app/[workspaceSlug]/(app)/deployments/Card.tsx (2)
21-28: LGTM! Well-structured utility function.The
getStartDatefunction effectively handles different time periods and provides clear fallback behavior.
47-59: Validate timezone string before using it in API calls.While using the browser's timezone is convenient, it's good practice to validate the timezone string to ensure it's in a format the backend can process.
packages/api/src/router/deployment-stats.ts (2)
56-82: LGTM! Well-implemented SQL metrics calculations.The SQL calculations for p50, p90, totalDuration, and successRate are well-structured with proper null handling and type casting.
86-95: LGTM! Comprehensive order by implementation.The
getOrderByfunction handles all possible sorting columns with a sensible default.packages/validators/package.json (1)
51-54: LGTM! Consistent export configuration.The new deployments export follows the established pattern and properly defines both types and default exports.
| <Link | ||
| href={`/${workspaceSlug}/systems/${deployment.systemSlug}/deployments/${deployment.slug}/releases`} | ||
| target="_blank" | ||
| > | ||
| <div className="flex items-center gap-2">{deployment.name}</div> | ||
| <div className="text-xs text-muted-foreground"> | ||
| {deployment.systemName} / {deployment.name} | ||
| </div> | ||
| </Link> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security attributes to external link.
When using target="_blank", it's recommended to add rel="noopener noreferrer" to prevent potential security vulnerabilities.
<Link
href={`/${workspaceSlug}/systems/${deployment.systemSlug}/deployments/${deployment.slug}/releases`}
target="_blank"
+ rel="noopener noreferrer"
>📝 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.
| <Link | |
| href={`/${workspaceSlug}/systems/${deployment.systemSlug}/deployments/${deployment.slug}/releases`} | |
| target="_blank" | |
| > | |
| <div className="flex items-center gap-2">{deployment.name}</div> | |
| <div className="text-xs text-muted-foreground"> | |
| {deployment.systemName} / {deployment.name} | |
| </div> | |
| </Link> | |
| <Link | |
| href={`/${workspaceSlug}/systems/${deployment.systemSlug}/deployments/${deployment.slug}/releases`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| <div className="flex items-center gap-2">{deployment.name}</div> | |
| <div className="text-xs text-muted-foreground"> | |
| {deployment.systemName} / {deployment.name} | |
| </div> | |
| </Link> |
Summary by CodeRabbit
New Features
DeploymentsCardcomponent with enhanced search functionality and time period selection.AggregateChartsandDeploymentTablecomponents for improved visualization of deployment statistics.DeploymentRowcomponent to display detailed deployment information within a table format.SkeletonRowcomponent for better loading state representation.DeploymentHistoryGraphcomponent for visualizing deployment success rates.Bug Fixes
Refactor
DeploymentsPageby removing unnecessary billing information.