Fix suggestion menu closes on typing @ at second line #27490

bernhardoj · 2023-09-15T04:31:37Z

Details

On Android, typing a new next on a new line will trigger a scroll callback. The scroll callback is set to hide the suggestion menu.

Fixed Issues

$ #26197
PROPOSAL: #26197 (comment)

Tests

Same as QA Steps

Verify that no errors appear in the JS console

Offline tests

Same as QA Steps

QA Steps

Open any chat
On the composer, type anything on the first line and press Enter to go to the 2nd line
Type @ on the 2nd line
Verify a suggestion menu is showing
Add multiple new lines to the composer so it's scrollable
Add enough text to reach the end of the line with your cursor (the line should end with a space)
Input "@", which should appear on the next line
Verify that the suggestion list shows up
Scroll the composer manually
Verify that the suggestion menu disappears

If you scroll right after typing (< 1 s), it's acceptable for the suggestion menu not to disappear after this interaction
Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-09-15.at.11.55.25.mov

Mobile Web - Chrome

Screen.Recording.2023-09-15.at.12.01.03.mov

Mobile Web - Safari

Screen.Recording.2023-09-15.at.11.58.18.mov

Desktop

Screen.Recording.2023-09-15.at.11.54.23.mov

iOS

Screen.Recording.2023-09-15.at.11.56.07.mov

Android

Screen.Recording.2023-09-15.at.11.59.58.mov

melvin-bot · 2023-09-15T04:31:41Z

@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

bernhardoj · 2023-09-15T04:32:22Z

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

            return;
        }
        suggestionsRef.current.updateShouldShowSuggestionMenuToFalse(false);
    }, [suggestionsRef]);

+    const debouncedHideSuggestionMenu = useMemo(() => _.debounce(hideSuggestionMenu, 200, true), [hideSuggestionMenu]);
+


I set immediate of debounce to true, so the suggestion hides immediately when user scrolls the composer.

Too be honest, I'm not sure if this is correct, as I have trouble reasoning about the current implementation. In the other thread I suggested something that's easier to reason about for me. Please let me know if the alternative way is also convincing to you.

cubuspl42 · 2023-09-15T11:39:05Z

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

