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

Better error handling in jump to date #10405

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e13e311
Better error handling in jump to date
MadLittleMods Mar 17, 2023
054688d
Actually find current room ID when comparing
MadLittleMods Mar 18, 2023
70f65f4
Merge branch 'develop' into madlittlemods/21263-better-error-handling…
MadLittleMods Mar 20, 2023
072d878
Add fallback for non HTTP errors
MadLittleMods Mar 20, 2023
2c247c9
Fix some strict errors
MadLittleMods Mar 20, 2023
72b2ebd
Add a way to submit debug logs for actual errors
MadLittleMods Mar 21, 2023
d5fd664
Add some tests
MadLittleMods Mar 21, 2023
8210828
Fix modals in tests leaking across
MadLittleMods Mar 21, 2023
bc13247
Generalize test
MadLittleMods Mar 21, 2023
d90f4f6
Remove duplicated info
MadLittleMods Mar 21, 2023
93d200f
Generalize modal problems into utilities
MadLittleMods Mar 21, 2023
1c724e1
Remove currentDate
MadLittleMods Mar 21, 2023
df2b4d2
Better language
MadLittleMods Mar 21, 2023
8c42581
Fix some imports
MadLittleMods Mar 21, 2023
bac5a89
Type error
MadLittleMods Mar 21, 2023
78cbc06
Try to type errors
MadLittleMods Mar 21, 2023
5ca2bb4
Fix event type
MadLittleMods Mar 21, 2023
6e921da
Update translations
MadLittleMods Mar 21, 2023
7079c69
Remove cruft
MadLittleMods Mar 21, 2023
bcb51b4
Fix test/components/structures/auth/ForgotPassword-test.tsx running f…
MadLittleMods Mar 21, 2023
99f8ff3
Merge branch 'develop' into madlittlemods/21263-better-error-handling…
MadLittleMods Mar 21, 2023
d3b0790
Fix export
MadLittleMods Mar 21, 2023
5fefd64
Add better comment
MadLittleMods Mar 21, 2023
b633a74
Merge branch 'develop' into madlittlemods/21263-better-error-handling…
MadLittleMods Mar 22, 2023
d901c90
No string interpolation needed
MadLittleMods Mar 22, 2023
9cecaa9
Better language
MadLittleMods Mar 22, 2023
52e6f30
Be more specific when we show the network problem language
MadLittleMods Mar 22, 2023
0902215
Add expects to show intention
MadLittleMods Mar 22, 2023
2aa7f91
Add comment about potential better future
MadLittleMods Mar 22, 2023
7c4a398
Update i18n
MadLittleMods Mar 23, 2023
23268af
Test other date picks
MadLittleMods Mar 23, 2023
1447b76
Add test for MatrixError
MadLittleMods Mar 23, 2023
222fcc4
Merge branch 'develop' into madlittlemods/21263-better-error-handling…
MadLittleMods Mar 23, 2023
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
11 changes: 9 additions & 2 deletions src/Modal.tsx
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning a boolean so we can keep looping until all modals are closed. See clearAllModals

}

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
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
Comment on lines -144 to -146
Copy link
Contributor Author

@MadLittleMods MadLittleMods Mar 17, 2023

Choose a reason for hiding this comment

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

This previous comment was bogus. Just copy/paste cruft from

// 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. " +
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
"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, {
Copy link
Contributor Author

@MadLittleMods MadLittleMods Mar 21, 2023

Choose a reason for hiding this comment

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

I don't know how to fix this TypeScript error:

src/components/views/messages/DateSeparator.tsx:214:36 - Argument of type 'typeof ErrorDialog' is not assignable to parameter of type 'ComponentType'.

Same pattern we use everywhere else in the codebase so I don't know of a better example to work from either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alunturner Any tips on how to fix this error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid I think I've probably done the same as you and checked the other uses of this, seeing that they all have the same issue. The problem must lie in Modal.createDialog (so it seems to me) given the consistency of the TS errors throughout the codebase. If I had tips, I'd give them.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Mar 22, 2023

Choose a reason for hiding this comment

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

I've created an issue to track this element-hq/element-web#24899

I guess we will just go with an exception here and worry about the problem generally in a separate PR down the line.

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
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
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