fix: discussions-url

[Araxeus]

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

• Fix discussion url in notifications (NotificationRow && native notifications)
• Add commentId to all notifications (regex from subject.last_comment_url or special logic for discussions)
• Add repo auth scope (can be reverted, but it allows the discussion fix to work on private repo notifications)
• Upgraded output to es2020 to allow newer syntax
• 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

• Hotfix missing discussion items url #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 1eaaa74

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
Araxeus/gitify@discussions-url-fix...markOnClick-native-notification (fix markOnClick option for native notifications)
Araxeus/gitify@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 #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 🚀

[afonsojramos]

Looks good!! Great PR here! I do agree that it might be bigger than the standard, but still very manageable.

[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 1eaaa74 / Araxeus/gitify@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 😉