Skip to content

Commit

Permalink
Copilot Chat: Reduce calls to MSAL getIdToken (#1612)
Browse files Browse the repository at this point in the history
### Motivation and Context
<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
Many Copilot Chat app React components call getActiveAccount() to get
the logged in user id/name as one of the setup steps. This will trigger
MSAL to retrieve ID tokens from the cache, resulting in unnecessary
calls to getIdToken as shown below.


![24cfecf0-4896-4561-98ba-3b65a9947c5a](https://github.com/microsoft/semantic-kernel/assets/12570346/f726cf97-fdf3-4913-a5a7-2f13a85cc04e)

### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
Cache the logged in user id/name in the Redux store so that components
don't have to repeatedly call getActiveAccount().

### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows SK Contribution Guidelines
(https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [ ] The code follows the .NET coding conventions
(https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions)
verified with `dotnet format`
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Adrian Bonar <56417140+adrianwyatt@users.noreply.github.com>
Co-authored-by: Teresa Hoang <125500434+teresaqhoang@users.noreply.github.com>
Co-authored-by: Desmond Howard <dehoward@microsoft.com>
Co-authored-by: Aman Sachan <51973971+amsacha@users.noreply.github.com>
  • Loading branch information
5 people authored Jul 6, 2023
1 parent 91ccbc5 commit 1e1d590
Show file tree
Hide file tree
Showing 12 changed files with 79 additions and 71 deletions.
26 changes: 21 additions & 5 deletions samples/apps/copilot-chat-app/webapp/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import BackendProbe from './components/views/BackendProbe';
import { ChatView } from './components/views/ChatView';
import Loading from './components/views/Loading';
import { Login } from './components/views/Login';
import { AlertType } from './libs/models/AlertType';
import { useChat } from './libs/useChat';
import { useAppDispatch, useAppSelector } from './redux/app/hooks';
import { RootState } from './redux/app/store';
import { removeAlert } from './redux/features/app/appSlice';
import { setLoggedInUserId } from './redux/features/conversations/conversationsSlice';
import { addAlert, removeAlert, setActiveUserInfo } from './redux/features/app/appSlice';
import { CopilotChatTokens } from './styles';

export const useClasses = makeStyles({
Expand Down Expand Up @@ -64,14 +64,29 @@ const App: FC = () => {
const dispatch = useAppDispatch();

const { instance, inProgress } = useMsal();
const account = instance.getActiveAccount();
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);
const isAuthenticated = useIsAuthenticated();

const chat = useChat();

useEffect(() => {
if (isAuthenticated && account) {
dispatch(setLoggedInUserId(account.homeAccountId));
if (isAuthenticated) {
let isActiveUserInfoSet = activeUserInfo !== undefined;
if (!isActiveUserInfoSet) {
const account = instance.getActiveAccount();
if (!account) {
dispatch(addAlert({ type: AlertType.Error, message: 'Unable to get active logged in account.' }));
} else {
dispatch(
setActiveUserInfo({
id: account.homeAccountId,
email: account.username, // username in an AccountInfo object is the email address
username: account.name ?? account.username,
}),
);
}
isActiveUserInfoSet = true;
}

if (appState === AppState.LoadingChats) {
// Load all chats from memory
Expand All @@ -82,6 +97,7 @@ const App: FC = () => {
});
}
}

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [instance, inProgress, isAuthenticated, appState]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ interface ChatInputProps {
export const ChatInput: React.FC<ChatInputProps> = ({ isDraggingOver, onDragLeave, onSubmit }) => {
const classes = useClasses();
const { instance, inProgress } = useMsal();
const account = instance.getActiveAccount();
const chat = useChat();
const dispatch = useAppDispatch();
const [value, setValue] = React.useState('');
Expand All @@ -85,6 +84,7 @@ export const ChatInput: React.FC<ChatInputProps> = ({ isDraggingOver, onDragLeav
const [documentImporting, setDocumentImporting] = React.useState(false);
const documentFileRef = useRef<HTMLInputElement | null>(null);
const { conversations, selectedId } = useAppSelector((state: RootState) => state.conversations);
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);

React.useEffect(() => {
async function initSpeechRecognizer() {
Expand Down Expand Up @@ -189,7 +189,7 @@ export const ChatInput: React.FC<ChatInputProps> = ({ isDraggingOver, onDragLeav
}
// User is considered typing if the input is in focus
dispatch(
updateUserIsTyping({ userId: account?.homeAccountId, chatId: selectedId, isTyping: true }),
updateUserIsTyping({ userId: activeUserInfo?.id, chatId: selectedId, isTyping: true }),
);
}}
onChange={(_event, data) => {
Expand All @@ -209,7 +209,7 @@ export const ChatInput: React.FC<ChatInputProps> = ({ isDraggingOver, onDragLeav
onBlur={() => {
// User is considered not typing if the input is not in focus
dispatch(
updateUserIsTyping({ userId: account?.homeAccountId, chatId: selectedId, isTyping: false }),
updateUserIsTyping({ userId: activeUserInfo?.id, chatId: selectedId, isTyping: false }),
);
}}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft. All rights reserved.

import { useMsal } from '@azure/msal-react';
import { makeStyles, shorthands, tokens } from '@fluentui/react-components';
import debug from 'debug';
import React from 'react';
Expand Down Expand Up @@ -42,12 +41,11 @@ const useClasses = makeStyles({

export const ChatRoom: React.FC = () => {
const { conversations, selectedId } = useAppSelector((state: RootState) => state.conversations);
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);

const messages = conversations[selectedId].messages;
const classes = useClasses();

const { instance } = useMsal();
const account = instance.getActiveAccount();

const dispatch = useAppDispatch();
const scrollViewTargetRef = React.useRef<HTMLDivElement>(null);
const scrollTargetRef = React.useRef<HTMLDivElement>(null);
Expand Down Expand Up @@ -88,17 +86,13 @@ export const ChatRoom: React.FC = () => {
};
}, []);

if (!account) {
return null;
}

const handleSubmit = async (options: GetResponseOptions) => {
log('submitting user chat message');

const chatInput: IChatMessage = {
timestamp: new Date().getTime(),
userId: account.homeAccountId,
userName: account.name ?? account.username,
userId: activeUserInfo?.id as string,
userName: activeUserInfo?.username as string,
content: options.value,
type: options.messageType,
authorRole: AuthorRoles.User,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
// Copyright (c) Microsoft. All rights reserved.

import { useMsal } from '@azure/msal-react';
import React from 'react';
import { IChatUser } from '../../libs/models/ChatUser';
import { useAppSelector } from '../../redux/app/hooks';
import { RootState } from '../../redux/app/store';
import { TypingIndicatorRenderer } from './typing-indicator/TypingIndicatorRenderer';

export const ChatStatus: React.FC = () => {
const { instance } = useMsal();
const account = instance.getActiveAccount();
const { conversations, selectedId } = useAppSelector((state: RootState) => state.conversations);
const { users } = conversations[selectedId];
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);
const [typingUserList, setTypingUserList] = React.useState<IChatUser[]>([]);

React.useEffect(() => {
const checkAreTyping = () => {
const updatedTypingUsers: IChatUser[] = users.filter(
(chatUser: IChatUser) =>
chatUser.id !== account?.homeAccountId &&
chatUser.id !== activeUserInfo?.id &&
chatUser.isTyping,
);

setTypingUserList(updatedTypingUsers);
};
checkAreTyping();
}, [account?.homeAccountId, users]);
}, [activeUserInfo, users]);

return (
<TypingIndicatorRenderer isBotTyping={conversations[selectedId].isBotTyping} numberOfUsersTyping={typingUserList.length} />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft. All rights reserved.

import { useMsal } from '@azure/msal-react';
import { Persona, Text, makeStyles, mergeClasses, shorthands, tokens } from '@fluentui/react-components';
import React from 'react';
import { AuthorRoles, ChatMessageType, IChatMessage } from '../../../libs/models/ChatMessage';
Expand Down Expand Up @@ -69,13 +68,11 @@ interface ChatHistoryItemProps {
export const ChatHistoryItem: React.FC<ChatHistoryItemProps> = ({ message, getResponse, messageIndex }) => {
const classes = useClasses();

const { instance } = useMsal();
const account = instance.getActiveAccount();

const chat = useChat();
const { conversations, selectedId } = useAppSelector((state: RootState) => state.conversations);
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);

const isMe = message.authorRole === AuthorRoles.User && message.userId === account?.homeAccountId;
const isMe = message.authorRole === AuthorRoles.User && message.userId === activeUserInfo?.id;
const isBot = message.authorRole === AuthorRoles.Bot;
const user = chat.getChatUserById(message.userName, selectedId, conversations[selectedId].users);
const fullName = user?.fullName ?? message.userName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import {
tokens,
} from '@fluentui/react-components';
import { AuthHelper } from '../../libs/auth/AuthHelper';
import { resetState } from '../../redux/app/store';
import { useAppSelector } from '../../redux/app/hooks';
import { RootState, resetState } from '../../redux/app/store';

export const useClasses = makeStyles({
root: {
Expand All @@ -37,7 +38,7 @@ export const UserSettings: FC<IUserSettingsProps> = ({ setLoadingState }) => {
const classes = useClasses();
const { instance } = useMsal();

const account = instance.getActiveAccount();
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);

const onLogout = useCallback(() => {
setLoadingState();
Expand All @@ -51,8 +52,8 @@ export const UserSettings: FC<IUserSettingsProps> = ({ setLoadingState }) => {
{
<Avatar
className={classes.root}
key={account?.name ?? account?.username}
name={account?.name ?? account?.username}
key={activeUserInfo?.username}
name={activeUserInfo?.username}
size={28}
badge={{ status: 'available' }}
/>
Expand All @@ -62,8 +63,8 @@ export const UserSettings: FC<IUserSettingsProps> = ({ setLoadingState }) => {
<MenuList>
<MenuItem className={classes.persona}>
<Persona
name={account?.name ?? account?.username}
secondaryText={account?.username}
name={activeUserInfo?.username}
secondaryText={activeUserInfo?.email}
presence={{ status: 'available' }}
avatar={{ color: 'colorful' }}
/>
Expand Down
52 changes: 25 additions & 27 deletions samples/apps/copilot-chat-app/webapp/src/libs/useChat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,20 @@ export interface GetResponseOptions {
export const useChat = () => {
const dispatch = useAppDispatch();
const { instance, inProgress } = useMsal();
const account = instance.getActiveAccount();
const { conversations } = useAppSelector((state: RootState) => state.conversations);
const { activeUserInfo } = useAppSelector((state: RootState) => state.app);

const botService = new BotService(process.env.REACT_APP_BACKEND_URI as string);
const chatService = new ChatService(process.env.REACT_APP_BACKEND_URI as string);
const documentImportService = new DocumentImportService(process.env.REACT_APP_BACKEND_URI as string);

const botProfilePictures: string[] = [botIcon1, botIcon2, botIcon3, botIcon4, botIcon5];

const homeAccountId = account?.homeAccountId ?? '';
const emailAddress = account?.username ?? '';
const fullName = account?.name ?? emailAddress;
const userId = activeUserInfo?.id ?? '';
const fullName = activeUserInfo?.username ?? '';
const emailAddress = activeUserInfo?.email ?? '';
const loggedInUser: IChatUser = {
id: homeAccountId,
id: userId,
fullName,
emailAddress,
photo: undefined, // TODO: Make call to Graph /me endpoint to load photo
Expand All @@ -72,24 +72,22 @@ export const useChat = () => {
const chatTitle = `Copilot @ ${new Date().toLocaleString()}`;
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
await chatService
.createChatAsync(homeAccountId, chatTitle, accessToken)
.then(async (result: IChatSession) => {
const chatMessages = await chatService.getChatMessagesAsync(result.id, 0, 1, accessToken);

const newChat: ChatState = {
id: result.id,
title: result.title,
messages: chatMessages,
users: [loggedInUser],
botProfilePicture: getBotProfilePicture(Object.keys(conversations).length),
input: '',
isBotTyping: false,
};
await chatService.createChatAsync(userId, chatTitle, accessToken).then(async (result: IChatSession) => {
const chatMessages = await chatService.getChatMessagesAsync(result.id, 0, 1, accessToken);

const newChat: ChatState = {
id: result.id,
title: result.title,
messages: chatMessages,
users: [loggedInUser],
botProfilePicture: getBotProfilePicture(Object.keys(conversations).length),
input: '',
isBotTyping: false,
};

dispatch(addConversation(newChat));
return newChat.id;
});
dispatch(addConversation(newChat));
return newChat.id;
});
} catch (e: any) {
const errorMessage = `Unable to create new chat. Details: ${getErrorDetails(e)}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
Expand All @@ -102,7 +100,7 @@ export const useChat = () => {
variables: [
{
key: 'userId',
value: homeAccountId,
value: userId,
},
{
key: 'userName',
Expand Down Expand Up @@ -138,7 +136,7 @@ export const useChat = () => {
const loadChats = async () => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
const chatSessions = await chatService.getAllChatsAsync(homeAccountId, accessToken);
const chatSessions = await chatService.getAllChatsAsync(userId, accessToken);

if (chatSessions.length > 0) {
const loadedConversations: Conversations = {};
Expand Down Expand Up @@ -188,7 +186,7 @@ export const useChat = () => {
const uploadBot = async (bot: Bot) => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
botService
.uploadAsync(bot, homeAccountId, accessToken)
.uploadAsync(bot, userId, accessToken)
.then(async (chatSession: IChatSession) => {
const chatMessages = await chatService.getChatMessagesAsync(chatSession.id, 0, 100, accessToken);

Expand Down Expand Up @@ -230,7 +228,7 @@ export const useChat = () => {
const importDocument = async (chatId: string, file: File) => {
try {
await documentImportService.importDocumentAsync(
homeAccountId,
userId,
fullName,
chatId,
file,
Expand All @@ -254,7 +252,7 @@ export const useChat = () => {
const joinChat = async (chatId: string) => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
await chatService.joinChatAsync(homeAccountId, chatId, accessToken).then(async (result: IChatSession) => {
await chatService.joinChatAsync(userId, chatId, accessToken).then(async (result: IChatSession) => {
// Get chat messages
const chatMessages = await chatService.getChatMessagesAsync(result.id, 0, 100, accessToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ import { AlertType } from '../../../libs/models/AlertType';

export interface AppState {
alerts: Alert[];
activeUserInfo?: ActiveUserInfo;
}

export interface ActiveUserInfo {
id: string;
email: string;
username: string;
}

export interface Alert {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import { createSlice, PayloadAction } from '@reduxjs/toolkit';
import { AlertType } from '../../../libs/models/AlertType';
import { Alert, AppState } from './AppState';
import { ActiveUserInfo, Alert, AppState } from './AppState';

const initialState: AppState = {
alerts: [
Expand Down Expand Up @@ -30,9 +30,12 @@ export const appSlice = createSlice({
removeAlert: (state: AppState, action: PayloadAction<number>) => {
state.alerts.splice(action.payload, 1);
},
setActiveUserInfo: (state: AppState, action: PayloadAction<ActiveUserInfo>) => {
state.activeUserInfo = action.payload;
},
},
});

export const { addAlert, removeAlert, setAlerts } = appSlice.actions;
export const { addAlert, removeAlert, setAlerts, setActiveUserInfo } = appSlice.actions;

export default appSlice.reducer;
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@ export type Conversations = Record<string, ChatState>;
export interface ConversationsState {
conversations: Conversations;
selectedId: string;
loggedInUserId: string;
}

export const initialState: ConversationsState = {
conversations: {},
selectedId: '',
loggedInUserId: '',
};

export interface UpdateConversationPayload {
Expand Down
Loading

0 comments on commit 1e1d590

Please sign in to comment.