Page MenuHomePhabricator
Differential D5541

[native] Maintain spoiler reveal state on message tap
AbandonedPublic
Actions

Authored by rohan on Nov 7 2022, 4:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 19, 2:44 AM
Unknown Object (File)
Fri, Apr 18, 4:59 PM
Unknown Object (File)
Fri, Apr 18, 2:59 PM
Unknown Object (File)
Fri, Apr 18, 2:55 PM
Unknown Object (File)
Fri, Apr 18, 1:15 PM
Unknown Object (File)
Fri, Apr 18, 10:16 AM
Unknown Object (File)
Fri, Apr 18, 8:11 AM
Unknown Object (File)
Fri, Apr 18, 7:53 AM
View All Files
Subscribers
• abosh
ashoat
tomek

Details

Reviewers
atul
ginsu
ashoat
Summary

This is the final step of https://linear.app/comm/issue/ENG-2072/maintain-the-spoiler-state-upon-opening-textmessagetooltipmodal, where messages will retain their 'revealed' state when tapped to reveal the tooltip. With this diff, spoilers for native should now be fully functional, with two remaining tasks to just handle notifications and message previews.

Depends on D5539

Test Plan

Tested the states being retained both visually by revealing specific spoilers and then tapping a message, and also by logging the newly created objects to track the expected behavior.

Diff Detail

Repository
rCOMM Comm
Branch
spoiler
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Nov 7 2022, 4:28 AM
Herald added subscribers: • abosh, tomek, ashoat. · View Herald TranscriptNov 7 2022, 4:28 AM
rohan edited the summary of this revision. (Show Details)Nov 7 2022, 4:29 AM
rohan edited the test plan for this revision. (Show Details)
rohan attached a referenced file: F225076: Spoilers.mp4. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 7 2022, 4:36 AM
Harbormaster failed remote builds in B13193: Diff 18099!
rohan requested review of this revision.Nov 7 2022, 4:53 AM

Bypassing Android CI since it currently has ongoing problems

atul added a comment.Nov 7 2022, 7:44 AM
In D5541#164089, @rohan wrote:

Bypassing Android CI since it currently has ongoing problems

(When you get a second can you run git pull --rebase && arc diff to retrigger Android CI with latest changes)

rohan updated this revision to Diff 18152.Nov 7 2022, 9:35 AM

Rebase for CI

Harbormaster failed remote builds in B13228: Diff 18152!Nov 7 2022, 9:45 AM
Harbormaster completed remote builds in B13228: Diff 18152.Nov 7 2022, 10:02 AM
atul requested changes to this revision.Nov 8 2022, 7:11 AM

Same feedback in D5539 about using React.useMemo instead of React.useState for holding onto style

