Page MenuHomePhabricator
Differential D7301

[native] Display edited message content in the pending threads
ClosedPublic
Actions

Authored by • kuba on Apr 4 2023, 11:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 20, 12:38 PM
Unknown Object (File)
Sun, Apr 20, 10:46 AM
Unknown Object (File)
Sun, Apr 20, 12:44 AM
Unknown Object (File)
Sat, Apr 19, 11:02 PM
Unknown Object (File)
Sat, Apr 19, 8:21 PM
Unknown Object (File)
Sat, Apr 19, 12:30 PM
Unknown Object (File)
Fri, Apr 11, 10:18 PM
Unknown Object (File)
Wed, Apr 2, 4:11 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
michal
inka
ginsu
Commits
rCOMM1f331c5ce520: [native] Display edited message content in the pending threads
Summary

Previously: [fix] Fix 'Maximum update depth exceeded' on native when opening tooltip
Can't open the tooltip menu, because of maximum update depth exceeded warning.
Issue: https://linear.app/comm/issue/ENG-3584/cant-open-tooltips-on-native

Changed the way in which the updated content in the pending thread was displayed. Now, a new edit message is added to the pending thread, instead of modifying the existing one.

Test Plan

Run the app on iOS, open the tooltip menu, it should work. Checked on web if everything still works. Checked if height measurement doesn't display warnings in the console.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• kuba created this revision.Apr 4 2023, 11:40 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptApr 4 2023, 11:40 AM
Harbormaster completed remote builds in B17980: Diff 24620.Apr 4 2023, 11:57 AM
• kuba requested review of this revision.Apr 4 2023, 11:57 AM
inka added a comment.Apr 5 2023, 7:12 AM

It would be nice if you could explain a bit more precisely what the problem was. From your fix it seems the problem was with a pending thread only? But from the description of the problem it seems that the problem was with opening any tooltip. Does it work in other cases?

Why don't you display the edited label when the thread is pending?

