Page MenuHomePhabricator
Differential D13447

[lib] fix changing thread description on native for thick threads
ClosedPublic
Actions

Authored by kamil on Sep 24 2024, 5:35 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 3:50 PM
Unknown Object (File)
Tue, Apr 1, 6:59 AM
Unknown Object (File)
Mon, Mar 31, 6:54 PM
Unknown Object (File)
Sat, Mar 15, 9:17 AM
Unknown Object (File)
Mar 3 2025, 6:48 AM
Unknown Object (File)
Mar 3 2025, 6:48 AM
Unknown Object (File)
Mar 3 2025, 6:48 AM
Unknown Object (File)
Feb 28 2025, 2:09 AM
View All 61 Files
Subscribers
None

Details

Reviewers
tomek
ashoat
Commits
rCOMMecb034ad0ed1: [lib] fix changing thread description on native for thick threads
Summary

ENG-9316.

The description is a nullable field (link), so whenever clicking editing button for not edited or not set description here it causes changing value from undefined to undefined and a result nothing changes. We're not entering this branch which renders TextInput.

For thin threads, it works only because for some reason keyserver is replacing undefined description with an empty string here.

The alternative solution is to create a thick thread with an empty string as a description (instead of undefined), but I don't like this because types still allow for descryption to be nullable so this bug might be caused again in the future.

Test Plan
  1. Change thread settings on native works for thin and thick threads.
  2. Notifs works.
  3. Changing the description to the same value is a no-op.

Diff Detail

