chat(feat): implement plugin configuration controls for chat and context_provider plugins #10639
Conversation
…ext_provider plugins Fixed chat.enabled to properly disable plugin when set to false or when agUiUrl is missing Added contextProvider.enabled with server-side validation and dependency injection pattern Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh
requested review from
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,
huyaboo,
angle943,
Maosaic,
wanglam,
d-buckner,
raintygao and
ps48
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10639 +/- ##
==========================================
- Coverage 60.18% 60.17% -0.01%
==========================================
Files 4428 4428
Lines 118504 118519 +15
Branches 19456 19460 +4
==========================================
- Hits 71319 71318 -1
- Misses 42269 42279 +10
- Partials 4916 4922 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Im generally okay with this approach, but can you have @Maosaic review the prop drilling of the services and if that impacts the page performance.
Maybe we can skip moving the code to an optional plugin.I dont like exposing hooks as a part of the service. The context framework plugin can be bundled with the code, just ignored in all codepaths except explore since it is not imported anywhere else
| const ExplorePageContextProvider: React.FC<{ | ||
| children: React.ReactNode; | ||
| }> = ({ children }) => { | ||
| services: ExploreServices; | ||
| }> = ({ children, services }) => { | ||
| const usePageContext = services.contextProvider?.hooks?.usePageContext || (() => {}); | ||
|
|
There was a problem hiding this comment.
we sould only pass usePageContext into ExplorePageContextProvider to minimal rerender.
Also, we are not creating any context here, is this intended? Why we are calling it ContextProvider?
There was a problem hiding this comment.
I could pass usePageContext into that. But I don't think it would causes any re-render as it is only used once in the component and not passed to child components as props. Also the usePageContext hook itself handles the function internally.
There was a problem hiding this comment.
The ExplorePageContextProvider component is not creating a React Context Instead, it just uses the hook which can automatically capture page context from URL state. Basically it is to convert the URL state to the format needed. I could re-name it as ExplorePageContextRegister.
| scrollToTop={scrollToTop} | ||
| onAddColumn={onAddColumn} | ||
| onRemoveColumn={onRemoveColumn} | ||
| services={services} |
There was a problem hiding this comment.
Why drilling services prop? can we use useOpenSearchDashboards<ExploreServices>() in child component when we need services`?
|
Should we split the changes that consuming context_provider plugin in Explore into a different PR? |
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh
force-pushed
the
chat-dependency-injection
branch
from
10f4982 to
290cee0
Compare
Description
We want to disable contextProvider and chat completely from browser and server side when no explore or when not config/ Need to use contextProvider as optionalPlugins not requiredBundles. Otherwise contextProvider will be loaded into browser. In this PR we change to use dependency injection for contextProvider. The challenge is that React hooks can't be passed through dependency injection in the traditional way because they need to be called at the component level and follow the rules of hooks. Therefore, we need to update the contextProvider plugin to 1) expose its hooks through the plugin's start interface, so they can be accessed via dependency injection 2) add a server side. Otherwise no way to read configuration from
opensearch_dashboards.ymlso plugin always loaded in browser regardless of settings. Also without server we could not useoptionalPluginspattern (requires server component). No change on how we use the hooks, only change how we import them.Both server side and browser:
No chat when explore is enabled but chat.enabled is false or no chat.agUiUrl.
No contextProvider when explore is enabled but contextProvider.enable is false
Setup chat when explore is enabled and both chat.enabled and chat.agUiUrl are configed.
Setup contextProvider when explore is enabled and contextProvider.enable is true
No chat or contextProvider when no explore
Issues Resolved
NA
Changelog
Check List
yarn test:jestyarn test:jest_integration