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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
685e3ef
Expiration check
peterszerzo Apr 5, 2024
af4e47e
Remove expiration-related logic from session retrieval
peterszerzo Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion packages/chat-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fetch from "isomorphic-fetch";
import ReconnectingWebSocket from "reconnecting-websocket";
import { equals, adjust, omit } from "ramda";
import { equals, adjust, omit, findLast } from "ramda";
import { v4 as uuid } from "uuid";

// Bot response
Expand Down Expand Up @@ -208,6 +208,20 @@ export const shouldReinitialize = (
);
};

const getLastExpirationTimestamp = (
responses: Response[],
): number | undefined => {
const lastExpirationResponse = findLast(
(response) =>
response.type === "bot" &&
typeof response.payload.expirationTimestamp === "number",
responses,
);
return lastExpirationResponse?.type === "bot"
? lastExpirationResponse.payload.expirationTimestamp
: undefined;
Comment on lines +220 to +222
Copy link
Collaborator

@michaelglass michaelglass Apr 5, 2024 •
edited
Loading

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.

};

export const createConversation = (config: Config): ConversationHandler => {
let socket: ReconnectingWebSocket | undefined;

Expand Down Expand Up @@ -303,6 +317,15 @@ export const createConversation = (config: Config): ConversationHandler => {
null;

const sendToBot = async (body: BotRequest): Promise<void> => {
const lastExpirationTimestamp = getLastExpirationTimestamp(state.responses);
if (
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.

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

});
}
const bodyWithContext = {
userId: state.userId,
conversationId: state.conversationId,
Expand Down
24 changes: 3 additions & 21 deletions packages/chat-widget/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,29 +258,11 @@ 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 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 };
}
if (responses != null) {
return { responses, conversationId };
}
return null;
} catch (err) {
Expand Down