fix: store the popup results per tab in session storage

DOCUMENTATION.md

@@ -22,7 +22,9 @@ See the [README overview](README.md#overview) section.
 
 ## How it works
 
-The extension has two parts that communicate through `browser.storage.local`.
+The extension has two parts that communicate through per-tab results in
+`browser.storage.session` (configuration — tokens and self-hosted instances —
+lives in `browser.storage.local`).
 
 It runs on both Firefox and Chrome from a single source tree:
 - the manifest declares both `background.scripts` (Firefox event page) and
@@ -39,18 +41,22 @@ Runs on every tab update:
 2.   **Detect the forge** by hostname (exact match or subdomain)
 3.   **Load the auth headers** for the forge from the stored token (if any)
 4.   **Promise chain**: list PRs, get files, keep PRs including the file
-5.   **Render**: set the toolbar badge text and stores `{ tabId, prs }` in
-     `browser.storage.local`.
+5.   **Render**: set the toolbar badge text and store the result under the
+     tab's key in `browser.storage.session` — `{ prs }`, or `{ error }` when a
+     request failed. Per-tab keys keep concurrent tabs from clobbering each
+     other; the key is dropped when the tab closes or loads a new page.
 
 ### Popup
 
 On open, the popup:
 
 1.   shows a localized **loading state**
-2.   reads `{ tabId, prs }` from storage:
-    -   if it matches the active tab, renders the PR list
-    -   otherwise shows a localized **error** state
-    -   if the list is empty, shows a localized **empty** state.
+2.   reads the active tab's result from session storage:
+    -   `{ prs }`: renders the PR list (or a localized **empty** state when no
+        PR touches the file)
+    -   `{ error }`: shows a localized **error** state
+    -   nothing stored yet: the background is still fetching — keeps the
+        loading state and renders when the result lands.
 
 ## Project layout
 

background.js

@@ -1,6 +1,6 @@
 import { browser } from './browser.js'
 import { forgeForHostname } from './forges.js'
-import { loadInstances } from './storage.js'
+import { loadInstances, saveTabResult, clearTabResult } from './storage.js'
 
 //
 // Main
@@ -159,9 +159,8 @@ function render(prs, tabId) {
         tabId: tabId
     })
 
-    // save results
-    // keys: "tabId", "prs"
-    browser.storage.local.set({ tabId, prs })
+    // save the results for the popup, keyed by tab
+    saveTabResult(tabId, { prs })
 }
 
 async function main(tab) {
@@ -192,6 +191,10 @@ async function main(tab) {
         return
     }
 
+    // drop the result of the tab's previous page: the popup shows its loading
+    // state until this page's result lands
+    await clearTabResult(tab.id)
+
     try {
         // load the stored API token, then check the forge rate limit
         const requestHeaders = await loadAuthHeaders(forge)
@@ -210,7 +213,8 @@ async function main(tab) {
             text: "err",
             tabId: tab.id
         })
-        // display the error
+        // store the error state for the popup and display the error
+        saveTabResult(tab.id, { error: `${error}` })
         console.error(`${error}`)
     }
 }
