Mobile drafts #8280

Rajat-Dabade · 2024-10-22T11:58:38Z

Summary

Separate tab for Drafts in mobile.

Ticket Link

https://mattermost.atlassian.net/browse/MM-39356

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.
Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on: iOS version 17.5 (Both on iPhone and ipad)

Screenshots

Release Note

Added a feature for having a separate tab for Drafts in mobile.

Note:
Will cover all the test case for this PR in separate PR.

Rajat-Dabade · 2024-10-22T12:08:13Z

/update-branch

larkox

Great work! Some comments.

app/actions/local/draft.ts

app/components/draft/draft.tsx

app/components/draft/draft_post/draft_files/index.ts

app/components/post_draft/post_input/post_input.tsx

app/queries/servers/drafts.ts

app/screens/global_threads/global_threads.tsx

assets/base/i18n/en.json

Rajat-Dabade · 2024-11-12T14:40:09Z

/update-branch

yasserfaraazkhan

Thank you @Rajat-Dabade

Most flows around

Creating Draft in DM, GM, Channel, Threads, Private Chanel, under deleted thread ✅
Sending Threads, Editing threads work as expected. ✅

There couple of flow I wanted to bring to your notice

On the Draft Screen, if we hit on Android's back button (not the back arrow of MM app) the app closes instead of going back to channels list page
Once all drafts are sent out from the draft page, we have an empty page looking like below. would it be nice to add a 'No drafts at the moment' icon, like in web app. @asaadmahmood thoughts?

Rajat-Dabade · 2024-11-18T09:41:21Z

Thank you @yasserfaraazkhan for reviewing the code.

On the Draft Screen, if we hit on Android's back button (not the back arrow of MM app) the app closes instead of going back to channels list page

Let me check on this.

Once all drafts are sent out from the draft page, we have an empty page looking like below. would it be nice to add a 'No drafts at the moment' icon, like in web app. @asaadmahmood thoughts?

I remember having an empty screen for the draft view, let me add it.

Rajat-Dabade · 2024-11-18T17:28:55Z

@yasserfaraazkhan

On the Draft Screen, if we hit on Android's back button (not the back arrow of MM app) the app closes instead of going back to channels list page ✅
Once all drafts are sent out from the draft page, we have an empty page looking like below. would it be nice to add a 'No drafts at the moment' icon, like in web app. @asaadmahmood thoughts? ✅

Can you please review again? Thanks.

yasserfaraazkhan

Thank you @Rajat-Dabade
just 1 last change, in below flow

Scenarios 1:
Steps:

From Draft page, Edit a draft. Verify you are taken to the Channel/DM/GM
Edit the Draft in the Channel
Click on the < back Icon on the channel

This is taking user to Draft page. should it take user to Channels List page?

Scenarios 2:

With above steps, if we edit draft message (without sending it) and Click back button, the draft message is not updated.

Screen.Recording.2024-11-19.at.12.16.41.AM.mov

Scenario 3:

The User group Name is shown like below

on web app it show as below

asaadmahmood

Overall looks good to me, minor changes though.

https://www.loom.com/share/37c3dde4ec484cd2a61d5afe00bb1657?from_recorder=1&focus_title=1

yasserfaraazkhan

Verified on android and iOS device

Drafts created in Channel/DM/GM
send/edit/delete actions on drafts
swipe left on draft to delete
drafts on public channel and then convert
Remove server that has drafts and add it back to see no drafts are present
Theme applied on the page.
Drafts on multiple teams
Draft count updated when we leave channel, archive channel

Few observations,

On android where we can see channels list page for fraction of a second, when we click on a particular Draft

az_recorder_20241211_150748.mp4

Theme issues when Device is in Dark mode.

Steps:

select Onxy (dark theme)
create Draft in channel ,
change to Denim theme
go to channel and see the theme is applied
go to Draft, long press draft, Click on Edit

Actual: we see Dark theme section

enahum

Overall looks good, great job!

I have added a few comments

enahum · 2024-12-13T22:16:40Z

app/actions/local/draft.ts

+
+export async function parseMarkdownImages(markdown: string, imageMetadata: Dictionary<PostImage | undefined>) {
+    let match;
+    const imageRegex = /!\[.*?\]\((https:\/\/[^\s)]+)\)/g;


If the url is only with http this will not match. Is that what we want?

This is absolutely a nice catch, I will update the regex. Thank you 🫡 .

enahum · 2024-12-13T22:17:38Z

app/actions/local/draft.ts

