Skip to content

Conversation

@Prithpal-Sooriya
Copy link
Contributor

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

Explanation

This PR addresses and resolves several ESLint rule violations across three files within packages/notification-services-controller, allowing for the removal of their corresponding suppressions from eslint-suppressions.json.

  • Current state and why change?
    ESLint suppressions were present in eslint-suppressions.json for rules such as @typescript-eslint/explicit-function-return-type, @typescript-eslint/naming-convention, @typescript-eslint/prefer-nullish-coalescing, and id-length. These suppressions indicated areas where the code did not conform to established linting standards. The goal is to improve code quality and maintainability by fixing these violations and removing the suppressions.

  • What is the solution your changes offer and how does it work?

    • feature-announcements.ts:
      • Explicit return types were added to getFeatureAnnouncementUrl and findIncludedItem.
      • prefer-nullish-coalescing was applied by replacing || with ??.
      • id-length violations were resolved by renaming short, non-descriptive variables (r, i, n) to more explicit names (response, entry/asset, item/rawNotification).
      • @typescript-eslint/naming-convention suppressions were added with explanations for Entry and Asset properties within the ContentfulResult type, as their casing is dictated by the external Contentful API response structure.
    • notification-config-cache.ts:
      • The private field #TTL was renamed to #ttl to conform to camelCase naming conventions for private class members.
    • perp-notifications.test.ts:
      • An explicit return type was added to the arrangeMocks test helper function.
    • All corresponding entries for these files were removed from eslint-suppressions.json.
  • Are there any changes whose purpose might not be obvious?
    The eslint-disable-next-line @typescript-eslint/naming-convention comments in feature-announcements.ts for Entry and Asset properties are intentional. These properties directly reflect the structure of responses from the Contentful API, and renaming them would break compatibility or require complex mapping.

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

Fixes ESLint violations in feature announcements, notification config cache, and perps test, and removes their suppressions.

  • Notification Services Controller:
    • services/feature-announcements.ts:
      • Add explicit return types to getFeatureAnnouncementUrl and findIncludedItem.
      • Replace || with ?? and improve variable names (response, entry/asset, item, rawNotification).
      • Add targeted @typescript-eslint/naming-convention disables for includes.Entry and includes.Asset to match Contentful schema.
      • Minor refactors: safer optional access, typed mappings, and consistent createdAt source.
    • services/notification-config-cache.ts:
      • Rename private field #TTL to #ttl and update references.
    • services/perp-notifications.test.ts:
      • Add explicit return type to arrangeMocks helper.
  • ESLint suppressions:
    • Remove entries for feature-announcements.ts, notification-config-cache.ts, and perp-notifications.test.ts from eslint-suppressions.json.

Written by Cursor Bugbot for commit e6f1d91. 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) that was removed from the repository.

  2. Updated the changelog - Added an entry under the [Unreleased] section in packages/notification-services-controller/CHANGELOG.md documenting the ESLint cleanup changes:

    • Added explicit return types to functions
    • Applied nullish coalescing operator (??) where appropriate
    • Renamed short variable names for better readability
    • Renamed private field #TTL to #ttl to follow camelCase convention

The amended commit has been force-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

@cursor cursor bot force-pushed the cursor/notification-services-eslint-cleanup-719a branch from 310f729 to 01c4f6d Compare December 15, 2025 10:29
@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review December 15, 2025 11:16
@Prithpal-Sooriya Prithpal-Sooriya requested review from a team as code owners December 15, 2025 11:16
env.spaceId,
)
.replace(DEFAULT_ACCESS_TOKEN, previewToken || env.accessToken)
.replace(DEFAULT_ACCESS_TOKEN, previewToken ?? env.accessToken)
Copy link

Choose a reason for hiding this comment

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

Bug: Inconsistent empty string handling between domain and token

The change from || to ?? on line 58 creates an inconsistency with the truthiness check on line 53. If previewToken is an empty string, line 53's previewToken ? PREVIEW_DOMAIN : DEFAULT_DOMAIN evaluates to DEFAULT_DOMAIN (treating empty string as falsy), but line 58's previewToken ?? env.accessToken returns the empty string instead of falling back to env.accessToken. This could result in API calls to the production domain with an empty access token, causing authentication failures.

Additional Locations (1)

Fix in Cursor Fix in Web

@Prithpal-Sooriya Prithpal-Sooriya changed the title Notification services eslint cleanup chore: Notification services 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 the ESLint cleanup changes. The changelog now no longer includes the entry about refactoring internal code, since these changes don't impact the implementation or external API.


Open in Cursor Open in Web

Learn more about Cursor Agents

@cursor cursor bot force-pushed the cursor/notification-services-eslint-cleanup-719a branch from 01c4f6d to b3089e3 Compare December 15, 2025 12:38
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
Copy link
Contributor Author

@cursor fix merge conflicts & cleanup eslint-suppressions.json

@cursor
Copy link

cursor bot commented Dec 15, 2025

Done! I've fixed the merge conflicts and cleaned up eslint-suppressions.json. Here's what was done:

  1. Rebased onto main - The branch was rebased onto the latest origin/main to incorporate all recent changes.

  2. Resolved merge conflict in eslint-suppressions.json - The conflict occurred because main had updated entries around the notification-services-controller section. I resolved it by:

    • Keeping the new entries from main (like process-notifications.ts, api-notifications.ts, feature-announcements.test.ts, etc.)
    • Removing the entries for the 3 files that were cleaned up (feature-announcements.ts, notification-config-cache.ts, perp-notifications.test.ts)
  3. Verified ESLint passes - Confirmed that ESLint runs without errors on all 3 cleaned up files.

  4. Force-pushed the rebased branch - The updated branch has been pushed to the remote.


Open in Cursor Open in Web

Learn more about Cursor Agents

@cursor cursor bot force-pushed the cursor/notification-services-eslint-cleanup-719a branch from b3089e3 to 2084216 Compare December 15, 2025 13:16
@Prithpal-Sooriya Prithpal-Sooriya force-pushed the cursor/notification-services-eslint-cleanup-719a branch from 2084216 to cef2d1e Compare December 15, 2025 13:23
- Add explicit return types to functions in feature-announcements.ts and perp-notifications.test.ts
- Apply nullish coalescing operator (??) where appropriate
- Rename short variable names for better readability (r -> response, i -> entry/asset, n -> item/rawNotification)
- Rename private field #TTL to #ttl in OnChainNotificationsCache class
- Add eslint-disable comments with explanations for Contentful API property names
- Remove corresponding suppressions from eslint-suppressions.json
- Update CHANGELOG.md
@Prithpal-Sooriya Prithpal-Sooriya force-pushed the cursor/notification-services-eslint-cleanup-719a branch from cef2d1e to e6f1d91 Compare December 15, 2025 13:29
@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Dec 15, 2025
Merged via the queue into main with commit 2a3a2eb Dec 15, 2025
285 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the cursor/notification-services-eslint-cleanup-719a branch December 15, 2025 13:38
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