fix: Add Trends Line graph on Deployment events page #652
Conversation
WalkthroughTwo new React components, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeploymentInsightsOverlay
participant DoraMetricsDuration
participant DoraMetricsTrend
User->>DeploymentInsightsOverlay: Open overlay
DeploymentInsightsOverlay->>User: Show tabs (Analytics, Events)
User->>DeploymentInsightsOverlay: Select "Deployment Analytics" tab
DeploymentInsightsOverlay->>DoraMetricsDuration: Render with deployments
DeploymentInsightsOverlay->>DoraMetricsTrend: Render with deployments
DoraMetricsDuration-->>DeploymentInsightsOverlay: Display duration cards
DoraMetricsTrend-->>DeploymentInsightsOverlay: Display trend chart
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (2)
1-5: Remove unused icon imports to avoid lint-failures
CodeRoundedandAccessTimeRoundedare imported but never referenced in this file, which will be flagged by the build/linter and bloats bundle size.-import { ArrowDownwardRounded, CodeRounded, AccessTimeRounded } from '@mui/icons-material'; +import { ArrowDownwardRounded } from '@mui/icons-material';
225-233: Replace redundantBoolean(deps.length)with truthy testThe explicit cast is unnecessary;
deps.lengthis already coerced totrue / false.
This also satisfies the Biome warning reported in the static-analysis hints.- {Boolean(deps.length) && ( + {deps.length > 0 && (web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx (1)
133-141: Redundant boolean castSame as in the overlay file—
Boolean(d.conducted_at)can be replaced by truthiness testing:- .filter((d): d is Deployment => Boolean(d.conducted_at)) + .filter((d): d is Deployment => !!d.conducted_at)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx(1 hunks)web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx (1)
web-server/src/types/resources.ts (1)
Deployment(216-228)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (4)
web-server/src/hooks/useStateTeamConfig.tsx (2)
useSingleTeamConfig(130-154)useBranchesForPrFilters(163-174)cli/source/store/index.ts (1)
useDispatch(30-30)web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx (1)
DoraMetricsDuration(126-180)web-server/src/components/PRTableMini/PullRequestsTableMini.tsx (1)
PullRequestsTableMini(42-378)
🪛 Biome (1.9.4)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx
[error] 310-310: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (1)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (1)
334-350: Guard against divide-by-zero in success-rate computation
percent(successfulDeps.length, deployments.length)will throw ifdeployments.lengthis0.
Although the surrounding conditional currently prevents an empty list from being rendered, an early return or a defensive check here protects against future refactors.- perc={percent( - successfulDeps.length, - deployments.length - )} + perc={ + deployments.length + ? percent(successfulDeps.length, deployments.length) + : 0 + }
| const DeploymentCard: FC<DeploymentCardProps> = ({ deployment }) => { | ||
| const theme = useTheme(); | ||
|
|
||
| const handleDeploymentClick = () => { | ||
| if (deployment.html_url) { | ||
| window.open(deployment.html_url, '_blank', 'noopener,noreferrer'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DeploymentCard crashes when deployment is null
longestDeployment / shortestDeployment can be null, yet the prop is typed as non-nullable.
This causes a TypeScript error and a potential runtime exception when there are no valid deployments.
-interface DeploymentCardProps {
- deployment: DeploymentWithValidDuration;
+interface DeploymentCardProps {
+ deployment?: DeploymentWithValidDuration | null;Add an early return inside the component:
- const theme = useTheme();
+ const theme = useTheme();
+ if (!deployment) {
+ return (
+ <Card elevation={0} sx={{ p: 1.6, flex: 1, textAlign: 'center' }}>
+ <Line white>No data</Line>
+ </Card>
+ );
+ }And adjust the caller:
-<DeploymentCard deployment={longestDeployment} type="Longest" />
+<DeploymentCard deployment={longestDeployment} type="Longest" />There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx (1)
147-161:⚠️ Potential issue❗
DeploymentCardis still called with a possiblenullvalue
longestDeployment/shortestDeploymentcan benull(see theuseMemoabove) yet the prop is typed as non-nullable and the component never checks for it.
This reproduces the compile/runtime issue flagged in the previous review and will crash or fail the type-check as soon as a repo has no valid deployments.Suggested fix (same as before):
-interface DeploymentCardProps { - deployment: Deployment; +interface DeploymentCardProps { + deployment?: Deployment | null; type: DeploymentCardType; } … -const DeploymentCard: FC<DeploymentCardProps> = ({ deployment }) => { +const DeploymentCard: FC<DeploymentCardProps> = ({ deployment }) => { + if (!deployment) { + return ( + <Card elevation={0} sx={{ p: 1.6, flex: 1, textAlign: 'center' }}> + <Line white>No data</Line> + </Card> + ); + } const theme = useTheme();and render the card only when data is present:
-{/* Longest */} -<DeploymentCard deployment={longestDeployment} type="Longest" /> +{longestDeployment && ( + <DeploymentCard deployment={longestDeployment} type="Longest" /> +)} -{/* Shortest */} -<DeploymentCard deployment={shortestDeployment} type="Shortest" /> +{shortestDeployment && ( + <DeploymentCard deployment={shortestDeployment} type="Shortest" /> +)}
🧹 Nitpick comments (7)
web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx (3)
124-126: RedundantBoolean()cast
Boolean(...)is unnecessary—JavaScript already coerces the expression to a boolean in the filter callback.
Removing it also satisfies the Biome warning.-.filter((d): d is Deployment => Boolean(d.conducted_at && typeof d.run_duration === 'number')) +.filter( + (d): d is Deployment => + d.conducted_at && typeof d.run_duration === 'number' +)
13-16: Unusedtypeprop
typeis declared inDeploymentCardPropsand passed by the parent but is never read insideDeploymentCard.
Drop the prop (and its derivations) or surface it in the UI to avoid dead code.
91-97: Minor grammar – plural apostrophe
new PR'sshould benew PRs(no apostrophe) to indicate plural, not possession.- {deployment.pr_count || 0} new PR's + {deployment.pr_count || 0} new PRsweb-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (1)
56-61: Prefer strongly-typed state for tab key
useState<string>(TabKeys.EVENTS)weakens type-safety. Constrain the state to theTabKeysenum so accidental strings are impossible.-const [activeTab, setActiveTab] = useState<string>(TabKeys.EVENTS); +const [activeTab, setActiveTab] = useState<TabKeys>(TabKeys.EVENTS); … -const handleTabSelect = (item: TabItem) => setActiveTab(item.key as string); +const handleTabSelect = (item: TabItem) => + setActiveTab(item.key as TabKeys);web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (3)
70-74: Remove strayconsole.logLeft-over debugging statement leaks to the browser console and can expose data in production.
- console.log(deployments)
15-16: Threshold is probably too low
MEANINGFUL_CHANGE_THRESHOLD = 0.5is compared against a percentage (e.g.,20,-3.4).
0.5therefore means “ignore < 0.5 % change”, which is almost noise-level.
Consider raising this to something like5or10% or make it configurable.
149-155:valueprop is unused
DeploymentTrendPillaccepts avalue(and optionalvalueFormat) but never uses it, which confuses callers and bloats render cycles.Either:
- Display the value in the pill, or
- Remove the prop from the interface and the call-sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx(1 hunks)web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx(1 hunks)web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx (1)
web-server/src/types/resources.ts (1)
Deployment(216-228)
🪛 Biome (1.9.4)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx
[error] 308-308: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
| const maxDuration = Math.max(...durations); | ||
| const yAxisMax = Math.ceil(maxDuration); | ||
|
|
There was a problem hiding this comment.
Guard against empty data when computing Math.max
When there are no deployments durations is an empty array and Math.max(...durations) returns -Infinity, breaking the y-axis.
-const maxDuration = Math.max(...durations);
-const yAxisMax = Math.ceil(maxDuration);
+const maxDuration =
+ durations.length > 0 ? Math.max(...durations) : 0;
+const yAxisMax = Math.ceil(maxDuration);📝 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 maxDuration = Math.max(...durations); | |
| const yAxisMax = Math.ceil(maxDuration); | |
| const maxDuration = | |
| durations.length > 0 ? Math.max(...durations) : 0; | |
| const yAxisMax = Math.ceil(maxDuration); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (7)
56-62: Consider defaulting to the Analytics tab.Since the PR introduces new analytics features as a major enhancement, it might make more sense to default to the ANALYTICS tab instead of EVENTS to highlight the new functionality for users.
- const [activeTab, setActiveTab] = useState<string>(TabKeys.EVENTS); + const [activeTab, setActiveTab] = useState<string>(TabKeys.ANALYTICS);
293-293: Fix spacing in style object.Minor formatting issue with the spacing in the style object.
- <Box sx={{mb:'10px'}} key={selectedRepo.value}> + <Box sx={{ mb: '10px' }} key={selectedRepo.value}>
308-308: Remove redundant Boolean call.The Boolean wrapper is unnecessary as the expression will already be coerced to a boolean in this context.
- {Boolean(failedDeps.length) ? ( + {failedDeps.length ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 308-308: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
351-355: Consider adding descriptions for analytics components.The DoraMetricsDuration and DoraMetricsTrend components are added without any surrounding context or descriptions for users. Consider adding brief explanatory text to help users understand what these visualizations represent.
<FlexBox> + <Line white medium sx={{ mb: 1 }}>Deployment Duration Analysis</Line> <DoraMetricsDuration deployments={deployments} /> </FlexBox> <FlexBox></FlexBox> + <Line white medium sx={{ mb: 1 }}>Deployment Trend Analysis</Line> <DoraMetricsTrend />
354-354: Remove empty FlexBox.There's an empty FlexBox that doesn't serve any purpose and should be removed.
<FlexBox> <DoraMetricsDuration deployments={deployments} /> </FlexBox> - <FlexBox></FlexBox> <DoraMetricsTrend />
290-473: Consider extracting tab content into separate components.The component is quite large and handles multiple concerns. Consider extracting the Analytics tab and Events tab content into separate components to improve readability and maintainability.
For example:
- Create a
DeploymentAnalyticsTabcomponent- Create a
DeploymentEventsTabcomponentThis refactoring would make the main component more focused on managing the tab state and layout.
🧰 Tools
🪛 Biome (1.9.4)
[error] 308-308: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
349-350: Maintain consistent indentation.The indentation in the analytics tab doesn't appear to be consistently applied, particularly for the closing tag of FlexBox.
/> </FlexBox> - </Box> + </Box>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx(1 hunks)web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx
[error] 308-308: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (3)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (3)
5-7: Good addition of new imports for tab functionality and analytics components.The imports for useState and the new DORA metrics components (DoraMetricsTrend and DoraMetricsDuration) are correctly added to support the new tabbed interface and analytics features.
45-48: Good use of enum for tab identification.Using an enum for tab keys is a clean approach that prevents magic strings and makes the code more maintainable.
285-289: Clean implementation of the Tabs component.The Tabs component is well integrated with appropriate props for items, selection handling, and active tab checking.
Linked Issue(s)
#544
Acceptance Criteria fulfillment
Proposed changes
Further comments
Summary by CodeRabbit
New Features
UI Improvements
Summary by CodeRabbit
New Features
Refactor