refactor: streamline notification closing logic in useBrowserNotifica…#1670
refactor: streamline notification closing logic in useBrowserNotifica…#1670
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the useBrowserNotifications hook to centralize and reuse browser notification closing behavior across multiple lifecycle/event handlers.
Changes:
- Introduces a shared
closeNotification()callback (with a try/catch guard) to close the activeNotification. - Updates
visibilitychangeandunloadhandlers to call the shared close function instead of duplicating close logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
Applied all the review changes in commit
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
paustint
force-pushed
the
bug/ensure-notification-close-no-ex
branch
from
42b932f to
8836832
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (notification.current && typeof notification.current.close === 'function') { | ||
| notification.current.close(); | ||
| notification.current = null; | ||
| } | ||
| } catch (ex) { | ||
| logger.error('[NOTIFICATION][CLOSE][ERROR]', ex); | ||
| } finally { | ||
| notification.current = null; |
There was a problem hiding this comment.
closeNotification sets notification.current = null both inside the if block and again in finally. The inner assignment is redundant and makes the control flow harder to follow; consider keeping the single assignment in finally (or only after a successful close) to reduce duplication.
| const closeNotification = useCallback(() => { | ||
| try { | ||
| if (notification.current && typeof notification.current.close === 'function') { | ||
| notification.current.close(); | ||
| notification.current = null; | ||
| } | ||
| } catch (ex) { | ||
| logger.error('[NOTIFICATION][CLOSE][ERROR]', ex); | ||
| } finally { | ||
| notification.current = null; | ||
| } |
There was a problem hiding this comment.
closeNotification assigns notification.current = null, but the ref is declared as useRef<Notification>(null) (and is written to elsewhere). With strictNullChecks enabled for this lib, the ref should be typed as nullable and mutable (e.g., useRef<Notification | null>(null)) so these assignments are type-safe.
|
@copilot apply changes based on the comments in this thread |
…ifications Agent-Logs-Url: https://github.com/jetstreamapp/jetstream/sessions/5011e050-237f-445c-9818-fd1b6689373a Co-authored-by: paustint <5461649+paustint@users.noreply.github.com>
Applied both changes in commit
|
Ensure that failures are handled instead of reporting to rollbar