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

[BUG] Fix management overview page duplicate rendering #4636

Merged

Conversation

Hailong-am
Copy link
Collaborator

Description

Refactor management overview page add useMemo to avoid duplicate rendering.

Issues Resolved

#4287 Add a new management overview page, whenever go to app/opensearch_management_overview page, the console reports below errors.

Screenshot

image

Testing the changes

Check List

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

@Hailong-am Hailong-am changed the title [BUG] Refactor for management overview page [BUG] Refactor for management overview page to avoid duplicate rendering Jul 28, 2023
@Hailong-am Hailong-am changed the title [BUG] Refactor for management overview page to avoid duplicate rendering [BUG] Fix management overview page duplicate rendering Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #4636 (0e1a830) into main (e9ad10f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4636      +/-   ##
==========================================
- Coverage   66.43%   66.43%   -0.01%     
==========================================
  Files        3396     3397       +1     
  Lines       64869    64882      +13     
  Branches    10358    10359       +1     
==========================================
+ Hits        43096    43102       +6     
- Misses      19214    19300      +86     
+ Partials     2559     2480      -79     
Flag Coverage Δ
Linux_1 34.85% <ø> (ø)
Linux_2 55.15% <ø> (ø)
Linux_3 44.23% <ø> (-0.01%) ⬇️
Linux_4 35.05% <100.00%> (+<0.01%) ⬆️
Windows_1 34.87% <ø> (ø)
Windows_2 55.12% <ø> (ø)
Windows_3 44.24% <ø> (ø)
Windows_4 35.05% <100.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
...plugins/management_overview/public/application.tsx 61.53% <100.00%> (ø)

... and 20 files with indirect coverage changes

@Hailong-am Hailong-am force-pushed the optimize_management_overview branch 2 times, most recently from ebd9a24 to fd252cd Compare August 2, 2023 01:00
@Hailong-am
Copy link
Collaborator Author

@ashwin-pc @manasvinibs @ananzh could you help to review the fix?

ashwin-pc
ashwin-pc previously approved these changes Aug 4, 2023
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM, can you resolve the changelog conflict though?

@Hailong-am
Copy link
Collaborator Author

LGTM, can you resolve the changelog conflict though?

rebased from main.

@Hailong-am Hailong-am force-pushed the optimize_management_overview branch from b3d7c13 to 892474e Compare August 9, 2023 03:14
@Hailong-am
Copy link
Collaborator Author

Link check failed due to, not sure whether it's transient issue

Errors in packages/osd-ui-shared-deps/flot_charts/jquery_flot_navigate.js
+ echo
+ echo ::set-output name=exit_code::2
✗ http://brandonaaron.net/ (HTTP status server error (523 <unknown status code>) for url (http://brandonaaron.net/))```

@Hailong-am
Copy link
Collaborator Author

@ashwin-pc conflicts merged and please help to add backport 2.x label.

@Hailong-am Hailong-am force-pushed the optimize_management_overview branch from 892474e to 5004ee0 Compare August 10, 2023 05:31
@Hailong-am Hailong-am force-pushed the optimize_management_overview branch from 30444a0 to c506258 Compare August 29, 2023 04:09
@Hailong-am Hailong-am requested a review from curq as a code owner August 29, 2023 04:09
ashwin-pc
ashwin-pc previously approved these changes Aug 29, 2023
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Looks good - I have one question for other maintainers to clarify first.

src/plugins/management_overview/public/index.ts Outdated Show resolved Hide resolved
@Hailong-am Hailong-am force-pushed the optimize_management_overview branch from 4b9f56c to bad97e2 Compare August 30, 2023 03:45
@ashwin-pc ashwin-pc merged commit 16b1e29 into opensearch-project:main Aug 31, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 31, 2023
* refactor for management overview page

Signed-off-by: Hailong Cui <[email protected]>

* Add missing changelog

Signed-off-by: Hailong Cui <[email protected]>

* Add unit test

Signed-off-by: Hailong Cui <[email protected]>

* remove duplicate snapshot validation

Signed-off-by: Hailong Cui <[email protected]>

* Update src/plugins/management_overview/public/application.test.tsx

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>

* Update src/plugins/management_overview/public/application.test.tsx

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>

* export empty plugin start to make it consistent

Signed-off-by: Hailong Cui <[email protected]>

* export empty plugin start to make it consistent

Signed-off-by: Hailong Cui <[email protected]>

---------

Signed-off-by: Hailong Cui <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 16b1e29)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
manasvinibs pushed a commit that referenced this pull request Sep 1, 2023
* refactor for management overview page

Signed-off-by: Hailong Cui <[email protected]>

* Add missing changelog

Signed-off-by: Hailong Cui <[email protected]>

* Add unit test

Signed-off-by: Hailong Cui <[email protected]>

* remove duplicate snapshot validation

Signed-off-by: Hailong Cui <[email protected]>

* Update src/plugins/management_overview/public/application.test.tsx

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>

* Update src/plugins/management_overview/public/application.test.tsx

Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>

* export empty plugin start to make it consistent

Signed-off-by: Hailong Cui <[email protected]>

* export empty plugin start to make it consistent

Signed-off-by: Hailong Cui <[email protected]>

---------

Signed-off-by: Hailong Cui <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Co-authored-by: Ashwin P Chandran <[email protected]>
(cherry picked from commit 16b1e29)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants