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
sirbrillig · Sep 9, 2024 · 3d0aaaa · 3d0aaaa
1 parent ea651ad
commit 3d0aaaa
Show file tree

Hide file tree

Showing 3 changed files with 195 additions and 69 deletions.
diff --git 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<CommentData> {
+	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<SubjectData> {
+	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<Note[]> {
 	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
@@ -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
@@ -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;
 	});