[Discover] Persist query mode to local storage#250388
[Discover] Persist query mode to local storage#250388AlexGPlay merged 20 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
|
@jughosta could you please take the review on this one? |
| } else { | ||
| dispatch(transitionFromESQLToDataView({ dataViewId: currentDataView?.id ?? '' })); | ||
| } | ||
| services.storage.set(DISCOVER_QUERY_MODE_KEY, 'classic'); |
There was a problem hiding this comment.
I would suggest to clear the previously saved value then so when we continue with #250038 it would not interfere.
There was a problem hiding this comment.
do you mean to instead of save which of the modes was the last one we just store if the last one was ESQL? i guess the problem then is going to be that once ESQL is the default in #250201 if someone switches back to classic it won't use it as the next default
There was a problem hiding this comment.
Okay, good point! Then we just need to keep in mind that if users had switched to classic before we enabled ES|QL by default then they will continue using the classic mode without noticing the change in Discover cc @kertal
There was a problem hiding this comment.
thx for raising, yes, also didn't think about this. so it makes sense, to store it with classic, but I think further down the road, we might think of solving it differently, because as @jughosta highlighted, one we generally enable ES|QL as default, those users still would see classic ... worth a comment in the code, so it doesn't get lost
| ) { | ||
| dispatch(internalStateActions.setIsESQLToDataViewTransitionModalVisible(true)); | ||
| } else { | ||
| dispatch(transitionFromESQLToDataView({ dataViewId: currentDataView?.id ?? '' })); |
There was a problem hiding this comment.
Since the transition modal to Classic can be dismissed, the value should not change to classic in this case.
Then a single place in the code, where to move services.storage.set(DISCOVER_QUERY_MODE_KEY, 'classic'); to, might be inside transitionFromESQLToDataView action. Or better as a redux listener to the action.
For consistency we could apply a similar change to transitionFromESQLToDataView.
There was a problem hiding this comment.
that makes sense, i thought of that at first but i wasn't sure if we wanted to introduce a side-effect in the redux actions
There was a problem hiding this comment.
updated to redux listener in 5a4c68b - it seems to be working but it's my first time doing one redux listener so I hope it's the way it should be
There was a problem hiding this comment.
I meant listeners for transitionFromESQLToDataView and transitionFromDataViewToESQL actions. We still want to capture only the manual changes to the mode.
There was a problem hiding this comment.
Thanks for looking into it!
There was a problem hiding this comment.
i wasn't sure if we wanted to introduce a side-effect in the redux actions
Just wanted to say this is totally fine to do in thunks and middleware, often preferable even. It keeps the logic consolidated and makes it easy to test.
Personally in your case I'd put the logic directly in the thunk to avoid the extra actions indirection, but middleware works too.
There was a problem hiding this comment.
i'm happy with either, i've seen now that i can access the storage in the thunks - no strong opinion from my side
jughosta
left a comment
There was a problem hiding this comment.
Works well and LGTM 👍
Sorry for the delay!
| await common.navigateToApp('discover', { path: '' }); | ||
| await testSubjects.existOrFail('discover-dataView-switch-link'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Would be nice to also add a test when the path is not empty.
There was a problem hiding this comment.
i think the unit tests are kind of covering this scenario, but i can follow up in the feature flag PR with more tests if it doesn't look that way
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
cc @AlexGPlay |
## Summary Closes elastic#200765 When changing between classic and ES|QL mode we persist it in local storage so the next default session uses that preference. --- Before (Example with ESQL) https://github.com/user-attachments/assets/a689c0f9-ff69-4bcb-a8da-108d4693cb56 After (Example with ESQL) https://github.com/user-attachments/assets/bebc03db-a860-4ec9-9843-5e1f0e53f585 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
) ## Summary Closes #250201 Follow up to #250388 - now on top of storing the last used query mode, if there's nothing stored as the last one, and if possible, ES|QL will be the default when the `isEsqlDefault` feature flag is set to true. > [!TIP] > To test it set `feature_flags.overrides.discover.isEsqlDefault` to true in your `kibana.dev.yml` ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
Summary
Closes #200765
When changing between classic and ES|QL mode we persist it in local storage so the next default session uses that preference.
Before (Example with ESQL)
before.default.query.mov
After (Example with ESQL)
after.default.query.mov
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:*label is applied per the guidelinesbackport:*labels.