Page MenuHomePhabricator
Differential D13243

[lib] Add useNewThickThread
ClosedPublic
Actions

Authored by inka on Sep 5 2024, 1:16 AM.
Tags
None
Referenced Files
F5255023: D13243.diff
Sun, Apr 6, 7:53 AM
Unknown Object (File)
Wed, Apr 2, 7:20 PM
Unknown Object (File)
Wed, Apr 2, 5:40 AM
Unknown Object (File)
Wed, Apr 2, 5:39 AM
Unknown Object (File)
Wed, Apr 2, 4:19 AM
Unknown Object (File)
Tue, Apr 1, 10:54 AM
Unknown Object (File)
Mar 7 2025, 1:50 AM
Unknown Object (File)
Mar 4 2025, 10:05 PM
View All 68 Files
Subscribers
ashoat

Details

Reviewers
tomek
kamil
Commits
rCOMM7143343df124: [lib] Add useNewThickThread
Summary

issue: ENG-8442

Test Plan

Tested with the next diff, tested that it is possible to create a thick thread

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Sep 5 2024, 1:16 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 5 2024, 1:16 AM
inka added a child revision: D13244: [lib][web][native] Update createRealThreadFromPendingThread to handle thick thread creation.Sep 5 2024, 1:22 AM
Harbormaster completed remote builds in B31431: Diff 43889.Sep 5 2024, 1:33 AM
inka requested review of this revision.Sep 5 2024, 1:33 AM
tomek accepted this revision.Sep 5 2024, 2:56 AM
tomek added inline comments.
lib/hooks/thread-search-hooks.js
121–124 ↗(On Diff #43889)

One thing to note is that these IDs should remain unchanged when we retry thread creation.

125 ↗(On Diff #43889)
This revision is now accepted and ready to land.Sep 5 2024, 2:56 AM
inka added inline comments.Sep 6 2024, 6:46 AM
lib/hooks/thread-search-hooks.js
121–124 ↗(On Diff #43889)

Will update this in the task handling retries ENG-9166, long with possibly other changes required for retires
It is easier for me to implement this step by step.

tomek added a comment.Sep 6 2024, 7:11 AM

Is there a better place to put this hook than thread-search-hooks?

ashoat added inline comments.Sep 9 2024, 5:31 AM
lib/types/thread-types.js
324–334 ↗(On Diff #43889)

When spreading, you lose the read-only properties. These need to be wrapped in $ReadOnly, and it would be good to apply the same to the other types in this file

inka added a parent revision: D13306: [lib] Fix read only in thread-types.Sep 12 2024, 6:27 AM
inka updated this revision to Diff 44080.Sep 12 2024, 6:33 AM

Address review

lib/types/thread-types.js
324–334 ↗(On Diff #43889)

Added D13306 to address other types

Harbormaster completed remote builds in B31562: Diff 44080.Sep 12 2024, 6:51 AM
Closed by commit rCOMM7143343df124: [lib] Add useNewThickThread (authored by inka). · Explain WhySep 12 2024, 8:04 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMM7143343df124: [lib] Add useNewThickThread.

Revision Contents
Changeset List

PathSize
lib/
hooks/
79 lines
types/
12 lines

Diff 44080

View Options

lib/hooks/thread-hooks.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

lib/types/thread-types.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