fix: Fix types associated with auto lock timer call by danjm · Pull Request #25109 · MetaMask/metamask-extension

danjm · 2024-06-06T18:02:00Z

Description

We see the following error in sentry

The problem occurs when the user sets this field in advanced settings

If that field is set to empty, then this code in /ui/pages/settings/advanced-tab/advanced-tab.component.test.js would result in setting the background state property to the string '0':

 if (autoLockTimeLimitBeforeNormalization === '') {
      this.setState({
        autoLockTimeLimitBeforeNormalization,
        autoLockTimeLimit: DEFAULT_AUTO_LOCK_TIME_LIMIT.toString(),
        lockTimeError: '',
      });

This toString call was introduced in #19653, but it doesn't seem to be necessary. It also conflicts with the type of the prop that is passed in to initially set this component state property. Also, the _resetTimer function in the app-state controller clearly expects this value to be a number.

The fix is to not set this component state value as a string. There are a couple of other type related fixes that are not strictly necessary for fixing the bug, but improve the code.

Manual testing steps

Go to the page Advanced Settings
Set the "Auto-lock timer (minutes)" field to a non-zero integer (1 is best)
Wait that many minutes without interacting with MetaMask, you will automatically be locked out
Enter your password to unlock MetaMask
Go to Advanced Settings
Set the "Auto-lock timer (minutes)" field to empty (delete the number in the field but don't enter anything else) and save it
Wait more than a minute, you won't be locked out.
Check the service worker console, there will not be an error.

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-06-06T18:02:16Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

app/scripts/controllers/app-state.js

ui/store/actions.ts

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Gudahtt

LGTM!

davidmurdoch · 2024-06-07T15:15:40Z

app/scripts/controllers/app-state.js

+    // This is a temporary fix until we add a state migration.
+    // Due to a bug in ui/pages/settings/advanced-tab/advanced-tab.component.js,
+    // it was possible for timeoutMinutes to be saved as a string, as explained
+    // in PR 25109. `alarms.create` will fail in that case. We are
+    // converting this to a number here to prevent that failure. Once
+    // we add a migration to update the malformed state to the right type,
+    // we will remove this conversion.
+    const timeoutToSet = Number(timeoutMinutes);


Do we have an issue # for the migration task? I want to add a comment to it that this cast should be removed when the migration work is done.

I do not think we have created one yet

codecov · 2024-06-07T15:34:01Z

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.72%. Comparing base (7eaf6b7) to head (5af6083).
Report is 3 commits behind head on develop.

Files	Patch %	Lines
app/scripts/controllers/app-state.js	14.29%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #25109   +/-   ##
========================================
  Coverage    65.71%   65.72%           
========================================
  Files         1360     1360           
  Lines        54041    54042    +1     
  Branches     14038    14038           
========================================
+ Hits         35513    35515    +2     
+ Misses       18528    18527    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-06-07T15:49:49Z

Builds ready [5af6083]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (49 ± 4 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	107	81	9	4
		domContentLoaded	9	16	10	2	1
		load	42	73	49	8	4
		domInteractive	9	16	10	2	1

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 26 Bytes (0.00%)
ui: -11 Bytes (-0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-06-07T16:09:16Z

Missing release label release-11.16.8 on PR. Adding release label release-11.16.8 on PR and removing other release labels(release-12.1.0), as PR was cherry-picked in branch 11.16.8.

danjm added 2 commits June 6, 2024 15:08

Fix auto lock timer call

94b2520

Add unit test

fb30e43

danjm requested a review from a team as a code owner June 6, 2024 18:02

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jun 6, 2024

Gudahtt reviewed Jun 6, 2024

View reviewed changes

app/scripts/controllers/app-state.js Show resolved Hide resolved

Gudahtt linked an issue Jun 6, 2024 that may be closed by this pull request

TypeError: Error in invocation of alarms.create(optional string name, alarms.AlarmCreateInfo alarmInfo, opti... #25119

Closed

danjm changed the title ~~Fix types associated with auto lock timer call~~ fix: Fix types associated with auto lock timer call Jun 7, 2024

danjm added the team-extension-platform Extension Platform team label Jun 7, 2024

Add comment explaining the temporary fix of a type conversion

c6614f8

Gudahtt reviewed Jun 7, 2024

View reviewed changes

ui/store/actions.ts Outdated Show resolved Hide resolved

Update ui/store/actions.ts

b39987b

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Gudahtt approved these changes Jun 7, 2024

View reviewed changes

NiranjanaBinoy approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'develop' into fix-auto-lock-timer

5af6083

davidmurdoch approved these changes Jun 7, 2024

View reviewed changes

davidmurdoch reviewed Jun 7, 2024

View reviewed changes

danjm merged commit 4d7bbec into develop Jun 7, 2024

danjm deleted the fix-auto-lock-timer branch June 7, 2024 15:35

github-actions bot locked and limited conversation to collaborators Jun 7, 2024

metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 7, 2024

metamaskbot added release-11.16.8 Issue or pull request that will be included in release 11.16.8 and removed release-12.1.0 Issue or pull request that will be included in release 12.1.0 labels Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix types associated with auto lock timer call#25109

fix: Fix types associated with auto lock timer call#25109
danjm merged 5 commits intodevelopfrom
fix-auto-lock-timer

danjm commented Jun 6, 2024 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Jun 6, 2024

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment

Uh oh!

davidmurdoch Jun 7, 2024

Uh oh!

Gudahtt Jun 7, 2024

Uh oh!

codecov bot commented Jun 7, 2024

Uh oh!

metamaskbot commented Jun 7, 2024

Uh oh!

metamaskbot commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

danjm commented Jun 6, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Jun 6, 2024

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

davidmurdoch Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

Gudahtt Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 7, 2024

Codecov Report

Uh oh!

metamaskbot commented Jun 7, 2024

Uh oh!

metamaskbot commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danjm commented Jun 6, 2024 •

edited by github-actions bot

Loading