Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Better error handling in jump to date (#10405)
Browse files Browse the repository at this point in the history 
 - Friendly error messages with details
 - Add a way to submit debug logs for actual errors (non-networking errors)
 - Don't jump someone back to a room they already navigated away from. Fixes bug mentioned in element-hq/element-web#21263 (comment)
  • Loading branch information
@MadLittleMods
MadLittleMods authored Mar 24, 2023
1 parent 1af7108 commit e5f06df
Show file tree
Hide file tree
Showing 11 changed files with 425 additions and 84 deletions.
11 changes: 9 additions & 2 deletions src/Modal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,18 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
return this.appendDialogAsync<C>(Promise.resolve(Element), props, className);
}

public closeCurrentModal(reason: string): void {
/**
* @param reason either "backgroundClick" or undefined
* @return whether a modal was closed
*/
public closeCurrentModal(reason?: string): boolean {
const modal = this.getCurrentModal();
if (!modal) {
return;
return false;
}
modal.closeReason = reason;
modal.close();
return true;
}

private buildModal<C extends ComponentType>(
Expand Down Expand Up @@ -346,6 +351,8 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
}

private async reRender(): Promise<void> {
// TODO: We should figure out how to remove this weird sleep. It also makes testing harder
//
// await next tick because sometimes ReactDOM can race with itself and cause the modal to wrongly stick around
await sleep(0);

Expand Down
147 changes: 123 additions & 24 deletions src/components/views/messages/DateSeparator.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,19 @@ limitations under the License.
import React from "react";
import { Direction } from "matrix-js-sdk/src/models/event-timeline";
import { logger } from "matrix-js-sdk/src/logger";
import { ConnectionError, MatrixError, HTTPError } from "matrix-js-sdk/src/http-api";

import { _t } from "../../../languageHandler";
import { formatFullDateNoTime } from "../../../DateUtils";
import { formatFullDateNoDay, formatFullDateNoTime } from "../../../DateUtils";
import { MatrixClientPeg } from "../../../MatrixClientPeg";
import dis from "../../../dispatcher/dispatcher";
import dispatcher from "../../../dispatcher/dispatcher";
import { Action } from "../../../dispatcher/actions";
import SettingsStore from "../../../settings/SettingsStore";
import { UIFeature } from "../../../settings/UIFeature";
import Modal from "../../../Modal";
import ErrorDialog from "../dialogs/ErrorDialog";
import BugReportDialog from "../dialogs/BugReportDialog";
import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton";
import { contextMenuBelow } from "../rooms/RoomTile";
import { ContextMenuTooltipButton } from "../../structures/ContextMenu";
import IconizedContextMenu, {
Expand All @@ -36,6 +39,7 @@ import IconizedContextMenu, {
} from "../context_menus/IconizedContextMenu";
import JumpToDatePicker from "./JumpToDatePicker";
import { ViewRoomPayload } from "../../../dispatcher/payloads/ViewRoomPayload";
import { SdkContextClass } from "../../../contexts/SDKContext";

function getDaysArray(): string[] {
return [_t("Sunday"), _t("Monday"), _t("Tuesday"), _t("Wednesday"), _t("Thursday"), _t("Friday"), _t("Saturday")];
Expand Down Expand Up @@ -76,7 +80,7 @@ export default class DateSeparator extends React.Component<IProps, IState> {
if (this.settingWatcherRef) SettingsStore.unwatchSetting(this.settingWatcherRef);
}

private onContextMenuOpenClick = (e: React.MouseEvent): void => {
private onContextMenuOpenClick = (e: ButtonEvent): void => {
e.preventDefault();
e.stopPropagation();
const target = e.target as HTMLButtonElement;
Expand Down Expand Up @@ -118,12 +122,12 @@ export default class DateSeparator extends React.Component<IProps, IState> {

private pickDate = async (inputTimestamp: number | string | Date): Promise<void> => {
const unixTimestamp = new Date(inputTimestamp).getTime();
const roomIdForJumpRequest = this.props.roomId;

const cli = MatrixClientPeg.get();
try {
const roomId = this.props.roomId;
const cli = MatrixClientPeg.get();
const { event_id: eventId, origin_server_ts: originServerTs } = await cli.timestampToEvent(
roomId,
roomIdForJumpRequest,
unixTimestamp,
Direction.Forward,
);
Expand All @@ -132,28 +136,113 @@ export default class DateSeparator extends React.Component<IProps, IState> {
`found ${eventId} (${originServerTs}) for timestamp=${unixTimestamp} (looking forward)`,
);

dis.dispatch<ViewRoomPayload>({
action: Action.ViewRoom,
event_id: eventId,
highlighted: true,
room_id: roomId,
metricsTrigger: undefined, // room doesn't change
});
} catch (e) {
const code = e.errcode || e.statusCode;
// only show the dialog if failing for something other than a network error
// (e.g. no errcode or statusCode) as in that case the redactions end up in the
// detached queue and we show the room status bar to allow retry
if (typeof code !== "undefined") {
// display error message stating you couldn't delete this.
// Only try to navigate to the room if the user is still viewing the same
// room. We don't want to jump someone back to a room after a slow request
// if they've already navigated away to another room.
const currentRoomId = SdkContextClass.instance.roomViewStore.getRoomId();
if (currentRoomId === roomIdForJumpRequest) {
dispatcher.dispatch<ViewRoomPayload>({
action: Action.ViewRoom,
event_id: eventId,
highlighted: true,
room_id: roomIdForJumpRequest,
metricsTrigger: undefined, // room doesn't change
});
} else {
logger.debug(
`No longer navigating to date in room (jump to date) because the user already switched ` +
`to another room: currentRoomId=${currentRoomId}, roomIdForJumpRequest=${roomIdForJumpRequest}`,
);
}
} catch (err) {
logger.error(
`Error occured while trying to find event in ${roomIdForJumpRequest} ` +
`at timestamp=${unixTimestamp}:`,
err,
);

// Only display an error if the user is still viewing the same room. We
// don't want to worry someone about an error in a room they no longer care
// about after a slow request if they've already navigated away to another
// room.
const currentRoomId = SdkContextClass.instance.roomViewStore.getRoomId();
if (currentRoomId === roomIdForJumpRequest) {
let friendlyErrorMessage = "An error occured while trying to find and jump to the given date.";
let submitDebugLogsContent: JSX.Element = <></>;
if (err instanceof ConnectionError) {
friendlyErrorMessage = _t(
"A network error occurred while trying to find and jump to the given date. " +
"Your homeserver might be down or there was just a temporary problem with " +
"your internet connection. Please try again. If this continues, please " +
"contact your homeserver administrator.",
);
} else if (err instanceof MatrixError) {
if (err?.errcode === "M_NOT_FOUND") {
friendlyErrorMessage = _t(
"We were unable to find an event looking forwards from %(dateString)s. " +
"Try choosing an earlier date.",
{ dateString: formatFullDateNoDay(new Date(unixTimestamp)) },
);
} else {
friendlyErrorMessage = _t("Server returned %(statusCode)s with error code %(errorCode)s", {
statusCode: err?.httpStatus || _t("unknown status code"),
errorCode: err?.errcode || _t("unavailable"),
});
}
} else if (err instanceof HTTPError) {
friendlyErrorMessage = err.message;
} else {
// We only give the option to submit logs for actual errors, not network problems.
submitDebugLogsContent = (
<p>
{_t(
"Please submit <debugLogsLink>debug logs</debugLogsLink> to help us " +
"track down the problem.",
{},
{
debugLogsLink: (sub) => (
<AccessibleButton
// This is by default a `<div>` which we
// can't nest within a `<p>` here so update
// this to a be a inline anchor element.
element="a"
kind="link"
onClick={() => this.onBugReport(err instanceof Error ? err : undefined)}
data-testid="jump-to-date-error-submit-debug-logs-button"
>
{sub}
</AccessibleButton>
),
},
)}
</p>
);
}

Modal.createDialog(ErrorDialog, {
title: _t("Error"),
description: _t("Unable to find event at that date. (%(code)s)", { code }),
title: _t("Unable to find event at that date"),
description: (
<div data-testid="jump-to-date-error-content">
<p>{friendlyErrorMessage}</p>
{submitDebugLogsContent}
<details>
<summary>{_t("Error details")}</summary>
<p>{String(err)}</p>
</details>
</div>
),
});
}
}
};

private onBugReport = (err?: Error): void => {
Modal.createDialog(BugReportDialog, {
error: err,
initialText: "Error occured while using jump to date #jump-to-date",
});
};

private onLastWeekClicked = (): void => {
const date = new Date();
date.setDate(date.getDate() - 7);
Expand Down Expand Up @@ -189,11 +278,20 @@ export default class DateSeparator extends React.Component<IProps, IState> {
onFinished={this.onContextMenuCloseClick}
>
<IconizedContextMenuOptionList first>
<IconizedContextMenuOption label={_t("Last week")} onClick={this.onLastWeekClicked} />
<IconizedContextMenuOption label={_t("Last month")} onClick={this.onLastMonthClicked} />
<IconizedContextMenuOption
label={_t("Last week")}
onClick={this.onLastWeekClicked}
data-testid="jump-to-date-last-week"
/>
<IconizedContextMenuOption
label={_t("Last month")}
onClick={this.onLastMonthClicked}
data-testid="jump-to-date-last-month"
/>
<IconizedContextMenuOption
label={_t("The beginning of the room")}
onClick={this.onTheBeginningClicked}
data-testid="jump-to-date-beginning"
/>
</IconizedContextMenuOptionList>

Expand All @@ -207,6 +305,7 @@ export default class DateSeparator extends React.Component<IProps, IState> {
return (
<ContextMenuTooltipButton
className="mx_DateSeparator_jumpToDateMenu mx_DateSeparator_dateContent"
data-testid="jump-to-date-separator-button"
onClick={this.onContextMenuOpenClick}
isExpanded={!!this.state.contextMenuPosition}
title={_t("Jump to date")}
Expand Down
9 changes: 8 additions & 1 deletion src/i18n/strings/en_EN.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2353,7 +2353,14 @@
"Saturday": "Saturday",
"Today": "Today",
"Yesterday": "Yesterday",
"Unable to find event at that date. (%(code)s)": "Unable to find event at that date. (%(code)s)",
"A network error occurred while trying to find and jump to the given date. Your homeserver might be down or there was just a temporary problem with your internet connection. Please try again. If this continues, please contact your homeserver administrator.": "A network error occurred while trying to find and jump to the given date. Your homeserver might be down or there was just a temporary problem with your internet connection. Please try again. If this continues, please contact your homeserver administrator.",
"We were unable to find an event looking forwards from %(dateString)s. Try choosing an earlier date.": "We were unable to find an event looking forwards from %(dateString)s. Try choosing an earlier date.",
"Server returned %(statusCode)s with error code %(errorCode)s": "Server returned %(statusCode)s with error code %(errorCode)s",
"unknown status code": "unknown status code",
"unavailable": "unavailable",
"Please submit <debugLogsLink>debug logs</debugLogsLink> to help us track down the problem.": "Please submit <debugLogsLink>debug logs</debugLogsLink> to help us track down the problem.",
"Unable to find event at that date": "Unable to find event at that date",
"Error details": "Error details",
"Last week": "Last week",
"Last month": "Last month",
"The beginning of the room": "The beginning of the room",
Expand Down
38 changes: 24 additions & 14 deletions test/components/structures/auth/ForgotPassword-test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,13 @@ import { MatrixClient, createClient } from "matrix-js-sdk/src/matrix";

import ForgotPassword from "../../../../src/components/structures/auth/ForgotPassword";
import { ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig";
import { filterConsole, flushPromisesWithFakeTimers, stubClient } from "../../../test-utils";
import Modal from "../../../../src/Modal";
import {
clearAllModals,
filterConsole,
flushPromisesWithFakeTimers,
stubClient,
waitEnoughCyclesForModal,
} from "../../../test-utils";
import AutoDiscoveryUtils from "../../../../src/utils/AutoDiscoveryUtils";

jest.mock("matrix-js-sdk/src/matrix", () => ({
Expand Down Expand Up @@ -55,11 +60,6 @@ describe("<ForgotPassword>", () => {
});
};

const waitForDialog = async (): Promise<void> => {
await flushPromisesWithFakeTimers();
await flushPromisesWithFakeTimers();
};

const itShouldCloseTheDialogAndShowThePasswordInput = (): void => {
it("should close the dialog and show the password input", () => {
expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument();
Expand Down Expand Up @@ -88,9 +88,9 @@ describe("<ForgotPassword>", () => {
jest.spyOn(AutoDiscoveryUtils, "authComponentStateForError");
});

afterEach(() => {
afterEach(async () => {
// clean up modals
Modal.closeCurrentModal("force");
await clearAllModals();
});

beforeAll(() => {
Expand Down Expand Up @@ -322,7 +322,9 @@ describe("<ForgotPassword>", () => {
describe("and submitting it", () => {
beforeEach(async () => {
await click(screen.getByText("Reset password"));
await waitForDialog();
await waitEnoughCyclesForModal({
useFakeTimers: true,
});
});

it("should send the new password and show the click validation link dialog", () => {
Expand Down Expand Up @@ -350,7 +352,9 @@ describe("<ForgotPassword>", () => {
await act(async () => {
await userEvent.click(screen.getByTestId("dialog-background"), { delay: null });
});
await waitForDialog();
await waitEnoughCyclesForModal({
useFakeTimers: true,
});
});

itShouldCloseTheDialogAndShowThePasswordInput();
Expand All @@ -359,7 +363,9 @@ describe("<ForgotPassword>", () => {
describe("and dismissing the dialog", () => {
beforeEach(async () => {
await click(screen.getByLabelText("Close dialog"));
await waitForDialog();
await waitEnoughCyclesForModal({
useFakeTimers: true,
});
});

itShouldCloseTheDialogAndShowThePasswordInput();
Expand All @@ -368,7 +374,9 @@ describe("<ForgotPassword>", () => {
describe("and clicking »Re-enter email address«", () => {
beforeEach(async () => {
await click(screen.getByText("Re-enter email address"));
await waitForDialog();
await waitEnoughCyclesForModal({
useFakeTimers: true,
});
});

it("should close the dialog and go back to the email input", () => {
Expand Down Expand Up @@ -400,7 +408,9 @@ describe("<ForgotPassword>", () => {
beforeEach(async () => {
await click(screen.getByText("Sign out of all devices"));
await click(screen.getByText("Reset password"));
await waitForDialog();
await waitEnoughCyclesForModal({
useFakeTimers: true,
});
});

it("should show the sign out warning dialog", async () => {
Expand Down
Loading

0 comments on commit e5f06df

Please sign in to comment.