Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Notifications Tray src/app/shared/notifications-tray/notifications-tray.component.html, src/app/shared/notifications-tray/notifications-tray.component.ts |
Wires mouseenter / mouseleave on the notifications container to pauseTimer() / resumeTimer(); adds timerPaused state, clears existing timeouts before fetching, and schedules polling only when not paused. |
Snackbar Hover Control src/app/web-app.component.ts |
Adds MatSnackBarRef usage, opens snackbars with duration: 0 (no auto-dismiss), and implements handleSnackbarHover to pause/resume snackbar dismissal via a hover-aware timer and resume with the default duration. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant NotificationsTray
participant NotificationsService
participant Timer
User->>NotificationsTray: mouseenter (hover)
NotificationsTray->>Timer: pauseTimer() — clear timeout
User->>NotificationsTray: mouseleave
NotificationsTray->>Timer: resumeTimer() — set timeout
Timer->>NotificationsService: fetchUnreadNotifications()
NotificationsService-->>NotificationsTray: return unread notifications
sequenceDiagram
participant User
participant WebAppComponent
participant MatSnackBar
participant Timer
WebAppComponent->>MatSnackBar: open(snackbar, duration=0) (store ref)
MatSnackBar-->>User: show snackbar
User->>MatSnackBar: mouseenter
MatSnackBar->>Timer: pause auto-dismiss timer
User->>MatSnackBar: mouseleave
MatSnackBar->>Timer: start timer with defaultDuration
Timer->>MatSnackBar: dismiss()
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
- IOhacker
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | The title mentions 'persistent popups' but the changes actually implement hover-based timer pause/resume for notifications, not persistent popups. | Update the title to accurately reflect the feature, such as 'Add hover-based pause/resume for notification and snackbar timers' or 'Implement pause-on-hover for notification timers'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/web-app.component.ts (1)
214-222: Bind alert stream subscription to component teardownThis subscription should be lifecycle-bound to avoid duplicate snackbar handlers when the component is recreated.
♻️ Suggested fix
- this.alertService.alertEvent.subscribe((alertEvent: Alert) => { + this.alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe((alertEvent: Alert) => { const snackBarRef = this.snackBar.open(`${alertEvent.message}`, 'Close', { duration: 0, // ✅ Set to 0 - no auto-dismiss initially horizontalPosition: 'right', verticalPosition: 'top' }); // ✅ Handle hover behavior this.handleSnackbarHover(snackBarRef, 2000); });As per coding guidelines: For Angular code in
src/app/**, verify clean observable patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/web-app.component.ts` around lines 214 - 222, The alertEvent subscription in web-app.component.ts is not tied to component teardown and can leak or duplicate handlers; modify the subscription made via this.alertService.alertEvent.subscribe(...) to be lifecycle-bound by either storing the Subscription returned (e.g., this.alertSub) and unsubscribing in ngOnDestroy, or by piping the observable with takeUntil(this.destroy$) and completing destroy$ in ngOnDestroy; ensure the logic that opens the snackBar and calls this.handleSnackbarHover(snackBarRef, 2000) remains unchanged but is executed only while the component is alive (update component class to declare the Subscription or destroy$ and implement ngOnDestroy accordingly).src/app/shared/notifications-tray/notifications-tray.component.html (1)
32-32: Add keyboard-equivalent pause/resume handlersPause/resume is mouse-only right now. Add focus handlers so keyboard navigation gets the same persistence behavior.
♿ Suggested fix
- <div class="notifications-container" (mouseenter)="pauseTimer()" (mouseleave)="resumeTimer()"> + <div + class="notifications-container" + (mouseenter)="pauseTimer()" + (mouseleave)="resumeTimer()" + (focusin)="pauseTimer()" + (focusout)="resumeTimer()">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/notifications-tray/notifications-tray.component.html` at line 32, The notifications container currently only binds mouse events; add keyboard/focus equivalents so keyboard users get the same persistence by wiring focus and blur to the existing pauseTimer() and resumeTimer() handlers on the same element (the element with class "notifications-container" in notifications-tray.component.html), and ensure the container is keyboard-focusable (e.g., has a tabindex or appropriate interactive role) so focus events will fire for keyboard navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/shared/notifications-tray/notifications-tray.component.ts`:
- Around line 128-131: resumeTimer currently flips timerPaused and calls
fetchUnreadNotifications unconditionally, which can spawn duplicate polling
loops; change resumeTimer to first check if timerPaused is false and return
early (i.e., only proceed when timerPaused is true), then set timerPaused =
false and call fetchUnreadNotifications; additionally modify
fetchUnreadNotifications to clear any existing timeout/interval handle before
scheduling a new one (store the handle on the component, e.g., this.pollTimeout
/ this.pollInterval) so multiple calls cannot stack, and ensure any existing
handle is cleared when pausing in pauseTimer.
In `@src/app/web-app.component.ts`:
- Around line 272-276: Replace the hardcoded setTimeout-based DOM probing with
the snackbar's lifecycle hook: where you currently call setTimeout(() => { const
snackbarContainer = document.querySelector('.mat-mdc-snack-bar-container'); ...
}, 100), subscribe to the MatSnackBarRef.afterOpened() observable (e.g.,
snackbarRef.afterOpened().subscribe(...)) and perform the
document.querySelector('.mat-mdc-snack-bar-container') lookup inside that
callback; if the lookup returns null, call snackbarRef.dismiss() instead of
silently returning. Ensure you reference the actual MatSnackBarRef instance used
in this component (snackbarRef or snackBarRef) and remove the setTimeout usage.
---
Nitpick comments:
In `@src/app/shared/notifications-tray/notifications-tray.component.html`:
- Line 32: The notifications container currently only binds mouse events; add
keyboard/focus equivalents so keyboard users get the same persistence by wiring
focus and blur to the existing pauseTimer() and resumeTimer() handlers on the
same element (the element with class "notifications-container" in
notifications-tray.component.html), and ensure the container is
keyboard-focusable (e.g., has a tabindex or appropriate interactive role) so
focus events will fire for keyboard navigation.
In `@src/app/web-app.component.ts`:
- Around line 214-222: The alertEvent subscription in web-app.component.ts is
not tied to component teardown and can leak or duplicate handlers; modify the
subscription made via this.alertService.alertEvent.subscribe(...) to be
lifecycle-bound by either storing the Subscription returned (e.g.,
this.alertSub) and unsubscribing in ngOnDestroy, or by piping the observable
with takeUntil(this.destroy$) and completing destroy$ in ngOnDestroy; ensure the
logic that opens the snackBar and calls this.handleSnackbarHover(snackBarRef,
2000) remains unchanged but is executed only while the component is alive
(update component class to declare the Subscription or destroy$ and implement
ngOnDestroy accordingly).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/shared/notifications-tray/notifications-tray.component.htmlsrc/app/shared/notifications-tray/notifications-tray.component.tssrc/app/web-app.component.ts
src/app/shared/notifications-tray/notifications-tray.component.ts
Outdated
Show resolved
Hide resolved
65aa045 to
17939b8
Compare
owwk!! fixed the comments!! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/web-app.component.ts (2)
214-222: Add lifecycle teardown toalertEventsubscription.Line 214 subscribes without teardown. Bind it to
destroy$for consistent observable cleanup.Proposed fix
- this.alertService.alertEvent.subscribe((alertEvent: Alert) => { + this.alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe((alertEvent: Alert) => { const snackBarRef = this.snackBar.open(`${alertEvent.message}`, 'Close', { duration: 0, // Set to 0 - no auto-dismiss initially horizontalPosition: 'right', verticalPosition: 'top' }); // Handle hover behavior this.handleSnackbarHover(snackBarRef, 2000); });As per coding guidelines: "src/app/**: For Angular code: verify ... clean observable patterns."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/web-app.component.ts` around lines 214 - 222, The subscription to alertService.alertEvent must be torn down using the component's destroy$ observable to avoid leaks; update the block where you subscribe (alertService.alertEvent.subscribe(...)) to pipe the observable through takeUntil(this.destroy$) (or equivalent) before subscribing, e.g. alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe(...) so the stream is cancelled on ngOnDestroy; ensure destroy$ is defined as a Subject and that ngOnDestroy calls destroy$.next() and destroy$.complete(); keep the existing logic (snackBar.open and this.handleSnackbarHover(snackBarRef, 2000)) intact.
270-278: Tighten types in hover handler (anyis avoidable).Use concrete types for
MatSnackBarRefand timer state instead ofany.Proposed fix
-import { MatSnackBar, MatSnackBarRef } from '@angular/material/snack-bar'; +import { MatSnackBar, MatSnackBarRef, TextOnlySnackBar } from '@angular/material/snack-bar'; ... - private handleSnackbarHover(snackBarRef: MatSnackBarRef<any>, defaultDuration: number): void { + private handleSnackbarHover(snackBarRef: MatSnackBarRef<TextOnlySnackBar>, defaultDuration: number): void { ... - let dismissTimer: any; + let dismissTimer: ReturnType<typeof setTimeout> | null = null;As per coding guidelines: "src/app/**: For Angular code: verify ... strict type safety ..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/web-app.component.ts` around lines 270 - 278, The hover handler currently uses broad any types; change the parameter type from MatSnackBarRef<any> to MatSnackBarRef<unknown> (or a specific snack component type if you have one) and tighten the timer variable by declaring let dismissTimer: number | null = null (or use ReturnType<typeof setTimeout> if you prefer) and update the clearTimeout/assignment checks to handle null safely (e.g., if (dismissTimer !== null) clearTimeout(dismissTimer); dismissTimer = window.setTimeout(...)). This updates handleSnackbarHover and its use of dismissTimer to avoid any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/web-app.component.ts`:
- Around line 214-222: The subscription to alertService.alertEvent must be torn
down using the component's destroy$ observable to avoid leaks; update the block
where you subscribe (alertService.alertEvent.subscribe(...)) to pipe the
observable through takeUntil(this.destroy$) (or equivalent) before subscribing,
e.g. alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe(...) so
the stream is cancelled on ngOnDestroy; ensure destroy$ is defined as a Subject
and that ngOnDestroy calls destroy$.next() and destroy$.complete(); keep the
existing logic (snackBar.open and this.handleSnackbarHover(snackBarRef, 2000))
intact.
- Around line 270-278: The hover handler currently uses broad any types; change
the parameter type from MatSnackBarRef<any> to MatSnackBarRef<unknown> (or a
specific snack component type if you have one) and tighten the timer variable by
declaring let dismissTimer: number | null = null (or use ReturnType<typeof
setTimeout> if you prefer) and update the clearTimeout/assignment checks to
handle null safely (e.g., if (dismissTimer !== null) clearTimeout(dismissTimer);
dismissTimer = window.setTimeout(...)). This updates handleSnackbarHover and its
use of dismissTimer to avoid any.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/shared/notifications-tray/notifications-tray.component.htmlsrc/app/shared/notifications-tray/notifications-tray.component.tssrc/app/web-app.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/shared/notifications-tray/notifications-tray.component.ts
17939b8 to
1fc8962
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/shared/notifications-tray/notifications-tray.component.ts (1)
138-141:⚠️ Potential issue | 🟠 MajorGuard resume to avoid redundant immediate fetches when already running.
If
resumeTimer()is triggered while not paused, Line 140 still forces another fetch cycle. Add an early return.🐛 Proposed fix
resumeTimer = () => { + if (!this.timerPaused) { + return; + } this.timerPaused = false; this.fetchUnreadNotifications(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/notifications-tray/notifications-tray.component.ts` around lines 138 - 141, The resumeTimer function should no-op when the timer is already running to avoid redundant immediate fetches; update resumeTimer in notifications-tray.component.ts (the method named resumeTimer referencing this.timerPaused and this.fetchUnreadNotifications) to check if this.timerPaused is false and return early, otherwise set this.timerPaused = false and call this.fetchUnreadNotifications().
🧹 Nitpick comments (1)
src/app/shared/notifications-tray/notifications-tray.component.ts (1)
133-136: Clear the handle after timeout cancellation for cleaner state.After Line 135, set
this.timer = nullso follow-up checks don’t treat a stale handle as active.♻️ Proposed cleanup
pauseTimer = () => { this.timerPaused = true; clearTimeout(this.timer); + this.timer = null; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/notifications-tray/notifications-tray.component.ts` around lines 133 - 136, The pauseTimer arrow method leaves a stale timeout handle in this.timer after clearTimeout, so update pauseTimer (the pauseTimer = () => { ... } function) to also set this.timer = null after clearTimeout(this.timer) and keep this.timerPaused = true; to ensure subsequent logic treating this.timer as active won't be misled by an old handle; locate the pauseTimer method in notifications-tray.component.ts and add the assignment to null immediately after the clearTimeout call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/shared/notifications-tray/notifications-tray.component.ts`:
- Around line 113-116: The code is creating duplicate timers which can survive
pauseTimer(); before scheduling a new timeout ensure any existing timer is
cleared and only schedule when not paused — i.e., in the method that currently
does the unconditional setTimeout (look for the block that sets this.timer
around lines 122/126) first call the existing clear logic (the if (this.timer) {
clearTimeout(this.timer); this.timer = null; }) then guard the setTimeout with a
paused check (only assign this.timer = setTimeout(...) when !this.isPaused /
!this.paused or when pauseTimer() isn’t active), and remove the second
unconditional scheduling so there is a single code path that both clears
previous timers and sets a new one only when not paused.
---
Duplicate comments:
In `@src/app/shared/notifications-tray/notifications-tray.component.ts`:
- Around line 138-141: The resumeTimer function should no-op when the timer is
already running to avoid redundant immediate fetches; update resumeTimer in
notifications-tray.component.ts (the method named resumeTimer referencing
this.timerPaused and this.fetchUnreadNotifications) to check if this.timerPaused
is false and return early, otherwise set this.timerPaused = false and call
this.fetchUnreadNotifications().
---
Nitpick comments:
In `@src/app/shared/notifications-tray/notifications-tray.component.ts`:
- Around line 133-136: The pauseTimer arrow method leaves a stale timeout handle
in this.timer after clearTimeout, so update pauseTimer (the pauseTimer = () => {
... } function) to also set this.timer = null after clearTimeout(this.timer) and
keep this.timerPaused = true; to ensure subsequent logic treating this.timer as
active won't be misled by an old handle; locate the pauseTimer method in
notifications-tray.component.ts and add the assignment to null immediately after
the clearTimeout call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/shared/notifications-tray/notifications-tray.component.htmlsrc/app/shared/notifications-tray/notifications-tray.component.tssrc/app/web-app.component.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/shared/notifications-tray/notifications-tray.component.html
- src/app/web-app.component.ts
1fc8962 to
e6943ba
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/shared/notifications-tray/notifications-tray.component.ts (1)
137-140:⚠️ Potential issue | 🟠 MajorMake
resumeTimeridempotent to avoid redundant fetches.Line 137 resumes unconditionally. If
mouseleavefires while already resumed, this can still trigger unnecessary immediate refetches.Suggested change
resumeTimer = () => { + if (!this.timerPaused) { + return; + } this.timerPaused = false; this.fetchUnreadNotifications(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/notifications-tray/notifications-tray.component.ts` around lines 137 - 140, The resumeTimer handler should be made idempotent to avoid redundant immediate fetches: inside the existing resumeTimer arrow function (resumeTimer) check the current flag (timerPaused) and only flip it to false and call fetchUnreadNotifications() when timerPaused is actually true; if timerPaused is already false, return early and do nothing. Keep the function as an arrow property so binding is preserved and only perform the state change and fetch when necessary.
🧹 Nitpick comments (2)
src/app/web-app.component.ts (2)
270-282: Tighten types in snackbar hover handling (anyusage).Line 270 and Line 281 use
anyin new logic. This weakens type safety in a changed path.Suggested change
-import { MatSnackBar, MatSnackBarRef } from '@angular/material/snack-bar'; +import { MatSnackBar, MatSnackBarRef, TextOnlySnackBar } from '@angular/material/snack-bar'; @@ - private handleSnackbarHover(snackBarRef: MatSnackBarRef<any>, defaultDuration: number): void { + private handleSnackbarHover(snackBarRef: MatSnackBarRef<TextOnlySnackBar>, defaultDuration: number): void { @@ - let dismissTimer: any; + let dismissTimer: ReturnType<typeof setTimeout> | null = null;As per coding guidelines
src/app/**: “verify ... strict type safety.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/web-app.component.ts` around lines 270 - 282, The local variable dismissTimer in handleSnackbarHover is typed as any; tighten it to a proper timer type such as ReturnType<typeof setTimeout> | undefined (or number | undefined) and update any clearTimeout/assignment sites accordingly so TypeScript knows it's a timer handle; also cast the result of document.querySelector('.mat-mdc-snack-bar-container') to the appropriate DOM type (e.g., HTMLElement | null) instead of using implicit any so snackbarContainer has a strict type when checked and manipulated in handleSnackbarHover.
214-222: Add teardown to the alert stream subscription.Line 214 subscribes directly to
alertEventwithout lifecycle teardown. Align this with existingtakeUntil(this.destroy$)usage in this component.Suggested change
- this.alertService.alertEvent.subscribe((alertEvent: Alert) => { + this.alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe((alertEvent: Alert) => { const snackBarRef = this.snackBar.open(`${alertEvent.message}`, 'Close', { duration: 0, // Set to 0 - no auto-dismiss initially horizontalPosition: 'right', verticalPosition: 'top' }); // Handle hover behavior this.handleSnackbarHover(snackBarRef, 2000); });As per coding guidelines
src/app/**: “verify ... clean observable patterns.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/web-app.component.ts` around lines 214 - 222, The subscription to alertService.alertEvent should be torn down using the existing destroy$ pattern: pipe the observable with takeUntil(this.destroy$) before calling subscribe (i.e., alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe(...)) so the snackBar handling in handleSnackbarHover(snackBarRef, 2000) is cleaned up on component destroy; also ensure destroy$ is completed in ngOnDestroy (this.destroy$.next(); this.destroy$.complete()) if not already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/shared/notifications-tray/notifications-tray.component.ts`:
- Around line 137-140: The resumeTimer handler should be made idempotent to
avoid redundant immediate fetches: inside the existing resumeTimer arrow
function (resumeTimer) check the current flag (timerPaused) and only flip it to
false and call fetchUnreadNotifications() when timerPaused is actually true; if
timerPaused is already false, return early and do nothing. Keep the function as
an arrow property so binding is preserved and only perform the state change and
fetch when necessary.
---
Nitpick comments:
In `@src/app/web-app.component.ts`:
- Around line 270-282: The local variable dismissTimer in handleSnackbarHover is
typed as any; tighten it to a proper timer type such as ReturnType<typeof
setTimeout> | undefined (or number | undefined) and update any
clearTimeout/assignment sites accordingly so TypeScript knows it's a timer
handle; also cast the result of
document.querySelector('.mat-mdc-snack-bar-container') to the appropriate DOM
type (e.g., HTMLElement | null) instead of using implicit any so
snackbarContainer has a strict type when checked and manipulated in
handleSnackbarHover.
- Around line 214-222: The subscription to alertService.alertEvent should be
torn down using the existing destroy$ pattern: pipe the observable with
takeUntil(this.destroy$) before calling subscribe (i.e.,
alertService.alertEvent.pipe(takeUntil(this.destroy$)).subscribe(...)) so the
snackBar handling in handleSnackbarHover(snackBarRef, 2000) is cleaned up on
component destroy; also ensure destroy$ is completed in ngOnDestroy
(this.destroy$.next(); this.destroy$.complete()) if not already present.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e018801-d682-41d5-a020-dfcfb9eec583
📒 Files selected for processing (3)
src/app/shared/notifications-tray/notifications-tray.component.htmlsrc/app/shared/notifications-tray/notifications-tray.component.tssrc/app/web-app.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/shared/notifications-tray/notifications-tray.component.html
Description
modified timer logic in notifications to have persistence if user hovers over it also modified the webapp component for this functionality
Related issues and discussion
WEB-806
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
[*] If you have multiple commits please combine them into one commit by squashing them.
[*] Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit