-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: allow pass create and delete DB funcs to UI #2087
Conversation
|
||
const uiFactoryBase: UIFactory = {}; | ||
|
||
export function configureUIFactory(overrides: UIFactory) { |
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.
Approach inspired by ytsaurus-ui
I think it is easier and more straightforward to register functions here than to pass it from props.
Function for monitoring and logs links could be added here too
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
This PR adds support for passing database creation and deletion functions to the UI package by exposing new hooks, API endpoints, and UI components.
- Introduces new hooks for retrieving cluster names and meta capabilities.
- Exposes create and delete database functions via the uiFactory and integrates them into the Tenants component.
- Updates API types and endpoints to support meta capabilities querying.
Reviewed Changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/utils/hooks/useDatabaseFromQuery.ts | Added new hook useClusterNameFromQuery to retrieve the clusterName from URL query parameters. |
src/utils/createToast.tsx | Modified createToast signature to allow autoHiding to be number or false and added className. |
src/uiFactory/uiFactory.ts | Provided a proxy-based uiFactory configuration to restrict direct modifications. |
src/uiFactory/types.ts | Defined UIFactory interface with onCreateDB and onDeleteDB handlers. |
src/types/api/tenant.ts | Updated Tenant type by refining UserAttributes definition. |
src/types/api/capabilities.ts | Added MetaCapabilitiesResponse and refined MetaCapability type union for new endpoints. |
src/store/reducers/capabilities/hooks.ts | Introduced hooks to fetch and check meta capabilities and feature availability. |
src/store/reducers/capabilities/capabilities.ts | Added a new query endpoint for meta capabilities with error handling and version selection. |
src/services/api/meta.ts | Implemented getMetaCapabilities API method with a timeout configuration. |
src/containers/Tenants/i18n/index.ts | Registered i18n keysets for the Tenants component. |
src/containers/Tenants/Tenants.ts | Integrated create and delete database buttons in Tenants table using uiFactory callbacks. |
src/containers/App/Content.ts | Updated content loader to handle meta capabilities loading alongside existing capabilities. |
Files not reviewed (3)
- src/components/TableWithControlsLayout/TableWithControlsLayout.scss: Language not supported
- src/containers/Tenants/Tenants.scss: Language not supported
- src/containers/Tenants/i18n/en.json: Language not supported
Comments suppressed due to low confidence (1)
src/containers/Tenants/Tenants.tsx:90
- The helper 'b' used for generating CSS class names is not defined or imported. Consider importing or defining 'b' (or using the already imported 'cn') to ensure the class names are generated as intended.
<Button view="action" onClick={() => uiFactory.onCreateDB?.({clusterName})} className={b('create-database')}>
@@ -30,6 +30,8 @@ | |||
&__table { | |||
position: relative; | |||
z-index: 2; | |||
|
|||
width: max-content; |
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 am a liitle worried of this style possible impact on other tables
could you please clarify why this style was added
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.
It doesn't needed, thanks
@@ -213,8 +215,11 @@ function GetCapabilities({children}: {children: React.ReactNode}) { | |||
useCapabilitiesQuery(); | |||
const capabilitiesLoaded = useCapabilitiesLoaded(); | |||
|
|||
useMetaCapabilitiesQuery(); | |||
const metaCapabilitiesLoaded = useMetaCapabilitiesLoaded(); |
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.
Did not dig deep but looks like if there is no meta metaCapabilitiesLoaded will always be falsy and loading will always be true?
export function useMetaCapabilitiesLoaded() { | ||
const {data, error} = useTypedSelector(selectMetaCapabilities); | ||
|
||
return Boolean(data || error); |
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.
as for me its not that obvious that Loaded is true when we receive error
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.
For capabilities queries error is a valid state and it means that there is no capability endpoint in current cluster / meta cluster. In that case all capabilities considered not available (version 0)
The alternative for this code (return data: {}
instead of error
in catch block):
getMetaCapabilities: build.query({
queryFn: async () => {
try {
if (!window.api.meta) {
throw new Error('Method is not implemented.');
}
const data = await window.api.meta.getMetaCapabilities();
return {data};
} catch {
// If capabilities endpoint is not available, there will be an error
// That means no new features are available
return {data: {}};
}
},
}),
What do you think, will it be more clear?
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.
Do we want to handle real errors from this endpoint?
I mean now HTTP 500 and non-existent state are the same data: {}
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 don't think so.
In case of error we don't know anything about cluster capabilities, so it's much safer to consider everything disabled.
If response is 500, we can only retry, but we do not retry requests currently
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.
as for me I would split loaded and availability state like
isMetaCapabilitiesAvailable=Boolean(window.api.meta.getMetaCapabilities)
loading={!isCapabilitiesLoaded || (isMetaCapabilitiesAvailable && !isMetaCapabilitiesLoaded)}
because when I see just !isMetaCapabilitiesLoaded I am expecting for it to be loaded
but thats just me and just readability issue
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 don't want to add additional code, but added some comments, I hope they will help
How it will work when ui is used as a package: https://nda.ya.ru/t/a_NVBW7U7DHZKw
Stand with multi-cluster embedded version: https://nda.ya.ru/t/AUgj5s8F7DHZHW
Connected issues: #1939, #1940, #1948
CI Results
Test Status: ✅ PASSED
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: 🔺
Current: 83.27 MB | Main: 83.25 MB
Diff: +0.02 MB (0.02%)
ℹ️ CI Information