Skip to content

Conversation

@artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Apr 2, 2025

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

Total Passed Failed Flaky Skipped
298 298 0 0 0

😟 No changes in tests. 😕

Bundle Size: 🔺

Current: 83.27 MB | Main: 83.25 MB
Diff: +0.02 MB (0.02%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.


const uiFactoryBase: UIFactory = {};

export function configureUIFactory(overrides: UIFactory) {
Copy link
Member Author

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

https://github.com/ytsaurus/ytsaurus-ui/blob/main/packages/ui/src/ui/UIFactory/index.tsx#L476

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

@artemmufazalov artemmufazalov marked this pull request as ready for review April 3, 2025 06:28
@astandrik astandrik requested a review from Copilot April 3, 2025 11:57
Copy link
Contributor

Copilot AI left a 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')}>

position: relative;
z-index: 2;

width: max-content;
Copy link
Collaborator

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

Copy link
Member Author

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

const capabilitiesLoaded = useCapabilitiesLoaded();

useMetaCapabilitiesQuery();
const metaCapabilitiesLoaded = useMetaCapabilitiesLoaded();
Copy link
Collaborator

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);
Copy link
Collaborator

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

Copy link
Member Author

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?

Copy link
Collaborator

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: {}

Copy link
Member Author

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

Copy link
Collaborator

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

Copy link
Member Author

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

@artemmufazalov artemmufazalov requested a review from astandrik April 3, 2025 12:36
astandrik
astandrik previously approved these changes Apr 3, 2025
@artemmufazalov artemmufazalov mentioned this pull request Apr 3, 2025
5 tasks
@artemmufazalov artemmufazalov added this pull request to the merge queue Apr 3, 2025
Merged via the queue into main with commit 810f60b Apr 3, 2025
7 checks passed
@artemmufazalov artemmufazalov deleted the allow-create-db branch April 3, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add delete DB button to Databases Table Delete database in UI Create database in UI

3 participants