web/chat/composed-message.react.js
128 ↗(On Diff #24620)

Hardcoding this label in two places is error-prone. Can you create it in one place and export it?

ashoat added a comment.Apr 5 2023, 12:15 PM

@kuba has not made it clear, but it appears that this diff solves a push-blocking issue: ENG-3584

If so, and if this diff cannot be landed within a matter of hours, then the offending commit should be immediately reverted.

ashoat added a comment.Apr 6 2023, 8:43 AM

@kuba, can you rebase this on master so that it includes the un-revert of your reverted diff?

• kuba updated this revision to Diff 24760.Apr 7 2023, 3:46 AM

Rebase to master

• kuba planned changes to this revision.Apr 7 2023, 3:48 AM

Address @inka comments

Harbormaster completed remote builds in B18071: Diff 24760.Apr 7 2023, 4:04 AM
• kuba updated this revision to Diff 25026.Apr 12 2023, 1:36 AM
• kuba marked an inline comment as done.

New function to display labels

• kuba added a comment.Apr 12 2023, 1:36 AM
In D7301#217546, @inka wrote:

It would be nice if you could explain a bit more precisely what the problem was. From your fix it seems the problem was with a pending thread only? But from the description of the problem it seems that the problem was with opening any tooltip. Does it work in other cases?

The problem was with changing the content of the messageInfo of the message. If the message content was changed it forced re-rendering and went into an infinite loop. The tooltip just triggered this bug, which froze the whole app. Right now, I changed how edited messages content in pending threads is updated, and prevented from going into an infinite loop.

I tested it (on iOS and the web) and right now it works, I couldn't trigger this same issue again.

Why don't you display the edited label when the thread is pending?

It was discussed here: https://phab.comm.dev/D7141?id=23989#inline-47335

In case, that we created a sidebar from an edited message we can't determine if the source message was edited for the sidebar thread. Therefore, I don't display the Edited label here. To have consistent behavior, I don't display the Edited label in the pending thread as well. Otherwise, it would disappear anyway after sending the message with creates the sidebar.

To make it clear, if we edit the source message after creating the sidebar, the Edit label is displayed as expected (both on the main thread and sidebar).

web/chat/composed-message.react.js
128 ↗(On Diff #24620)

I put it into a separate method. I hope that was what you meant.

Harbormaster completed remote builds in B18265: Diff 25026.Apr 12 2023, 1:51 AM
inka accepted this revision.Apr 12 2023, 3:02 AM

Thank you for explaining

This revision is now accepted and ready to land.Apr 12 2023, 3:02 AM
• kuba added a child revision: D7342: [native] Add EditState to the InputState.Apr 12 2023, 3:18 AM
• kuba retitled this revision from [fix] Fix 'Maximum update depth exceeded' on native when opening tooltip to [native] Display edited message content in the pending threads.Apr 12 2023, 6:02 AM
• kuba edited the summary of this revision. (Show Details)
Closed by commit rCOMM1f331c5ce520: [native] Display edited message content in the pending threads (authored by kosmydel <kukubaczek@gmail.com>). · Explain WhyApr 12 2023, 6:05 AM
This revision was automatically updated to reflect the committed changes.
kosmydel <kukubaczek@gmail.com> added a commit: rCOMM1f331c5ce520: [native] Display edited message content in the pending threads.

Revision Contents
Changeset List

PathSize
lib/
selectors/
41 lines
native/
chat/
10 lines
8 lines
web/
chat/
10 lines

Diff 24760

View Options

lib/selectors/chat-selectors.js

// @flow // @flow
import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js'; import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js'; import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js'; import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js'; import type { AppState } from '../redux/redux-setup.js';
import { import {
workerRequestMessageTypes, workerRequestMessageTypes,
type WorkerRequestMessage, type WorkerRequestMessage,
type WorkerResponseMessage, type WorkerResponseMessage,
} from '../types/worker-types.js'; } from '../types/worker-types.js';
Show All 16 Linesclass DatabaseModule {
status: DatabaseStatus; status: DatabaseStatus;
constructor() { constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined ? undefined
: preloadedState.currentUserInfo?.id; : preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID); const isSupported = isSQLiteSupported(currentLoggedInUserID);
if (!isSupported) { if (!isSupported || isDesktopSafari()) {
tomekUnsubmitted
Not Done
Inline Actions

Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app.

tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
kamilAuthorUnsubmitted
Done
Inline Actions

good point, thanks

kamil: good point, thanks
this.status = databaseStatuses.notSupported; this.status = databaseStatuses.notSupported;
kamilAuthorUnsubmitted
Done
Inline Actions

Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key

kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
tomekUnsubmitted
Not Done
Inline Actions

Do we change the status later?

tomek: Do we change the status later?
kamilAuthorUnsubmitted
Done
Inline Actions

yes, in initDBForLoggedInUser()

kamil: yes, in `initDBForLoggedInUser()`
} else { } else {
this.init(); this.init();
} }
} }
init(encryptionKey?: ?SubtleCrypto$JsonWebKey) { init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress; this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH); this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines
View Options

native/chat/composed-message.react.js

// @flow // @flow
import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js'; import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js'; import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js'; import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js'; import type { AppState } from '../redux/redux-setup.js';
import { import {
workerRequestMessageTypes, workerRequestMessageTypes,
type WorkerRequestMessage, type WorkerRequestMessage,
type WorkerResponseMessage, type WorkerResponseMessage,
} from '../types/worker-types.js'; } from '../types/worker-types.js';
Show All 16 Linesclass DatabaseModule {
status: DatabaseStatus; status: DatabaseStatus;
constructor() { constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined ? undefined
: preloadedState.currentUserInfo?.id; : preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID); const isSupported = isSQLiteSupported(currentLoggedInUserID);
if (!isSupported) { if (!isSupported || isDesktopSafari()) {
tomekUnsubmitted
Not Done
Inline Actions

Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app.

tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
kamilAuthorUnsubmitted
Done
Inline Actions

good point, thanks

kamil: good point, thanks
this.status = databaseStatuses.notSupported; this.status = databaseStatuses.notSupported;
kamilAuthorUnsubmitted
Done
Inline Actions

Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key

kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
tomekUnsubmitted
Not Done
Inline Actions

Do we change the status later?

tomek: Do we change the status later?
kamilAuthorUnsubmitted
Done
Inline Actions

yes, in initDBForLoggedInUser()

kamil: yes, in `initDBForLoggedInUser()`
} else { } else {
this.init(); this.init();
} }
} }
init(encryptionKey?: ?SubtleCrypto$JsonWebKey) { init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress; this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH); this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines
View Options

