Brand Concierge Component #1391
Conversation
packages/core/src/components/BrandConcierge/createSendConversationEvent.js
Outdated
Show resolved
Hide resolved
packages/core/src/components/BrandConcierge/createSendConversationEvent.js
Show resolved
Hide resolved
packages/core/src/components/BrandConcierge/createSendConversationServiceRequest.js
Show resolved
Hide resolved
packages/core/test/unit/specs/components/Personalization/topLevel/buildAlloy.js
Outdated
Show resolved
Hide resolved
jonsnyder
left a comment
jonsnyder
left a comment
There was a problem hiding this comment.
My review is not complete, but I wanted to give you these comments before I start some meetings. I will schedule a check-in to go over these issues.
| }); | ||
| } | ||
|
|
||
| const ecid = decodeKndctrCookie(); |
There was a problem hiding this comment.
Why are we explicitly including the ECID? The conversation backend should be able to read the ECID cookie either via the first party cookie or via the state section in the request for third-party data collection. We'd like to keep this protocol as close to Konductor as possible.
Can we also follow the established cookie protocol for the session_id cookie? i.e. conversation backend returning a session id when called without the cookie? Or does the client need to create the session id?
There was a problem hiding this comment.
We need the sessionId to be in queryParam for proper session stickiness.
There was a problem hiding this comment.
But why are we explicitly including the ECID for the event? Is it for third party collection domains?
There was a problem hiding this comment.
I've added the cookie in the meta.state too, but for now our BC runtime is looking for identityMap ECID.
| return consent.awaitConsent().then(() => { | ||
| return lifecycle | ||
| .onBeforeEvent({ | ||
| event, |
There was a problem hiding this comment.
Calling onBeforeEvent here may cause problems for the other components. We need to audit each use and make sure it is appropriate. Off the top of my head I know that this will cause problems with personalization. Consider if "sendConversationEvent" is called as the first event of a page load. In this case, the personalization component would add requests for personalization to the payload, and then not request personalization in any subsequent events. We could add a flag or type parameter to the onBeforeEvent params.
There was a problem hiding this comment.
@jonsnyder we need onBeforeEvent to ensure we have all the right context data being added to XDM. I am not sure why there is a dependency between Personalization and Brand Concierge. As far as I can tell each component should be independent and as long as no component overrides the event details of the other component we should be good.
If there are some unintended dependencies caused by onBeforeEvent usage in Personalization component, then Personalization component should be adjusted accordingly.
As an AEP Web SDK user I would be very surprised if am invoking sendConversationEvent and this somehow affects personalization functionality. If customer needs personalization after a sendConversationEvent has been fired, he can always use sendEvent with query -> personalization.
Adding a flag seems we are trying to hide a design issue, instead of addressing the real problem.
packages/core/src/components/BrandConcierge/createSendConversationServiceRequest.js
Show resolved
Hide resolved
packages/core/src/components/BrandConcierge/createSendConversationEvent.js
Show resolved
Hide resolved
|
|
||
| const streamParser = createStreamParser(); | ||
|
|
||
| streamParser(response.body, onStreamResponseCallback); |
There was a problem hiding this comment.
We should have a discussion on the correct protocol to use. Florentine mentioned that Konductor has implemented a streaming protocol already for mobile. Perhaps we should use that? The beauty of using EventSource is that it can resume if the connection is dropped while responses are streaming. I think the best way to do it would be to trigger a LLM call on a POST event, but then have part of the handle be a url to the rest of the response that can be retrieved via an eventSource enabled URL. In this way you actually could have the brand concierge service be an upstream that starts the stream, and then your BC backend would just serve Event Source get responses.
|
@jonsnyder @carterworks please take another look, I have addressed all feedback and added unit and functional tests. Thank you |
carterworks
left a comment
carterworks
left a comment
There was a problem hiding this comment.
I don't have any major concerns at this point. Let's see what Jon has to say.
…1405) * collect sources * fix page --------- Co-authored-by: Nina Ciocanu <[email protected]>
Description
Related Issue
Motivation and Context
Screenshots (if appropriate):
Checklist: