[Discover] A bunch of navigation fixes #5168

ashwin-pc · 2023-10-02T05:21:50Z

Description

Noteable fixes here are:

Replaces history api with application service's navigate method to correctly preserve state during redirection. The changes were made in:
- Breadcrumbs
- Opening a saved search
- Creating a new saved search
Adds a hydration action to load a url state into redux if a change to url should update the redux store. This fixes the issue where browser navigation did not update redux state.

Note: Recreated from #5123 since targeting that branch to main caused some issues during rebase.

Issues Resolved

closes #5056
closes #5157
closes #5066
closes #5136

Screenshot

Screen.Recording.2023-09-26.at.2.27.21.PM.mov

Testing the changes

Opening a saved search

Change timerange
Open a saved search
Timerange should persist

Creating a new search

Open a saved search
Change timerange
Click "New" in the menu bar
Timerange should persist

Reset

Open a saved search
Change timerange
Click "Reset" above the histogram
Timerange should persist

Navigation

Go to Discover
Open a saved search
Go back in the browser
You should be able to go back to the page where the saved search was not open

Check List

codecov · 2023-10-02T05:38:58Z

Codecov Report

Merging #5168 (21c71a3) into main (7d89cca) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 18.75%.

@@           Coverage Diff           @@
##             main    #5168   +/-   ##
=======================================
  Coverage   66.72%   66.73%           
=======================================
  Files        3278     3278           
  Lines       62985    62999   +14     
  Branches    10029    10031    +2     
=======================================
+ Hits        42026    42041   +15     
  Misses      18488    18488           
+ Partials     2471     2470    -1

Flag	Coverage Δ
Linux_1	`35.31% <ø> (-0.01%)`	⬇️
Linux_2	`55.24% <ø> (ø)`
Linux_3	`43.74% <18.75%> (-0.01%)`	⬇️
Linux_4	`35.42% <ø> (-0.01%)`	⬇️
Windows_1	`35.33% <ø> (?)`
Windows_3	`?`
Windows_4	`35.42% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...lication/utils/state_management/discover_slice.tsx	`45.16% <ø> (ø)`
.../discover/public/opensearch_dashboards_services.ts	`58.82% <ø> (ø)`
...plication/components/top_nav/open_search_panel.tsx	`40.00% <0.00%> (+11.42%)`	⬆️
...ta_explorer/public/utils/state_management/store.ts	`21.42% <21.42%> (+0.73%)`	⬆️

... and 6 files with indirect coverage changes

AMoo-Miki · 2023-10-02T19:04:29Z

CHANGELOG.md

@@ -10,6 +10,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

 ### 🛡 Security

+- [CVE-2022-25869] Removes angularJS 1.8 ([#5068](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5068))


This URL takes me to an issue; is there a typo?

Also:

Suggested change

- [CVE-2022-25869] Removes angularJS 1.8 ([#5068](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5068))

- [CVE-2022-25869] Remove AngularJS 1.8 ([#5068](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5068))

- ```

AMoo-Miki · 2023-10-02T19:09:28Z

CHANGELOG.md

@@ -53,6 +55,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Correct the generated path for downloading plugins by their names on Windows ([#4953](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4953))
 - [BUG] Fix buildPointSeriesData unit test fails due to local timezone ([#4992](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4992))
 - [BUG][Data Explorer][Discover] Fix total hits issue for no time based data ([#5087](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5087))
+- [BUG][Discover] Fix misc navigation issues ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))
+- [BUG][Discover] Fixes mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))


Suggested change

- [BUG][Discover] Fixes mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))

- [BUG][Discover] Fix mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))

AMoo-Miki · 2023-10-02T19:09:42Z

CHANGELOG.md

@@ -38,6 +39,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Adds Data explorer framework and implements Discover using it ([#4806](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4806))
 - [Theme] Use themes' definitions to render the initial view ([#4936](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4936/))
 - [Theme] Make `next` theme the default ([#4854](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4854/))
+- [Discover] Updated embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081/))


Suggested change

- [Discover] Updated embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081/))

- [Discover] Update embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081/))

AMoo-Miki · 2023-10-02T19:12:31Z

src/plugins/data_explorer/public/utils/state_management/store.ts