native/chat/utils.js

// @flow // @flow
import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js'; import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js'; import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js'; import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js'; import type { AppState } from '../redux/redux-setup.js';
import { import {
workerRequestMessageTypes, workerRequestMessageTypes,
type WorkerRequestMessage, type WorkerRequestMessage,
type WorkerResponseMessage, type WorkerResponseMessage,
} from '../types/worker-types.js'; } from '../types/worker-types.js';
Show All 16 Linesclass DatabaseModule {
status: DatabaseStatus; status: DatabaseStatus;
constructor() { constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined ? undefined
: preloadedState.currentUserInfo?.id; : preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID); const isSupported = isSQLiteSupported(currentLoggedInUserID);
if (!isSupported) { if (!isSupported || isDesktopSafari()) {
tomekUnsubmitted
Not Done
Inline Actions

Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app.

tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
kamilAuthorUnsubmitted
Done
Inline Actions

good point, thanks

kamil: good point, thanks
this.status = databaseStatuses.notSupported; this.status = databaseStatuses.notSupported;
kamilAuthorUnsubmitted
Done
Inline Actions

Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key

kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
tomekUnsubmitted
Not Done
Inline Actions

Do we change the status later?

tomek: Do we change the status later?
kamilAuthorUnsubmitted
Done
Inline Actions

yes, in initDBForLoggedInUser()

kamil: yes, in `initDBForLoggedInUser()`
} else { } else {
this.init(); this.init();
} }
} }
init(encryptionKey?: ?SubtleCrypto$JsonWebKey) { init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress; this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH); this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines
View Options

web/chat/composed-message.react.js

// @flow // @flow
import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js'; import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js'; import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js'; import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js'; import type { AppState } from '../redux/redux-setup.js';
import { import {
workerRequestMessageTypes, workerRequestMessageTypes,
type WorkerRequestMessage, type WorkerRequestMessage,
type WorkerResponseMessage, type WorkerResponseMessage,
} from '../types/worker-types.js'; } from '../types/worker-types.js';
Show All 16 Linesclass DatabaseModule {
status: DatabaseStatus; status: DatabaseStatus;
constructor() { constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined ? undefined
: preloadedState.currentUserInfo?.id; : preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID); const isSupported = isSQLiteSupported(currentLoggedInUserID);
if (!isSupported) { if (!isSupported || isDesktopSafari()) {
tomekUnsubmitted
Not Done
Inline Actions

Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app.

tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
kamilAuthorUnsubmitted
Done
Inline Actions

good point, thanks

kamil: good point, thanks
this.status = databaseStatuses.notSupported; this.status = databaseStatuses.notSupported;
kamilAuthorUnsubmitted
Done
Inline Actions

Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key

kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
tomekUnsubmitted
Not Done
Inline Actions

Do we change the status later?

tomek: Do we change the status later?
kamilAuthorUnsubmitted
Done
Inline Actions

yes, in initDBForLoggedInUser()

kamil: yes, in `initDBForLoggedInUser()`
} else { } else {
this.init(); this.init();
} }
} }
init(encryptionKey?: ?SubtleCrypto$JsonWebKey) { init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress; this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH); this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines