From e13e3113d3ff1f6067264c8d8a74ebba8358ab3a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Mar 2023 18:45:05 -0500 Subject: [PATCH 01/29] Better error handling in jump to date --- .../views/messages/DateSeparator.tsx | 84 ++++++++++++++----- src/i18n/strings/en_EN.json | 7 +- 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index b9cb3456804..9399563d3bc 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -20,7 +20,7 @@ import { Direction } from "matrix-js-sdk/src/models/event-timeline"; import { logger } from "matrix-js-sdk/src/logger"; import { _t } from "../../../languageHandler"; -import { formatFullDateNoTime } from "../../../DateUtils"; +import { formatFullDateNoDay, formatFullDateNoTime } from "../../../DateUtils"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; import dis from "../../../dispatcher/dispatcher"; import { Action } from "../../../dispatcher/actions"; @@ -118,12 +118,12 @@ export default class DateSeparator extends React.Component { private pickDate = async (inputTimestamp: number | string | Date): Promise => { 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, ); @@ -132,23 +132,67 @@ export default class DateSeparator extends React.Component { `found ${eventId} (${originServerTs}) for timestamp=${unixTimestamp} (looking forward)`, ); - dis.dispatch({ - 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 roomIdBeforeNavigation = this.props.roomId; + if (roomIdBeforeNavigation === roomIdForJumpRequest) { + dis.dispatch({ + action: Action.ViewRoom, + event_id: eventId, + highlighted: true, + room_id: roomIdBeforeNavigation, + metricsTrigger: undefined, // room doesn't change + }); + } + } 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 roomIdBeforeDisplayingError = this.props.roomId; + if (roomIdBeforeDisplayingError === roomIdForJumpRequest) { + let friendlyErrorMessage = `An error occured while trying to find and jump to the given date.`; + 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)) }, + ); + } + if (err.name === "ConnectionError") { + friendlyErrorMessage = _t( + "Your homeserver was unreachable and was not able to log you in. Please try again. " + + "If this continues, please contact your homeserver administrator.", + ); + } + 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: ( + <> +

{friendlyErrorMessage}

+
+ {_t("Error details")} + +
    +
  • {_t("Request status code: %(statusCode)s", { statusCode: err.httpStatus })}
    • +
  • + {_t("Error code: %(errorCode)s", { + errorCode: err.errcode || _t("Error code not available"), + })} +
    • +
+

{String(err)}

+
+ + ), }); } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 0828b7b6d71..c82ee8839c1 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2350,7 +2350,12 @@ "Saturday": "Saturday", "Today": "Today", "Yesterday": "Yesterday", - "Unable to find event at that date. (%(code)s)": "Unable to find event at that date. (%(code)s)", + "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.", + "Unable to find event at that date": "Unable to find event at that date", + "Error details": "Error details", + "Request status code: %(statusCode)s": "Request status code: %(statusCode)s", + "Error code: %(errorCode)s": "Error code: %(errorCode)s", + "Error code not available": "Error code not available", "Last week": "Last week", "Last month": "Last month", "The beginning of the room": "The beginning of the room", From 054688d19b8757e8c859dc80dbc7c252b48562ba Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Mar 2023 19:13:48 -0500 Subject: [PATCH 02/29] Actually find current room ID when comparing See https://github.com/matrix-org/matrix-react-sdk/pull/10405#discussion_r1140839347 --- src/components/views/messages/DateSeparator.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 9399563d3bc..87b0074808d 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -36,6 +36,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")]; @@ -135,7 +136,7 @@ export default class DateSeparator extends React.Component { // 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 roomIdBeforeNavigation = this.props.roomId; + const roomIdBeforeNavigation = SdkContextClass.instance.roomViewStore.getRoomId(); if (roomIdBeforeNavigation === roomIdForJumpRequest) { dis.dispatch({ action: Action.ViewRoom, @@ -156,7 +157,7 @@ export default class DateSeparator extends React.Component { // 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 roomIdBeforeDisplayingError = this.props.roomId; + const roomIdBeforeDisplayingError = SdkContextClass.instance.roomViewStore.getRoomId(); if (roomIdBeforeDisplayingError === roomIdForJumpRequest) { let friendlyErrorMessage = `An error occured while trying to find and jump to the given date.`; if (err.errcode === "M_NOT_FOUND") { From 072d8789171473c1571a67893b718959206b0c8b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 18:38:53 -0500 Subject: [PATCH 03/29] Add fallback for non HTTP errors --- src/components/views/messages/DateSeparator.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 028d96d8eae..adfa1c946ef 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -183,7 +183,11 @@ export default class DateSeparator extends React.Component { {_t("Error details")}
    -
  • {_t("Request status code: %(statusCode)s", { statusCode: err.httpStatus })}
    • +
  • + {_t("Request status code: %(statusCode)s", { + statusCode: err.httpStatus || _t("HTTP status code not available"), + })} +
  • {_t("Error code: %(errorCode)s", { errorCode: err.errcode || _t("Error code not available"), From 2c247c95c2c8d46ac7519bbefb9546317569d84b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 18:41:30 -0500 Subject: [PATCH 04/29] Fix some strict errors --- src/components/views/messages/DateSeparator.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index adfa1c946ef..53e0827a056 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -160,14 +160,14 @@ export default class DateSeparator extends React.Component { const roomIdBeforeDisplayingError = SdkContextClass.instance.roomViewStore.getRoomId(); if (roomIdBeforeDisplayingError === roomIdForJumpRequest) { let friendlyErrorMessage = `An error occured while trying to find and jump to the given date.`; - if (err.errcode === "M_NOT_FOUND") { + 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)) }, ); } - if (err.name === "ConnectionError") { + if (err?.name === "ConnectionError") { friendlyErrorMessage = _t( "Your homeserver was unreachable and was not able to log you in. Please try again. " + "If this continues, please contact your homeserver administrator.", @@ -185,12 +185,12 @@ export default class DateSeparator extends React.Component {
    • {_t("Request status code: %(statusCode)s", { - statusCode: err.httpStatus || _t("HTTP status code not available"), + statusCode: err?.httpStatus || _t("HTTP status code not available"), })}
    • {_t("Error code: %(errorCode)s", { - errorCode: err.errcode || _t("Error code not available"), + errorCode: err?.errcode || _t("Error code not available"), })}
    From 72b2ebd5f38e162e661eddacfe39127471cc1807 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 19:11:55 -0500 Subject: [PATCH 05/29] Add a way to submit debug logs for actual errors Inspired by https://github.com/matrix-org/matrix-react-sdk/pull/8354 --- .../views/messages/DateSeparator.tsx | 56 +++++++++++++++---- src/i18n/strings/en_EN.json | 3 + 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 53e0827a056..cdf4edda910 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -28,6 +28,8 @@ 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 from "../elements/AccessibleButton"; import { contextMenuBelow } from "../rooms/RoomTile"; import { ContextMenuTooltipButton } from "../../structures/ContextMenu"; import IconizedContextMenu, { @@ -160,17 +162,43 @@ export default class DateSeparator extends React.Component { const roomIdBeforeDisplayingError = SdkContextClass.instance.roomViewStore.getRoomId(); if (roomIdBeforeDisplayingError === roomIdForJumpRequest) { let friendlyErrorMessage = `An error occured while trying to find and jump to the given date.`; - 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)) }, - ); - } + let submitDebugLogsContent: JSX.Element = <>; if (err?.name === "ConnectionError") { - friendlyErrorMessage = _t( - "Your homeserver was unreachable and was not able to log you in. Please try again. " + - "If this continues, please contact your homeserver administrator.", + 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( + "A network error occurred while trying to find and jump to the given date. " + + "Your homeserver might be down or was just a temporary problem with your " + + "internet connection. Please try again. If this continues, please " + + "contact your homeserver administrator.", + ); + } + } else { + // We only give the option to submit logs for actual errors, not network problems. + submitDebugLogsContent = ( +

    + {_t( + "Please submit debug logs to help us " + + "track down the problem.", + {}, + { + debugLogsLink: (sub) => ( + this.onBugReport(err)} + data-testid="jump-to-date-error-submit-debug-logs-button" + > + {sub} + + ), + }, + )} +

    ); } @@ -179,6 +207,7 @@ export default class DateSeparator extends React.Component { description: ( <>

    {friendlyErrorMessage}

    + {submitDebugLogsContent}
    {_t("Error details")} @@ -203,6 +232,13 @@ export default class DateSeparator extends React.Component { } }; + private onBugReport = (err): 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); diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index c82ee8839c1..f5918eb61e4 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2351,9 +2351,12 @@ "Today": "Today", "Yesterday": "Yesterday", "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.", + "A network error occurred while trying to find and jump to the given date. Your homeserver might be down or 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 was just a temporary problem with your internet connection. Please try again. If this continues, please contact your homeserver administrator.", + "Please submit debug logs to help us track down the problem.": "Please submit debug logs 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", "Request status code: %(statusCode)s": "Request status code: %(statusCode)s", + "HTTP status code not available": "HTTP status code not available", "Error code: %(errorCode)s": "Error code: %(errorCode)s", "Error code not available": "Error code not available", "Last week": "Last week", From d5fd6647e510dddb5f093c53613a280c24e07c4e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 21:18:38 -0500 Subject: [PATCH 06/29] Add some tests --- .../views/messages/DateSeparator.tsx | 30 ++++-- .../views/messages/DateSeparator-test.tsx | 102 ++++++++++++++---- .../__snapshots__/DateSeparator-test.tsx.snap | 1 + 3 files changed, 101 insertions(+), 32 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index cdf4edda910..3871610e32b 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -22,7 +22,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { _t } from "../../../languageHandler"; 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"; @@ -138,15 +138,20 @@ export default class DateSeparator extends React.Component { // 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 roomIdBeforeNavigation = SdkContextClass.instance.roomViewStore.getRoomId(); - if (roomIdBeforeNavigation === roomIdForJumpRequest) { - dis.dispatch({ + const currentRoomId = SdkContextClass.instance.roomViewStore.getRoomId(); + if (currentRoomId === roomIdForJumpRequest) { + dispatcher.dispatch({ action: Action.ViewRoom, event_id: eventId, highlighted: true, - room_id: roomIdBeforeNavigation, + 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( @@ -159,8 +164,8 @@ export default class DateSeparator extends React.Component { // 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 roomIdBeforeDisplayingError = SdkContextClass.instance.roomViewStore.getRoomId(); - if (roomIdBeforeDisplayingError === roomIdForJumpRequest) { + 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?.name === "ConnectionError") { @@ -205,7 +210,7 @@ export default class DateSeparator extends React.Component { Modal.createDialog(ErrorDialog, { title: _t("Unable to find event at that date"), description: ( - <> +

    {friendlyErrorMessage}

    {submitDebugLogsContent}
    @@ -225,7 +230,7 @@ export default class DateSeparator extends React.Component {

{String(err)}

- + ), }); } @@ -274,7 +279,11 @@ export default class DateSeparator extends React.Component { onFinished={this.onContextMenuCloseClick} > - + { return ( { const HOUR_MS = 3600000; const DAY_MS = HOUR_MS * 24; // Friday Dec 17 2021, 9:09am - const now = "2021-12-17T08:09:00.000Z"; - const nowMs = 1639728540000; + const nowDate = new Date("2021-12-17T08:09:00.000Z"); + const roomId = "!unused:example.org"; const defaultProps = { - ts: nowMs, - now, - roomId: "!unused:example.org", + ts: nowDate.getTime(), + roomId, }; - const RealDate = global.Date; - class MockDate extends Date { - constructor(date: string | number | Date) { - super(date || now); - } - } - - const mockClient = getMockClientWithEventEmitter({}); + + const mockClient = getMockClientWithEventEmitter({ + timestampToEvent: jest.fn(), + }); const getComponent = (props = {}) => render( @@ -55,11 +55,11 @@ describe("DateSeparator", () => { type TestCase = [string, number, string]; const testCases: TestCase[] = [ - ["the exact same moment", nowMs, "Today"], - ["same day as current day", nowMs - HOUR_MS, "Today"], - ["day before the current day", nowMs - HOUR_MS * 12, "Yesterday"], - ["2 days ago", nowMs - DAY_MS * 2, "Wednesday"], - ["144 hours ago", nowMs - HOUR_MS * 144, "Sat, Dec 11 2021"], + ["the exact same moment", nowDate.getTime(), "Today"], + ["same day as current day", nowDate.getTime() - HOUR_MS, "Today"], + ["day before the current day", nowDate.getTime() - HOUR_MS * 12, "Yesterday"], + ["2 days ago", nowDate.getTime() - DAY_MS * 2, "Wednesday"], + ["144 hours ago", nowDate.getTime() - HOUR_MS * 144, "Sat, Dec 11 2021"], [ "6 days ago, but less than 144h", new Date("Saturday Dec 11 2021 23:59:00 GMT+0100 (Central European Standard Time)").getTime(), @@ -68,16 +68,20 @@ describe("DateSeparator", () => { ]; beforeEach(() => { - global.Date = MockDate as unknown as DateConstructor; + // Set a consistent fake time here so the test is always consistent + jest.useFakeTimers(); + jest.setSystemTime(nowDate.getTime()); + (SettingsStore.getValue as jest.Mock) = jest.fn((arg) => { if (arg === UIFeature.TimelineEnableRelativeDates) { return true; } }); + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue(roomId); }); afterAll(() => { - global.Date = RealDate; + jest.useRealTimers(); }); it("renders the date separator correctly", () => { @@ -114,16 +118,70 @@ describe("DateSeparator", () => { }); describe("when feature_jump_to_date is enabled", () => { + const currentDate = new Date("2023-03-05"); beforeEach(() => { mocked(SettingsStore).getValue.mockImplementation((arg): any => { if (arg === "feature_jump_to_date") { return true; } }); + jest.spyOn(dispatcher, "dispatch"); }); + it("renders the date separator correctly", () => { const { asFragment } = getComponent(); expect(asFragment()).toMatchSnapshot(); }); + + it("can jump to last week", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Jump to "last week" + const lastWeekDate = new Date(); + lastWeekDate.setDate(currentDate.getDate() - 7); + const returnedEventId = "$abc"; + mockClient.timestampToEvent.mockResolvedValue({ + event_id: returnedEventId, + origin_server_ts: String(lastWeekDate.getTime()), + } satisfies ITimestampToEventResponse); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Flush out the dispatcher which uses `window.setTimeout(...)` since we're + // using `jest.useFakeTimers()` in these tests. + await flushPromisesWithFakeTimers(); + + expect(dispatcher.dispatch).toHaveBeenCalledWith({ + action: Action.ViewRoom, + event_id: returnedEventId, + highlighted: true, + room_id: roomId, + metricsTrigger: undefined, + } satisfies ViewRoomPayload); + }); + + it("should show error dialog with option to submit debug logs when something went wrong while jumping", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Try to jump to "last week" but we want it with a non-network error so it + // shows the "Submit debug logs" UI + mockClient.timestampToEvent.mockRejectedValue(new Error("Fake error in test")); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Expect error to be shown. We have to wait for the UI to transition. + await screen.findByTestId("jump-to-date-error-content"); + + // Expect an option to submit debug logs to be shown + await screen.findByTestId("jump-to-date-error-submit-debug-logs-button"); + }); }); }); diff --git a/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap b/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap index d0af14f5097..16151fc0007 100644 --- a/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap +++ b/test/components/views/messages/__snapshots__/DateSeparator-test.tsx.snap @@ -44,6 +44,7 @@ exports[`DateSeparator when feature_jump_to_date is enabled renders the date sep aria-haspopup="true" aria-label="Jump to date" class="mx_AccessibleButton mx_DateSeparator_jumpToDateMenu mx_DateSeparator_dateContent" + data-testid="jump-to-date-separator-button" role="button" tabindex="0" > From 82108282f14003eaae7ecab9197d3ed04eef7ed0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 23:24:37 -0500 Subject: [PATCH 07/29] Fix modals in tests leaking across --- src/Modal.tsx | 10 ++- .../views/messages/DateSeparator.tsx | 6 +- .../views/messages/DateSeparator-test.tsx | 76 ++++++++++++++++++- 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index b3887795b2c..4710b3b9549 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -149,13 +149,19 @@ export class ModalManager extends TypedEventEmitter(Promise.resolve(Element), props, className); } - public closeCurrentModal(reason: string): void { + /** + * @callback onBeforeClose + * @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( diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 3871610e32b..77a898c057e 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -168,7 +168,7 @@ export default class DateSeparator extends React.Component { if (currentRoomId === roomIdForJumpRequest) { let friendlyErrorMessage = `An error occured while trying to find and jump to the given date.`; let submitDebugLogsContent: JSX.Element = <>; - if (err?.name === "ConnectionError") { + if (err?.name === "ConnectionError" || err?.httpStatus) { if (err?.errcode === "M_NOT_FOUND") { friendlyErrorMessage = _t( "We were unable to find an event looking forwards from %(dateString)s. " + @@ -194,6 +194,10 @@ export default class DateSeparator extends React.Component { { debugLogsLink: (sub) => ( ` which we + // can't nest within a `

` here so update + // this to a be a inline anchor element. + element="a" kind="link" onClick={() => this.onBugReport(err)} data-testid="jump-to-date-error-submit-debug-logs-button" diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index 803bc5a01b5..547bcb66b95 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -18,6 +18,7 @@ import React from "react"; import { mocked } from "jest-mock"; import { fireEvent, render, screen } from "@testing-library/react"; import { ITimestampToEventResponse } from "matrix-js-sdk/src/client"; +import { ConnectionError, HTTPError } from "matrix-js-sdk/src/http-api"; import dispatcher from "../../../../src/dispatcher/dispatcher"; import { Action } from "../../../../src/dispatcher/actions"; @@ -29,6 +30,7 @@ import { UIFeature } from "../../../../src/settings/UIFeature"; import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; import { flushPromisesWithFakeTimers, getMockClientWithEventEmitter } from "../../../test-utils"; import DateSeparator from "../../../../src/components/views/messages/DateSeparator"; +import Modal from "../../../../src/Modal"; jest.mock("../../../../src/settings/SettingsStore"); @@ -120,12 +122,24 @@ describe("DateSeparator", () => { describe("when feature_jump_to_date is enabled", () => { const currentDate = new Date("2023-03-05"); beforeEach(() => { + jest.clearAllMocks(); mocked(SettingsStore).getValue.mockImplementation((arg): any => { if (arg === "feature_jump_to_date") { return true; } }); - jest.spyOn(dispatcher, "dispatch"); + jest.spyOn(dispatcher, "dispatch").mockImplementation(() => {}); + }); + + afterEach(async () => { + // Prevent modals from leaking and polluting other tests + let keepClosingModals = true; + while (keepClosingModals) { + keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)"); + } + + // Then wait for the screen to update (probably React rerender) + await flushPromisesWithFakeTimers(); }); it("renders the date separator correctly", () => { @@ -164,14 +178,22 @@ describe("DateSeparator", () => { } satisfies ViewRoomPayload); }); - it("should show error dialog with option to submit debug logs when something went wrong while jumping", async () => { + it("should not jump to date if we already switched to another room (simulate slow network request)", async () => { + // TODO + }); + + it("should not show jump to date error if we already switched to another room (simulate slow network request)", async () => { + // TODO + }); + + it("should show error dialog with submit debug logs option when non-networking error occurs", async () => { // Render the component getComponent(); // Open the jump to date context menu fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); - // Try to jump to "last week" but we want it with a non-network error so it + // Try to jump to "last week" but we want a non-network error to occur so it // shows the "Submit debug logs" UI mockClient.timestampToEvent.mockRejectedValue(new Error("Fake error in test")); const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); @@ -180,8 +202,54 @@ describe("DateSeparator", () => { // Expect error to be shown. We have to wait for the UI to transition. await screen.findByTestId("jump-to-date-error-content"); - // Expect an option to submit debug logs to be shown + // Expect an option to submit debug logs to be shown when a non-network error occurs await screen.findByTestId("jump-to-date-error-submit-debug-logs-button"); }); + + it("should show error dialog without submit debug logs option when networking error (connection) occurs", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Try to jump to "last week" but we want a network error to occur + const fakeConnectionError = new ConnectionError("Fake connection error in test"); + mockClient.timestampToEvent.mockRejectedValue(fakeConnectionError); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Expect error to be shown. We have to wait for the UI to transition. + await screen.findByTestId("jump-to-date-error-content"); + + screen.debug(); + + // The submit debug logs option should *NOT* be shown for network errors. + // + // We have to use `queryBy` so that it can return `null` for something that does not exist. + expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); + }); + + it("should show error dialog without submit debug logs option when networking error (http) occurs", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Try to jump to "last week" but we want a network error to occur + const fakeHttpError = new HTTPError("Fake connection error in test", 418); + mockClient.timestampToEvent.mockRejectedValue(fakeHttpError); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Expect error to be shown. We have to wait for the UI to transition. + await screen.findByTestId("jump-to-date-error-content"); + + // The submit debug logs option should *NOT* be shown for network errors. + // + // We have to use `queryBy` so that it can return `null` for something that does not exist. + expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); + }); }); }); From bc13247658a2743227b53f58ca4ae85fb1dcf2fa Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 23:34:22 -0500 Subject: [PATCH 08/29] Generalize test --- .../views/messages/DateSeparator-test.tsx | 70 +++++++------------ 1 file changed, 26 insertions(+), 44 deletions(-) diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index 547bcb66b95..5897ead6630 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -206,50 +206,32 @@ describe("DateSeparator", () => { await screen.findByTestId("jump-to-date-error-submit-debug-logs-button"); }); - it("should show error dialog without submit debug logs option when networking error (connection) occurs", async () => { - // Render the component - getComponent(); - - // Open the jump to date context menu - fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); - - // Try to jump to "last week" but we want a network error to occur - const fakeConnectionError = new ConnectionError("Fake connection error in test"); - mockClient.timestampToEvent.mockRejectedValue(fakeConnectionError); - const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); - fireEvent.click(jumpToLastWeekButton); - - // Expect error to be shown. We have to wait for the UI to transition. - await screen.findByTestId("jump-to-date-error-content"); - - screen.debug(); - - // The submit debug logs option should *NOT* be shown for network errors. - // - // We have to use `queryBy` so that it can return `null` for something that does not exist. - expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); - }); - - it("should show error dialog without submit debug logs option when networking error (http) occurs", async () => { - // Render the component - getComponent(); - - // Open the jump to date context menu - fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); - - // Try to jump to "last week" but we want a network error to occur - const fakeHttpError = new HTTPError("Fake connection error in test", 418); - mockClient.timestampToEvent.mockRejectedValue(fakeHttpError); - const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); - fireEvent.click(jumpToLastWeekButton); - - // Expect error to be shown. We have to wait for the UI to transition. - await screen.findByTestId("jump-to-date-error-content"); - - // The submit debug logs option should *NOT* be shown for network errors. - // - // We have to use `queryBy` so that it can return `null` for something that does not exist. - expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); + [ + new ConnectionError("Fake connection error in test"), + new HTTPError("Fake connection error in test", 418), + ].forEach((fakeError) => { + it(`should show error dialog without submit debug logs option when networking error (${fakeError.name}) occurs`, async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Try to jump to "last week" but we want a network error to occur + mockClient.timestampToEvent.mockRejectedValue(fakeError); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Expect error to be shown. We have to wait for the UI to transition. + await screen.findByTestId("jump-to-date-error-content"); + + screen.debug(); + + // The submit debug logs option should *NOT* be shown for network errors. + // + // We have to use `queryBy` so that it can return `null` for something that does not exist. + expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(); + }); }); }); }); From d90f4f6a60fa79f7678a783bba97f3e7e6a48b64 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 20 Mar 2023 23:35:50 -0500 Subject: [PATCH 09/29] Remove duplicated info --- src/components/views/messages/DateSeparator.tsx | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 77a898c057e..3cbf0ff42ae 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -219,19 +219,6 @@ export default class DateSeparator extends React.Component { {submitDebugLogsContent}

{_t("Error details")} - -
    -
  • - {_t("Request status code: %(statusCode)s", { - statusCode: err?.httpStatus || _t("HTTP status code not available"), - })} -
    • -
  • - {_t("Error code: %(errorCode)s", { - errorCode: err?.errcode || _t("Error code not available"), - })} -
    • -

{String(err)}

From 93d200f56beccd9933c30c0bceaf0946539d526b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 00:25:13 -0500 Subject: [PATCH 10/29] Generalize modal problems into utilities --- src/Modal.tsx | 2 + .../structures/auth/ForgotPassword-test.tsx | 38 +++++---- .../views/messages/DateSeparator-test.tsx | 81 +++++++++++++++---- .../views/right_panel/UserInfo-test.tsx | 5 +- .../views/rooms/MessageComposer-test.tsx | 15 ++-- .../tabs/user/SessionManagerTab-test.tsx | 5 +- test/test-utils/utilities.ts | 35 ++++++++ .../models/VoiceBroadcastPlayback-test.tsx | 13 ++- 8 files changed, 144 insertions(+), 50 deletions(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index 4710b3b9549..406445f9f56 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -352,6 +352,8 @@ export class ModalManager extends TypedEventEmitter { + // TODO: We should 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); diff --git a/test/components/structures/auth/ForgotPassword-test.tsx b/test/components/structures/auth/ForgotPassword-test.tsx index 5a8dbc2359b..c193342a41c 100644 --- a/test/components/structures/auth/ForgotPassword-test.tsx +++ b/test/components/structures/auth/ForgotPassword-test.tsx @@ -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", () => ({ @@ -55,11 +60,6 @@ describe("", () => { }); }; - const waitForDialog = async (): Promise => { - 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(); @@ -88,9 +88,9 @@ describe("", () => { jest.spyOn(AutoDiscoveryUtils, "authComponentStateForError"); }); - afterEach(() => { + afterEach(async () => { // clean up modals - Modal.closeCurrentModal("force"); + await clearAllModals(); }); beforeAll(() => { @@ -322,7 +322,9 @@ describe("", () => { 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", () => { @@ -350,7 +352,9 @@ describe("", () => { await act(async () => { await userEvent.click(screen.getByTestId("dialog-background"), { delay: null }); }); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); itShouldCloseTheDialogAndShowThePasswordInput(); @@ -359,7 +363,9 @@ describe("", () => { describe("and dismissing the dialog", () => { beforeEach(async () => { await click(screen.getByLabelText("Close dialog")); - await waitForDialog(); + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); }); itShouldCloseTheDialogAndShowThePasswordInput(); @@ -368,7 +374,9 @@ describe("", () => { 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", () => { @@ -400,7 +408,9 @@ describe("", () => { 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 () => { diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index 5897ead6630..de202702ce4 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -28,9 +28,13 @@ import { formatFullDateNoTime } from "../../../../src/DateUtils"; import SettingsStore from "../../../../src/settings/SettingsStore"; import { UIFeature } from "../../../../src/settings/UIFeature"; import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; -import { flushPromisesWithFakeTimers, getMockClientWithEventEmitter } from "../../../test-utils"; +import { + clearAllModals, + flushPromisesWithFakeTimers, + getMockClientWithEventEmitter, + waitEnoughCyclesForModal, +} from "../../../test-utils"; import DateSeparator from "../../../../src/components/views/messages/DateSeparator"; -import Modal from "../../../../src/Modal"; jest.mock("../../../../src/settings/SettingsStore"); @@ -132,14 +136,7 @@ describe("DateSeparator", () => { }); afterEach(async () => { - // Prevent modals from leaking and polluting other tests - let keepClosingModals = true; - while (keepClosingModals) { - keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)"); - } - - // Then wait for the screen to update (probably React rerender) - await flushPromisesWithFakeTimers(); + await clearAllModals(); }); it("renders the date separator correctly", () => { @@ -178,12 +175,66 @@ describe("DateSeparator", () => { } satisfies ViewRoomPayload); }); - it("should not jump to date if we already switched to another room (simulate slow network request)", async () => { - // TODO + it("should not jump to date if we already switched to another room", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Mimic the outcome of switching rooms while waiting for the jump to date + // request to finish. Imagine that we started jumping to "last week", the + // network request is taking a while, so we got bored, switched rooms; we + // shouldn't jump back to the previous room after the network request + // happens to finish later. + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!some-other-room"); + + // Jump to "last week" + mockClient.timestampToEvent.mockResolvedValue({ + event_id: "$abc", + origin_server_ts: "0", + }); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Flush out the dispatcher which uses `window.setTimeout(...)` since we're + // using `jest.useFakeTimers()` in these tests. + await flushPromisesWithFakeTimers(); + + // We should not see any room switching going on (`Action.ViewRoom`) + expect(dispatcher.dispatch).not.toHaveBeenCalled(); }); - it("should not show jump to date error if we already switched to another room (simulate slow network request)", async () => { - // TODO + it("should not show jump to date error if we already switched to another room", async () => { + // Render the component + getComponent(); + + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); + + // Mimic the outcome of switching rooms while waiting for the jump to date + // request to finish. Imagine that we started jumping to "last week", the + // network request is taking a while, so we got bored, switched rooms; we + // shouldn't jump back to the previous room after the network request + // happens to finish later. + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!some-other-room"); + + // Try to jump to "last week" but we want an error to occur and ensure that + // we don't show an error dialog for it since we already switched away to + // another room and don't care about the outcome here anymore. + mockClient.timestampToEvent.mockRejectedValue(new Error("Fake error in test")); + const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); + fireEvent.click(jumpToLastWeekButton); + + // Wait the necessary time in order to see if any modal will appear + await waitEnoughCyclesForModal({ + useFakeTimers: true, + }); + + // We should not see any error modal dialog + // + // We have to use `queryBy` so that it can return `null` for something that does not exist. + expect(screen.queryByTestId("jump-to-date-error-content")).not.toBeInTheDocument(); }); it("should show error dialog with submit debug logs option when non-networking error occurs", async () => { @@ -225,8 +276,6 @@ describe("DateSeparator", () => { // Expect error to be shown. We have to wait for the UI to transition. await screen.findByTestId("jump-to-date-error-content"); - screen.debug(); - // The submit debug logs option should *NOT* be shown for network errors. // // We have to use `queryBy` so that it can return `null` for something that does not exist. diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 140cd14ffef..3722e995e18 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -18,6 +18,7 @@ import React from "react"; import { fireEvent, render, screen, waitFor, cleanup, act } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { mocked } from "jest-mock"; +import { clearAllModals } from "../../../test-utils"; import { Room, User, MatrixClient, RoomMember, MatrixEvent, EventType } from "matrix-js-sdk/src/matrix"; import { Phase, VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest"; import { DeviceTrustLevel, UserTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; @@ -417,7 +418,9 @@ describe("", () => { mockClient.setIgnoredUsers.mockClear(); }); - afterEach(() => Modal.closeCurrentModal("End of test")); + afterEach(async () => { + await clearAllModals(); + }); afterAll(() => { inviteSpy.mockRestore(); diff --git a/test/components/views/rooms/MessageComposer-test.tsx b/test/components/views/rooms/MessageComposer-test.tsx index 3ffe68424d2..d2a6e9c12ee 100644 --- a/test/components/views/rooms/MessageComposer-test.tsx +++ b/test/components/views/rooms/MessageComposer-test.tsx @@ -21,6 +21,7 @@ import { act, render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { + clearAllModals, createTestClient, filterConsole, flushPromises, @@ -28,6 +29,7 @@ import { mkStubRoom, mockPlatformPeg, stubClient, + waitEnoughCyclesForModal, } from "../../../test-utils"; import MessageComposer from "../../../../src/components/views/rooms/MessageComposer"; import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; @@ -48,7 +50,6 @@ import { Action } from "../../../../src/dispatcher/actions"; import { VoiceBroadcastInfoState, VoiceBroadcastRecording } from "../../../../src/voice-broadcast"; import { mkVoiceBroadcastInfoStateEvent } from "../../../voice-broadcast/utils/test-utils"; import { SdkContextClass } from "../../../../src/contexts/SDKContext"; -import Modal from "../../../../src/Modal"; jest.mock("../../../../src/components/views/rooms/wysiwyg_composer", () => ({ SendWysiwygComposer: jest.fn().mockImplementation(() =>
), @@ -77,15 +78,9 @@ const setCurrentBroadcastRecording = (room: Room, state: VoiceBroadcastInfoState SdkContextClass.instance.voiceBroadcastRecordingsStore.setCurrent(recording); }; -const waitForModal = async (): Promise => { - await flushPromises(); - await flushPromises(); -}; - const shouldClearModal = async (): Promise => { afterEach(async () => { - Modal.closeCurrentModal("force"); - await waitForModal(); + await clearAllModals(); }); }; @@ -434,7 +429,7 @@ describe("MessageComposer", () => { setCurrentBroadcastRecording(room, VoiceBroadcastInfoState.Started); wrapAndRender({ room }); await startVoiceMessage(); - await waitForModal(); + await waitEnoughCyclesForModal(); }); shouldClearModal(); @@ -450,7 +445,7 @@ describe("MessageComposer", () => { setCurrentBroadcastRecording(room, VoiceBroadcastInfoState.Stopped); wrapAndRender({ room }); await startVoiceMessage(); - await waitForModal(); + await waitEnoughCyclesForModal(); }); shouldClearModal(); diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 83589225272..fcfca9e3039 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -31,6 +31,7 @@ import { IAuthData, } from "matrix-js-sdk/src/matrix"; +import { clearAllModals } from "../../../../../test-utils"; import SessionManagerTab from "../../../../../../src/components/views/settings/tabs/user/SessionManagerTab"; import MatrixClientContext from "../../../../../../src/contexts/MatrixClientContext"; import { @@ -161,7 +162,7 @@ describe("", () => { await flushPromises(); }; - beforeEach(() => { + beforeEach(async () => { jest.clearAllMocks(); jest.spyOn(logger, "error").mockRestore(); mockClient.getStoredDevice.mockImplementation((_userId, id) => { @@ -199,7 +200,7 @@ describe("", () => { // sometimes a verification modal is in modal state when these tests run // make sure the coast is clear - Modal.closeCurrentModal(""); + await clearAllModals(); }); it("renders spinner while devices load", () => { diff --git a/test/test-utils/utilities.ts b/test/test-utils/utilities.ts index 2549b5971e8..baf0f84468b 100644 --- a/test/test-utils/utilities.ts +++ b/test/test-utils/utilities.ts @@ -19,6 +19,7 @@ import EventEmitter from "events"; import { ActionPayload } from "../../src/dispatcher/payloads"; import defaultDispatcher from "../../src/dispatcher/dispatcher"; import { DispatcherAction } from "../../src/dispatcher/actions"; +import Modal from "../../src/Modal"; export const emitPromise = (e: EventEmitter, k: string | symbol) => new Promise((r) => e.once(k, r)); @@ -174,3 +175,37 @@ export const advanceDateAndTime = (ms: number) => { jest.spyOn(global.Date, "now").mockReturnValue(Date.now() + ms); jest.advanceTimersByTime(ms); }; + +/** + * A horrible hack necessary to wait enough time to ensure any modal is shown after a + * `Modal.createDialog(...)` call. We have to contend with the Modal code which renders + * things asyncronhously and has weird sleeps which we should strive to remove. + */ +export const waitEnoughCyclesForModal = async ({ + useFakeTimers = false, +}: { + useFakeTimers?: boolean; +} = {}): Promise => { + const flushFunc = useFakeTimers ? flushPromisesWithFakeTimers : flushPromises; + + await flushFunc(); + await flushFunc(); + await flushFunc(); +}; + +/** + * A horrible hack necessary to make sure modals don't leak and pollute tests. + * `@testing-library/react` automatic cleanup function does not pick up the async modal + * rendering and the modals don't unmount when the component unmounts. We should strive + * to fix this. + */ +export const clearAllModals = async (): Promise => { + // Prevent modals from leaking and polluting other tests + let keepClosingModals = true; + while (keepClosingModals) { + keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)"); + } + + // Then wait for the screen to update (probably React rerender) + await flushPromisesWithFakeTimers(); +}; diff --git a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx index 9f59ba6369e..ba87df2e2e8 100644 --- a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx +++ b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx @@ -34,7 +34,11 @@ import { } from "../../../src/voice-broadcast"; import { filterConsole, flushPromises, flushPromisesWithFakeTimers, stubClient } from "../../test-utils"; import { createTestPlayback } from "../../test-utils/audio"; -import { mkVoiceBroadcastChunkEvent, mkVoiceBroadcastInfoStateEvent } from "../utils/test-utils"; +import { + mkVoiceBroadcastChunkEvent, + mkVoiceBroadcastInfoStateEvent, + waitEnoughCyclesForModal, +} from "../utils/test-utils"; import { LazyValue } from "../../../src/utils/LazyValue"; jest.mock("../../../src/utils/MediaEventHelper", () => ({ @@ -77,11 +81,6 @@ describe("VoiceBroadcastPlayback", () => { ); }; - const waitForDialog = async () => { - await flushPromises(); - await flushPromises(); - }; - const itShouldSetTheStateTo = (state: VoiceBroadcastPlaybackState) => { it(`should set the state to ${state}`, () => { expect(playback.getState()).toBe(state); @@ -480,7 +479,7 @@ describe("VoiceBroadcastPlayback", () => { jest.spyOn(recording, "stop"); SdkContextClass.instance.voiceBroadcastRecordingsStore.setCurrent(recording); playback.start(); - await waitForDialog(); + await waitEnoughCyclesForModal(); }); it("should display a confirm modal", () => { From 1c724e19a2d48337cd2823157f116e853e3b9eb3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 00:27:57 -0500 Subject: [PATCH 11/29] Remove currentDate --- test/components/views/messages/DateSeparator-test.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index de202702ce4..910a9b56bc1 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -124,7 +124,6 @@ describe("DateSeparator", () => { }); describe("when feature_jump_to_date is enabled", () => { - const currentDate = new Date("2023-03-05"); beforeEach(() => { jest.clearAllMocks(); mocked(SettingsStore).getValue.mockImplementation((arg): any => { @@ -153,7 +152,7 @@ describe("DateSeparator", () => { // Jump to "last week" const lastWeekDate = new Date(); - lastWeekDate.setDate(currentDate.getDate() - 7); + lastWeekDate.setDate(nowDate.getDate() - 7); const returnedEventId = "$abc"; mockClient.timestampToEvent.mockResolvedValue({ event_id: returnedEventId, From df2b4d220806da438b4823aad641c32194ad33aa Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 00:33:14 -0500 Subject: [PATCH 12/29] Better language --- src/Modal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index 406445f9f56..326775e6324 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -352,7 +352,7 @@ export class ModalManager extends TypedEventEmitter { - // TODO: We should remove this weird sleep. It also makes testing harder + // 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); From 8c4258199acda557bd1708ae75cfec2b4bc7073f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 00:43:56 -0500 Subject: [PATCH 13/29] Fix some imports --- test/components/views/right_panel/UserInfo-test.tsx | 3 +-- .../models/VoiceBroadcastPlayback-test.tsx | 12 +++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/components/views/right_panel/UserInfo-test.tsx b/test/components/views/right_panel/UserInfo-test.tsx index 3722e995e18..7ca7daa8f22 100644 --- a/test/components/views/right_panel/UserInfo-test.tsx +++ b/test/components/views/right_panel/UserInfo-test.tsx @@ -18,7 +18,6 @@ import React from "react"; import { fireEvent, render, screen, waitFor, cleanup, act } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { mocked } from "jest-mock"; -import { clearAllModals } from "../../../test-utils"; import { Room, User, MatrixClient, RoomMember, MatrixEvent, EventType } from "matrix-js-sdk/src/matrix"; import { Phase, VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest"; import { DeviceTrustLevel, UserTrustLevel } from "matrix-js-sdk/src/crypto/CrossSigning"; @@ -46,7 +45,7 @@ import * as mockVerification from "../../../../src/verification"; import Modal from "../../../../src/Modal"; import { E2EStatus } from "../../../../src/utils/ShieldUtils"; import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages"; -import { flushPromises } from "../../../test-utils"; +import { clearAllModals, flushPromises } from "../../../test-utils"; jest.mock("../../../../src/utils/direct-messages", () => ({ ...jest.requireActual("../../../../src/utils/direct-messages"), diff --git a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx index ba87df2e2e8..9ee165d87af 100644 --- a/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx +++ b/test/voice-broadcast/models/VoiceBroadcastPlayback-test.tsx @@ -32,13 +32,15 @@ import { VoiceBroadcastPlaybackState, VoiceBroadcastRecording, } from "../../../src/voice-broadcast"; -import { filterConsole, flushPromises, flushPromisesWithFakeTimers, stubClient } from "../../test-utils"; -import { createTestPlayback } from "../../test-utils/audio"; import { - mkVoiceBroadcastChunkEvent, - mkVoiceBroadcastInfoStateEvent, + filterConsole, + flushPromises, + flushPromisesWithFakeTimers, + stubClient, waitEnoughCyclesForModal, -} from "../utils/test-utils"; +} from "../../test-utils"; +import { createTestPlayback } from "../../test-utils/audio"; +import { mkVoiceBroadcastChunkEvent, mkVoiceBroadcastInfoStateEvent } from "../utils/test-utils"; import { LazyValue } from "../../../src/utils/LazyValue"; jest.mock("../../../src/utils/MediaEventHelper", () => ({ From bac5a89ddeae970e044600035a8245098f8790fd Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 00:45:14 -0500 Subject: [PATCH 14/29] Type error --- src/components/views/messages/DateSeparator.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 3cbf0ff42ae..6dd0bff1c02 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -228,7 +228,7 @@ export default class DateSeparator extends React.Component { } }; - private onBugReport = (err): void => { + private onBugReport = (err: Error): void => { Modal.createDialog(BugReportDialog, { error: err, initialText: "Error occured while using jump to date #jump-to-date", From 78cbc06ed929b151e498b6aa087c8df96b7c4274 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 01:01:44 -0500 Subject: [PATCH 15/29] Try to type errors --- src/components/views/messages/DateSeparator.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 6dd0bff1c02..2284d3d6662 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -18,6 +18,7 @@ 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 { formatFullDateNoDay, formatFullDateNoTime } from "../../../DateUtils"; @@ -168,8 +169,8 @@ export default class DateSeparator extends React.Component { if (currentRoomId === roomIdForJumpRequest) { let friendlyErrorMessage = `An error occured while trying to find and jump to the given date.`; let submitDebugLogsContent: JSX.Element = <>; - if (err?.name === "ConnectionError" || err?.httpStatus) { - if (err?.errcode === "M_NOT_FOUND") { + if (err instanceof ConnectionError || err instanceof HTTPError) { + if (err instanceof MatrixError && err?.errcode === "M_NOT_FOUND") { friendlyErrorMessage = _t( "We were unable to find an event looking forwards from %(dateString)s. " + "Try choosing an earlier date.", @@ -199,7 +200,7 @@ export default class DateSeparator extends React.Component { // this to a be a inline anchor element. element="a" kind="link" - onClick={() => this.onBugReport(err)} + onClick={() => this.onBugReport(err instanceof Error ? err : undefined)} data-testid="jump-to-date-error-submit-debug-logs-button" > {sub} @@ -228,7 +229,7 @@ export default class DateSeparator extends React.Component { } }; - private onBugReport = (err: Error): void => { + private onBugReport = (err?: Error): void => { Modal.createDialog(BugReportDialog, { error: err, initialText: "Error occured while using jump to date #jump-to-date", From 5ca2bb417a2801fb459fd1817f0239fc01ee7354 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 01:04:40 -0500 Subject: [PATCH 16/29] Fix event type --- src/components/views/messages/DateSeparator.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 2284d3d6662..ffe6a5913f4 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -30,7 +30,7 @@ import { UIFeature } from "../../../settings/UIFeature"; import Modal from "../../../Modal"; import ErrorDialog from "../dialogs/ErrorDialog"; import BugReportDialog from "../dialogs/BugReportDialog"; -import AccessibleButton from "../elements/AccessibleButton"; +import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton"; import { contextMenuBelow } from "../rooms/RoomTile"; import { ContextMenuTooltipButton } from "../../structures/ContextMenu"; import IconizedContextMenu, { @@ -80,7 +80,7 @@ export default class DateSeparator extends React.Component { 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; From 6e921da9fbfe34e433f0b4fbd626babdb5bb64fc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 01:05:21 -0500 Subject: [PATCH 17/29] Update translations --- src/i18n/strings/en_EN.json | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index f5918eb61e4..fad6199ec89 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2355,10 +2355,6 @@ "Please submit debug logs to help us track down the problem.": "Please submit debug logs 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", - "Request status code: %(statusCode)s": "Request status code: %(statusCode)s", - "HTTP status code not available": "HTTP status code not available", - "Error code: %(errorCode)s": "Error code: %(errorCode)s", - "Error code not available": "Error code not available", "Last week": "Last week", "Last month": "Last month", "The beginning of the room": "The beginning of the room", From 7079c69a3c76725b8af691f9f4cc180866309e0b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 01:18:10 -0500 Subject: [PATCH 18/29] Remove cruft --- src/Modal.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index 326775e6324..c92741cfc6d 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -150,7 +150,6 @@ export class ModalManager extends TypedEventEmitter Date: Tue, 21 Mar 2023 02:10:23 -0500 Subject: [PATCH 19/29] Fix test/components/structures/auth/ForgotPassword-test.tsx running forever --- test/test-utils/utilities.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test-utils/utilities.ts b/test/test-utils/utilities.ts index baf0f84468b..530db5dfdbe 100644 --- a/test/test-utils/utilities.ts +++ b/test/test-utils/utilities.ts @@ -204,8 +204,8 @@ export const clearAllModals = async (): Promise => { let keepClosingModals = true; while (keepClosingModals) { keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)"); - } - // Then wait for the screen to update (probably React rerender) - await flushPromisesWithFakeTimers(); + // Then wait for the screen to update (probably React rerender) + await flushPromisesWithFakeTimers(); + } }; From d3b07901e59b5edf42a950aa15ca8699596846f5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 12:41:08 -0500 Subject: [PATCH 20/29] Fix export --- test/components/views/messages/DateSeparator-test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index 910a9b56bc1..a3adeaff6cf 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -17,7 +17,7 @@ limitations under the License. import React from "react"; import { mocked } from "jest-mock"; import { fireEvent, render, screen } from "@testing-library/react"; -import { ITimestampToEventResponse } from "matrix-js-sdk/src/client"; +import { TimestampToEventResponse } from "matrix-js-sdk/src/client"; import { ConnectionError, HTTPError } from "matrix-js-sdk/src/http-api"; import dispatcher from "../../../../src/dispatcher/dispatcher"; @@ -157,7 +157,7 @@ describe("DateSeparator", () => { mockClient.timestampToEvent.mockResolvedValue({ event_id: returnedEventId, origin_server_ts: String(lastWeekDate.getTime()), - } satisfies ITimestampToEventResponse); + } satisfies TimestampToEventResponse); const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); fireEvent.click(jumpToLastWeekButton); From 5fefd647c630815901c3f1bc68494b3047d29bbe Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 21 Mar 2023 13:41:41 -0500 Subject: [PATCH 21/29] Add better comment --- test/test-utils/utilities.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test-utils/utilities.ts b/test/test-utils/utilities.ts index 530db5dfdbe..2df6afea045 100644 --- a/test/test-utils/utilities.ts +++ b/test/test-utils/utilities.ts @@ -205,7 +205,9 @@ export const clearAllModals = async (): Promise => { while (keepClosingModals) { keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)"); - // Then wait for the screen to update (probably React rerender) + // Then wait for the screen to update (probably React rerender and async/await). + // Important for tests using Jest fake timers to not get into an infinite loop + // of removing the same modal because the promises don't flush otherwise. await flushPromisesWithFakeTimers(); } }; From d901c9070a0682e85c895ce4e1aa6a6baa3950fe Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Mar 2023 15:00:12 -0500 Subject: [PATCH 22/29] No string interpolation needed See https://github.com/matrix-org/matrix-react-sdk/pull/10405#discussion_r1144454466 --- src/components/views/messages/DateSeparator.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index ffe6a5913f4..f4ddbcc38e4 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -167,7 +167,7 @@ export default class DateSeparator extends React.Component { // 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 friendlyErrorMessage = "An error occured while trying to find and jump to the given date."; let submitDebugLogsContent: JSX.Element = <>; if (err instanceof ConnectionError || err instanceof HTTPError) { if (err instanceof MatrixError && err?.errcode === "M_NOT_FOUND") { From 9cecaa9e035e63194db199fdc6312861043ed400 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Mar 2023 15:08:16 -0500 Subject: [PATCH 23/29] Better language See https://github.com/matrix-org/matrix-react-sdk/pull/10405#discussion_r1144462578 --- src/components/views/messages/DateSeparator.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index f4ddbcc38e4..979241c4a1c 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -179,8 +179,8 @@ export default class DateSeparator extends React.Component { } else { friendlyErrorMessage = _t( "A network error occurred while trying to find and jump to the given date. " + - "Your homeserver might be down or was just a temporary problem with your " + - "internet connection. Please try again. If this continues, please " + + "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.", ); } From 52e6f30963fb8f94313c3e021b08e18ae42415be Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Mar 2023 15:26:06 -0500 Subject: [PATCH 24/29] Be more specific when we show the network problem language --- .../views/messages/DateSeparator.tsx | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 979241c4a1c..89f50615077 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -169,21 +169,28 @@ export default class DateSeparator extends React.Component { 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 || err instanceof HTTPError) { - if (err instanceof MatrixError && err?.errcode === "M_NOT_FOUND") { + 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( - "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.", - ); + 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 = ( From 090221569267ffb733cf0b73dc0d8193b2be6e4f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Mar 2023 15:35:40 -0500 Subject: [PATCH 25/29] Add expects to show intention See https://github.com/matrix-org/matrix-react-sdk/pull/10405#discussion_r1144483180 --- test/components/views/messages/DateSeparator-test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index a3adeaff6cf..88ed0e03052 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -250,10 +250,10 @@ describe("DateSeparator", () => { fireEvent.click(jumpToLastWeekButton); // Expect error to be shown. We have to wait for the UI to transition. - await screen.findByTestId("jump-to-date-error-content"); + expect(await screen.findByTestId("jump-to-date-error-content")).toBeInTheDocument(); // Expect an option to submit debug logs to be shown when a non-network error occurs - await screen.findByTestId("jump-to-date-error-submit-debug-logs-button"); + expect(await screen.findByTestId("jump-to-date-error-submit-debug-logs-button")).toBeInTheDocument(); }); [ @@ -273,7 +273,7 @@ describe("DateSeparator", () => { fireEvent.click(jumpToLastWeekButton); // Expect error to be shown. We have to wait for the UI to transition. - await screen.findByTestId("jump-to-date-error-content"); + expect(await screen.findByTestId("jump-to-date-error-content")).toBeInTheDocument(); // The submit debug logs option should *NOT* be shown for network errors. // From 2aa7f915641f8d5bff4cd6fa057b77f5af9536c5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Mar 2023 15:43:12 -0500 Subject: [PATCH 26/29] Add comment about potential better future See https://github.com/matrix-org/matrix-react-sdk/pull/10405#discussion_r1144487635 --- test/test-utils/utilities.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/test-utils/utilities.ts b/test/test-utils/utilities.ts index 2df6afea045..420e5d3bca2 100644 --- a/test/test-utils/utilities.ts +++ b/test/test-utils/utilities.ts @@ -186,6 +186,7 @@ export const waitEnoughCyclesForModal = async ({ }: { useFakeTimers?: boolean; } = {}): Promise => { + // XXX: Maybe in the future with Jest 29.5.0+, we could use `runAllTimersAsync` instead. const flushFunc = useFakeTimers ? flushPromisesWithFakeTimers : flushPromises; await flushFunc(); @@ -208,6 +209,8 @@ export const clearAllModals = async (): Promise => { // Then wait for the screen to update (probably React rerender and async/await). // Important for tests using Jest fake timers to not get into an infinite loop // of removing the same modal because the promises don't flush otherwise. + // + // XXX: Maybe in the future with Jest 29.5.0+, we could use `runAllTimersAsync` instead. await flushPromisesWithFakeTimers(); } }; From 7c4a398916076a7bba8fbe86515ecce7da83e3fa Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 22 Mar 2023 23:26:48 -0500 Subject: [PATCH 27/29] Update i18n --- src/i18n/strings/en_EN.json | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index a7d34fcc920..f588822ce67 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2352,8 +2352,11 @@ "Saturday": "Saturday", "Today": "Today", "Yesterday": "Yesterday", + "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.", - "A network error occurred while trying to find and jump to the given date. Your homeserver might be down or 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 was just a temporary problem with your internet connection. Please try again. If this continues, please contact your homeserver administrator.", + "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 debug logs to help us track down the problem.": "Please submit debug logs 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", From 23268af2e4b111200c301d2e42c54ead1f1d9fc0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 23 Mar 2023 13:37:29 -0500 Subject: [PATCH 28/29] Test other date picks --- .../views/messages/DateSeparator.tsx | 7 +- .../views/messages/DateSeparator-test.tsx | 71 ++++++++++++------- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/src/components/views/messages/DateSeparator.tsx b/src/components/views/messages/DateSeparator.tsx index 89f50615077..7f2b5afc880 100644 --- a/src/components/views/messages/DateSeparator.tsx +++ b/src/components/views/messages/DateSeparator.tsx @@ -283,10 +283,15 @@ export default class DateSeparator extends React.Component { onClick={this.onLastWeekClicked} data-testid="jump-to-date-last-week" /> - + diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index 88ed0e03052..ad039d1fe5c 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -143,35 +143,52 @@ describe("DateSeparator", () => { expect(asFragment()).toMatchSnapshot(); }); - it("can jump to last week", async () => { - // Render the component - getComponent(); - - // Open the jump to date context menu - fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); - - // Jump to "last week" - const lastWeekDate = new Date(); - lastWeekDate.setDate(nowDate.getDate() - 7); - const returnedEventId = "$abc"; - mockClient.timestampToEvent.mockResolvedValue({ - event_id: returnedEventId, - origin_server_ts: String(lastWeekDate.getTime()), - } satisfies TimestampToEventResponse); - const jumpToLastWeekButton = await screen.findByTestId("jump-to-date-last-week"); - fireEvent.click(jumpToLastWeekButton); + [ + { + timeDescriptor: "last week", + jumpButtonTestId: "jump-to-date-last-week", + }, + { + timeDescriptor: "last month", + jumpButtonTestId: "jump-to-date-last-month", + }, + { + timeDescriptor: "the beginning", + jumpButtonTestId: "jump-to-date-beginning", + }, + ].forEach((testCase) => { + it(`can jump to ${testCase.timeDescriptor}`, async () => { + // Render the component + getComponent(); - // Flush out the dispatcher which uses `window.setTimeout(...)` since we're - // using `jest.useFakeTimers()` in these tests. - await flushPromisesWithFakeTimers(); + // Open the jump to date context menu + fireEvent.click(screen.getByTestId("jump-to-date-separator-button")); - expect(dispatcher.dispatch).toHaveBeenCalledWith({ - action: Action.ViewRoom, - event_id: returnedEventId, - highlighted: true, - room_id: roomId, - metricsTrigger: undefined, - } satisfies ViewRoomPayload); + // Jump to "x" + const returnedDate = new Date(); + // Just an arbitrary date before "now" + returnedDate.setDate(nowDate.getDate() - 100); + const returnedEventId = "$abc"; + mockClient.timestampToEvent.mockResolvedValue({ + event_id: returnedEventId, + origin_server_ts: String(returnedDate.getTime()), + } satisfies TimestampToEventResponse); + const jumpToXButton = await screen.findByTestId(testCase.jumpButtonTestId); + fireEvent.click(jumpToXButton); + + // Flush out the dispatcher which uses `window.setTimeout(...)` since we're + // using `jest.useFakeTimers()` in these tests. + await flushPromisesWithFakeTimers(); + + // Ensure that we're jumping to the event at the given date + expect(dispatcher.dispatch).toHaveBeenCalledWith({ + action: Action.ViewRoom, + event_id: returnedEventId, + highlighted: true, + room_id: roomId, + metricsTrigger: undefined, + } satisfies ViewRoomPayload); + }); }); it("should not jump to date if we already switched to another room", async () => { From 1447b76891ddf8a2eb7d041c2145368bdd8116a6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 23 Mar 2023 13:41:34 -0500 Subject: [PATCH 29/29] Add test for MatrixError --- test/components/views/messages/DateSeparator-test.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/components/views/messages/DateSeparator-test.tsx b/test/components/views/messages/DateSeparator-test.tsx index ad039d1fe5c..ff00cf44e94 100644 --- a/test/components/views/messages/DateSeparator-test.tsx +++ b/test/components/views/messages/DateSeparator-test.tsx @@ -18,7 +18,7 @@ import React from "react"; import { mocked } from "jest-mock"; import { fireEvent, render, screen } from "@testing-library/react"; import { TimestampToEventResponse } from "matrix-js-sdk/src/client"; -import { ConnectionError, HTTPError } from "matrix-js-sdk/src/http-api"; +import { ConnectionError, HTTPError, MatrixError } from "matrix-js-sdk/src/http-api"; import dispatcher from "../../../../src/dispatcher/dispatcher"; import { Action } from "../../../../src/dispatcher/actions"; @@ -275,7 +275,12 @@ describe("DateSeparator", () => { [ new ConnectionError("Fake connection error in test"), - new HTTPError("Fake connection error in test", 418), + new HTTPError("Fake http error in test", 418), + new MatrixError( + { errcode: "M_FAKE_ERROR_CODE", error: "Some fake error occured" }, + 518, + "https://fake-url/", + ), ].forEach((fakeError) => { it(`should show error dialog without submit debug logs option when networking error (${fakeError.name}) occurs`, async () => { // Render the component