Report which account a client error happened to - #1238
Merged
Conversation
Collaborator
Author
|
Fourteen review passes were needed to reach zero findings. Most of them were spent on one theme: reading a value somebody else threw, without letting that reading cost the report. The substantive findings, in the order they came up:
Two decisions are deliberate and documented in the description: a failure reported before the session has loaded carries no account, and the window listeners report without one. Both were weighed against the alternative and lost to it. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1237 (frontend half).
Depends on DFXswiss/api#4594 — that one must be merged and deployed first. The global validation
pipe strips a property the DTO does not declare, so sending this field before then would look like
it works and record nothing.
What
A client error report now carries the signed-in account, so a recorded failure can be tied to the
customer who calls support about it. Omitted entirely when nobody is signed in.
How
reportClientErrortakes the account as an argument and stays a plain function that reads noReact context: it also runs from the window listeners installed at startup, and a failure during
startup — before any session is loaded — is exactly what this reporting exists to catch. Making it
depend on user context being loaded would lose that case.
The error screen passes the account, because that screen belongs to the tree that failed. Keeping
the value in the reporting module instead would have made it shared state: a module is evaluated
once, while
Main.widget.tsxandMain.lib.tsxrender oneAppper embedded instance.The account is part of the deduplication signature for the same reason. The same failure under two
accounts is two customers, and dropping the second would hide exactly the one this reporting exists
to find. That signature is now composed with
JSON.stringifyrather than joined by a separator amessage is free to contain — otherwise two different failures could produce the same signature and
the second would be dropped as a repeat.
The value is left out unless it has the shape of an account id: a safe integer greater than zero,
matching what the endpoint accepts. Losing a report over the field that only helps to find it would
defeat the point of sending it.
What this value is not
The ingest endpoint is unauthenticated on purpose, so this is a correlation hint that anyone could
post — never an ownership or authorization signal. The API side documents the same.
What it does not cover
A failure that happens before the session has loaded is reported without an account: the report
goes out as soon as the failure is known, and waiting for a session would risk losing it.
So is a chunk failure that reaches the window listeners rather than the router — including a failed
dynamic import from a click handler, where the customer may well be signed in. Covering that would
mean handing the reporter a session getter from the standalone entry point, evaluated at the time
of the event. It is deliberately left out here: the listeners are installed before any session
exists, and the review that raised it agreed the obvious alternative — keeping the account in the
reporting module — is the design this PR already rejected, because the embedded builds share that
module across instances. Worth doing as a follow-up if the gap matters in practice.
Tests
when nobody is signed in
range
from a missing one
a message that throws when it is read, or a
toStringthat throws — the last of these used tocost the report entirely, which the old test did not notice because it only asserted that nothing
escaped
an unreadable field keeps both its report and the reload that recovers the customer
toErrorFactsconverts the fields it can and drops the ones it cannot, and reads a message off athrown object that is not an Error — previously reported as
[object Object]npm run lint,npm run test(691 tests, 63 suites),npm run build:devandnpm run widget:devall pass locally.