@@ -62,6 +80,21 @@ export const getPreloadedStore = async (services: DataExplorerServices) => {
  // the store subscriber will automatically detect changes and call handleChange function
  const unsubscribe = store.subscribe(handleChange);

+  services.scopedHistory.listen(async (location, action) => {


Should we hold this for better testing?

So the reason for this is because of the shared data model of OSD where the state is maintained in both the URL and the react memory. So far the assumption i've made is to have redux be the source of truth and update every other place from there. The problem here is that when the user navigates back using browser history, the URL state is no longer in sync with the redux state. The opensearch_dashboard_util wrappers that do the url state sync do something very similar.

The one big concern I had with this was a race condition. This here is mitigated for both a URL update and Redux store update using the check to see if the url update was a pop action and if the states are identical. I'm open to improvements here but I think we should ship with this given that its a jarring expereince for users who have relied on browser navigation for discover.

AMoo-Miki · 2023-10-02T20:21:28Z

src/plugins/data_explorer/public/utils/state_management/store.ts

+    if (isEqual(urlState, currentState)) {
+      return;
+    }
+
+    // If the url state is different from the current state, then we need to update the store
+    // the state should have a view property if it was loaded from the url
+    if (action === 'POP' && urlState.metadata?.view) {
+      store.dispatch(hydrate(urlState as RootState));
+    }


While the way this is coded has excellent readability, I wonder if we would get noticeable performance improvement if we moved the isEqual check inside the action === 'POP' && urlState.metadata?.view condition.

That is a nice improvement :) Will make the change

AMoo-Miki · 2023-10-02T20:22:45Z

src/plugins/discover/public/application/components/top_nav/get_top_nav_links.tsx

+      core.application.navigateToApp('discover', {
+        path: '#/',
+      });


Would this reload the page if i was already on discover?

Yes it does :)

AMoo-Miki · 2023-10-02T20:24:49Z

src/plugins/discover/public/application/view_components/canvas/index.tsx

-        <EuiFlexItem>
-          <DiscoverNoResults timeFieldName={timeField} queryLanguage={''} />
-        </EuiFlexItem>
+        <DiscoverNoResults timeFieldName={timeField} queryLanguage={''} />


Does this appear at the vertical and horizontal center of the page?

It spans the width of the container but does not center to the vertical center. The flex was originally added to make the app appear as a full page application. Users have since complained about its usability like that so this change reverts it to a simpler layout allowing for scrolling, so having a centered component would not be really helpful.

github-actions · 2023-10-02T20:49:51Z

[MANUAL CYPRESS TEST RUN RESULTS]

✅ Cypress test run succeeded!

Inputs:

Source repo: opensearch-project/OpenSearch-Dashboards
Source branch: Test repo: `opensearch-project/opensearch-dashboards-functional-test` Test branch:

Test spec:
cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/vis_*,cypress/integration/core-opensearch-dashboards/opensearch-dashboards/dashboard_sanity_test_spec.js,

Link to results:

https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/6385136186

joshuarrrr · 2023-10-03T13:45:04Z

Needs conflict resolution

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

opensearch-trigger-bot · 2023-10-03T18:21:53Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5168-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cb6e0f05aa4c8b8f43352d8de0642e059afea093
# Push it to GitHub
git push --set-upstream origin backport/backport-5168-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5168-to-2.x.

* Discover: Fixes state persistence after nav * Fixed breadcrumbs and navigation * fixes mobile view --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Signed-off-by: Leo Deng <leo7deng@gmail.com>

* Discover: Fixes state persistence after nav * Fixed breadcrumbs and navigation * fixes mobile view --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit cb6e0f0) Signed-off-by: Miki <miki@amazon.com>

