Skip to content

fix: use EMPTY_DATA_PLACEHOLDER instead of hardcoded “no data”#3532

Merged
astandrik merged 8 commits intoydb-platform:mainfrom
Arunkoo:fix/empty-data-placeholder-tables
Mar 3, 2026
Merged

fix: use EMPTY_DATA_PLACEHOLDER instead of hardcoded “no data”#3532
astandrik merged 8 commits intoydb-platform:mainfrom
Arunkoo:fix/empty-data-placeholder-tables

Conversation

@Arunkoo
Copy link
Contributor

@Arunkoo Arunkoo commented Feb 28, 2026

Replaces hardcoded "No data" text in table empty states with the shared EMPTY_DATA_PLACEHOLDER constant.

Updated ProgressViewer and TopicPreviewTable to use the placeholder. Verified locally in dev mode.

Greptile Summary

This PR improves empty state handling across node and storage tables by replacing hardcoded "No data" text with the shared EMPTY_DATA_PLACEHOLDER constant and adding isNumeric guards before rendering ProgressViewer components.

Changes verified:

  • src/components/nodesColumns/columns.tsx: Four columns (getRAMColumn, getMemoryColumn, getCpuColumn, getLoadAverageColumn) now include defensive isNumeric checks before rendering progress bars, falling back to the placeholder when data is missing or non-numeric.
  • src/containers/Node/NodeStructure/Pdisk.tsx: The VDisk Size column adds an isNumeric guard for AllocatedSize. One minor improvement: the capacity expression should reuse the already-extracted and validated allocated variable instead of re-reading row.AllocatedSize directly.

All changes follow existing codebase patterns and are consistent with similar defensive checks in other columns (e.g., getConnectionsColumn, getNetworkUtilizationColumn).

Confidence Score: 4/5

  • Safe to merge. Changes are defensive guards that improve UX with no functional regressions.
  • All four column guards in columns.tsx are logically correct and consistent with existing codebase patterns. The Pdisk.tsx change is also correct but has a minor style suggestion to improve consistency by reusing the extracted variable instead of re-reading the property.
  • src/containers/Node/NodeStructure/Pdisk.tsx — consider the style suggestion to reuse the extracted allocated variable in the capacity expression for consistency.

Last reviewed commit: 654b9db

}

return <div className={`${b({size})} ${className} error`}>no data</div>;
return <div className={`${b({size})} ${className} error`}>{EMPTY_DATA_PLACEHOLDER}</div>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your contribution and effort on this task!

I wanted to clarify the requirements to make sure we're aligned. We don't need to change the default behavior of the component itself. Instead, the goal is to:

Review all the places where this component is currently used
Check if there's sufficient data in each case
When data is insufficient, render EMPTY_DATA_PLACEHOLDER instead of the component
So the changes should be made at the usage sites, not within the component's core logic.

I hope this clarifies things! Please let me know if you have any questions.

Thanks again for your help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for the clarification.

I’ll revert the changes made inside the component and instead update the usage sites to conditionally render EMPTY_DATA_PLACEHOLDER when data is insufficient. This keeps the component’s default behavior unchanged while aligning with the requirement.

I’ll push the updates shortly.

rowKey={getRowIndex}
wrapperClassName={b('table-wrapper')}
emptyDataMessage="No data"
emptyDataMessage={EMPTY_DATA_PLACEHOLDER}
Copy link
Contributor

Choose a reason for hiding this comment

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

The task was about table cells only, so this change is not needed.

@Arunkoo Arunkoo force-pushed the fix/empty-data-placeholder-tables branch from 69bcaec to b1f40f7 Compare March 2, 2026 16:51
@Arunkoo Arunkoo requested a review from Raubzeug March 2, 2026 17:00
Copy link
Contributor

@Raubzeug Raubzeug left a comment

Choose a reason for hiding this comment

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

Thank you for the update. I think there might still be some confusion, so let me be more specific:

The task is to find all places where the ProgressViewer component is used in tables (table cells), and add data sufficiency checks in those specific cells.

So the steps would be:

  • Search for all occurrences of ProgressViewer in table contexts
  • For each table cell that renders ProgressViewer, add a check to verify if there's sufficient data
  • If data is insufficient, render EMPTY_DATA_PLACEHOLDER instead of ProgressViewer
    The component itself should remain unchanged - we only need to modify the table cells where it's being used.

Does this make sense? Let me know if you need any clarification!

Thanks for your patience!

? JSON.stringify(data).slice(1, -1)
: String(data);
return <Cell value={normalizedData} />;
const cell = row[name];
Copy link
Contributor

Choose a reason for hiding this comment

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

For the query results table, let's skip these checks entirely. It's critically important that we don't substitute or alter any data in the query results - they should always display exactly what the query returns, even if the data appears insufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification that makes sense.

I understand now that

  • I shouldn’t modify QueryResultTable or alter query result values.
  • I only need to update table cells where ProgressViewer is used.
  • The check should be added at the usage site, not inside the component.