-        if (!suggestionsRef.current) {
+    const hideSuggestionMenu = useCallback(() => {
+        if (!suggestionsRef.current || shouldIgnoreScrollCallback.current) {
+            shouldIgnoreScrollCallback.current = false;


I'm not sure whether this is a right way to lower this flag...

Wouldn't this be more direct?

const debouncedLowerIsScrollLikelyLayoutTriggered = useDebounce(() => { isScrollLikelyLayoutTriggered.value = false; }, 200); const raiseIsScrollLikelyLayoutTriggered = () => { isScrollLikelyLayoutTriggered.value = true; debouncedLowerIsScrollLikelyLayoutTriggered(); }

Then, we could call raiseIsScrollLikelyLayoutTriggered when the text changes. And check if isScrollLikelyLayoutTriggered.value directly in the onScoll callback.

Also, I suggested using a useDebounce hook instead of directly calling _.debounce inside useMemo, because technically speaking starting a debounce is a side-effect...

I think that a possible implementation could be:

const useDebounce = (func, wait, options) => { const debouncedFnRef = useRef(); useEffect(() => { const debouncedFn = _.debounce(func, wait, options); debouncedFnRef.current = debouncedFn; return () => debouncedFn.cancel(); }); return (...args) => { const debouncedFn = debouncedFnRef.current; if (debouncedFn) { debouncedFn(...args); } }; };

(Based on lodash debounce)

But this would need to be tested.

Interesting. I'm actually having trouble deciding where to set the value back to false, seeing your alternative is better. I will try it and the useDebounce hook and update if all going well.

Found 2 issues:

The useEffect does not have any deps so it's always triggered on re-render making the debounce get canceled all the time. I fixed it by putting func, wait, options into the deps array.

We need a longer timeout now, from my testing 500 is enough. Looks like we need a longer time because we are debouncing it from onChangeText instead of from onScroll. The extra 300ms is possibly the time gap between onChangeText and onScroll calls.

The question is, are we okay to make the timeout 500ms?

The useEffect does not have any deps

Yeah, I was writing it out of my head, it was just a starting point. You're right that deps are required here.

The question is, are we okay to make the timeout 500ms?

You're talking about a Native build, I presume?

We're fine with a value that's good at distinguishing the layout-related scrolls from the manual ones, taking into account that a real user rarely scrolls immediately after typing.

We should test this on a release build, preferably on a physical device, as in my experience it makes a difference. Compiling a release build is very slow, so you can create a spin-off branch that adds a debug UI component for choosing the wait value. Visually, it can be placed anywhere. Then we can experiment with this manually.

Yep. So, are we good with the 500ms? I will push the update if we have no other concerns

I think we are, I'm not against any particular number, as I mentioned. I will test it and figure out if I can see any UX issues with it. Because we are in agreement that the only thing this number does is draw the boundary between true/false positives (/negatives) on debug/release modes, right?

Also, the constant itself isn't the only open topic here. We also have the issue of impureness in useMemo and the wording of the comment we leave for the others.

Because we are in agreement that the only thing this number does is draw the boundary between true/false positives (/negatives) on debug/release modes, right?

yes

We also have the issue of impureness in useMemo and the wording of the comment we leave for the others.

I will address this along with this.

I will probably push the update tomorrow. (it's midnight)

Sure, I didn't mean to rush

Pushed the update, only comment left.

cubuspl42 · 2023-09-15T11:43:10Z

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

            return;
        }
        suggestionsRef.current.updateShouldShowSuggestionMenuToFalse(false);
    }, [suggestionsRef]);

+    const debouncedHideSuggestionMenu = useMemo(() => _.debounce(hideSuggestionMenu, 200, true), [hideSuggestionMenu]);


We definitely need to explain ourselves out of this 200, as in Expensify we're (rightfully) skeptical against timeout-based solutions. We need to comment why do this and why 200 ms.

src/hooks/useDebounce.js

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

src/hooks/useDebounce.js

cubuspl42 · 2023-09-20T09:01:44Z

src/hooks/useDebounce.js

+        return debouncedFn.cancel;
+    }, [func, wait, options]);
+
+    return debouncedFnRef.current;


Actually, maybe let's wrap this into a lambda that calls debouncedFnRef.current only if it's not undefined? I think the case when it would be undefined are always an error, as we don't expect the returned function to be called inside a render function directly, but maybe dropping a call in a corner case is better than a crash?...

I actually prefer the opposite so we can catch the error/crash earlier, but I'm okay with the suggestion. Updated.

I know what you mean. Perfectly, it would be great to combine both things, i.e. a safe handling of a corner case + an analytics report of an error. But we don't have such conventions in Expensify.

If you disagree with a suggestion, you can always say so before applying the change. Sometimes C+s will push for a change, sometimes not.

It's okay, I will say it if it is something that I strongly disagree 😄.

cubuspl42 · 2023-09-20T09:03:11Z

src/hooks/useDebounce.js

