Notification management #1189
Notification management #1189
Conversation
…loading for notifcations.
|
Rishabh Shinde seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Resolving conflicts in the pull requests |
There was a problem hiding this comment.
There are a lot of package-lock.json changes that don't seem relevant to this PR. They should be removed. If package-lock.json files are out of sync this should be addressed in a separate PR.
There are attempts to communicate with the database from client-side in this PR. That will never work.
There was a problem hiding this comment.
Why was this modified? This seems out-of-scope for this PR. If this package-lock.json is out of sync we should update it but in a different PR.
| @@ -883,4 +883,4 @@ class AIChatService extends BaseService { | |||
| } | |||
| } | |||
|
|
|||
| module.exports = { AIChatService }; | |||
| module.exports = { AIChatService }; | |||
There was a problem hiding this comment.
text files should end with a line feed (LF) character so append operations behave as expected
| @@ -1,5 +1,4 @@ | |||
| // METADATA // {"ai-commented":{"service":"mistral","model":"mistral-large-latest"}} | |||
There was a problem hiding this comment.
Why was this comment removed? We have a tool that's able to generate automated comments (it's experimental; it's not very reliable but we might do more work on it later) and this metadata comment tells the tool if the file has already been commented on. We might use this same format for storing other metadata for source files in the future.
|
|
||
| async function UIDesktop(options){ | ||
| let h = ''; | ||
|
|
||
| // Set up the desktop channel for communication between different tabs in the same browser | ||
| window.channel = new BroadcastChannel('puter-desktop-channel'); |
There was a problem hiding this comment.
Was it verified that this former-global wasn't being referenced anywhere else?
| var x1= day + ", " + month + " " + dt; | ||
| x1 = hours + ":" + minutes + ampm + " " + x1; | ||
| var x1=month + "/" + dt + "/" + x.getFullYear(); | ||
| x1 = x1 + " - " + hours + ":" + minutes + ":" + seconds + " " + ampm; |
There was a problem hiding this comment.
Why are we making changes to the clock here?
|
|
||
| $(document).on('click', '.notification', function() { | ||
| // Update badge after notification is clicked | ||
| setTimeout(window.update_notification_badge_count, 100); |
There was a problem hiding this comment.
Why are we polling so frequently here? If I merged this as-is, I wouldn't be surprised if it had a noticeable impact on puter.com. We get notifications over websockets, so we can usually update the count there; this should then be a very infrequent poll (several minutes) just in case the badge goes out of sync.
| } | ||
|
|
||
| const db = await open({ | ||
| filename: window.dbPath || './puter-database.sqlite', |
There was a problem hiding this comment.
ngl this scared the crap out of me because I thought maybe this is working. After a lot of manual testing I can confirm that this does not work. Not only does this not work, but this won't and should not ever work. If you ever do this and it works it means there's a severe security issue in the project your contributing to.
…ad more button testing.
…ication read or not.
|
I was not able to complete manual testing due to GUI build errors. pr-1189_1.6x.mp4 |
|
Thanks for fixing, here's my second manual test. I noticed a few issues. I also noticed there was a
pr-1189_mt-2_1.5x.mp4 |
|
I did another manual test. This looks a lot better than previously. I noticed the notification toast is still gone but I see discussion that that's still being worked on. pr-1189_mt-3_1.5x.mp4 |
Fixed the notification management system with a load more button to load additional notifications