Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
There was a problem hiding this comment.
🟡 Changes recommended
An older clipboard operation can overwrite feedback for a newer snackbar, and the redaction logic leaves personal data exposed in copied request or response bodies.
2 open issues
What changed in this PR
Adds copyable error and warning notifications to support bug reports, extending the shared snackbar with request context and clipboard feedback.
Changes:
- Adds a copy button with success/failure feedback while preserving close-only success notifications.
- Builds diagnostic details with query-string removal, key-based redaction, payload limits, and fallback handling.
- Adds translations and tests for diagnostic formatting and clipboard behavior.
| File | Description |
|---|---|
src/utils/constants.js |
Defines the diagnostic payload length limit. |
src/utils/actions.js |
Builds and attaches diagnostic details to error notifications. |
src/utils/__tests__/actions.test.js |
Tests redaction, formatting, truncation, and fallback behavior. |
src/i18n/en.json |
Adds copy, clipboard feedback, and close labels. |
src/components/mui/SnackbarNotification/index.js |
Adds plain-text copying and clipboard status feedback. |
src/components/mui/__tests__/snackbar-notification.test.js |
Tests copying, feedback, message resets, and success notifications. |
Review effort: Balanced
96312db to
e4a3cc0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Clipboard output can still expose personal data through RSVP value fields and unsanitized error messages, while stale copy completions can overwrite newer feedback.
2 open issues
2 resolved since last review
Review effort: Balanced
| }; | ||
|
|
||
| // secrets, payment details and personal information are never copied | ||
| const SENSITIVE_KEYS = /^(.*_key|.*(token|secret|password).*|hash|.*emails?|.*phone.*|.*(first|last)_name|.*full_?names?|surname|(.*_)?owner_name|requestor_name|contact_name|(.*_)?address(\d|_.*)?|zip_code|gender|ip|payment_info_details|payment_gateway_result|answers?)$/i; |
There was a problem hiding this comment.
@ako3131 SENSITIVE_KEYS does not match qr_code, so a failed badge check-in copies the attendee's email and full name.
summit-admin's checkInBadge sends { qr_code: code } through snackbarErrorHandler (badge-actions.js#L284-L289). summit-api builds that QR from the badge prefix, ticket number, owner email and owner full name, and only encrypts it when the summit has a QR encryption key (SummitAttendeeBadge.php#L125-L136). The value lands in the "Request body" block of the copied details as is.
Suggested fix: add qr_code to the pattern, and reword the comment on line 59 from "never copied" to something like "best-effort redaction by key name", since the pattern only sees key names and cannot catch personal data inside string values.
On the question in the PR description: I would keep the request body but copy only its shape, meaning keys, numbers and booleans, with every string value replaced by a placeholder. That keeps the ids and flags that make a bug report useful and removes the need to maintain the list as new fields appear. The response message and errors can stay, since they are already on screen. This part is a recommendation, not a blocker.
| () => finish("copied"), | ||
| () => finish("failed") | ||
| ); | ||
| } catch (e) { |
There was a problem hiding this comment.
@ako3131 The synchronous catch branch has no test. Every clipboard test installs a writeText mock, so the case this branch exists for, navigator.clipboard being undefined on a non-https page, is never exercised.
Suggested fix: add a test that clicks the copy button without calling mockClipboard and expects general.copy_failed in the role="status" element and no CheckIcon.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@ako3131 please review
@santipalenque for vis
| const ui = (snackbarMessage) => ( | ||
| <SnackbarNotification | ||
| snackbarMessage={snackbarMessage} | ||
| clearSnackbarMessage={clearSnackbarMessage} |
There was a problem hiding this comment.
@ako3131 this PR no longer merges, and the rebase needs two manual fixes that git will not flag for you.
-
The base (Fix/snackbar error visibility #384) renamed the prop
clearSnackbarMessagetoclearConsumedSnackbarMessage. The conflict markers only cover the nestedpinned and grouped errorsblock. The hoistedlet clearSnackbarMessage, theui()helper on this line and thebeforeEachmerge cleanly with the old name. After the merge the component callsclearConsumedSnackbarMessage(snackbarMessage), the prop is undefined becauseconnectis mocked as a passthrough, and every test that usesui()fails with aTypeError.- Rename the hoisted variable, the prop passed in
ui()and thebeforeEachassignment toclearConsumedSnackbarMessage. - Keep the base's
clearConsumedSnackbarMessage: jest.fn()in theutils/actionsmock. - Drop the duplicate declarations inside the nested
describe.
- Rename the hoisted variable, the prop passed in
-
mainis now 6.0.0 (React 19, MUI 9). That upgrade removedimport request from 'superagent/lib/client'fromsrc/utils/__tests__/actions.test.js, and this PR usesrequestin three new tests and adds theURIimport next to it. Keep both imports when resolving that conflict.
Please re-run both suites after the rebase. The green build on this PR is from before the v6 upgrade.
| const copyText = | ||
| count > 1 | ||
| ? queue.shown | ||
| .map( | ||
| (m, i) => | ||
| `[${i + 1}/${count}] ${formatSentAt(m)}\n${toCopyText(m)}` | ||
| ) | ||
| .join("\n\n") | ||
| : toCopyText(msgData); |
There was a problem hiding this comment.
@ako3131 When several requests fail with the same message inside the dedupe window, the copy only contains the first request's details.
add() (lines 81-88) treats a message as a repeat when html and type match within NOTIFICATION_DEDUPE_WINDOW, and returns prev without looking at details. Every error that falls through to the default branch of authErrorHandler has the same errors.server_error text. So when a page fires several requests and the API returns 500 for all of them, one message is kept and the other requests' method, URL and body are dropped. That is the case where the request line is most useful in a bug report, and the copied text points at only one of the failing endpoints.
Suggested fix: keep showing a single message, but append the repeat's details to it. A sketch of the body of add():
const repeat = prev.find(
(m) =>
!closingRef.current.includes(m) &&
m.html === msg.html &&
m.type === msg.type &&
now - m.receivedAt < NOTIFICATION_DEDUPE_WINDOW
);
if (!repeat) return [...prev, { ...msg, receivedAt: now }];
// same text from another request: one message on screen, both requests in the copy
if (!msg.details) return prev;
return prev.map((m) =>
m === repeat
? { ...m, details: [m.details, msg.details].filter(Boolean).join("\n\n") }
: m
);After the rebase, keep the base's window condition (!grouped || now - m.receivedAt < NOTIFICATION_DEDUPE_WINDOW). Replacing the message object also clears the "Copied" state, which is correct because the copied text is now out of date. Please add a test with two errors that share html and type but have different details, asserting that both request lines are in the copied text.
| // post, delete and file requests keep the URI object superagent was given | ||
| const url = req.url ? String(req.url).split("?")[0] : ""; | ||
| // timeouts carry no method or url, only the timeout | ||
| const status = err.status || (err.timeout && "timed out") || (navigator.onLine ? "no response" : "no response, offline"); |
There was a problem hiding this comment.
@ako3131 When a request times out, the copied details name no request: the line reads Request: (timed out).
superagent's timeout error is a bare Error with only timeout, code and errno, so req.method and req.url are both undefined here. getRequest is the only helper that sets a timeout (60s), so this is every slow GET. On a page that fires several requests, the bug report then has the page and the time but not the endpoint that hung.
buildErrorDetails already handles errors that have no response: superagent's crossDomainError puts method and url on the error, and the offline test relies on that. The timeout error just needs the same two fields.
Suggested fix, in getRequest, where key is already the URL without the access_token:
req.timeout({
response: 60000,
deadline: 60000,
})
.end((err, res) => {
// a timeout error carries no method or url, so name the request for the copied details
if (err && !err.response && !err.url) {
err.method = "GET";
err.url = key;
}
responseHandler(dispatch, state, receiveActionCreator, errorHandler, resolve, reject, key, useEtag)(err, res);
});Please add a test that dispatches snackbarErrorHandler with { timeout: 60000, code: "ECONNABORTED", method: "GET", url: "https://api.example.com/api/v1/summits?page=2" } and expects Request: GET https://api.example.com/api/v1/summits (timed out).
… to screen readers
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
4bc00b6 to
0663664
Compare
ref: https://app.clickup.com/t/9014802374/86bcjxrkv
Summary
Error and warning snackbars now have a copy button next to the close button.
It copies the message as plain text, followed by details for bug reports:
access_token) and status, or "timed out" / "no response, offline"Sensitive fields are replaced with
[REDACTED], using a key pattern insrc/utils/actions.js. String request bodies are skipped, because they can't be redacted by key. Each payload is cut off at 2000 characters. If building the details fails, the snackbar and the 401 re-login still run.Success snackbars keep only the close button. The button's tooltip shows "Copied" or "Couldn't copy" after a click.
Question for the reviewer
The redaction list (
SENSITIVE_KEYS) is a list of summit-api field names that appear sensitive. The API doesn't provide it, so it won't pick up fields added later. Is this list good enough, or should we: