[HOLD for payment 2023-09-21] [$1000] dev: ESC keyboard shortcut shows up in modal on second render on login page #25448

kavimuru · 2023-08-17T23:44:38Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

signout if you are logged in
refresh the page
press cmd+j to open shortcut modal
now close the modal and observe content while it closes
now open it again

Expected Result:

ESC shortcut should always be shown in the modal

Actual Result:

ESC shortcut is visible for a brief moment while shortcut modal closes, later it is shown consistently

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: v1.3.55-7
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-08-08.at.4.52.04.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @chiragxarora
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691493991739999

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a6b0de738da985ac
Upwork Job ID: 1693559881555742720
Last Price Increase: 2023-08-21
Automatic offers:

mollfpr | Reviewer | 26508978
chiragxarora | Reporter | 26508980

melvin-bot · 2023-08-17T23:44:43Z

Triggered auto assignment to @conorpendergrast (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot · 2023-08-17T23:44:45Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

kavimuru · 2023-08-17T23:44:58Z

Proposal by @chiragxarora

Please re-state the problem that we are trying to solve in this issue.

ESC keyboard shortcut shows up in modal on second render on login page

What is the root cause of that problem?

Root cause of this issue is as follows:
We add shortcuts to the modal via documentedShortcuts object and a shortcut gets added to it only when that shortcut is subscribed here in subscribe function

App/src/libs/KeyboardShortcut/index.js

Lines 135 to 142 in fdcae9f

    
           if (descriptionKey) { 
        
               documentedShortcuts[displayName] = { 
        
                   shortcutKey: key, 
        
                   descriptionKey, 
        
                   displayName, 
        
                   modifiers, 
        
               }; 
        
           }

Now coming to the modal, we are subscribing to the ESC shortcut inside subscribeOpenModalShortcuts function here

App/src/components/KeyboardShortcutsModal.js

Lines 49 to 56 in fdcae9f

    
           const subscribeOpenModalShortcuts = () => { 
        
               // Allow closing the modal with the both Enter and Escape keys 
        
               // Both callbacks have the lowest priority (0) to ensure that they are called before any other callbacks 
        
               // and configured so that event propagation is stopped after the callback is called (only when the modal is open) 
        
               subscribedOpenModalShortcuts.current = [ 
        
                   KeyboardShortcut.subscribe( 
        
                       closeShortcutEscapeModalConfig.shortcutKey,

And from when the page loads till the point we open the modal once using CMD+J, this piece of code never executes because isShortcutsModalOpen remains false till that point

App/src/components/KeyboardShortcutsModal.js

Lines 128 to 131 in fdcae9f

    
           if (isShortcutsModalOpen) { 
        
               // The modal started open, which can happen if you reload the page when the modal is open. 
        
               subscribeOpenModalShortcuts(); 
        
           }

same case with second useEffect, where it always lands in the else object

App/src/components/KeyboardShortcutsModal.js

Lines 143 to 152 in fdcae9f

    
           useEffect(() => { 
        
               if (isShortcutsModalOpen) { 
        
                   subscribeOpenModalShortcuts(); 
        
               } else { 
        
                   // Modal is closing, remove keyboard shortcuts 
        
                   unsubscribeOpenModalShortcuts(); 
        
               } 
        
               // subscribeOpenModalShortcuts and unsubscribeOpenModalShortcuts functions are not added as dependencies since they don't change between renders 
        
               // eslint-disable-next-line react-hooks/exhaustive-deps 
        
           }, [isShortcutsModalOpen]);

Thus the ESC shortcut is never subscribed until we open the modal once on login screen, unlike other places

Now once you open the modal, it gets subscribed, and now when you close it, you get to see the ESC shortcut for a brief moment and after that, we see it whenever we open the modal

What changes do you think we should make in order to solve the problem?

We need to subscribe to openmodal shortcuts without any condition on modal mounting which essentially means the following changes:

replacing this

App/src/components/KeyboardShortcutsModal.js

Lines 128 to 131 in fdcae9f

    
           if (isShortcutsModalOpen) { 
        
               // The modal started open, which can happen if you reload the page when the modal is open. 
        
               subscribeOpenModalShortcuts(); 
        
           }

with this

subscribeOpenModalShortcuts();

Results

What alternative solutions did you explore? (Optional)

None

Charan-hs · 2023-08-18T13:09:38Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

ESC keyboard shortcut shows up in modal on second render on login page

What is the root cause of that problem?

App/src/components/KeyboardShortcutsModal.js

Line 44 in fdcae9f

const shortcuts = KeyboardShortcut.getDocumentedShortcuts();

When the components load (initial), we call the function KeyboardShortcut.getDocumentedShortcuts() and store the result in the constant variable shortcuts. In useEffect we are callingsubscribeOpenModalShortcuts() subscribe function but fail to handle any changes in the shortcuts variable. Based on changes we are not rendering it.

For this #25448 (comment) cause is

App/src/libs/KeyboardShortcut/index.js

Lines 83 to 87 in fdcae9f

    
            */ 
        
           function unsubscribe(displayName, callbackID) { 
        
               eventHandlers[displayName] = _.reject(eventHandlers[displayName], (callback) => callback.id === callbackID); 
        
           }

When unsubscribing, we remove event listeners but not shortcuts from the variable documentedShortcuts.

What changes do you think we should make in order to solve the problem?

App/src/components/KeyboardShortcutsModal.js

Line 44 in fdcae9f

const shortcuts = KeyboardShortcut.getDocumentedShortcuts();

we can handle shortcuts variable with UseState function so it will take care the onChanges render issue.

    const [shortcuts,setShortcurts] = useState([])

In these two places call SetState Function along with subscribeOpenModalShortcuts()

App/src/components/KeyboardShortcutsModal.js

Lines 143 to 146 in fdcae9f

    
           useEffect(() => { 
        
               if (isShortcutsModalOpen) { 
        
                   subscribeOpenModalShortcuts(); 
        
               } else {

App/src/components/KeyboardShortcutsModal.js

Lines 127 to 131 in fdcae9f

    
           if (isShortcutsModalOpen) { 
        
               // The modal started open, which can happen if you reload the page when the modal is open. 
        
               subscribeOpenModalShortcuts(); 
        
           }

if (isShortcutsModalOpen) {
            subscribeOpenModalShortcuts();
            setShortcurts(KeyboardShortcut.getDocumentedShortcuts())

        }

We are setting the state when the modal is opened, Once the component unmounts reset the state.

bug.resolved.shortcut.mp4

melvin-bot · 2023-08-21T09:30:21Z

@conorpendergrast Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

conorpendergrast · 2023-08-21T09:44:13Z

@Charan-hs, yes please report to #expensify-bugs! I'll hide the comments here for that separate bug, to keep this one focused on the bug in the OP

melvin-bot · 2023-08-21T09:45:17Z

Job added to Upwork: https://www.upwork.com/jobs/~01a6b0de738da985ac

melvin-bot · 2023-08-21T09:45:21Z

Current assignee @conorpendergrast is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-08-21T09:45:26Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

tienifr · 2023-08-21T12:32:29Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

ESC shortcut is visible for a brief moment while shortcut modal closes, later it is shown consistently

What is the root cause of that problem?

In here, we're getting the list of shortcuts to display, but we're using the method KeyboardShortcut.getDocumentedShortcuts which retrieves a static value documentedShortcuts from another file, so when documentedShortcuts is updated in the other file, we don't have any knowledge of it so the UI is not updated.

So when new shortcuts are subscribed, for example in here, the UI is not updated.

What changes do you think we should make in order to solve the problem?

We need to make sure shortcuts in KeyboardShortcutsModal will always be updated whenever a new shortcut is subscribed/unsubscribed.

To do this, we can add a keyboardShortcuts key to Onyx, and subscribe to it from KeyboardShortcutsModal, whenever a shortcut is subscribed, unsubscribed, we'll update keyboardShortcuts in Onyx.

We cannot just use a state in KeyboardShortcutsModal since it won't work if the shortcut update is from another component, and that update occurs after we have reset the state in KeyboardShortcutsModal.

What alternative solutions did you explore? (Optional)

If we don't want to use Onyx, we can also create a useDocumentedShortcuts in here and use it instead of the getDocumentedShortcuts, when the documentedShortcuts changes (a shortcut is subscribed, unsubscribed), we'll update the state in the hook as well. Then use the hook in KeyboardShortcutsModal, this will make sure KeyboardShortcutsModal is always updated via hook with the latest value of documentedShortcuts.

mollfpr · 2023-08-22T14:26:23Z

I'll review the proposals in the morning; I ran out of time and got cold (again) 🙏

Charan-hs · 2023-08-22T14:54:13Z

@tienifr
When KeyboardShortcutsModal is mounted, it calls KeyboardShortcut.getDocumentedShortcuts to update the state and render changes. Event handlers are handled in here. we are only setting the state for viewing purposes, which also saves storage.

tienifr · 2023-08-23T03:18:33Z

setting the state for viewing purposes

@Charan-hs This is exactly why we should make the state in the component always in sync with the actual data, currently the data is in another place and any changes to that data, will not reflect in the state.

Setting the state when we mount the KeyboardShortcutsModal seems like a workaround to me, what if the shortcuts are subscribed/unsubscribed after the KeyboardShortcutsModal mounted? The state will be stale and this issue will occur again.

mollfpr · 2023-08-23T15:42:02Z

@chiragxarora I think there will be a drawback: #18425. Even thought we don't have it now, I'd like to keep the logic as it is.

@tienifr I don't think we need new Onyx keys for this where it can be avoided

I think using the local state seems to be working. It makes sense to me we get the current shortcuts list, and we only care to show the list when the shortcuts modal opens.

The proposal from @Charan-hs looks good to me!

🎀 👀 🎀 C+ reviewed!

melvin-bot · 2023-09-11T12:03:08Z

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

conorpendergrast · 2023-09-11T12:04:48Z

@Christinadobrzyn I'm on parental leave; re-assigning! A proposal has been accepted and is getting worked on at the moment

melvin-bot · 2023-09-12T15:25:26Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @Charan-hs got assigned: 2023-09-05 14:52:49 Z
when the PR got merged: 2023-09-12 15:25:25 UTC
days elapsed: 5

On to the next one 🚀

Charan-hs · 2023-09-12T16:15:58Z

Thanks @mollfpr @arosiclair @conorpendergrast

@arosiclair is this still eligible for a speed bonus?

melvin-bot · 2023-09-14T10:18:08Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-09-14T10:18:11Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Fix: 25448 ESC keyboard shortcut shows up in modal on second render on login page #26809

If no regressions arise, payment will be issued on 2023-09-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@mollfpr requires payment offer (Reviewer)
@Charan-hs requires payment
@chiragxarora requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-09-14T10:18:13Z

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:

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:
[@mollfpr] 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:
[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Christinadobrzyn · 2023-09-15T19:08:24Z

prepping for payment...

@mollfpr do you think we need a regression test for this?
@mollfpr you're paid through Upwork correct?
@Charan-hs @chiragxarora are you paid through Upwork? if yes, can you please apply for the job here https://www.upwork.com/jobs/~01a6b0de738da985ac

Christinadobrzyn · 2023-09-15T20:25:16Z

Just bringing up this comment

The speed bonus timeline might need to be reevaluated.

Looks like the PR was created on Sept 5th
Final C+ review before merge was Sept 8th (3rd day)
Then there was a delay with the merge until Sept 11th because of a 'freeze'

So I think the bonus would actually apply here? @arosiclair @mollfpr?

chiragxarora · 2023-09-16T09:51:58Z

@Christinadobrzyn pls send offer, I don't have connects

arosiclair · 2023-09-16T23:24:03Z

So I think the bonus would actually apply here? @arosiclair @mollfpr?

Yeah that makes sense to me. There's no hard rule but here's a thread where we talked about it.

mollfpr · 2023-09-19T15:57:03Z

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:

I couldn't make sure if there's a PR causing the regression. The implementation was already there years ago, and a lot of changes on our keyboard shortcut handle.

[@mollfpr] 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:

The regression step should be enough.

[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Sign out from the app
Refresh the page (for iOS and Android you need to close and reopen the app)
Press cmd+j to open the shortcut modal (testing on iOS and Android required to use physical keyboard)
Verify escape key has been added
👍 or 👎

Christinadobrzyn · 2023-09-19T22:50:22Z

@chiragxarora I sent an Upwork invitation to you - here's the job posting - https://www.upwork.com/jobs/~01a6b0de738da985ac

chiragxarora · 2023-09-20T01:11:54Z

Accepted! @Christinadobrzyn

mollfpr · 2023-09-22T05:47:15Z

Friendly bump @Christinadobrzyn

Christinadobrzyn · 2023-09-22T21:23:42Z

Regression test: https://github.com/Expensify/Expensify/issues/319173

This issue was created before Aug 30th so we'll adhere to the old payment scale

Payouts due:

Issue Reporter: $250 @chiragxarora
Contributor: $1000 + 500 speed bonus @Charan-hs
Contributor+: $1000 + 500 speed bonus @mollfpr

Eligible for 50% #urgency bonus? Y - #25448 (comment)

Upwork job is here.

Paid! Thanks for all your help!

kavimuru added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 17, 2023

melvin-bot bot assigned conorpendergrast Aug 17, 2023

This comment was marked as off-topic.

Sign in to view

melvin-bot bot added the Overdue label Aug 21, 2023

melvin-bot bot removed the Overdue label Aug 21, 2023

conorpendergrast added the External Added to denote the issue can be worked on by a contributor label Aug 21, 2023

melvin-bot bot changed the title ~~dev: ESC keyboard shortcut shows up in modal on second render on login page~~ [$1000] dev: ESC keyboard shortcut shows up in modal on second render on login page Aug 21, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 21, 2023

conorpendergrast removed the Needs Reproduction Reproducible steps needed label Aug 21, 2023

melvin-bot bot assigned mollfpr Aug 21, 2023

melvin-bot bot assigned Christinadobrzyn Sep 11, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 11, 2023

This comment was marked as duplicate.

Sign in to view

conorpendergrast added Weekly KSv2 and removed Daily KSv2 labels Sep 11, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Sep 14, 2023

melvin-bot bot changed the title ~~[$1000] dev: ESC keyboard shortcut shows up in modal on second render on login page~~ [HOLD for payment 2023-09-21] [$1000] dev: ESC keyboard shortcut shows up in modal on second render on login page Sep 14, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Sep 14, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 21, 2023

Christinadobrzyn closed this as completed Sep 22, 2023

[HOLD for payment 2023-09-21] [$1000] dev: ESC keyboard shortcut shows up in modal on second render on login page #25448

[HOLD for payment 2023-09-21] [$1000] dev: ESC keyboard shortcut shows up in modal on second render on login page #25448

Comments

kavimuru commented Aug 17, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Aug 17, 2023

melvin-bot bot commented Aug 17, 2023 • edited by conorpendergrast Loading

Bug0 Triage Checklist (Main S/O)

kavimuru commented Aug 17, 2023

Proposal by @chiragxarora

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

This comment was marked as off-topic.

This comment was marked as off-topic.

Charan-hs commented Aug 18, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

melvin-bot bot commented Aug 21, 2023

conorpendergrast commented Aug 21, 2023

melvin-bot bot commented Aug 21, 2023

melvin-bot bot commented Aug 21, 2023

melvin-bot bot commented Aug 21, 2023

tienifr commented Aug 21, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

mollfpr commented Aug 22, 2023

Charan-hs commented Aug 22, 2023

tienifr commented Aug 23, 2023

mollfpr commented Aug 23, 2023

melvin-bot bot commented Sep 11, 2023

This comment was marked as duplicate.

conorpendergrast commented Sep 11, 2023

melvin-bot bot commented Sep 12, 2023

Charan-hs commented Sep 12, 2023

melvin-bot bot commented Sep 14, 2023

melvin-bot bot commented Sep 14, 2023 • edited by Christinadobrzyn Loading

melvin-bot bot commented Sep 14, 2023 • edited by Christinadobrzyn Loading

Christinadobrzyn commented Sep 15, 2023 • edited Loading

Christinadobrzyn commented Sep 15, 2023 • edited Loading

chiragxarora commented Sep 16, 2023

arosiclair commented Sep 16, 2023

mollfpr commented Sep 19, 2023

Christinadobrzyn commented Sep 19, 2023

chiragxarora commented Sep 20, 2023

mollfpr commented Sep 22, 2023

Christinadobrzyn commented Sep 22, 2023

kavimuru commented Aug 17, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Aug 17, 2023 •

edited by conorpendergrast

Loading

melvin-bot bot commented Sep 14, 2023 •

edited by Christinadobrzyn

Loading

melvin-bot bot commented Sep 14, 2023 •

edited by Christinadobrzyn

Loading

Christinadobrzyn commented Sep 15, 2023 •

edited

Loading

Christinadobrzyn commented Sep 15, 2023 •

edited

Loading