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

Expiration check #46

Closed
wants to merge 2 commits into from
Closed

Expiration check #46

wants to merge 2 commits into from

Conversation

peterszerzo
Copy link
Collaborator

@peterszerzo peterszerzo commented Apr 5, 2024

If the conversation expired since the user interacted with the bot last, reset to a new ID.

@peterszerzo peterszerzo requested a review from jakub-nlx as a code owner April 5, 2024 07:13
@peterszerzo peterszerzo requested review from ndrppnc and sam-trost April 5, 2024 07:14
@peterszerzo peterszerzo marked this pull request as draft April 5, 2024 07:18
@peterszerzo
Copy link
Collaborator Author

Moved to drafts because

export const retrieveSession = (storeIn: StorageType): SessionData | null => {
try {
const storage =
storeIn === "sessionStorage" ? sessionStorage : localStorage;
// initial eslint integration
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions, @typescript-eslint/prefer-nullish-coalescing
const data = JSON.parse(storage.getItem(storageKey) || "");
const responses: Response[] | undefined = data?.responses;
const conversationId: string | undefined = data?.conversationId;
// initial eslint integration
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
if (responses) {
let expirationTimestamp: number | undefined;
responses.forEach((response) => {
// initial eslint integration
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
if (response.type === "bot" && response.payload.expirationTimestamp) {
expirationTimestamp = response.payload.expirationTimestamp;
}
});
// initial eslint integration
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
if (!expirationTimestamp || new Date().getTime() < expirationTimestamp) {
return { responses, conversationId };
} else {
return { responses };
}
}
return null;
} catch (err) {
return null;
}
};
is already handling this.

Comment on lines +233 to +222
return lastExpirationResponse?.type === "bot"
? lastExpirationResponse.payload.expirationTimestamp
: undefined;
Copy link
Collaborator

@michaelglass michaelglass Apr 5, 2024

Choose a reason for hiding this comment

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

minor nit (and less precise but)

Suggested change
return lastExpirationResponse?.type === "bot"
? lastExpirationResponse.payload.expirationTimestamp
: undefined;
return lastExpirationResponse?.payload.expirationTimestamp;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would rather do more precise and a bit clearer on the data structure.

new Date().getTime() > lastExpirationTimestamp
) {
setState({
conversationId: uuid(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious how this works for a multimodal when someone is already on the phone. Should this fail hard instead of resetting with a new conversation ID? What's the UX here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This package does not cover multimodal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah! whoops

@peterszerzo peterszerzo marked this pull request as ready for review April 9, 2024 12:26
typeof lastExpirationTimestamp === "number" &&
new Date().getTime() > lastExpirationTimestamp
) {
setState({
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 this seems like it should raise an exception or signal in some way that there's a new conversation starting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@michaelglass totally with you there, we are discussing the behind-the-scenes way to do this, basically any 'your conversation has expired' type of text content needs to be controlled inside the platform, so you can imagine some kind of explicit special message passing will happen here, but later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a conversationEnded subscription thinger here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sam-trost that sounds like a bit of a change in direction, I would rather have a discussion about it at some point this week.

Copy link
Collaborator

@michaelglass michaelglass left a comment

Choose a reason for hiding this comment

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

the code here looks fine. I'm not sure if it's the right approach.

@peterszerzo peterszerzo mentioned this pull request Jul 9, 2024
@peterszerzo
Copy link
Collaborator Author

Abandoning this in favor of #124

@peterszerzo peterszerzo deleted the expiration-check branch July 23, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants