Details

Reviewers

marcin
tomek
kamil
ashoat

Commits

rCOMM720e34270f15: [native] Add alerts to `SQLiteContextProvider` with exception info

Summary

Add additional "logging" (via Alert.alert(...)) to provide user with information about potential issues with setting of threadStore/messageStore or with fetchNewCookieFromNativeCredentials. The logic/behavior should be equivalent to before.

Test Plan

NA, careful reading

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Oct 12 2022, 5:35 PM

Herald added a subscriber: • abosh. · View Herald TranscriptOct 12 2022, 5:35 PM

atul removed a reviewer: ashoat.Oct 12 2022, 5:40 PM

Harbormaster completed remote builds in B12767: Diff 17536.Oct 12 2022, 5:45 PM

atul requested review of this revision.Oct 12 2022, 5:45 PM

ashoat requested changes to this revision.Oct 13 2022, 6:17 AM

ashoat added a subscriber: ashoat.

ashoat added inline comments.

native/data/sqlite-context-provider.js
52 ↗	(On Diff #17536)	Is this meant to be landed, or just patched in before a release? If it's meant to be landed, I think we should only show the alerts if `__DEV__` or `isStaff` If it's meant to be patched in before a release, it would be good to mention that in the diff description somewhere or a comment
53 ↗	(On Diff #17536)	I don't think you can just put an exception inline like this... don't you need to use `getMessageForException` or something?
65 ↗	(On Diff #17536)	Probably need `getMessageForException` here It seems like there's a scenario where the loading screen just ends up spinning indefinitely here. Instead, should we call `ExitApp.exitApp()` here? Or if it's just an internal build, we could indicate to the user they should kill the app, or we could set up the `Alert` so when it's acknowledged it kills the app (eg. there could be only one button on the alert and it could say "kill app" or something)

This revision now requires changes to proceed.Oct 13 2022, 6:17 AM

atul planned changes to this revision.Oct 13 2022, 7:16 AM

atul added inline comments.

native/data/sqlite-context-provider.js
52 ↗	(On Diff #17536)	It's meant to be landed, want to minimize the number of temporary "changes before release." I figured it would be fine to provide these alerts even in a general release to help surface any issues, but I'll update so they only apply to `__DEV__` or staff.
53 ↗	(On Diff #17536)	Here's what I saw locally: I'll modify to use `getMessageForException` for consistency with rest of the codebase
65 ↗	(On Diff #17536)	Will update Since this will be internal only, will add a note to the alert that the app should be killed.