The popup's saveState() and the background's updateState() each read the stored record, change it and write it back, each ordered only within its own context. So the background's write of a network the user approved could land between a popup save's read and write, and the popup then wrote the old network back (#641).
Both now hold the browser's lock (navigator.locks) from the read to the write, through withLock() in src/shared/browserApi.js. The StateRecovery reset, the only other writer of the record, takes it too. The lock grants requests in the order made, so it also orders each context's own writes, and both per-context queues are gone.
Jest's node has no navigator.locks; tests/support/navigatorLocks.js supplies a small one through setupFiles in package.json. Seven view tests that replaced navigator for the clipboard now set only navigator.clipboard, so the lock stays.
Deviation: goHome() in tests/e2e/run.js now waits for its navigation's save, since two Chrome tests read the stored stack right after it and a save under the lock lands a step later.
Judgement call: the reset taking the lock goes past the plan's two functions, so every write of the record holds it.
Not covered: a page in a Firefox private window gets locks of its own, so its save can still undo the background's (#646).
Model: opus-5-5
The popup's `saveState()` and the background's `updateState()` each read the stored record, change it and write it back, each ordered only within its own context. So the background's write of a network the user approved could land between a popup save's read and write, and the popup then wrote the old network back (https://git.eeqj.de/sneak/AutistMask/issues/641).
Both now hold the browser's lock (`navigator.locks`) from the read to the write, through `withLock()` in `src/shared/browserApi.js`. The StateRecovery reset, the only other writer of the record, takes it too. The lock grants requests in the order made, so it also orders each context's own writes, and both per-context queues are gone.
Jest's node has no `navigator.locks`; `tests/support/navigatorLocks.js` supplies a small one through `setupFiles` in `package.json`. Seven view tests that replaced `navigator` for the clipboard now set only `navigator.clipboard`, so the lock stays.
- Deviation: `goHome()` in `tests/e2e/run.js` now waits for its navigation's save, since two Chrome tests read the stored stack right after it and a save under the lock lands a step later.
- Judgement call: the reset taking the lock goes past the plan's two functions, so every write of the record holds it.
- Not covered: a page in a Firefox private window gets locks of its own, so its save can still undo the background's (https://git.eeqj.de/sneak/AutistMask/issues/646).
Model: opus-5-5
Firefox private windows are not covered.withLock() in src/shared/browserApi.js; README "Stored state and its version". Firefox gives extension pages in a private window their own locks: while the background page holds a lock, an extension page in a private window is granted the same lock at once. manifest/firefox.json lets the user allow the extension in private windows, so a popup opened there can still undo the background's network switch exactly as #641 describes, while README says every page of the extension and its background share the lock. Acceptable: no page in a Firefox private window can write between the background's read and write (by ordering those saves as well, or by not allowing the extension in private windows if the owner chooses that on the issue), and README states exactly which pages share the lock.
The second new test passes without the fix.tests/stateWriteLock.test.js, "a save that fails holding the lock lets it go": it passes on next, which has no lock, and the only code in it that lets a lock go is the test's own stub in tests/support/navigatorLocks.js. Acceptable: a check that fails on next, for example a rejected save followed by a save that still keeps a background network switch that lands between its read and its write.
The header of src/shared/browserApi.js does not cover withLock(). The header describes the module as the one place that names browser or chrome, there to make the two extension namespaces behave alike; withLock() wraps the browser's navigator.locks, which is neither. Acceptable: the header says the module also holds this wrapper.
A changed comment gives the wrong cause.src/shared/state.js, the comment above saveFailureHandler: with the queue clause removed, it now says a failed save was swallowed because showView() does not await saveState(). It was the removed queue's rejection handler that swallowed it; a rejection nobody handles is reported by the browser. Acceptable: a sentence that matches the code, or the history removed.
Finding 1 was seen with an extension page in a private-window tab; the toolbar popup itself cannot be driven by the test browser and is taken to behave the same.
Judgement call: the goHome() wait in tests/e2e/run.js fixes a test race (without it the two tests read the stack one save behind); a save landing one step later is nothing a user sees.
Model: opus-5-5
**Review: FAIL (`needs-rework`)**
1. **Firefox private windows are not covered.** `withLock()` in `src/shared/browserApi.js`; README "Stored state and its version". Firefox gives extension pages in a private window their own locks: while the background page holds a lock, an extension page in a private window is granted the same lock at once. `manifest/firefox.json` lets the user allow the extension in private windows, so a popup opened there can still undo the background's network switch exactly as https://git.eeqj.de/sneak/AutistMask/issues/641 describes, while README says every page of the extension and its background share the lock. Acceptable: no page in a Firefox private window can write between the background's read and write (by ordering those saves as well, or by not allowing the extension in private windows if the owner chooses that on the issue), and README states exactly which pages share the lock.
2. **The second new test passes without the fix.** `tests/stateWriteLock.test.js`, "a save that fails holding the lock lets it go": it passes on `next`, which has no lock, and the only code in it that lets a lock go is the test's own stub in `tests/support/navigatorLocks.js`. Acceptable: a check that fails on `next`, for example a rejected save followed by a save that still keeps a background network switch that lands between its read and its write.
3. **The header of `src/shared/browserApi.js` does not cover `withLock()`.** The header describes the module as the one place that names `browser` or `chrome`, there to make the two extension namespaces behave alike; `withLock()` wraps the browser's `navigator.locks`, which is neither. Acceptable: the header says the module also holds this wrapper.
4. **A changed comment gives the wrong cause.** `src/shared/state.js`, the comment above `saveFailureHandler`: with the queue clause removed, it now says a failed save was swallowed because `showView()` does not await `saveState()`. It was the removed queue's rejection handler that swallowed it; a rejection nobody handles is reported by the browser. Acceptable: a sentence that matches the code, or the history removed.
- Finding 1 was seen with an extension page in a private-window tab; the toolbar popup itself cannot be driven by the test browser and is taken to behave the same.
- Judgement call: the `goHome()` wait in `tests/e2e/run.js` fixes a test race (without it the two tests read the stack one save behind); a save landing one step later is nothing a user sees.
Model: opus-5-5
README "Stored state and its version" now names the one exception to who shares the lock, a page in a Firefox private window, linking #646; the PR body discloses it. Manifests and the save path are unchanged.
Replaced: a save whose write is refused, then a save with the background's switch to mainnet landing between its read and its write, which must leave mainnet stored. It fails on next.
The header now says the module also holds withLock(), the wrapper around navigator.locks.
The comment now gives the reason that holds in the code as it is (a failure of an unawaited or unguarded save would reach only the console) and drops the history.
Judgement call: the doc comment on withLock() made the same claim as README, so it names the same private-window exception.
The lock comments in src/shared/state.js and src/background/state.js now name the same exception; the latter states the claim twice (its header and the comment above updateState()), so both do.
Model: opus-5-5
1. README "Stored state and its version" now names the one exception to who shares the lock, a page in a Firefox private window, linking https://git.eeqj.de/sneak/AutistMask/issues/646; the PR body discloses it. Manifests and the save path are unchanged.
2. Replaced: a save whose write is refused, then a save with the background's switch to mainnet landing between its read and its write, which must leave mainnet stored. It fails on `next`.
3. The header now says the module also holds `withLock()`, the wrapper around `navigator.locks`.
4. The comment now gives the reason that holds in the code as it is (a failure of an unawaited or unguarded save would reach only the console) and drops the history.
- Judgement call: the doc comment on `withLock()` made the same claim as README, so it names the same private-window exception.
- The lock comments in `src/shared/state.js` and `src/background/state.js` now name the same exception; the latter states the claim twice (its header and the comment above `updateState()`), so both do.
Model: opus-5-5
Two places still say the removed save queue swallowed a failed save. The comment at the top of init() in src/popup/index.js ("rather than being swallowed by the save queue"), and the test name "is reported, not swallowed by the save queue" in tests/persistedEntryFloors.test.js. This PR removes that queue, so both now give the cause that round-one finding 4 took out of src/shared/state.js. Acceptable: wording that matches the code as it is now, as the comment above saveFailureHandler does (without the reporter, a failure would reach only the console).
Two comments this PR adds still say the lock covers every page, with no exception. The header of tests/support/navigatorLocks.js ("as every page of the extension and its background share the browser's"), and the comment in resetProfile() in src/popup/views/stateRecovery.js ("the erase cannot land between another write's read and its write"). A page in a Firefox private window does not share the lock, as README and the other lock comments now say. Acceptable: these two name the same exception, or drop the claim.
The PR body is over about 250 words. Acceptable: under that, for example by dropping the browser-version paragraph, which repeats README.
Model: opus-5-5
**Review: FAIL (`needs-rework`)**
1. **Two places still say the removed save queue swallowed a failed save.** The comment at the top of `init()` in `src/popup/index.js` ("rather than being swallowed by the save queue"), and the test name "is reported, not swallowed by the save queue" in `tests/persistedEntryFloors.test.js`. This PR removes that queue, so both now give the cause that round-one finding 4 took out of `src/shared/state.js`. Acceptable: wording that matches the code as it is now, as the comment above `saveFailureHandler` does (without the reporter, a failure would reach only the console).
2. **Two comments this PR adds still say the lock covers every page, with no exception.** The header of `tests/support/navigatorLocks.js` ("as every page of the extension and its background share the browser's"), and the comment in `resetProfile()` in `src/popup/views/stateRecovery.js` ("the erase cannot land between another write's read and its write"). A page in a Firefox private window does not share the lock, as README and the other lock comments now say. Acceptable: these two name the same exception, or drop the claim.
3. **The PR body is over about 250 words.** Acceptable: under that, for example by dropping the browser-version paragraph, which repeats README.
Model: opus-5-5
The popup's saveState() and the background's updateState() each read the
stored record, change it and write it back, each ordered only within its own
context, so a write by one could land between the other's read and write and
be lost. Both now hold the browser's lock (navigator.locks) from the read to
the write, through withLock() in src/shared/browserApi.js, and the recovery
screen's reset takes it too. A page in a Firefox private window gets locks of
its own and is not covered. The lock also orders a page's own saves, so both
per-context queues are gone. The unit tests get a small lock from a Jest setup
file.
Model: opus-5-5
init() in src/popup/index.js now says a failed save would otherwise reach only the console; the test in tests/persistedEntryFloors.test.js is now "is reported when the caller does not await it".
The header of tests/support/navigatorLocks.js and the comment in resetProfile() in src/popup/views/stateRecovery.js now name the Firefox private-window exception.
The PR body is under 250 words; the browser-version paragraph is gone.
Also fixed: the comment above baseline = current in saveStateOnce() (src/shared/state.js) and the approval-window test's comment in tests/stateMerge.test.js, which still named the queue; the header of tests/stateWriteLock.test.js, which said every page shares the lock; and the commit message.
Judgement call: left the dated Completed Steps entries in TODO.md that mention the queues (they record what was true then) and the queued variables in tests/stateMerge.test.js (code; such a save still waits its turn).
Model: opus-5-5
1. `init()` in `src/popup/index.js` now says a failed save would otherwise reach only the console; the test in `tests/persistedEntryFloors.test.js` is now "is reported when the caller does not await it".
2. The header of `tests/support/navigatorLocks.js` and the comment in `resetProfile()` in `src/popup/views/stateRecovery.js` now name the Firefox private-window exception.
3. The PR body is under 250 words; the browser-version paragraph is gone.
- Also fixed: the comment above `baseline = current` in `saveStateOnce()` (`src/shared/state.js`) and the approval-window test's comment in `tests/stateMerge.test.js`, which still named the queue; the header of `tests/stateWriteLock.test.js`, which said every page shares the lock; and the commit message.
- Judgement call: left the dated Completed Steps entries in `TODO.md` that mention the queues (they record what was true then) and the `queued` variables in `tests/stateMerge.test.js` (code; such a save still waits its turn).
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The popup's
saveState()and the background'supdateState()each read the stored record, change it and write it back, each ordered only within its own context. So the background's write of a network the user approved could land between a popup save's read and write, and the popup then wrote the old network back (#641).Both now hold the browser's lock (
navigator.locks) from the read to the write, throughwithLock()insrc/shared/browserApi.js. The StateRecovery reset, the only other writer of the record, takes it too. The lock grants requests in the order made, so it also orders each context's own writes, and both per-context queues are gone.Jest's node has no
navigator.locks;tests/support/navigatorLocks.jssupplies a small one throughsetupFilesinpackage.json. Seven view tests that replacednavigatorfor the clipboard now set onlynavigator.clipboard, so the lock stays.goHome()intests/e2e/run.jsnow waits for its navigation's save, since two Chrome tests read the stored stack right after it and a save under the lock lands a step later.Model: opus-5-5
Review: FAIL (
needs-rework)Firefox private windows are not covered.
withLock()insrc/shared/browserApi.js; README "Stored state and its version". Firefox gives extension pages in a private window their own locks: while the background page holds a lock, an extension page in a private window is granted the same lock at once.manifest/firefox.jsonlets the user allow the extension in private windows, so a popup opened there can still undo the background's network switch exactly as #641 describes, while README says every page of the extension and its background share the lock. Acceptable: no page in a Firefox private window can write between the background's read and write (by ordering those saves as well, or by not allowing the extension in private windows if the owner chooses that on the issue), and README states exactly which pages share the lock.The second new test passes without the fix.
tests/stateWriteLock.test.js, "a save that fails holding the lock lets it go": it passes onnext, which has no lock, and the only code in it that lets a lock go is the test's own stub intests/support/navigatorLocks.js. Acceptable: a check that fails onnext, for example a rejected save followed by a save that still keeps a background network switch that lands between its read and its write.The header of
src/shared/browserApi.jsdoes not coverwithLock(). The header describes the module as the one place that namesbrowserorchrome, there to make the two extension namespaces behave alike;withLock()wraps the browser'snavigator.locks, which is neither. Acceptable: the header says the module also holds this wrapper.A changed comment gives the wrong cause.
src/shared/state.js, the comment abovesaveFailureHandler: with the queue clause removed, it now says a failed save was swallowed becauseshowView()does not awaitsaveState(). It was the removed queue's rejection handler that swallowed it; a rejection nobody handles is reported by the browser. Acceptable: a sentence that matches the code, or the history removed.goHome()wait intests/e2e/run.jsfixes a test race (without it the two tests read the stack one save behind); a save landing one step later is nothing a user sees.Model: opus-5-5
6a1fb7da39to195e2b3e1cnext.withLock(), the wrapper aroundnavigator.locks.withLock()made the same claim as README, so it names the same private-window exception.src/shared/state.jsandsrc/background/state.jsnow name the same exception; the latter states the claim twice (its header and the comment aboveupdateState()), so both do.Model: opus-5-5
195e2b3e1ctodb0d81e78eReview: FAIL (
needs-rework)init()insrc/popup/index.js("rather than being swallowed by the save queue"), and the test name "is reported, not swallowed by the save queue" intests/persistedEntryFloors.test.js. This PR removes that queue, so both now give the cause that round-one finding 4 took out ofsrc/shared/state.js. Acceptable: wording that matches the code as it is now, as the comment abovesaveFailureHandlerdoes (without the reporter, a failure would reach only the console).tests/support/navigatorLocks.js("as every page of the extension and its background share the browser's"), and the comment inresetProfile()insrc/popup/views/stateRecovery.js("the erase cannot land between another write's read and its write"). A page in a Firefox private window does not share the lock, as README and the other lock comments now say. Acceptable: these two name the same exception, or drop the claim.Model: opus-5-5
db0d81e78eto9b6c4bd399init()insrc/popup/index.jsnow says a failed save would otherwise reach only the console; the test intests/persistedEntryFloors.test.jsis now "is reported when the caller does not await it".tests/support/navigatorLocks.jsand the comment inresetProfile()insrc/popup/views/stateRecovery.jsnow name the Firefox private-window exception.baseline = currentinsaveStateOnce()(src/shared/state.js) and the approval-window test's comment intests/stateMerge.test.js, which still named the queue; the header oftests/stateWriteLock.test.js, which said every page shares the lock; and the commit message.TODO.mdthat mention the queues (they record what was true then) and thequeuedvariables intests/stateMerge.test.js(code; such a save still waits its turn).Model: opus-5-5
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.