fix: users logged out despite a valid refresh token (PROD-8704) - #250
fix: users logged out despite a valid refresh token (PROD-8704)#250deaflynx wants to merge 3 commits into
Conversation
onClientError force-unwrapped ThingsboardError.message, throwing "Null check operator used on a null value" on an offline cold start when the client surfaces an error whose message is null. Fall back to the localized unknownError, matching the existing _getMessage pattern in the same file. The session-loss root cause is fixed in the dart client (PROD-8704); this is the accompanying app-side crash guard.
deaflynx
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 1 changed file in fix: avoid crash rendering a null client error message (PROD-8704). Left 1 comment inline.
The null-safe fallback is correct and matches _getMessage in the same file. The inline comment is about the same force-unwrap surviving in two sibling call sites and about pulling the fallback into one helper.
Additional findings
These observations are about existing code outside the PR's diff — spotted while reading surrounding context.
- lib/utils/providers/error_provider/error_provider.dart:22 and lib/core/context/tb_context.dart:142 — both still do
showErrorNotification((_) => tbError.message!), i.e. the exact crash this PR fixes.TbContextis currently commented out inlocator.dart, but theErrorRiverpod provider is the same shape asonClientError; if either is wired to the client'sonError, the same null message will throw there. Worth fixing in the same pass (see the inline comment for a shared helper). - lib/utils/services/tb_client_service/tb_client_service.dart:66 —
onInitErrordoesn't branch onUtils.isConnectionErrorthe wayonClientErrordoes a few lines below. With the dart client now rethrowing transport failures frominit()(thingsboard-dart-client#6) while keeping the tokens, an offline cold start reaches this path withmessage: 'Unable to connect'. If I'm reading the client right, the interceptor also fires theonErrorcallback for the same failure, so the user sees the connection-error dialog and a "Fatal application error occurred / Unable to connect" dialog. Probably not for this PR, but a connection-aware branch here (or a short-circuit when the error is a connection error) would make the offline-resume path much less alarming. - lib/utils/utils.dart:312 —
isConnectionErrorclassifies bye.message == 'Unable to connect', an exact English string produced inside the dart client'stoThingsboardError. Anything the client reports without that precise wording (including a null message) falls through to the generic toast instead of the connection dialog, and a wording change upstream silently reroutes UI here.ThingsboardErrorcarrieserrorCode,statusand the wrappederror(aDioExceptionwhosetype/erroridentify aSocketExceptionor timeout), so keying off those would be more durable. Separate cleanup.
This review was auto-generated. Findings may contain errors — please verify before applying changes.
| } | ||
| _overlayService.showErrorNotification((_) => e.message!); | ||
| _overlayService.showErrorNotification( | ||
| (context) => e.message ?? S.of(context).unknownError, |
There was a problem hiding this comment.
The fallback itself is right. Two thoughts while here:
This is now the fourth copy of e.message ?? S.of(context).unknownError — _getMessage just above (line 60), Error._getMessage in error_provider.dart:28, and the nested getMessage in TbContext.onFatalError (tb_context.dart:123). Meanwhile error_provider.dart:22 and tb_context.dart:142 still force-unwrap tbError.message! — the exact crash this PR fixes. Would it be worth a tiny extension next to the other translation helpers, e.g. in lib/utils/translation_utils.dart:
extension ThingsboardErrorTranslation on ThingsboardError {
String translatedMessage(BuildContext context) =>
message ?? S.of(context).unknownError;
}Then this becomes (context) => e.translatedMessage(context), _getMessage shrinks to the fatalApplicationErrorOccurred prefix plus that call, and the two remaining ! sites get a drop-in replacement instead of waiting for the same crash report from whichever path is still live. It also gives you something cheap to cover with a testWidgets that pumps a MaterialApp with S.delegate and asserts both branches — there's no test for the new behavior right now (and no test/ dir at all), and since the bug was precisely a null-handling assumption, pinning it seems worth a few lines.
On UX: the user now gets a 2-second toast saying "Unknown error." with nothing to act on or quote to support. Since we only land here when the client gave no message, errorCode/status are usually still populated — folding a short code into the toast (there's already an errorOccured: "Error occured: {e}" string of that shape) tends to pay for itself in support triage. Fine to leave as-is for the crash fix; just flagging the trade-off.
There was a problem hiding this comment.
Done in f6a8885.
- Added
ThingsboardErrorTranslation.translatedMessage(context)inlib/utils/translation_utils.dart. - All four
message ?? unknownErrorcopies now go through it (_getMessagehere,Error._getMessage, the nestedgetMessageinTbContext.onFatalError, and this toast). - Replaced the two remaining
tbError.message!force-unwraps inError.onErrorandTbContext.onErrorwith the same call.
Skipped the widget test: the project has no test/ directory, so adding one for this alone is out of scope for this PR. Left the toast text as-is; folding in errorCode/status needs a new localized string, so I'd take that as a follow-up.
…andlers (PROD-8704) Add ThingsboardErrorTranslation.translatedMessage and use it in place of the four duplicated 'message ?? unknownError' expressions. Also replace the two remaining 'tbError.message!' force-unwraps in Error.onError and TbContext.onError, which would throw on the same null message.
|
Re the additional findings:
|
Replace the three identical _getMessage helpers in TbClientService, Error provider and TbContext with translatedFatalErrorMessage next to the ThingsboardError message extension.
Context (PROD-8704)
The customer-facing bug — users logged out after a long idle despite a valid refresh token — is fixed in the dart client: thingsboard/thingsboard-dart-client#6 (the client cleared the session on any transport-failed token refresh).
This PR (app-side)
A small crash guard surfaced while reproducing the issue on device:
TbClientService.onClientErrorforce-unwrappedThingsboardError.message, throwingNull check operator used on a null valueon an offline cold start when the client reports an error whose message is null.onClientErrornow falls back to the localizedunknownError, matching the existing_getMessagepattern in the same file.CE change; merges to PE. Verified on device (Pixel 6): 0 null-check crashes on the offline-resume path that previously produced them.