Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MM-60308] Add a set of "Developer Mode" settings that allow the user to turn off systems or force the app to behave a certain way #3144

Merged
merged 9 commits into from
Sep 18, 2024

Conversation

devinbinnie
Copy link
Member

Summary

This PR creates a Developer Mode, hidden behind an environment variable that when set, will unlock certain options allowing users to fiddle with the behaviour of the Desktop App. This mode is designed to aid developers in debugging performance issues with the Desktop App, where we believe certain systems with the Desktop App may be the culprit.

The settings added here are:

  • Browser Mode Only: Completely disables the preload script, stops web app from knowing it's in the Desktop App
    • Should be the best indicator of if something we're doing with the web app is causing it to slow down or retain memory
    • NOTE: Disables notifications, cross-tab navigation, unread/mentions badges, calls widget, breaks resizing on macOS
  • Disable Notification Storage: Turns off the maps that hold references to the notifications until they've been clicked
    • Good for debugging if we're just holding onto too many references to unused notifications
  • Disable User Activity Monitor: Turns off the interval that checks if the user is AFK or not
    • Good for debugging if that is affecting the user status negatively, or causing other strange behavior
  • Disable Context Menu: Turns off the context menu attached to the BrowserViews
    • Just a sanity check around if that library is causing anything odd
  • Force Legacy API: Forces the app to revert back to the old messaging API instead of the newer contextBridge API
    • Another sanity check to see if the new API is responsible for holding onto memory
  • Force New API: Forces the app to use the contextBridge API and completely disables the legacy one
    • Currently the application runs some listeners for the legacy API side-by-side, so this forces them off entirely.

This mode should not be turned on by anyone who isn't a developer, or was instructed by a Mattermost developer to turn this on. When the feature is on, an indicator will show up on the top bar showing that you are in Developer Mode:

Screen Recording 2024-09-11 at 11 26 00 AM

Ticket Link

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

Add `Developer Mode` settings to help debug performance issues

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer 1: UX Review Requires review by a UX Designer 3: Security Review Review requested from Security Team labels Sep 11, 2024
@devinbinnie devinbinnie added this to the v5.10.0 milestone Sep 11, 2024
@devinbinnie devinbinnie added the Build Apps for PR Builds signed builds for testing label Sep 11, 2024
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM, just one small detail.

src/main/app/initialize.ts Show resolved Hide resolved
@@ -82,6 +82,7 @@ export class UserActivityMonitor extends EventEmitter {
*/
stopMonitoring() {
clearInterval(this.systemIdleTimeIntervalID);
this.systemIdleTimeIntervalID = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related to Developer Mode? Just wondering

Copy link
Member Author

Choose a reason for hiding this comment

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

Not directly, it fixed an issue where if we wanted to start monitoring again, we check if the ID is <0. But we never reset it after we stopped monitoring, so it wouldn't be able to restart.

@devinbinnie devinbinnie removed the 2: Dev Review Requires review by a core committer label Sep 16, 2024
@devinbinnie devinbinnie removed the 3: Security Review Review requested from Security Team label Sep 17, 2024
src/main/developerMode.ts Show resolved Hide resolved
src/main/notifications/index.ts Show resolved Hide resolved
src/main/developerMode.ts Show resolved Hide resolved
Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

@devinbinnie Not sure how to test this, which env variable do I change and how?

But I'm also not sure if this needs any UX testing. We should just make sure it works as expected on Windows too if we haven't already checked that. Other than that, feel free to remove me as a reviewer and go ahead with this.

@devinbinnie
Copy link
Member Author

@abhijit-singh

Not sure how to test this, which env variable do I change and how?

The variable is MM_DESKTOP_DEVELOPER_MODE and it should be set to true. On Mac, you can run export MM_DESKTOP_DEVELOPER_MODE=true and then run the build from the command line.

But I'm also not sure if this needs any UX testing. We should just make sure it works as expected on Windows too if we haven't already checked that. Other than that, feel free to remove me as a reviewer and go ahead with this.

The reason I asked for UX was to confirm that the indicator (screenshotted above) looked alright, and that the descriptions of the menu items made sense. If that's all okay, you can approve (or remove yourself) and we'll go from there

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Looks good to me!

One quick suggestion — For Force Legacy API and Force New API, we can make it clearer what it is about by changing them to something like Force Legacy Messaging API and Force New Messaging API. I'm 0/5 on it though and it might be obvious enough for the right audience as is.

@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 1: UX Review Requires review by a UX Designer labels Sep 18, 2024
@devinbinnie devinbinnie merged commit 42a0bc4 into master Sep 18, 2024
20 checks passed
@devinbinnie devinbinnie deleted the MM-60308 branch September 18, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Build Apps for PR Builds signed builds for testing Docs/Done release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants