fix discussions-url

[Araxeus]

fix #521 fix #531 fix #500 (indirectly) Feats:

• [x] Fix discussion url in notifications (NotificationRow && native notifications)
• [x] Add commentId to all notifications (regex from subject.last_comment_url or special logic for discussions)
• [x] Add repo auth scope (can be reverted, but it allows the discussion fix to work on private repo notifications)
• [x] Upgraded output to es2020 to allow newer syntax
• [x] Added tests (and fixed some)

we get the Discussion thread url using GraphQL search in both NotificationRow and NativeNotification

main filters are the discussion's repo, name (exact match), lastUpdated (>notificationCreationTime-2Hours), and viewerSubscription (only if there are still multiple discussions with the same exact name that qualify)

if either:

• somehow none are found (I don't really see how that would be possible, just a precaution)
• its a discussion in a private repository and user have no repo scope in his token/OAuth

then it will default to the method in #487

---

Notes

• #487 only makes the url redirect to the general discussions tab, which isn't super useful (and also doesn't fix native notifications)
• I've created a repo which can be used to test the GraphQL query externally: https://github.com/Araxeus/github_discussion_notification_url
• most added logic can be found in utils/helpers.ts (and implemented in components/NotificationRow.tsx and utils/notifications.ts)
• all tests (and added tests) pass
• if this is merged I will open a pr that fixes markOnClick option for native notifications in https://github.com/manosim/gitify/commit/1eaaa74541de697994607023952688de8f86b9ea

I'm guessing this Repo is abandoned (no activity and all dependencies out of date) but I hope this can help someone

if anyone wants an updated Gitify windows setup.exe that includes this and a few more fixes - its available at https://github.com/Araxeus/gitify/releases/tag/v4.3.2

[codebytere]

hey @Araxeus - this is great work but it's a bit much for one PR. Can we split this up so we can more effectively review? For example, es2020 upgrade should be a standalone PR.

[Araxeus]

hey @Araxeus - this is great work but it's a bit much for one PR. Can we split this up so we can more effectively review? For example, es2020 upgrade should be a standalone PR.

First of all thank you for taking the time to look at this PR and give feedback, I really appreciate it :)

The way I see it, All changes are directly related to the main change (discussion-url)

• es2020 - allows using the array functions flatMap() and at() which are used here (I guess we could use map().flat() and arr[arr.length - 1] if you really don't want newer syntax on this PR)
• typescript - errors are thrown if it isn't updated
• native notification change - directly related to the new behavior of helpers.openInBrowser() (both NotificationRow.tsx and notifications.ts use this function instead of handling it themselves)
• add commentId - its part of the logic in most of the changed functions, especially the GraphQL search
• interfaces/types + tests are half of the added code but pretty sure they are needed :P

Btw I have 2 more PR's ready that depends on this one but are unrelated, so I'm waiting for this one to be merged first https://github.com/Araxeus/gitify/compare/discussions-url-fix...markOnClick-native-notification (fix markOnClick option for native notifications) https://github.com/Araxeus/gitify/compare/discussions-url-fix...update-dependencies (update dependencies and rework remote to @Electron/Remote)

Maybe I'm mistaken or missing something? please let me know your thoughts

[Araxeus]

I could make the commentId linking optional in routes/Settings.tsx if you think that would be better

and then changing both https://github.com/manosim/gitify/blob/8c93f916a35d9bb05e7f91c9fab1b1cf8543250a/src/utils/helpers.ts#L148 https://github.com/manosim/gitify/blob/8c93f916a35d9bb05e7f91c9fab1b1cf8543250a/src/utils/helpers.ts#L158

to settings.commentId && latestCommentId ? ...

and then maybe make it enabled by default? in: https://github.com/manosim/gitify/blob/f8a6ac26c5a58d17f9c2eb639489b045aec1a780/src/context/App.tsx#L32

once again that would be more seemingly unrelated code so maybe in a follow-up PR if needed

[bmulholland]

@codebytere Thing is, this PR spiritually builds on top of https://github.com/manosim/gitify/pull/487. That PR is a smaller chunk of this one, but it's sitting without being merged. So I understand that this one is big, but also there's actions that could be taken to make it less imposing, and those are blocked until a maintainer can help move existing PRs forward.

Should we open a broader discussion on how to improve the maintenance of this repo?

[Araxeus]

@manosim @codebytere whats up?

If this repo is abandoned, can you transfer ownership or give release rights to other people?

[Araxeus]

@afonsojramos thoughts?

[afonsojramos]

@Araxeus haven't reached this one yet. Still combing through the issues while on a full-time job. This repo will eventually be ✨ cleaned out ✨!

[setchy]

very excited for this feature to be added 🚀

[Araxeus]

Haha amazing, it only took 2 years for this PR to get some love 😅

Might be worth to take a look at the other patch/fix I had lined up (fix native notifications not marking as read on click in https://github.com/gitify-app/gitify/commit/1eaaa74541de697994607023952688de8f86b9ea / https://github.com/Araxeus/gitify/compare/discussions-url-fix...markOnClick-native-notification)

[afonsojramos]

Better late than never 😅 Let's keep on working on this and maybe think of a Tauri rewrite 👀 Added you to the collaborators list btw 😉