Skip to content

Conversation

martgil
Copy link
Collaborator

@martgil martgil commented Jan 24, 2025

This PR adds the FlowCrypt's "Secure Reply All" option to Gmail's context menu.

close #5905


Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@martgil martgil requested a review from sosnovsky as a code owner January 24, 2025 02:52
@martgil martgil changed the title #5905 Add secure reply all to gmail context menu #5905 Add secure reply all to GMail context menu Jan 24, 2025
@martgil martgil marked this pull request as draft January 24, 2025 05:01
@martgil
Copy link
Collaborator Author

martgil commented Jan 24, 2025

Hi @sosnovsky, we do have some problem with the recent change. While it works, it may not works best occasionally.

Screen.Recording.2025-01-24.at.4.33.22.PM.mov

That's what I recently noticed when constantly checking it here. The reason why this is very tricky DOM manipulation is that, the actionMenu aka the Gmail context menu is not hidden or present in the document.body not unless a single click on the actionMenuBtn is made which would then programmatically added the Gmail's reply,forward buttons, etc. That makes me solve it by immediately binding the click event and modify the actionMenu right after the click - this solves the bug in delayed injection of secure actions.

However, now that we move it on gmail replace, the time it works may vary now since they're maybe some latency plus the fact that if i click on the actionMenuBtn, it is not what the $(document).click('... is actual targetting but its parent div.aHU.hx and then checks for $(event.currentTarget).find(this.sel.msgActionsBtn).length may not work for some unknown reason. Not to mention that, I may not be clicking the the div.aHU.hx...

Is there any alternative left for us? What if the the function for adding secure action buttons on context menu works "reliably" in this.replacer.runIntervalFunctionsPeriodically() or maybe we could find some efficient way... It's too buggy in a way its unreliable, it showed up when clicked but may occationally failed unlike in runIntervalFunctionsPeriodically() where it had the chance pretty quickly to get it back while still not populating the context menu.

@martgil
Copy link
Collaborator Author

martgil commented Jan 25, 2025

I have been consistently testing this, and it seems like a bug is working against me. It's no joke. When I test the same scenario with the JavaScript console or web developer console enabled, the UI bug does not occur. However, as soon as I close the web developer console and refresh the page, the issue reappears.

It can be tested by opening an encrypted email that was sent to two recipients (to enable reply all too), and then opening the Gmail's context menu. (for both scenario where dev tools is enabled/disabled)

@sosnovsky
Copy link
Collaborator

I think it happens because on document load div.b7.J-M is empty and addMenuButton fails to insert secure buttons, as it doesn't find requested selectors. Probably in $(document).on('click', 'div.aHU.hx', event => { we can add check for $(this.sel.msgActionsMenu).is(':empty'), and if it's empty - retry to add buttons in 100ms.

@sosnovsky
Copy link
Collaborator

Hi @martgil, do you think we can finalize this PR before the end of the week to publish new extension release?

@martgil
Copy link
Collaborator Author

martgil commented Jan 29, 2025

Yes @sosnovsky - I believe thats completely possible. I'll try your suggestion but if that doesn't work. Could you please let me know your thoughts on my latest change for the time being? It tried to address the constant addition of click() event handler. Just in case i need to look for another way to address the issue.

5c01186

…re-reply-all-to-gmail-context-menu-live-test
@martgil
Copy link
Collaborator Author

martgil commented Jan 29, 2025

I think it happens because on document load div.b7.J-M is empty and addMenuButton fails to insert secure buttons, as it doesn't find requested selectors. Probably in $(document).on('click', 'div.aHU.hx', event => { we can add check for $(this.sel.msgActionsMenu).is(':empty'), and if it's empty - retry to add buttons in 100ms.

Thanks for sharing your insight. I'll try it right away.

@sosnovsky
Copy link
Collaborator

Yes @sosnovsky - I believe thats completely possible. I'll try your suggestion but if that doesn't work. Could you please let me know your thoughts on my latest change for the time being? It tried to address the constant addition of click() event handler. Just in case i need to look for another way to address the issue.

5c01186

I think it would still affect performance as binding/unbinding happens each second in this case.
Finding way to bind it only once, should be the most effective way of handling this, but we can have your current solution as a backup option.

@martgil
Copy link
Collaborator Author

martgil commented Jan 29, 2025

Yes @sosnovsky - I believe thats completely possible. I'll try your suggestion but if that doesn't work. Could you please let me know your thoughts on my latest change for the time being? It tried to address the constant addition of click() event handler. Just in case i need to look for another way to address the issue.
5c01186

