-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve rageshake upload experience by providing useful error information #29378
Open
Half-Shot
wants to merge
14
commits into
develop
Choose a base branch
from
hs/rageshake-errors
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+238
−28
Open
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
cdbc97c
Refactor submit rageshake so that it uses the new error codes.
Half-Shot a811cfc
Improve error information given in Bug Report Dialog
Half-Shot fc11699
use type
Half-Shot 05f5b34
Refactor with generic error & policy link.
Half-Shot f9c6fde
lint
Half-Shot e43a598
lint
Half-Shot 2533814
Add BugReportDialog test
Half-Shot bd597b3
fix time travel
Half-Shot 08cec85
use waitFor while waiting for fetch to finish
Half-Shot dd34c0a
Merge remote-tracking branch 'origin/develop' into hs/rageshake-errors
Half-Shot 66e5ca4
lint
Half-Shot 5435918
Drop error prefix as per https://github.com/matrix-org/rageshake/pull…
Half-Shot 894f9f2
small fixes
Half-Shot 1e801c2
Don't change string here.
Half-Shot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
132 changes: 132 additions & 0 deletions
132
test/unit-tests/components/views/dialogs/BugReportDialog-test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
/* | ||
Copyright 2025 New Vector Ltd. | ||
|
||
SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial | ||
Please see LICENSE files in the repository root for full details. | ||
*/ | ||
|
||
import { render, waitFor, type RenderResult } from "jest-matrix-react"; | ||
import userEvent from "@testing-library/user-event"; | ||
import React from "react"; | ||
import fetchMock from "fetch-mock-jest"; | ||
import { type Mocked } from "jest-mock"; | ||
|
||
import BugReportDialog, { | ||
type BugReportDialogProps, | ||
} from "../../../../../src/components/views/dialogs/BugReportDialog"; | ||
import SdkConfig from "../../../../../src/SdkConfig"; | ||
import { type ConsoleLogger } from "../../../../../src/rageshake/rageshake"; | ||
|
||
const BUG_REPORT_URL = "https://example.org/submit"; | ||
|
||
describe("BugReportDialog", () => { | ||
const onFinished: jest.Mock<any, any> = jest.fn(); | ||
|
||
function renderComponent(props: Partial<BugReportDialogProps> = {}): RenderResult { | ||
return render(<BugReportDialog onFinished={onFinished} />); | ||
} | ||
|
||
beforeEach(() => { | ||
jest.resetAllMocks(); | ||
SdkConfig.put({ | ||
bug_report_endpoint_url: BUG_REPORT_URL, | ||
}); | ||
|
||
const mockConsoleLogger = { | ||
flush: jest.fn(), | ||
consume: jest.fn(), | ||
warn: jest.fn(), | ||
} as unknown as Mocked<ConsoleLogger>; | ||
|
||
// @ts-ignore - mock the console logger | ||
global.mx_rage_logger = mockConsoleLogger; | ||
|
||
// @ts-ignore | ||
mockConsoleLogger.flush.mockReturnValue([ | ||
{ | ||
id: "instance-0", | ||
line: "line 1", | ||
}, | ||
{ | ||
id: "instance-1", | ||
line: "line 2", | ||
}, | ||
]); | ||
}); | ||
|
||
afterEach(() => { | ||
SdkConfig.reset(); | ||
fetchMock.restore(); | ||
}); | ||
|
||
it("can close the bug reporter", async () => { | ||
const { getByTestId } = renderComponent(); | ||
await userEvent.click(getByTestId("dialog-cancel-button")); | ||
expect(onFinished).toHaveBeenCalledWith(false); | ||
}); | ||
|
||
it("can submit a bug report", async () => { | ||
const { getByLabelText, getByText } = renderComponent(); | ||
fetchMock.postOnce(BUG_REPORT_URL, { report_url: "https://exmaple.org/report/url" }); | ||
await userEvent.type(getByLabelText("GitHub issue"), "https://example.org/some/issue"); | ||
await userEvent.type(getByLabelText("Notes"), "Additional text"); | ||
await userEvent.click(getByText("Send logs")); | ||
await waitFor(() => expect(getByText("Thank you!")).toBeInTheDocument()); | ||
expect(onFinished).toHaveBeenCalledWith(false); | ||
expect(fetchMock).toHaveFetched(BUG_REPORT_URL); | ||
}); | ||
|
||
it.each([ | ||
{ | ||
errcode: undefined, | ||
text: "The rageshake server encountered an unknown error and could not handle the report.", | ||
}, | ||
{ | ||
errcode: "CUSTOM_ERROR_TYPE", | ||
text: "The rageshake server encountered an unknown error and could not handle the report.", | ||
}, | ||
{ | ||
errcode: "DISALLOWED_APP", | ||
text: "Your bug report was rejected. The rageshake server does not support this application.", | ||
}, | ||
{ | ||
errcode: "REJECTED_BAD_VERSION", | ||
text: "Your bug report was rejected as the version you are running is too old.", | ||
}, | ||
{ | ||
errcode: "REJECTED_UNEXPECTED_RECOVERY_KEY", | ||
text: "Your bug report was rejected for safety reasons, as it contained a recovery key.", | ||
}, | ||
{ | ||
errcode: "REJECTED_CUSTOM_REASON", | ||
text: "Your bug report was rejected. The rageshake server rejected the contents of the report due to a policy.", | ||
}, | ||
])("handles bug report upload errors ($errcode)", async ({ errcode, text }) => { | ||
const { getByLabelText, getByText } = renderComponent(); | ||
fetchMock.postOnce(BUG_REPORT_URL, { status: 400, body: errcode ? { errcode: errcode, error: "blah" } : "" }); | ||
await userEvent.type(getByLabelText("GitHub issue"), "https://example.org/some/issue"); | ||
await userEvent.type(getByLabelText("Notes"), "Additional text"); | ||
await userEvent.click(getByText("Send logs")); | ||
expect(onFinished).not.toHaveBeenCalled(); | ||
expect(fetchMock).toHaveFetched(BUG_REPORT_URL); | ||
await waitFor(() => getByText(text)); | ||
}); | ||
|
||
it("should show a policy link when provided", async () => { | ||
const { getByLabelText, getByText } = renderComponent(); | ||
fetchMock.postOnce(BUG_REPORT_URL, { | ||
status: 404, | ||
body: { errcode: "REJECTED_CUSTOM_REASON", error: "blah", policy_url: "https://example.org/policyurl" }, | ||
}); | ||
await userEvent.type(getByLabelText("GitHub issue"), "https://example.org/some/issue"); | ||
await userEvent.type(getByLabelText("Notes"), "Additional text"); | ||
await userEvent.click(getByText("Send logs")); | ||
expect(onFinished).not.toHaveBeenCalled(); | ||
expect(fetchMock).toHaveFetched(BUG_REPORT_URL); | ||
await waitFor(() => { | ||
const learnMoreLink = getByText("Learn more"); | ||
expect(learnMoreLink).toBeInTheDocument(); | ||
expect(learnMoreLink.getAttribute("href")).toEqual("https://example.org/policyurl"); | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing the i18n key will be confusing here for other languages as the meaning has drastically changed. Also doesn't this fit within "causes"? e.g. cause=unreachable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't actually need to change this now there is sufficient coverage via
failed_send_logs_causes
for errors returned by the API. For everything else, I think this error is fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous string (which will be what all other languages use for the foreseeable) is a partial sentence, ending with a colon. It'll never have any text rendered after it again, it'll look broken. It still needs a new key