[CommCoreModule] Update thread `REPLACE_OPERATION` to handle `avatar`
ClosedPublic
Actions

Authored by atul on Apr 7 2023, 9:28 AM.

Details

Reviewers

ashoat
ginsu

Commits

rCOMMeca24273bc62: [CommCoreModule] Update thread `REPLACE_OPERATION` to handle `avatar`

Summary

We need to update construction of REPLACE_OPERATION to handle avatar.

After this diff avatar will be persisted in the SQLite threads table and will REHYDRATE Redux.

NOTE: I am NOT going to include a migration to handle avatars that may already be in the Redux store because there should be no avatars on prod as of this diff landing.

Test Plan

Set thread avatar from native.

Observe that thread avatar is set in serverDB:

Observe that Redux state on native is updated with avatar:

Observe that avatar field is persisted in clientDB threads table.

Observe that avatar is included in SET_CLIENT_DB_STORE action:

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 7 2023, 9:28 AM

Herald added a subscriber: tomek. · View Herald TranscriptApr 7 2023, 9:28 AM

atul published this revision for review.Apr 7 2023, 9:28 AM

Harbormaster completed remote builds in B18114: Diff 24815.Apr 7 2023, 9:45 AM

ashoat accepted this revision.Apr 7 2023, 5:50 PM

ashoat added a subscriber: rohan.

ashoat added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
641	In D7211, @rohan got a crash when `pinnedCount` was missing and he called this: std::lround(threadObj.getProperty(rt, "pinnedCount").asNumber()) I'm not sure which part of that caused the crash... eg. does `getProperty()` cause a crash if the property doesn't exist, or does `asNumber()` cause that crash, or does `std::lround` cause it. But worth making sure that your code doesn't crash if `avatar` is missing from `threadObj`

This revision is now accepted and ready to land.Apr 7 2023, 5:50 PM

atul added inline comments.Apr 8 2023, 12:49 PM

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
641	Yeah I ensured that mine didn't crash. I'm fairly confident it's the `asNumber()` call that's causing the crash there. That's why we first check `maybe{Property}` before making any further JSI calls that assume truthiness/existence.