-
Notifications
You must be signed in to change notification settings - Fork 935
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
[Discover] Query Editor Shortcut Fires Incorrect Query #9248
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sean Li <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9248 +/- ##
==========================================
- Coverage 61.66% 61.66% -0.01%
==========================================
Files 3817 3817
Lines 91711 91712 +1
Branches 14515 14515
==========================================
- Hits 56558 56556 -2
- Misses 31524 31526 +2
- Partials 3629 3630 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
did we want to get this into 2.19? |
@@ -354,11 +355,11 @@ export const QueryEditorUI: React.FC<Props> = (props) => { | |||
|
|||
editor.addCommand(monaco.KeyCode.Enter, () => { | |||
const newQuery = { | |||
...query, | |||
...queryRef.current, |
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.
Regarding this change, I see that earlier in the file, queryRef
exists for monaco 'commands' to access the latest query state.
If query
was a stale query state, why wasn't the single line editor failing a test that would update the query?
If there is a test for an updated query state, and query
has the updated query state, do we need queryRef at all? It only seems to be used here.
Description
onSubmit()
function from the editor shortcut, theQueryEditorTopRow
uses the props passed from the search bar as dates, which may be out of sync compared to the query service state. This PR passes in the correct date form the query service.query
object was being used to construct the new query. This PR updates it to match the single line editor, which uses thequeryRef
defined in the same file.Issues Resolved
Screenshot
Testing the changes
Changelog
-fix: Fix Discover query editor enter shortcuts reverting date range
Check List
yarn test:jest
yarn test:jest_integration