I think it would still affect performance as binding/unbinding happens each second in this case. Finding way to bind it only once, should be the most effective way of handling this, but we can have your current solution as a backup option.

Thanks @sosnovsky - I have good news. My recent change with the help of your guidance, has worked flawlessly. I'm working on fixing the failing test.

@martgil martgil changed the title #5905 Add secure reply all to GMail context menu #5905 Add Secure Reply-All to Gmail context menu Jan 29, 2025
@sosnovsky
Copy link
Collaborator

Hi @martgil, what issues left to be fixed in this task?
Let's finish it today-tomorrow to be able to publish new release on Wednesday.

@martgil
Copy link
Collaborator Author

martgil commented Feb 3, 2025

Hello @sosnovsky - I'm trying to do my best in fixing this one. If we're using $(document)... I'd get an unreliable results... like i've shared in the above screen recording - sometimes it showed up sometime its not showing at all. so I'm just working now on its reliability since its very tricky.

@martgil
Copy link
Collaborator Author

martgil commented Feb 3, 2025

I would like to see our opportunity to use the previous approach @sosnovsky since that's we're getting reliable results compared to using $(document).on('click', selector) since HTML structure is obscure in Gmail with that approach.

Is there anyhting I could test just to see how much performance loss were worrying about? if its not that much, maybe we could allow it?

@sosnovsky
Copy link
Collaborator

Is there anyhting I could test just to see how much performance loss were worrying about? if its not that much, maybe we could allow it?

Ok, please try to use previous approach, and I'll investigate $(document) usage.
So we'll have at least 1 working solution, and then can decide on better approach.

@martgil martgil force-pushed the issue-5905-add-secure-reply-all-to-gmail-context-menu-live-test branch from 2d15976 to 80aa8b6 Compare February 3, 2025 11:10
@sosnovsky
Copy link
Collaborator

@martgil live tests probably fail because of current 45 minutes execution limit -


Let's increase it to 60 minutes for now, but later will try to merge some live tests, so they'll be executed faster.

@martgil martgil marked this pull request as ready for review February 4, 2025 06:08
@martgil martgil requested a review from sosnovsky February 4, 2025 06:08
@martgil
Copy link
Collaborator Author

martgil commented Feb 4, 2025

I'm just checking any issues on the tests. Meanwhile, @sosnovsky please try the latest change and it should be the most acceptable solution that we can find.

@martgil
Copy link
Collaborator Author

martgil commented Feb 4, 2025

@sosnovsky are you preparing the new FlowCrypt browser extension release? This is already a good PR but the tests are not yet passing (at least one is failing too often) but I'm hoping to get this added. My added tests are working fine.

@sosnovsky
Copy link
Collaborator

@sosnovsky are you preparing the new FlowCrypt browser extension release? This is already a good PR but the tests are not yet passing (at least one is failing too often) but I'm hoping to get this added. My added tests are working fine.

Yes, new browser extension release should be ready tomorrow, on Wednesday, as it includes fixes for 2 issues reported by our customer.

@martgil
Copy link
Collaborator Author

martgil commented Feb 4, 2025

Thanks for your answer, Roma - please feel free to test this one. Meanwhile, its just the unrelated tests that are failing, but the test dedicated for this change is passing without any issues.

@sosnovsky
Copy link
Collaborator

Hi @martgil, let's work on finishing this one today, so everything will be ready for release.
I pushed my proposed changes to 4ddea2e, mock tests pass there, currently waiting for live tests results.

@martgil
Copy link
Collaborator Author

martgil commented Feb 4, 2025

Yes @sosnovsky - I'll finish this one today and do things as you suggested.

Copy link
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@martgil
Copy link
Collaborator Author

martgil commented Feb 4, 2025

Hi @sosnovsky - the live tests are now passing :) Could we merge this one?

@sosnovsky sosnovsky merged commit 86f73f7 into master Feb 4, 2025
12 checks passed
@sosnovsky sosnovsky deleted the issue-5905-add-secure-reply-all-to-gmail-context-menu-live-test branch February 4, 2025 13:25
@martgil martgil requested a review from sosnovsky February 4, 2025 13:27
@martgil
Copy link
Collaborator Author

martgil commented Feb 4, 2025

Thank you, Roma!

@sosnovsky
Copy link
Collaborator

Thanks for finding a well-working solution 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add "secure reply to all" button to message actions menu

2 participants