Add sidecar service

[raintygao]

Description

Support a new layout service called sidecar in dashboard, so that external plugin can use this service as its container. This flyout supports three modes and resizable.

Issues Resolved

https://github.com/opensearch-project/OpenSearch-Dashboards/issues/5620

Screenshot

Testing the changes

Check List

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

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 91.50943% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 67.21%. Comparing base (31e8481) to head (23223c6).

:exclamation: Current head 23223c6 differs from pull request most recent head 988d1bc. Consider uploading reports for the commit 988d1bc to get more accurate results

Files	Patch %	Lines
src/core/public/overlays/overlay_service.ts	0.00%	4 Missing :warning:
...re/public/overlays/sidecar/sidecar_service.mock.ts	62.50%	3 Missing :warning:
...c/core/public/overlays/sidecar/sidecar_service.tsx	94.28%	0 Missing and 2 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5920      +/-   ##
==========================================
+ Coverage   67.20%   67.21%   +0.01%     
==========================================
  Files        3328     3333       +5     
  Lines       64461    64553      +92     
  Branches    10376    10392      +16     
==========================================
+ Hits        43318    43391      +73     
- Misses      18610    18628      +18     
- Partials     2533     2534       +1     

Flag	Coverage Δ
Linux_1	31.69% <11.32%> (-0.06%)	:arrow_down:
Linux_2	55.45% <91.26%> (+0.15%)	:arrow_up:
Linux_3	44.65% <11.32%> (-0.13%)	:arrow_down:
Linux_4	34.99% <13.20%> (-0.07%)	:arrow_down:
Windows_1	31.72% <11.32%> (-0.08%)	:arrow_down:
Windows_2	55.41% <91.26%> (+0.15%)	:arrow_up:
Windows_3	44.65% <11.32%> (-0.13%)	:arrow_down:
Windows_4	34.99% <13.20%> (-0.07%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[kavilla]

Pretty cool.

Will check out the implementation. In the meantime I see there are some snapshots are not updated for tests. Also wanted to check. Is it intentional that the sidecar pushes the elements over when it is mounted?

[raintygao]

Thank you. @kavilla

In the meantime I see there are some snapshots are not updated for tests. Also wanted to check.

I will do some updates to snapshots.

Is it intentional that the sidecar pushes the elements over when it is mounted?

Yes. When plugin calls open function, the sidecar will be mounted with the paddingSize in passed parameter to push elements over.

[kgcreative]

We need the sidecar and a fly out to be able to exist together

[ashwin-pc]

@kgcreative but cant the two be the same OUI component? They share a lot of features. Dont you think we can combine the two into one interface that is flexible as either? Here are some similarities.

Flyout	Sidecar
Lives on document root	Lives on document root
Opens from the side	opens from sides and bottm
Push and overlay	Only push
wants resize	Supports resize
...	...

And even of they cant be the same component, the raw component should be bult in OUI and consumed hee like the flyout and Modal. Neither are built in OSD.

[raintygao]

@ashwin-pc Although sidecar and flyout have some similarities, they are actually totally different. And these two components should be able to exist at the same time without affecting each other. There are also several technical reasons that this should be implemented in OSD:

1. The push effect of flyout and sidecar are two completely different approaches. Push of flyout is implemented by adding padding attributes to body DOM, but this can only affect app-wrapper on the OSD and cannot affect the chrome header after testing. However, sidecar needs to push the entire OSD. Due to the structure of the OSD DOM, the component must affect on header and app-wrapper separately to implement the push effect of the entire OSD, which is unable for an external component. Meanwhile I think there are certain risks if rashly modifying the DOM structure of OSD.
2. There are many customization requirements in the sidecar, such as switching directions in real time without unmounting, the hidden function not only needs to hide the sidecar's DOM but also cancel the push effect of the header and app-wrapper on the OSD. I think component in generic UI library component should not support these custom functions.
3. Since implementing push effects and resizable involves communication between document root and OSD, observable is used. Performance may not be easy to control if using external components.

I think it's reasonable for OUI to support a single resizable container like some others, but if it involves interacting with entire OSD and some customization requirements, i think this should be handled internally by the OSD, this should be more of a specific component inside the OSD. It may be confused and more difficult to put it as a component in generic UI library.

[ashwin-pc]

@raintygao Okay i see the difference. I no longer think that the two need to be the same component, but I do still think that the component needs to come from OUI. So my hesitation with the approach is that we are defining a lot of standard UI interaction patterns in OSD for resizing that should ideally be taken care of by OUI.

As for the mount point, I think the way the flyout does it is not ideal but doesnt mean we have to do it the same way. You can always pass a ref to a react component so that it can mount correctly. In this case the app container can be provided to an OUI Sidecar component and it can mount appropriately. All its states and combinations are also easier to test and validate while also abstracting away the accessibility and ui interaction code from the OSD codebase. not sure what your performance concerns are but lets chat about it.

I think component in generic UI library component should not support these custom functions.

Yes. I expects similar separation of concerns similar to how the flyout or modal behaves. The UI Library is only responsible for the UI and resize properties. The state management and what to do once actions are clicked should all be handled by the sidecar service that you have here. It also makes testing these a lot easier since UI tests are quite different from the integ tests that the service needs.

[raintygao]

@ashwin-pc Thanks. I agree with you that some standard UI components should be imported from OUI, like resizable button, resizable flyouts. Once OUI support these components we can use them to replace raw implementation in OSD sidecar service.

Excluding some reused UI components above, I think the remaining content of sidecar service should be implemented in OSD instead of OUI because it is not a widely used service and currently only used by assistant plugin. Many important parts are handled in this service, such as how chrome header and app-wrapper respond respectively when the container is dragged to implement the overall push effect, some state management, some real-time configuration switching, display and hiding and many customization functions, etc.

[ashwin-pc]

+1 Can you work on adding these to OUI? Lemme know if you need any help contributing that change :)

[raintygao]

@ashwin-pc Sure. I don't know if there is a team support new features in OUI, if not, I can work on supporting this. Since I have ever not been working on OUI codebase, it may take some time. My team is rushing on releasing workspace feature on 2.13 so I may not have extra time to support this until 2.14 at the earliest.

Sidecar feature is a high property feature which was implemented in 2.12 and planned to be released in 2.13. I agree with you that it should be updated. I think what we discussed above is not about design or performance issues, but more like standards for code and component reuse, its priority may should not affect the release and feature delivery, especially my team don't have extra time currently, right?

So my suggestion is that we review and merge current implementation to ensure deliver on time and try to support new UI components in OUI from the next version. When completed, we will replace raw implementation in OSD. I think this is a balanced solution.

[ashwin-pc]

@raintygao following up on a feature after its release is always a hard pill to swallow since we have been burnt by that pill many times. It often ends up deprioritized over the next shiny thing we need to build, which is why we need to insist on higher standards the first time around. Even our own team struggles with this so its not like its specific to you.

Also i dont agree that this is purely a standards for code issue. Having the component defined within the service makes it a lot harder to test and validate that all front end pieces of the component for its various use cases and how does it react to all its properties visually. While the service is declared in OSD, there isnt a single use of it here either, making validating any breaking change to its UI even more difficult. This is why the separation exists.

While i wont block this feature because of it, do consider these risks when going ahead with this approach. Atleast try to separate the UI component clearly from the service in a way that the UI component can be independently tested and validated.

[ashwin-pc]

Speaking of testing, I see that you have not included any details in the testing section of the PR Description. Can you add details in there on how to test this change?

[raintygao]

Sure, i will update test cases and test sections soon. Sorry for the missing test part, i'm trying hard to make time for this.

In terms of code structure, this component is indeed defined within the service, but I think it is also the similar to other common UI components. I will also add tests for the UI components.

I really agree with you to insist on higher standards. In this case I think it is not a flaw, it is more like a common processes: Found a widely used component containing UI patterns can be reused -> Add in the component library -> Replace the native implementation with component imported from library. I prefer to divide it into enhancement or refactor. Of course, I have promised that I will follow up and update it in OUI after release.

[raintygao]

src/core/public/overlays/overlay_service.ts 0.00% 4 Missing ⚠️ ...re/public/overlays/sidecar/sidecar_service.mock.ts 62.50% 3 Missing ⚠️

These two are originally missing and file to create mock for test.

[raintygao]

