fix: format bytes columns and avoid refetch on column toggle #3149
Conversation
DaryaVorontsova
requested review from
Raubzeug,
adameat,
artemmufazalov and
astandrik
as code owners
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
src/utils/tableUtils/getRequiredDataFields.ts, line 7 (link)logic: potential runtime error if column doesn't exist in columnsToFields
columnFieldswill be undefined if the column key doesn't exist, causingforEachto throw. Add null check:
23 files reviewed, 2 comments
src/utils/utils.ts
Outdated
| } | ||
|
|
||
| export function bytesToMB(bytes?: number | string, fractionDigits?: number) { | ||
| function formatMbWithMaxFractionDigits(value: number, fractionDigits?: number): string { |
There was a problem hiding this comment.
Does not formatBytes from src/utils/bytesParsers/formatBytes.ts suit you? It has precision and it properly formats number inside
|
|
||
| //sort ids to prevent refetch if only order was changed | ||
| const columnsIds = columns.map((column) => column.name).toSorted(); | ||
| const columnsIds = React.useMemo( |
There was a problem hiding this comment.
may be if !useColumnsIdsInRequest, columnsIds will be []? To avoid nullish coalescing in getNodes фтв getGroups
There was a problem hiding this comment.
Pull request overview
This PR refactors byte formatting in the Node Network table and optimizes the PaginatedTable component to prevent unnecessary data refetches when toggling column visibility.
- Consolidates the
UNBREAKABLE_GAPconstant to a single location inconstants.ts - Replaces direct
bytesToMBcalls with the more robustformatBytesutility for consistent byte value formatting - Adds
useColumnsIdsInRequestflag to PaginatedTable to control whether column IDs trigger refetches (defaults totruefor backward compatibility, set tofalsefor Node Network table)
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/utils.ts | Removes UNBREAKABLE_GAP export (moved to constants.ts) |
| src/utils/constants.ts | Adds UNBREAKABLE_GAP constant (renamed from NON_BREAKING_SPACE) |
| src/utils/numeral.ts | Updates import to use UNBREAKABLE_GAP from constants |
| src/utils/dataFormatters/formatNumber.ts | Updates import to use UNBREAKABLE_GAP from constants |
| src/utils/dataFormatters/dataFormatters.ts | Updates import to use UNBREAKABLE_GAP from constants |
| src/utils/bytesParsers/formatBytes.ts | Updates import to use UNBREAKABLE_GAP from constants |
| src/utils/dataFormatters/test/*.test.ts | Updates test imports to use UNBREAKABLE_GAP from constants |
| src/utils/bytesParsers/test/formatBytes.test.ts | Updates test import to use UNBREAKABLE_GAP from constants |
| src/components/nodesColumns/test/utils.test.ts | Updates test import to use UNBREAKABLE_GAP from constants |
| src/containers/Tenant/Diagnostics/TenantOverview/MetricsTabs/components/PlaceholderTab.tsx | Updates to use UNBREAKABLE_GAP instead of NON_BREAKING_SPACE |
| src/containers/Node/NodeNetwork/columns.ts | Replaces bytesToMB with formatBytes utility for byte columns |
| src/containers/Node/NodeNetwork/NodeNetworkTable.tsx | Disables column-based refetching with useColumnsIdsInRequest={false} |
| src/components/PaginatedTable/PaginatedTable.tsx | Adds useColumnsIdsInRequest prop with default value true |
| src/components/PaginatedTable/TableChunksRenderer.tsx | Threads useColumnsIdsInRequest through to child components |
| src/components/PaginatedTable/TableChunk.tsx | Conditionally includes columnsIds in request based on useColumnsIdsInRequest flag |
| onDataFetched: (data?: PaginatedTableData<T>) => void; | ||
|
|
||
| keepCache?: boolean; | ||
| useColumnsIdsInRequest?: boolean; |
There was a problem hiding this comment.
I suggest to name this prop something like "preserveColsOrderInRequest"
useColumnsIdsInRequest is
looks like hook name
doesnt explicitly state how they are actually used
|
btw is it intentional that in NODE_PEERS column order changing triggers refetch? If its not - we could completely erase these checks and always preserve columns order for fetching |
|
(actually I meant something like raising columns order logics somewhere to actual fetch functions - for fetch logics (which is column ordering condition) to be closely wired to actual fetches, but current approach is ok too) |
|
Actually I had to avoid refetching every time when we add or remove a column in the table of peers, because backend for /peers doesn't support passing "required columns" (unlike other handlers like /nodes, etc.). And my idea is: if we have the peers table, we keep the columnsIds array unchanged (it is always empty) -> changing visible columns doesn’t trigger a new request to the backend ))) |
I mean what will change if we make [PAGINATED_TABLE_IDS.NODE_PEERS]: true |
|
In this case changing visible columns will trigger a request to the backend for the peers table with array of columnsIds - so, it is unnecessary, I guess |
|
Okay now I see, thanks enums for table names is great idea though - much better than strings |
Stand: https://nda.ya.ru/t/VsEVE-UU7Nw5yn
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 62.33 MB | Main: 62.33 MB
Diff: +2.88 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information