+ * @param {Boolean} options.trailing Specify invoking on the trailing edge of the timeout.
+ * @returns Returns the new debounced function.
+ */
+export default function useDebounce(func, wait, options) {


I ask for a review of this new code pattern

👍 to useDebounce but I'd maybe include some notes about passing a stable function reference to it in order for it to work as expected.

I would start with a comment and a few examples using useCallback so people will copy examples with stable references 🙂

@bernhardoj

I believe that the examples don't have to be in the JSDoc, maybe we can start by giving a good example in our PR?

We can just explicitly document that the debouncing operator resets its internal state when func changes, so it's recommended to provide stable function references to achieve predictable/expected results.

Updated the comment.

maybe we can start by giving a good example in our PR?

Yeah, I think the use of useCallback in the composer code as the example is enough.

cubuspl42 · 2023-09-21T07:53:36Z

I think we're looking good, I'll start completing the checklist.

cubuspl42 · 2023-09-21T10:46:17Z

Please merge main

bernhardoj · 2023-09-21T10:47:41Z

Done

cubuspl42 · 2023-09-21T10:55:57Z

@bernhardoj

Scrolling doesn't close the suggestion list for me on the feature branch... Do you reproduce this behavior?

The code looked good for me on paper, but it doesn't seem to achieve its goal (unless there is a flaw in my testing)

prod.mp4

feature-branch.mp4

bernhardoj · 2023-09-21T10:57:09Z

Hmm, that's weird. Last time I tested it works fine. Let me check it!

cubuspl42 · 2023-09-21T10:59:20Z

@bernhardoj Also, update the test steps to include this case

bernhardoj · 2023-09-21T11:46:26Z

Somehow the lodash debounce is not fired if we pass undefined trailing options. I tried defaulting it to true to make it work.

update the test steps to include this case

I actually included this case in the recording, but I don't know what to write for the test step 😭.

Keep pressing Shift+Enter (to add a new line) until you reach the composer maximum line
Type anything to fill the current line
Press space once and type @ (make sure @ is wrapped to the next line)
Verify a suggestion menu shows

Do you think the wording is good enough?

cubuspl42 · 2023-09-21T12:00:55Z

@bernhardoj It's okey, I guess. Could also be:

Ensure that the composer has many enough lines so it's scrollable (they can be empty)
Add enough text to reach the end of line with your cursor (the line should end with a space)
Input "@", which should appear on the next line
Verify that the suggestion list shows up

...but also...

Ensure that the composer has many enough lines so it's scrollable (they can be empty)
Make the suggestion menu show up by inputting "@"
After at least a second, scroll the composer manually
Verify that the suggestion menu disappears
- If you scroll right after typing (< 1 s), it's acceptable for the suggestion menu not to disappear after this interaction

bernhardoj · 2023-09-21T12:54:47Z

Great, thanks! Updated with a little bit modification.

cubuspl42 · 2023-09-22T09:17:48Z

@bernhardoj "After at least a second, " can be removed in my opinion. I was too defensive suggesting that. I have big trouble reproducing a case when the manual scroll is considered layout-triggered, so I think we can just drop that part.

cubuspl42 · 2023-09-22T09:21:43Z

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

@@ -324,8 +346,8 @@ function ComposerWithSuggestions({
        [suggestionsRef],
    );

-    const updateShouldShowSuggestionMenuToFalse = useCallback(() => {
-        if (!suggestionsRef.current) {
+    const hideSuggestionMenu = useCallback(() => {


I really like this name change, it's like changing giveCatLikeHighPitchedSound to meow

cubuspl42 · 2023-09-22T09:22:37Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

fix-suggestions-ios-web.mp4

Mobile Web - Chrome

fix-suggestions-android-web-compressed.mp4

Mobile Web - Safari

fix-suggestions-ios-web.mp4

Desktop

iOS

fix-suggestions-ios.mp4

Android

fix-suggestions-android-compressed.mp4

bernhardoj · 2023-09-22T09:26:14Z

"After at least a second, " can be removed in my opinion

Removed

Should we remove this too?

If you scroll right after typing (< 1 s), it's acceptable for the suggestion menu not to disappear after this interaction

cubuspl42 · 2023-09-22T10:04:26Z

"After at least a second, " can be removed in my opinion

Removed

Should we remove this too?

If you scroll right after typing (< 1 s), it's acceptable for the suggestion menu not to disappear after this interaction

I don't have a strong opinion here.

OSBotify · 2023-09-26T07:20:50Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-09-28T19:19:31Z

🚀 Deployed to staging by https://github.com/Gonals in version: 1.3.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-10-02T17:23:26Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.75-12 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-10-02T17:52:04Z

🚀 Deployed to staging by https://github.com/Gonals in version: 1.3.76-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-10-03T15:53:01Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

ignore scroll callback if triggered by text update

a3b1597

bernhardoj requested a review from a team as a code owner September 15, 2023 04:31

melvin-bot bot removed the request for review from a team September 15, 2023 04:31

melvin-bot bot requested a review from cubuspl42 September 15, 2023 04:31

bernhardoj commented Sep 15, 2023

View reviewed changes

cubuspl42 reviewed Sep 15, 2023

View reviewed changes

bernhardoj added 4 commits September 16, 2023 00:27

Merge branch 'main' into fix/26197-suggestion-hide-on-typing

8a72080

Merge branch 'main' into fix/26197-suggestion-hide-on-typing

cd7f773

Merge branch 'main' into fix/26197-suggestion-hide-on-typing

542d7cd

fix suggestion menu hides on typing

d9ab017

cubuspl42 reviewed Sep 20, 2023

View reviewed changes

src/hooks/useDebounce.js Outdated Show resolved Hide resolved

cubuspl42 suggested changes Sep 20, 2023

View reviewed changes

bernhardoj added 3 commits September 20, 2023 16:37

add jsdoc

a187423

add comment

3a577e6

use lodash debounce

762e549

cubuspl42 reviewed Sep 20, 2023

View reviewed changes

src/hooks/useDebounce.js Outdated Show resolved Hide resolved

cubuspl42 suggested changes Sep 20, 2023

View reviewed changes

src/hooks/useDebounce.js Outdated Show resolved Hide resolved

bernhardoj added 4 commits September 20, 2023 16:42

update params and jsdoc

c212e9e

add comment

2f93e76

update comment

81d66d4

Merge branch 'main' into fix/26197-suggestion-hide-on-typing

8ebedc7

cubuspl42 reviewed Sep 20, 2023

View reviewed changes

bernhardoj added 5 commits September 20, 2023 17:10

make sure debounced fn is not undefined before calling it

a6a03e1

add return type

34b6825

prettier

dfa7435

update comment

4fe1076

prettier

76b9cc7

Merge branch 'main' into fix/26197-suggestion-hide-on-typing

1e4684c

make trailing default to true

489b600

cubuspl42 reviewed Sep 22, 2023

View reviewed changes

cubuspl42 approved these changes Sep 22, 2023

View reviewed changes

melvin-bot bot requested a review from Gonals September 22, 2023 09:23

Gonals approved these changes Sep 26, 2023

View reviewed changes

Gonals merged commit b9a2f62 into Expensify:main Sep 26, 2023
1 check passed

OSBotify mentioned this pull request Sep 28, 2023

Deploy Checklist: New Expensify 2023-09-28 #28412

Closed

lanitochka17 mentioned this pull request Sep 29, 2023

[HOLD 2023-10-16] [$125] mweb/Safari - Composer - Cursor does not moved to a new line in the composer when tapped on the "return" #28501

Closed

6 tasks

OSBotify mentioned this pull request Oct 2, 2023

Deploy Checklist: New Expensify 2023-10-02 #28635

Closed

melvin-bot bot mentioned this pull request Oct 2, 2023

[HOLD for payment 2023-10-09] [$500] Chat - User suggestion @ list not appearing on the second #26197

Closed

6 tasks

DrLoopFall mentioned this pull request Mar 22, 2024

[$500] Android - Chat - User suggestion list is not opening on entering @ in 2nd line. #37660

Closed

6 tasks

Fix suggestion menu closes on typing @ at second line #27490

Fix suggestion menu closes on typing @ at second line #27490

Conversation

bernhardoj commented Sep 15, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 15, 2023

Choose a reason for hiding this comment

cubuspl42 Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

cubuspl42 Sep 15, 2023 • edited Loading

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

cubuspl42 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

bernhardoj Sep 20, 2023 • edited Loading

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

cubuspl42 commented Sep 21, 2023

cubuspl42 commented Sep 21, 2023

bernhardoj commented Sep 21, 2023

cubuspl42 commented Sep 21, 2023

bernhardoj commented Sep 21, 2023

cubuspl42 commented Sep 21, 2023

bernhardoj commented Sep 21, 2023

cubuspl42 commented Sep 21, 2023

bernhardoj commented Sep 21, 2023 • edited Loading

cubuspl42 commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cubuspl42 commented Sep 22, 2023

Reviewer Checklist

Screenshots/Videos

bernhardoj commented Sep 22, 2023

cubuspl42 commented Sep 22, 2023

OSBotify commented Sep 26, 2023

OSBotify commented Sep 28, 2023

OSBotify commented Oct 2, 2023

OSBotify commented Oct 2, 2023

OSBotify commented Oct 3, 2023

bernhardoj commented Sep 15, 2023 •

edited

Loading

cubuspl42 Sep 15, 2023 •

edited

Loading

cubuspl42 Sep 15, 2023 •

edited

Loading

cubuspl42 Sep 20, 2023 •

edited

Loading

bernhardoj Sep 20, 2023 •

edited

Loading

bernhardoj commented Sep 21, 2023 •

edited

Loading