feat: rollout drawer #634
feat: rollout drawer #634
Conversation
WalkthroughThis change introduces a comprehensive rollout visualization and management drawer for deployments. It adds new React components for displaying rollout curves, pie charts, and percentage cards, integrates a rollout drawer UI, and updates existing tables and cells to provide rollout triggers. Supporting hooks, utility modules, and API adjustments are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Table
participant RolloutDrawerTrigger
participant useRolloutDrawer
participant RolloutDrawer
participant API
participant ChartsSection
User->>Table: Clicks "View rollout"
Table->>RolloutDrawerTrigger: Renders trigger button
RolloutDrawerTrigger->>useRolloutDrawer: setEnvironmentVersionIds(envId, versionId)
useRolloutDrawer->>RolloutDrawer: Updates URL, opens drawer
RolloutDrawer->>API: Fetch rollout data for envId/versionId
API-->>RolloutDrawer: Returns rollout info
RolloutDrawer->>ChartsSection: Passes IDs for chart rendering
ChartsSection->>ChartsSection: Renders charts (curve, percent, pie)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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
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 (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/rollout-charts/colors.ts (1)
1-16: LGTM! Good centralized color management.The color palette provides good variety for chart visualizations and uses Tailwind's consistent color system. Consider verifying that the color combinations provide sufficient contrast for accessibility compliance when used together in charts.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/rollout-charts/rollout.ts (1)
3-3: Remove unused lodash import.The lodash import is not used anywhere in the file and should be removed to keep the code clean.
Apply this diff to remove the unused import:
-import _ from "lodash";apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/rollout-charts/RolloutCurve.tsx (4)
34-54: Consider improving type safety for the props parameter.The component correctly formats time values for the Y-axis. However, the
anytype for props could be improved for better type safety.Consider typing the props more specifically:
-const PrettyYAxisTick = (props: any) => { +const PrettyYAxisTick = (props: { payload: { value: number } } & any) => {While not ideal, this pattern is common with recharts custom components where the full prop interface is complex.
34-54: Y-axis tick formatting is well-implementedThe custom tick component properly converts minutes to milliseconds and uses
pretty-msfor human-readable formatting. The props typing could be more specific.-const PrettyYAxisTick = (props: any) => { +const PrettyYAxisTick = (props: { payload: { value: string } } & any) => {
77-77: Array access could be saferUsing
at()with a number that could be out of bounds. Consider adding bounds checking.- const releaseTarget = props.rolloutInfoList.at(Number(position)); + const positionIndex = Number(position); + const releaseTarget = positionIndex >= 0 && positionIndex < props.rolloutInfoList.length + ? props.rolloutInfoList[positionIndex] + : null;
162-165: Consider refetch interval configurationThe 10-second refetch interval is hardcoded. Consider making this configurable or using a more strategic refetch strategy based on rollout status.
const { data: rolloutInfo } = api.policy.rollout.list.useQuery( { environmentId, versionId }, - { refetchInterval: 10_000 }, + { + refetchInterval: rolloutInfo?.isActive ? 10_000 : 30_000, + refetchIntervalInBackground: false + }, );apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/useRolloutDrawer.ts (1)
9-10: Consider more robust parameter parsing.The current parameter parsing using
split(delimiter)could be more robust. If the URL parameter is malformed or contains unexpected data, it might not handle edge cases gracefully.Consider adding validation or more defensive parsing:
- const [environmentId, versionId, releaseTargetId] = - paramValue?.split(delimiter) ?? []; + const parts = paramValue?.split(delimiter) ?? []; + const [environmentId, versionId, releaseTargetId] = [ + parts[0] || undefined, + parts[1] || undefined, + parts[2] || undefined + ];apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/rollout-charts/RolloutPieChart.tsx (1)
17-17: Empty config object could be more explicitThe
ChartContainerreceives an empty config object. Consider making this more explicit or adding a comment explaining why no configuration is needed.- <ChartContainer config={{}}> + <ChartContainer config={{}} className="h-full w-full">apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/RolloutDrawer.tsx (2)
22-45: Consider adding error handling for URL construction.The PolicyLink component properly constructs URLs and uses appropriate link attributes. However, consider what happens if
workspaceSlugis undefined or if URL construction fails.const PolicyLink: React.FC<{ policy: { id: string; name: string } }> = ({ policy, }) => { const { workspaceSlug } = useParams<{ workspaceSlug: string }>(); + + if (!workspaceSlug) { + return null; + } + const policyUrl = urls .workspace(workspaceSlug) .policies() .edit(policy.id) .rollouts();
69-69: Consider renaming for clarity.The variable name
setIsOpenis misleading since it's actually used to close the drawer by removing environment and version IDs.- const setIsOpen = removeEnvironmentVersionIds; + const closeDrawer = removeEnvironmentVersionIds;And update the usage:
- <Drawer open={isOpen} onOpenChange={setIsOpen}> + <Drawer open={isOpen} onOpenChange={closeDrawer}>
| deploymentId: string; | ||
| environmentId: string; | ||
| }> = ({ deploymentId, environmentId }) => { | ||
| const versionCounts = useChartData(deploymentId, environmentId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider error handling for chart data
The useChartData hook is used without error handling. If the hook can return undefined data or fail, the component should handle these cases gracefully.
🏁 Script executed:
#!/bin/bash
# Check the useChartData hook implementation for error handling
ast-grep --pattern 'export const useChartData = $_'Length of output: 4565
Add explicit error and loading states for useChartData and handle them in RolloutPieChart
The useChartData hook currently only returns data (falling back to []), so any errors or loading state from the underlying query are swallowed. To surface failures and loading in the UI, update both the hook and the chart component:
• In useChartData.ts
export const useChartData = (deploymentId: string, environmentId: string) => {
- const { data } =
+ const { data, error, isLoading } =
api.dashboard.widget.data.deploymentVersionDistribution.useQuery(
{ deploymentId, environmentIds: [environmentId] },
{ refetchInterval: 10_000 },
);
- return data ?? [];
+ return {
+ data: data ?? [],
+ error,
+ isLoading,
+ };
};• In RolloutPieChart.tsx
- const versionCounts = useChartData(deploymentId, environmentId);
+ const { data: versionCounts, error, isLoading } =
+ useChartData(deploymentId, environmentId);
+ if (isLoading) {
+ return <Spinner size="md" />;
+ }
+ if (error) {
+ return <ErrorMessage message={error.message} />;
+ }
// …render chart using versionCountsThis ensures the component can display a loading indicator and surface query errors instead of silently rendering an empty chart.
🤖 Prompt for AI Agents
In
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/(sidebar)/jobs/_components/rule-drawers/environment-version-rollout/rollout-charts/RolloutPieChart.tsx
at line 14, the useChartData hook only returns data without exposing loading or
error states, causing the component to silently render empty charts on failure
or while loading. Update useChartData.ts to return loading and error states
along with data, then modify RolloutPieChart.tsx to handle these states by
displaying a loading indicator when loading and an error message when an error
occurs, ensuring proper UI feedback.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores