refactor: extract shared utilities and constants from code review

- Extract mergeAndSaveIfCurrent() helper to deduplicate version-guarded saves
- Add classifyError() to format-utils.js for consistent error categorization
- Add buildProfileUrl() to url-builder.js, replacing duplicate implementations
- Move concurrency limits (VISIBLE_COUNT, PRIORITY, BACKGROUND) to constants.js
- Fix 'Chrome minimum' comment to 'Browser minimum'

Author: euxx

src/background/service-worker.js

@@ -12,8 +12,9 @@ import {
   MESSAGE_TYPES,
   NOTIFICATION_TYPES,
   NOTIFICATION_TYPE_ICONS,
+  CONCURRENCY,
 } from "../lib/constants.js";
-import { formatReason } from "../lib/format-utils.js";
+import { formatReason, classifyError } from "../lib/format-utils.js";
 import { buildNotificationUrl } from "../lib/url-builder.js";
 import { LRUCache, DEFAULT_LRU_CACHE_SIZE } from "../lib/lru-cache.js";
 
@@ -279,6 +280,30 @@ function filterToCurrentlyStored(detailedNotifications, currentStoredNotificatio
   return detailedNotifications.filter((n) => currentStoredIds.has(n.id));
 }
 
+/**
+ * Merge notifications with current storage and save, guarded by fetch version.
+ * Aborts if a newer fetch has started (before or during the async storage read).
+ * @param {number} fetchVersion - Version of the fetch that produced these notifications
+ * @param {Array} notifications - Notifications to save
+ * @param {string} label - Log label for debugging
+ * @returns {Promise<boolean>} true if saved, false if superseded
+ */
+async function mergeAndSaveIfCurrent(fetchVersion, notifications, label) {
+  if (fetchVersion < notificationFetchVersion) {
+    console.log(`Fetch #${fetchVersion} superseded before ${label}, skipping`);
+    return false;
+  }
+  const currentStored = await storage.getNotifications();
+  // Re-check after async read: user mark-as-read may have bumped the version
+  if (fetchVersion < notificationFetchVersion) {
+    console.log(`Fetch #${fetchVersion} superseded during ${label} storage read, skipping`);
+    return false;
+  }
+  const safe = filterToCurrentlyStored(notifications, currentStored);
+  await storage.setNotifications(safe);
+  return true;
+}
+
 /**
  * Check for new notifications
  *
@@ -414,23 +439,22 @@ async function checkNotifications() {
         }
       }
 
-      // Split into priority (first 10 visible) and background loading
-      const VISIBLE_COUNT = 10; // Approximately one screen of notifications
-      const priorityNotifications = notificationsNeedingDetails.slice(0, VISIBLE_COUNT);
-      const backgroundNotifications = notificationsNeedingDetails.slice(VISIBLE_COUNT);
+      // Split into priority (visible) and background loading
+      const priorityNotifications = notificationsNeedingDetails.slice(0, CONCURRENCY.VISIBLE_COUNT);
+      const backgroundNotifications = notificationsNeedingDetails.slice(CONCURRENCY.VISIBLE_COUNT);
 
       console.log(
         `Loading details: ${priorityNotifications.length} priority, ${backgroundNotifications.length} background`,
       );
 
-      // Priority loading: First 10 notifications (visible on screen)
+      // Priority loading: First visible notifications
       let priorityResults = []; // Define in outer scope for background logging
       if (priorityNotifications.length > 0) {
         priorityResults = await fetchWithConcurrencyLimit(
           priorityNotifications.map(({ notification: n, index }) =>
             createDetailFetchTask(n, index, detailedNotifications),
           ),
-          5, // Concurrency limit: 5 requests at a time
+          CONCURRENCY.PRIORITY,
         );
 
         // Log priority loading results
@@ -440,21 +464,16 @@ async function checkNotifications() {
           `Fetch #${currentFetchVersion} priority: ${prioritySuccess} loaded, ${priorityFailed} failed`,
         );
 
-        // Check if superseded before updating
-        if (currentFetchVersion >= notificationFetchVersion) {
-          // Merge with current storage to avoid overwriting user deletions
-          const currentStored = await storage.getNotifications();
-          // Re-check version after async read: a user mark-as-read may have bumped
-          // notificationFetchVersion between the read and write, making our snapshot stale.
-          if (currentFetchVersion >= notificationFetchVersion) {
-            const safeDetailed = filterToCurrentlyStored(detailedNotifications, currentStored);
-
-            // Save priority notifications immediately
-            await storage.setNotifications(safeDetailed);
-            console.log(
-              `Fetch #${currentFetchVersion} saved ${priorityNotifications.length} priority notifications`,
-            );
-          }
+        // Merge with current storage and save (guarded by version check)
+        const saved = await mergeAndSaveIfCurrent(
+          currentFetchVersion,
+          detailedNotifications,
+          "priority save",
+        );
+        if (saved) {
+          console.log(
+            `Fetch #${currentFetchVersion} saved ${priorityNotifications.length} priority notifications`,
+          );
         }
       }
 
@@ -465,7 +484,7 @@ async function checkNotifications() {
           backgroundNotifications.map(({ notification: n, index }) =>
             createDetailFetchTask(n, index, detailedNotifications),
           ),
-          3, // Lower concurrency for background: 3 requests at a time
+          CONCURRENCY.BACKGROUND,
         )
           .then(async (backgroundResults) => {
             // Check if a newer fetch has completed while we were fetching details
@@ -490,27 +509,15 @@ async function checkNotifications() {
               `Author cache: ${cacheStats.size}/${cacheStats.maxSize} (${cacheStats.utilization})`,
             );
 
-            // Double-check before final save
-            if (currentFetchVersion >= notificationFetchVersion) {
-              // Merge with current storage to avoid overwriting user deletions
-              const currentStoredNotifications = await storage.getNotifications();
-              // Re-check version after async read: a user mark-as-read may have bumped
-              // notificationFetchVersion between the read and write, making our snapshot stale.
-              if (currentFetchVersion >= notificationFetchVersion) {
-                const safeDetailed = filterToCurrentlyStored(
-                  detailedNotifications,
-                  currentStoredNotifications,
-                );
-
-                // Update storage with all completed details
-                await storage.setNotifications(safeDetailed);
-                console.log(
-                  `Fetch #${currentFetchVersion} updated storage with detailed notifications`,
-                );
-              }
-            } else {
+            // Merge with current storage and save (guarded by version check)
+            const saved = await mergeAndSaveIfCurrent(
+              currentFetchVersion,
+              detailedNotifications,
+              "background save",
+            );
+            if (saved) {
               console.log(
-                `Fetch #${currentFetchVersion} superseded before final save, skipping storage update`,
+                `Fetch #${currentFetchVersion} updated storage with detailed notifications`,
               );
             }
           })
@@ -529,27 +536,21 @@ async function checkNotifications() {
     console.error(`Failed to check notifications (fetch #${currentFetchVersion}):`, error);
 
     // Handle different error types with appropriate UI feedback
-    if (error.message && error.message.includes("Rate limited")) {
-      // Rate limited - show timer badge with reset info
+    const errorType = classifyError(error);
+    if (errorType === "rate-limited") {
       const rateLimitInfo = github.getRateLimitInfo();
       await action.setBadgeText({ text: "⏱" });
       await action.setBadgeBackgroundColor({ color: BADGE_COLORS.RATE_LIMITED });
       await action.setTitle({
         title: `Rate limited. Resets ${rateLimitInfo.resetIn || "soon"}`,
       });
-    } else if (error.message && error.message.includes("timeout")) {
-      // Network timeout
+    } else if (errorType === "timeout") {
       await action.setBadgeText({ text: "⏱" });
       await action.setBadgeBackgroundColor({ color: BADGE_COLORS.TIMEOUT });
       await action.setTitle({ title: "Request timeout - will retry" });
-    } else if (
-      error.message &&
-      (error.message.includes("NetworkError") || error.message.includes("Failed to fetch"))
-    ) {
-      // Network error - keep last known state, update title only
+    } else if (errorType === "offline") {
       await action.setTitle({ title: "Offline - showing cached data" });
     } else {
-      // Other errors
       console.error("Unexpected error:", error);
       await action.setTitle({ title: `Error: ${error.message}` });
     }

src/lib/constants.js

@@ -5,7 +5,7 @@
 
 // Alarm Configuration
 export const ALARM_NAME = "check-notifications";
-export const DEFAULT_POLL_INTERVAL_MINUTES = 1; // Chrome minimum is 1 minute
+export const DEFAULT_POLL_INTERVAL_MINUTES = 1; // Browser minimum is 1 minute
 
 // API Configuration
 export const GITHUB_API_BASE = "https://api.github.com";
@@ -42,6 +42,13 @@ export const API_TIMEOUTS = {
   RETRY_REQUEST_BASE_DELAY: 500,
 };
 
+// Concurrency Configuration
+export const CONCURRENCY = {
+  PRIORITY: 5, // Concurrent requests for visible notifications
+  BACKGROUND: 3, // Concurrent requests for remaining notifications
+  VISIBLE_COUNT: 10, // Approximately one screen of notifications
+};
+
 // Time Conversion
 export const TIME_CONVERSION = {
   MS_PER_SECOND: 1000,

src/lib/format-utils.js

@@ -94,3 +94,16 @@ export function escapeHtml(text) {
     .replace(/"/g, """)
     .replace(/'/g, "'");
 }
+
+/**
+ * Classify a network/API error into a category for consistent handling
+ * @param {Error} error - Error object
+ * @returns {'rate-limited'|'timeout'|'offline'|'unknown'} Error category
+ */
+export function classifyError(error) {
+  const msg = error?.message || "";
+  if (msg.includes("Rate limited")) return "rate-limited";
+  if (msg.includes("timeout")) return "timeout";
+  if (msg.includes("NetworkError") || msg.includes("Failed to fetch")) return "offline";
+  return "unknown";
+}

src/lib/url-builder.js

@@ -137,3 +137,13 @@ function buildVulnerabilityUrl(fullName) {
 function buildDependabotUrl(fullName) {
   return `${GITHUB_SITE_BASE}/${fullName}/security/dependabot`;
 }
+
+/**
+ * Build GitHub profile URL from a username/login
+ * @param {string|null|undefined} login - GitHub username
+ * @returns {string|null} Profile URL or null if login is falsy
+ */
+export function buildProfileUrl(login) {
+  if (!login) return null;
+  return `${GITHUB_SITE_BASE}/${encodeURIComponent(login)}`;
+}

src/popup/notification-renderer.js

@@ -10,6 +10,7 @@ import {
 } from "../lib/constants.js";
 import { formatReason, getNotificationStatus } from "../lib/format-utils.js";
 import { getIconSVGElement } from "../lib/icons.js";
+import { buildProfileUrl } from "../lib/url-builder.js";
 
 /**
  * Build icon class with state information
@@ -51,10 +52,7 @@ export function truncateReleaseBody(body, maxLength = 200) {
   return trimmed.substring(0, maxLength);
 }
 
-function buildAuthorProfileUrl(login) {
-  if (!login) return null;
-  return `https://github.com/${encodeURIComponent(login)}`;
-}
+// Author profile URL construction delegated to url-builder.js (buildProfileUrl)
 
 // Cache for notifications to avoid unnecessary re-renders
 let cachedNotifications = null;
@@ -141,7 +139,7 @@ export function createHoverCard(notif) {
   const hasAuthor = notif.author?.login;
   const hasComments = notif.comment_count > 0;
   const hasDescription = notif.body?.trim();
-  const authorProfileUrl = hasAuthor ? buildAuthorProfileUrl(notif.author.login) : null;
+  const authorProfileUrl = hasAuthor ? buildProfileUrl(notif.author.login) : null;
 
   const card = document.createElement("div");
   card.className = "notification-hover-card";
@@ -284,7 +282,7 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
   const releaseBody =
     notif.type === NOTIFICATION_TYPES.RELEASE && notif.body ? notif.body.trim() : "";
   const truncatedBody = releaseBody ? truncateReleaseBody(releaseBody) : "";
-  const authorProfileUrl = notif.author?.login ? buildAuthorProfileUrl(notif.author.login) : null;
+  const authorProfileUrl = notif.author?.login ? buildProfileUrl(notif.author.login) : null;
 
   // Create notification icon container
   const iconDiv = document.createElement("div");

src/popup/popup.js

@@ -15,6 +15,8 @@ import {
   TIMING_THRESHOLDS,
 } from "../lib/constants.js";
 import { applyTheme } from "../lib/theme.js";
+import { buildProfileUrl } from "../lib/url-builder.js";
+import { classifyError } from "../lib/format-utils.js";
 import {
   initRenderer,
   renderNotifications,
@@ -47,7 +49,7 @@ function buildUserProfileUrl(username, userInfo) {
   // Fallback to building URL from username (GitHub.com only)
   const login = username || userInfo?.login;
   if (!login || login === "User") return null;
-  return `https://github.com/${encodeURIComponent(login)}`;
+  return buildProfileUrl(login);
 }
 
 /**
@@ -628,17 +630,16 @@ async function refresh() {
     let message;
     let className = "error-message";
 
-    if (
-      !navigator.onLine ||
-      error.message?.includes("NetworkError") ||
-      error.message?.includes("Failed to fetch")
-    ) {
+    // Use shared error classification (also used by service-worker)
+    const errorType = classifyError(error);
+
+    if (!navigator.onLine || errorType === "offline") {
       message = "⚠️ Offline - showing cached notifications";
       className = "offline-message";
-    } else if (error.message?.includes("timeout")) {
+    } else if (errorType === "timeout") {
       message = "⏱ Request timeout - showing cached data";
       className = "warning-message";
-    } else if (error.message?.includes("Rate limited")) {
+    } else if (errorType === "rate-limited") {
       message = "⏱ Rate limited - will retry automatically";
       className = "warning-message";
     } else {