+    while ((match = imageRegex.exec(markdown)) !== null) {
+        const imageUrl = match[1];
+        // eslint-disable-next-line no-await-in-loop
+        imageMetadata[imageUrl] = await getImageMetadata(imageUrl);


This looks prone to race

So what I understand from the race condition you pointed out is the use of await within the loop, which processes each imageUrl sequentially rather than concurrently which can lead to inefficiencies, especially if there are many URLs and the processing of one URL depends on the others completing.

Is this what you mean?

Basically it can complete iterating while the valus on the object have not been set yet

I have updated the code which ensures that the iteration waits for all asynchronous operation completes before proceeding. Promise.all guarantees that the imageMetadata object is updated after all getImageMetadata calls are resolved.

enahum · 2024-12-13T22:23:11Z

app/components/channel_info/avatar/index.tsx

+    const serverUrl = useServerUrl();
+
+    let uri = '';
+    if (author) {


Based on the prop, author should always be defined, something I'm missing?

True, we don't need this check here. Will remove it.

enahum · 2024-12-13T22:24:38Z

app/components/channel_info/channel_info.tsx

+import {typography} from '@utils/typography';
+import {getUserTimezone} from '@utils/user';
+
+import CompassIcon from '../compass_icon';


Suggested change

import CompassIcon from '../compass_icon';

import CompassIcon from '@components/compass_icon';

enahum · 2024-12-13T22:30:27Z

app/components/channel_info/avatar/index.tsx

The name of the folder does not make a lot of sense to me

Changed the name to ProfileAavator, I think it makes more sense now.

enahum · 2024-12-13T23:37:49Z

app/screens/global_drafts/components/global_drafts_list/global_drafts_list.tsx

+
+    return (
+        <View
+            style={{flex: 1}}


Set this in the stylesheet

enahum · 2024-12-13T23:50:31Z

app/screens/draft_options/send_draft/send_draft.tsx

+                size={ICON_SIZE}
+                color={changeOpacity(theme.centerChannelColor, 0.56)}
+            />
+            <Text style={style.title}>{intl.formatMessage({id: 'draft.options.send.title', defaultMessage: 'Edit draft'})}</Text>


Use FormattedText

enahum · 2024-12-13T23:51:17Z

app/screens/global_drafts/components/draft_empty_component/index.tsx

+
+import React from 'react';
+import {useIntl} from 'react-intl';
+import {Image, Text, View} from 'react-native';


Somewhere else you are using Image from expo-image for the same purpose, which makes sense, change it here

enahum · 2024-12-13T23:51:49Z

app/screens/global_drafts/components/global_drafts_list/tooltip.tsx

+    onClose: () => void;
+}
+
+const logo = require('@assets/images/emojis/swipe.png');


Perhaps change the name of the variable so that it makes sense instead of logo

Changed it.

enahum · 2024-12-13T23:53:31Z

app/screens/global_drafts/index.tsx

+
+import type {AvailableScreens} from '@typings/screens/navigation';
+
+const edges: Edge[] = ['left', 'right'];


Why not include bottom? Is it added elsewhere? I see top in the styles but not bottom. Or is it that we don't want it at all?

This is what the design suggests https://www.figma.com/design/VbEx15Tpvi9mXhs9IEjJtq/MM-45558-Drafts-%26-Schedule-Messages?node-id=4013-70751&m=dev, also we have not used it in the Threads View.

enahum · 2024-12-19T04:08:56Z

app/actions/local/draft.ts

+    // [^\s()<>]+ - Matches the main part of the URL, excluding spaces, parentheses, and angle brackets.
+    // (?:\([^\s()<>]+\))* - Allows balanced parentheses inside the URL path or query parameters.
+    // !\[.*?\]\((...)\) - Matches an image markdown syntax ![alt text](image url)
+    const imageRegex = /!\[.*?\]\((([a-zA-Z][a-zA-Z\d+\-.]*):\/\/[^\s()<>]+(?:\([^\s()<>]+\))*)\)/g;


hmmm, should this only allow http/https/ftp/ftps ? what about the extensions, do we want to do anything with that to only attempt those that are supported?

enahum · 2024-12-19T04:11:53Z

app/actions/local/draft.ts

+    const promises = matches.map(async (match) => {
+        const imageUrl = match[1];
+        if (isValidUrl(imageUrl)) {
+            const metadata = await getImageMetadata(imageUrl);
+            imageMetadata[imageUrl] = metadata;
+        }
+    });


this does return anything so the promises will always be an array of undefined.

Suggested change

const promises = matches.map(async (match) => {

const imageUrl = match[1];

if (isValidUrl(imageUrl)) {

const metadata = await getImageMetadata(imageUrl);

imageMetadata[imageUrl] = metadata;

}

});

const promises = matches.reduce<Array<Promise<MetadataReturnedByGetImageImagedataType>>((result, match) => {

const imageUrl = match[1];

if (isValidUrl(imageUrl)) {

result.push(getImageMetadata(imageUrl));

}

return result;

}, []);

getImageMetadata should include the url in the return object so that you can map it after or something like that

another option would be to use for await but this will make it sequential and not parallel

enahum · 2024-12-19T04:24:13Z

app/screens/draft_options/index.tsx

+    const theme = useTheme();
+    const styles = getStyleSheet(theme);
+    const snapPoints = useMemo(() => {
+        const bottomSheetAdjust = Platform.select({ios: 5, default: 20});


what is this adjust and why is it needed ?

enahum · 2024-12-19T04:25:47Z

app/screens/global_drafts/components/global_drafts_list/SwipeableDraft/index.tsx

SwipeableDraft name convention does not match

enahum · 2024-12-19T04:30:45Z

app/utils/helpers.ts

@@ -182,3 +182,12 @@ export function areBothStringArraysEqual(a: string[], b: string[]) {

    return areBothEqual;
 }
+
+export function isValidUrl(url: string): boolean {


there is an isValidUrl function in app/utils/url which may cause some confusion to devs, either be more specific, use or modify the one we already have. Also if you decide to keep this and change the name, make sure to at least move it the right file, and do add test coverage for it

enahum · 2024-12-19T04:30:57Z

app/utils/draft/index.ts

add unit test for code coverage

enahum · 2024-12-19T04:31:15Z

app/utils/post/index.ts

add unit test for code coverage

mm-cloud-bot added the release-note label Oct 22, 2024

Rajat-Dabade self-assigned this Oct 22, 2024

Rajat-Dabade marked this pull request as ready for review November 5, 2024 12:34

Rajat-Dabade requested review from larkox and enahum November 5, 2024 12:34

larkox requested changes Nov 6, 2024

View reviewed changes

Rajat-Dabade added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester 2: UX Review Requires review by a UX Designer labels Nov 7, 2024

Rajat-Dabade requested review from asaadmahmood and yasserfaraazkhan November 7, 2024 12:32

yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test E2E iOS tests for PR Run iOS E2E Detox tests labels Nov 8, 2024

Rajat-Dabade requested a review from larkox November 13, 2024 10:46

yasserfaraazkhan added E2E iOS tests for PR Run iOS E2E Detox tests and removed E2E iOS tests for PR Run iOS E2E Detox tests labels Nov 14, 2024

yasserfaraazkhan requested changes Nov 18, 2024

View reviewed changes

Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Nov 18, 2024

Rajat-Dabade requested a review from yasserfaraazkhan November 18, 2024 18:51

yasserfaraazkhan requested changes Nov 18, 2024

View reviewed changes

Rajat-Dabade mentioned this pull request Nov 20, 2024

Unit test for mobile draft #8363

Open

5 tasks

asaadmahmood requested changes Nov 25, 2024

View reviewed changes

Rajat-Dabade requested a review from asaadmahmood November 26, 2024 14:45

yasserfaraazkhan approved these changes Dec 11, 2024

View reviewed changes

fix the theme issue and navigation issue

cd93248

Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Dec 11, 2024

Rajat-Dabade added 2 commits December 11, 2024 16:56

reverted back the draft click navigation changes

8965394

observing draft when hitting back button

8c8e6f6

Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Dec 11, 2024

removed the unwanted animiation

be07659

enahum requested changes Dec 13, 2024

View reviewed changes

Rajat-Dabade added 14 commits December 16, 2024 13:58

updated regex for parsing markdown

ab7ce3a

removed unnecessary checks and change folder name

2b5a671

removed react memo and merge unwanted observes function

22f1528

removed unnecessary comments

a542590

changed the name for observing and querying draft function

ea131a8

removed memo from component level

1ca7ddb

Text to FormattedText component

8a1952e

Text to formatted text, change image name

2941ab1

added confirmation modal for deleting draft from bottomsheet

07ec62c

using common send_handler for both draft and post

80681bd

removed magic number for tooltip and bottomsheet

9ea0914

renamed channel_info to draft_post_header

ba7fb70

text to formattedText for Edit drafts

8d427db

removed unnecessary changes

2954843

Rajat-Dabade requested a review from enahum December 17, 2024 13:07

minor fixes

d4c988b

enahum requested changes Dec 19, 2024

View reviewed changes

mounting draft only when there is draft

2fca19c

	import CompassIcon from '../compass_icon';
	import CompassIcon from '@components/compass_icon';


		import type {AvailableScreens} from '@typings/screens/navigation';

		const edges: Edge[] = ['left', 'right'];

Mobile drafts #8280

Are you sure you want to change the base?

Mobile drafts #8280

Conversation

Rajat-Dabade commented Oct 22, 2024 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

Rajat-Dabade commented Oct 22, 2024

larkox left a comment

Choose a reason for hiding this comment

Rajat-Dabade commented Nov 12, 2024

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

Rajat-Dabade commented Nov 18, 2024

Rajat-Dabade commented Nov 18, 2024

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

asaadmahmood left a comment • edited Loading

Choose a reason for hiding this comment

yasserfaraazkhan left a comment • edited Loading

Choose a reason for hiding this comment

enahum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rajat-Dabade commented Oct 22, 2024 •

edited

Loading

asaadmahmood left a comment •

edited

Loading

yasserfaraazkhan left a comment •

edited

Loading