From 534fab040f0247f5fc13914e2e9963646c3f3a38 Mon Sep 17 00:00:00 2001 From: Brendon Thiede Date: Fri, 18 Sep 2026 13:57:36 -0400 Subject: [PATCH 1/2] fix: Forget open tabs of files a Pull deletes Pybricks remembers open editor tabs as file uuids in sessionStorage (editor.activeFileHistory..) and reopens them after a reload. A tab whose file the Pull deleted failed that reopen with an "unexpected error ... file with uuid '...' not found" toast. pull() now computes the uuids apply-files will delete (pullmerge.js:deletedUuids) and prunes them from every open-tab history key (pruneOpenTabs) inside the reload timer, immediately before location.reload(), so the page's in-memory history can't rewrite them. The drive.mjs merge step opens gone.py and keep.py as tabs before the Pull and records toasts from page load; with the prune disabled it fails on the toast. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- README.md | 2 +- src/content.js | 17 +++++++++++- src/pullmerge.js | 54 ++++++++++++++++++++++++++++++++++++ test/e2e/README.md | 13 +++++++-- test/e2e/drive.mjs | 59 +++++++++++++++++++++++++++++++++++++++ test/load-pullmerge.mjs | 2 +- test/pullmerge.test.mjs | 61 ++++++++++++++++++++++++++++++++++++++++- 8 files changed, 203 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cdee4fa..f650902 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ Everything runs in the browser; the only external party is `github.com`: **The ISOLATED world is six classic scripts**, listed in `manifest.json` `content_scripts` in load order: `menu-config.js` (pure parse/generate/analyze helpers, no DOM), `blocksplice.js` (pure block-file setup splicing, no DOM — phase 4), `pullmerge.js` (pure Pull-merge planning, no DOM — see "Pull merges, it does not clobber" below), then `menu-panel.js` (`makeMenuPanel`) and `file-list.js` (`makeFileListWatcher`) which depend on those helpers being in scope, then `content.js` last — it wires the toolbar and constructs the panel/watcher. They share one global scope with no ESM `export`s, so **order matters** — a helper must be defined in an earlier file than its caller. The three pure-helper files have Node test shims (`test/load-menu-config.mjs`, `test/load-blocksplice.mjs`, `test/load-pullmerge.mjs`, same pattern as `load-inject.mjs`) and unit tests; the three DOM-heavy files have no Node loader and are exercised only via the browser E2E path. See the "Menu manager (phase 3)" and "Setup propagation (phase 4)" sections below. -**Pull merges, it does not clobber.** `content.js:pull()` never hands the repo's file set straight to `apply-files` — that op deletes every path it isn't given, which used to destroy uncommitted local work. `src/pullmerge.js:planPull()` (pure, no DOM, unit-tested via `test/load-pullmerge.mjs`) computes the complete desired set from the editor's files, the repo's files, the `lastPullShas` base, and the protected list. Files untouched since the last Pull take the repo's version; locally edited files let the repo keep the canonical path and are rescued to `_mine.py` (a valid module name, so the hub can still import it); files created locally and never committed keep their own name; protected paths **that the repo actually has** are overwritten with no rescue copy. Protection is checked after the never-committed rule on purpose — a manifest can reserve a path the repo doesn't have (the starter reserves `robot_setup.py`, never authored), and with no repo version to restore, "the repo wins" would just delete the file the kid made. Rescues are reported through the `pullRescued` key as a notice on the next load. Design: `docs/superpowers/specs/2026-08-09-pull-merge-design.md`. +**Pull merges, it does not clobber.** `content.js:pull()` never hands the repo's file set straight to `apply-files` — that op deletes every path it isn't given, which used to destroy uncommitted local work. `src/pullmerge.js:planPull()` (pure, no DOM, unit-tested via `test/load-pullmerge.mjs`) computes the complete desired set from the editor's files, the repo's files, the `lastPullShas` base, and the protected list. Files untouched since the last Pull take the repo's version; locally edited files let the repo keep the canonical path and are rescued to `_mine.py` (a valid module name, so the hub can still import it); files created locally and never committed keep their own name; protected paths **that the repo actually has** are overwritten with no rescue copy. Protection is checked after the never-committed rule on purpose — a manifest can reserve a path the repo doesn't have (the starter reserves `robot_setup.py`, never authored), and with no repo version to restore, "the repo wins" would just delete the file the kid made. Rescues are reported through the `pullRescued` key as a notice on the next load. **Open tabs of deleted files are forgotten.** Pybricks remembers open editor tabs as a JSON array of file uuids in sessionStorage, under `editor.activeFileHistory..` (`ActiveFileHistoryManager`, pybricks-code `src/editor/lib.ts`), and reopens them after a reload. A uuid whose file the Pull deleted raises Pybricks' "unexpected error … file with uuid '…' not found" toast. So `pull()` computes the doomed uuids before `apply-files` (`pullmerge.js:deletedUuids`) and prunes them from every such key (`pruneOpenTabs`) **inside the reload timer, immediately before `location.reload()`**. Earlier would be undone, because the page's in-memory history rewrites the key whenever a tab opens or closes. Design: `docs/superpowers/specs/2026-08-09-pull-merge-design.md`. **Why an ISOLATED/MAIN split:** The MAIN-world script (`inject.js`) can see the page's globals (and could in principle use the page's Dexie instance, though we don't); the ISOLATED-world scripts have access to `chrome.runtime` APIs so they can message the service worker. The split is mandatory — only the MAIN world reaches the page's IndexedDB, only the ISOLATED world reaches `chrome.runtime`. They communicate via `window.postMessage` with `pybricks-git:request` / `pybricks-git:response` envelopes; `content.js` reaches the service worker via `chrome.runtime.sendMessage`. diff --git a/README.md b/README.md index 05a99c6..a20b117 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ When the mentor updates the upstream shared repository, each team pulls the chan In rough priority order: -1. **Open-tab cleanup on delete** — when Pull deletes a file, also clean up its entry in Pybricks' "open tabs" state so the page doesn't log a non-fatal error after reload. +Nothing queued right now. ## License diff --git a/src/content.js b/src/content.js index 2ce9ee3..fa7d555 100644 --- a/src/content.js +++ b/src/content.js @@ -478,6 +478,9 @@ async function pull(btn) { console.warn('[pybricks-git] rescued local edits:', plan.rescued); } + // The files apply-files is about to delete — their uuids must also + // leave Pybricks' open-tab history, or the reload tries to reopen them. + const goneUuids = deletedUuids(editor.metadata, plan.files.map((f) => f.path)); const summary = await pageRequest('apply-files', { files: plan.files }); console.log('[pybricks-git] applied:', summary); btn.textContent = `↓ +${summary.added} ~${summary.changed} -${summary.deleted}`; @@ -492,7 +495,19 @@ async function pull(btn) { // dexie-observable doesn't see raw IDB writes, so reload to refresh // the React UI. Brief delay so the user can see the summary. if (summary.added || summary.changed || summary.deleted) { - setTimeout(() => location.reload(), 1500); + setTimeout(() => { + // Prune right before the reload, not earlier: the page's + // in-memory tab history rewrites sessionStorage whenever a tab + // opens or closes, so an earlier prune could be undone. + try { + pruneOpenTabs(sessionStorage, goneUuids); + } catch (err) { + // Storage can be unavailable; the cost is only Pybricks' + // own "file not found" toast after the reload. + console.warn('[pybricks-git] open-tab cleanup failed:', err); + } + location.reload(); + }, 1500); } else { setTimeout(() => (btn.textContent = original), 3000); } diff --git a/src/pullmerge.js b/src/pullmerge.js index efb6334..39e93c1 100644 --- a/src/pullmerge.js +++ b/src/pullmerge.js @@ -72,3 +72,57 @@ function planPull({ local, repo, base = {}, protectedPaths = [] }) { } return { files, rescued }; } + +// --- Open-tab cleanup --------------------------------------------------------- +// +// Pybricks remembers open editor tabs in sessionStorage, as a JSON array of +// file uuids under `editor.activeFileHistory..` +// (pybricks-code src/editor/lib.ts ActiveFileHistoryManager), and reopens each +// on load. A uuid whose file a Pull deleted fails that reopen with an +// "unexpected error" toast ("file with uuid '…' not found"). These helpers +// find the uuids a Pull removes and prune them from that history. + +const OPEN_TAB_HISTORY_PREFIX = 'editor.activeFileHistory.'; + +// uuids of the editor's files that are absent from `keptPaths` — exactly what +// apply-files deletes when handed a file set with those paths. +// metadata [{path, uuid}] the editor's metadata rows before the apply +// keptPaths [path] the paths passed to apply-files +function deletedUuids(metadata, keptPaths) { + const kept = new Set(keptPaths); + return metadata.filter((m) => !kept.has(m.path)).map((m) => m.uuid); +} + +// One history value with `uuids` removed. Returns the new JSON string, or null +// when nothing changes (including a value that isn't a JSON array — Pybricks +// itself treats that as empty, so it's left alone). +function pruneTabHistory(value, uuids) { + let history; + try { + history = JSON.parse(value); + } catch { + return null; + } + if (!Array.isArray(history)) return null; + const drop = new Set(uuids); + const pruned = history.filter((u) => !drop.has(u)); + return pruned.length === history.length ? null : JSON.stringify(pruned); +} + +// Applies pruneTabHistory to every open-tab history key in `storage` (a Web +// Storage object — sessionStorage in the page). Returns the number of keys +// rewritten. +function pruneOpenTabs(storage, uuids) { + if (!uuids.length) return 0; + let rewritten = 0; + for (let i = 0; i < storage.length; i++) { + const key = storage.key(i); + if (!key || !key.startsWith(OPEN_TAB_HISTORY_PREFIX)) continue; + const next = pruneTabHistory(storage.getItem(key), uuids); + if (next !== null) { + storage.setItem(key, next); + rewritten++; + } + } + return rewritten; +} diff --git a/test/e2e/README.md b/test/e2e/README.md index 9616f60..b25c20b 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -79,8 +79,9 @@ have to rediscover them: 8. **Merge on Pull.** Writes local edits straight into IndexedDB via `upsert-files` (a new `scratch.py`, an edited `starter.py`, an edited `coach.py`), pushes a competing commit to the bare repo - (`starter.py`/`keep.py`/`coach.py` changed, `gone.py` deleted), real-clicks - **Pull** again → asserts label `↓ +1 ~3 -1` → waits for reload → asserts the + (`starter.py`/`keep.py`/`coach.py` changed, `gone.py` deleted), opens + `keep.py` and `gone.py` as editor tabs (`editor.action.activateFile` on the + app's store), real-clicks **Pull** again → asserts label `↓ +1 ~3 -1` → waits for reload → asserts the rescue notice names `starter.py`/`starter_mine.py` and says nothing about the untouched `keep.py` or the protected `coach.py` → asserts the post-merge IndexedDB: `scratch.py` (never committed) survives untouched, `starter.py` @@ -88,6 +89,11 @@ have to rediscover them: `starter_mine.py`, `keep.py` silently took the upstream version with no `keep_mine.py` sibling, `gone.py` is gone, and `coach.py` took the repo's version with **no** `coach_mine.py` — protection overwrites, never rescues. + **Open-tab cleanup:** once Pybricks has reopened its remembered tabs, asserts + no "file with uuid '…' not found" toast was shown (toasts are recorded from + page load, since they auto-dismiss after 5s), that `gone.py` left the + sessionStorage tab history, and that `keep.py` is still in it. (Verified by + disabling the prune in `content.js`: the toast assertion fails.) 9. **Deletion freshness.** Pushes another competing `keep.py`, deletes `keep.py` from IndexedDB (`apply-files` with it withheld), real-clicks **Commit** → asserts the push succeeded, the bare repo still holds the teammate's @@ -138,6 +144,9 @@ driver's own comments use for it: [e2e] PASS: exactly one keep.py in the editor [e2e] PASS: no keep_mine.py sibling for the untouched file [e2e] PASS: untouched gone.py went away with the upstream deletion +[e2e] PASS: no "file … not found" toast after the reload ([]) +[e2e] PASS: the deleted gone.py left Pybricks' open-tab history +[e2e] PASS: the kept keep.py is still a remembered tab [e2e] PASS: the protected coach.py took the repo's version despite the local edit [e2e] PASS: a protected file is overwritten with no rescue copy diff --git a/test/e2e/drive.mjs b/test/e2e/drive.mjs index e5510fc..ec36f6c 100644 --- a/test/e2e/drive.mjs +++ b/test/e2e/drive.mjs @@ -652,6 +652,46 @@ async function main() { ); log('pushed competing change to the bare repo'); + // Open gone.py (deleted upstream below) and keep.py (kept) in editor + // tabs, the way the Explorer does. Pybricks remembers open tabs in + // sessionStorage and reopens them after the reload; a stale uuid for + // gone.py would raise its "unexpected error … not found" toast. That + // toast auto-dismisses after 5s, so record every toast from load on. + const evalMain = async (expression) => { + const r = await page.send('Runtime.evaluate', { expression, awaitPromise: true, returnByValue: true }); + if (r.exceptionDetails) { + throw new Error('main eval threw: ' + (r.exceptionDetails.exception?.description || r.exceptionDetails.text)); + } + return r.result.value; + }; + await page.send('Page.addScriptToEvaluateOnNewDocument', { source: ` + window.__toasts = []; + new MutationObserver(() => { + for (const t of document.querySelectorAll('.bp5-toast')) { + if (!window.__toasts.includes(t.textContent)) window.__toasts.push(t.textContent); + } + }).observe(document, { childList: true, subtree: true, characterData: true }); + ` }); + const tabMeta = (await evalIsolated(`pageRequest('list-files')`)).metadata; + const goneUuid = tabMeta.find((m) => m.path === 'gone.py').uuid; + const keepUuid = tabMeta.find((m) => m.path === 'keep.py').uuid; + for (const uuid of [keepUuid, goneUuid]) { + await evalMain(`findAppStore().dispatch({ type: 'editor.action.activateFile', uuid: ${JSON.stringify(uuid)} })`); + await poll( + () => evalMain(`findAppStore().getState().editor.openFileUuids.includes(${JSON.stringify(uuid)})`), + { timeout: 15000, what: 'a file to open in an editor tab' }, + ); + } + const tabHistory = () => + evalMain( + `Object.keys(sessionStorage).filter((k) => k.startsWith('editor.activeFileHistory.')).flatMap((k) => JSON.parse(sessionStorage.getItem(k)))`, + ); + const historyBefore = await tabHistory(); + assert( + historyBefore.includes(goneUuid) && historyBefore.includes(keepUuid), + 'gone.py and keep.py are open editor tabs before the Pull', + ); + await trustedClick(await buttonRect('Pull')); const mergeLabel = await poll( async () => { @@ -703,6 +743,25 @@ async function main() { timeout: 40000, what: 'buttons to remount after the merge reload', }); + // Pybricks reopens remembered tabs as the editor mounts; give it time + // to try (and to toast, if a stale uuid were still there). + await poll(() => evalMain(`findAppStore()?.getState().editor.openFileUuids.length > 0`), { + timeout: 20000, + what: 'Pybricks to reopen the remembered tabs', + }); + await sleep(2000); + // The toast is the decisive check: once Pybricks reopens keep.py it + // rewrites the history from memory, which drops a stale gone.py uuid + // anyway — but only AFTER the failed reopen has already toasted. + // (Verified: with the prune disabled, only this assertion fails.) + const staleToasts = (await evalMain(`window.__toasts`)).filter((t) => /not found/.test(t)); + assert( + staleToasts.length === 0, + `no "file … not found" toast after the reload (${JSON.stringify(staleToasts)})`, + ); + const historyAfter = await tabHistory(); + assert(!historyAfter.includes(goneUuid), "the deleted gone.py left Pybricks' open-tab history"); + assert(historyAfter.includes(keepUuid), 'the kept keep.py is still a remembered tab'); const afterMerge = await evalIsolated(`pageRequest('list-files')`); const mergedPaths = afterMerge.contents.map((c) => c.path); const mergedBy = new Map(afterMerge.contents.map((c) => [c.path, c.contents])); diff --git a/test/load-pullmerge.mjs b/test/load-pullmerge.mjs index 9f4297c..46c2778 100644 --- a/test/load-pullmerge.mjs +++ b/test/load-pullmerge.mjs @@ -12,7 +12,7 @@ const srcPath = path.join(here, '..', 'src', 'pullmerge.js'); export function loadPullMerge() { const src = readFileSync(srcPath, 'utf8') + - '\n;globalThis.__pybricksPullMergeTest = { rescueName, planPull };'; + '\n;globalThis.__pybricksPullMergeTest = { rescueName, planPull, deletedUuids, pruneTabHistory, pruneOpenTabs };'; // eslint-disable-next-line no-new-func new Function(src)(); return globalThis.__pybricksPullMergeTest; diff --git a/test/pullmerge.test.mjs b/test/pullmerge.test.mjs index cbf9536..9bb5866 100644 --- a/test/pullmerge.test.mjs +++ b/test/pullmerge.test.mjs @@ -2,7 +2,7 @@ import { describe, test } from 'node:test'; import assert from 'node:assert/strict'; import { loadPullMerge } from './load-pullmerge.mjs'; -const { rescueName, planPull } = loadPullMerge(); +const { rescueName, planPull, deletedUuids, pruneTabHistory, pruneOpenTabs } = loadPullMerge(); describe('rescueName', () => { test('inserts _mine before the extension', () => { @@ -192,3 +192,62 @@ describe('planPull', () => { assert.deepEqual(rescued, []); }); }); + +describe('open-tab cleanup', () => { + // A minimal Web Storage stand-in (key/getItem/setItem/length). + function fakeStorage(entries) { + const m = new Map(Object.entries(entries)); + return { + get length() { + return m.size; + }, + key: (i) => [...m.keys()][i] ?? null, + getItem: (k) => (m.has(k) ? m.get(k) : null), + setItem: (k, v) => m.set(k, String(v)), + dump: () => Object.fromEntries(m), + }; + } + + test('deletedUuids names the files missing from the kept set', () => { + const meta = [ + { path: 'a.py', uuid: 'u-a' }, + { path: 'gone.py', uuid: 'u-gone' }, + { path: 'b.py', uuid: 'u-b' }, + ]; + assert.deepEqual(deletedUuids(meta, ['a.py', 'b.py', 'new.py']), ['u-gone']); + assert.deepEqual(deletedUuids(meta, ['a.py', 'gone.py', 'b.py']), []); + }); + + test('pruneTabHistory drops deleted uuids and keeps order', () => { + assert.equal(pruneTabHistory('["u-a","u-gone","u-b"]', ['u-gone']), '["u-a","u-b"]'); + assert.equal(pruneTabHistory('["u-gone"]', ['u-gone']), '[]'); + }); + + test('pruneTabHistory returns null when nothing changes or the value is not an array', () => { + assert.equal(pruneTabHistory('["u-a"]', ['u-gone']), null); + assert.equal(pruneTabHistory('not json', ['u-gone']), null); + assert.equal(pruneTabHistory('{"u-gone":1}', ['u-gone']), null); + assert.equal(pruneTabHistory(null, ['u-gone']), null); + }); + + test('pruneOpenTabs rewrites only open-tab history keys that change', () => { + const storage = fakeStorage({ + 'activities.selectedActivity': '"activity.explorer"', + 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:1': '["u-a","u-gone"]', + 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:2': '["u-b"]', + 'other.key': '["u-gone"]', + }); + assert.equal(pruneOpenTabs(storage, ['u-gone']), 1); + assert.deepEqual(storage.dump(), { + 'activities.selectedActivity': '"activity.explorer"', + 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:1': '["u-a"]', + 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:2': '["u-b"]', + 'other.key': '["u-gone"]', + }); + }); + + test('pruneOpenTabs does nothing with no deleted uuids', () => { + const storage = fakeStorage({ 'editor.activeFileHistory.w.e': '["u-a"]' }); + assert.equal(pruneOpenTabs(storage, []), 0); + }); +}); From acef733856a7e9988b9fd8d52918e8661c60cd11 Mon Sep 17 00:00:00 2001 From: Brendon Thiede Date: Fri, 18 Sep 2026 14:07:06 -0400 Subject: [PATCH 2/2] refactor: Keep sessionStorage I/O out of pullmerge.js CodeRabbit: pullmerge.js is a pure-helper file. pruneOpenTabs(storage) becomes planTabPrunes(entries, uuids) -> [[key, newValue]]; content.js reads the entries and writes the planned values. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- src/content.js | 5 ++++- src/pullmerge.js | 30 +++++++++++++-------------- test/load-pullmerge.mjs | 2 +- test/pullmerge.test.mjs | 45 ++++++++++++----------------------------- 5 files changed, 33 insertions(+), 51 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f650902..2e70c03 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ Everything runs in the browser; the only external party is `github.com`: **The ISOLATED world is six classic scripts**, listed in `manifest.json` `content_scripts` in load order: `menu-config.js` (pure parse/generate/analyze helpers, no DOM), `blocksplice.js` (pure block-file setup splicing, no DOM — phase 4), `pullmerge.js` (pure Pull-merge planning, no DOM — see "Pull merges, it does not clobber" below), then `menu-panel.js` (`makeMenuPanel`) and `file-list.js` (`makeFileListWatcher`) which depend on those helpers being in scope, then `content.js` last — it wires the toolbar and constructs the panel/watcher. They share one global scope with no ESM `export`s, so **order matters** — a helper must be defined in an earlier file than its caller. The three pure-helper files have Node test shims (`test/load-menu-config.mjs`, `test/load-blocksplice.mjs`, `test/load-pullmerge.mjs`, same pattern as `load-inject.mjs`) and unit tests; the three DOM-heavy files have no Node loader and are exercised only via the browser E2E path. See the "Menu manager (phase 3)" and "Setup propagation (phase 4)" sections below. -**Pull merges, it does not clobber.** `content.js:pull()` never hands the repo's file set straight to `apply-files` — that op deletes every path it isn't given, which used to destroy uncommitted local work. `src/pullmerge.js:planPull()` (pure, no DOM, unit-tested via `test/load-pullmerge.mjs`) computes the complete desired set from the editor's files, the repo's files, the `lastPullShas` base, and the protected list. Files untouched since the last Pull take the repo's version; locally edited files let the repo keep the canonical path and are rescued to `_mine.py` (a valid module name, so the hub can still import it); files created locally and never committed keep their own name; protected paths **that the repo actually has** are overwritten with no rescue copy. Protection is checked after the never-committed rule on purpose — a manifest can reserve a path the repo doesn't have (the starter reserves `robot_setup.py`, never authored), and with no repo version to restore, "the repo wins" would just delete the file the kid made. Rescues are reported through the `pullRescued` key as a notice on the next load. **Open tabs of deleted files are forgotten.** Pybricks remembers open editor tabs as a JSON array of file uuids in sessionStorage, under `editor.activeFileHistory..` (`ActiveFileHistoryManager`, pybricks-code `src/editor/lib.ts`), and reopens them after a reload. A uuid whose file the Pull deleted raises Pybricks' "unexpected error … file with uuid '…' not found" toast. So `pull()` computes the doomed uuids before `apply-files` (`pullmerge.js:deletedUuids`) and prunes them from every such key (`pruneOpenTabs`) **inside the reload timer, immediately before `location.reload()`**. Earlier would be undone, because the page's in-memory history rewrites the key whenever a tab opens or closes. Design: `docs/superpowers/specs/2026-08-09-pull-merge-design.md`. +**Pull merges, it does not clobber.** `content.js:pull()` never hands the repo's file set straight to `apply-files` — that op deletes every path it isn't given, which used to destroy uncommitted local work. `src/pullmerge.js:planPull()` (pure, no DOM, unit-tested via `test/load-pullmerge.mjs`) computes the complete desired set from the editor's files, the repo's files, the `lastPullShas` base, and the protected list. Files untouched since the last Pull take the repo's version; locally edited files let the repo keep the canonical path and are rescued to `_mine.py` (a valid module name, so the hub can still import it); files created locally and never committed keep their own name; protected paths **that the repo actually has** are overwritten with no rescue copy. Protection is checked after the never-committed rule on purpose — a manifest can reserve a path the repo doesn't have (the starter reserves `robot_setup.py`, never authored), and with no repo version to restore, "the repo wins" would just delete the file the kid made. Rescues are reported through the `pullRescued` key as a notice on the next load. **Open tabs of deleted files are forgotten.** Pybricks remembers open editor tabs as a JSON array of file uuids in sessionStorage, under `editor.activeFileHistory..` (`ActiveFileHistoryManager`, pybricks-code `src/editor/lib.ts`), and reopens them after a reload. A uuid whose file the Pull deleted raises Pybricks' "unexpected error … file with uuid '…' not found" toast. So `pull()` computes the doomed uuids before `apply-files` (`pullmerge.js:deletedUuids`) and prunes them from every such key (`planTabPrunes` plans the rewrites purely; `content.js` does the sessionStorage reads and writes) **inside the reload timer, immediately before `location.reload()`**. Earlier would be undone, because the page's in-memory history rewrites the key whenever a tab opens or closes. Design: `docs/superpowers/specs/2026-08-09-pull-merge-design.md`. **Why an ISOLATED/MAIN split:** The MAIN-world script (`inject.js`) can see the page's globals (and could in principle use the page's Dexie instance, though we don't); the ISOLATED-world scripts have access to `chrome.runtime` APIs so they can message the service worker. The split is mandatory — only the MAIN world reaches the page's IndexedDB, only the ISOLATED world reaches `chrome.runtime`. They communicate via `window.postMessage` with `pybricks-git:request` / `pybricks-git:response` envelopes; `content.js` reaches the service worker via `chrome.runtime.sendMessage`. diff --git a/src/content.js b/src/content.js index fa7d555..04a027c 100644 --- a/src/content.js +++ b/src/content.js @@ -500,7 +500,10 @@ async function pull(btn) { // in-memory tab history rewrites sessionStorage whenever a tab // opens or closes, so an earlier prune could be undone. try { - pruneOpenTabs(sessionStorage, goneUuids); + const entries = Object.keys(sessionStorage).map((k) => [k, sessionStorage.getItem(k)]); + for (const [key, value] of planTabPrunes(entries, goneUuids)) { + sessionStorage.setItem(key, value); + } } catch (err) { // Storage can be unavailable; the cost is only Pybricks' // own "file not found" toast after the reload. diff --git a/src/pullmerge.js b/src/pullmerge.js index 39e93c1..4dc2055 100644 --- a/src/pullmerge.js +++ b/src/pullmerge.js @@ -80,7 +80,8 @@ function planPull({ local, repo, base = {}, protectedPaths = [] }) { // (pybricks-code src/editor/lib.ts ActiveFileHistoryManager), and reopens each // on load. A uuid whose file a Pull deleted fails that reopen with an // "unexpected error" toast ("file with uuid '…' not found"). These helpers -// find the uuids a Pull removes and prune them from that history. +// find the uuids a Pull removes and plan pruning them from that history; +// content.js does the sessionStorage reads and writes. const OPEN_TAB_HISTORY_PREFIX = 'editor.activeFileHistory.'; @@ -109,20 +110,17 @@ function pruneTabHistory(value, uuids) { return pruned.length === history.length ? null : JSON.stringify(pruned); } -// Applies pruneTabHistory to every open-tab history key in `storage` (a Web -// Storage object — sessionStorage in the page). Returns the number of keys -// rewritten. -function pruneOpenTabs(storage, uuids) { - if (!uuids.length) return 0; - let rewritten = 0; - for (let i = 0; i < storage.length; i++) { - const key = storage.key(i); - if (!key || !key.startsWith(OPEN_TAB_HISTORY_PREFIX)) continue; - const next = pruneTabHistory(storage.getItem(key), uuids); - if (next !== null) { - storage.setItem(key, next); - rewritten++; - } +// Plans the rewrites for every open-tab history entry. `entries` is +// [[key, value]] as read from sessionStorage by the caller (content.js does +// the storage I/O; this stays pure). Returns [[key, newValue]] for just the +// keys that change. +function planTabPrunes(entries, uuids) { + if (!uuids.length) return []; + const writes = []; + for (const [key, value] of entries) { + if (typeof key !== 'string' || !key.startsWith(OPEN_TAB_HISTORY_PREFIX)) continue; + const next = pruneTabHistory(value, uuids); + if (next !== null) writes.push([key, next]); } - return rewritten; + return writes; } diff --git a/test/load-pullmerge.mjs b/test/load-pullmerge.mjs index 46c2778..05c67db 100644 --- a/test/load-pullmerge.mjs +++ b/test/load-pullmerge.mjs @@ -12,7 +12,7 @@ const srcPath = path.join(here, '..', 'src', 'pullmerge.js'); export function loadPullMerge() { const src = readFileSync(srcPath, 'utf8') + - '\n;globalThis.__pybricksPullMergeTest = { rescueName, planPull, deletedUuids, pruneTabHistory, pruneOpenTabs };'; + '\n;globalThis.__pybricksPullMergeTest = { rescueName, planPull, deletedUuids, pruneTabHistory, planTabPrunes };'; // eslint-disable-next-line no-new-func new Function(src)(); return globalThis.__pybricksPullMergeTest; diff --git a/test/pullmerge.test.mjs b/test/pullmerge.test.mjs index 9bb5866..f8e7ad2 100644 --- a/test/pullmerge.test.mjs +++ b/test/pullmerge.test.mjs @@ -2,7 +2,7 @@ import { describe, test } from 'node:test'; import assert from 'node:assert/strict'; import { loadPullMerge } from './load-pullmerge.mjs'; -const { rescueName, planPull, deletedUuids, pruneTabHistory, pruneOpenTabs } = loadPullMerge(); +const { rescueName, planPull, deletedUuids, pruneTabHistory, planTabPrunes } = loadPullMerge(); describe('rescueName', () => { test('inserts _mine before the extension', () => { @@ -194,20 +194,6 @@ describe('planPull', () => { }); describe('open-tab cleanup', () => { - // A minimal Web Storage stand-in (key/getItem/setItem/length). - function fakeStorage(entries) { - const m = new Map(Object.entries(entries)); - return { - get length() { - return m.size; - }, - key: (i) => [...m.keys()][i] ?? null, - getItem: (k) => (m.has(k) ? m.get(k) : null), - setItem: (k, v) => m.set(k, String(v)), - dump: () => Object.fromEntries(m), - }; - } - test('deletedUuids names the files missing from the kept set', () => { const meta = [ { path: 'a.py', uuid: 'u-a' }, @@ -230,24 +216,19 @@ describe('open-tab cleanup', () => { assert.equal(pruneTabHistory(null, ['u-gone']), null); }); - test('pruneOpenTabs rewrites only open-tab history keys that change', () => { - const storage = fakeStorage({ - 'activities.selectedActivity': '"activity.explorer"', - 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:1': '["u-a","u-gone"]', - 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:2': '["u-b"]', - 'other.key': '["u-gone"]', - }); - assert.equal(pruneOpenTabs(storage, ['u-gone']), 1); - assert.deepEqual(storage.dump(), { - 'activities.selectedActivity': '"activity.explorer"', - 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:1': '["u-a"]', - 'editor.activeFileHistory.win1.vs.editor.ICodeEditor:2': '["u-b"]', - 'other.key': '["u-gone"]', - }); + test('planTabPrunes rewrites only open-tab history keys that change', () => { + const entries = [ + ['activities.selectedActivity', '"activity.explorer"'], + ['editor.activeFileHistory.win1.vs.editor.ICodeEditor:1', '["u-a","u-gone"]'], + ['editor.activeFileHistory.win1.vs.editor.ICodeEditor:2', '["u-b"]'], + ['other.key', '["u-gone"]'], + ]; + assert.deepEqual(planTabPrunes(entries, ['u-gone']), [ + ['editor.activeFileHistory.win1.vs.editor.ICodeEditor:1', '["u-a"]'], + ]); }); - test('pruneOpenTabs does nothing with no deleted uuids', () => { - const storage = fakeStorage({ 'editor.activeFileHistory.w.e': '["u-a"]' }); - assert.equal(pruneOpenTabs(storage, []), 0); + test('planTabPrunes plans nothing with no deleted uuids', () => { + assert.deepEqual(planTabPrunes([['editor.activeFileHistory.w.e', '["u-a"]']], []), []); }); });