-
Notifications
You must be signed in to change notification settings - Fork 15
feat: download dump with all info #1862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
@@ -13,6 +13,10 @@ | |||
"title.result": "Result", | |||
"text_plan-svg": "Execution plan", | |||
"text_open-new-tab": "Open in new tab", | |||
"text_open-new-tab_description": "New tab", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description seems to be redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
const diagnosticsData = { | ||
...queryResultsInfo, | ||
database, | ||
timestamp: new Date().toISOString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were user, I would expect that in timestamp
field it is some query stats - start or finish. What do you think about renaming this property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
queryResultsInfo={{ | ||
ast: data.ast, | ||
stats: data.stats, | ||
queryText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets' move queryText
on top, so it would be easier to identify what about diagnostics file is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
const url = URL.createObjectURL(blob); | ||
const link = document.createElement('a'); | ||
link.href = url; | ||
link.download = `query-diagnostics-${new Date().getTime()}.json`; | ||
document.body.appendChild(link); | ||
link.click(); | ||
document.body.removeChild(link); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this logic to helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
stand redeployed |
As for me |
|
||
const renderSwitcher = (props: ButtonProps) => { | ||
return ( | ||
<Button view="flat-secondary" loading={isLoading} disabled={isLoading} {...props}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add ActionTooltip
for title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
Closes #984
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: 🔺
Current: 80.06 MB | Main: 80.05 MB
Diff: +9.74 KB (0.01%)
ℹ️ CI Information