Skip to content

Conversation

@DaryaVorontsova
Copy link
Contributor

@DaryaVorontsova DaryaVorontsova commented Nov 10, 2025

Stand: https://nda.ya.ru/t/MBz7A5F67MiSzf
Example with empty error field: https://nda.ya.ru/t/ySdr2jMd7MiSXF

CI Results

Test Status: ❌ FAILED

πŸ“Š Full Report

Total Passed Failed Flaky Skipped
378 372 1 3 2
Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

  1. Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
  2. Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: πŸ”½

Current: 66.07 MB | Main: 66.08 MB
Diff: 8.51 KB (-0.01%)

βœ… Bundle size decreased.

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

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 10, 2025

Skipped: This PR does not contain any of your configured keywords: (greptile-review)

@DaryaVorontsova DaryaVorontsova marked this pull request as ready for review November 11, 2025 06:01
@DaryaVorontsova
Copy link
Contributor Author

ydb-platform/ydb#25588

const items = prepareStreamingQueryItems(sysData);

return (
<Flex direction="column" gap="4">
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, It doesn't affect anything, i will remove it

let errorData: ErrorResponse | string | undefined;
if (typeof errorRaw === 'string') {
try {
errorData = JSON.parse(errorRaw) as ErrorResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case it seems that if error is not ErrorResponse, we shouldn't try to do anything with is. Lets add check isQueryErrorResponse and it will be enough.

Copy link
Contributor Author

@DaryaVorontsova DaryaVorontsova Nov 11, 2025

Choose a reason for hiding this comment

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

Error, that I received, is not ErrorResponse:
{\"severity\":1,\"issues\":[{\"message\"...
I don't have field error in the answer so this function isQueryErrorResponse will always return false(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed it, decided to keep this manual check of the error type

return text;
}

let normalized = text.replace(/^\s*\n+/, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe trim will be enough?

@@ -0,0 +1,6 @@
{
"noData": "No data for entity:",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use naming rules https://nda.ya.ru/t/IA1ObQgE7MmHgF

return {error};
}
},
forceRefetch() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need forceRefetch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, you're right, we can drop forceRefetch. I only added it to keep the status up to date))

return content;
return (
<Button view="normal" onClick={() => setShowIssues(!showIssues)}>
{showIssues ? 'Hide details' : 'Show details'}
Copy link
Contributor

Choose a reason for hiding this comment

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

lets add i18n


&__error-list,
&__error-list-item {
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

lets better use Flex component instead of custom stypes


background-color: var(--g-color-base-background);

&_column {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use Flex here too

}

export function ResultIssues({data, hideSeverity}: ResultIssuesProps) {
export function ResultIssues({
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do it this way instead:

  1. Let's split the component into ErrorPreview and ErrorDetails - we will constructor all other views from them
  2. Make a new component ResultIssuesModal
  3. If the error has several roots, then let's output a separate view for each root like
Screenshot 2025-11-11 at 11 30 56

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