feat: real integration with Experience Shell #43
Conversation
askayastha22
changed the title
askayastha22
changed the title
| @@ -66,7 +66,7 @@ export function GenerateButton() { | |||
|
|
|||
| const generateResults = useCallback(async () => { | |||
| const finalPrompt = renderPrompt(prompt, parameters); | |||
| const { queryId, response } = await firefallService.complete(finalPrompt, temperature, imsToken); | |||
| const { queryId, response } = await firefallService.complete(finalPrompt, temperature, user.imsToken); | |||
There was a problem hiding this comment.
The IMS token should be injected into the service in a constructor. This way, we won't even touch this file.
There was a problem hiding this comment.
The EXC Shell triggers a refresh token when there is 20% duration left. A configuration event is sent to the iframe. The app could also listen directly to user.on('change:imsToken', () => {})
If we move this in a constructor we need to re-initialize firefall service when token changes.
There was a problem hiding this comment.
I just realized that any configuration change will recreate the FirefallService due to shellEventsHandler being called.
web-src/src/components/ResultCard.js
Outdated
| @@ -127,7 +127,7 @@ export function ResultCard({ result, ...props }) { | |||
| const saveSession = useSaveSession(); | |||
|
|
|||
| const sendFeedback = useCallback((sentiment) => { | |||
| firefallService.feedback(result.resultId, sentiment, imsToken) | |||
| firefallService.feedback(result.resultId, sentiment, user.imsToken) | |||
There was a problem hiding this comment.
Please pass the token to the service in its constructor.
|
|
||
| export const ShellAuthContext = createContext({}); | ||
|
|
||
| export const ShellAuthProvider = ({ children }) => { |
There was a problem hiding this comment.
I would suggest merging this provider with the Application Provider, as auth data is not required on its own anywhere in the app. It is only needed to construct the services.
There was a problem hiding this comment.
I considered separating the Auth context too, as it makes things easier to read and follow in the code.
There was a problem hiding this comment.
In my mental models, each app has a model but:
- Models could also be split into classes to isolate the concerns, simplify testing.
- A single large context that holds all the state for the entire app could lead to unnecessary re-renders.
- Smaller, more focused contexts can be more easily reused across different parts of our app, especially as we change the app based on our learnings.
- Managing the state of a large, monolithic context can become cumbersome and error-prone.
There was a problem hiding this comment.
I considered separating the Auth context too
Separating contexts doesn't necessarily mean separating the providers. The context itself has a modular structure. React does not care if the context object has only one root or many. I would suggest that we at least move the Shell provider above the Application Context.
cc @ddragosd @askayastha22
There was a problem hiding this comment.
@vtsaplin The Unified Shell config is set in ApplicationProvider and read in ShellAuthProvider. Is moving ShellAuthProvider above ApplicationProvider okay?
There was a problem hiding this comment.
@vtsaplin let's do this change in a separate PR to unblock the Shell testing
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: