Skip to content

Conversation

@Raubzeug
Copy link
Contributor

@Raubzeug Raubzeug commented May 30, 2025

closes #2308
Stand

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
318 317 0 1 0

😟 No changes in tests. 😕

Bundle Size: 🔺

Current: 83.76 MB | Main: 83.65 MB
Diff: +0.10 MB (0.12%)

⚠️ 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.

@Raubzeug Raubzeug force-pushed the healthcheck branch 2 times, most recently from bae0560 to 9136f5f Compare May 31, 2025 13:58
@astandrik astandrik requested a review from Copilot June 2, 2025 14:14
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 redesigns the Healthcheck feature by replacing the old DiagnosticCard-based preview with a new Alert-based component and cleans up related styles and imports. It also introduces an xs size for EmptyState, fixes import paths for EmptyFilter, adds a showVeil option to Drawer, wraps the fullscreen button in a tooltip, and adds a new HealthcheckStatus component.

Reviewed Changes

Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/IssuesViewer/IssueTree.scss Removed legacy issue-tree styles
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.tsx Refactored preview component to use Alert and new layout logic
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.scss Added styles for the redesigned HealthcheckPreview
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckDetails.tsx Deleted old details view component
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/Healthcheck.scss Removed legacy Healthcheck styles
src/containers/Storage/PaginatedStorageNodesTable/StorageNodesEmptyDataMessage.tsx Corrected EmptyFilter import path
src/containers/Storage/PaginatedStorageGroupsTable/StorageGroupsEmptyDataMessage.tsx Corrected EmptyFilter import path
src/components/SplitPane/SplitPane.scss Removed gutter z-index override
src/components/HealthcheckStatus/i18n/index.ts & en.json & HealthcheckStatus.tsx Added new HealthcheckStatus component and translations
src/components/EnableFullscreenButton/EnableFullscreenButton.tsx Wrapped fullscreen button in ActionTooltip
src/components/EmptyState/EmptyState.tsx Introduced xs size variant
src/components/EmptyState/EmptyState.scss Added styles for the new xs size
src/components/EmptyFilter/i18n/index.ts Fixed i18n import path
src/components/EmptyFilter/EmptyFilter.tsx Updated component import paths
src/components/Drawer/Drawer.tsx Added showVeil prop to toggle veil visibility
src/components/Drawer/Drawer.scss Increased drawer item z-index
Comments suppressed due to low confidence (2)

src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.tsx:23

  • [nitpick] The active prop is declared in HealthcheckPreviewProps but never used in the component. Remove it to clean up unused code.
active?: boolean;

src/components/SplitPane/SplitPane.scss:25

  • [nitpick] Removing the gutter's z-index may cause it to drop behind other elements and break drag functionality. Confirm this change or add a comment explaining the new stacking context.
z-index: 10;

@Raubzeug
Copy link
Contributor Author

Raubzeug commented Jun 3, 2025

@astandrik @artemmufazalov design review passed, stand redeployed!


&__item {
z-index: 4;
z-index: 11;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could make some global constants?
It unobvious what 11 here and not 12 or 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed there's no need in this change. I removed it.

detectClickOutside?: boolean;
defaultWidth?: number;
isPercentageWidth?: boolean;
showVeil?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe hideVeil with hideVeil=false is better
why? To represent the original components' prop that is "hideVeil"

width: 321px;
height: 100px;
#{$block}__image {
margin-right: 20px;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--g-spacing-5

<Button onClick={onEnableFullscreen} view={view} disabled={disabled} title="Fullscreen">
<Icon data={SquareDashed} />
</Button>
<ActionTooltip title="Fullscreen">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i18n

[SelfCheckResult.DEGRADED]: 'info',
[SelfCheckResult.MAINTENANCE_REQUIRED]: 'warning',
[SelfCheckResult.EMERGENCY]: 'danger',
[SelfCheckResult.UNSPECIFIED]: 'normal',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's map SelfCheckResultToLabelTheme, no need to translate here

"status_message.ok": "No issues",
"no-data": "no healthcheck data"
"description_problems": [
"There is {{count}} issues.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"issues" is plural and "is" - for singular

getMonitoringLink?: GetMonitoringLink;
getMonitoringClusterLink?: GetMonitoringClusterLink;

getHealthckechViewTitles: GetHealthcheckViewTitles<CommonIssueType>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it would be better to make one field for all healthcheck functions? I doubt that there are two app with same getHealthckechViewTitles but different getHealthcheckViewsOrder

() =>
view
? filteredIssues.filter((issue) => {
const type = issue.upperType || issue.type;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats upperType? I would suppose that it's issue in upperCase but looking at getLeaves function more likely its parentType

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the structure is: Root => firstParent => ... => nthParent => child.
UpperType is the type of the firstParent... I will reword.

export function Issues({issues}: IssuesProps) {
const {view, issuesFilter} = useTenantQueryParams();

const filteredIssues = React.useMemo(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe could try to hide them with css has-text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately not possible for now

return null;
}

return pdisk.map((el: {id: string; path: string}) => (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe something instead of el would be great =)

@astandrik
Copy link
Collaborator

maybe when searching - we could open found issues?

7) warningThreshold - the percentage of fullness at which the color of the progress bar changes to yellow
8) dangerThreshold - the percentage of fullness at which the color of the progress bar changes to red
9) overflow - parcents may be more that 100%
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pErcents

position: sticky;
z-index: 1;
top: 0px;
left: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

top, left lets both be 0px or 0

@Raubzeug Raubzeug requested a review from astandrik June 4, 2025 14:55
@Raubzeug Raubzeug added this pull request to the merge queue Jun 5, 2025
Merged via the queue into main with commit eac76af Jun 5, 2025
7 checks passed
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.

Rework health check according to design add support for network data on database page

3 participants