Page MenuHomePhabricator
Differential D11864

[lib/native] introduce useUnlinkFID
ClosedPublic
Actions

Authored by ginsu on May 2 2024, 11:42 PM.
Tags
None
Referenced Files
F5251934: D11864.id40248.diff
Sun, Apr 6, 4:46 AM
Unknown Object (File)
Fri, Apr 4, 10:25 PM
Unknown Object (File)
Thu, Apr 3, 4:40 PM
Unknown Object (File)
Mon, Mar 31, 9:39 PM
Unknown Object (File)
Mon, Mar 31, 5:12 AM
Unknown Object (File)
Sun, Mar 30, 8:42 PM
Unknown Object (File)
Sun, Mar 30, 3:52 PM
Unknown Object (File)
Sun, Mar 30, 3:00 PM
View All Files
Subscribers
ashoat
tomek
varun

Details

Reviewers
varun
will
ashoat
Commits
rCOMM3d533a2092bb: [lib/native] introduce useUnlinkFID
Summary

This diff introduces useUnlinkFID which is a hook that returns a callback that calls the unlinkFarcasterAccount identity rpc and then clears the fid from the synced metadata client store

Linear task: https://linear.app/comm/issue/ENG-7412/call-unlink-farcaster-rpc-on-the-client

Depends on D11863

Test Plan

Confirmed that the fid was being cleared from the synced metadata client store + confirmed in meeting with @varun that I was correctly calling and getting the expected behavior of unlinkFarcasterAccount RPC

Confirmed that loading + error states worked as expected

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.May 2 2024, 11:42 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 2 2024, 11:42 PM
ginsu edited the summary of this revision. (Show Details)May 2 2024, 11:46 PM
ginsu added reviewers: varun, will.
ginsu added a parent revision: D11863: [lib/native] introduce useLinkFID hook.
ginsu edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.May 2 2024, 11:53 PM
Harbormaster failed remote builds in B28639: Diff 39757!
ginsu added a comment.May 2 2024, 11:54 PM

will make sure ci passes before landing

ginsu requested review of this revision.May 2 2024, 11:54 PM
ginsu planned changes to this revision.May 3 2024, 12:14 PM

Improve error handling

ashoat requested changes to this revision.May 3 2024, 12:27 PM
ashoat added inline comments.
native/profile/farcaster-account-settings.react.js
31 ↗(On Diff #39757)

What happens on error? Should we show an Alert to the user to explain that it failed?

ashoat added a comment.May 3 2024, 12:27 PM

(Ah you planned changes before I requested them, cool)

ginsu updated this revision to Diff 39782.May 3 2024, 11:57 PM

address comments

Harbormaster completed remote builds in B28656: Diff 39782.May 4 2024, 12:13 AM
ashoat requested changes to this revision.May 4 2024, 11:38 AM
ashoat added inline comments.
native/profile/farcaster-account-settings.react.js
33 ↗(On Diff #39782)

How do we handle in-progress state here while we’re waiting for the identity service to be called? Should we keep the spinner spinning?

This revision now requires changes to proceed.May 4 2024, 11:38 AM
ginsu updated this revision to Diff 40141.May 13 2024, 10:18 PM

address comments

ginsu updated this revision to Diff 40142.May 13 2024, 10:21 PM

update

ginsu updated this revision to Diff 40143.May 13 2024, 10:24 PM

forgot to hit save

ginsu edited the test plan for this revision. (Show Details)May 13 2024, 10:26 PM
Harbormaster completed remote builds in B28882: Diff 40141.May 13 2024, 10:34 PM
Harbormaster completed remote builds in B28883: Diff 40142.May 13 2024, 10:37 PM
Harbormaster completed remote builds in B28884: Diff 40143.May 13 2024, 10:40 PM
ashoat accepted this revision.May 14 2024, 9:30 AM

Thanks! Please always keep in mind progress and loading state going forward. As a frontend dev, those things need to be second nature to you, and not something you should be relying on reviewers to catch

This revision is now accepted and ready to land.May 14 2024, 9:30 AM
ginsu updated this revision to Diff 40242.May 15 2024, 11:27 AM

rebase before landing

Harbormaster completed remote builds in B28958: Diff 40242.May 15 2024, 11:43 AM
Closed by commit rCOMM3d533a2092bb: [lib/native] introduce useUnlinkFID (authored by ginsu). · Explain WhyMay 15 2024, 11:51 AM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM3d533a2092bb: [lib/native] introduce useUnlinkFID.

Revision Contents
Changeset List

PathSize
lib/
utils/
28 lines
native/
profile/
47 lines

Diff 40142

View Options

lib/utils/farcaster-utils.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/profile/farcaster-account-settings.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines