From 3d0aaaae722c10d55be8b719d876821a86d6a0bb Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 9 Sep 2024 18:00:34 -0400 Subject: [PATCH] Fetch unread notifications in addition to read ones (#194) * Extract helper functions from mergeNotifications * Fetch unread notifications in addition to read ones * Fetch accounts in parellel * Extract fetching comment/subject into own functions * Extract building Note into own function * Load extra notification data in parallel --- src/main/lib/github-interface.ts | 200 ++++++++++++++++++++-------- src/renderer/lib/gitnews-fetcher.ts | 25 +++- src/renderer/lib/helpers.ts | 39 ++++-- 3 files changed, 195 insertions(+), 69 deletions(-) diff --git a/src/main/lib/github-interface.ts b/src/main/lib/github-interface.ts index 8fa6df2..ce64f6b 100644 --- a/src/main/lib/github-interface.ts +++ b/src/main/lib/github-interface.ts @@ -101,76 +101,172 @@ export async function markNotficationAsRead( } } +interface RawNotification { + id: string; + url: string; + subject: { + title: string; + type: string; + latest_comment_url: string; + url: string; + }; + unread: boolean; + reason: string; + repository: { + full_name: string; + name: string; + owner: { + avatar_url: string; + }; + }; + updated_at: string; +} + +interface CommentData { + commentAvatar: string; + commentHtmlUrl: string; +} + +async function getCommentDataForNotification( + octokit: Octokit, + account: AccountInfo, + notification: RawNotification +): Promise { + let commentAvatar: string = ''; + let commentHtmlUrl: string = ''; + const commentPath = getOctokitRequestPathFromUrl( + account, + notification.subject.latest_comment_url ?? notification.subject.url + ); + try { + const comment = await octokit.request(`GET ${commentPath}`, {}); + commentAvatar = comment.data.user.avatar_url; + commentHtmlUrl = comment.data.html_url; + } catch (error) { + logMessage( + `Failed to fetch comment for ${commentPath} (${notification.subject.latest_comment_url ?? notification.subject.url})`, + 'error' + ); + } + return { + commentAvatar, + commentHtmlUrl, + }; +} + +interface SubjectData { + noteState: string; + noteMerged: boolean; + subjectHtmlUrl: string; +} + +async function getSubjectDataForNotification( + octokit: Octokit, + account: AccountInfo, + notification: RawNotification +): Promise { + let noteState: string = ''; + let noteMerged: boolean = false; + let subjectHtmlUrl: string = ''; + const subjectPath = getOctokitRequestPathFromUrl( + account, + notification.subject.url + ); + try { + const subject = await octokit.request(`GET ${subjectPath}`, {}); + noteState = subject.data.state; + noteMerged = subject.data.merged; + subjectHtmlUrl = subject.data.html_url; + } catch (error) { + logMessage( + `Failed to fetch comment for ${subjectPath} (${notification.subject.url})`, + 'error' + ); + } + return { + noteState, + noteMerged, + subjectHtmlUrl, + }; +} + +function buildNoteFromData({ + account, + notification, + commentData, + subjectData, +}: { + account: AccountInfo; + notification: RawNotification; + commentData: CommentData; + subjectData: SubjectData; +}): Note { + return { + gitnewsAccountId: account.id, + id: notification.id, + url: notification.url, + title: notification.subject.title, + unread: notification.unread, + repositoryFullName: notification.repository.full_name, + commentUrl: commentData.commentHtmlUrl, + updatedAt: notification.updated_at, + repositoryName: notification.repository.name, + type: notification.subject.type, + subjectUrl: subjectData.subjectHtmlUrl, + commentAvatar: + commentData.commentAvatar ?? notification.repository.owner.avatar_url, + repositoryOwnerAvatar: notification.repository.owner.avatar_url, + api: { + subject: { state: subjectData.noteState, merged: subjectData.noteMerged }, + notification: { reason: notification.reason as NoteReason }, + }, + }; +} + export async function fetchNotificationsForAccount( account: AccountInfo ): Promise { const octokit = createOctokit(account); const notificationsResponse = await octokit.rest.activity.listNotificationsForAuthenticatedUser({ - all: false, + all: true, }); const notes: Note[] = []; + // We need to make more requests to get the details of each notification. Do + // these fetches in parallel. + let promises = []; for (const notification of notificationsResponse.data) { - let commentAvatar: string; - let commentHtmlUrl: string; - const commentPath = getOctokitRequestPathFromUrl( + logMessage( + `Fetching additional details for notification ${notification.id}`, + 'info' + ); + const commentPromise = getCommentDataForNotification( + octokit, account, - notification.subject.latest_comment_url ?? notification.subject.url + notification ); - try { - const comment = await octokit.request(`GET ${commentPath}`, {}); - commentAvatar = comment.data.user.avatar_url; - commentHtmlUrl = comment.data.html_url; - } catch (error) { - logMessage( - `Failed to fetch comment for ${commentPath} (${notification.subject.latest_comment_url ?? notification.subject.url})`, - 'error' - ); - continue; - } - - let noteState: string; - let noteMerged: boolean; - let subjectHtmlUrl: string; - const subjectPath = getOctokitRequestPathFromUrl( + const subjectPromise = getSubjectDataForNotification( + octokit, account, - notification.subject.url + notification ); - try { - const subject = await octokit.request(`GET ${subjectPath}`, {}); - noteState = subject.data.state; - noteMerged = subject.data.merged; - subjectHtmlUrl = subject.data.html_url; - } catch (error) { - logMessage( - `Failed to fetch comment for ${subjectPath} (${notification.subject.url})`, - 'error' + const promise = Promise.all([commentPromise, subjectPromise]); + promises.push(promise); + promise.then(([commentData, subjectData]) => { + notes.push( + buildNoteFromData({ + account, + notification, + subjectData, + commentData, + }) ); - continue; - } - - notes.push({ - gitnewsAccountId: account.id, - id: notification.id, - url: notification.url, - title: notification.subject.title, - unread: notification.unread, - repositoryFullName: notification.repository.full_name, - commentUrl: commentHtmlUrl, - updatedAt: notification.updated_at, - repositoryName: notification.repository.name, - type: notification.subject.type, - subjectUrl: subjectHtmlUrl, - commentAvatar: commentAvatar ?? notification.repository.owner.avatar_url, - repositoryOwnerAvatar: notification.repository.owner.avatar_url, - api: { - subject: { state: noteState, merged: noteMerged }, - notification: { reason: notification.reason as NoteReason }, - }, }); } + await Promise.all(promises); + return notes; } diff --git a/src/renderer/lib/gitnews-fetcher.ts b/src/renderer/lib/gitnews-fetcher.ts index 72fda5d..b8e0f69 100644 --- a/src/renderer/lib/gitnews-fetcher.ts +++ b/src/renderer/lib/gitnews-fetcher.ts @@ -70,6 +70,7 @@ export function createFetcher(): Middleware<{}, AppReduxState> { return next(action); } + // FIXME: why does this trigger so many times during a fetch? if (action.type === 'GITNEWS_FETCH_NOTIFICATIONS') { debug('Fetching accounts'); window.electronApi.logMessage('Fetching accounts', 'info'); @@ -143,13 +144,27 @@ export function createFetcher(): Middleware<{}, AppReduxState> { } return async () => { let allNotes: Note[] = []; + + const promises = []; + + // Do the fetching in parallel. for (const account of accounts) { - const notes = await fetchNotifications(account); - if ('error' in notes) { - throw notes.error; - } - allNotes = [...allNotes, ...notes]; + window.electronApi.logMessage( + `Fetching notifications for ${account.name} (${account.serverUrl})`, + 'info' + ); + const promise = fetchNotifications(account); + promises.push(promise); + promise.then((notes) => { + if ('error' in notes) { + throw notes.error; + } + allNotes = [...allNotes, ...notes]; + }); } + + await Promise.all(promises); + allNotes.sort((a, b) => { if (a.updatedAt < b.updatedAt) { return 1; diff --git a/src/renderer/lib/helpers.ts b/src/renderer/lib/helpers.ts index e48c0c6..0752168 100644 --- a/src/renderer/lib/helpers.ts +++ b/src/renderer/lib/helpers.ts @@ -7,26 +7,41 @@ export function getNoteId(note: Note) { return note.id; } +function hasNoteUpdated(note: Note, prevNote: Note): boolean { + if ( + note.updatedAt && + prevNote.gitnewsSeenAt && + Date.parse(note.updatedAt) > prevNote.gitnewsSeenAt + ) { + return true; + } + return false; +} + +function getMatchingPrevNote(prevNotes: Note[], note: Note): Note | undefined { + return prevNotes.find((prevNote) => getNoteId(prevNote) === getNoteId(note)); +} + +/** + * Return all the new notes, but if they match one of the previous notes, + * update the new note's "seen" and "unread" properties to match those of the + * previous note. + * + * This allows updated unread notes which have been "seen" to retain that + * property if the user has already seen them. + */ export function mergeNotifications( prevNotes: Note[], nextNotes: Note[] ): Note[] { - const getMatchingPrevNote = (note: Note) => - prevNotes.find((prevNote) => getNoteId(prevNote) === getNoteId(note)); - const hasNoteUpdated = (note: Note, prevNote: Note) => - Boolean( - note.updatedAt && - prevNote.gitnewsSeenAt && - Date.parse(note.updatedAt) > prevNote.gitnewsSeenAt - ); - return nextNotes.map((note) => { - const previousNote = getMatchingPrevNote(note); + const previousNote = getMatchingPrevNote(prevNotes, note); if (previousNote && !hasNoteUpdated(note, previousNote)) { - return Object.assign({}, note, { + return { + ...note, gitnewsSeen: previousNote.gitnewsSeen, gitnewsMarkedUnread: previousNote.gitnewsMarkedUnread, - }); + }; } return note; });