fix: refactor notification system to use Notyf #2327
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @robfrank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on fixing and improving the application's notification system by migrating it to the Notyf library. The changes include refactoring the notification logic for better reliability and introducing a new end-to-end test to ensure the system functions as expected, addressing a previously reported issue with notifications not appearing correctly.
Highlights
- Notification System Migration: The existing notification system has been refactored to utilize the Notyf library, replacing the previous implementation (likely Bootstrap Notify) to address reliability issues.
- Robust Notyf Initialization: Notyf initialization has been moved to occur within a
DOMContentLoadedevent listener inindex.htmlto prevent race conditions and ensure thenotyfinstance is available when needed. This resolves potential 'not initialized' JavaScript errors. - Deferred Notification Handling: The
globalNotifyfunction now includes a check to ensure the Notyf instance is initialized before attempting to display a notification. If Notyf is not yet ready, the notification call is deferred using asetTimeoutto prevent messages from being lost. - New End-to-End Test: A new Playwright end-to-end test (
notification-test.spec.ts) has been added to validate the functionality of the updated notification system. This test specifically checks for the display of error notifications and logs any related JavaScript errors.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request refactors the notification system to use Notyf, addressing an initialization issue. The review focuses on enhancing the robustness of the fix and the associated test. It suggests a more reliable queuing mechanism for handling notifications and points out issues with the test's selectors and assertions.
| await page.getByRole('button', { name: '' }).first().click(); | ||
|
|
||
| // Wait a moment for the query to execute and error to appear | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
Using waitForTimeout can lead to flaky tests as the wait time is fixed. It's a better practice to wait for a specific condition or element state. Since this PR fixes notifications, you should wait for the notification to appear. This would also serve as a good assertion that the fix is working.
| await page.waitForTimeout(2000); | |
| // Wait for the error notification to be visible | |
| await expect(page.locator('.notyf__toast--error')).toBeVisible(); |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
…handling in tests
Superb! Minor or unrelated remarks:
|
Related issues
#2323
Checklist
mvn clean packagecommand