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

Provide a more detailed error message than "No known servers" #6048

Merged
merged 23 commits into from
Feb 24, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
efd793c
Provide a more detailed error message than "No known servers"
aaronraimist May 17, 2021
513300a
Merge branch 'develop' into no-known-servers
aaronraimist Jun 23, 2021
bcb12af
Merge branch 'develop' into no-known-servers
aaronraimist Jun 29, 2021
9b33261
Fix PR since file was refactored
aaronraimist Jun 29, 2021
3f4ee2f
Fix formatting
aaronraimist Jun 29, 2021
72f9d7a
Merge branch 'develop' into no-known-servers
aaronraimist Jan 23, 2022
b6aa7e6
lint
aaronraimist Jan 25, 2022
bfb5707
Merge branch 'develop' into no-known-servers
aaronraimist Jan 25, 2022
170da1b
Update src/stores/RoomViewStore.tsx
aaronraimist Jan 26, 2022
26f974d
Merge branch 'develop' into no-known-servers
aaronraimist Jul 2, 2022
c58fe0b
Add example identifiers and a more detailed explanation
aaronraimist Jul 3, 2022
0c52759
Lint
aaronraimist Jul 3, 2022
0b3f050
Lint
aaronraimist Jul 3, 2022
78eb743
Merge branch 'develop' into no-known-servers
aaronraimist Feb 21, 2023
c72ac81
Revert back to original wording (except s/alias/address)
aaronraimist Feb 21, 2023
2791011
Prettier
aaronraimist Feb 21, 2023
030ced3
Merge branch 'develop' into no-known-servers
florianduros Feb 23, 2023
6c72541
Fix ts error
florianduros Feb 23, 2023
19ad1a2
Add snapshot test
florianduros Feb 23, 2023
ce8c0d9
Check the Modal props
florianduros Feb 23, 2023
a99f514
Add test case to reach quality gate
florianduros Feb 23, 2023
b2b4732
Merge branch 'develop' into no-known-servers
florianduros Feb 24, 2023
e3982da
Merge branch 'develop' into no-known-servers
florianduros Feb 24, 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
2 changes: 2 additions & 0 deletions src/i18n/strings/en_EN.json
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,8 @@
"Please contact your homeserver administrator.": "Please contact your homeserver administrator.",
"The person who invited you has already left.": "The person who invited you has already left.",
"The person who invited you has already left, or their server is offline.": "The person who invited you has already left, or their server is offline.",
"You attempted to join using a room ID without providing a list of servers to join through. Room IDs are internal identifiers and cannot be used to join a room without additional information.": "You attempted to join using a room ID without providing a list of servers to join through. Room IDs are internal identifiers and cannot be used to join a room without additional information.",
"If you know a room address, try joining through that instead.": "If you know a room address, try joining through that instead.",
"Failed to join": "Failed to join",
"Connection lost": "Connection lost",
"You were disconnected from the call. (Error: %(message)s)": "You were disconnected from the call. (Error: %(message)s)",
Expand Down
23 changes: 21 additions & 2 deletions src/stores/RoomViewStore.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ export class RoomViewStore extends EventEmitter {
);
} else if (err.httpStatus === 404) {
const invitingUserId = this.getInvitingUserId(roomId);
// only provide a better error message for invites
// provide a better error message for invites
if (invitingUserId) {
// if the inviting user is on the same HS, there can only be one cause: they left.
if (invitingUserId.endsWith(`:${MatrixClientPeg.get().getDomain()}`)) {
Expand All @@ -602,6 +602,23 @@ export class RoomViewStore extends EventEmitter {
description = _t("The person who invited you has already left, or their server is offline.");
}
}

// provide a more detailed error than "No known servers" when attempting to
// join using a room ID and no via servers
if (roomId === this.state.roomId && this.state.viaServers.length === 0) {
aaronraimist marked this conversation as resolved.
Show resolved Hide resolved
description = (
<div>
{_t(
"You attempted to join using a room ID without providing a list " +
"of servers to join through. Room IDs are internal identifiers and " +
"cannot be used to join a room without additional information.",
)}
<br />
<br />
{_t("If you know a room address, try joining through that instead.")}
</div>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to https://github.com/vector-im/element-web/issues/24475

Maybe some language inspiration in this:

Failed to join

The !JsbthXjlaqosxISjgl:gitter.im room isn't known to your homeserver (matrix.org).

You can try using a "via" server to join through which is supplied as the 2nd parameter in the /join slash command: /join !JsbthXjlaqosxISjgl:gitter.im gitter.im

Or if you're using a matrix.to link, you can try adding a ?via=gitter.im query parameter to the URL: https://matrix.to/#/!JsbthXjlaqosxISjgl:gitter.im?via=gitter.im

Alternatively, if you know a room alias (starts with a #) associated with this room, that will always be routable and allow you to join.

Error details (expand/collapse)

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I feel like using an alias is the more user friendly option and should be suggested first (move up to 2nd paragraph).
  2. If unavailable, the next best option is to have whoever provided the alias provide a matrix.to with vias (automatically, by using their client's share or "copy room link" function).
  3. Then the more advanced stuff about the join command can follow.

Copy link
Contributor

@MadLittleMods MadLittleMods Feb 9, 2023

Choose a reason for hiding this comment

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

Not all rooms have an alias and you can stumble upon a room ID in an unlimited number of ways. The user obviously tried to use a room ID here and we should make it possible for them to continue with what they have.

Leaving the user helpless to go ask someone else for a room alias isn't very empowering.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any data to argue that point, except I think the user probably doesn't have that choice and can at best make a very educated guess.

However I do disagree with your previous suggestion:

You can try using a "via" server to join through which is supplied as the 2nd parameter in the /join slash command: /join !JsbthXjlaqosxISjgl:gitter.im gitter.im

Honestly, either

  • there is no way to progress at this point, because how are you supposed to guess a via server when the room ID does not hint at one (I think new IDs don't have them anymore?). Basically what I already said.
  • it is completely ridiculous to ask users to repeat part of the room ID (!room:domain.tld domain.tld) in some command instead of detecting and trying it automatically.

The best I can come up with is to "try using the server (latter part of matrix ID) or a user who you know is in the room as a via server)".

Copy link
Contributor

@MadLittleMods MadLittleMods Feb 9, 2023

Choose a reason for hiding this comment

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

(I think new IDs don't have them anymore?).

All room IDs include this information at the moment. And is why I created https://github.com/vector-im/element-web/issues/24475 to suggest to people how to do the right thing (derive the via server from the room ID)

it is completely ridiculous to ask users to repeat part of the room ID (!room:domain.tld domain.tld) in some command instead of detecting and trying it automatically.

Agreed that this should just be automatic, but I think we're at ideological odds with the decision makers since they would want to treat room IDs as opaque strings that shouldn't be parsed. Treating it as an opaque string is generally a good heuristic since there is some future-looking here where room IDs potentially won't include this information (wish I had some links for future desire).

The half-measure of suggesting the right thing at least gets them to the right place and teaches for next time. And I think has the potential to actually be merged.

}

Modal.createDialog(ErrorDialog, {
Expand All @@ -615,7 +632,9 @@ export class RoomViewStore extends EventEmitter {
joining: false,
joinError: payload.err,
});
this.showJoinRoomError(payload.err, payload.roomId);
if (payload.err) {
this.showJoinRoomError(payload.err, payload.roomId);
}
}

public reset(): void {
Expand Down
26 changes: 25 additions & 1 deletion test/stores/RoomViewStore-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { Room } from "matrix-js-sdk/src/matrix";
import { mocked } from "jest-mock";
import { MatrixError, Room } from "matrix-js-sdk/src/matrix";
import { sleep } from "matrix-js-sdk/src/utils";

import { RoomViewStore } from "../../src/stores/RoomViewStore";
Expand Down Expand Up @@ -284,6 +285,29 @@ describe("RoomViewStore", function () {
await viewCall();
});

it("should display an error message when the room is unreachable via the roomId", async () => {
// When
// View and wait for the room
dis.dispatch({ action: Action.ViewRoom, room_id: roomId });
await untilDispatch(Action.ActiveRoomChanged, dis);
// Generate error to display the expected error message
const error = new MatrixError(undefined, 404);
roomViewStore.showJoinRoomError(error, roomId);

// Check the modal props
expect(mocked(Modal).createDialog.mock.calls[0][1]).toMatchSnapshot();
});

it("should display the generic error message when the roomId doesnt match", async () => {
// When
// Generate error to display the expected error message
const error = new MatrixError({ error: "my 404 error" }, 404);
roomViewStore.showJoinRoomError(error, roomId);

// Check the modal props
expect(mocked(Modal).createDialog.mock.calls[0][1]).toMatchSnapshot();
});

describe("when listening to a voice broadcast", () => {
let voiceBroadcastPlayback: VoiceBroadcastPlayback;

Expand Down
19 changes: 19 additions & 0 deletions test/stores/__snapshots__/RoomViewStore-test.ts.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,24 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`RoomViewStore should display an error message when the room is unreachable via the roomId 1`] = `
{
"description": <div>
You attempted to join using a room ID without providing a list of servers to join through. Room IDs are internal identifiers and cannot be used to join a room without additional information.
<br />
<br />
If you know a room address, try joining through that instead.
</div>,
"title": "Failed to join",
}
`;

exports[`RoomViewStore should display the generic error message when the roomId doesnt match 1`] = `
{
"description": "MatrixError: [404] my 404 error",
"title": "Failed to join",
}
`;

exports[`RoomViewStore when recording a voice broadcast and trying to view a call, it should not actually view it and show the info dialog 1`] = `
[MockFunction] {
"calls": [
Expand Down