-
Notifications
You must be signed in to change notification settings - Fork 577
feat(ui): add multi-cluster user logout #4425
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
base: main
Are you sure you want to change the base?
feat(ui): add multi-cluster user logout #4425
Conversation
|
Welcome @alokdangre! |
4fa5f4b to
a9ece5c
Compare
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.
Thanks for this!
Can you please check the git commit guidelines in the development documentation and update your commit message? You can compare your message to other ones.
The frontend tests are failing in the GitHub check. Can you please try running the tests locally? Also, can you please add a storybook state for this?
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.
Pull request overview
Adds multi-cluster-aware user identity display and logout behavior in the TopBar, including a hook fix to keep multi-cluster selection derived from the URL in sync.
Changes:
- Updated
TopBarto fetch/display per-cluster authenticated user info and provide per-cluster (and “logout all”) actions. - Added
SelfSubjectReview-based user info fetching (getClusterUserInfo) in the v1 cluster API layer. - Fixed
useSelectedClustersto react to pathname changes (not only cluster changes) for correct multi-cluster state updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/lib/k8s/index.ts | Updates useSelectedClusters to recompute when location.pathname changes. |
| frontend/src/lib/k8s/api/v1/clusterApi.ts | Adds ClusterUserInfo + getClusterUserInfo() using the SelfSubjectReview API with fallback behavior. |
| frontend/src/components/App/TopBar.tsx | Implements multi-cluster user menu UX, per-cluster logout, and queries user info for selected clusters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a9ece5c to
a61d03c
Compare
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a61d03c to
0c38f12
Compare
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@illume sorry for that test error, i forgot to take snapshot, i have also added the requested changes |
|
@illume ptal |
142c9cd to
160e43d
Compare
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.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
frontend/src/components/App/TopBar.stories.tsx:138
- The new SelfSubjectReview call in
PureTopBartriggers a POST to/apis/authentication.k8s.io/v1/selfsubjectreviewswheneverclusteris set. TheTwoClusterstory setscluster: 'ak8s-desktop'but doesn’t register an MSW handler, which will be reported as an unhandled request by the Storybook test harness (seefrontend/src/storybook.test.tsxunhandled request assertion). Add an MSW handler for this story (or define a default handler at the story/file level).
export const TwoCluster = PureTemplate.bind({});
TwoCluster.args = {
appBarActions: [],
logout: () => {},
cluster: 'ak8s-desktop',
clusters: { 'ak8s-desktop': '', 'ak8s-desktop2': '' },
};
frontend/src/setupTests.ts:71
- This file mocks
window.localStoragewhen it’s missing/non-functional, butbeforeEachcallslocalStorage.clear()(the global identifier). In environments wherelocalStorageisn’t attached to the global scope (even ifwindow.localStorageis defined), this will still throw. To make the setup robust, either setglobalThis.localStorage = window.localStoragewhen mocking, or update thebeforeEachto callwindow.localStorage.clear()(guarded byglobalThis.window).
if (!window.localStorage || typeof window.localStorage.getItem !== 'function') {
Object.defineProperty(window, 'localStorage', {
value: {
getItem: vi.fn(() => null),
setItem: vi.fn(),
removeItem: vi.fn(),
clear: vi.fn(),
length: 0,
key: vi.fn(),
},
writable: true,
});
}
}
beforeEach(() => {
// Clears the database and adds some testing data.
// Jest will wait for this promise to resolve before running tests.
localStorage.clear();
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
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.
🎉 thanks!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alokdangre, illume The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
joaquimrocha
left a comment
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.
I think this commit needs to be separated in several, even if within the same PR.
@illume was this what you had in mind in the issue that this PR relates too (it's not clear from the title and there's no body)?
I personally wonder why someone who want to log out from a single cluster that they are viewing as part of a group. I understand "Log out from all clusters" and I like that we have this option (we should have a confirmation by the user though), but not sure that having multiple "Log out from XYZ" is a good idea in terms of UX. What would be the use case?
I think a more expected approach is that users who want to log out from a certain cluster would use the context menu in the home table or click the cluster, then log out while viewing it.
@alokdangre yes, breaking this change up into small changes would make it easier to review/test and understand. Are you able to break it up into separate commits? Maybe using your list of changes as a rough guide? (or maybe you would have a better idea on how to break it up)
There is precedent for wanting to logout with only one account in a few apps when you can sign in multiple times. For example, inside MS products that allow multiple logins it's possible to select which login you want to sign-out with. I use this functionality personally in a few different websites. But, maybe we need some more feedback on this part. |
160e43d to
ade1303
Compare
886c0cd to
1c189b6
Compare
debdc58 to
c1f1725
Compare
Implement getClusterUserInfo in clusterApi to fetch user details using SelfSubjectReview. Export the function in apiProxy and add unit tests. Signed-off-by: alokdangre <[email protected]>
…anges Update useSelectedClusters to use useLocation and track pathname changes, fixing a bug where cluster selection state could become stale. Signed-off-by: alokdangre <[email protected]>
Fetch and display authenticated user information per cluster in the user menu. Fallback to showing the cluster name if no user info is available. Update TopBar stories to reflect multi-user scenarios. Update e2e tests to match the new menu item structure. Signed-off-by: alokdangre <[email protected]>
|
Hii @alokdangre the CI is failing |
c1f1725 to
a806ab8
Compare
fixed , snapshot added |
Add logic to handle logout from specific clusters in a multi-cluster context. Update the URL to remove the logged-out cluster while persisting the view for remaining clusters. Signed-off-by: alokdangre <[email protected]>
|
this fail is unusuall, becuase earlier the this tests where not failing but now it fails and locally all tests passes, im figuring it out |
|
@illume ,
Sure, but this I would say is the equivalent of logging out of a cluster from the home view, which is showing all clusters there. I would err on not providing this option and instead just the "Log out of these grouped clusters" if we need a log out from all while visualizing a group of clusters. |
Summary
Related Issue
Fixes #4082
Changes
Steps to Test
Screenshots (if applicable)
Screen.Recording.2026-01-23.145438.mp4