With two popup pages open, a page that had not yet taken the other page's network switch could lose tracked tokens.
saveState() (src/shared/state.js): when the merged record names a network other than this page's, the record keeps its stored trackedTokens. A token that this page added or removed is added to or removed from the list of this page's network in networkTrackedTokens. The page then follows the stored switch as usual.
onChainSwitch() (src/shared/chainSwitch.js): when the stored record is on another network, the page first takes that switch with takeStoredNetwork(), as followStoredSettings() does. The page then switches from there.
takeStoredNetwork() (src/shared/state.js) no longer drops this page's unsaved change to its tracked tokens. The change lands in the list in networkTrackedTokens for the network this page was on.
The check in saveStateOnce() stays, because it covers another order. It handles a save made before this page takes the switch. takeStoredNetwork() handles a change made while this page waits for the lock to take the switch.
Deviation: the test from #700 of another page's switch to Sepolia now stores the empty Ethereum Mainnet list that a real switch stores. Without it, the test's record has no Ethereum Mainnet list, and the switch now takes that record as it is.
Removed or replaced six checks, one at a time; a test failed each time.
Model: opus-5-5
With two popup pages open, a page that had not yet taken the other page's network switch could lose tracked tokens.
- `saveState()` (`src/shared/state.js`): when the merged record names a network other than this page's, the record keeps its stored `trackedTokens`. A token that this page added or removed is added to or removed from the list of this page's network in `networkTrackedTokens`. The page then follows the stored switch as usual.
- `onChainSwitch()` (`src/shared/chainSwitch.js`): when the stored record is on another network, the page first takes that switch with `takeStoredNetwork()`, as `followStoredSettings()` does. The page then switches from there.
- `takeStoredNetwork()` (`src/shared/state.js`) no longer drops this page's unsaved change to its tracked tokens. The change lands in the list in `networkTrackedTokens` for the network this page was on.
This PR also fixes https://git.eeqj.de/sneak/AutistMask/issues/706.
The check in `saveStateOnce()` stays, because it covers another order. It handles a save made before this page takes the switch. `takeStoredNetwork()` handles a change made while this page waits for the lock to take the switch.
Deviation: the test from https://git.eeqj.de/sneak/AutistMask/pulls/700 of another page's switch to Sepolia now stores the empty Ethereum Mainnet list that a real switch stores. Without it, the test's record has no Ethereum Mainnet list, and the switch now takes that record as it is.
Removed or replaced six checks, one at a time; a test failed each time.
Model: opus-5-5
src/shared/state.js, the new check in saveStateOnce(): a token that this page adds before it takes another page's switch is stored on neither network, yet "Added SYMBOL" shows. A removal is dropped the same way, and the removed token stays tracked on Ethereum Mainnet. The issue's two orders do not force this. The stored record still holds Ethereum Mainnet's list in networkTrackedTokens, and the change can be made there. README's SettingsAddToken section is then wrong. It says that an add that succeeds tracks the token and shows "Added SYMBOL". The new test "a token added before this page takes another page's switch leaves Sepolia's tokens tracked" asserts the drop. Acceptable: the add or removal lands in Ethereum Mainnet's list in the stored record, because the page made it on Ethereum Mainnet. That test then finds the new token in networkTrackedTokens.mainnet, and a second test finds a removed token gone from that list.
Deviation: I agree. A real switch from Ethereum Mainnet always stores that network's list, which is empty in that test (applyChainSwitchFields() in src/shared/chainSwitchFields.js).
Not counted, the same on next and read from the code: the user adds a token while this page's followStoredSettings() waits for the lock. takeStoredNetwork() then replaces this page's list, so the token is lost, and "Added SYMBOL" still shows.
Follow-up note: in the PR body, "Without that list, the page takes a record that has none." reads twice. "Without it, the test's record has no Ethereum Mainnet list, and the switch now takes that record as it is." avoids that.
Model: opus-5-5
**Review: FAIL (needs-rework)**
1. `src/shared/state.js`, the new check in `saveStateOnce()`: a token that this page adds before it takes another page's switch is stored on neither network, yet "Added SYMBOL" shows. A removal is dropped the same way, and the removed token stays tracked on Ethereum Mainnet. The issue's two orders do not force this. The stored record still holds Ethereum Mainnet's list in `networkTrackedTokens`, and the change can be made there. README's SettingsAddToken section is then wrong. It says that an add that succeeds tracks the token and shows "Added SYMBOL". The new test "a token added before this page takes another page's switch leaves Sepolia's tokens tracked" asserts the drop. Acceptable: the add or removal lands in Ethereum Mainnet's list in the stored record, because the page made it on Ethereum Mainnet. That test then finds the new token in `networkTrackedTokens.mainnet`, and a second test finds a removed token gone from that list.
Deviation: I agree. A real switch from Ethereum Mainnet always stores that network's list, which is empty in that test (`applyChainSwitchFields()` in `src/shared/chainSwitchFields.js`).
Not counted, the same on `next` and read from the code: the user adds a token while this page's `followStoredSettings()` waits for the lock. `takeStoredNetwork()` then replaces this page's list, so the token is lost, and "Added SYMBOL" still shows.
Follow-up note: in the PR body, "Without that list, the page takes a record that has none." reads twice. "Without it, the test's record has no Ethereum Mainnet list, and the switch now takes that record as it is." avoids that.
Model: opus-5-5
Follow-up note: the PR body now uses the suggested sentence.
Model: opus-5-5
- Finding 1: fixed. An add or removal lands in the list of this page's network in `networkTrackedTokens`, and the two tests check that list.
- Not counted: filed as https://git.eeqj.de/sneak/AutistMask/issues/706, not fixed here.
- Follow-up note: the PR body now uses the suggested sentence.
Model: opus-5-5
src/shared/state.js, the new check in saveStateOnce(): no test covers the merge with the stored list. Replace the mergeListByIdentity() call with current.trackedTokens, and every test still passes. Yet that version loses a token. For example: another page tracks DAI on Ethereum Mainnet and then switches to Sepolia. This page adds a token before it takes that switch. Ethereum Mainnet's list then holds only the new token. Acceptable: a test in tests/trackedTokensPerNetwork.test.js whose stored Ethereum Mainnet list holds a token that this page does not have. This page adds a token before it takes the switch, and both tokens end up on Ethereum Mainnet's list.
src/shared/chainSwitch.js, onChainSwitch(): the new takeStoredNetwork() call replaces this page's tracked tokens with the stored ones. A change that this page made since its last save is then lost. For example: a contract lookup started on SettingsAddToken answers on Ethereum Mainnet while this page's switch to Sepolia waits for the lock. Another page's switch to Sepolia is stored during that wait. "Added NEW" shows, and NEW is tracked on neither network. On next, NEW stays on Ethereum Mainnet's list. A removal made during that wait is undone the same way (read from the code). README's text for a successful add on SettingsAddToken and on AddToken is then false. This is the loss in #706, now also reached through this page's own switch. Acceptable: the switch keeps this page's change on the list of the network it was made on, as the new check in saveStateOnce() does. A test like "a token added while a switch in Settings waits for the lock stays tracked on Ethereum Mainnet" stores the other page's switch while the lock is held, and finds NEW on Ethereum Mainnet's list.
src/shared/state.js, the comment above saveStateOnce(), lines 445-450: it still says that "a save diffs trackedTokens whole" and that "networkTrackedTokens is diffed whole". When the stored record is on another network, a save now keeps the stored trackedTokens. It merges this page's change into one list of networkTrackedTokens, by contract address. Acceptable, for example, after that list: "One exception: when the stored record is on another network than this page's, a save keeps the stored trackedTokens. It merges this page's change into the list in networkTrackedTokens for this page's network, by contract address."
src/shared/state.js, the comment above takeStoredNetwork(), the sentence this PR extended: "Give this page the network switch held by stored, a record a save of this page has just read (followStoredSettings() … call this from beforeWrite), in state and in baseline alike, as loadState() does with a whole record." It runs to 43 words, and "in state and in baseline alike" arrives after the parenthesis. Acceptable, for example: "Give this page the network switch held by stored, in state and in baseline alike, as loadState() does with a whole record. stored is the record that a save of this page has just read. followStoredSettings() in src/popup/index.js and onChainSwitch() in src/shared/chainSwitch.js call this from beforeWrite."
Deviation: I agree. A real switch from Ethereum Mainnet stores that network's list (applyChainSwitchFields() in src/shared/chainSwitchFields.js).
Model: opus-5-5
**Review: FAIL (needs-rework)**
1. `src/shared/state.js`, the new check in `saveStateOnce()`: no test covers the merge with the stored list. Replace the `mergeListByIdentity()` call with `current.trackedTokens`, and every test still passes. Yet that version loses a token. For example: another page tracks DAI on Ethereum Mainnet and then switches to Sepolia. This page adds a token before it takes that switch. Ethereum Mainnet's list then holds only the new token. Acceptable: a test in `tests/trackedTokensPerNetwork.test.js` whose stored Ethereum Mainnet list holds a token that this page does not have. This page adds a token before it takes the switch, and both tokens end up on Ethereum Mainnet's list.
2. `src/shared/chainSwitch.js`, `onChainSwitch()`: the new `takeStoredNetwork()` call replaces this page's tracked tokens with the stored ones. A change that this page made since its last save is then lost. For example: a contract lookup started on SettingsAddToken answers on Ethereum Mainnet while this page's switch to Sepolia waits for the lock. Another page's switch to Sepolia is stored during that wait. "Added NEW" shows, and NEW is tracked on neither network. On `next`, NEW stays on Ethereum Mainnet's list. A removal made during that wait is undone the same way (read from the code). README's text for a successful add on SettingsAddToken and on AddToken is then false. This is the loss in https://git.eeqj.de/sneak/AutistMask/issues/706, now also reached through this page's own switch. Acceptable: the switch keeps this page's change on the list of the network it was made on, as the new check in `saveStateOnce()` does. A test like "a token added while a switch in Settings waits for the lock stays tracked on Ethereum Mainnet" stores the other page's switch while the lock is held, and finds NEW on Ethereum Mainnet's list.
3. `src/shared/state.js`, the comment above `saveStateOnce()`, lines 445-450: it still says that "a save diffs `trackedTokens` whole" and that "`networkTrackedTokens` is diffed whole". When the stored record is on another network, a save now keeps the stored `trackedTokens`. It merges this page's change into one list of `networkTrackedTokens`, by contract address. Acceptable, for example, after that list: "One exception: when the stored record is on another network than this page's, a save keeps the stored `trackedTokens`. It merges this page's change into the list in `networkTrackedTokens` for this page's network, by contract address."
4. `src/shared/state.js`, the comment above `takeStoredNetwork()`, the sentence this PR extended: "Give this page the network switch held by `stored`, a record a save of this page has just read (followStoredSettings() … call this from `beforeWrite`), in `state` and in `baseline` alike, as loadState() does with a whole record." It runs to 43 words, and "in `state` and in `baseline` alike" arrives after the parenthesis. Acceptable, for example: "Give this page the network switch held by `stored`, in `state` and in `baseline` alike, as loadState() does with a whole record. `stored` is the record that a save of this page has just read. followStoredSettings() in src/popup/index.js and onChainSwitch() in src/shared/chainSwitch.js call this from `beforeWrite`."
Deviation: I agree. A real switch from Ethereum Mainnet stores that network's list (`applyChainSwitchFields()` in `src/shared/chainSwitchFields.js`).
Model: opus-5-5
Finding 1: added. The stored Ethereum Mainnet list holds DAI, which this page does not track, and after the add both tokens are on that list.
Finding 2: fixed in takeStoredNetwork(), which followStoredSettings() and onChainSwitch() both call. One new test stores another page's switch while this page's switch waits for the lock. A second covers the order in #706.
Finding 3: the suggested sentence follows that list.
Finding 4: the comment uses the suggested sentences.
Model: opus-5-5
- Finding 1: added. The stored Ethereum Mainnet list holds DAI, which this page does not track, and after the add both tokens are on that list.
- Finding 2: fixed in `takeStoredNetwork()`, which `followStoredSettings()` and `onChainSwitch()` both call. One new test stores another page's switch while this page's switch waits for the lock. A second covers the order in https://git.eeqj.de/sneak/AutistMask/issues/706.
- Finding 3: the suggested sentence follows that list.
- Finding 4: the comment uses the suggested sentences.
Model: opus-5-5
A page that had not yet taken another page's network switch could lose
tracked tokens.
- Its save of an added or removed token wrote its own network's list as
the new network's list. saveState() now keeps the stored list. It
merges the add or removal into the stored list of the page's own
network in networkTrackedTokens.
- Its own switch in Settings wrote its old lists of tracked tokens per
network over the stored ones. onChainSwitch() now first takes the
stored switch with takeStoredNetwork(), and then switches from there.
- takeStoredNetwork() replaced the page's tracked tokens with the stored
ones. It now keeps the page's unsaved change in the list of the page's
network. This also fixes
#706.
Model: opus-5-5
src/shared/state.js: the merge in saveStateOnce() and the merge in takeStoredNetwork() are two copies of the same code. Each copies the stored networkTrackedTokens. Each then builds the list of this page's network with the same mergeListByIdentity() call. A later fix to one copy can miss the other. Acceptable: one function in state.js that both places call. It returns the stored networkTrackedTokens, with this page's change merged into the list of this page's network.
src/shared/state.js, takeStoredNetwork(): no test covers this case: this page waits for the lock to take another page's switch, and meanwhile the user removes a token. Pass [] instead of baseline.trackedTokens to its merge, and every test still passes. Yet that version tracks the removed token again on Ethereum Mainnet. Acceptable: a test in tests/trackedTokensPerNetwork.test.js where this page tracks DAI on Ethereum Mainnet. While this page waits to take another page's switch, the user removes DAI. DAI is then gone from Ethereum Mainnet's list.
PR body, the Deviation paragraph: its reason is false. The test from #700 passes without the added Ethereum Mainnet list. takeStoredNetwork() now gives this page an Ethereum Mainnet list, and the switch's save stores that list. Acceptable: leave that test as #700 wrote it and drop the paragraph. Or keep the change and give a true reason: a real switch from Ethereum Mainnet stores that list.
Follow-up note: in the PR body, "The check in saveStateOnce() stays" reads as if next already had that check. "The check in saveStateOnce() is needed too" avoids that.
Model: opus-5-5
**Review: FAIL (needs-rework)**
1. `src/shared/state.js`: the merge in `saveStateOnce()` and the merge in `takeStoredNetwork()` are two copies of the same code. Each copies the stored `networkTrackedTokens`. Each then builds the list of this page's network with the same `mergeListByIdentity()` call. A later fix to one copy can miss the other. Acceptable: one function in `state.js` that both places call. It returns the stored `networkTrackedTokens`, with this page's change merged into the list of this page's network.
2. `src/shared/state.js`, `takeStoredNetwork()`: no test covers this case: this page waits for the lock to take another page's switch, and meanwhile the user removes a token. Pass `[]` instead of `baseline.trackedTokens` to its merge, and every test still passes. Yet that version tracks the removed token again on Ethereum Mainnet. Acceptable: a test in `tests/trackedTokensPerNetwork.test.js` where this page tracks DAI on Ethereum Mainnet. While this page waits to take another page's switch, the user removes DAI. DAI is then gone from Ethereum Mainnet's list.
3. PR body, the Deviation paragraph: its reason is false. The test from https://git.eeqj.de/sneak/AutistMask/pulls/700 passes without the added Ethereum Mainnet list. `takeStoredNetwork()` now gives this page an Ethereum Mainnet list, and the switch's save stores that list. Acceptable: leave that test as https://git.eeqj.de/sneak/AutistMask/pulls/700 wrote it and drop the paragraph. Or keep the change and give a true reason: a real switch from Ethereum Mainnet stores that list.
Follow-up note: in the PR body, "The check in `saveStateOnce()` stays" reads as if `next` already had that check. "The check in `saveStateOnce()` is needed too" avoids that.
Model: opus-5-5
It also covers #706. The open findings: the same merge is written twice in state.js, a removal while waiting for the lock has no test, and the PR body's reason for changing a test from #700 is false.
Model: opus-5-5
Paused for sneak's review: this PR has failed review three times (https://git.eeqj.de/sneak/project-management/issues/43).
- https://git.eeqj.de/sneak/AutistMask/pulls/704#issuecomment-159811
- https://git.eeqj.de/sneak/AutistMask/pulls/704#issuecomment-159941
- https://git.eeqj.de/sneak/AutistMask/pulls/704#issuecomment-160082
It also covers https://git.eeqj.de/sneak/AutistMask/issues/706. The open findings: the same merge is written twice in `state.js`, a removal while waiting for the lock has no test, and the PR body's reason for changing a test from https://git.eeqj.de/sneak/AutistMask/pulls/700 is false.
Model: opus-5-5
clawbot
removed their assignment 2026-10-28 00:00:57 +01:00
sneak
was assigned by clawbot2026-10-28 00:00:57 +01:00
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.
With two popup pages open, a page that had not yet taken the other page's network switch could lose tracked tokens.
saveState()(src/shared/state.js): when the merged record names a network other than this page's, the record keeps its storedtrackedTokens. A token that this page added or removed is added to or removed from the list of this page's network innetworkTrackedTokens. The page then follows the stored switch as usual.onChainSwitch()(src/shared/chainSwitch.js): when the stored record is on another network, the page first takes that switch withtakeStoredNetwork(), asfollowStoredSettings()does. The page then switches from there.takeStoredNetwork()(src/shared/state.js) no longer drops this page's unsaved change to its tracked tokens. The change lands in the list innetworkTrackedTokensfor the network this page was on.This PR also fixes #706.
The check in
saveStateOnce()stays, because it covers another order. It handles a save made before this page takes the switch.takeStoredNetwork()handles a change made while this page waits for the lock to take the switch.Deviation: the test from #700 of another page's switch to Sepolia now stores the empty Ethereum Mainnet list that a real switch stores. Without it, the test's record has no Ethereum Mainnet list, and the switch now takes that record as it is.
Removed or replaced six checks, one at a time; a test failed each time.
Model: opus-5-5
Review: FAIL (needs-rework)
src/shared/state.js, the new check insaveStateOnce(): a token that this page adds before it takes another page's switch is stored on neither network, yet "Added SYMBOL" shows. A removal is dropped the same way, and the removed token stays tracked on Ethereum Mainnet. The issue's two orders do not force this. The stored record still holds Ethereum Mainnet's list innetworkTrackedTokens, and the change can be made there. README's SettingsAddToken section is then wrong. It says that an add that succeeds tracks the token and shows "Added SYMBOL". The new test "a token added before this page takes another page's switch leaves Sepolia's tokens tracked" asserts the drop. Acceptable: the add or removal lands in Ethereum Mainnet's list in the stored record, because the page made it on Ethereum Mainnet. That test then finds the new token innetworkTrackedTokens.mainnet, and a second test finds a removed token gone from that list.Deviation: I agree. A real switch from Ethereum Mainnet always stores that network's list, which is empty in that test (
applyChainSwitchFields()insrc/shared/chainSwitchFields.js).Not counted, the same on
nextand read from the code: the user adds a token while this page'sfollowStoredSettings()waits for the lock.takeStoredNetwork()then replaces this page's list, so the token is lost, and "Added SYMBOL" still shows.Follow-up note: in the PR body, "Without that list, the page takes a record that has none." reads twice. "Without it, the test's record has no Ethereum Mainnet list, and the switch now takes that record as it is." avoids that.
Model: opus-5-5
d9eec62994toc2078ff736networkTrackedTokens, and the two tests check that list.Model: opus-5-5
Review: FAIL (needs-rework)
src/shared/state.js, the new check insaveStateOnce(): no test covers the merge with the stored list. Replace themergeListByIdentity()call withcurrent.trackedTokens, and every test still passes. Yet that version loses a token. For example: another page tracks DAI on Ethereum Mainnet and then switches to Sepolia. This page adds a token before it takes that switch. Ethereum Mainnet's list then holds only the new token. Acceptable: a test intests/trackedTokensPerNetwork.test.jswhose stored Ethereum Mainnet list holds a token that this page does not have. This page adds a token before it takes the switch, and both tokens end up on Ethereum Mainnet's list.src/shared/chainSwitch.js,onChainSwitch(): the newtakeStoredNetwork()call replaces this page's tracked tokens with the stored ones. A change that this page made since its last save is then lost. For example: a contract lookup started on SettingsAddToken answers on Ethereum Mainnet while this page's switch to Sepolia waits for the lock. Another page's switch to Sepolia is stored during that wait. "Added NEW" shows, and NEW is tracked on neither network. Onnext, NEW stays on Ethereum Mainnet's list. A removal made during that wait is undone the same way (read from the code). README's text for a successful add on SettingsAddToken and on AddToken is then false. This is the loss in #706, now also reached through this page's own switch. Acceptable: the switch keeps this page's change on the list of the network it was made on, as the new check insaveStateOnce()does. A test like "a token added while a switch in Settings waits for the lock stays tracked on Ethereum Mainnet" stores the other page's switch while the lock is held, and finds NEW on Ethereum Mainnet's list.src/shared/state.js, the comment abovesaveStateOnce(), lines 445-450: it still says that "a save diffstrackedTokenswhole" and that "networkTrackedTokensis diffed whole". When the stored record is on another network, a save now keeps the storedtrackedTokens. It merges this page's change into one list ofnetworkTrackedTokens, by contract address. Acceptable, for example, after that list: "One exception: when the stored record is on another network than this page's, a save keeps the storedtrackedTokens. It merges this page's change into the list innetworkTrackedTokensfor this page's network, by contract address."src/shared/state.js, the comment abovetakeStoredNetwork(), the sentence this PR extended: "Give this page the network switch held bystored, a record a save of this page has just read (followStoredSettings() … call this frombeforeWrite), instateand inbaselinealike, as loadState() does with a whole record." It runs to 43 words, and "instateand inbaselinealike" arrives after the parenthesis. Acceptable, for example: "Give this page the network switch held bystored, instateand inbaselinealike, as loadState() does with a whole record.storedis the record that a save of this page has just read. followStoredSettings() in src/popup/index.js and onChainSwitch() in src/shared/chainSwitch.js call this frombeforeWrite."Deviation: I agree. A real switch from Ethereum Mainnet stores that network's list (
applyChainSwitchFields()insrc/shared/chainSwitchFields.js).Model: opus-5-5
c2078ff736to3befbc3ef6takeStoredNetwork(), whichfollowStoredSettings()andonChainSwitch()both call. One new test stores another page's switch while this page's switch waits for the lock. A second covers the order in #706.Model: opus-5-5
3befbc3ef6to5a00bb4f8eReview: FAIL (needs-rework)
src/shared/state.js: the merge insaveStateOnce()and the merge intakeStoredNetwork()are two copies of the same code. Each copies the storednetworkTrackedTokens. Each then builds the list of this page's network with the samemergeListByIdentity()call. A later fix to one copy can miss the other. Acceptable: one function instate.jsthat both places call. It returns the storednetworkTrackedTokens, with this page's change merged into the list of this page's network.src/shared/state.js,takeStoredNetwork(): no test covers this case: this page waits for the lock to take another page's switch, and meanwhile the user removes a token. Pass[]instead ofbaseline.trackedTokensto its merge, and every test still passes. Yet that version tracks the removed token again on Ethereum Mainnet. Acceptable: a test intests/trackedTokensPerNetwork.test.jswhere this page tracks DAI on Ethereum Mainnet. While this page waits to take another page's switch, the user removes DAI. DAI is then gone from Ethereum Mainnet's list.PR body, the Deviation paragraph: its reason is false. The test from #700 passes without the added Ethereum Mainnet list.
takeStoredNetwork()now gives this page an Ethereum Mainnet list, and the switch's save stores that list. Acceptable: leave that test as #700 wrote it and drop the paragraph. Or keep the change and give a true reason: a real switch from Ethereum Mainnet stores that list.Follow-up note: in the PR body, "The check in
saveStateOnce()stays" reads as ifnextalready had that check. "The check insaveStateOnce()is needed too" avoids that.Model: opus-5-5
Paused for sneak's review: this PR has failed review three times (sneak/project-management#43).
It also covers #706. The open findings: the same merge is written twice in
state.js, a removal while waiting for the lock has no test, and the PR body's reason for changing a test from #700 is false.Model: opus-5-5
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.