Repository
rCOMM Comm
Branch
fixes-3
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Sep 24 2024, 5:35 AM
kamil held this revision as a draft.
kamil published this revision for review.Sep 24 2024, 5:37 AM
kamil edited the summary of this revision. (Show Details)
kamil edited the test plan for this revision. (Show Details)
tomek accepted this revision.Sep 24 2024, 5:39 AM
This revision is now accepted and ready to land.Sep 24 2024, 5:39 AM
tomek added inline comments.Sep 24 2024, 5:40 AM
native/chat/settings/thread-settings-description.react.js
231 ↗(On Diff #44482)

Wondering if this won't result in sending operations that replace null with '' - after clinking the input and accepting changes.

kamil updated this revision to Diff 44483.Sep 24 2024, 5:52 AM

add empty string as default description

Harbormaster completed remote builds in B31814: Diff 44482.Sep 24 2024, 5:52 AM
kamil added inline comments.Sep 24 2024, 5:53 AM
native/chat/settings/thread-settings-description.react.js
231 ↗(On Diff #44482)

As we disccussed offline - this causes an empty update (without robotext), so avoid that we're gonna match what the keyserver is doing and fill not not-defined description with an empty string in the thread creation spec.

Harbormaster completed remote builds in B31815: Diff 44483.Sep 24 2024, 6:08 AM
Closed by commit rCOMMecb034ad0ed1: [lib] fix changing thread description on native for thick threads (authored by kamil). · Explain WhySep 25 2024, 6:00 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMMecb034ad0ed1: [lib] fix changing thread description on native for thick threads.

Revision Contents
Changeset List

PathSize
lib/
shared/
dm-ops/
2 lines
native/
chat/
settings/
2 lines

Diff 44483

View Options

lib/shared/dm-ops/create-thread-spec.js

  • This file was added.
#import "CommServicesAuthMetadataEmitter.h"
#import <Foundation/Foundation.h>
#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>
NSString *const commServicesAuthMetadata = @"commServicesAuthMetadata";
@interface CommServicesAuthMetadataEmitterIOSWrapper
marcinAuthorUnsubmitted
Done
Inline Actions

It might look uncommon to have interface declaration and implementation next to each other in a single file but in fact we don't need to expose CommServicesAuthMetadataIOSWrapper. If we want to call sendAuthMetadataToJS from Objective - C we can use the common C++ API so let's avoid creating unnecessary files and duplication the same API.

marcin: It might look uncommon to have interface declaration and implementation next to each other in a…
bartekUnsubmitted
Not Done
Inline Actions

It's not that uncommon 😉 I've seen this pattern already in some RN-related code and it makes sense here

bartek: It's not that uncommon 😉 I've seen this pattern already in some RN-related code and it makes…
: RCTEventEmitter <RCTBridgeModule>
@property(nonatomic) BOOL hasListeners;
+ (void)sendToJSAccessToken:(NSString *)accessToken
withUserID:(NSString *)userID
withDeviceID:(NSString *)deviceID;
bartekUnsubmitted
Not Done
Inline Actions

What about sendAccessTokenToJS: ... withUserID: ... withDeviceID?

Initially, I got confused because I understood sendToJSAccessToken as "send something to JS-based access token" 😅

bartek: What about `sendAccessTokenToJS: ... withUserID: ... withDeviceID`? Initially, I got confused…
varunUnsubmitted
Not Done
Inline Actions

+1

varun: +1
@end
@implementation CommServicesAuthMetadataEmitterIOSWrapper
RCT_EXPORT_MODULE(CommServicesAuthMetadataEmitter);
static CommServicesAuthMetadataEmitterIOSWrapper *sharedInstance = nil;
- (instancetype)init {
self = [super init];
if (!self) {
return self;
}
_hasListeners = NO;
return self;
}
- (void)startObserving {
@synchronized([self class]) {
_hasListeners = YES;
sharedInstance = self;
}
}
- (void)stopObserving {
@synchronized([self class]) {
_hasListeners = NO;
sharedInstance = nil;
}
}
- (NSArray<NSString *> *)supportedEvents {
return @[ commServicesAuthMetadata ];
}
- (NSDictionary *)constantsToExport {
return @{@"COMM_SERVICES_AUTH_METADATA" : commServicesAuthMetadata};
}
+ (BOOL)requiresMainQueueSetup {
return YES;
}
+ (void)sendToJSAccessToken:(NSString *)accessToken
withUserID:(NSString *)userID
withDeviceID:(NSString *)deviceID {
@synchronized([self class]) {
if (!sharedInstance) {
return;
}
NSDictionary *eventBody = @{
@"accessToken" : accessToken,
@"userID" : userID,
@"deviceID" : deviceID
};
[sharedInstance sendEventWithName:commServicesAuthMetadata body:eventBody];
}
}
@end
namespace comm {
void CommServicesAuthMetadataEmitter::sendAuthMetadataToJS(
rust::String accessToken,
rust::String userID,
rust::String deviceID) {
NSString *accessTokenObjC =
[NSString stringWithCString:std::string(accessToken).c_str()
encoding:NSUTF8StringEncoding];
NSString *userIDObjC = [NSString stringWithCString:std::string(userID).c_str()
encoding:NSUTF8StringEncoding];
NSString *deviceIDObjC =
[NSString stringWithCString:std::string(deviceID).c_str()
encoding:NSUTF8StringEncoding];
[CommServicesAuthMetadataEmitterIOSWrapper sendToJSAccessToken:accessTokenObjC
withUserID:userIDObjC
withDeviceID:deviceIDObjC];
}
} // namespace comm
View Options

native/chat/settings/thread-settings-description.react.js

  • This file was added.
#import "CommServicesAuthMetadataEmitter.h"
#import <Foundation/Foundation.h>
#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>
NSString *const commServicesAuthMetadata = @"commServicesAuthMetadata";
@interface CommServicesAuthMetadataEmitterIOSWrapper
marcinAuthorUnsubmitted
Done
Inline Actions

It might look uncommon to have interface declaration and implementation next to each other in a single file but in fact we don't need to expose CommServicesAuthMetadataIOSWrapper. If we want to call sendAuthMetadataToJS from Objective - C we can use the common C++ API so let's avoid creating unnecessary files and duplication the same API.

marcin: It might look uncommon to have interface declaration and implementation next to each other in a…
bartekUnsubmitted
Not Done
Inline Actions

It's not that uncommon 😉 I've seen this pattern already in some RN-related code and it makes sense here

bartek: It's not that uncommon 😉 I've seen this pattern already in some RN-related code and it makes…
: RCTEventEmitter <RCTBridgeModule>
@property(nonatomic) BOOL hasListeners;
+ (void)sendToJSAccessToken:(NSString *)accessToken
withUserID:(NSString *)userID
withDeviceID:(NSString *)deviceID;
bartekUnsubmitted
Not Done
Inline Actions

What about sendAccessTokenToJS: ... withUserID: ... withDeviceID?

Initially, I got confused because I understood sendToJSAccessToken as "send something to JS-based access token" 😅

bartek: What about `sendAccessTokenToJS: ... withUserID: ... withDeviceID`? Initially, I got confused…
varunUnsubmitted
Not Done
Inline Actions

+1

varun: +1
@end
@implementation CommServicesAuthMetadataEmitterIOSWrapper
RCT_EXPORT_MODULE(CommServicesAuthMetadataEmitter);
static CommServicesAuthMetadataEmitterIOSWrapper *sharedInstance = nil;
- (instancetype)init {
self = [super init];
if (!self) {
return self;
}
_hasListeners = NO;
return self;
}
- (void)startObserving {
@synchronized([self class]) {
_hasListeners = YES;
sharedInstance = self;
}
}
- (void)stopObserving {
@synchronized([self class]) {
_hasListeners = NO;
sharedInstance = nil;
}
}
- (NSArray<NSString *> *)supportedEvents {
return @[ commServicesAuthMetadata ];
}
- (NSDictionary *)constantsToExport {
return @{@"COMM_SERVICES_AUTH_METADATA" : commServicesAuthMetadata};
}
+ (BOOL)requiresMainQueueSetup {
return YES;
}
+ (void)sendToJSAccessToken:(NSString *)accessToken
withUserID:(NSString *)userID
withDeviceID:(NSString *)deviceID {
@synchronized([self class]) {
if (!sharedInstance) {
return;
}
NSDictionary *eventBody = @{
@"accessToken" : accessToken,
@"userID" : userID,
@"deviceID" : deviceID
};
[sharedInstance sendEventWithName:commServicesAuthMetadata body:eventBody];
}
}
@end
namespace comm {
void CommServicesAuthMetadataEmitter::sendAuthMetadataToJS(
rust::String accessToken,
rust::String userID,
rust::String deviceID) {
NSString *accessTokenObjC =
[NSString stringWithCString:std::string(accessToken).c_str()
encoding:NSUTF8StringEncoding];
NSString *userIDObjC = [NSString stringWithCString:std::string(userID).c_str()
encoding:NSUTF8StringEncoding];
NSString *deviceIDObjC =
[NSString stringWithCString:std::string(deviceID).c_str()
encoding:NSUTF8StringEncoding];
[CommServicesAuthMetadataEmitterIOSWrapper sendToJSAccessToken:accessTokenObjC
withUserID:userIDObjC
withDeviceID:deviceIDObjC];
}
} // namespace comm