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
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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 8000 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); |
Linked Issue(s)
#544
Acceptance Criteria fulfillment
Proposed changes
Further comments
Summary by CodeRabbit
New Features
UI Improvements
Summary by CodeRabbit
New Features
Refactor