App: [HOLD for payment 2024-04-15] [$500] Characters missing when typing message in compose box

@wildan-m there may be performance implications associated with introducing debounce and throttle functions for tracking typing status.

@tienifr’s proposal works really well and is the cleanest one out there. We can move forward with it. 🎀 👀 🎀 C+ Reviewed.

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@getusha] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/25758/ [@getusha] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/25758/files#r1573699887 [@getusha] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/a, this is hard to reproduce and catch. i don’t think we need other actions to be taken other than adding the tests steps.

Contributor: @tienifr paid $500 via Upwork Contributor+: @getusha paid $500 via Upwork

I created the TestRail GH with the test steps below from the PR, comment if there are better steps.

@getusha, please complete the rest of the BZ checklist above. thx

Tests

1. Open Script Editor (Mac)
2. New Document > Paste the following script

on run
	set textToType to "JUMP "
	repeat
		emulateTyping(textToType)
		delay 1
	end repeat
end run

on emulateTyping(textToType)
	tell application "System Events"
		repeat with i from 1 to count of characters of textToType
			set currentChar to character i of textToType
			keystroke currentChar
			delay 1
		end repeat
	end tell
end emulateTyping

1. Press the play button (run the script)
2. Focus the desktop newDot app composer
3. Wait until you observe that a letter from the word ‘JUMP’ is being omitted after being typed.
4. The word ‘JUMP’ needs to be typed with consistency in the composer. ignore the first word it is because we had a delay when focusing the input.

i think the side effect you mentioned is not that much noticeable and the draft seems to sync anyway. @tienifr am i missing anything?

@getusha Hi the side effect I mentioned (assuming you’re talking about the one you quoted here) here is not about my proposal, it’s about the only sync comment if the tab is not visible approach in this proposal

My proposal doesn’t have that problem (or any other regression/problem as I’ve tested).

If you think it makes sense then we can move forward 🚀

@getusha , what were the results from testing @tienifr 's proposal?

FYI the approach of only sync comment if the tab is not visible, will not work because if we type in a tab then quickly change to another tab, that “another tab” will not have the comment synced (regression), because at the time that tab is visible.

@tienifr your solution works well but i was concerned about this regression, testing it now.

I think relying on another state is unnecessary performance overhead, considering there’s another potential approach that doesn’t rely on any such new state.

useEffect(() => { //if unfocusing debouncedSaveReportComment.flush(); }, [Visibility.hasFocus()]);

Also I doubt implementing it the way you’re doing here will work, since Visibility.hasFocus() is not a state.

And visibility changes are on many different cases, not just when switching tabs, so debounce flushing will happen more frequently than necessary, also will impact performance. Eg. When I’m typing and open another Mac app, it will also trigger flushing (although it doesn’t need to)

We are actually improving the performance and re-renders here by not setting a new and unnecessary draft state on the tab we are working on for a feature that is intended to sync other tabs only.

The flushing of the debounce will only perform a draft save if there something unsaved and we are navigating away, it now just does it immediately, this is not a performance overhead.

If you’re referring to a no-op call being a performance overhead when navigating away then I disagree.

The visibility API being used for my intended purpose can by done in multiple ways as we have an on change call back available in the visibility utils, we can leverage as well

will be reviewing @jeremy-croff’s proposal.

@mallenexpensify could we add external label to see if we can see more proposals?

If you type 1 character per second, and the wait is 1 second, it will now never call the debounce. I think we see a few of those occurrences in your video.

@jeremy-croff i checked this and doesn’t seem to experience this issue & mind sharing your test branch?

@getusha , 👀 plz on @jeremy-croff 's comment above. I’d love to get this resolved soon, if only cuz I personally run in it semi-frequently.

@jeremy-croff Thanks for your comment. To help readers stay focused, I filter my console log with the ‘wildebug’ keyword. This is my personal debug code.

@johncschuster, I’ll take this, I have context from the issue this came form, and the bug.