Details
- Throw some error in fetchUserIdentitiesPromise() so that finding user indentity always fails
- Mobile user goes to profile -> Friend list and adds the web user as a friend
- Before a thin thread was created. Now mobile user sees an alert, because there is no auxUserInfo, no thick thread and call to identity server fails.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
This is a scary change... it affects all callers of useFindUserIdentities:
- FarcasterDataHandler
- UserInfosHandler
- useSendDMOperationUtils
- useThreadListSearch via useUsersSupportThickThreads
- useUserProfileThreadInfo via useUsersSupportThickThreads
- Thread composer code (ConnectedMessageListContainer on native, ChatThreadComposer on web) via useUsersSupportThickThreads
- useThreadInfoForPossiblyPendingThread on web via useUsersSupportThickThreads
- useUpdateRelationships (after D13938, via useUsersSupportThickThreads)
Is this really safe for all of those callers?
I'm also a bit confused as to why this is necessary or what it accomplishes. I re-read through the Linear task but couldn't find anything. Could you explain why we want to stop swallowing errors here?
I'm also a bit confused as to why this is necessary or what it accomplishes. I re-read through the Linear task but couldn't find anything. Could you explain why we want to stop swallowing errors here?
With swallowing errors in case findUserIdentities fails because for example there is no internet connection, a thin thread will be created.
Without swallowing errors an error message will be shown to the user and no thread is created.
Also this swallowing errors in general causes confusion and bugs that are hard to debug.
- Request or something fails because there is no internet connection or backend is down (which is expected)
- We swallow the error, log it and move along
- Sometimes a weird behaviour is happening (like in this case thin threads created)
- We report a bug without logs
- We have a bug that is hard to debug and it's hard to figure out what went wrong
It happened at least 2-3 times already in the month I was here. Expected errors like this should be handled.
Thanks @angelika. Some concerns:
- Your test plan is insufficient. You're modifying 8-10 callsites and have not tested all of them. At the very least, I'd expect you to read through each callsite and analyze what would happen if this function threw an exception, as currently it never throws exceptions. (You can document analysis like this in the Test Plan section, even if it technically doesn't count as testing.)
- While I understand where you're coming from, this breaks a pattern in all of our caching code. You'll find in ENSCache, FCCache, etc. that we always handle a timeout this way.
- Beyond breaking a pattern, I'm not sure I agree with the general thinking here. It's true that swallowing exceptions can sometimes lead to unexpected behavior. But, similarly, throwing exceptions instead of swallowing them can lead to certain codepaths being broken. I think it sometimes makes sense to swallow exceptions, and it doesn't seem like you've done an exhaustive analysis of callsites to determine which pattern makes more sense.
I'd be open to rethinking this, but I would need two things:
- Evidence that you've analyzed each callsite and determined what the impact is.
- More details regarding "it happened at least 2-3 times already in the month I was here": detail each case, and how it was caused by this issue.
As an alternative, if there is a specific callsite (perhaps useUpdateRelationships?) where you think this error should not be swallowed, it seems like it would be better to address that callsite directly, by differentiating between null and undefined results.