Skip to content

Conversation

@Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Dec 15, 2025

Explanation

This PR addresses and removes ESLint suppressions from three files within packages/notification-services-controller as part of a larger cleanup effort (ASSETS-2100).

  • Current state and why it needs to change: The identified files contained suppressions for @typescript-eslint/explicit-function-return-type, no-restricted-globals, require-atomic-updates, and id-length. These suppressions masked potential code quality issues and reduced code clarity.
  • Solution:
    • push-utils.test.ts: Explicit return types were added to all functions to satisfy @typescript-eslint/explicit-function-return-type. For no-restricted-globals related to self, targeted eslint-disable-next-line comments with explanations were added, as self is required for testing service worker functionality.
    • push-utils.ts: Explicit return types were added to functions to satisfy @typescript-eslint/explicit-function-return-type. The require-atomic-updates issue was addressed with a targeted eslint-disable-next-line comment and explanation, as the race condition for caching a boolean is acceptable.
    • is-onchain-notification.ts: The id-length violation was fixed by renaming the parameter n to notification for improved readability.
    • All corresponding entries for these files have been removed from eslint-suppressions.json.
  • Changes whose purpose might not be obvious: The eslint-disable-next-line comments for no-restricted-globals and require-atomic-updates are intentional and include inline explanations in the code, detailing why these specific rules are being ignored at those points.

References

  • Fixes ASSETS-2100

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Open in Cursor Open in Web


Note

Adds explicit return types and stricter typings to web push utils and tests, renames a short param in on-chain notification check, and removes related entries from eslint-suppressions.json.

  • Notification Services Push (web):
    • Typing/ESLint: Add explicit return types to getPushAvailability, listenToPushNotificationsReceived, listenToPushNotificationsClicked, and createSubscribeToPushNotifications; ensure handlers return Promise<void>/void; type unsubscribe functions.
    • Support caching: Document acceptable race; add targeted eslint-disable-next-line require-atomic-updates.
  • Tests (web/push-utils.test.ts):
    • Refactor helper utilities into typed functions; add explicit return types; type Jest spies; add targeted eslint-disable-next-line no-restricted-globals for self usage; simulate notificationclick via self.dispatchEvent.
  • Shared:
    • Rename isOnChainRawNotification param from n to notification for clarity.
  • Tooling:
    • Remove corresponding entries for these files from eslint-suppressions.json.

Written by Cursor Bugbot for commit ba5e91a. This will update automatically on new commits. Configure here.

@cursor
Copy link

cursor bot commented Dec 15, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@Prithpal-Sooriya
Copy link
Contributor Author

@cursor update the packages/notification-services-controller/CHANGELOG.md changelog to reflect changes made.

core Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cursor remove this file

Copy link

@cursor cursor bot Dec 15, 2025

Choose a reason for hiding this comment

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

Done! I've completed both tasks:

  1. Removed the core file - This was an accidentally committed core dump file (36MB ELF binary from light-locker)

  2. Updated the changelog - Added an entry under [Unreleased] > Changed for PR chore: Notification controller eslint cleanup #7483:

    - Improve code quality by adding explicit return types and fixing ESLint violations in push notification utilities ([#7483](https://github.com/MetaMask/core/pull/7483))
    

The changes have been committed and pushed to the branch.


Open in Cursor Open in Web

Learn more about Cursor Agents

@cursor
Copy link

cursor bot commented Dec 15, 2025

Taking a look!

Open in Cursor Open in Web

- Remove accidentally committed core dump file
- Add changelog entry for ESLint cleanup PR #7483
@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review December 15, 2025 11:15
@Prithpal-Sooriya Prithpal-Sooriya requested review from a team as code owners December 15, 2025 11:15
@Prithpal-Sooriya Prithpal-Sooriya changed the title Notification controller eslint cleanup chore: Notification controller eslint cleanup Dec 15, 2025
@Prithpal-Sooriya
Copy link
Contributor Author

@cursor please remove the changelog changes, as these eslint changes does not impact the implementation

@cursor
Copy link

cursor bot commented Dec 15, 2025

Done! I've removed the changelog entry for this PR. Since the ESLint changes are purely internal code quality improvements that don't affect the implementation or API, they don't need to be documented in the changelog.


Open in Cursor Open in Web

Learn more about Cursor Agents

ESLint changes don't impact implementation, so no changelog entry needed.
bergarces
bergarces previously approved these changes Dec 15, 2025
juanmigdr
juanmigdr previously approved these changes Dec 15, 2025
@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Dec 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Dec 15, 2025
@Prithpal-Sooriya Prithpal-Sooriya dismissed stale reviews from juanmigdr and bergarces via 6a319e9 December 15, 2025 13:05
@Prithpal-Sooriya Prithpal-Sooriya force-pushed the cursor/notification-controller-eslint-cleanup-f7ec branch from 6a319e9 to ba5e91a Compare December 15, 2025 13:47
@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Dec 15, 2025
Merged via the queue into main with commit 9fb9729 Dec 15, 2025
285 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the cursor/notification-controller-eslint-cleanup-f7ec branch December 15, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants