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

[Data Explorer] Allow render from View directly, not from Data Explorer #6167

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Mar 15, 2024

Description

This PR avoids re-rendering the entire AppContainer when data services update. The re-render is caused by history object which is in AppMountParameters. This history object is part of the application's routing context and can change frequently as the url is updated by query, filter or time range. Each change in the history object could potentially trigger a re-render of the AppContainer and its child components.

With this PR, the AppContainer will not be re-loaded. Instead, each component, like Canvas and Panel, will be re-rendered based on the updates from data.

Issues Resolved

NA

Screenshot

  • new workflow
Screen.Recording.2024-03-18.at.1.37.25.PM.mov

Changelog

  • feat: [Data Explorer] Allow render from View directly, not from Data Explorer

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.42%. Comparing base (c6820db) to head (56a33c7).
Report is 5 commits behind head on main.

Files Patch % Lines
.../data_explorer/public/components/app_container.tsx 66.66% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6167      +/-   ##
==========================================
- Coverage   67.44%   67.42%   -0.03%     
==========================================
  Files        3444     3445       +1     
  Lines       67849    67791      -58     
  Branches    11035    11026       -9     
==========================================
- Hits        45764    45705      -59     
  Misses      19418    19418              
- Partials     2667     2668       +1     
Flag Coverage Δ
Linux_1 33.11% <ø> (+0.02%) ⬆️
Linux_2 55.04% <ø> (-0.09%) ⬇️
Linux_3 ?
Linux_4 ?
Windows_1 33.13% <ø> (+0.02%) ⬆️
Windows_2 55.00% <ø> (-0.09%) ⬇️
Windows_3 45.27% <80.00%> (-0.03%) ⬇️
Windows_4 34.85% <ø> (+0.03%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashwin-pc
Copy link
Member

the issue with this approach is that when the user changes the query, the current context is lost. In 2.9 the lable once rendered does not change until eother the new result comes in or there are no results

@ashwin-pc
Copy link
Member

ashwin-pc commented Mar 19, 2024

if history is the reason for the rerender, cant we just move it out of the props?

This PR avoids re-rendering the entire AppContainer when data services update.
The re-render is caused by history object which is in AppMountParameters. This
history object is part of the application's routing context and can change frequently
as the url is updated by query, filter or time range. Each change in the history object
could potentially trigger a re-render of the AppContainer and its child components.

With this PR, the AppContainer will not be re-loaded. Instead each component, like Canvas
and Panel, will be updated.

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh dismissed stale reviews from ashwin-pc and AMoo-Miki via 0af3b50 June 4, 2024 23:47
opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Jun 4, 2024
ananzh added a commit to ananzh/opensearch-dashboards-functional-test that referenced this pull request Jun 5, 2024
Due to opensearch-project/OpenSearch-Dashboards#6167,
we don't have the double render issue when global state is updated.

Therefore we don't need to call makeDatePickerMenuOpen function, which is just
to reclick the DatePickerToggle to make the menu re-open. This function is a tmp
solution to allow test passing.

In this PR, we will remove the function and all the usages.

Signed-off-by: Anan <ananzh@amazon.com>
@ananzh
Copy link
Member Author

ananzh commented Jun 5, 2024

@ananzh I think one of the tests are failing for refresh interval after re-running still. Mind checking it?

@kavilla I have fixed the test in opensearch-project/opensearch-dashboards-functional-test#1358

Since we don't have double render, the tmp solution

Cypress.Commands.add('makeDatePickerMenuOpen', () => {
  cy.get(
    '[class="euiFormControlLayout euiFormControlLayout--group euiSuperDatePicker"]'
  ).then(($popover) => {
    // Check if the popover does not have the 'euiPopover-isOpen' class
    if (!$popover.hasClass('euiPopover-isOpen')) {
      // If not open, click the button to open the quick menu
      cy.getElementByTestId('superDatePickerToggleQuickMenuButton').click();
    }
  });
})

, which is just to re-click the DatePickerToggle to make the menu re-open, is not required. We could just simply remove this tmp fix.

after.mp4

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh added the data explorer Issues related to the Data Explorer project label Jun 5, 2024
Copy link
Contributor

github-actions bot commented Jun 5, 2024

[MANUAL CYPRESS TEST RUN RESULTS]

✅ Cypress test run succeeded!

Inputs:

Test repo: 'ananzh/opensearch-dashboards-functional-test'
Test branch: 'update-test'

Test spec:
''

Link to results:

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

ananzh added a commit to opensearch-project/opensearch-dashboards-functional-test that referenced this pull request Jun 6, 2024
Due to opensearch-project/OpenSearch-Dashboards#6167,
we don't have the double render issue when global state is updated.

Therefore we don't need to call makeDatePickerMenuOpen function, which is just
to reclick the DatePickerToggle to make the menu re-open. This function is a tmp
solution to allow test passing.

In this PR, we will remove the function and all the usages.

Signed-off-by: Anan <ananzh@amazon.com>
opensearch-trigger-bot bot pushed a commit to opensearch-project/opensearch-dashboards-functional-test that referenced this pull request Jun 6, 2024
Due to opensearch-project/OpenSearch-Dashboards#6167,
we don't have the double render issue when global state is updated.

Therefore we don't need to call makeDatePickerMenuOpen function, which is just
to reclick the DatePickerToggle to make the menu re-open. This function is a tmp
solution to allow test passing.

In this PR, we will remove the function and all the usages.

Signed-off-by: Anan <ananzh@amazon.com>
(cherry picked from commit 2881c0f)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: A better home for this is @osd/std.

ananzh added a commit to opensearch-project/opensearch-dashboards-functional-test that referenced this pull request Jun 6, 2024
Due to opensearch-project/OpenSearch-Dashboards#6167,
we don't have the double render issue when global state is updated.

Therefore we don't need to call makeDatePickerMenuOpen function, which is just
to reclick the DatePickerToggle to make the menu re-open. This function is a tmp
solution to allow test passing.

In this PR, we will remove the function and all the usages.

Signed-off-by: Anan <ananzh@amazon.com>
(cherry picked from commit 2881c0f)

Co-authored-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh merged commit b792242 into opensearch-project:main Jun 6, 2024
67 of 69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 6, 2024
…er (#6167)

* [Discover] Remove double render

This PR avoids re-rendering the entire AppContainer when data services update.
The re-render is caused by history object which is in AppMountParameters. This
history object is part of the application's routing context and can change frequently
as the url is updated by query, filter or time range. Each change in the history object
could potentially trigger a re-render of the AppContainer and its child components.

With this PR, the AppContainer will not be re-loaded. Instead each component, like Canvas
and Panel, will be updated.

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

* fix PR comment

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

---------

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
(cherry picked from commit b792242)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ananzh pushed a commit that referenced this pull request Jun 7, 2024
…er (#6167) (#6959)

* [Discover] Remove double render

This PR avoids re-rendering the entire AppContainer when data services update.
The re-render is caused by history object which is in AppMountParameters. This
history object is part of the application's routing context and can change frequently
as the url is updated by query, filter or time range. Each change in the history object
could potentially trigger a re-render of the AppContainer and its child components.

With this PR, the AppContainer will not be re-loaded. Instead each component, like Canvas
and Panel, will be updated.

---------


(cherry picked from commit b792242)

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
yubonluo pushed a commit to yubonluo/opensearch-dashboards-functional-test that referenced this pull request Jun 7, 2024
Due to opensearch-project/OpenSearch-Dashboards#6167,
we don't have the double render issue when global state is updated.

Therefore we don't need to call makeDatePickerMenuOpen function, which is just
to reclick the DatePickerToggle to make the menu re-open. This function is a tmp
solution to allow test passing.

In this PR, we will remove the function and all the usages.

Signed-off-by: Anan <ananzh@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x data explorer Issues related to the Data Explorer project discover for discover reinvent distinguished-contributor v2.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants