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

Fix the display and jump logic for recent assets #8136

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4707fac
Hide global objects in recent assets when workspace enabled, and jump…
Kapian1234 Sep 11, 2024
66f2b68
Changeset file for PR #8136 created/updated
opensearch-changeset-bot[bot] Sep 11, 2024
5a07c11
Update recently-accessed-service
Kapian1234 Sep 12, 2024
8ff5a28
Merge branch 'recent_assets' of github.com:Kapian1234/OpenSearch-Dash…
Kapian1234 Sep 12, 2024
9a540d3
fix the boolean value
Kapian1234 Sep 12, 2024
3340ea8
add unit test and fix some bugs
Kapian1234 Sep 12, 2024
41a1ca4
rename test cases
Kapian1234 Sep 12, 2024
37db157
Pass workspaceEnabled to createRecentNavLink
Kapian1234 Sep 30, 2024
8fff3cf
resolve conflicts
Kapian1234 Oct 9, 2024
e4fe87c
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
Kapian1234 Oct 29, 2024
879917e
Merge branch 'main' into recent_assets
ruanyl Oct 30, 2024
ea1e2ab
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
Kapian1234 Nov 4, 2024
0d046b9
Update unit tests
Kapian1234 Nov 4, 2024
291e82d
Merge branch 'recent_assets' of github.com:Kapian1234/OpenSearch-Dash…
Kapian1234 Nov 4, 2024
07a6f74
Merge branch 'main' into recent_assets
SuZhou-Joe Nov 15, 2024
af72d86
Resolve conflicts
Kapian1234 Dec 19, 2024
9c0064e
Merge branch 'recent_assets' of github.com:Kapian1234/OpenSearch-Dash…
Kapian1234 Dec 19, 2024
7c28a46
Remove workspaceEnabled props in recent_items
Kapian1234 Dec 24, 2024
0c92d9e
Fix links at recent accessed items
Kapian1234 Dec 24, 2024
4c83a8c
Fix unit tests
Kapian1234 Dec 25, 2024
101c4b8
Update snapshots
Kapian1234 Dec 25, 2024
d6ee2f1
Merge branch 'main' into recent_assets
SuZhou-Joe Dec 27, 2024
4d375a1
Modify unit tests
Kapian1234 Jan 9, 2025
bef66a3
Merge branch 'recent_assets' of github.com:Kapian1234/OpenSearch-Dash…
Kapian1234 Jan 9, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/8136.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Fix the display and jump logic for recent assets ([#8136](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8136))
2 changes: 1 addition & 1 deletion src/core/public/chrome/chrome_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ export class ChromeService {

const navControls = this.navControls.start();
const navLinks = this.navLinks.start({ application, http });
const recentlyAccessed = await this.recentlyAccessed.start({ http, workspaces });
const recentlyAccessed = await this.recentlyAccessed.start({ http, workspaces, application });
const docTitle = this.docTitle.start({ document: window.document });
const navGroup = await this.navGroup.start({
navLinks,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@
*/

import { Observable } from 'rxjs';

import { map } from 'rxjs/operators';
import { PersistedLog } from './persisted_log';
import { createLogKey } from './create_log_key';
import { HttpSetup } from '../../http';
import { WorkspacesStart } from '../../workspace';
import { InternalApplicationStart } from '../../application';

/** @public */
export interface ChromeRecentlyAccessedHistoryItem {
Expand All @@ -50,16 +51,18 @@ export interface ChromeRecentlyAccessedHistoryItem {
interface StartDeps {
http: HttpSetup;
workspaces: WorkspacesStart;
application: InternalApplicationStart;
}

/** @internal */
export class RecentlyAccessedService {
async start({ http, workspaces }: StartDeps): Promise<ChromeRecentlyAccessed> {
async start({ http, workspaces, application }: StartDeps): Promise<ChromeRecentlyAccessed> {
const logKey = await createLogKey('recentlyAccessed', http.basePath.getBasePath());
const history = new PersistedLog<ChromeRecentlyAccessedHistoryItem>(logKey, {
maxLength: 20,
isEqual: (oldItem, newItem) => oldItem.id === newItem.id,
});
const workspaceEnabled = application.capabilities.workspaces.enabled;

return {
/** Adds a new item to the history. */
Expand All @@ -81,10 +84,16 @@ export class RecentlyAccessedService {
},

/** Gets the current array of history items. */
get: () => history.get(),
get: () => history.get().filter((item) => !!item.workspaceId),
Copy link
Collaborator

Choose a reason for hiding this comment

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

will this impact workspace disabled case? I think we should check workspaceEnabled flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I forgot it here, thx for reminding!


/** Gets an observable of the current array of history items. */
get$: () => history.get$(),
get$: () => {
return history.get$().pipe(
map((items) => {
return items.filter((item) => (workspaceEnabled ? !!item.workspaceId : true));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some UT to test this filtering logic?

})
);
},
};
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/core/public/chrome/ui/header/nav_link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,17 @@ export function createRecentNavLink(
recentLink: ChromeRecentlyAccessedHistoryItem,
navLinks: ChromeNavLink[],
basePath: HttpStart['basePath'],
navigateToUrl: InternalApplicationStart['navigateToUrl']
navigateToUrl: InternalApplicationStart['navigateToUrl'],
workspaceEnabled: boolean = true
Copy link
Member

Choose a reason for hiding this comment

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

The default value should be false I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it should be false.

): RecentNavLink {
const { link, label, workspaceId } = recentLink;
const href = relativeToAbsolute(
basePath.prepend(formatUrlWithWorkspaceId(link, workspaceId || '', basePath), {
withoutClientBasePath: true,
})
basePath.prepend(
formatUrlWithWorkspaceId(link, workspaceEnabled ? workspaceId || '' : '', basePath),
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic looks a little bit strange to me, I think we don't need to call formatUrlWithWorkspaceId if workspaceEnabled equal false. How about refactor with below code?

Suggested change
formatUrlWithWorkspaceId(link, workspaceEnabled ? workspaceId || '' : '', basePath),
workspaceEnabled ? formatUrlWithWorkspaceId(link, workspaceId || '', basePath) : link,

By the way, can we add some unit tests about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

{
withoutClientBasePath: true,
}
)
);
const navLink = navLinks.find((nl) => href.startsWith(nl.baseUrl));
let titleAndAriaLabel = label;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,9 @@ export const RecentWork = (props: { core: CoreStart; workspaceEnabled?: boolean
recentAccessItem,
navLinks,
core.http.basePath,
core.application.navigateToUrl
core.application.navigateToUrl,
!!workspaceEnabled
Copy link
Member

Choose a reason for hiding this comment

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

Shall we do a search for the places that are using createRecentNavLink? IIRC there are at least 2 places:

  • recent menu in breadcrumb.
  • recent works in home page.

);

content = (
<EuiCard
title={
Expand Down