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
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 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)
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.
There was a problem hiding this comment.
- Remove white borders from bars
- Legends dark text should be brighter and same color as the legends. (you do have access to the source visual no?)
- Format the duration instead of just being in hours. Try
getDurationString. It's in the webserver codebase.
Additionally, lint issues.
Please run the linter before you push. Or, you could help us by updating the configuration to run it as part of the precommit hook.
|
The code is largely fine tbh. Good job. |
|
@harshit078 please update your screenshots after visual changes. |
|
@jayantbh sure, I'll add the screen recording along with screenshots by today ! |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (4)
48-51: TabKeys enum & state initialization
TheTabKeysenum and the initialization ofactiveTabwith its correspondingtabItemscorrectly model the two views. For stronger type safety, consider defining the state asuseState<TabKeys>(TabKeys.EVENTS)instead ofstringto eliminate the need for casting inhandleTabSelect.Also applies to: 59-64
361-364: Remove unnecessary empty FlexBox
There’s an unused<FlexBox></FlexBox>wrapper immediately beforeDoraMetricsTrend. It doesn’t affect layout and can be removed to keep the markup clean.
315-324: Avoid redundantBooleancasts on.lengthchecks
Rather than{Boolean(failedDeps.length) ? …}, you can simplify to{failedDeps.length > 0 ? …}and similarly for the pass branch. This removes extra function calls and improves readability.Also applies to: 327-335
🧰 Tools
🪛 Biome (1.9.4)
[error] 315-315: 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)
297-378: Analytics tab: layout refinements
The analytics pane correctly presents deployment counts, the progress bar, and conditionally renders metrics or a settings link. A couple of optional tweaks:
- Use
ROUTES.TEAMS.PATHinstead ofROUTES.TEAMS.ROUTE.PATHfor brevity.- Add a brief comment on
key={selectedRepo.value}for the<Box>to explain the remount-on-repo-change hack.🧰 Tools
🪛 Biome (1.9.4)
[error] 315-315: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (6)
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(118-165)web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (1)
DoraMetricsTrend(243-456)web-server/src/constants/routes.ts (1)
ROUTES(58-73)web-server/src/components/PRTableMini/PullRequestsTableMini.tsx (1)
PullRequestsTableMini(42-378)
🪛 Biome (1.9.4)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx
[error] 315-315: 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 (5)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (5)
5-7: Clean introduction of new imports
The new imports for React hooks (FC,useCallback,useEffect,useMemo,useState),DoraMetricsTrend,DoraMetricsDuration,Tabs,Link,ROUTES, andDeploymentSourcesare all used in this component and correctly referenced.Also applies to: 14-14, 34-36
229-232: Selected repo styling is applied correctly
The conditionalborderandfontWeightprops on each repo card clearly highlight the selected repo. This dynamic styling is implemented as intended.Also applies to: 244-246
201-203: Determine PR_MERGE deployment source
AddingcurrentBaseRepoandisPRMergeSourceto gate display of analytics is appropriate. Please verify that thedeployment_typeproperty on your repo objects always matches theDeploymentSourcesenum values (e.g.,PR_MERGE).
292-296: Tabs component integration
TheTabscomponent is correctly wired withitems,onSelect, andcheckSelected. Users will be able to switch between "Deployment Analytics" and "Deployment Events" seamlessly.
379-380: Events tab content preserved under tabs
The deployment events UI is now properly nested under the "Deployment Events" tab and all existing functionality (filters, grouping, PR details) remains intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (3)
319-319: Remove redundant Boolean call.The
Boolean()call is unnecessary sincefailedDeps.lengthalready evaluates to a truthy/falsy value in this context.- {Boolean(failedDeps.length) ? ( + {failedDeps.length ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 319-319: 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)
359-367: Consider adding deployment trends even for PR_MERGE sources.The conditional rendering logic is correct, but consider if there are alternative metrics that could be shown for PR_MERGE sources instead of completely hiding the analytics.
The current implementation appropriately hides deployment trends for PR_MERGE sources, but you might consider showing alternative analytics or a more informative message about what metrics are available for this deployment type.
364-364: Remove empty FlexBox element.This empty FlexBox serves no purpose and should be removed for cleaner code.
- <FlexBox></FlexBox>
📜 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(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web-server/src/content/DoraMetrics/DoraMetricsDuration.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx
[error] 319-319: 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 (7)
web-server/src/content/PullRequests/DeploymentInsightsOverlay.tsx (7)
3-3: LGTM! Clean import organization for new tabbed functionality.The new imports are well-organized and properly support the tabbed interface refactoring. All imported components and utilities are used appropriately throughout the component.
Also applies to: 14-14, 16-16, 33-33, 39-40
50-54: Well-structured tab key enumeration.The TabKeys enum provides a clean and maintainable way to manage tab identifiers, following good TypeScript practices.
61-66: Solid tab state management implementation.The tab state initialization and handling logic is clean and follows React best practices. The tab items configuration and selection handler are properly structured.
203-208: Good conditional logic for deployment source detection.The logic to detect PR_MERGE deployment sources is well-implemented and serves the important purpose of conditionally showing deployment trends based on the repository's configuration.
296-300: Clean tab component integration.The Tabs component integration is properly implemented with appropriate props and selection handling.
368-386: Well-crafted user guidance for unsupported deployment sources.The informational message and link to settings provides clear guidance to users about why deployment trends aren't available and how to potentially enable them.
388-503: Excellent preservation of existing events functionality.The events tab maintains all the existing deployment filtering, grouping, and selection functionality while being properly integrated into the new tabbed interface. The layout and user experience are preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (1)
338-340: Guard against empty data when computing Math.max (addressing previous review).This matches the exact issue flagged in the past review comment. When
durationsis an empty array,Math.max(...durations)returns-Infinity, which would break the y-axis.The previous review comment is still valid and should be addressed:
-const maxDuration = Math.max(...durations); -const yAxisMax = Math.ceil(maxDuration); +const maxDuration = + durations.length > 0 ? Math.max(...durations) : 0; +const yAxisMax = Math.ceil(maxDuration);
🧹 Nitpick comments (4)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (4)
120-120: Remove console.log statement.Console.log statements should not be left in production code.
- console.log('prCount', deployments[0]);
184-190: Remove unused props from component interface.The
DeploymentTrendPillcomponent acceptsvalueandvalueFormatprops but doesn't use them in the implementation.export const DeploymentTrendPill: FC<{ label: string; change: number; state: 'positive' | 'negative' | 'neutral'; - value: number; - valueFormat?: (val: number) => string; -}> = ({ label, change, state }) => { +}> = ({ label, change, state }) => {
67-77: Consider extracting duplicate validation logic.The deployment validation logic is duplicated between
calculateDeploymentTrendsand thechartDatauseMemo. This violates DRY principles and increases maintenance burden.Consider extracting this into a shared validation function:
+const isValidDeployment = (dep: Deployment): boolean => { + const hasValidDates = + dep.conducted_at && + new Date(dep.conducted_at).toString() !== 'Invalid Date'; + + if (dep.id?.startsWith('WORKFLOW')) { + return ( + hasValidDates && + typeof dep.run_duration === 'number' && + dep.run_duration >= 0 + ); + } + + return ( + hasValidDates && + dep.created_at && + new Date(dep.created_at).toString() !== 'Invalid Date' + ); +};Then use this function in both places to reduce code duplication.
Also applies to: 258-276
392-450: Chart configuration complexity could benefit from extraction.The chart options configuration is quite complex and embedded within the component. Consider extracting it to improve readability and testability.
Consider creating a separate function to generate chart options:
const createChartOptions = (theme: Theme, yAxisMax: number) => ({ scales: { y: { type: 'linear', display: true, position: 'left', title: { display: true, text: 'Duration (minutes)', color: theme.colors.success.main }, ticks: { color: theme.colors.success.main, callback: (value) => value + 'm' }, max: yAxisMax, grid: { color: darken(theme.colors.success.lighter, 0.2) } }, // ... rest of configuration } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx(1 hunks)
🔇 Additional comments (1)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (1)
263-263:⚠️ Potential issueAdd null check for deployment ID.
Similar to line 72, this code assumes
dep.idexists without null checking.- if (dep.id?.startsWith('WORKFLOW')) { + if (dep.id?.startsWith('WORKFLOW')) {Wait, I see this already has the optional chaining operator. This is actually correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (1)
72-74:⚠️ Potential issueNull-safety missing on
dep.id– re-introduces the exact crash flagged earlier
dep.idis still accessed without an optional-chain, soundefined.startsWithwill throw when deployments come from sources that omitid.
The same issue was pointed out in a previous review; please address it this time.- if (dep.id.startsWith('WORKFLOW')) { + if (dep.id?.startsWith('WORKFLOW')) {
🧹 Nitpick comments (2)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (2)
120-123: Left-over debugconsole.logThe stray
console.log('prCount', deployments[0]);leaks implementation details to the browser console and needlessly clutters logs.- console.log('prCount', deployments[0]);
18-23: Dead fieldsizeinTrendData
sizeis declared but never referenced anywhere in this file. Drop it until a use-case emerges to keep the type lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx (2)
web-server/src/types/resources.ts (1)
Deployment(216-228)cli/source/store/index.ts (1)
useSelector(28-28)
| export const DeploymentTrendPill: FC<{ | ||
| label: string; | ||
| change: number; | ||
| state: 'positive' | 'negative' | 'neutral'; | ||
| value: number; | ||
| valueFormat?: (val: number) => string; | ||
| }> = ({ label, change, state }) => { | ||
| const theme = useTheme(); | ||
|
|
||
| const text = | ||
| state === 'neutral' | ||
| ? label | ||
| : state === 'positive' | ||
| ? 'Increasing ' + label | ||
| : 'Decreasing ' + label; | ||
|
|
||
| const useMultiplierFormat = Math.abs(change) > 100; | ||
| const formattedChange = useMultiplierFormat | ||
| ? `${percentageToMultiplier(Math.abs(change))}` | ||
| : `${Math.abs(Math.round(change))}%`; |
There was a problem hiding this comment.
value / valueFormat props are unused – compilation will fail under noUnusedParameters
DeploymentTrendPill’s signature exposes value and valueFormat, yet neither is referenced in the body. Besides misleading API consumers, this breaks builds with stricter TS configs.
Two options:
- Remove the unused props:
-export const DeploymentTrendPill: FC<{
- label: string;
- change: number;
- state: 'positive' | 'negative' | 'neutral';
- value: number;
- valueFormat?: (val: number) => string;
-}> = ({ label, change, state }) => {
+export const DeploymentTrendPill: FC<{
+ label: string;
+ change: number;
+ state: 'positive' | 'negative' | 'neutral';
+}> = ({ label, change, state }) => {- Or actually surface the value:
const formattedChange = useMultiplierFormat
? `${percentageToMultiplier(Math.abs(change))}`
: `${Math.abs(Math.round(change))}%`;
const color = darken(
@@
<FlexBox color={color} alignCenter>
<Line bold>{formattedChange}</Line>
+ {value !== undefined && (
+ <Line sx={{ ml: 0.5 }}>{valueFormat ? valueFormat(value) : value}</Line>
+ )}
{icon}
</FlexBox>Choose whichever matches your UX intention.
📝 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.
| export const DeploymentTrendPill: FC<{ | |
| label: string; | |
| change: number; | |
| state: 'positive' | 'negative' | 'neutral'; | |
| value: number; | |
| valueFormat?: (val: number) => string; | |
| }> = ({ label, change, state }) => { | |
| const theme = useTheme(); | |
| const text = | |
| state === 'neutral' | |
| ? label | |
| : state === 'positive' | |
| ? 'Increasing ' + label | |
| : 'Decreasing ' + label; | |
| const useMultiplierFormat = Math.abs(change) > 100; | |
| const formattedChange = useMultiplierFormat | |
| ? `${percentageToMultiplier(Math.abs(change))}` | |
| : `${Math.abs(Math.round(change))}%`; | |
| export const DeploymentTrendPill: FC<{ | |
| label: string; | |
| change: number; | |
| state: 'positive' | 'negative' | 'neutral'; | |
| }> = ({ label, change, state }) => { | |
| const theme = useTheme(); | |
| const text = | |
| state === 'neutral' | |
| ? label | |
| : state === 'positive' | |
| ? 'Increasing ' + label | |
| : 'Decreasing ' + label; | |
| const useMultiplierFormat = Math.abs(change) > 100; | |
| const formattedChange = useMultiplierFormat | |
| ? `${percentageToMultiplier(Math.abs(change))}` | |
| : `${Math.abs(Math.round(change))}%`; |
🤖 Prompt for AI Agents
In web-server/src/content/DoraMetrics/DoraMetricsTrend.tsx around lines 184 to
203, the DeploymentTrendPill component declares value and valueFormat props but
does not use them, causing unused parameter errors under strict TypeScript
settings. To fix this, either remove value and valueFormat from the component's
props if they are unnecessary, or update the component to use these props in its
rendering logic, such as formatting and displaying the value using valueFormat
if provided.
Linked Issue(s)
#544
Acceptance Criteria fulfillment
Proposed changes
Further comments
Summary by CodeRabbit
New Features
UI Improvements
Summary by CodeRabbit
New Features
Refactor