* [BUG][Discover] Add onQuerySubmit to top nav and allow force update to embeddable (#5160) * all reload to force update embeddable * add onQuerySubmit to top nav Issue Resolve #5116 #5159 Signed-off-by: ananzh <ananzh@amazon.com> Co-authored-by: Miki <miki@amazon.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit 7d89cca) Signed-off-by: Miki <miki@amazon.com> * [Discover] A bunch of navigation fixes (#5168) * Discover: Fixes state persistence after nav * Fixed breadcrumbs and navigation * fixes mobile view --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit cb6e0f0) Signed-off-by: Miki <miki@amazon.com> * [BUG][Data Explorer][Discover] Automatically load solo added default index pattern (#5171) * [BUG][Data Explorer][Discover] Automatically load solo added default index pattern This fix ensures that when add a default index pattern, Discover will automatically select and load its details. Issue Resolve #5128 --------- Signed-off-by: ananzh <ananzh@amazon.com> (cherry picked from commit 9601c6c) Signed-off-by: Miki <miki@amazon.com> * [BUG][Data Explorer][Discover] Allow data grid to auto adjust size based on fetched data count (#5191) * This PR adds a new rows state to the DiscoverCanvas component and updated it whenever the data$ observable emitted new row data. * The DiscoverTable component was then refactored to accept rows as a prop, making it dependent on the parent component to provide the correct set of data. This ensures that the table renders correctly based on the current data and doesn't rely on its internal state, which could be outdated. Issue Resolve #5181 Signed-off-by: ananzh <ananzh@amazon.com> (cherry picked from commit 677fdf5) Signed-off-by: Miki <miki@amazon.com> * Fixes mobile layout (#5195) Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit 0ffd2ab) * [BUG][Data Explorer][Discover] Allow filter and query persist when refresh page or paste url to a new tab (#5206) Issue Resolve #5179 #5071 Signed-off-by: ananzh <ananzh@amazon.com> (cherry picked from commit 5623cef) Signed-off-by: Miki <miki@amazon.com> * fixes DataTable rendering in doscover (#5207) Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit c70125f) --------- Signed-off-by: ananzh <ananzh@amazon.com> Signed-off-by: Miki <miki@amazon.com> Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>

* Discover: Fixes state persistence after nav * Fixed breadcrumbs and navigation * fixes mobile view --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Signed-off-by: Willie Hung <willie880201044@gmail.com>

* Discover: Fixes state persistence after nav * Fixed breadcrumbs and navigation * fixes mobile view --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc requested review from ananzh, kavilla, seanneumann, AMoo-Miki, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy and curq as code owners October 2, 2023 05:21

github-actions bot added the distinguished-contributor label Oct 2, 2023

ashwin-pc mentioned this pull request Oct 2, 2023

[Discover] Fixes state persistence after nav #5123

Closed

8 tasks

ashwin-pc added backport 2.x discover for discover reinvent de-angular de-angularize work v2.11.0 labels Oct 2, 2023

AMoo-Miki reviewed Oct 2, 2023

View reviewed changes

joshuarrrr mentioned this pull request Oct 2, 2023

[Fix] ValidationError when setting maps.proxyOpenSearchMapsServiceInMaps #5170

Merged

8 tasks

AMoo-Miki reviewed Oct 2, 2023

View reviewed changes

AMoo-Miki previously approved these changes Oct 3, 2023

View reviewed changes

ananzh previously approved these changes Oct 3, 2023

View reviewed changes

ashwin-pc added 6 commits October 3, 2023 16:43

Discover: Fixes state persistence after nav

ca16442

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Fixed breadcrumbs and navigation.

4e9ee48

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Adds changelog

e8db30f

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

fixes mobile view

b8316cd

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

Addresses comments

d8b133b

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

fix changelog

21c71a3

Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>

ashwin-pc dismissed stale reviews from ananzh and AMoo-Miki via 21c71a3 October 3, 2023 16:57

ashwin-pc force-pushed the fix/persist-state-nav branch from 8fe670a to 21c71a3 Compare October 3, 2023 16:57

AMoo-Miki approved these changes Oct 3, 2023

View reviewed changes

ananzh approved these changes Oct 3, 2023

View reviewed changes

ananzh merged commit cb6e0f0 into opensearch-project:main Oct 3, 2023
54 of 55 checks passed

opensearch-trigger-bot bot added the failed backport label Oct 3, 2023

AMoo-Miki mentioned this pull request Oct 4, 2023

[Manual 2.x] De-angular backports collection #5219

Merged

8 tasks

AMoo-Miki removed the failed backport label Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] A bunch of navigation fixes #5168

[Discover] A bunch of navigation fixes #5168

ashwin-pc commented Oct 2, 2023 •

edited

codecov bot commented Oct 2, 2023 •

edited

AMoo-Miki Oct 2, 2023 •

edited

AMoo-Miki Oct 2, 2023

AMoo-Miki Oct 2, 2023

AMoo-Miki Oct 2, 2023

ashwin-pc Oct 2, 2023

AMoo-Miki Oct 2, 2023

ashwin-pc Oct 2, 2023

AMoo-Miki Oct 2, 2023

ashwin-pc Oct 2, 2023

AMoo-Miki Oct 2, 2023

ashwin-pc Oct 2, 2023

github-actions bot commented Oct 2, 2023

joshuarrrr commented Oct 3, 2023

opensearch-trigger-bot bot commented Oct 3, 2023

		@@ -10,6 +10,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

		### 🛡 Security

		- [CVE-2022-25869] Removes angularJS 1.8 ([#5068](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5068))

	- [CVE-2022-25869] Removes angularJS 1.8 ([#5068](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5068))
	- [CVE-2022-25869] Remove AngularJS 1.8 ([#5068](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5068))
	- ```

	- [BUG][Discover] Fixes mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))
	- [BUG][Discover] Fix mobile view ([#5168](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5168))

	- [Discover] Updated embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081/))
	- [Discover] Update embeddable for saved searches ([#5081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5081/))

[Discover] A bunch of navigation fixes #5168

[Discover] A bunch of navigation fixes #5168

Conversation

ashwin-pc commented Oct 2, 2023 • edited

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Oct 2, 2023 • edited

Codecov Report

AMoo-Miki Oct 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 2, 2023

[MANUAL CYPRESS TEST RUN RESULTS]

✅ Cypress test run succeeded!

Inputs:

Link to results:

joshuarrrr commented Oct 3, 2023

opensearch-trigger-bot bot commented Oct 3, 2023

ashwin-pc commented Oct 2, 2023 •

edited

codecov bot commented Oct 2, 2023 •

edited

AMoo-Miki Oct 2, 2023 •

edited