🐋Add cluster upgrade notifications in server status and bell popover#1012
🐋Add cluster upgrade notifications in server status and bell popover#1012DavidReque wants to merge 1 commit intokubetail-org:mainfrom
Conversation
c15badc to
20dfd07
Compare
There was a problem hiding this comment.
When I run this branch in development I'm seeing 500 errors from the graphql endpoint and in the UI it says "Checking..." for both live and dead clusters:
Also, instead of adding a new row to the health check UI I think it would be better to set the dot color to blue and set the status string to "Update available".
Additionally, I think we should get rid of the Install button. It's buggy to begin with and users might not even have the permissions to install things in the cluster so it might not work. For now I think it's ok just to let users know that an update is available and let them manage the upgrade themselves.
20dfd07 to
3ecc0e3
Compare
3ecc0e3 to
4c30b1d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c30b1d467
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
amorey
left a comment
There was a problem hiding this comment.
Sorry for the delay. I added some comments inline. In addition, it looks like there's a lot of duplicated code in update-notifications.tsx. I wonder if you can simplify the code by using inheritance and sharing UpdateNotificationProvider and the useUpdateNotification() hook for both CLI and cluster updates.
|
Just a heads up - I did a rebase so you may want to checkout a fresh version of your branch. |
4c30b1d to
4ec2124
Compare
amorey
left a comment
There was a problem hiding this comment.
There are some merge conflicts with main that would probably be easier for me to handle. Let's address the home page issue I mentioned then collapse this into one commit. Then I can do a rebase on top of that.
There was a problem hiding this comment.
Why do we need to add <UpdateNotificationProvider> to the home page test?
There was a problem hiding this comment.
We don't need <UpdateNotificationProvider> in the home test anymore. useUpdateNotification(kubeContext) now returns an inactive default state when used outside the provider instead of throwing, so unrelated tests work without extra setup.
Also rebased on main and collapsed everything into a single commit. The merge conflict in update-notifications.tsx was just the now state variable you added — incorporated it into our refactored code.
2ef2e70 to
1aeaa2e
Compare
1aeaa2e to
726db0d
Compare
There was a problem hiding this comment.
I added some comments inline but I think the bigger thing to focus on is the context/hook issue (Jotai vs. React Context). We should make a decision about that first because it will affect the overall architecture and may make my other suggestions moot.
| export function useUpdateNotification(): UpdateNotificationState { | ||
| return useContext(UpdateNotificationContext); | ||
| export function useUpdateNotification(): UpdateNotificationState; | ||
| export function useUpdateNotification(kubeContext: string): ClusterUpdateNotificationState; |
There was a problem hiding this comment.
Currently, any updates to the cluster registry will trigger a re-render of all useUpdateNotication() subscribers so even if a caller only subscribes to one kubeContext they are still effectively listening for changes in all the others.
To deal with this I think we have two options:
- Create a hook that returns all cluster updates (e.g.
useClusterUpdateNotications()) - Use Jotai to make targeted listeners
I'm not necessarily in favor of one over the other. Jotai will add complexity in some ways but also make other parts of the code simpler (e.g. registry updates). Keeeping things as-is will be quicker to implement but this is basically the point where you start running into the limits of React Context.
What do you think?
There was a problem hiding this comment.
I agree this is where React Context starts to show its limits.
I split the CLI and cluster hooks so CLI-only consumers no longer subscribe to cluster updates, but you're right that cluster consumers are still all keyed off the same registry and will re-render when any kubeContext changes.
Between the two options, I think useClusterUpdateNotifications() would be the smaller follow-up and keeps the implementation simpler, but it still leaves the caller responsible for selecting from a shared snapshot. If we want truly targeted subscriptions, Jotai is probably the better long-term fit since it maps more naturally to per-kubeContext state and would also simplify the registry/update flow.
For this PR I'd prefer to keep the current approach minimal and avoid a larger architectural shift mid-review, then follow up with a focused PR to decide between:
- a
useClusterUpdateNotifications()API as the lighter option, or - moving cluster update state to Jotai if we want per-context subscriptions and cleaner state updates.
|
Sorry, I jumped the gun on a rebase. It didn't affect the code in this PR but you'll need to pull the changes regardless. |
- Centralize cluster version queries in UpdateNotificationProvider - Overload useUpdateNotification() for CLI and cluster - Rename Upgrade -> Update throughout - Unify localStorage helpers with shared generics - Use Status.UpdateAvailable in ServerStatus widget Signed-off-by: David Requeno <davidrequeno52@gmail.com>
726db0d to
c3f09a2
Compare
I think the current PR is in a better place now, but I agree the broader architecture question is still open. What I addressed here was the immediate React Context issues:
That said, the deeper concern you raised still stands: cluster consumers are still backed by a shared registry, so this does not fully solve the targeted subscription problem. In that sense, I’d say this PR improves the current React Context approach, but it does not fully settle the Context vs. Jotai decision. My preference for this PR would be to keep the current implementation minimal and shippable, then do a focused follow-up to decide whether we want:
What do you think? |
Fixes #1011
Summary
Adds cluster (Helm chart) upgrade hints in desktop mode: queries
CLUSTER_VERSION_STATUSper kube context and surfaces it in the health dialog and notifications bell.Changes
useClusterUpgradeNotificationhook with per-context cache/dismiss (12h TTL, same idea as CLI update notifications).NotificationsPopoverwhen a cluster or CLI update is available.upgrade-notifications.test.tsxSubmitter checklist
Footnotes
See suggested commit format ↩