@@ -233,13 +237,10 @@ function handleUpdated(tabId, changeInfo, tabInfo) {
 }
 
 async function handleRemoved(tabId, removeInfo) {
-    // remove info from storage
-    const results = await browser.storage.local.get()
-    if (Object.keys(results).length != 0 && results.tabId == tabId) {
-        try {
-            await browser.storage.local.remove(["tabId", "prs"])
-        } catch (error) {
-            console.error(`Error: ${error}`)
-        }
+    // forget the closed tab's result
+    try {
+        await clearTabResult(tabId)
+    } catch (error) {
+        console.error(`Error: ${error}`)
     }
 }

popup/popup.js

@@ -1,7 +1,7 @@
 import { browser } from "../browser.js"
 import { forgeForHostname } from "../forges.js"
 import { localize } from "../i18n.js"
-import { loadInstances } from "../storage.js"
+import { loadInstances, loadTabResult, onTabResult } from "../storage.js"
 
 //
 // Functions
@@ -53,9 +53,12 @@ export function renderError() {
     div.setAttribute("id", "error")
     div.textContent = browser.i18n.getMessage("popupErrorMessage")
 
-    // replace the ul element with the div element
+    // replace the ul element with the div element (already gone when an
+    // earlier result showed the error state)
     const ul = document.getElementById("prs")
-    document.body.replaceChild(div, ul)
+    if (ul) {
+        document.body.replaceChild(div, ul)
+    }
 }
 
 //
@@ -73,13 +76,16 @@ async function init() {
         return
     }
 
-    // get the prs list and render
-    const result = await browser.storage.local.get()
-    if (result.tabId == tabId) {
-        render(result.prs, urlInfo)
-    } else {
-        // error in bg script
-        renderError()
+    // show a result: the PR list on success, the error state on failure
+    const show = result => result.error ? renderError() : render(result.prs, urlInfo)
+
+    // the result is stored per tab; none stored yet means the background is
+    // still fetching, so keep the loading state and show the result when the
+    // background stores it (the listener registers first to not miss it)
+    onTabResult(tabId, show)
+    const result = await loadTabResult(tabId)
+    if (result) {
+        show(result)
     }
 }
 // register the popup logic against the browser; skipped when this module is

storage.js

@@ -14,3 +14,34 @@ export async function loadInstances() {
 export async function saveInstances(instances) {
     await browser.storage.local.set({ [INSTANCES_KEY]: instances })
 }
+
+// storage.session key holding the popup result for a tab. Per-tab keys let
+// concurrent tabs store results without clobbering each other, and the session
+// area clears itself when the browser closes.
+function tabResultKey(tabId) {
+    return `prs:${tabId}`
+}
+
+// save a tab's popup result: { prs } on success, { error } on failure
+export async function saveTabResult(tabId, result) {
+    await browser.storage.session.set({ [tabResultKey(tabId)]: result })
+}
+
+// load a tab's popup result (undefined while the background is still fetching)
+export async function loadTabResult(tabId) {
+    const stored = await browser.storage.session.get(tabResultKey(tabId))
+    return stored[tabResultKey(tabId)]
+}
+
+// forget a tab's popup result (tab closed, or a new page load under way)
+export async function clearTabResult(tabId) {
+    await browser.storage.session.remove(tabResultKey(tabId))
+}
+
+// call back with a tab's popup result whenever the background stores one
+export function onTabResult(tabId, callback) {
+    browser.storage.session.onChanged.addListener(changes => {
+        const change = changes[tabResultKey(tabId)]
+        if (change?.newValue) callback(change.newValue)
+    })
+}

tests/storage.test.js

@@ -11,19 +11,31 @@ import assert from "node:assert/strict"
 // process, so it cannot leak into other test files. Both assumptions break
 // under an in-process runner (e.g. --test-isolation=none).
 
-// a stub storage.local recording the last set()/remove() and returning a
-// configurable backing object from get()
+// stub storage areas recording the last set()/remove() and returning a
+// configurable backing object from get(); session also records its onChanged
+// listeners so tests can fire changes by hand
 let backing = {}
-const calls = { set: null }
+let sessionBacking = {}
+const sessionListeners = []
+const calls = { set: null, sessionSet: null, sessionRemoved: null }
 globalThis.browser = {
     storage: {
         local: {
             get: async (key) => (key in backing ? { [key]: backing[key] } : {}),
             set: async (obj) => { calls.set = obj },
         },
+        session: {
+            get: async (key) => (key in sessionBacking ? { [key]: sessionBacking[key] } : {}),
+            set: async (obj) => { calls.sessionSet = obj },
+            remove: async (key) => { calls.sessionRemoved = key },
+            onChanged: { addListener: (fn) => sessionListeners.push(fn) },
+        },
     },
 }
-const { loadInstances, saveInstances } = await import("../storage.js")
+const {
+    loadInstances, saveInstances,
+    saveTabResult, loadTabResult, clearTabResult, onTabResult,
+} = await import("../storage.js")
 
 test("loadInstances returns the stored instances", async () => {
     backing = { selfHostedInstances: [{ type: "gitlab", hostname: "git.example.com" }] }
@@ -42,3 +54,41 @@ test("saveInstances writes under the selfHostedInstances key", async () => {
     await saveInstances(instances)
     assert.deepEqual(calls.set, { selfHostedInstances: instances })
 })
+
+//
+// per-tab popup results (session storage)
+//
+test("saveTabResult writes under the tab's key in session storage", async () => {
+    await saveTabResult(7, { prs: [1, 2] })
+    assert.deepEqual(calls.sessionSet, { "prs:7": { prs: [1, 2] } })
+})
+
+test("loadTabResult returns the result stored for the tab", async () => {
+    sessionBacking = { "prs:7": { prs: [42] } }
+    assert.deepEqual(await loadTabResult(7), { prs: [42] })
+})
+
+test("loadTabResult returns undefined while no result is stored", async () => {
+    sessionBacking = {}
+    assert.equal(await loadTabResult(7), undefined)
+})
+
+test("clearTabResult removes the tab's key", async () => {
+    await clearTabResult(7)
+    assert.equal(calls.sessionRemoved, "prs:7")
+})
+
+test("onTabResult fires only for its own tab's stored results", () => {
+    const seen = []
+    onTabResult(7, result => seen.push(result))
+    const listener = sessionListeners.at(-1)
+
+    // another tab's result is ignored
+    listener({ "prs:8": { newValue: { prs: [1] } } })
+    // a removal (no newValue) is ignored
+    listener({ "prs:7": { oldValue: { prs: [1] } } })
+    // this tab's stored result fires the callback
+    listener({ "prs:7": { newValue: { error: "boom" } } })
+
+    assert.deepEqual(seen, [{ error: "boom" }])
+})