Just to confirm:

Should “sufficient data” mean that both value and capacity are defined?
And should 0 still be treated as valid data?

I’ll revert the QueryResultTable changes and focus only on the relevant table cells.

Thanks for your guidance — I’m still learning the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Arunkoo no problem, our codebase is tricky, and issues may not be as clear as wanted unfortunately...
Sufficient data in this case means that isNumeric(value) === true.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

src/components/nodesColumns/columns.tsx, line 352
sortAccessor returns undefined for empty LoadAveragePercents

The sortAccessor returns LoadAveragePercents[0] which is undefined when the array is empty (the default parameter = [] only applies when the field is absent; [][0] is still undefined). Now that rows with empty LoadAveragePercents render EMPTY_DATA_PLACEHOLDER instead of a bar, the sort accessor should align with this render logic to avoid passing undefined to the table's sort algorithm.

Consider returning a sentinel value like -Infinity to consistently push rows with no data to the bottom:

        sortAccessor: ({LoadAveragePercents = []}) => LoadAveragePercents.length > 0 ? LoadAveragePercents[0] : -Infinity,

@Arunkoo Arunkoo requested a review from Raubzeug March 3, 2026 09:35
const allocated = row.AllocatedSize;
const available = row.AvailableSize;

if (!isNumeric(allocated) || !isNumeric(available)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!isNumeric(available) is redundant here

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

src/containers/Node/NodeStructure/Pdisk.tsx, line 138
The variable allocated was introduced to hold row.AllocatedSize (and is already verified numeric on line 131), but the capacity expression redundantly accesses row.AllocatedSize again instead of reusing allocated. This is inconsistent and makes the code harder to reason about.

                        value={allocated}
                        capacity={Number(allocated) + Number(row.AvailableSize)}

@Raubzeug
Copy link
Contributor

Raubzeug commented Mar 3, 2026

Please rebase your branch to the latest main!

@Arunkoo Arunkoo force-pushed the fix/empty-data-placeholder-tables branch 2 times, most recently from a3f603c to adbb375 Compare March 3, 2026 12:48
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

src/containers/Node/NodeStructure/Pdisk.tsx, line 142
The allocated variable is extracted and used for the value prop, but the capacity expression still accesses row.AllocatedSize directly. Since allocated === row.AllocatedSize, this is functionally correct, but it's inconsistent — either always use the extracted variable or always use the row field.

                return (
                    <ProgressViewer
                        value={allocated}
                        capacity={Number(allocated) + Number(row.AvailableSize)}
                        formatValues={formatStorageValuesToGb}
                        colorizeProgress={true}
                    />
                );

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@Arunkoo Arunkoo force-pushed the fix/empty-data-placeholder-tables branch from adbb375 to a7cc0a7 Compare March 3, 2026 12:53
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (2)

src/components/nodesColumns/columns.tsx, line 210
The PR description states "Updated ProgressViewer...to use the placeholder", but src/components/ProgressViewer/ProgressViewer.tsx itself was not modified. Its hardcoded "no data" fallback (line 106) remains unchanged:

return <div className={`${b({size})} ${className} error`}>no data</div>;

Guards were added here and in Pdisk.tsx to prevent ProgressViewer from being called when data is missing — which is a valid defensive pattern — but all other call sites throughout the codebase (e.g. FullNodeViewer.tsx, StorageGroupInfo.tsx, PDiskPage/PDiskSpaceDistribution) still pass potentially non-numeric values directly to ProgressViewer and will still display the raw "no data" string rather than EMPTY_DATA_PLACEHOLDER.

To achieve full consistency across all usages without per-callsite guards, consider also updating the fallback in ProgressViewer.tsx line 106:

return <div className={`${b({size})} ${className} error`}>{EMPTY_DATA_PLACEHOLDER}</div>;

src/containers/Node/NodeStructure/Pdisk.tsx, line 142
The extracted allocated variable is not reused consistently in the capacity expression. Line 137 correctly uses value={allocated}, but line 138 still references row.AllocatedSize directly.

                return (
                    <ProgressViewer
                        value={allocated}
                        capacity={Number(allocated) + Number(row.AvailableSize)}
                        formatValues={formatStorageValuesToGb}
                        colorizeProgress={true}

@Arunkoo Arunkoo force-pushed the fix/empty-data-placeholder-tables branch from a7cc0a7 to 654b9db Compare March 3, 2026 13:30
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Additional Comments (1)

src/containers/Node/NodeStructure/Pdisk.tsx, line 138
After extracting allocated from row.AllocatedSize and passing it as value, the capacity expression re-reads row.AllocatedSize directly instead of reusing the local variable. This creates unnecessary duplication. Use the extracted variable consistently:

                        value={allocated}
                        capacity={Number(allocated) + Number(row.AvailableSize)}

@astandrik astandrik added this pull request to the merge queue Mar 3, 2026
Merged via the queue into ydb-platform:main with commit 6372d9b Mar 3, 2026
16 of 17 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.

3 participants