[Workspace] feat: optimize recent items and filter out items whose workspace is deleted #8900
Conversation
raintygao
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli,
joshuali925 and
huyaboo
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8900 +/- ##
=======================================
Coverage 60.87% 60.87%
=======================================
Files 3802 3802
Lines 91060 91060
Branches 14370 14370
=======================================
+ Hits 55435 55436 +1
+ Misses 32085 32084 -1
Partials 3540 3540
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -213,45 +226,45 @@ export const RecentItems = ({ | |||
| type: item.meta?.type || '', | |||
| id: item.id, | |||
| })); | |||
|
|
|||
| if (savedObjects.length) { | |||
| // Deep compare navLinks to avoid unnecessary requests | |||
There was a problem hiding this comment.
How about using useDeepCompareEffect instead? Current implementation won't be executed again after other dependencies(eg: recentlyAccessedItems or workspaceList ) changed but navLinks are same.
There was a problem hiding this comment.
After offline discussion, we only need to deep compare navLinks because only its reference would be changed after appUpdater.
| .href, | ||
| }; | ||
| // If the workspace id is existing but the workspace is deleted, filter the item | ||
| if (recentAccessItem.workspaceId && !findWorkspace) { |
There was a problem hiding this comment.
Do we need to check if workspace feature enabled here?
There was a problem hiding this comment.
If workspace feature is disabled, recentAccessItem would not have workspaceId.
| bulkGetDetail(savedObjects, http).then((res) => { | ||
| const filteredNavLinks = navLinks.filter((link) => !link.hidden); | ||
| const formatDetailedSavedObjects = res.map((obj) => { | ||
| const formatDetailedSavedObjects = res.flatMap((obj) => { |
There was a problem hiding this comment.
can we do filtering by workspace before bulkGet as ChromeRecentlyAccessedHistoryItem has workspace already.
…eleted Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
Signed-off-by: tygao <[email protected]>
…rkspace is deleted (#8900) * feat: optimize recent items and filter out items whose workspace is deleted Signed-off-by: tygao <[email protected]> * Changeset file for PR #8900 created/updated * seperate link Signed-off-by: tygao <[email protected]> * update filter sequence Signed-off-by: tygao <[email protected]> --------- Signed-off-by: tygao <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit c24d5bc) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Screenshot
No UI change
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration