-
Notifications
You must be signed in to change notification settings - Fork 884
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
Select the correct language when a different one is chosen by the dataset selector #8671
base: main
Are you sure you want to change the base?
Conversation
…aset selector Authored-by: Suchit Sahoo <suchsah@amazon.com> Signed-off-by: Miki <miki@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8671 +/- ##
=======================================
Coverage 60.85% 60.85%
=======================================
Files 3793 3793
Lines 90368 90368
Branches 14181 14181
=======================================
Hits 54997 54997
- Misses 31893 31894 +1
+ Partials 3478 3477 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
const newQuery = this.queryString.getInitialQuery(); | ||
const newQueryString = newQuery.query; | ||
if (this.inputRef.getValue() !== newQueryString) { | ||
const newQueryString = this.props.query.query; |
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.
i think now when i select the dataset for the new language the query string is not updated with the updated dataset.
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.
recreation steps:
- have an index pattern and navigate to discover. load results
- select an index from a cluster.
- notice how the query (if for sql and ppl) still references the old dataset
@@ -44,7 +44,7 @@ export const QueryLanguageSelector = (props: QueryLanguageSelectorProps) => { | |||
: undefined; | |||
|
|||
useEffect(() => { | |||
const subscription = queryString.getUpdates$().subscribe((query: Query) => { | |||
const subscription = queryString.getUpdates$(0).subscribe((query: Query) => { |
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.
is it potentially just a race condition or something?
could we verify if the props.query.language is the current query always. and this component gets re-rendered. i need to verify since last checking these things out. or if you have the insight already.
because what im thinking is that potentially the dataset is checking which languages are supported and adding to a list while this is listening to changes and before the language is added this setter is called but set to the first language it found.
there's a couple of places i think were we need loading spinners. shooting in the dark perhaps we just need to check if everything is done loading before setting the current language?
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.
or opt for just disabling unsupported languages and setting the current value vs removing them from view
Marking as draft because we have another fix. Will close once Miki confirms that there is nothing in this PR thats needs to be picked up |
Description
Select the correct language when a different one is chosen by the dataset selector
Changelog
Check List
yarn test:jest
yarn test:jest_integration