Page MenuHomePhabricator

[native] Introduce simple error handling
ClosedPublic

Authored by tomek on May 19 2023, 10:16 AM.
Tags
None
Referenced Files
F1547562: D7884.diff
Mon, Apr 15, 12:11 AM
Unknown Object (File)
Sat, Apr 13, 12:06 AM
Unknown Object (File)
Wed, Apr 10, 11:59 PM
Unknown Object (File)
Tue, Apr 9, 12:36 AM
Unknown Object (File)
Fri, Apr 5, 10:15 AM
Unknown Object (File)
Fri, Apr 5, 4:22 AM
Unknown Object (File)
Fri, Apr 5, 4:22 AM
Unknown Object (File)
Fri, Apr 5, 4:22 AM
Subscribers

Details

Summary

This is a simplified version of error handling, without useful error message. Created a task https://linear.app/comm/issue/ENG-3916/better-error-handling to track implementing the proper solution.

Depends on D7883

Test Plan

Check if creating a link with white char or one that is already in use results in an error.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil added inline comments.
native/invite-links/manage-public-link-screen.react.js
44

I would prefer a less generic name to be more descriptive but up to you

53

I don't where this error will be caught and how this will behave, but maybe logging an error message here is a good idea?

68–71

You can use an empty string instead of null in the state (const [error, setError] = React.useState('');), and then render this component always and get rid of this logic, which is shorter and I think also cleaner

This revision is now accepted and ready to land.May 22 2023, 9:42 AM
native/invite-links/manage-public-link-screen.react.js
44

When there's just one action creator in this component, I think it is ok to be really generic - it's not confusing. Later, in D7908, I'm introducing another action creator and then this name changes to be more precise.

53

There's no need for that. Throwing an error in async function passed to dispatchActionPromise results in appropriate _FAILED action being dispatched. Then the caller can decide how to handle that (using createLoadingStatusSelector).

68–71

The difference is that it would then take vertical space, so it's better to not render the error in that case.

This revision was automatically updated to reflect the committed changes.