Skip to content
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

make accessing query result records in ascii view type safe #1281

Merged
merged 5 commits into from
Feb 16, 2021

Conversation

HerrEmil
Copy link
Contributor

@HerrEmil HerrEmil commented Feb 12, 2021

A crash was reported when closing a frame while it is running a query, with a screenshot that showed a runtime type error when evaluating result.records.length when result was null. I was unable to recreate exactly the error in the screenshot, but could trigger a runtime type error when evaluating result.records.length in ascii view by opening the text tab, re-running a query and closing the frame while the query was running.

This PR fixes the potential type error in ascii view and also adds stricter typings to the ascii view component, requests duck file and other files connected to these files to make it easier to fix any other unknown type errors when evaluating query result in other components.

changelog: Fix bug when closing a frame with a running query

Copy link
Contributor

@OskarDamkjaer OskarDamkjaer left a comment

Choose a reason for hiding this comment

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

Great to see these kinds of changes 💯

src/browser/modules/Stream/CypherFrame/AsciiView.tsx Outdated Show resolved Hide resolved
src/browser/modules/Stream/CypherScriptFrame/Summary.tsx Outdated Show resolved Hide resolved
@@ -124,7 +128,7 @@ function Stream(props: StreamProps): JSX.Element {
stack: frameObject.stack
}

let MyFrame = getFrame(frame.type)
let MyFrame = getFrame(frame.type as FrameType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope to change frame.type to avoid the cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried but it was not trivial to share the type unfortunately

@@ -0,0 +1,89 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This file had a long time coming, thanks! 🙏

src/shared/modules/commands/helpers/cypher.ts Show resolved Hide resolved
Emil Andersson added 2 commits February 15, 2021 12:17
@OskarDamkjaer OskarDamkjaer self-requested a review February 15, 2021 12:27
Copy link
Contributor

@OskarDamkjaer OskarDamkjaer left a comment

Choose a reason for hiding this comment

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

Nice changes, merge on green!

@HerrEmil HerrEmil merged commit ce1c2cf into neo4j:master Feb 16, 2021
@HerrEmil HerrEmil deleted the cancel-crash-fix branch February 16, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants