From e68db9389a84b3dd6c5b07432c592ca497ecf5f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Magimel?= Date: Sun, 5 Jul 2026 12:58:25 +0000 Subject: [PATCH] fix: store the popup results per tab in session storage The single global `{ tabId, prs }` slot in `storage.local` is replaced by `prs:${tabId}` key in `storage.session`. Then, results are stored per tab, two forge tabs can't clobber each other, and the session area clears itself when the browser closes. The stored value is `{ prs }` on success or `{ error }` on failure. This also lets the popup tell "failed" apart from "still loading". With this fix, the popup does not read all of storage. --- DOCUMENTATION.md | 20 +++++++++------ background.js | 27 ++++++++++---------- popup/popup.js | 26 +++++++++++-------- storage.js | 31 +++++++++++++++++++++++ tests/storage.test.js | 58 ++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 128 insertions(+), 34 deletions(-) diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 2c81d68..138a3b1 100644 --- a/DOCUMENTATION.md +++ b/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 diff --git a/background.js b/background.js index 1689061..374de24 100644 --- a/background.js +++ b/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}`) } } diff --git a/popup/popup.js b/popup/popup.js index 3e84df8..dce3fd7 100644 --- a/popup/popup.js +++ b/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 diff --git a/storage.js b/storage.js index 63536e6..798130a 100644 --- a/storage.js +++ b/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) + }) +} diff --git a/tests/storage.test.js b/tests/storage.test.js index 5939db3..8da3024 100644 --- a/tests/storage.test.js +++ b/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" }]) +})