feat: add ntfy support#3462
Conversation
|
Hey @pauln17, please make sure your Prettier is updated. Even then, if you see widespread changes in the source control panel, once you stage the files, other file changes should be ignored/not added. Before doing this, could you please confirm whether there are actual backspaces or if it's just showing file changes in the source control panel? |
|
Hi @akashmannil, my prettier versions are 3.6.2 on server and 3.8.1 on client, I reinstalled a couple times. I ran format on only files I changed this time and it is all visible changes now. Edit (Two Things):
|
|
Hey @pauln17, if possible after running npm run format on server/client directories and staging those files, can you please share me the ss of source control panel in VS code(or are you using any other IDE)? Clarification for 1 - you can raise another PR later for additions. Clarification 2 - My take is wait for the PR to merge, then rebase and make changes. Im not sure, if we're expecting more changes in that PR |
|
Hi @akashmannil, Thanks for the clarifications. The most recent commit should show all the changes for running npm run format on only the files I touched, unless I am missing something. If you specifically want, I could checkout to the previous commit, run format and send you the SS. I am using VS Code via Cursor. |
ajhollid
left a comment
There was a problem hiding this comment.
Hello!
This PR is currently not thoroughly tested so I won't review it further than I have.
Please thoroughly test the PR and make sure the implementation is technically correct before we get into stylistic issues.
PRs that aren't fully functional cannot be properly reviewed.
Thanks for your interest in the project, we'll be happy to review this more thoroughly when it is in working order!
352383f to
b43c886
Compare
|
Hi @ajhollid, Thanks for your time and advice. I had misunderstood how the database side works, so for now I reverted the Timescale-related changes and implemented ntfy support using the default database, MongoDB. Most of my implementation and testing was done with MongoDB, and to the best of my knowledge it should be working properly. I’m still actively trying to better understand how Timescale is set up, especially around migrations. If that work belongs in this PR, I’m happy to continue with it, but if it makes more sense as a separate PR, let me know. I also used PR #3454 as a reference in a few places, including authType, sanitizeNotifications, and some related patterns. I’m still new to contributing here, so I really appreciate the feedback and I’m doing my best to learn and improve as I go. |
No problem, we all start off somewhere 👍 Due to the significant investment in resources it takes to properly review a PR we do need to be very selective in what we review. That said, it is important that any PR that modifies the DB schema does so for both MongoDB and TimescaleDB. What would happen if the change was only made to the MongoDB schemas, but a user is using TimescaleDB? Since this change is at the database level, the application will still compile, but we will have a runtime error when the user tries to use the new feature. So the answer to your question is yes, the TimescaleDB implementation is required for this feature as you are touching hte database layer. TimescaleDB is an extension of PostgreSQL so there really isn't anything fancy going on there, especially in the context of notifications. All that is really required here is to add a migration to add the new columns, and update the parameterized queries. Hope that helps point you in the right direction! |
|
Thanks @ajhollid, I apologize if mentioning you is incorrect. I pushed a recent commit for ntfy compatibility with timescale/postgres, but I have a few concerns:
|
|
hey @pauln17, heads up: i don't know this codebase and i didn't read through the complete code. i just really want ntfy support in checkmate, so i ran this pr end-to-end against my own self-hosted ntfy to see how it behaves. hope it helps. tested blocker: missing migration file
i got past it locally with a no-op stub just so i could test the rest. the real file looks like it just needs to follow the
happy pathsafter the stub, all three auth modes work against a real ntfy:
two bugs, both matching your own notesform leaks stale fields across auth type changes. switch from bearer to basic and
every failure shows "sending notification failed". the status code from happy to re-run the same matrix once the migration file lands. |
ac2e190 to
2c14982
Compare
|
Hey @pranav-bhatkar, I really appreciate the review and testing you did! For the missing migration file, it looks like the TimescaleDB migration directory was being gitignored, so I did not realize the file had not been committed. That should be fixed now. As for the two bugs:
|
5a529b1 to
74929de
Compare
83e5e28 to
035245b
Compare
035245b to
89ba500
Compare


Describe your changes
Briefly describe the changes you made and their purpose.
Added support for ntfy (notification option), allowing for Checkmate to send alerts (test + real notifications). Handled authentication methods for ntfy (basic username + password or bearer access token) for protected topics.
UI changes (Added section for NTFY):

Write your issue number after "Fixes "
Fixes #2817
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Additional Notes for Reviewers