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 in three files within packages/notification-services-controller/src/NotificationServicesPushController. The goal is to improve code quality and consistency by adhering to established linting rules.

The changes include:

  • Adding explicit return types to functions to satisfy @typescript-eslint/explicit-function-return-type.
  • Using the nullish coalescing operator (??) to fix @typescript-eslint/prefer-nullish-coalescing.
  • Renaming short, non-descriptive identifiers (e.g., num to numericValue, t to translate, n to notification) to resolve id-denylist and id-length violations.
  • Refactoring generic type parameters (e.g., N to TNotification, K to TriggerType/TKey) for better clarity and to fix @typescript-eslint/naming-convention.
  • Adding an eslint-disable comment with an explanation for the registration_token property in services.ts. Its snake_case naming is dictated by an external API contract, making a code change impractical.
  • Removing all corresponding entries from eslint-suppressions.json.

References

  • Fixes ASSETS-2100

Checklist

  • I've updated the test suite for new or updated code as appropriate (Functional tests pass, coverage thresholds are a separate issue)
  • 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

Tightens TypeScript types and naming in NotificationServicesPushController utilities/services, adds a justified snake_case exception for registration_token, and removes related ESLint suppressions.

  • Notification Services (push):
    • utils/get-notification-data.ts:
      • Add explicit return types (calcTokenAmount: BigNumber, getLeadingZeroCount: number, formatAmount: string, getAmount: string).
      • Use nullish coalescing in parsing; type handleShouldEllipse and options.
      • Rename params/vars for clarity (e.g., numnumericValue).
    • utils/get-notification-message.ts:
      • Strengthen generics and parameter names (N/KTNotification/TKey, nnotification, ttranslate).
      • Add explicit return types across helpers and message creators; type getChainSymbol.
    • services/services.ts:
      • Document API contract and add ESLint naming exception for registration_token in PushTokenRequest.
  • Lint:
    • Remove corresponding entries from eslint-suppressions.json for the updated files.

Written by Cursor Bugbot for commit 8dfbc12. 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.

@cursor
Copy link

cursor bot commented Dec 15, 2025

I've updated the packages/notification-services-controller/CHANGELOG.md with the following entry under the [Unreleased] section:

- Improve code quality in `NotificationServicesPushController` utils ([#7484](https://github.com/MetaMask/core/pull/7484))
  - Add explicit return types to functions in `get-notification-data.ts` and `get-notification-message.ts`
  - Rename short identifiers for better clarity (e.g., `num` to `numericValue`, `t` to `translate`, `n` to `notification`)
  - Use nullish coalescing operator (`??`) instead of logical OR (`||`) where appropriate

This changelog entry describes the code quality improvements made in this PR, which includes:

  • Adding explicit return types to satisfy @typescript-eslint/explicit-function-return-type
  • Renaming short identifiers to fix id-denylist and id-length violations
  • Using nullish coalescing to fix @typescript-eslint/prefer-nullish-coalescing

The CI changelog check should now pass.


Open in Cursor Open in Web

Learn more about Cursor Agents

@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 I previously added. The changelog now has no changes from this PR, which is correct since the ESLint cleanup is purely internal code quality improvements that don't affect the implementation or API.


Open in Cursor Open in Web

Learn more about Cursor Agents

bergarces
bergarces 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 force-pushed the cursor/notification-controller-eslint-cleanup-27b8 branch from df69908 to 010ce45 Compare December 15, 2025 13:39
@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Dec 15, 2025
Merged via the queue into main with commit 4507871 Dec 15, 2025
285 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the cursor/notification-controller-eslint-cleanup-27b8 branch December 15, 2025 15:09
@cursor cursor bot restored the cursor/notification-controller-eslint-cleanup-27b8 branch December 15, 2025 20:42
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.

5 participants