Conversation
There was a problem hiding this comment.
Copilot reviewed 46 out of 50 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- src/components/EmptyState/EmptyState.scss: Language not supported
- src/components/MultilineTableHeader/MultilineTableHeader.scss: Language not supported
- src/components/PaginatedTable/PaginatedTable.scss: Language not supported
- src/containers/Tenant/Diagnostics/Partitions/Headers/Headers.scss: Language not supported
| <div className={b('head-cell-content')}>{content}</div> | ||
| {column.note && ( | ||
| <HelpMark | ||
| placement={columnAlignToHelpMarkPlacement[column.align]} |
There was a problem hiding this comment.
If column.align is undefined or not one of 'left', 'right', or 'center', this mapping will return undefined. Consider providing a default placement value to prevent potential runtime issues.
| placement={columnAlignToHelpMarkPlacement[column.align]} | |
| placement={columnAlignToHelpMarkPlacement[column.align] ?? 'top'} |
|
|
||
| import './MultilineTableHeader.scss'; | ||
|
|
||
| const b = cn('ydb-mulitiline-table-header'); |
There was a problem hiding this comment.
There appears to be a typo in the class name 'ydb-mulitiline-table-header'; consider renaming it to 'ydb-multiline-table-header' for clarity.
| const b = cn('ydb-mulitiline-table-header'); | |
| const b = cn('ydb-multiline-table-header'); |
There was a problem hiding this comment.
Copilot reviewed 46 out of 50 changed files in this pull request and generated no comments.
Files not reviewed (4)
- src/components/EmptyState/EmptyState.scss: Language not supported
- src/components/MultilineTableHeader/MultilineTableHeader.scss: Language not supported
- src/components/PaginatedTable/PaginatedTable.scss: Language not supported
- src/containers/Tenant/Diagnostics/Partitions/Headers/Headers.scss: Language not supported
Comments suppressed due to low confidence (1)
src/components/PaginatedTable/TableHead.tsx:138
- The 'align' property is used to determine the HelpMark placement but is not declared in the Column interface. Consider adding an optional 'align' field to the Column type with a default value to ensure consistent behavior.
placement={columnAlignToHelpMarkPlacement[column.align]}
| <FullValue value={fullValue} onClose={() => setFullValue(undefined)} /> | ||
| <ResizeablePaginatedTable | ||
| //if any filter change, we need to initiate a new table component to ensure correct start offset calculations in generateTopicDataGetter | ||
| key={topicDataFilter + selectedOffset + startTimestamp + selectedPartition} |
There was a problem hiding this comment.
Table chunks are reset every time value in filters changes, why do you need key here?
There was a problem hiding this comment.
Because of logic inside generateTopicDataGetter https://github.com/ydb-platform/ydb-embedded-ui/pull/2145/files/c38c503b6fcc6c99e35da4324056d8374ccb5c03#diff-55d62e69373cb3d825abe22e33d121b8cda18477596ea4d1038a5560934ac1f4 ! Every time we should calculate initial offset, and table offset should be reset to 0.
There was a problem hiding this comment.
But there is already
parentRef.current.scrollTo(0, 0);
in React.useLayoutEffect in PaginatedTable
shouldnt this be enough?
There was a problem hiding this comment.
I made a little improvement to PaginatedTable, so it does't refetch previous active chunks when filters changes. So the key is not needed any more.
| import {TextInput} from '@gravity-ui/uikit'; | ||
|
|
||
| import {cn} from '../../utils/cn'; | ||
| import {DebouncedInput} from '../DebouncedInput/DebouncedInput'; |
There was a problem hiding this comment.
I dont think we need search component now - we can use DebouncedInput directly where Search was used
There was a problem hiding this comment.
We definitely can, but it will require to pass similar props in a lot of places where was used. Do you think it is really needed?
| import type {TextInputProps} from '@gravity-ui/uikit'; | ||
| import {TextInput} from '@gravity-ui/uikit'; | ||
|
|
||
| interface SearchProps extends TextInputProps { |
| flex-grow: 1; | ||
|
|
||
| height: 100%; | ||
| margin-right: -20px; |
There was a problem hiding this comment.
calc(-1 * var(--g-spacing-5))
There was a problem hiding this comment.
it seems we don't need it at all
| {TopicDataFilterValues.TIMESTAMP} | ||
| </RadioButton.Option> | ||
| <RadioButton.Option value="OFFSET"> | ||
| {TopicDataFilterValues.OFFSET} |
There was a problem hiding this comment.
I suppose value in RadioButton should be TopicDataFilterValues.OFFSET and text i18(...)
according to current project rules
| {TopicDataFilterValues.OFFSET} | ||
| </RadioButton.Option> | ||
| </RadioButton> | ||
| {topicDataFilter === 'OFFSET' && ( |
|
|
||
| &__table { | ||
| @include mixins.freeze-nth-column(1); | ||
| // padding-right: 0; |
|
@astandrik @artemmufazalov stand redeployed |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new "Data" tab for topic data diagnostics and enhances related page linking and filtering logic for topic and database entity types.
- Updated diagnostics pages to include the new topicData tab
- Refactored page lookup logic to handle topic data and database feature flags
- Replaced TextInput with a DebouncedInput component for better search handling
Reviewed Changes
Copilot reviewed 45 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/containers/Tenant/Diagnostics/DiagnosticsPages.ts | Added topicData tab and updated pages filtering logic |
| src/containers/Tenant/Diagnostics/Diagnostics.tsx | Integrated topicData rendering with updated diagnostics page linking |
| src/containers/Storage/shared.ts | Removed unused paginated table error message helper |
| src/containers/Storage/PaginatedStorageNodes.tsx | Updated import usage for error message rendering |
| src/containers/Storage/PaginatedStorageGroups.tsx | Updated import usage for error message rendering |
| src/containers/Storage/EmptyFilter/EmptyFilter.tsx | Modified to allow an optional custom image for empty filters |
| src/components/Search/Search.tsx | Replaced TextInput with DebouncedInput for improved debounced search input |
| src/components/PaginatedTable/useScrollBasedChunks.ts | Adjusted active chunks calculation logic |
| src/components/PaginatedTable/types.ts | Added a note property to the Column type |
| src/components/PaginatedTable/TableHead.tsx | Added HelpMark support for displaying column notes |
| src/components/PaginatedTable/TableChunk.tsx | Updated offset calculation to include startOffset |
| src/components/PaginatedTable/PaginatedTable.tsx | Introduced startOffset prop and refined filters update and dependency |
| src/components/MultilineTableHeader/MultilineTableHeader.tsx | New component for rendering multi-line table headers |
| src/components/MetricChart/getDefaultDataFormatter.ts | Replaced numeric conversion helper with a safer version |
| src/components/DebouncedInput/DebouncedInput.tsx | New debounced input component implementation |
Files not reviewed (3)
- src/components/EmptyState/EmptyState.scss: Language not supported
- src/components/MultilineTableHeader/MultilineTableHeader.scss: Language not supported
- src/components/PaginatedTable/PaginatedTable.scss: Language not supported
ff8bc98 to
0be62cc
Compare
| sortParams?: SortParams; | ||
| isActive: boolean; | ||
| tableName: string; | ||
| startOffset: number; |
There was a problem hiding this comment.
mb its possible to set startOffset as rowIndex?
491eb03 to
8db1ae0
Compare
|
@artemmufazalov @astandrik final version is here, stand redeployed! |
57c3d42 to
92739a8
Compare
92739a8 to
e3fbd51
Compare
|
@astandrik @artemmufazalov final version is here, stand redeployed! |
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: 🔺
Current: 83.46 MB | Main: 83.38 MB
Diff: +0.08 MB (0.10%)
ℹ️ CI Information