⚡ Improved cluster-status #310
Conversation
Reviewer's Guide by SourceryThis pull request improves the cluster status fetching mechanism by introducing a new hook Sequence diagram for Cluster Status FetchingsequenceDiagram
actor User
participant UI
participant useClusterStatusV3
participant API
User->>UI: View Environment Resources
UI->>useClusterStatusV3: Fetch Cluster Status
useClusterStatusV3->>API: getClusterStatus(name)
API-->>useClusterStatusV3: Return lastOnlineAt
useClusterStatusV3-->>UI: Update Cluster Status
UI-->>User: Display Updated Status
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @abdheshnayak - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues 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.
| }; | ||
| }); | ||
| } catch (e) { | ||
| console.log('error', e); |
There was a problem hiding this comment.
suggestion: Improve error handling in useClusterStatusV3 hook
The current error handling simply logs to the console. Consider implementing more robust error handling, such as updating the UI to reflect the error state or retrying the request.
setError(e);
if (retryCount < 3) {
setTimeout(() => fetchData(retryCount + 1), 1000 * (retryCount + 1));
} else {
notifyError('Failed to fetch cluster status', e);
}
| [key: string]: number; | ||
| }>({}); | ||
|
|
||
| const addToWatchList = (clusterNames: string[]) => { |
There was a problem hiding this comment.
suggestion: Simplify watchlist management logic
The addToWatchList and removeFromWatchList functions are quite complex. Consider simplifying this logic, possibly by using a Set instead of a counter-based system. This would make the code more readable and potentially more efficient.
const watchList = new Set<string>();
const addToWatchList = (clusterNames: string[]) => {
clusterNames.forEach(name => watchList.add(name));
setWatchList(new Set(watchList));
};
| } | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
suggestion: Extract polling logic into a custom hook
Consider extracting the polling logic in the effect hook into a custom hook. This would improve reusability and separation of concerns. For example, you could create a usePolling hook that encapsulates the setTimeout and setInterval logic.
const usePolling = (callback, delay) => {
useEffect(() => {
const timer = setInterval(callback, delay);
return () => clearInterval(timer);
}, [callback, delay]);
};
usePolling(() => {
caller(watchList);
}, 5000);
| @@ -201,18 +203,18 @@ const GridView = ({ items = [], onAction }: IResource) => { | |||
|
|
|||
| const ListView = ({ items, onAction }: IResource) => { | |||
| const { account } = useParams(); | |||
| const { clusters } = useClusterStatusV2(); | |||
| // const { clusters } = useClusterStatusV2(); | |||
| const { clusters: clusterStatus } = useClusterStatusV3({ | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the cluster status rendering logic into a separate function.
The changes introduce unnecessary complexity in the cluster status rendering logic. While the new useClusterStatusV3 hook might offer benefits, the implementation can be simplified. Here's a suggestion to refactor the code:
const ListView = ({ items, onAction }: IResource) => {
const { account } = useParams();
const { clusters: clusterStatus } = useClusterStatusV3({
clusterNames: items.map((i) => i.clusterName),
});
const renderClusterStatus = (item) => {
if (item.clusterName === '') {
return <ListItemV2 className="px-4xl" data="-" />;
}
if (item.isArchived) {
return <Badge type="neutral">Archived</Badge>;
}
if (item.spec?.suspend) {
return <Badge type="neutral">Suspended</Badge>;
}
const status = clusterStatus[item.clusterName];
if (status === undefined) {
return <LoadingSpinner />;
}
if (!findClusterStatusv3(status)) {
return <Badge type="warning">Cluster Offline</Badge>;
}
return <SyncStatusV2 item={item} />;
};
// ... rest of the component
return (
<ListV2.Root
// ... other props
data={{
// ... other data
rows: items.map((i) => {
const { name, id, updateInfo } = parseItem(i);
const isClusterOnline = findClusterStatusv3(clusterStatus[i.clusterName]);
return {
columns: {
// ... other columns
status: {
render: () => renderClusterStatus(i),
},
// ... other columns
},
// ... other properties
};
}),
}}
/>
);
};
const LoadingSpinner = () => (
<div className="cursor-pointer w-fit">
<span className="animate-spin relative flex items-center justify-center text-text-warning">
<CircleNotch size={12} />
<span className="absolute">
<CircleFill size={8} />
</span>
</span>
</div>
);This refactoring:
- Extracts the cluster status rendering logic into a separate function
renderClusterStatus, improving readability. - Simplifies the conditional logic by using early returns.
- Moves the loading spinner into a separate component for reusability.
- Maintains all the new functionality while reducing nesting and complexity.
These changes make the code more maintainable and easier to understand while preserving the improved handling of cluster statuses.
| removeFromWatchList: () => {}, | ||
| }); | ||
|
|
||
| const ClusterStatusProvider = ({ children }: ChildrenProps) => { |
There was a problem hiding this comment.
issue (complexity): Consider simplifying complex code sections to improve readability and maintainability.
While the complexity in this file may be justified for performance reasons, there are several areas where we can simplify the code without losing functionality:
-
Simplify watchlist management:
TheaddToWatchListandremoveFromWatchListfunctions can be simplified using the spread operator andObject.fromEntries(). For example:const addToWatchList = (clusterNames: string[]) => { setWatchList(prevList => ({ ...prevList, ...Object.fromEntries(clusterNames.filter(Boolean).map(name => [name, (prevList[name] || 0) + 1])) })); }; const removeFromWatchList = (clusterNames: string[]) => { setWatchList(prevList => { const newList = { ...prevList }; clusterNames.filter(Boolean).forEach(name => { if (newList[name]) { newList[name]--; if (newList[name] === 0) delete newList[name]; } }); return newList; }); };
-
Refactor the
callerfunction:
Replace the for loop withPromise.all()to improve readability and potentially performance:const caller = async (wl: { [key: string]: number }) => { const keys = Object.keys(wl); await Promise.all(keys.map(async (w) => { try { const { data: cluster } = await api.getClusterStatus({ name: w }); setClusters(s => ({ ...s, [w]: cluster.lastOnlineAt })); } catch (e) { console.error('Error fetching cluster status:', e); } })); };
-
Reconsider timer logic:
The current implementation uses bothsetTimeoutandsetInterval. Consider using a singlesetIntervalwith error handling:useEffect(() => { const fetchData = async () => { await caller(watchList); }; fetchData(); // Initial call const intervalId = setInterval(fetchData, 30 * 1000); return () => clearInterval(intervalId); }, [watchList]);
These changes will reduce complexity while maintaining the core functionality. Consider adding comments to explain the rationale behind the remaining complex parts, especially around the watchlist concept and the use of React context for state management.
| const resp = clusterNames.reduce((acc, curr) => { | ||
| if (!curr) { | ||
| return acc; | ||
| } | ||
| if (acc[curr]) { | ||
| acc[curr] += acc[curr]; | ||
| } else { | ||
| acc[curr] = 1; | ||
| } | ||
|
|
||
| return acc; | ||
| }, s); | ||
|
|
||
| return resp; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const resp = clusterNames.reduce((acc, curr) => { | |
| if (!curr) { | |
| return acc; | |
| } | |
| if (acc[curr]) { | |
| acc[curr] += acc[curr]; | |
| } else { | |
| acc[curr] = 1; | |
| } | |
| return acc; | |
| }, s); | |
| return resp; | |
| return clusterNames.reduce((acc, curr) => { | |
| if (!curr) { | |
| return acc; | |
| } | |
| if (acc[curr]) { | |
| acc[curr] += acc[curr]; | |
| } else { | |
| acc[curr] = 1; | |
| } | |
| return acc; | |
| }, s); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const resp = clusterNames.reduce((acc, curr) => { | ||
| if (!curr) { | ||
| return acc; | ||
| } | ||
|
|
||
| if (acc[curr] && acc[curr] >= 1) { | ||
| acc[curr] -= acc[curr]; | ||
| } | ||
|
|
||
| if (acc[curr] === 0) { | ||
| delete acc[curr]; | ||
| } | ||
|
|
||
| return acc; | ||
| }, s); | ||
|
|
||
| return resp; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const resp = clusterNames.reduce((acc, curr) => { | |
| if (!curr) { | |
| return acc; | |
| } | |
| if (acc[curr] && acc[curr] >= 1) { | |
| acc[curr] -= acc[curr]; | |
| } | |
| if (acc[curr] === 0) { | |
| delete acc[curr]; | |
| } | |
| return acc; | |
| }, s); | |
| return resp; | |
| return clusterNames.reduce((acc, curr) => { | |
| if (!curr) { | |
| return acc; | |
| } | |
| if (acc[curr] && acc[curr] >= 1) { | |
| acc[curr] -= acc[curr]; | |
| } | |
| if (acc[curr] === 0) { | |
| delete acc[curr]; | |
| } | |
| return acc; | |
| }, s); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
⚡ Improved cluster-status
Summary by Sourcery
Improve cluster status fetching by introducing a new hook
useClusterStatusV3and aClusterStatusProviderfor better performance and real-time updates. This replaces the previous version and enhances the overall efficiency of cluster status management.New Features:
useClusterStatusV3to manage cluster status with improved performance and real-time updates.Enhancements:
useClusterStatusV3.ClusterStatusProviderto wrap components and provide cluster status context.