Skip to content

chore: improve typings for bell elements#1397

Merged
fadi-george merged 3 commits intomainfrom
fg/bell-fixes
Oct 17, 2025
Merged

chore: improve typings for bell elements#1397
fadi-george merged 3 commits intomainfrom
fg/bell-fixes

Conversation

@fadi-george
Copy link
Copy Markdown
Contributor

@fadi-george fadi-george commented Oct 14, 2025 •
edited
Loading

Description

1 Line Summary

  • improve types for Bell elements and get rid of any usages

Details

  • replace any usages with actual element types
  • split up constants used by bell message badge etc. to remove cyclical dependencies (apart from types)

Systems Affected

  • WebSDK
  • Backend
  • Dashboard

Validation

Tests

Info

Checklist

  • All the automated tests pass or I explained why that is not possible
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

  • Don't use default export
  • New interfaces are in model files

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed

Related Tickets

This change is Reviewable

@fadi-george fadi-george requested review from jkasten2 and sherwinski and removed request for jkasten2 October 14, 2025 22:08
@fadi-george fadi-george changed the title Improve Bell Typings chore: improve typings for bell elements Oct 14, 2025
sherwinski
sherwinski reviewed Oct 16, 2025
View reviewed changes
Comment thread src/page/bell/Bell.ts Outdated
if (state.to === BellState._Subscribed) {
this.launcher.inactivate();
} else if (state.to === Bell.STATES.UNSUBSCRIBED || Bell.STATES.BLOCKED) {
} else if (state.to === BellState._Unsubscribed || BellState._Blocked) {
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Oct 16, 2025

Choose a reason for hiding this comment

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

Doesn't BellState._Blocked always evaluate to truthy? I'm not sure what this condition is trying to check for.

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Oct 16, 2025

Choose a reason for hiding this comment

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

Yeah its an exisiting, ill change it to state.to === BellState._Blocked

Comment thread src/page/bell/Bell.ts Outdated
Comment thread src/page/bell/constants.ts Outdated
Comment thread src/page/bell/Launcher.ts Outdated
Comment on lines +87 to +90
return this.bell.badge.show();
} else {
await Promise.all([super.inactivate(), this.resize('small')]);
return this;
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Oct 16, 2025

Choose a reason for hiding this comment

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

I understand this is a refactor of existing code, but why does it either return badge.show() or this?

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Oct 16, 2025

Choose a reason for hiding this comment

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

Removed this, helpful for chaining but not really used

Comment thread src/page/bell/Bell.ts
.then(() => delay(MESSAGE_TIMEOUT))
.then(() => this.message.hide())
.then(() => {
if (this.launcher.wasInactive && this.dialog.hidden) {
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Oct 16, 2025

Choose a reason for hiding this comment

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

Why is this condition changed to use a negative check but this line does the opposite?

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Oct 16, 2025

Choose a reason for hiding this comment

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

Line 302 is !this.dialog.hidden, there is a bang in front so it would be equivalent to shown.

Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Oct 17, 2025

Choose a reason for hiding this comment

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

I understand that they're equivalent. I'm asking why the choice to use !this.dialog.shown instead of this.dialog.hidden here.

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Oct 17, 2025

Choose a reason for hiding this comment

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

We dont have this.dialog.hidden

Comment thread src/page/bell/Bell.ts Outdated
Comment thread src/page/bell/Bell.ts
this.launcher.inactivate();
} else if (state.to === Bell.STATES.UNSUBSCRIBED || Bell.STATES.BLOCKED) {
} else if (state.to === BellState._Unsubscribed || BellState._Blocked) {
this.launcher.activate();
Copy link
Copy Markdown
Contributor

@sherwinski sherwinski Oct 16, 2025

Choose a reason for hiding this comment

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

I don't know if it's intentional but I would double check that other instances of activate() need to be awaited

Copy link
Copy Markdown
Contributor Author

@fadi-george fadi-george Oct 16, 2025

Choose a reason for hiding this comment

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

Its in the emitter callback so awaiting wouldnt do anything from what I can tell

@fadi-george fadi-george force-pushed the fg/bell-fixes branch 4 times, most recently from cb980ea to fb5886c Compare October 16, 2025 21:51
@fadi-george fadi-george merged commit b2638aa into main Oct 17, 2025
3 checks passed
@fadi-george fadi-george deleted the fg/bell-fixes branch October 17, 2025 16:04
@github-actions github-actions Bot mentioned this pull request Feb 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sherwinski sherwinski sherwinski approved these changes

@jkasten2 jkasten2 Awaiting requested review from jkasten2

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fadi-george @sherwinski