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

[Workspace] Make url stateful in hash with workspace id #5984

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Feb 29, 2024

Description

In order to let users switch to specific workspace when paste a deep link from other users, we need to make url stateful with workspace info. This PR mainly introduce the capability to keep the workspace Id in url consistent with the workspace id in memory so that user can feel free to copy the url and share to others.

Issues Resolved

#6015

Screenshot

20240301200100457

Testing the changes

  • Using the branch to bootstrap
  • Enable workspace by adding workspace.enabled to opensearch_dashboards.yml file
  • Start OSD
  • Navigate to any page, append with hash #/?_w=foo
  • Jump to any other pages by clicking nav on left navigation
  • You will find _w params will always be there, making the url shareable.

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

@SuZhou-Joe SuZhou-Joe force-pushed the feature/stateful_url_hash_official_pr branch from 46623fd to c29af53 Compare February 29, 2024 05:45
@SuZhou-Joe SuZhou-Joe changed the title Make url stateful in hash [Workspace] Make url stateful in hash with workspace id Feb 29, 2024
@ashwin-pc ashwin-pc assigned kavilla and AMoo-Miki and unassigned kavilla Feb 29, 2024
@SuZhou-Joe
Copy link
Member Author

@kavilla Could you please take a look on this PR as you have enough context on both the issue and the related RFC?

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.15%. Comparing base (70adcc9) to head (1392a65).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5984      +/-   ##
==========================================
+ Coverage   67.13%   67.15%   +0.01%     
==========================================
  Files        3318     3318              
  Lines       64021    64021              
  Branches    10256    10256              
==========================================
+ Hits        42981    42991      +10     
+ Misses      18548    18537      -11     
- Partials     2492     2493       +1     
Flag Coverage Δ
Linux_1 31.63% <ø> (+<0.01%) ⬆️
Linux_2 55.20% <ø> (ø)
Linux_3 ?
Linux_4 35.16% <ø> (+<0.01%) ⬆️
Windows_1 31.68% <ø> (+0.02%) ⬆️
Windows_2 55.17% <ø> (ø)
Windows_3 44.60% <ø> (+<0.01%) ⬆️
Windows_4 35.16% <ø> (+<0.01%) ⬆️

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.

src/plugins/workspace/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/workspace/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/workspace/public/plugin.ts Show resolved Hide resolved
private hashChangeHandler = async () => {
if (this.shouldPatchUrl()) {
const workspaceId = await this.getWorkpsaceId();
this.URLChange$.next(this.getPatchedUrl(window.location.href, workspaceId));
Copy link
Member

Choose a reason for hiding this comment

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

getWorkpsaceId could return empty string '', what's the consequence if it's empty string? I guess that should remove the workspace from the url?

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Mar 6, 2024

Choose a reason for hiding this comment

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

Yes, in that case we will remove the _w params from url hash.

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.

Hmm this has an issue. It does not hook into the default routing API's of the app. This has some risks, e.g. opening links in a new tab to name just the first that comes to mind. Take a look at Discover. Go to discover, expand a row, click on view surrounding documents. This will not work there since the workspace is not carried over. This is just one place. Another way this can break is if a full page navigation occurs. This should not happen if the app developer uses the navigateToUrl or naigateToApp api, but not sure if all plugins actually follow that. We almost didnt use them for the breadcrumbs and i had to push back saying what a bad idea that was. But breadcrumbs are implemented by the plugin so even there its not garaunteed. Have you looked at https://ashwin-pc.github.io/OpenSearch-Dashboards/docs/index.html#/plugins/data_persistence?

Cant you reuse the _g key? That way no app needs to make a change since global query persistence was always a thing for the app. Or else the workspace features should not break an app if the key is missing when they transition from one page to the next.

Also this should definitely have a test. That way we can ensure that under all circumstances the workspace key is passed :)

Make url stateful in hash

Signed-off-by: SuZhou-Joe <[email protected]>
Signed-off-by: SuZhou-Joe <[email protected]>
@SuZhou-Joe SuZhou-Joe force-pushed the feature/stateful_url_hash_official_pr branch from bdc7fd5 to c9474ff Compare March 6, 2024 03:36
@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Mar 6, 2024

@ashwin-pc So grateful for the edge case.

  • For the edge case in Discover, I add a localStorage to reserve the current workspace so that the new-tab-link will have workspace params patched without doing any code changes. Even if we reuse the _g params Discover app will only honor the filters params. And the approach should solve the concern for breadcrumbs and hard refresh when navigating inside OSD.
  • For the test cases, sure I will add cypress test to cover the edge cases.

Signed-off-by: SuZhou-Joe <[email protected]>
@ashwin-pc
Copy link
Member

@SuZhou-Joe the edge cases I called is just one of them. What I'm asking is if you folks have explored the persistence service to see if that can be used as is or if can be extended instead of rolling out a custom solution. Let's try to build on what existing already since this is just the first edge case I came across in 5 mins. And the last time I tried to mess with routing, I was burnt badly 😅

If you explore it and decide this is still the best way that's fine too. Just want to make sure that we considered the options an made the right tradeoff.

@SuZhou-Joe
Copy link
Member Author

@ashwin-pc , we had a discussion on all the options we can choose from. This is the RFC issue(#5243).

@kavilla has some concerns on making the workspaceId into path as it may affect the OSD a lot, so we did a trade-off to make it in hash.

I am still free to accept advice on how we store the workspace id into url and look forward to your advice as you have much more knowledge on OSD router.

@SuZhou-Joe
Copy link
Member Author

Closed as we will use #6060.

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.

6 participants