Hi @ashwin-pc @kavilla, most tests for this PR have been updated, please feel free to review. I will also try to add some cypress tests in functional test repo to make this more complete.

[raintygao]

Hi Kevin @kgcreative , as I discussed with Ashwin above, there may exist some components in this PR that we can potentially support in OUI for code reuse in future, he replied that he would not block this PR because of this and I have promised him that I would try to follow up this in OUI when I have enough bandwidth in the future. So I think we could try to move forward this PR and release it as planned in 2.13, and try to follow up some updates in OUI in future. Do you have any concerns?

[kgcreative]

Hi Kevin @kgcreative , as I discussed with Ashwin above, there may exist some components in this PR that we can potentially support in OUI for code reuse in future, he replied that he would not block this PR because of this and I have promised him that I would try to follow up this in OUI when I have enough bandwidth in the future. So I think we could try to move forward this PR and release it as planned in 2.13, and try to follow up some updates in OUI in future. Do you have any concerns?

As long as @ashwin-pc is on board, I'm supportive of this approach. This will unblock, and we can then make sure we can move forward with generalizing for OUI. I also agree that there may be some enhancements to flyout that can be made based on this work (For example, being able to resize flyout)

[ruanyl]

@raintygao I suggest to create an issue to track the future enhancement, specifically for the resizable button component. It's crucial to avoid duplicating code between OUI and OSD. Let's keep track of this in the issue for better visibility and planning.

So my suggestion is that we review and merge current implementation to ensure deliver on time and try to support new UI components in OUI from the next version.

As you suggested, please also start by creating an issue in OUI repo accordingly.

I noticed some failures in the GitHub workflows associated with your pull request. Kindly investigate these failures and address them.

[ashwin-pc]

Yeah I'm okay with a follow-up task for OUI. Can you create the issue and link it here. Can you do a few things for this PR:

1. Can you add functional tests for the PR?
2. Can you add steps to the "Testing the changes" section of the PR?

This should help reviewing the PR faster.

[raintygao]

@ruanyl I have created an issue in OUI to track this https://github.com/opensearch-project/oui/issues/1257. We could do component reuse. I can't be sure now that we can add a rezableButton component, at present it seems that rezableFlyout is more suitable, which is one of the reasons why I said that we need to follow up later, because we need to take some time to research on what components and how to design in OUI. Once we support in OUI, we could start to replace in OSD.

Github checks failure is not related to my PR, you could do a check on failed reasons. All flows are successful before I rebase main.

[raintygao]

Yeah I'm okay with a follow-up task for OUI. Can you create the issue and link it here. Can you do a few things for this PR:

1. Can you add functional tests for the PR?
2. Can you add steps to the "Testing the changes" section of the PR?

This should help reviewing the PR faster.

@ashwin-pc

1. Functional tests have been added. https://github.com/opensearch-project/opensearch-dashboards-functional-test/pull/1141
2. Updated in description section of this PR.

[raintygao]

The only one failed check is fixed in https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6119.

[raintygao]

There are many UI changes happening in this PR within core directory, which should be moved to OUI, and I find the following issue has been raised: opensearch-project/oui#1257 . It would be great that all the files under directory src/core/public/overlays/sidecar/components can be moved to OUI as standard components. Given the fact that @kgcreative wants a new sidecar mode, which can push the OSD instead of overlapping on the OSD content, to be available for all the plugins, current implementation is a trade-off for the tight 2.13 release.

I am giving the approval as I do not see blocker issues on the code change itself, but for the code design, I'd like to follow @ashwin-pc @kavilla 's opinion.

Thank you very much for your review and approval.

For now I will ensure release as scheduled first and follow up with component migration for enhancement later. I think this part needs some time to research and design, so I have created the issue https://github.com/opensearch-project/oui/issues/1257 to track and left some thoughts on it. I will share more thoughts and design on it with your guys about this part later.

[raintygao]

@SuZhou-Joe @ruanyl I just rebased main to resolve conflicts in changelog, and your approvals were dismissed automatically. Could you approve again?

[opensearch-trigger-bot[bot]]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5920-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c2c4f53f140a655705e0e8830ea14e8d958a8de9
# Push it to GitHub
git push --set-upstream origin backport/backport-5920-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5920-to-2.x.