fix: change description validating logic in lablup-notification #2503
Conversation
Your org requires the Graphite merge queue for merging into mainAdd the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @ironAiken2 and the rest of your teammates on |
| const event = new CustomEvent('backend-ai-resource-refreshed', { | ||
| detail: 'running', | ||
| }); | ||
| document.dispatchEvent(event); |
There was a problem hiding this comment.
Is there any reason to delete this part?
There was a problem hiding this comment.
The _terminateSessionWithCheck function uses the _terminateKernel function internally. The catch statement is the error catching syntax for the _terminateKernel function, and if an error occurs inside that function, it outputs the error to the notification. Therefore, I removed that code to avoid duplication of the notification. (Duplicate handling in the parent PR is determined by the existence of log, but the corresponding error is the absence of log).
Merge activity
|
### Existing Problems The existing web component notification does not output any accompanying description when a text field is provided for the title. [ref](https://github.com/lablup/backend.ai-webui/blob/119f64e320befec545680934722a05cce00ede65/src/components/lablup-notification.ts#L196) However, most of the usage of notifications in web components is providing the notification title through the painkiller, so even if the title of the error is undefined, the painkiller will always provide a specific string (e.g. `problem occured`). Therefore, a description of the error is never provided. ### Changes - Changed the logic for checking if description is visible to a comparison to see if it is the same as title. - Fixed an issue where the same notification would appear twice when deleting a session. ### How to Test 1. After causing an error in the webui, make sure both title and description are displayed 2. Delete the session that is causing the deletion error, then verify that the same notification doesn't occur more than once <!-- Please precisely, concisely, and concretely describe what this PR changes, the rationale behind codes, and how it affects the users and other developers. --> **Checklist:** (if applicable) - [ ] Mention to the original issue - [ ] Documentation - [ ] Minium required manager version - [ ] Specific setting for review (eg., KB link, endpoint or how to setup) - [ ] Minimum requirements to check during review - [ ] Test case(s) to demonstrate the difference of before/after
yomybaby
force-pushed
the
feature/prevent-staking-duplicated-notifications
branch
from
082820c to
e4145c4
Compare
yomybaby
force-pushed
the
fix/validate-notification-description
branch
from
f50ad51 to
a50d0f4
Compare
yomybaby
changed the base branch from
feature/prevent-staking-duplicated-notifications
to
main
Existing Problems
The existing web component notification does not output any accompanying description when a text field is provided for the title. ref
However, most of the usage of notifications in web components is providing the notification title through the painkiller, so even if the title of the error is undefined, the painkiller will always provide a specific string (e.g.
problem occured). Therefore, a description of the error is never provided.Changes
How to Test
Checklist: (if applicable)