-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add no logs and metrics banner when cluster is offline #317
Conversation
Reviewer's Guide by SourceryThis pull request adds functionality to display a "No Logs and Metrics" banner when a cluster is offline or when dealing with template applications. The changes primarily affect the logs and metrics routes for both regular applications and managed services. Sequence diagram for displaying No Logs and Metrics bannersequenceDiagram
participant User
participant LogsAndMetrics
participant NoLogsAndMetricsBanner
User->>LogsAndMetrics: Access logs and metrics page
alt Cluster is offline or template app
LogsAndMetrics->>NoLogsAndMetricsBanner: Render banner
NoLogsAndMetricsBanner-->>User: Display "No Logs and Metrics" message
else Cluster is online
LogsAndMetrics-->>User: Display logs and metrics
end
Class diagram for NoLogsAndMetricsBanner componentclassDiagram
class NoLogsAndMetricsBanner {
+NoLogsAndMetricsBanner(title: string, description: string)
}
NoLogsAndMetricsBanner --> motion: uses
NoLogsAndMetricsBanner --> Pulsable: uses
NoLogsAndMetricsBanner --> EmptyState: uses
NoLogsAndMetricsBanner --> SmileySad: uses
NoLogsAndMetricsBanner --> Wrapper: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nxtcoder19 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the common logic for checking cluster status and rendering the NoLogsAndMetricsBanner into a shared hook or utility function to reduce duplication between route files.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -0,0 +1,38 @@ | |||
import { motion } from 'framer-motion'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Evaluate necessity of Framer Motion
Consider the performance implications of using Framer Motion for this simple animation. React's built-in transitions might be sufficient and more lightweight for this use case.
import { generatePlainColor } from '~/root/lib/utils/color-generator'; | ||
import { IAppContext } from '../_layout'; | ||
|
||
const LogsAndMetrics = () => { | ||
const { app, environment, account } = useOutletContext<IAppContext>(); | ||
|
||
const { clustersMap: clusterStatus } = useClusterStatusV3({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting cluster status checks and conditional rendering into a separate component.
The added cluster status checks and conditional rendering have increased the complexity of the LogsAndMetrics component. To improve readability and maintainability, consider extracting this logic into a separate component or custom hook. Here's a suggested refactor:
// Create a new component
const LogsAndMetricsContent = ({ environment, isClusterOnline, children }) => {
if (environment.clusterName === '') {
return (
<NoLogsAndMetricsBanner
title="Logs and Metrics Unavailable for Template Applications"
description="Logs and metrics will become available once your app is associated with a cluster."
/>
);
}
if (isClusterOnline === false) {
return (
<NoLogsAndMetricsBanner
title="Logs and Metrics Unavailable for Offline Cluster-Based Applications"
description="Logs and metrics will be accessible once the cluster is back online."
/>
);
}
return children;
};
// Modify the main component
const LogsAndMetrics = () => {
const { app, environment, account } = useOutletContext<IAppContext>();
const { clustersMap: clusterStatus } = useClusterStatusV3({
clusterName: environment.clusterName,
});
const isClusterOnline = findClusterStatusv3(
clusterStatus[environment.clusterName]
);
// ... rest of your component logic ...
return (
<LogsAndMetricsContent environment={environment} isClusterOnline={isClusterOnline}>
<div className="flex flex-col gap-6xl pt-6xl">
{/* ... your existing JSX ... */}
</div>
</LogsAndMetricsContent>
);
};
This refactoring maintains all the new functionality while reducing nesting in the main component. It separates the concerns of checking cluster status and rendering the appropriate content, making the code more modular and easier to understand at a glance.
Add no logs and metrics banner when cluster is offline
Add no logs and metrics banner when cluster is offline
Add no logs and metrics banner when cluster is offline
Summary by Sourcery
Add a feature to display a banner indicating the unavailability of logs and metrics when the application is not associated with a cluster or when the cluster is offline. Enhance the system by checking the cluster's status to conditionally render logs and metrics.
New Features:
Enhancements: