Page MenuHomePhabricator
Differential D11626

[lib] have landing send `fid` as a string and not a number
ClosedPublic
Actions

Authored by will on Apr 10 2024, 11:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 23, 10:27 AM
Unknown Object (File)
Sat, Mar 15, 3:56 PM
Unknown Object (File)
Mar 5 2025, 6:59 PM
Unknown Object (File)
Mar 5 2025, 6:59 PM
Unknown Object (File)
Mar 5 2025, 6:59 PM
Unknown Object (File)
Mar 5 2025, 6:59 PM
Unknown Object (File)
Mar 5 2025, 6:59 PM
Unknown Object (File)
Mar 5 2025, 6:59 PM
View All 81 Files
Subscribers
ashoat
tomek

Details

Reviewers
ginsu
atul
ashoat
Commits
rCOMM17bad29c6ff5: [lib] have landing send `fid` as a string and not a number
Summary

native expects a string but fid received is a number.

Context: https://linear.app/comm/issue/ENG-7679/type-mismatch-bug-in-farcaster-web-view-reactjs

Test Plan

flow check. Checked on native that receiving correct string fid after connecting my farcaster account

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

will created this revision.Apr 10 2024, 11:19 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptApr 10 2024, 11:19 PM
will added a child revision: D11627: Rename addSyncedMetadata and removeSyncedMetadata action types to set and clear.Apr 10 2024, 11:34 PM
Harbormaster completed remote builds in B28152: Diff 39026.Apr 10 2024, 11:35 PM
will requested review of this revision.Apr 10 2024, 11:35 PM
will updated this revision to Diff 39031.Apr 11 2024, 12:17 AM

toString on landing instead of after parse

will retitled this revision from [lib] convert `data.fid` which is a number to a string to [lib] have landing send `fid` as a string and not a number.Apr 11 2024, 12:19 AM
will edited the summary of this revision. (Show Details)
will edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B28157: Diff 39031.Apr 11 2024, 12:33 AM
ashoat accepted this revision.Apr 11 2024, 4:10 AM
This revision is now accepted and ready to land.Apr 11 2024, 4:10 AM
ginsu added a comment.Apr 11 2024, 8:41 AM

thanks for catching this @will. I thought I addressed this but looking at connect-farcaster.react.js now I can see I accidentally only toString() the farcaster auth url. Would you mind updating this diff to remove the toString() for the auth url? It's typed as a string already so doesn't make sense to toString() a string

ginsu accepted this revision.Apr 11 2024, 8:41 AM
will updated this revision to Diff 39059.Apr 11 2024, 8:57 AM
will edited the test plan for this revision. (Show Details)

remove toString() on url which is already a string

Harbormaster completed remote builds in B28172: Diff 39059.Apr 11 2024, 9:15 AM
Closed by commit rCOMM17bad29c6ff5: [lib] have landing send `fid` as a string and not a number (authored by will). · Explain WhyApr 11 2024, 10:25 AM
This revision was automatically updated to reflect the committed changes.
will added a commit: rCOMM17bad29c6ff5: [lib] have landing send `fid` as a string and not a number.

Revision Contents
Changeset List

PathSize
native/
components/
2 lines

Diff 39026

View Options

native/components/farcaster-web-view.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