Skip to content

Commit

Permalink
🐛 Fix connection error handling + uncaught exceptions (microsoft#134)
Browse files Browse the repository at this point in the history
### Motivation and Context

<!-- Thank you for your contribution to the copilot-chat 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.
-->
Fix connection errors in useChat and appSlice + fix red screens of death

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- Added an ID to Alerts in AppState, add logic to update connection
status in appSlice, and update SignalRMiddleware to add an ID to
connection alerts.
- Added more error handling to reduce red screens of death
- Move all getAccessToken calls into try catch blocks
- Added some missing details around Stepwise Planner

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [Contribution
Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
~~- [ ] All unit tests pass, and I have added new tests where possible~~
- [x] I didn't break anyone 😄
  • Loading branch information
teresaqhoang authored Aug 9, 2023
1 parent 0e3c204 commit 334fac9
Show file tree
Hide file tree
Showing 10 changed files with 91 additions and 27 deletions.
2 changes: 1 addition & 1 deletion webapi/Options/PlannerOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class MissingFunctionErrorOptions
public const string PropertyName = "Planner";

/// <summary>
/// Define if the planner must be Sequential or not.
/// The type of planner to used to create plan.
/// </summary>
[Required]
public PlanType Type { get; set; } = PlanType.Action;
Expand Down
1 change: 1 addition & 0 deletions webapi/appsettings.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
// - Set Planner:Type to "Action" to use the single-step ActionPlanner
// - Set Planner:Type to "Sequential" to enable the multi-step SequentialPlanner
// Note: SequentialPlanner works best with `gpt-4`. See the "Enabling Sequential Planner" section in webapi/README.md for configuration instructions.
// - Set Planner:Type to "Stepwise" to enable MRKL style planning
// - Set Planner:RelevancyThreshold to a decimal between 0 and 1.0.
//
"Planner": {
Expand Down
1 change: 1 addition & 0 deletions webapp/src/Constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export const Constants = {
app: {
name: 'Copilot',
updateCheckIntervalSeconds: 60 * 5,
CONNECTION_ALERT_ID: 'connection-alert',
},
msal: {
method: 'redirect', // 'redirect' | 'popup'
Expand Down
5 changes: 4 additions & 1 deletion webapp/src/checkEnv.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
/**
* Checks if all required environment variables are defined
* @returns {string[]} An array of missing environment variables
*/
export const getMissingEnvVariables = () => {
// Should be aligned with variables defined in .env.example
const envVariables = ['REACT_APP_BACKEND_URI', 'REACT_APP_AAD_AUTHORITY', 'REACT_APP_AAD_CLIENT_ID'];

const missingVariables = [];

for (const variable of envVariables) {
Expand Down
3 changes: 2 additions & 1 deletion webapp/src/components/token-usage/TokenUsageGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ const contrastColors = [
export const TokenUsageGraph: React.FC<ITokenUsageGraph> = ({ promptView, tokenUsage }) => {
const classes = useClasses();
const { conversations, selectedId } = useAppSelector((state: RootState) => state.conversations);
const loadingResponse = conversations[selectedId].botResponseStatus && Object.entries(tokenUsage).length === 0;
const loadingResponse =
selectedId !== '' && conversations[selectedId].botResponseStatus && Object.entries(tokenUsage).length === 0;

const responseGenerationView: TokenUsageView = {};
const memoryGenerationView: TokenUsageView = {};
Expand Down
4 changes: 3 additions & 1 deletion webapp/src/components/views/BackendProbe.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ const BackendProbe: FC<IData> = ({ uri, onBackendFound }) => {
}
};

void fetchAsync();
fetchAsync().catch(() => {
// Ignore - this page is just a probe, so we don't need to show any errors if backend is not found
});
}, 3000);

return () => {
Expand Down
34 changes: 18 additions & 16 deletions webapp/src/libs/hooks/useChat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,9 @@ export const useChat = () => {

const createChat = async () => {
const chatTitle = `Copilot @ ${new Date().toLocaleString()}`;
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
await chatService
.createChatAsync(userId, chatTitle, accessToken)
.createChatAsync(userId, chatTitle, await AuthHelper.getSKaaSAccessToken(instance, inProgress))
.then((result: ICreateChatSessionResponse) => {
const newChat: ChatState = {
id: result.chatSession.id,
Expand Down Expand Up @@ -148,8 +147,8 @@ export const useChat = () => {
};

const loadChats = async () => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
const chatSessions = await chatService.getAllChatsAsync(userId, accessToken);

if (chatSessions.length > 0) {
Expand Down Expand Up @@ -201,10 +200,9 @@ export const useChat = () => {
};

const uploadBot = async (bot: Bot) => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
botService
.uploadAsync(bot, userId, accessToken)
.then(async (chatSession: IChatSession) => {
try {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
await botService.uploadAsync(bot, userId, accessToken).then(async (chatSession: IChatSession) => {
const chatMessages = await chatService.getChatMessagesAsync(chatSession.id, 0, 100, accessToken);

const newChat = {
Expand All @@ -217,11 +215,11 @@ export const useChat = () => {
};

dispatch(addConversation(newChat));
})
.catch((e: any) => {
const errorMessage = `Unable to upload the bot. Details: ${getErrorDetails(e)}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
});
} catch (e: any) {
const errorMessage = `Unable to upload the bot. Details: ${getErrorDetails(e)}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
}
};

const getBotProfilePicture = (index: number): string => {
Expand Down Expand Up @@ -282,8 +280,8 @@ export const useChat = () => {
};

const joinChat = async (chatId: string) => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
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 Expand Up @@ -315,19 +313,23 @@ export const useChat = () => {
};

const editChat = async (chatId: string, title: string, syetemDescription: string, memoryBalance: number) => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
await chatService.editChatAsync(chatId, title, syetemDescription, memoryBalance, accessToken);
await chatService.editChatAsync(
chatId,
title,
syetemDescription,
memoryBalance,
await AuthHelper.getSKaaSAccessToken(instance, inProgress),
);
} catch (e: any) {
const errorMessage = `Error editing chat ${chatId}. Details: ${getErrorDetails(e)}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
}
};

const getServiceOptions = async () => {
const accessToken = await AuthHelper.getSKaaSAccessToken(instance, inProgress);
try {
return await chatService.getServiceOptionsAsync(accessToken);
return await chatService.getServiceOptionsAsync(await AuthHelper.getSKaaSAccessToken(instance, inProgress));
} catch (e: any) {
const errorMessage = `Error getting service options. Details: ${getErrorDetails(e)}`;
dispatch(addAlert({ message: errorMessage, type: AlertType.Error }));
Expand Down
1 change: 1 addition & 0 deletions webapp/src/redux/features/app/AppState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export interface ActiveUserInfo {
export interface Alert {
message: string;
type: AlertType;
id?: string;
}

interface Feature {
Expand Down
46 changes: 43 additions & 3 deletions webapp/src/redux/features/app/appSlice.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft. All rights reserved.

import { createSlice, PayloadAction } from '@reduxjs/toolkit';
import { Constants } from '../../../Constants';
import { ServiceOptions } from '../../../libs/models/ServiceOptions';
import { TokenUsage } from '../../../libs/models/TokenUsage';
import { ActiveUserInfo, Alert, AppState, FeatureKeys, initialState } from './AppState';
Expand All @@ -13,10 +14,14 @@ export const appSlice = createSlice({
state.alerts = action.payload;
},
addAlert: (state: AppState, action: PayloadAction<Alert>) => {
if (state.alerts.length === 3) {
state.alerts.shift();
if (
action.payload.id == Constants.app.CONNECTION_ALERT_ID ||
isServerConnectionError(action.payload.message)
) {
updateConnectionStatus(state, action.payload);
} else {
addNewAlert(state.alerts, action.payload);
}
state.alerts.push(action.payload);
},
removeAlert: (state: AppState, action: PayloadAction<number>) => {
state.alerts.splice(action.payload, 1);
Expand Down Expand Up @@ -89,3 +94,38 @@ const getTotalTokenUsage = (previousSum?: number, current?: number) => {

return previousSum + current;
};

const isServerConnectionError = (message: string) => {
return (
message.includes(`Cannot send data if the connection is not in the 'Connected' State.`) ||
message.includes(`Server timeout elapsed without receiving a message from the server.`)
);
};

const addNewAlert = (alerts: Alert[], newAlert: Alert) => {
if (alerts.length === 3) {
alerts.shift();
}

alerts.push(newAlert);
};

const updateConnectionStatus = (state: AppState, statusUpdate: Alert) => {
if (isServerConnectionError(statusUpdate.message)) {
statusUpdate.message =
// Constant message so alert UI doesn't feel glitchy on every connection error from SignalR
'Cannot send data due to lost connection or server timeout. Try refreshing this page to restart the connection.';
}

// There should only ever be one connection alert at a time,
// so we tag the alert with a unique ID so we can remove if needed
statusUpdate.id ??= Constants.app.CONNECTION_ALERT_ID;

// Remove the existing connection alert if it exists
const connectionAlertIndex = state.alerts.findIndex((alert) => alert.id === Constants.app.CONNECTION_ALERT_ID);
if (connectionAlertIndex !== -1) {
state.alerts.splice(connectionAlertIndex, 1);
}

addNewAlert(state.alerts, statusUpdate);
};
21 changes: 17 additions & 4 deletions webapp/src/redux/features/message-relay/signalRMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import * as signalR from '@microsoft/signalr';
import { AnyAction, Dispatch } from '@reduxjs/toolkit';
import { Constants } from '../../../Constants';
import { AlertType } from '../../../libs/models/AlertType';
import { IChatUser } from '../../../libs/models/ChatUser';
import { PlanState } from '../../../libs/models/Plan';
Expand Down Expand Up @@ -65,23 +66,35 @@ const registerCommonSignalConnectionEvents = (store: Store) => {
hubConnection.onclose((error) => {
if (hubConnection.state === signalR.HubConnectionState.Disconnected) {
const errorMessage = 'Connection closed due to error. Try refreshing this page to restart the connection';
store.dispatch(addAlert({ message: String(errorMessage), type: AlertType.Error }));
store.dispatch(
addAlert({
message: String(errorMessage),
type: AlertType.Error,
id: Constants.app.CONNECTION_ALERT_ID,
}),
);
console.log(errorMessage, error);
}
});

hubConnection.onreconnecting((error) => {
if (hubConnection.state === signalR.HubConnectionState.Reconnecting) {
const errorMessage = 'Connection lost due to error. Reconnecting...';
store.dispatch(addAlert({ message: String(errorMessage), type: AlertType.Info }));
store.dispatch(
addAlert({
message: String(errorMessage),
type: AlertType.Info,
id: Constants.app.CONNECTION_ALERT_ID,
}),
);
console.log(errorMessage, error);
}
});

hubConnection.onreconnected((connectionId = '') => {
if (hubConnection.state === signalR.HubConnectionState.Connected) {
const message = 'Connection reestablished.';
store.dispatch(addAlert({ message, type: AlertType.Success }));
const message = 'Connection reestablished. Please refresh the page to ensure you have the latest data.';
store.dispatch(addAlert({ message, type: AlertType.Success, id: Constants.app.CONNECTION_ALERT_ID }));
console.log(message + ` Connected with connectionId ${connectionId}`);
}
});
Expand Down

0 comments on commit 334fac9

Please sign in to comment.