native/markdown/markdown-spoiler.react.js
30–35 ↗(On Diff #18152)

Since the style is an "output" of the state we should use a React.useMemo instead of React.useState that changes in response to changes in state.

That keeps things declarative and we don't have to imperatively call setStyleBasedOnState anytime the style should change.

This revision now requires changes to proceed.Nov 8 2022, 7:11 AM
rohan mentioned this in D5539: [native] Maintain the revealed state of spoilers.Nov 9 2022, 10:22 AM
rohan updated this revision to Diff 18290.Nov 9 2022, 11:15 AM

Rebase, resolve conflicts, and use React.useMemo for spoiler style

Harbormaster completed remote builds in B13338: Diff 18290.Nov 9 2022, 11:29 AM
rohan added a child revision: D5597: [IGNORE] GestureTouchableOpacity Spoiler changes.Nov 10 2022, 9:21 AM
ginsu accepted this revision.Nov 11 2022, 9:47 AM

Logic makes sense, just one question inline

native/markdown/markdown-context.js
12

I am guessing this new line was an accident

atul requested changes to this revision.Nov 14 2022, 2:17 PM

Two things to be addressed before land-able

  • Clean up spoilerIdentifier naming/type/handling
  • Stop using styleBasedOnSpoilerState in subsequent logic... styles should be an "output" not an "input"
native/chat/inner-text-message.react.js
105

Let's rename this messageContextValue just to be a bit more specific

105–107

Can we use React.useMemo here to avoid unnecessarily re-computing the contextValue object?

native/markdown/markdown-context.js
12

Yeah let's remove this newline to keep git history clean

native/markdown/markdown-spoiler.react.js
26

Can we instead call this something like spoilerIdentifier so it's more descriptive?

What are we trying to achieve with the casted prefix? Should we be typing identifier as a number?

I'm guessing it's typed as string | number | void because that's what key is typed as?

If that's the case, is it fine for castedIdentifier to equal identifier if parseInt fails/is falsey? Or would that break things? I'd assume using void as an object key would lead to some broken behavior.

39

We don't want to be using styles in our logic like this. Can we use the same castedIdentifier && spoilerRevealed?.[messageID]?.[castedIdentifier] conditional?

This revision now requires changes to proceed.Nov 14 2022, 2:17 PM
rohan added inline comments.Nov 15 2022, 5:48 AM
native/markdown/markdown-spoiler.react.js
26

Sure, rename will come in the revision.

As for the casted prefix, it's just a way to identify that it's not the original identifier typed as string | number | void - instead, it's now a number instead of a string. Though a better naming convention would probably be parsedSpoilerIdentifier since we're calling parseInt instead of casting it.

To your other point, state.key is typed as string | number | void so it has to have the same type (identifier), though I agree - I think I better check would be:

const { text, spoilerIdentifier } = props;

const parsedSpoilerIdentifier = spoilerIdentifier ? parseInt(spoilerIdentifier) : -1;
rohan updated this revision to Diff 18427.Nov 15 2022, 5:50 AM

Address the comments from @atul

Harbormaster completed remote builds in B13433: Diff 18427.Nov 15 2022, 6:04 AM
rohan updated this revision to Diff 18438.Nov 15 2022, 10:05 AM

Rebase on previous diff in stack (should be successful)

Harbormaster completed remote builds in B13440: Diff 18438.Nov 15 2022, 10:19 AM
atul accepted this revision.Nov 15 2022, 3:39 PM

👍🏽

This revision is now accepted and ready to land.Nov 15 2022, 3:39 PM
rohan updated this revision to Diff 18450.Nov 15 2022, 6:21 PM

Rebase

Harbormaster completed remote builds in B13451: Diff 18450.Nov 15 2022, 6:35 PM
rohan updated this revision to Diff 18467.Nov 16 2022, 4:36 AM

Rebase

Harbormaster completed remote builds in B13468: Diff 18467.Nov 16 2022, 4:48 AM
rohan added a child revision: D5688: Allow for tooltip to be hidden when revealing spoiler while maintaining styles.Nov 18 2022, 3:06 PM
rohan removed a child revision: D5597: [IGNORE] GestureTouchableOpacity Spoiler changes.
ashoat added a comment.Nov 21 2022, 11:08 AM

After adding MessageContext.Provider to InnerTextMessage, there is no reason that it needs to stay in TextMessage, which renders InnerTextMessage inside of itself anyways. Let's make sure we don't have two components in the same hierarchy setting MessageContext

rohan updated this revision to Diff 18695.Nov 21 2022, 12:46 PM

Rebase

Harbormaster completed remote builds in B13646: Diff 18695.Nov 21 2022, 1:01 PM
rohan updated this revision to Diff 18701.Nov 21 2022, 1:40 PM

Update to use messageKey and remove extra MessageContext from
TextMessage

Harbormaster completed remote builds in B13652: Diff 18701.Nov 21 2022, 1:53 PM
rohan mentioned this in D5515: [native] Update the existing Context API for Markdown to encompass AppNavigator.Nov 21 2022, 3:48 PM
ashoat requested changes to this revision.Nov 22 2022, 6:11 AM

Let's abandon this diff... see my comment in D5515

This revision now requires changes to proceed.Nov 22 2022, 6:11 AM
rohan mentioned this in D5688: Allow for tooltip to be hidden when revealing spoiler while maintaining styles.Nov 22 2022, 12:54 PM
rohan removed a child revision: D5688: Allow for tooltip to be hidden when revealing spoiler while maintaining styles.
rohan abandoned this revision.Nov 22 2022, 1:01 PM

Abandoning since this can easily just be done in D5515, as pointed out by @ashoat. I'll include it there when I'm able to put out a revision to that diff

Revision Contents
Changeset List

PathSize
native/
chat/
13 lines
markdown/
1 line
35 lines

Diff 18290

View Options

native/chat/inner-text-message.react.js

Show First 20 Lines • Show All 240 Lines • ▼ Show 20 Lines const messageDatas = userIDs.map(otherUserID => ({
creatorID: viewer.userID, creatorID: viewer.userID,
creatorFID: viewerFID, creatorFID: viewerFID,
targetID: otherUserID, targetID: otherUserID,
targetFID: request.userIDsToFID[otherUserID], targetFID: request.userIDsToFID[otherUserID],
time: now, time: now,
operation: 'farcaster_mutual', operation: 'farcaster_mutual',
})); }));
await createMessages(viewer, messageDatas, 'broadcast'); await createMessages(viewer, messageDatas, 'broadcast');
} else if (request.action === relationshipActions.ACKNOWLEDGE) {
updateIDs.push(...userIDs);
const insertRows = userIDs.map(userID => {
const [user1, user2] = sortUserIDs(viewer.userID, userID);
return { user1, user2, status: undirectedStatus.KNOW_OF };
});
const updateDatas = await updateChangedUndirectedRelationships(insertRows);
inkaAuthorUnsubmitted
Done
Inline Actions

updateChangedUndirectedRelationships only updates the relationship if there was none, or the relationship status provided it greater than the one in the db. This will not be the case here, because KNOW_OF is 0-the lowest possible value. So we will not override a more meaningful relationship.

inka: `updateChangedUndirectedRelationships` only updates the relationship if there was none, or the…
tomekUnsubmitted
Not Done
Inline Actions

Makes sense, but is there an easy way of checking if the relationship doesn't need to be updated?

tomek: Makes sense, but is there an easy way of checking if the relationship doesn't need to be…
inkaAuthorUnsubmitted
Done
Inline Actions

The only thing this function does is checking if the relationship needs to be updated, and if so updates it. So I don't think it makes sense to duplicate this logic.
We discussed this offline and agreed that this code makes sense and can be left as is.

inka: The only thing this function does is checking if the relationship needs to be updated, and if…
await createUpdates(updateDatas);
} else { } else {
invariant( invariant(
false, false,
`action ${request.action} is invalid or not supported currently`, `action ${request.action} is invalid or not supported currently`,
); );
} }
await createUpdates( await createUpdates(
▲ Show 20 Lines • Show All 169 Lines • Show Last 20 Lines
View Options

native/markdown/markdown-context.js

Show First 20 Lines • Show All 240 Lines • ▼ Show 20 Lines const messageDatas = userIDs.map(otherUserID => ({
creatorID: viewer.userID, creatorID: viewer.userID,
creatorFID: viewerFID, creatorFID: viewerFID,
targetID: otherUserID, targetID: otherUserID,
targetFID: request.userIDsToFID[otherUserID], targetFID: request.userIDsToFID[otherUserID],
time: now, time: now,
operation: 'farcaster_mutual', operation: 'farcaster_mutual',
})); }));
await createMessages(viewer, messageDatas, 'broadcast'); await createMessages(viewer, messageDatas, 'broadcast');
} else if (request.action === relationshipActions.ACKNOWLEDGE) {
updateIDs.push(...userIDs);
const insertRows = userIDs.map(userID => {
const [user1, user2] = sortUserIDs(viewer.userID, userID);
return { user1, user2, status: undirectedStatus.KNOW_OF };
});
const updateDatas = await updateChangedUndirectedRelationships(insertRows);
inkaAuthorUnsubmitted
Done
Inline Actions

updateChangedUndirectedRelationships only updates the relationship if there was none, or the relationship status provided it greater than the one in the db. This will not be the case here, because KNOW_OF is 0-the lowest possible value. So we will not override a more meaningful relationship.

inka: `updateChangedUndirectedRelationships` only updates the relationship if there was none, or the…
tomekUnsubmitted
Not Done
Inline Actions

Makes sense, but is there an easy way of checking if the relationship doesn't need to be updated?

tomek: Makes sense, but is there an easy way of checking if the relationship doesn't need to be…
inkaAuthorUnsubmitted
Done
Inline Actions

The only thing this function does is checking if the relationship needs to be updated, and if so updates it. So I don't think it makes sense to duplicate this logic.
We discussed this offline and agreed that this code makes sense and can be left as is.

inka: The only thing this function does is checking if the relationship needs to be updated, and if…
await createUpdates(updateDatas);
} else { } else {
invariant( invariant(
false, false,
`action ${request.action} is invalid or not supported currently`, `action ${request.action} is invalid or not supported currently`,
); );
} }
await createUpdates( await createUpdates(
▲ Show 20 Lines • Show All 169 Lines • Show Last 20 Lines
View Options

native/markdown/markdown-spoiler.react.js

Show First 20 Lines • Show All 240 Lines • ▼ Show 20 Lines const messageDatas = userIDs.map(otherUserID => ({
creatorID: viewer.userID, creatorID: viewer.userID,
creatorFID: viewerFID, creatorFID: viewerFID,
targetID: otherUserID, targetID: otherUserID,
targetFID: request.userIDsToFID[otherUserID], targetFID: request.userIDsToFID[otherUserID],
time: now, time: now,
operation: 'farcaster_mutual', operation: 'farcaster_mutual',
})); }));
await createMessages(viewer, messageDatas, 'broadcast'); await createMessages(viewer, messageDatas, 'broadcast');
} else if (request.action === relationshipActions.ACKNOWLEDGE) {
updateIDs.push(...userIDs);
const insertRows = userIDs.map(userID => {
const [user1, user2] = sortUserIDs(viewer.userID, userID);
return { user1, user2, status: undirectedStatus.KNOW_OF };
});
const updateDatas = await updateChangedUndirectedRelationships(insertRows);
inkaAuthorUnsubmitted
Done
Inline Actions

updateChangedUndirectedRelationships only updates the relationship if there was none, or the relationship status provided it greater than the one in the db. This will not be the case here, because KNOW_OF is 0-the lowest possible value. So we will not override a more meaningful relationship.

inka: `updateChangedUndirectedRelationships` only updates the relationship if there was none, or the…
tomekUnsubmitted
Not Done
Inline Actions

Makes sense, but is there an easy way of checking if the relationship doesn't need to be updated?

tomek: Makes sense, but is there an easy way of checking if the relationship doesn't need to be…
inkaAuthorUnsubmitted
Done
Inline Actions

The only thing this function does is checking if the relationship needs to be updated, and if so updates it. So I don't think it makes sense to duplicate this logic.
We discussed this offline and agreed that this code makes sense and can be left as is.

inka: The only thing this function does is checking if the relationship needs to be updated, and if…
await createUpdates(updateDatas);
} else { } else {
invariant( invariant(
false, false,
`action ${request.action} is invalid or not supported currently`, `action ${request.action} is invalid or not supported currently`,
); );
} }
await createUpdates( await createUpdates(
▲ Show 20 Lines • Show All 169 Lines • Show Last 20 Lines